fix: update LedgerResponseExpanded type for ledger RPC in APIv2#3209
Open
slurpyone wants to merge 1 commit intoXRPLF:mainfrom
Open
fix: update LedgerResponseExpanded type for ledger RPC in APIv2#3209slurpyone wants to merge 1 commit intoXRPLF:mainfrom
slurpyone wants to merge 1 commit intoXRPLF:mainfrom
Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughThis pull request refactors ledger transaction types to distinguish between API v1 and v2 response formats. New types Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/xrpl/src/models/ledger/Ledger.ts`:
- Around line 94-101: The JSDoc for Ledger/LedgerV1 claims transactions may be
hash strings or expanded objects, but the type for transactions is only
Array<LedgerTransactionExpanded> (and LedgerTransactionExpandedV1), causing a
mismatch; either remove the hash-string wording from the comment or widen the
type to reflect both shapes. Fix by updating the transactions property on the
Ledger and LedgerV1 models: if responses can be non-expanded, change the type to
Array<string | LedgerTransactionExpanded> (and Array<string |
LedgerTransactionExpandedV1> for V1); otherwise, simplify the JSDoc to state
transactions are always expanded and remove reference to hash strings. Ensure
you update both Ledger.transactions and LedgerV1.transactions and their JSDoc to
stay consistent.
In `@packages/xrpl/src/utils/hashes/hashLedger.ts`:
- Around line 183-185: computeTransactionHash is passing APIv1 expanded
transaction wrappers directly into hashTxTree, causing wrong encoding and hash
mismatches; before calling hashTxTree (where transactionHash is computed),
detect LedgerTransactionExpandedV1 wrappers (objects containing tx_json and
meta) and normalize them to the v2 flat shape expected by hashTxTree by mapping
each item to {tx: tx_json, metaData: meta, hash: hash, ...} (preserving other
fields) so encode(tx) and metadata access work correctly; update the code that
constructs transactionHash (the call to hashTxTree) to pass the normalized array
instead of casting ledger.transactions to LedgerTransactionExpanded[]. Ensure
LedgerTransactionExpandedV1 is imported if not already.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/xrpl/test/utils/hashLedger.test.ts (1)
160-222: Type-shape tests are nested inside thehashLedgerdescribe blockThe new
'LedgerTransactionExpanded types'describe block testsLedger/LedgerV1type assignability, which is unrelated to the hash-computation behavior exercised by the surroundinghashLedgertests. Consider moving these to a dedicatedpackages/xrpl/test/models/Ledger.test.ts(or similar) file to keep hashing tests cohesive and type-model tests discoverable alongside other model tests.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/xrpl/test/utils/hashLedger.test.ts` around lines 160 - 222, The type-shape tests for LedgerTransactionExpanded and LedgerTransactionExpandedV1 are misplaced inside the hashLedger suite; move the entire describe block titled 'LedgerTransactionExpanded types' out of the hashLedger test file into a new dedicated model test file (e.g., a Ledger model test) so they sit alongside other model/type tests; ensure the new file imports the same types (LedgerTransactionExpanded, LedgerTransactionExpandedV1, Ledger, LedgerV1) and keep the assertions unchanged so Ledger/ LedgerV1 assignability checks continue to pass.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/xrpl/test/utils/hashLedger.test.ts`:
- Around line 162-176: The Payment test fixtures in hashLedger.test.ts use a
LedgerTransactionExpanded object (variable tx) but omit required Payment fields
(Amount and Destination) causing type errors; either add realistic Amount and
Destination fields to the tx object (e.g., populate Amount and Destination
consistent with a Payment) or apply a type assertion like "as any" to the
fixture object to bypass the discriminated-union check so the test compiles;
locate the LedgerTransactionExpanded tx definitions near the failing test and
update those object literals accordingly.
---
Nitpick comments:
In `@packages/xrpl/test/utils/hashLedger.test.ts`:
- Around line 160-222: The type-shape tests for LedgerTransactionExpanded and
LedgerTransactionExpandedV1 are misplaced inside the hashLedger suite; move the
entire describe block titled 'LedgerTransactionExpanded types' out of the
hashLedger test file into a new dedicated model test file (e.g., a Ledger model
test) so they sit alongside other model/type tests; ensure the new file imports
the same types (LedgerTransactionExpanded, LedgerTransactionExpandedV1, Ledger,
LedgerV1) and keep the assertions unchanged so Ledger/ LedgerV1 assignability
checks continue to pass.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/xrpl/test/models/LedgerTransactionExpanded.test.ts`:
- Around line 12-26: The Payment fixtures for LedgerTransactionExpanded (the
objects named tx and the second fixture) are missing required Payment fields;
add an Amount (e.g., a numeric string like '1000' for drops or an IssuedCurrency
object) and a Destination (a valid XRPL address string) to each Payment
transaction fixture so they satisfy the Payment interface; update the objects
used in the tests (LedgerTransactionExpanded tx and the other Payment fixture)
to include these properties.
---
Duplicate comments:
In `@packages/xrpl/src/models/ledger/Ledger.ts`:
- Around line 94-99: The JSDoc/type mismatch for the Ledger.transactions
property was fixed for the first occurrence by scoping the comment to the
expanded shape (transactions?: Array<LedgerTransactionExpanded>) — apply the
same fix to the other occurrence around lines 115-120: locate the second JSDoc
block that documents the non-expanded/alternate transactions representation and
update or split the comment so it only describes the expanded case (referencing
LedgerTransactionExpanded), or add a separate accurate JSDoc for the
non-expanded type to match the actual type declaration.
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
packages/xrpl/src/utils/hashes/hashLedger.ts (2)
184-209: Indentation misalignment on line 184.Line 184 has 4-space indentation while the surrounding code (lines 185–209) uses 2-space indentation. This appears to be an accidental extra indent on the first comment line.
Proposed fix
- // Normalize transactions to the v2 flat format expected by hashTxTree. + // Normalize transactions to the v2 flat format expected by hashTxTree.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/xrpl/src/utils/hashes/hashLedger.ts` around lines 184 - 209, The leading comment line before the normalization block is over-indented; align it with the surrounding two-space indentation to match the rest of the block. Locate the comment and the normalization code around the normalizedTransactions declaration and adjust the indentation so the comment and the following lines (the .filter/.map chain and the hashTxTree(normalizedTransactions) call) use the same two-space indentation level as the rest of the function (symbols to find: normalizedTransactions, LedgerTransactionExpanded, LedgerTransactionExpandedV1, hashTxTree). Ensure no other lines in that block keep the four-space indent.
187-209: Normalization logic for v1 → v2 transactions is well-structured.The approach of filtering out hash strings, detecting v1 wrappers via
'tx_json' in tx, and then mapping to the flat v2 shape usingObject.assignis correct and handles both API versions cleanly.One consideration: if
ledger.transactionscontains only hash strings (non-expanded), all entries get filtered out, producing an empty SHAMap that will mismatchtransaction_hashat line 211. The resultingValidationError("does not match computed hash") is technically correct but could be confusing. A guard after filtering that checksnormalizedTransactions.length === 0and throws a more descriptive error (e.g., "expanded transactions required to compute tree hash") would improve debuggability. Not a blocking issue though — the current behavior is safe.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/xrpl/src/utils/hashes/hashLedger.ts` around lines 187 - 209, After filtering ledger.transactions into normalizedTransactions (the array computed before calling hashTxTree), add a guard that checks if normalizedTransactions.length === 0 and throw a descriptive error (e.g., "expanded transactions required to compute tree hash" or a ValidationError) to avoid computing a SHAMap from an empty set; this change should be made around the normalizedTransactions creation site and before the call to hashTxTree(normalizedTransactions) so callers get a clear, actionable error instead of the later "does not match computed hash" ValidationError.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@packages/xrpl/src/utils/hashes/hashLedger.ts`:
- Around line 184-209: The leading comment line before the normalization block
is over-indented; align it with the surrounding two-space indentation to match
the rest of the block. Locate the comment and the normalization code around the
normalizedTransactions declaration and adjust the indentation so the comment and
the following lines (the .filter/.map chain and the
hashTxTree(normalizedTransactions) call) use the same two-space indentation
level as the rest of the function (symbols to find: normalizedTransactions,
LedgerTransactionExpanded, LedgerTransactionExpandedV1, hashTxTree). Ensure no
other lines in that block keep the four-space indent.
- Around line 187-209: After filtering ledger.transactions into
normalizedTransactions (the array computed before calling hashTxTree), add a
guard that checks if normalizedTransactions.length === 0 and throw a descriptive
error (e.g., "expanded transactions required to compute tree hash" or a
ValidationError) to avoid computing a SHAMap from an empty set; this change
should be made around the normalizedTransactions creation site and before the
call to hashTxTree(normalizedTransactions) so callers get a clear, actionable
error instead of the later "does not match computed hash" ValidationError.
… hashTxTree - Export LedgerTransactionExpanded and LedgerTransactionExpandedV1 types from Ledger.ts - Move transactions field from BaseLedger to Ledger/LedgerV1 with version-specific types - Normalize v1 wrapped transactions in hashTxTree for cross-version compatibility - Add tests for LedgerTransactionExpanded type handling - Fix lint errors (prettier, max-lines-per-function, type assertions)
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/xrpl/src/utils/hashes/hashLedger.ts (1)
194-206: The v1/v2 normalization addresses the prior review concern correctly.The cast-to-union → filter → normalize pipeline properly handles both API versions. One minor observation:
Consider throwing early when string (non-expanded) transactions are encountered.
If
ledger.transactionscontains hash strings (non-expanded), they are silently discarded, and the resulting tree hash will inevitably mismatchtransaction_hash, producing a confusingValidationErrorat line 208. An explicit early check would give a much clearer error:💡 Optional: clearer error for non-expanded transactions
const txs = ledger.transactions as Array< string | LedgerTransactionExpanded | LedgerTransactionExpandedV1 > + if (txs.some((tx) => typeof tx === 'string')) { + throw new ValidationError( + 'transactions must be expanded (not hashes) to compute the tree hash', + ) + } const normalizedTransactions = txs .filter( (tx): tx is LedgerTransactionExpanded | LedgerTransactionExpandedV1 => typeof tx !== 'string', ) .map(normalizeToV2)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/xrpl/src/utils/hashes/hashLedger.ts` around lines 194 - 206, Detect and fail fast if ledger.transactions contains non-expanded transaction hashes: before mapping to normalizedTransactions (where ledger.transactions is cast to txs and then filtered/mapped), check txs for any typeof === 'string' and throw a clear error (e.g., include ledger.transaction_hash and a message that expanded transactions are required) so that hashTxTree and subsequent ValidationError mismatches are avoided; update the function containing this logic (the code around txs / normalizedTransactions / hashTxTree) to perform this early check and throw a descriptive error when string entries are present.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@packages/xrpl/src/utils/hashes/hashLedger.ts`:
- Around line 194-206: Detect and fail fast if ledger.transactions contains
non-expanded transaction hashes: before mapping to normalizedTransactions (where
ledger.transactions is cast to txs and then filtered/mapped), check txs for any
typeof === 'string' and throw a clear error (e.g., include
ledger.transaction_hash and a message that expanded transactions are required)
so that hashTxTree and subsequent ValidationError mismatches are avoided; update
the function containing this logic (the code around txs / normalizedTransactions
/ hashTxTree) to perform this early check and throw a descriptive error when
string entries are present.
slurpyone
force-pushed
the
fix/ledger-response-expanded-apiv2
branch
from
485e8dc to
2687d1e
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fixes #2886
The
LedgerResponseExpandedtype did not correctly reflect the structural differences between APIv1 and APIv2 responses for theledgerRPC whentransactionsandexpandare enabled.Problem
The
transactionsarray in expanded ledger responses has different shapes per API version:APIv1 wraps each transaction:
{ "tx_json": { ... }, "meta": { ... }, "hash": "...", "validated": true, "ledger_index": 93637993, "close_time_iso": "...", "ledger_hash": "..." }APIv2 returns flat transaction objects:
{ "Account": "...", "TransactionType": "Payment", "hash": "...", "metaData": { ... } }Previously,
BaseLedgerdefined a singletransactionstype matching only the v2 shape.Changes
LedgerTransactionExpandedtype for APIv2 (flat transaction +hash+metaData)LedgerTransactionExpandedV1type for APIv1 (wrapped intx_json,meta, etc.)transactionsproperty fromBaseLedgertoLedger(v2) andLedgerV1(v1) with the correct type for eachhashTxTreeto use the newLedgerTransactionExpandedtype