fix: add missing contract_credited/contract_debited response handling#173
fix: add missing contract_credited/contract_debited response handling#173
contract_credited/contract_debited response handling#173Conversation
Code reviewNo issues found. Checked for bugs and CLAUDE.md compliance. |
There was a problem hiding this comment.
Pull request overview
Wires up missing Horizon effect types so contract_credited / contract_debited effects no longer fail polymorphic deserialization (addresses #172).
Changes:
- Extend
EffectResponseJsonConverterto dispatchtype_i=96andtype_i=97to the existing Soroban SAC effect response classes. - Update the converter’s “unknown type” error message to include the new supported range.
- Add MSTest coverage for deserializing both new effect types.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
StellarDotnetSdk/Converters/EffectResponseJsonConverter.cs |
Adds type_i switch cases for 96/97 and updates the supported-range message. |
StellarDotnetSdk.Tests/Converters/EffectResponseJsonConverterTest.cs |
Adds tests validating dispatch/binding for contract_credited and contract_debited effects. |
| /// <see cref="ContractCreditedEffectResponse" /> and that every wire-format field is | ||
| /// bound to the expected property. |
There was a problem hiding this comment.
The XML doc for this new test claims "every wire-format field" is verified, but the assertions only cover the subtype dispatch and a subset of fields (TypeId/Type/Contract/Amount/Asset*). Consider either asserting the other common fields (e.g., Id, PagingToken, Account, CreatedAt, Links) or tightening the doc comment to match what the test actually verifies.
| /// <see cref="ContractCreditedEffectResponse" /> and that every wire-format field is | |
| /// bound to the expected property. | |
| /// <see cref="ContractCreditedEffectResponse" /> and that the contract, amount, and | |
| /// asset-related fields asserted below are bound to the expected properties. |
There was a problem hiding this comment.
Added assertions.
| /// <summary> | ||
| /// Tests deserialization of a contract_debited effect with type_i=97. | ||
| /// Payload shape constructed from the same wire format Horizon emits for the symmetric | ||
| /// SAC transfer debit, with a credit_alphanum4 asset to exercise the non-native branch | ||
| /// of <see cref="ContractDebitedEffectResponse.Asset" />. | ||
| /// </summary> |
There was a problem hiding this comment.
This test's summary says the credit_alphanum4 payload is used to exercise the non-native branch of ContractDebitedEffectResponse.Asset, but the test never accesses or asserts the computed Asset property. If the intent is to cover that branch, add an assertion against debited.Asset (type/code/issuer) or update the comment to avoid implying coverage that isn't present.
There was a problem hiding this comment.
Added the assertion.
| 96 => root.Deserialize<ContractCreditedEffectResponse>(options), | ||
| 97 => root.Deserialize<ContractDebitedEffectResponse>(options), |
There was a problem hiding this comment.
The PR description/checkboxes state that documentation changes were required and already updated, but this PR only changes the converter and its tests (no docs/README updates). Either include the intended documentation updates or adjust the PR description/checkboxes to avoid a mismatch for reviewers and release notes.
Auto-generated Summary 🤖
This addresses #172.
Types of changes