Fix serialization of Issue type when asset is MPT #3090
Conversation
WalkthroughAdds MPToken (MPT) serialization/deserialization and JSON handling in ripple-binary-codec, updates XRPL currency/type guards and Vault model/flags to accept MPT assets, adjusts tests/fixtures for MPT flows and ledger close-time usage, and updates HISTORY docs. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing touches🧪 Generate unit tests
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
✅ Files skipped from review due to trivial changes (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
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}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (7)
packages/xrpl/HISTORY.md (1)
7-9: Changelog looks good; add cross-reference for traceability.Consider linking issue #3083 and/or this PR (#3090) in the bullet for easy auditing.
packages/ripple-binary-codec/HISTORY.md (1)
5-7: Clarify naming and link the fix.Suggest “Fix Issue serialization/deserialization for MPT Issue” (or “MPT issuance”) instead of “MPTIssue”, and add a link to the PR/issue.
packages/ripple-binary-codec/src/types/st-object.ts (1)
155-155: Guard against undefined associatedType to preserve explicit errors.Directly calling
field.associatedType.from(...)can throw a generic TypeError if definitions are customized or a field lacks an associated type. Add an explicit check to keep error messages actionable.Apply this diff:
- : field.associatedType.from(xAddressDecoded[field.name]) + : (() => { + const assoc = field.associatedType as + | { from: (v: unknown) => unknown } + | undefined + if (!assoc || typeof assoc.from !== 'function') { + throw new TypeError( + `No associated type constructor for field ${field.name}`, + ) + } + return assoc.from(xAddressDecoded[field.name]) + })()packages/xrpl/test/integration/transactions/singleAssetVault.test.ts (2)
306-308: Use classicAddress for consistency.Other txs use
classicAddress. Keep consistent to avoid confusion across test code.- Account: issuerWallet.address, + Account: issuerWallet.classicAddress,
294-534: End-to-end MPT vault flow is comprehensive; consider small helpers.Optional: extract repeated “fetch vault and assert” blocks into helpers to reduce duplication and test time.
packages/ripple-binary-codec/src/types/issue.ts (2)
125-139: Endian conversions look correct for MPT; minor tidy possible.You read LE from the last 4 bytes and re‑emit BE into the mpt_issuance_id. This matches the stated fix. Consider using a small helper for symmetry (e.g., readUInt32LE) to avoid inline DataView usage.
-const sequence = new DataView(this.toBytes().slice(40).buffer).getUint32(0, true) +const tail4 = this.toBytes().slice(40, 44) +const sequence = new DataView(tail4.buffer).getUint32(0, true)
29-37: Type guard can throw on non-objects.isIssueObject assumes an object; Object.keys(null) will throw. Not critical but easy hardening.
-function isIssueObject(arg): arg is IssueObject { +function isIssueObject(arg: unknown): arg is IssueObject { + if (arg === null || typeof arg !== 'object') return false const keys = Object.keys(arg as Record<string, unknown>).sort() ...
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
packages/ripple-binary-codec/HISTORY.md(1 hunks)packages/ripple-binary-codec/src/types/issue.ts(4 hunks)packages/ripple-binary-codec/src/types/st-object.ts(1 hunks)packages/ripple-binary-codec/test/fixtures/codec-fixtures.json(47 hunks)packages/ripple-binary-codec/test/issue.test.ts(2 hunks)packages/xrpl/HISTORY.md(1 hunks)packages/xrpl/src/models/ledger/Vault.ts(2 hunks)packages/xrpl/src/models/transactions/common.ts(5 hunks)packages/xrpl/test/integration/transactions/singleAssetVault.test.ts(4 hunks)packages/xrpl/test/models/vaultCreate.test.ts(1 hunks)
🧰 Additional context used
🧠 Learnings (4)
📓 Common learnings
Learnt from: ckeshava
PR: XRPLF/xrpl.js#3081
File: packages/xrpl/src/models/transactions/MPTokenIssuanceCreate.ts:57-64
Timestamp: 2025-09-16T05:00:20.420Z
Learning: In XRPL's MPTokenIssuanceCreate flags, the tfMPTCanMutate* flags intentionally use the same bit values as their corresponding base flags (e.g., tfMPTCanMutateCanLock = 0x00000002 matches tfMPTCanLock = 0x00000002) because they operate in different contexts - base flags go in the Flags field while mutable flags go in the MutableFlags field. This design avoids ambiguity since the flags are applied to separate bit fields.
📚 Learning: 2024-12-06T18:44:55.095Z
Learnt from: shawnxie999
PR: XRPLF/xrpl.js#2661
File: packages/xrpl/test/models/MPTokenAuthorize.test.ts:60-71
Timestamp: 2024-12-06T18:44:55.095Z
Learning: In the XRPL.js library's TypeScript test file `packages/xrpl/test/models/MPTokenAuthorize.test.ts`, negative test cases for invalid `Account` address format, invalid `Holder` address format, invalid `MPTokenIssuanceID` format, and invalid flag combinations are not necessary.
Applied to files:
packages/xrpl/test/models/vaultCreate.test.tspackages/xrpl/test/integration/transactions/singleAssetVault.test.ts
📚 Learning: 2025-02-12T23:28:55.377Z
Learnt from: mvadari
PR: XRPLF/xrpl.js#2895
File: packages/xrpl/test/models/clawback.test.ts:0-0
Timestamp: 2025-02-12T23:28:55.377Z
Learning: The `validate` function in xrpl.js is synchronous and should be tested using `assert.doesNotThrow` rather than async assertions.
Applied to files:
packages/xrpl/test/models/vaultCreate.test.ts
📚 Learning: 2024-12-06T19:27:11.147Z
Learnt from: shawnxie999
PR: XRPLF/xrpl.js#2661
File: packages/xrpl/test/integration/transactions/clawback.test.ts:165-178
Timestamp: 2024-12-06T19:27:11.147Z
Learning: In the integration tests for `clawback.test.ts`, it's acceptable to use `ts-expect-error` to bypass type checking when verifying ledger entries, and no additional type safety improvements are needed.
Applied to files:
packages/xrpl/test/integration/transactions/singleAssetVault.test.tspackages/ripple-binary-codec/test/issue.test.ts
🧬 Code graph analysis (2)
packages/xrpl/test/integration/transactions/singleAssetVault.test.ts (10)
packages/xrpl/test/integration/utils.ts (2)
generateFundedWallet(182-186)testTransaction(240-290)packages/xrpl/src/models/transactions/MPTokenIssuanceCreate.ts (1)
MPTokenIssuanceCreateMetadata(125-127)packages/xrpl/src/models/transactions/vaultCreate.ts (1)
VaultCreate(55-93)packages/xrpl/src/models/ledger/Vault.ts (1)
Vault(11-83)packages/xrpl/src/models/common/index.ts (1)
MPTCurrency(17-19)packages/xrpl/src/models/transactions/vaultSet.ts (1)
VaultSet(20-43)packages/xrpl/src/models/transactions/vaultDeposit.ts (1)
VaultDeposit(16-29)packages/xrpl/src/models/transactions/vaultWithdraw.ts (1)
VaultWithdraw(19-37)packages/xrpl/src/models/transactions/vaultClawback.ts (1)
VaultClawback(24-41)packages/xrpl/src/models/transactions/vaultDelete.ts (1)
VaultDelete(13-20)
packages/ripple-binary-codec/src/types/issue.ts (4)
packages/ripple-binary-codec/src/utils.ts (2)
readUInt32BE(105-107)writeUInt32BE(42-51)packages/isomorphic/src/utils/shared.ts (1)
concat(5-7)packages/isomorphic/src/utils/browser.ts (1)
bytesToHex(15-20)packages/isomorphic/src/utils/index.ts (1)
bytesToHex(62-65)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
- GitHub Check: unit (22.x)
- GitHub Check: browser (22.x)
- GitHub Check: integration (22.x)
- GitHub Check: integration (20.x)
- GitHub Check: unit (20.x)
- GitHub Check: semgrep-cloud-platform/scan
- GitHub Check: semgrep-cloud-platform/scan
🔇 Additional comments (16)
packages/xrpl/test/models/vaultCreate.test.ts (1)
38-50: LGTM: Positive coverage for Asset as MPT and IOU.Solid assertions that both shapes pass validateVaultCreate.
packages/xrpl/src/models/ledger/Vault.ts (2)
22-22: LGTM: Flags widened to number.This aligns with ledger usage.
85-90: VaultFlags enum addition looks correct; verify constant value.Please confirm
lsfVaultPrivate = 0x00010000matches rippled’s definitions to avoid mismatch.packages/xrpl/test/integration/transactions/singleAssetVault.test.ts (1)
43-48: LGTM: Ensure issuer is funded before flows.Good improvement to reduce flakiness.
packages/xrpl/src/models/transactions/common.ts (5)
118-126: LGTM: Currency and amount size constants.These make the guards clearer and less error-prone.
210-221: LGTM: isCurrency now recognizes XRP, IOU, and MPT shapes.This unblocks MPT as an Asset.
246-256: LGTM: isIssuedCurrencyAmount guard tightened.Exact key-count + type checks are appropriate.
282-289: LGTM: isMPTAmount guard.Consistent with new size constants.
229-238: Bug: isIssuedCurrency incorrectly allows XRP.
isIssuedCurrencyshould only accept the IOU form; permitting XRP can corrupt validations (e.g., XChainBridge issues).Apply this diff:
export function isIssuedCurrency(input: unknown): input is IssuedCurrency { return ( isRecord(input) && - ((Object.keys(input).length === ISSUE_CURRENCY_SIZE && - isString(input.issuer) && - isString(input.currency)) || - (Object.keys(input).length === XRP_CURRENCY_SIZE && - input.currency === 'XRP')) + Object.keys(input).length === ISSUE_CURRENCY_SIZE && + isString(input.issuer) && + isString(input.currency) ) }Run this to locate usages that rely on XRP being accepted (none should):
packages/ripple-binary-codec/test/issue.test.ts (2)
73-74: LGTM: fromParser single-arg usage.Matches updated API.
82-83: LGTM: Updated error message assertion.Error text aligns with “Issue” terminology.
packages/ripple-binary-codec/test/fixtures/codec-fixtures.json (2)
5118-5129: Good addition: VaultCreate with MPT asset round‑trip coverage.This fixture correctly exercises an MPT Asset with mpt_issuance_id, aligned with the codec changes. It should catch regressions in endianness or width handling.
Please confirm we also have a JSON→binary→JSON round‑trip assertion for this exact blob to ensure no case folding or zero‑padding differences occur in mpt_issuance_id.
4515-4521: Formatting-only diffs look fine.The expanded XChainBridge objects and AMM sections do not alter semantics. No issues.
Also applies to: 4538-4545, 4587-4594
packages/ripple-binary-codec/src/types/issue.ts (3)
77-91: Confirm NO_ACCOUNT sentinel value for MPT layout.The MPT bytes are composed as [issuerAccount (20)] + [NO_ACCOUNT (20)] + [sequence LE (4)]. Using AccountID.from('000...0001') as NO_ACCOUNT may be spec‑sensitive. If the canonical “no account” is 20 zero bytes (or another reserved value), this will break interop.
Please confirm against the latest XRPL MPT Issue serialization spec that the 20‑byte NO_ACCOUNT sentinel should be 00..01 (not 00..00). If it must be zeros, switch to:
-const NO_ACCOUNT = AccountID.from('0000000000000000000000000000000000000001') +const NO_ACCOUNT = new AccountID(new Uint8Array(20))
48-50: Constructor defaulting is safe.Using Issue.XRP_ISSUE as the fallback makes construction robust. Looks good.
104-118: MPT detection via parser.size() is brittle; verify all call sites pass a length‑limited parser.Relying on parser.size() === 44 assumes a sub‑parser scoped to the Issue field length. If a full-object parser is used here, size() will include trailing bytes and the MPT path won’t trigger, mis-parsing 44‑byte MPT Issues as IOU/XRP.
If call sites already pass a scoped parser, we’re fine. Otherwise, consider peeking ahead without consuming bytes: if remaining >= 44, check bytes 20..39 for the NO_ACCOUNT sentinel and bytes 40..43 (LE) as a plausible sequence, then branch.
Run to confirm call patterns:
Patel-Raj11
changed the title
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (7)
packages/xrpl/test/integration/transactions/oracleSet.test.ts (2)
28-33: Anchor to validated ledger time looks good; consider DRYing via a small helper.Fetching
close_time_isoavoids clock-skew flakiness. Optional: extract this into a shared test helper to reduce duplication across tests.Example helper (to add in test utils):
// utils.ts export async function getValidatedCloseTimeSeconds(client): Promise<number> { const { result } = await client.request({ command: 'ledger', ledger_index: 'validated', }) return Math.floor(Date.parse(result.ledger.close_time_iso) / 1000) }Then replace the block with:
- const closeTime: string = ( - await testContext.client.request({ - command: 'ledger', - ledger_index: 'validated', - }) - ).result.ledger.close_time_iso + const lastUpdateTime = + (await getValidatedCloseTimeSeconds(testContext.client)) + 20And set
LastUpdateTimetolastUpdateTime(see next comment).
39-39: Use the computed value (and avoid reparsing).Use a precomputed
lastUpdateTimefor clarity and to avoid repeat parsing.- LastUpdateTime: Math.floor(new Date(closeTime).getTime() / 1000) + 20, + LastUpdateTime: lastUpdateTime,packages/xrpl/test/integration/requests/getAggregatePrice.test.ts (2)
27-33: Good shift to ledger close time; suggest a shared helper to reduce duplication.Same retrieval pattern appears in multiple tests; consider centralizing.
- const closeTime: string = ( - await testContext.client.request({ - command: 'ledger', - ledger_index: 'validated', - }) - ).result.ledger.close_time_iso + const lastUpdateTime = + (await getValidatedCloseTimeSeconds(testContext.client)) + 20Remember to import the helper from
../utilsand adjustLastUpdateTimebelow.
38-38: Prefer using the precomputed value.- LastUpdateTime: Math.floor(new Date(closeTime).getTime() / 1000) + 20, + LastUpdateTime: lastUpdateTime,packages/xrpl/test/integration/transactions/oracleDelete.test.ts (3)
27-33: Same note: factor out ledger time retrieval.DRY with a shared helper as in other tests.
- const closeTime: string = ( - await testContext.client.request({ - command: 'ledger', - ledger_index: 'validated', - }) - ).result.ledger.close_time_iso + const lastUpdateTime = + (await getValidatedCloseTimeSeconds(testContext.client)) + 20
38-38: Use the precomputedlastUpdateTime.- LastUpdateTime: Math.floor(new Date(closeTime).getTime() / 1000) + 20, + LastUpdateTime: lastUpdateTime,
73-76: Filter account_objects by type to avoid brittle assertions.After deletion, explicitly request
type: 'oracle'to avoid interference from unrelated account objects.- const aoResult2 = await testContext.client.request({ - command: 'account_objects', - account: testContext.wallet.classicAddress, - }) + const aoResult2 = await testContext.client.request({ + command: 'account_objects', + account: testContext.wallet.classicAddress, + type: 'oracle', + })
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
packages/xrpl/test/integration/requests/getAggregatePrice.test.ts(1 hunks)packages/xrpl/test/integration/transactions/oracleDelete.test.ts(1 hunks)packages/xrpl/test/integration/transactions/oracleSet.test.ts(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: ckeshava
PR: XRPLF/xrpl.js#3081
File: packages/xrpl/src/models/transactions/MPTokenIssuanceCreate.ts:57-64
Timestamp: 2025-09-16T05:00:20.420Z
Learning: In XRPL's MPTokenIssuanceCreate flags, the tfMPTCanMutate* flags intentionally use the same bit values as their corresponding base flags (e.g., tfMPTCanMutateCanLock = 0x00000002 matches tfMPTCanLock = 0x00000002) because they operate in different contexts - base flags go in the Flags field while mutable flags go in the MutableFlags field. This design avoids ambiguity since the flags are applied to separate bit fields.
📚 Learning: 2024-12-06T19:27:11.147Z
Learnt from: shawnxie999
PR: XRPLF/xrpl.js#2661
File: packages/xrpl/test/integration/transactions/clawback.test.ts:165-178
Timestamp: 2024-12-06T19:27:11.147Z
Learning: In the integration tests for `clawback.test.ts`, it's acceptable to use `ts-expect-error` to bypass type checking when verifying ledger entries, and no additional type safety improvements are needed.
Applied to files:
packages/xrpl/test/integration/transactions/oracleSet.test.tspackages/xrpl/test/integration/requests/getAggregatePrice.test.tspackages/xrpl/test/integration/transactions/oracleDelete.test.ts
🧬 Code graph analysis (3)
packages/xrpl/test/integration/transactions/oracleSet.test.ts (1)
packages/xrpl/src/models/transactions/oracleSet.ts (1)
OracleSet(31-68)
packages/xrpl/test/integration/requests/getAggregatePrice.test.ts (1)
packages/xrpl/src/models/transactions/oracleSet.ts (1)
OracleSet(31-68)
packages/xrpl/test/integration/transactions/oracleDelete.test.ts (1)
packages/xrpl/src/models/transactions/oracleSet.ts (1)
OracleSet(31-68)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
- GitHub Check: unit (20.x)
- GitHub Check: integration (22.x)
- GitHub Check: integration (20.x)
- GitHub Check: browser (22.x)
- GitHub Check: unit (22.x)
- GitHub Check: semgrep-cloud-platform/scan
- GitHub Check: semgrep-cloud-platform/scan
🔇 Additional comments (1)
packages/xrpl/test/integration/transactions/oracleSet.test.ts (1)
28-39: All Oracle tests use ledger-based time; no Date.now remainingIntegration tests compute LastUpdateTime from ledger.close_time_iso; model tests use a fixed numeric LastUpdateTime for validation.
- packages/xrpl/test/integration/transactions/oracleSet.test.ts — LastUpdateTime from close_time_iso (line ~39)
- packages/xrpl/test/integration/transactions/oracleDelete.test.ts — LastUpdateTime from close_time_iso (line ~38)
- packages/xrpl/test/integration/requests/getAggregatePrice.test.ts — LastUpdateTime from close_time_iso (line ~38); response.time asserted equals tx.LastUpdateTime (line ~81)
- packages/xrpl/test/models/oracleSet.test.ts — static LastUpdateTime = 768062172 (line ~23) (expected for model validation)
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
packages/xrpl/test/integration/transactions/singleAssetVault.test.ts (2)
307-308: Use classicAddress for consistencyOther transactions use classicAddress. Unless Wallet.address is guaranteed to be the same format, prefer classicAddress here.
Apply:
- Account: issuerWallet.address, + Account: issuerWallet.classicAddress,
388-389: Flag assertion should check bit presence, not numeric equalitynotEqual won’t catch cases where lsfVaultPrivate is set alongside other bits. Use a bitwise test.
Apply:
- assert.notEqual(vaultFlags, VaultFlags.lsfVaultPrivate) + assert.equal(vaultFlags & VaultFlags.lsfVaultPrivate, 0)packages/ripple-binary-codec/src/types/issue.ts (1)
39-41: Document MPT 44‑byte layout and confirm NO_ACCOUNT sentinel
- Add a one‑line comment by MPT_WIDTH describing layout: issuer (20) | NO_ACCOUNT (20) | sequence (LE, 4).
- AccountID.from accepts 40‑char hex strings (uses HEX_REGEX then hexToBytes), so AccountID.from('0000000000000000000000000000000000000001') parses correctly — the same sentinel appears across codec fixtures/tests.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
packages/ripple-binary-codec/src/types/issue.ts(4 hunks)packages/xrpl/src/models/ledger/index.ts(2 hunks)packages/xrpl/test/integration/transactions/singleAssetVault.test.ts(4 hunks)
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: ckeshava
PR: XRPLF/xrpl.js#3081
File: packages/xrpl/src/models/transactions/MPTokenIssuanceCreate.ts:57-64
Timestamp: 2025-09-16T05:00:20.420Z
Learning: In XRPL's MPTokenIssuanceCreate flags, the tfMPTCanMutate* flags intentionally use the same bit values as their corresponding base flags (e.g., tfMPTCanMutateCanLock = 0x00000002 matches tfMPTCanLock = 0x00000002) because they operate in different contexts - base flags go in the Flags field while mutable flags go in the MutableFlags field. This design avoids ambiguity since the flags are applied to separate bit fields.
📚 Learning: 2024-12-06T18:44:55.095Z
Learnt from: shawnxie999
PR: XRPLF/xrpl.js#2661
File: packages/xrpl/test/models/MPTokenAuthorize.test.ts:60-71
Timestamp: 2024-12-06T18:44:55.095Z
Learning: In the XRPL.js library's TypeScript test file `packages/xrpl/test/models/MPTokenAuthorize.test.ts`, negative test cases for invalid `Account` address format, invalid `Holder` address format, invalid `MPTokenIssuanceID` format, and invalid flag combinations are not necessary.
Applied to files:
packages/xrpl/test/integration/transactions/singleAssetVault.test.ts
📚 Learning: 2024-12-06T19:27:11.147Z
Learnt from: shawnxie999
PR: XRPLF/xrpl.js#2661
File: packages/xrpl/test/integration/transactions/clawback.test.ts:165-178
Timestamp: 2024-12-06T19:27:11.147Z
Learning: In the integration tests for `clawback.test.ts`, it's acceptable to use `ts-expect-error` to bypass type checking when verifying ledger entries, and no additional type safety improvements are needed.
Applied to files:
packages/xrpl/test/integration/transactions/singleAssetVault.test.ts
🧬 Code graph analysis (2)
packages/ripple-binary-codec/src/types/issue.ts (3)
packages/ripple-binary-codec/src/utils.ts (2)
readUInt32BE(105-107)writeUInt32BE(42-51)packages/isomorphic/src/utils/browser.ts (1)
bytesToHex(15-20)packages/isomorphic/src/utils/index.ts (1)
bytesToHex(62-65)
packages/xrpl/test/integration/transactions/singleAssetVault.test.ts (4)
packages/xrpl/test/integration/utils.ts (2)
generateFundedWallet(182-186)testTransaction(240-290)packages/xrpl/src/models/transactions/MPTokenIssuanceCreate.ts (1)
MPTokenIssuanceCreateMetadata(125-127)packages/xrpl/src/models/ledger/Vault.ts (1)
Vault(11-83)packages/xrpl/src/models/common/index.ts (1)
MPTCurrency(17-19)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: integration (22.x)
- GitHub Check: integration (20.x)
- GitHub Check: unit (20.x)
- GitHub Check: browser (22.x)
- GitHub Check: semgrep-cloud-platform/scan
- GitHub Check: semgrep-cloud-platform/scan
🔇 Additional comments (3)
packages/xrpl/src/models/ledger/index.ts (1)
34-34: Exporting VaultFlags looks goodMaking VaultFlags part of the public surface via the ledger index is appropriate and aligns with the tests’ usage.
Also applies to: 80-80
packages/ripple-binary-codec/src/types/issue.ts (2)
77-92: MPT from(value): endianness conversion is correct; confirm AccountID.from hex and concat orderThe BE→LE conversion for sequence and the final concat [issuer | NO_ACCOUNT | seqLE] look consistent with toJSON. Please just verify that AccountID.from accepts the raw 40‑hex string used to derive NO_ACCOUNT and that this concat order is what the binary codec expects for MPT Issue.
126-141: MPT toJSON endianness fix LGTMLE→BE reordering for the sequence and mpt_issuance_id construction is correct and matches the from(value) path.
| static fromParser(parser: BinaryParser): Issue { | ||
| // MPT | ||
| if (parser.size() === MPT_WIDTH) { | ||
| const mptBytes = parser.read(MPT_WIDTH) | ||
| return new Issue(mptBytes) | ||
| } | ||
|
|
||
| // XRP/IOU |
There was a problem hiding this comment.
🧩 Analysis chain
Detecting MPT by parser.size() assumes sized sub‑parsers
This relies on the caller providing a parser whose size equals the field width (44 for MPT). If any call site passes a shared parser (with remaining >44), MPT parsing will silently fall back to the XRP/IOU path and misread bytes.
If all call sites now pass a sized parser, great—otherwise consider keeping an optional width hint or plumbing field width to avoid ambiguity.
To confirm, please check call sites of Issue.fromParser:
Expected: invocation with a subparser (or previously: width hint removed everywhere).
🏁 Script executed:
#!/bin/bash
# Find all call sites and inspect arguments
rg -nP -C3 '\bIssue\.fromParser\s*\('Length of output: 2294
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== locate BinaryParser ==="
rg -n --hidden -S 'class BinaryParser' || true
echo "=== BinaryParser occurrences ==="
rg -n --hidden -S '\bBinaryParser\b' || true
BP_FILE=$(rg -n --hidden -S 'class BinaryParser' -l | head -n1 || true)
if [ -n "$BP_FILE" ]; then
echo "=== BinaryParser file: $BP_FILE ==="
sed -n '1,240p' "$BP_FILE"
else
echo "BinaryParser class not found by class name; searching for file defining 'size()' method used on parser..."
rg -n --hidden -S "size\(\)\s*:" || true
fi
echo "=== locate MPT_WIDTH ==="
rg -n --hidden -S 'MPT_WIDTH' || true
echo "=== show Issue.fromParser implementation ==="
sed -n '1,240p' packages/ripple-binary-codec/src/types/issue.ts || true
echo "=== show test file around calls ==="
sed -n '1,240p' packages/ripple-binary-codec/test/issue.test.ts || trueLength of output: 25347
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== list enums directory ==="
ls -la packages/ripple-binary-codec/src/enums || true
echo
echo "=== find DEFAULT_DEFINITIONS ==="
rg -n --hidden -S 'DEFAULT_DEFINITIONS' packages/ripple-binary-codec || true
DEF_FILE=$(rg -n --hidden -S 'DEFAULT_DEFINITIONS' packages/ripple-binary-codec -l | head -n1 || true)
if [ -n "$DEF_FILE" ]; then
echo
echo "=== DEFAULT_DEFINITIONS file: $DEF_FILE ==="
sed -n '1,400p' "$DEF_FILE"
fi
echo
echo "=== search for Issue references across src ==="
rg -n --hidden -S '\bIssue\b' packages/ripple-binary-codec/src || true
echo
echo "=== search for 'types/issue' imports ==="
rg -n --hidden -S "types/issue" packages/ripple-binary-codec/src || true
echo
echo "=== search for 'Issue' inside enums/definitions files ==="
rg -n --hidden -S 'Issue' packages/ripple-binary-codec/src/enums || trueLength of output: 14143
Don't detect MPT with parser.size() — it misparses MPT when the parser is shared
Issue.fromParser uses parser.size() === MPT_WIDTH to detect MPT, but BinaryParser.readFieldValue calls type.fromParser(this, ...) with the shared parser (not a sized subparser), so an MPT field inside a larger STObject will be parsed as an IOU (consume 40 bytes, leave 4 bytes) — tests construct sized parsers and hide this.
- Fix options (pick one):
- Add an optional size hint: change to static fromParser(parser: BinaryParser, hint?: number) and have BinaryParser.readFieldValue pass a field length hint (or compute it from field metadata) so Issue can use hint === 44 to parse MPT.
- Or make BinaryParser create a sized subparser for fixed-length fields before calling type.fromParser.
- Or detect MPT by peeking bytes (e.g., check bytes[20..40] === NO_ACCOUNT) without consuming, then read 44 if matched.
Locations: packages/ripple-binary-codec/src/types/issue.ts (fromParser), packages/ripple-binary-codec/src/serdes/binary-parser.ts (readFieldValue); tests: packages/ripple-binary-codec/test/issue.test.ts
🤖 Prompt for AI Agents
In packages/ripple-binary-codec/src/types/issue.ts around lines 105-112 and the
caller in packages/ripple-binary-codec/src/serdes/binary-parser.ts
(readFieldValue), Issue.fromParser currently detects MPT by parser.size() which
misparses when the parser is shared; change the API to accept an optional size
hint (static fromParser(parser: BinaryParser, hint?: number)), have
BinaryParser.readFieldValue compute or pass the field length hint (from field
metadata or provided length) into type.fromParser, and update Issue.fromParser
to use hint === 44 to decide MPT vs IOU (falling back to existing logic when
hint is undefined); update tests
(packages/ripple-binary-codec/test/issue.test.ts) to exercise the new hint path
and ensure existing behavior remains when sized subparsers are used.
There was a problem hiding this comment.
fromParser gets called based on the associatedType of the field. In this case it's Issue. The Issue sub-classes fromParser only gets called once we identify the associatedType is Issue. So the binary parser should exactly point to either MPT/XRP/IOU asset and these checks are in-place while parsing.
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Let's examine BinaryParser.readFieldValue implementation to see how it calls fromParser
echo "=== BinaryParser.readFieldValue implementation ==="
rg -n -A 10 -B 5 "readFieldValue" packages/ripple-binary-codec/src/serdes/binary-parser.ts
echo ""
echo "=== Look for how parser is constructed/passed to fromParser ==="
rg -n -A 5 -B 5 "\.fromParser" packages/ripple-binary-codec/src/serdes/binary-parser.ts
echo ""
echo "=== Check if there are methods that create subparsers ==="
rg -n -A 3 -B 3 "new BinaryParser\|\.slice\|subparser" packages/ripple-binary-codec/src/serdes/binary-parser.ts
Length of output: 1889
🏁 Script executed:
#!/bin/bash
# Let's examine the full BinaryParser class to understand how it manages bytes and parsing
echo "=== Full BinaryParser implementation ==="
cat packages/ripple-binary-codec/src/serdes/binary-parser.ts
echo ""
echo "=== Check how Issue field is defined in field definitions ==="
rg -n -A 5 -B 5 '"Issue"' packages/ripple-binary-codec/src/enums/definitions.json
echo ""
echo "=== Look for variable length encoding info ==="
rg -n "isVariableLengthEncoded" packages/ripple-binary-codec/src/enums/
Length of output: 7117
🏁 Script executed:
#!/bin/bash
# Let's examine how STObject calls Issue.fromParser to understand the parsing context
echo "=== Check STObject implementation and how it handles fields ==="
rg -n -A 10 -B 5 "readFieldValue\|fromParser" packages/ripple-binary-codec/src/types/st-object.ts
echo ""
echo "=== Check if Issue has fixed width in type system ==="
rg -n -A 5 -B 5 "width\|WIDTH" packages/ripple-binary-codec/src/types/issue.ts
echo ""
echo "=== Look at how other fixed-length types handle fromParser ==="
echo "--- AccountID fromParser ---"
rg -n -A 8 -B 2 "static fromParser" packages/ripple-binary-codec/src/types/account-id.ts
echo ""
echo "--- Currency fromParser ---"
rg -n -A 8 -B 2 "static fromParser" packages/ripple-binary-codec/src/types/currency.ts
echo ""
echo "=== Check Hash192 implementation for comparison ==="
rg -n -A 8 -B 2 "static fromParser" packages/ripple-binary-codec/src/types/hash-192.ts
Length of output: 293
🏁 Script executed:
#!/bin/bash
# Let's find the actual files and check their contents
echo "=== Find all TypeScript files in types directory ==="
find packages/ripple-binary-codec/src/types -name "*.ts" -type f
echo ""
echo "=== Check st-object.ts content ==="
cat packages/ripple-binary-codec/src/types/st-object.ts
echo ""
echo "=== Check account-id.ts content ==="
cat packages/ripple-binary-codec/src/types/account-id.ts
echo ""
echo "=== Check hash-192.ts content ==="
cat packages/ripple-binary-codec/src/types/hash-192.ts
Length of output: 10428
@Patel-Raj11, thank you for the explanation! I understand your reasoning about the parser pointing exactly to the field data. However, after examining the BinaryParser implementation, I found a discrepancy that concerns me.
Looking at the code flow:
- BinaryParser.readFieldValue calls
type.fromParser(this, sizeHint)- passing the full parser instance, not a sized subparser - The parser operates on shared bytes -
BinaryParser.read(n)consumes bytes from the front and advances position, butparser.size()returns the total remaining bytes - Issue.fromParser receives the full parser which may contain multiple fields worth of data beyond just the current Issue field
The current logic parser.size() === MPT_WIDTH would only work correctly if:
- The Issue field is the last field in the serialized data, OR
- The parser happens to have exactly 44 bytes remaining
For an Issue field in the middle of a transaction, parser.size() would return the total remaining bytes (potentially hundreds), not just the 44 bytes for the current field.
This could cause MPT fields to be incorrectly parsed as IOU fields (consuming 40 bytes instead of 44), leaving 4 bytes unconsumed and potentially corrupting subsequent field parsing.
Could you help me understand how this size detection works correctly in practice? Are there integration tests that serialize/deserialize transactions with MPT Issue fields in the middle of the transaction data that I could look at to verify the behavior?
✏ Learnings added
Learnt from: Patel-Raj11
PR: XRPLF/xrpl.js#3090
File: packages/ripple-binary-codec/src/types/issue.ts:105-112
Timestamp: 2025-09-24T14:59:08.342Z
Learning: In the ripple-binary-codec, BinaryParser.readFieldValue passes the full parser instance (not a sized subparser) to type.fromParser methods, so parser.size() returns total remaining bytes across all fields, not just the current field's bytes.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
There was a problem hiding this comment.
@Patel-Raj11 I agree with the rabbit's suggestion. Since the Issue type is shared by IOU/XRP/MPT types, how can you determine the appropriate size of the parser?
If we are provided with a sufficiently long buffer of bytes (say a serialized transaction), the size of the parser will mislead us, because we can read 44 bytes (for an MPT), 40 bytes ( for an IOU) or 20 bytes (for XRP representation).
This suggestion from the AI is more robust:
Or detect MPT by peeking bytes (e.g., check bytes[20..40] === NO_ACCOUNT) without consuming, then read 44 if matched.
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
There was a problem hiding this comment.
Makes sense, addressed here 1169182
| (Object.keys(input).length === XRP_CURRENCY_SIZE && | ||
| input.currency === 'XRP') || | ||
| (Object.keys(input).length === MPT_CURRENCY_SIZE && | ||
| isString(input.mpt_issuance_id))) |
There was a problem hiding this comment.
this could throw an unhandled-error if mpt_issuance_id is not a part of the Object.keys(input).
(Object.keys(input).length == 1 && input['mpt_issuance_id'] != undefined && isString(input.mpt_issuance_id) might help resolve that case.
There was a problem hiding this comment.
isString will return false, if that's the case.
| : field.associatedType?.from | ||
| ? field.associatedType.from(xAddressDecoded[field.name]) | ||
| : (() => { | ||
| throw new Error( |
There was a problem hiding this comment.
This error is useful when rippled has a new internal type but xrpl.js is missing an implementation for such a new type.
There was a problem hiding this comment.
We already throw an error from base class SerializedType if the subclass does not overrides this method.
static from(value: SerializedType | JSON | bigint): SerializedType {
throw new Error('from not implemented')
return this.from(value)
}
| "Flags": 0, | ||
| "RootIndex": "059D1E86DE5DCCCF956BF4799675B2425AF9AD44FE4CCA6FEE1C812EEF6423E6", | ||
| "Indexes": ["908D554AA0D29F660716A3EE65C61DD886B744DDF60DE70E6B16EADB770635DB"] | ||
| "Indexes": [ |
There was a problem hiding this comment.
Can you confirm that this file only has formatting changes?
There was a problem hiding this comment.
Ya, there is a VaultCreate test case with MPT added at the bottom.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/ripple-binary-codec/src/types/issue.ts (1)
39-41: Consider making constants private or moving to shared moduleThe constants
MPT_WIDTHandNO_ACCOUNTare implementation details that may not need to be exported at module level. Consider either:
- Making them private to the class (
private static readonly)- Moving to a shared constants file if used elsewhere
-const MPT_WIDTH = 44 -const NO_ACCOUNT = AccountID.from('0000000000000000000000000000000000000001') - /** * Class for serializing/Deserializing Issue */ class Issue extends SerializedType { + private static readonly MPT_WIDTH = 44 + private static readonly NO_ACCOUNT = AccountID.from('0000000000000000000000000000000000000001') + static readonly XRP_ISSUE: Issue = new Issue(new Uint8Array(20))
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/ripple-binary-codec/src/types/issue.ts(3 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-09-24T14:59:08.342Z
Learnt from: Patel-Raj11
PR: XRPLF/xrpl.js#3090
File: packages/ripple-binary-codec/src/types/issue.ts:105-112
Timestamp: 2025-09-24T14:59:08.342Z
Learning: In the ripple-binary-codec, BinaryParser.readFieldValue passes the full parser instance (not a sized subparser) to type.fromParser methods, so parser.size() returns total remaining bytes across all fields, not just the current field's bytes.
Applied to files:
packages/ripple-binary-codec/src/types/issue.ts
🧬 Code graph analysis (1)
packages/ripple-binary-codec/src/types/issue.ts (2)
packages/ripple-binary-codec/src/utils.ts (2)
readUInt32BE(105-107)writeUInt32BE(42-51)packages/isomorphic/src/utils/index.ts (1)
bytesToHex(62-65)
🔇 Additional comments (4)
packages/ripple-binary-codec/src/types/issue.ts (4)
82-91: Correct endian conversion for MPT sequence handlingThe implementation correctly handles the endianness conversion for MPT sequence data:
- Line 83: Extracts sequence in big-endian format from
mpt_issuance_id- Lines 85-87: Converts to little-endian for internal storage
- This matches the reverse conversion in
toJSON()(lines 139-141)
131-146: LGTM: MPT serialization with correct endian handlingThe MPT detection and JSON serialization logic is correct:
- Line 132: Uses explicit width check (
MPT_WIDTH) rather than unreliable parser size- Lines 134-137: Extracts little-endian sequence from internal storage
- Lines 140-141: Converts to big-endian for
mpt_issuance_idoutput- Line 144: Properly concatenates sequence (big-endian) + issuer for final hex output
1-8: Approve utility imports usageAll imported utilities are used correctly and are properly exported; no changes needed.
105-123: CRITICAL: MPT detection via fixed-width pattern unsafe with shared parserBased on XRPL binary format field definitions in SField.h and the retrieved learnings showing BinaryParser.readFieldValue passes the full parser instance (not a sized subparser) to fromParser methods, this implementation has a critical flaw:
- Detection Logic: Lines 107-110 first attempt to parse XRP by reading 20 bytes for currency. If not XRP, lines 113-119 assume remaining data forms an MPT if bytes[20-40] match NO_ACCOUNT pattern.
- Critical Issue: When parsing an Issue field within a larger transaction,
parsercontains bytes for all remaining fields, not just the current Issue. An IOU Issue (40 bytes) followed by any field will trigger the MPT detection if the next field's initial bytes happen to match NO_ACCOUNT.- Silent Corruption: This causes IOU fields to be parsed as MPT, consuming 44 bytes instead of 40, leaving 4 bytes unconsumed and potentially corrupting subsequent field parsing.
Based on retrieved learnings
Fix approach: Add field width detection that doesn't rely on parser size:
static fromParser(parser: BinaryParser): Issue { - // XRP - const currencyOrAccount = parser.read(20) - if (new Currency(currencyOrAccount).toJSON() === 'XRP') { - return new Issue(currencyOrAccount) - } - - // MPT - const issuerAccountId = new AccountID(parser.read(20)) - if (NO_ACCOUNT.toHex() === issuerAccountId.toHex()) { - const sequence = parser.read(4) - return new Issue( - concat([currencyOrAccount, NO_ACCOUNT.toBytes(), sequence]), - ) - } - - // IOU - return new Issue(concat([currencyOrAccount, issuerAccountId.toBytes()])) + // Peek to detect MPT pattern without consuming bytes + const peekBuffer = parser.peek(44) + + // XRP check + if (new Currency(peekBuffer.slice(0, 20)).toJSON() === 'XRP') { + return new Issue(parser.read(20)) + } + + // MPT check - verify NO_ACCOUNT pattern at offset 20 + if (peekBuffer.length >= 44 && + NO_ACCOUNT.toHex() === bytesToHex(peekBuffer.slice(20, 40))) { + const mptBytes = parser.read(44) + return new Issue(mptBytes) + } + + // IOU - consume 40 bytes + return new Issue(parser.read(40)) }Note: This requires adding a
peek(n)method to BinaryParser that returns bytes without advancing position.⛔ Skipped due to learnings
Learnt from: Patel-Raj11 PR: XRPLF/xrpl.js#3090 File: packages/ripple-binary-codec/src/types/issue.ts:105-112 Timestamp: 2025-09-24T14:59:08.342Z Learning: In the ripple-binary-codec, BinaryParser.readFieldValue passes the full parser instance (not a sized subparser) to type.fromParser methods, so parser.size() returns total remaining bytes across all fields, not just the current field's bytes.Learnt from: shawnxie999 PR: XRPLF/xrpl.js#2661 File: packages/xrpl/test/models/MPTokenAuthorize.test.ts:60-71 Timestamp: 2024-12-06T18:44:55.095Z Learning: In the XRPL.js library's TypeScript test file `packages/xrpl/test/models/MPTokenAuthorize.test.ts`, negative test cases for invalid `Account` address format, invalid `Holder` address format, invalid `MPTokenIssuanceID` format, and invalid flag combinations are not necessary.
| issuerWallet = testContext.wallet | ||
| issuerWallet = await generateFundedWallet(testContext.client) |
There was a problem hiding this comment.
why was this change needed?
There was a problem hiding this comment.
hmm, I'm not particularly sure myself. I believe any valid wallet on the network suffices for the purposes of this test.
There was a problem hiding this comment.
To reuse a wallet and write integration tests that are not flaky, we need to move away from querying account objects and picking up the first one in our integration tests.
We should write integration tests that computes LedgerIndex using such example methods introduced here - #3086
Somehow we have stopped writing these utility functions and we should write those for all the missing ledger objects and refactor all test cases to deterministically compute ledger index.
|
LGTM, one small question for understanding |
High Level Overview of Change
validateVaultCreatethat prevented vault with MPT as an asset.Context of Change
This bug prevented creation of Vault that has MPT as an asset.
Type of Change
Did you update HISTORY.md?
Test Plan
Added unit and integration test cases.