Skip to content

fix: TradeResponse class#108

Merged
jopmiddelkamp merged 3 commits intomasterfrom
feature/fix-trade-response
Dec 8, 2025
Merged

fix: TradeResponse class#108
jopmiddelkamp merged 3 commits intomasterfrom
feature/fix-trade-response

Conversation

@cuongph87
Copy link
Copy Markdown
Contributor

@cuongph87 cuongph87 commented Dec 2, 2025

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • I have read the CONTRIBUTING document.
  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have added tests to cover my changes.
  • All new and existing tests passed.

Note

Refactors TradeResponse to properly model orderbook and liquidity pool trades (required/nullable fields, links, assets), and updates tests and fixtures to validate both trade types.

  • Responses:
    • Refactor TradeResponse:
      • Add trade_type, liquidity_pool_fee_bp, base_liquidity_pool_id, counter_liquidity_pool_id.
      • Make always-present fields required; adjust others to nullable.
      • Fix CounterAsset property (was CountAsset).
      • Change _links.base/_links.counter to non-generic Link.
      • Remove unused OfferId field; keep base_offer_id/counter_offer_id.
  • Tests:
    • Replace tradePage.json with tradePageOrderBook.json and add tradePageLiquidityPool.json.
    • Update TradesRequestBuilderTest.TestTradesExecute to use orderbook fixture and new assertions.
    • Expand TradesPageDeserializerTest with serialize/deserialize tests and assertions for both orderbook and liquidity pool trades.
  • Project:
    • Update .csproj to include new test data files and remove the old trade page fixture.

Written by Cursor Bugbot for commit 5a5b79c. Configure here.

@jopmiddelkamp jopmiddelkamp requested a review from Copilot December 2, 2025 16:26
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR fixes issues in the TradeResponse class related to incorrect property nullability and adds comprehensive test coverage for both orderbook and liquidity pool trade types. The changes mark always-present properties as required, make conditionally-present properties nullable, and update the class to be sealed. Additionally, it corrects a previously misspelled property name (CountAssetCounterAsset).

Key Changes

  • Added required modifier to properties that are always present in trade responses (e.g., Id, PagingToken, TradeType)
  • Made conditionally-present properties nullable (e.g., BaseAccount, LiquidityPoolFeeBp, BaseOfferId)
  • Fixed typo: renamed CountAsset to CounterAsset

Reviewed changes

Copilot reviewed 7 out of 7 changed files in this pull request and generated 2 comments.

Show a summary per file
File Description
StellarDotnetSdk/Responses/TradeResponse.cs Updated property nullability with required and nullable modifiers, added comprehensive XML documentation, fixed property name typo, and changed to sealed class
StellarDotnetSdk.Tests/TestData/Responses/tradePageOrderBook.json New test data file containing orderbook trade example
StellarDotnetSdk.Tests/TestData/Responses/tradePageLiquidityPool.json New test data file containing liquidity pool trade examples
StellarDotnetSdk.Tests/TestData/Responses/tradePage.json Removed outdated test data file
StellarDotnetSdk.Tests/StellarDotnetSdk.Tests.csproj Updated test data file references
StellarDotnetSdk.Tests/Responses/TradesPageDeserializerTest.cs Replaced single test with separate tests for orderbook and liquidity pool trades, with expanded assertions
StellarDotnetSdk.Tests/Requests/TradesRequestBuilderTest.cs Updated to use new test data file and assertion method

Comment thread StellarDotnetSdk/Responses/TradeResponse.cs Outdated
Comment thread StellarDotnetSdk/Responses/TradeResponse.cs Outdated
@jopmiddelkamp
Copy link
Copy Markdown
Contributor

bugbot run

Copy link
Copy Markdown

@cursor cursor Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ Bugbot reviewed your changes and found no bugs!


@cuongph87 cuongph87 force-pushed the feature/fix-trade-response branch from 7c90a44 to 9233d80 Compare December 4, 2025 04:19
Comment thread StellarDotnetSdk/Responses/TradeResponse.cs Outdated
Comment thread StellarDotnetSdk/Responses/TradeResponse.cs
Comment thread StellarDotnetSdk/Responses/TradeResponse.cs
Comment thread StellarDotnetSdk/Responses/TradeResponse.cs
Comment thread StellarDotnetSdk/Responses/TradeResponse.cs
jopmiddelkamp
jopmiddelkamp previously approved these changes Dec 8, 2025
@cuongph87 cuongph87 force-pushed the feature/fix-trade-response branch 3 times, most recently from 219d03d to 2450b6b Compare December 8, 2025 03:21
jopmiddelkamp
jopmiddelkamp previously approved these changes Dec 8, 2025
@jopmiddelkamp jopmiddelkamp merged commit 718937c into master Dec 8, 2025
1 check passed
@jopmiddelkamp jopmiddelkamp deleted the feature/fix-trade-response branch December 8, 2025 07:00
@cuongph87 cuongph87 added the fix label Apr 9, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Fix TradeResponse class

3 participants