Skip to content

fix!: ClaimableBalanceResponse class#102

Merged
cuongph87 merged 5 commits intomasterfrom
feature/fix-claimable-balance-response
Dec 4, 2025
Merged

fix!: ClaimableBalanceResponse class#102
cuongph87 merged 5 commits intomasterfrom
feature/fix-claimable-balance-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

Replaces flat Predicate with a polymorphic model + JSON converter and expands ClaimableBalanceResponse (required fields, links, flags), updating tests and fixtures accordingly.

  • Responses:
    • ClaimableBalanceResponse: Sealed; adds required on core fields; introduces last_modified_time, flags.clawback_enabled, and _links (self, operations, transactions).
    • Predicates (polymorphic): Replace flat Responses.Predicate with Responses.Predicates hierarchy (PredicateAnd, PredicateOr, PredicateNot, PredicateUnconditional, PredicateBeforeAbsoluteTime, PredicateBeforeRelativeTime) and a custom PredicateJsonConverter for polymorphic (de)serialization and numeric-as-string handling.
    • Responses.Claimant: Uses new Predicates.Predicate.
    • Docs: Add XML summaries to Claimants.ClaimPredicate and Claimants.Claimant.
  • Tests & Fixtures:
    • Update claimable balance JSON and assertions to include new fields/links/flags and complex predicate shapes; fix rel_before key naming; add coverage for epoch/string values and polymorphic predicate (de)serialization.
    • Adjust create-claimable-balance operation tests to assert PredicateBeforeAbsoluteTime.

Written by Cursor Bugbot for commit a880f3a. Configure here.

@cuongph87 cuongph87 changed the title fix: ClaimableBalanceResponse class fix!: ClaimableBalanceResponse class Dec 2, 2025
@jopmiddelkamp jopmiddelkamp requested a review from Copilot December 2, 2025 16:27
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 the ClaimableBalanceResponse class to properly represent the Horizon API response structure for claimable balances. The changes address missing properties, incorrect nullability, and add comprehensive documentation.

Key changes:

  • Added required modifier to always-present properties in ClaimableBalanceResponse and related classes
  • Added missing Flags, Links, and LastModifiedTime properties to match Horizon API responses
  • Enhanced Predicate class with AbsBeforeEpoch property and improved documentation
  • Updated test data and assertions to reflect actual Horizon API responses

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.

File Description
StellarDotnetSdk/Responses/ClaimableBalanceResponse.cs Added missing properties (Flags, Links, LastModifiedTime), marked always-present properties as required, added comprehensive XML documentation, and removed file-level nullable disable directive
StellarDotnetSdk/Responses/Predicate.cs Added missing AbsBeforeEpoch property, changed Unconditional to nullable, added XML documentation, made class sealed, and improved exception handling
StellarDotnetSdk.Tests/TestData/Responses/claimableBalance.json Updated test data to reflect actual Horizon API response including flags, last_modified_time, and additional claimant with nested predicate
StellarDotnetSdk.Tests/Responses/ClaimableBalanceDeserializerTest.cs Updated test assertions to validate new properties and more comprehensive test data structure
Comments suppressed due to low confidence (1)

StellarDotnetSdk/Responses/Predicate.cs:102

  • The logic prioritizes AbsBeforeEpoch over AbsBefore, but both represent the same deadline. According to the documentation (lines 40-49), AbsBeforeEpoch is described as 'representing the same deadline date as abs_before.' Consider documenting why AbsBeforeEpoch takes precedence, or verify that they should be mutually exclusive. If Horizon returns both fields, there should be clarity on which is the source of truth.
        if (AbsBeforeEpoch != null)
        {
            var dateTime = DateTimeOffset.FromUnixTimeSeconds(AbsBeforeEpoch.Value);
            return claimant_ClaimPredicate.BeforeAbsoluteTime(dateTime);
        }
        
        if (AbsBefore != null)
        {
            var dateTime = DateTimeOffset.Parse(AbsBefore);
            return claimant_ClaimPredicate.BeforeAbsoluteTime(dateTime);
        }

Comment thread StellarDotnetSdk.Tests/TestData/Responses/claimableBalance.json 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!


Comment thread StellarDotnetSdk/Responses/Predicate.cs Outdated
@cuongph87 cuongph87 force-pushed the feature/fix-claimable-balance-response branch from 3135956 to 923bb5e Compare December 4, 2025 03:57
@jopmiddelkamp jopmiddelkamp requested a review from Copilot December 4, 2025 05:39
@jopmiddelkamp
Copy link
Copy Markdown
Contributor

bugbot run

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

Copilot reviewed 17 out of 17 changed files in this pull request and generated 1 comment.

Comment thread StellarDotnetSdk/Responses/ClaimableBalanceResponse.cs
@jopmiddelkamp jopmiddelkamp self-requested a review December 4, 2025 05:40
jopmiddelkamp
jopmiddelkamp previously approved these changes Dec 4, 2025
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!


jopmiddelkamp
jopmiddelkamp previously approved these changes Dec 4, 2025
@cuongph87 cuongph87 force-pushed the feature/fix-claimable-balance-response branch from 64f5cef to 6fd5705 Compare December 4, 2025 06:11
@cuongph87 cuongph87 force-pushed the feature/fix-claimable-balance-response branch from 6fd5705 to 7619be6 Compare December 4, 2025 06:21
@cuongph87 cuongph87 merged commit 01cc654 into master Dec 4, 2025
1 check failed
@cuongph87 cuongph87 deleted the feature/fix-claimable-balance-response branch December 4, 2025 06:24
@cuongph87 cuongph87 added the breaking Breaking change label Apr 9, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

breaking Breaking change

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Fix ClaimableBalanceResponse class

3 participants