Rewrite SEP-0011 TxRep to use generated XDR serialization#134
Rewrite SEP-0011 TxRep to use generated XDR serialization#134christian-rogobete merged 8 commits intomasterfrom
Conversation
… methods Replace the ~4000-line hand-written TxRep implementation with a ~370-line version that delegates serialization to code-generated toTxRep()/fromTxRep() methods on XDR types. The top-level envelope structure, memo text encoding, and signatures are still handled manually for correctness. Key changes: - Add toTxRep()/fromTxRep() methods to all generated XDR types - Add TxRepHelper utility class for shared parsing/formatting - Add compact format support for XdrAsset, XdrAccountID, XdrMuxedAccount, XdrSignerKey, XdrAllowTrustOpAsset via TXREP_COMPACT_TYPES - Add hybrid compact/expanded format for XdrChangeTrustAsset and XdrTrustlineAsset (compact for non-pool-share, expanded for pool share) - Fix memo text round-trip: use json.decode() to reverse JsonEncoder escapes - Add backward compatibility for legacy tx.timeBounds._present format - Update test expectations for stroops-based amount representation - Remove dead code: formatAmount, parseAmount, dateComment, octal parsing, formatAssetCode, parseAssetCode, unused toTxRep/fromTxRep on XdrAllowTrustOpAsset Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Run dart format on all generated XDR files to match CI expectations. Fix generator to only emit 'import txrep_helper.dart' when the type actually references TxRepHelper methods, eliminating 111 unused import warnings. Update snapshots. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Remove redundant '!' operator on optional list variable that was just assigned an empty list, fixing an unnecessary_non_null_assertion warning that caused CI analyze step to fail. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #134 +/- ##
==========================================
+ Coverage 86.21% 88.98% +2.77%
==========================================
Files 622 623 +1
Lines 27746 28711 +965
==========================================
+ Hits 23920 25549 +1629
+ Misses 3826 3162 -664
🚀 New features to boost your workflow:
|
…p roundtrip tests Only generate toTxRep()/fromTxRep() for 143 types reachable from TransactionEnvelope, restoring test coverage from 69% back to 87%. - Add TXREP_TYPES frozen set (txrep_types.rb) gating TxRep generation - Fix simple typedef fromTxRep for named XDR inner types (was incorrectly calling getValue for types that emit sub-keys like unions/structs) - Add 349 TxRep roundtrip tests to the test generator (859 total) - Add parseTxRepLines helper with malformed-line detection - Remove dead XdrSCSpecTypeDef.fromTxRep (never called) - Update snapshots Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Fix TxRep string interpolation for primitive types: `$_field!` produced
`true!`/`false!` instead of `true`/`false` — changed to `${_field!}`.
Expand generated TxRep tests to cover all enum members and union arms
(was testing only one each). Add fallback values for deep struct types
(XdrFeeBumpTransaction, XdrTransaction, etc.) to enable tests that
previously exceeded MAX_DEPTH. Add dedicated TxRepHelper unit tests
(93 tests) covering parse, escape/unescape, hex, asset formatting,
signer keys, and all edge cases.
Generated tests: 510 → 1101. Total unit tests: 5706 → 6041.
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Validate TxRep type field: reject null or unrecognized envelope types - Validate feeBump.tx.innerTx.type during decode - Replace JsonEncoder() allocation with jsonEncode() top-level function - Use jsonDecode() instead of json.decode() for consistency - Throw explicit error for invalid 64-char hex in pool share parsing - Remove always-true hash.length == 32 check - Add doc comment on _decodeSignatures empty-list fallback Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
@claude review this PR. It rewrites SEP-0011 TxRep to delegate to generated Focus on these hand-written files:
Verify: (1) correct handling of all envelope types, memo encoding, and signatures, (2) compact vs expanded format logic for change trust and trustline assets, (3) backward compat with legacy Skip all other |
|
Claude finished @christian-rogobete's task in 7m 37s —— View job PR Review: SEP-0011 TxRep RewriteTasks:
Overall the refactoring is clean and well-structured. The generated-code delegation strategy works correctly, and the facade layer is easy to reason about. A few issues worth addressing are noted below. 1. Misleading comment on memo override
// Memo — handle manually to avoid double-quoting bug in generated code.The current generated The comment should be updated to explain this format-compatibility reason. Fix this → 2. Implicit null promotion on
|
| Area | Result |
|---|---|
| Envelope type handling (V0/V1/fee bump) | ✓ Correct |
| Memo encoding/decoding | ✓ Correct (but see item 1) |
| Compact vs expanded asset format | ✓ Correct |
Legacy timeBounds._present compat |
✓ Correct |
| Dead code / stale imports | ✓ Clean |
| String interpolation null assertions | ✓ Correct |
feeBumpEnv null consistency |
⚠ Minor (item 2) |
Exception type in TrustlineAsset |
⚠ Minor (item 3) |
| Comment accuracy on memo manual override | ⚠ Misleading (item 1) |
…otion - Update memo comment to explain jsonEncode/jsonDecode vs escapeString format incompatibility (not a double-quoting bug) - Use local variable for feeBumpEnv null promotion instead of relying on implicit flow promotion after bang operator Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
Thanks for the thorough review! Item 1 — Fixed in bebc2d6. Updated the comment to explain the actual reason: the facade uses Item 2 — Fixed in bebc2d6. Both Item 3 — Intentionally keeping |
|
@claude review this PR. It rewrites SEP-0011 TxRep to delegate to generated Focus on these hand-written files:
Verify: (1) correct handling of all envelope types, memo encoding, and signatures, (2) compact vs expanded format logic for change trust and trustline assets, (3) backward compat with legacy Skip all other |
|
Claude finished @christian-rogobete's task in 4m 13s —— View job PR Review: SEP-0011 TxRep Rewrite (updated state)Tasks:
The two issues from the prior review that were fixed in bebc2d6 are confirmed resolved. A few additional observations are noted below; none are blocking. Previously reported items — statusItem 1 (memo comment) — Fixed. Item 2 (feeBumpEnv null promotion) — Fixed. Both Item 3 (exception type in New observations1.
|
| Area | Result |
|---|---|
| Envelope type handling (V0→V1 conversion, V1, fee bump) | ✓ Correct |
V0 source-account promotion to XdrMuxedAccount |
✓ Correct |
Fee-bump inner tx assumed V1 (v1!) |
✓ Correct |
| Default envelope type throws with discriminant value | ✓ Correct |
| Memo NONE/TEXT/ID/HASH/RETURN — encode + decode symmetric | ✓ Correct |
jsonEncode/jsonDecode quoted-string + comment stripping interaction |
✓ Correct |
removeComment escape handling (\\, \") for JSON-encoded memo |
✓ Correct |
| Compact (single-line) vs expanded (multi-key) format for non-pool assets | ✓ Correct |
Pool-share XdrChangeTrustAsset expanded encode/decode round-trip |
✓ Correct |
Pool-share XdrTrustlineAsset expanded encode/decode round-trip |
✓ Correct |
Legacy tx.timeBounds._present backward compat |
✓ Correct |
_decodeSignatures returns empty list for unsigned transactions |
✓ Correct |
| Signatures encoded/decoded as lowercase hex | ✓ Correct |
| No dead code / stale imports in hand-written files | ✓ Clean |
${...} form used for all null-asserted string interpolation |
✓ Correct |
parseInt/parseBigInt used appropriately for 32-bit vs 64-bit fields |
✓ Correct |
Overall the rewrite is clean and the delegation strategy is sound. The two new observations above are informational and do not require changes before merging.
…t comments - Validate feeBump.tx.ext.v during decode, throw on unsupported values - Add test for unsupported feeBump.tx.ext.v - Clarify compact format comments in XdrTrustlineAsset and XdrChangeTrustAsset to note the path is for external interoperability Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
Claude finished @christian-rogobete's task in 2m 1s —— View job PR Review: Latest commit f0c2b6eTasks:
Both findings from the second review are confirmed resolved in f0c2b6e. Finding 1 ( String? fbExtV = TxRepHelper.getValue(map, 'feeBump.tx.ext.v');
if (fbExtV != null && fbExtV != '0') {
throw Exception('unsupported feeBump.tx.ext.v: $fbExtV');
}This correctly rejects any future non-zero ext version rather than silently ignoring it, which is better than the documentation-only fix I suggested. Finding 2 (compact pool-share decode unreachable from this encoder) — Resolved. // Compact format: native, CODE:ISSUER, or 64-char hex pool ID.
// This path is not produced by this library's encoder (which always
// uses expanded format for pool shares) but supports externally
// authored TxRep.This clearly documents the intent and removes the ambiguity noted in the prior review. VerdictAll findings across both review passes have been addressed. The codebase is clean:
The PR is ready to merge. Note: I'm unable to formally approve PRs through GitHub's review mechanism — you'll need to approve it manually. |
1 participant
Summary
toTxRep()/fromTxRep()methods on XDR typesTxRepHelperutility class and compact format support for asset types, accounts, and signer keystx.timeBounds._presentformatformatAmount,parseAmount,dateComment, octal parsing,formatAssetCode,parseAssetCode,txrep_legacy.dartTest plan
tx.timeBounds._presentformat parses correctly🤖 Generated with Claude Code