Open
Conversation
…ts for the binary-codec of path-set type
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:
WalkthroughAdds XLS-82D MPT support across the repo: enables MPTokensV2 in rippled config; extends ripple-binary-codec with Hash192 fields, MPT hop serialization, and exports; widens xrpl model types and validators to accept MPTAmount / mpt_issuance_id; and adds many unit and integration tests and test helpers for MPT flows. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 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: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/xrpl/src/models/transactions/AMMClawback.ts (1)
120-138:⚠️ Potential issue | 🟠 MajorEnforce Asset/Amount type alignment and MPT issuance match.
Right now, a MPT Amount can be used with an issued-currency Asset (or vice versa) without any validation, and MPT issuance IDs are not checked for equality. Add explicit checks to keep Asset/Amount consistent and validate the MPT issuance ID.
🧩 Suggested fix
- if ( - tx.Amount != null && - isIssuedCurrencyAmount(tx.Amount) && - isIssuedCurrency(asset) - ) { - if (tx.Amount.currency !== asset.currency) { - throw new ValidationError( - 'AMMClawback: Amount.currency must match Asset.currency', - ) - } - - if (tx.Amount.issuer !== asset.issuer) { - throw new ValidationError( - 'AMMClawback: Amount.issuer must match Amount.issuer', - ) - } - } + if (tx.Amount != null) { + if (isIssuedCurrencyAmount(tx.Amount)) { + if (!isIssuedCurrency(asset)) { + throw new ValidationError( + 'AMMClawback: Amount must match Asset type', + ) + } + if (tx.Amount.currency !== asset.currency) { + throw new ValidationError( + 'AMMClawback: Amount.currency must match Asset.currency', + ) + } + if (tx.Amount.issuer !== asset.issuer) { + throw new ValidationError( + 'AMMClawback: Amount.issuer must match Asset.issuer', + ) + } + } else if (isMPTAmount(tx.Amount)) { + if (!('mpt_issuance_id' in asset)) { + throw new ValidationError( + 'AMMClawback: Amount.mpt_issuance_id requires Asset.mpt_issuance_id', + ) + } + if (tx.Amount.mpt_issuance_id !== asset.mpt_issuance_id) { + throw new ValidationError( + 'AMMClawback: Amount.mpt_issuance_id must match Asset.mpt_issuance_id', + ) + } + } + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/xrpl/src/models/transactions/AMMClawback.ts` around lines 120 - 138, The code currently allows mixing MPT and issued-currency types and doesn't verify MPT issuance IDs; update the validation around validateOptionalField(tx, 'Amount', isClawbackAmountValid) to first assert type alignment (i.e., require isIssuedCurrencyAmount(tx.Amount) === isIssuedCurrency(asset) and throw a ValidationError if they differ), then keep the existing currency check (tx.Amount.currency === asset.currency) and fix the typo in the issuer error message to compare tx.Amount.issuer === asset.issuer, and finally, when both sides are MPT (the non-issued case) add a check that their MPT issuance IDs match (compare the MPT issuance identifier fields on tx.Amount and asset and throw a ValidationError if they differ).
🧹 Nitpick comments (3)
packages/xrpl/test/models/AMMClawback.test.ts (1)
119-123: Pre-existing: error message appears self-referential.The expected error message
'AMMClawback: Amount.issuer must match Amount.issuer'looks like a copy-paste typo — it likely should read'Amount.issuer must match Asset.issuer'. This predates this PR, but worth fixing while you're in the area.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/xrpl/test/models/AMMClawback.test.ts` around lines 119 - 123, The test in AMMClawback.test.ts asserts a self-referential error message; update the expected message so it matches the actual validation wording (replace 'AMMClawback: Amount.issuer must match Amount.issuer' with the correct text, e.g. 'AMMClawback: Amount.issuer must match Asset.issuer') in the test that calls assertInvalid(ammClawback, errorMessage); also scan the AMMClawback validation code that throws this error to ensure the thrown string uses the same corrected wording and adjust it if necessary.packages/xrpl/test/integration/requests/bookOffers.test.ts (1)
88-93: Track theBookOffersRequestMPTCurrency gap as an issue.The
@ts-expect-errorsuppresses a missing type definition fortaker_gets: { mpt_issuance_id }inBookOffersRequest. The inline comment acknowledges the gap but there's no linked issue to ensure it doesn't slip.Would you like me to open a follow-up issue to add
MPTCurrencysupport to thetaker_gets/taker_paysfields inBookOffersRequest?🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/xrpl/test/integration/requests/bookOffers.test.ts` around lines 88 - 93, The test uses a ts-expect-error to bypass the missing MPTCurrency type for BookOffersRequest (taker_gets with mpt_issuance_id); create a tracked issue (e.g., "Add MPTCurrency support to BookOffersRequest taker_gets/taker_pays") in the repo issue tracker, add the issue number to the inline comment next to the existing `@ts-expect-error`, and update the task to remove the ts-expect-error and correctly type taker_gets/taker_pays (BookOffersRequest, mpt_issuance_id) once the new MPTCurrency type is implemented.packages/xrpl/test/integration/transactions/offerCreate.test.ts (1)
127-130: Explicit flags are redundant withcreateMPTIssuanceAndAuthorizedefaults.The fourth argument
tfMPTCanTrade | tfMPTCanTransferis identical to the function's default parameter value. You can drop the argument (and the accompanying eslint-disable comment) to simplify the call site.♻️ Suggested simplification
const mptIssuanceId = await createMPTIssuanceAndAuthorize( testContext.client, issuerWallet, sourceWallet, - // eslint-disable-next-line no-bitwise -- combining flags requires bitwise OR - MPTokenIssuanceCreateFlags.tfMPTCanTrade | - MPTokenIssuanceCreateFlags.tfMPTCanTransfer, )The same pattern appears in
bookOffers.test.ts(lines 70–73) and can be cleaned up there as well.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/xrpl/test/integration/transactions/offerCreate.test.ts` around lines 127 - 130, The call to createMPTIssuanceAndAuthorize explicitly passes MPTokenIssuanceCreateFlags.tfMPTCanTrade | MPTokenIssuanceCreateFlags.tfMPTCanTransfer as the fourth argument (with an eslint-disable) even though that is the function's default; remove that fourth argument and the accompanying "// eslint-disable-next-line no-bitwise" comment from the createMPTIssuanceAndAuthorize invocation so it relies on the default flags, and apply the same cleanup for the identical pattern in bookOffers.test.ts (remove the explicit tfMPTCanTrade|tfMPTCanTransfer argument and its eslint-disable).
🤖 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/ripple-binary-codec/test/path-set.test.ts`:
- Around line 130-155: Rename the test description string in the test case that
currently reads "Two paths with mpt_issaunce_id hops" to correct the typo
"mpt_issaunce_id" → "mpt_issuance_id" (the test defined with it(...) containing
the path with mpt_issuance_id fields); ensure the test name and any other
occurrences in this file (e.g., the it(...) description in
packages/ripple-binary-codec/test/path-set.test.ts) are updated consistently.
- Around line 85-109: The test named "PathSet: Two hops inside a Path" is
misleading because the constructed `path` contains two separate path elements
(separated by 0xFF) rather than two hops in a single path; update the test to
reflect actual structure by renaming the test to something like "PathSet: Two
paths (separated by 0xFF)" or change the `path` variable to be a single path
with two hop objects if you intended to test two hops in one path; adjust the
test name string in the `it(...)` call and/or the `path` construction used by
`coreTypes.PathSet.from(path)` and `serializedHexRepr` accordingly so the name
matches the serialized structure validated by `BinaryParser` and assertions.
In `@packages/xrpl/src/models/transactions/AMMClawback.ts`:
- Around line 100-116: The AMMClawback validation allows plain XRP objects
because isCurrency now permits {currency: 'XRP'}; to fix, add a guard in the
AMMClawback validation (before the isIssuedCurrency(asset) branch) that checks
if tx.Asset is a currency with currency === 'XRP' and throw a ValidationError
rejecting XRP assets so only issued currencies (isIssuedCurrency) or MPT assets
are accepted; update the logic around validateRequiredField, isCurrency, and
isIssuedCurrency to enforce this check and reference tx.Asset, Holder, and
Account in the error path.
In `@packages/xrpl/src/models/transactions/payment.ts`:
- Around line 281-291: Ensure any path step that includes mpt_issuance_id is
always validated and never allowed to coexist with currency/issuer: when
pathStep.mpt_issuance_id is present, first reject if pathStep.currency or
pathStep.issuer are defined, then validate type is string and that it matches
/^[A-F0-9]{48}$/iu, returning false on any failure and true only if all checks
pass; conversely, if currency/issuer are present ensure mpt_issuance_id is
absent before returning true. Use the existing pathStep.mpt_issuance_id,
pathStep.currency and pathStep.issuer checks in the validation function to
enforce these mutual-exclusion and validation rules.
In `@packages/xrpl/test/integration/transactions/payment.test.ts`:
- Around line 260-263: The comment above the Payment test is stale: it
references a non-existent MPT_A and two alternative paths but the test only sets
up mptIdB and an XRP/MPT_B AMM pool and builds a Payment object (payTx) with a
single Paths entry; either update the comment to describe the single
cross-currency path (XRP → MPT_B via the XRP/MPT_B pool) and remove all MPT_A
references, or if two paths were intended, add creation of MPT_A and its AMM
pool and update the Paths field in the payTx to include the additional path;
locate the Payment object (payTx), the Paths array, and mptIdB in the test to
make the change.
---
Outside diff comments:
In `@packages/xrpl/src/models/transactions/AMMClawback.ts`:
- Around line 120-138: The code currently allows mixing MPT and issued-currency
types and doesn't verify MPT issuance IDs; update the validation around
validateOptionalField(tx, 'Amount', isClawbackAmountValid) to first assert type
alignment (i.e., require isIssuedCurrencyAmount(tx.Amount) ===
isIssuedCurrency(asset) and throw a ValidationError if they differ), then keep
the existing currency check (tx.Amount.currency === asset.currency) and fix the
typo in the issuer error message to compare tx.Amount.issuer === asset.issuer,
and finally, when both sides are MPT (the non-issued case) add a check that
their MPT issuance IDs match (compare the MPT issuance identifier fields on
tx.Amount and asset and throw a ValidationError if they differ).
---
Nitpick comments:
In `@packages/xrpl/test/integration/requests/bookOffers.test.ts`:
- Around line 88-93: The test uses a ts-expect-error to bypass the missing
MPTCurrency type for BookOffersRequest (taker_gets with mpt_issuance_id); create
a tracked issue (e.g., "Add MPTCurrency support to BookOffersRequest
taker_gets/taker_pays") in the repo issue tracker, add the issue number to the
inline comment next to the existing `@ts-expect-error`, and update the task to
remove the ts-expect-error and correctly type taker_gets/taker_pays
(BookOffersRequest, mpt_issuance_id) once the new MPTCurrency type is
implemented.
In `@packages/xrpl/test/integration/transactions/offerCreate.test.ts`:
- Around line 127-130: The call to createMPTIssuanceAndAuthorize explicitly
passes MPTokenIssuanceCreateFlags.tfMPTCanTrade |
MPTokenIssuanceCreateFlags.tfMPTCanTransfer as the fourth argument (with an
eslint-disable) even though that is the function's default; remove that fourth
argument and the accompanying "// eslint-disable-next-line no-bitwise" comment
from the createMPTIssuanceAndAuthorize invocation so it relies on the default
flags, and apply the same cleanup for the identical pattern in
bookOffers.test.ts (remove the explicit tfMPTCanTrade|tfMPTCanTransfer argument
and its eslint-disable).
In `@packages/xrpl/test/models/AMMClawback.test.ts`:
- Around line 119-123: The test in AMMClawback.test.ts asserts a
self-referential error message; update the expected message so it matches the
actual validation wording (replace 'AMMClawback: Amount.issuer must match
Amount.issuer' with the correct text, e.g. 'AMMClawback: Amount.issuer must
match Asset.issuer') in the test that calls assertInvalid(ammClawback,
errorMessage); also scan the AMMClawback validation code that throws this error
to ensure the thrown string uses the same corrected wording and adjust it if
necessary.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (39)
.ci-config/rippled.cfgpackages/ripple-binary-codec/HISTORY.mdpackages/ripple-binary-codec/src/enums/definitions.jsonpackages/ripple-binary-codec/src/types/index.tspackages/ripple-binary-codec/src/types/issue.tspackages/ripple-binary-codec/src/types/path-set.tspackages/ripple-binary-codec/test/path-set.test.tspackages/xrpl/HISTORY.mdpackages/xrpl/src/models/common/index.tspackages/xrpl/src/models/methods/ledgerEntry.tspackages/xrpl/src/models/transactions/AMMClawback.tspackages/xrpl/src/models/transactions/AMMDeposit.tspackages/xrpl/src/models/transactions/AMMWithdraw.tspackages/xrpl/src/models/transactions/checkCreate.tspackages/xrpl/src/models/transactions/common.tspackages/xrpl/src/models/transactions/payment.tspackages/xrpl/test/integration/mptUtils.tspackages/xrpl/test/integration/requests/ammInfo.test.tspackages/xrpl/test/integration/requests/bookOffers.test.tspackages/xrpl/test/integration/requests/ledgerEntry.test.tspackages/xrpl/test/integration/requests/pathFind.test.tspackages/xrpl/test/integration/requests/ripplePathFind.test.tspackages/xrpl/test/integration/transactions/ammClawback.test.tspackages/xrpl/test/integration/transactions/ammCreate.test.tspackages/xrpl/test/integration/transactions/ammDeposit.test.tspackages/xrpl/test/integration/transactions/ammWithdraw.test.tspackages/xrpl/test/integration/transactions/checkCash.test.tspackages/xrpl/test/integration/transactions/checkCreate.test.tspackages/xrpl/test/integration/transactions/offerCreate.test.tspackages/xrpl/test/integration/transactions/payment.test.tspackages/xrpl/test/models/AMMClawback.test.tspackages/xrpl/test/models/AMMCreate.test.tspackages/xrpl/test/models/AMMDeposit.test.tspackages/xrpl/test/models/AMMWithdraw.test.tspackages/xrpl/test/models/checkCash.test.tspackages/xrpl/test/models/checkCreate.test.tspackages/xrpl/test/models/offerCreate.test.tspackages/xrpl/test/models/payment.test.tspackages/xrpl/test/testUtils.ts
Comment on lines
+130
to
+155
| it(`Two paths with mpt_issaunce_id hops`, () => { | ||
| const mptIssuanceId1 = '00000001B5F762798A53D543A014CAF8B297CFF8F2F937E8' | ||
| const mptIssuanceId2 = '000004C463C52827307480341125DA0577DEFC38405B0E3E' | ||
| const path = [ | ||
| [{ mpt_issuance_id: mptIssuanceId1 } as HopObject], | ||
| [{ mpt_issuance_id: mptIssuanceId2 } as HopObject], | ||
| ] | ||
| const serializedHexRepr = coreTypes.PathSet.from(path).toHex().toUpperCase() | ||
|
|
||
| expect(serializedHexRepr).toEqual( | ||
| '40' /* MPT has a type code of 40 */ + | ||
| mptIssuanceId1.toUpperCase() /* raw 24-byte MPTID */ + | ||
| 'FF' /* Two path elements are separated by 0xFF type code */ + | ||
| '40' /* MPT has a type code of 40 */ + | ||
| mptIssuanceId2.toUpperCase() /* raw 24-byte MPTID */ + | ||
| '00' /* PathSet elements end with 00 code */, | ||
| ) | ||
|
|
||
| expect(coreTypes.PathSet.from(path).toJSON()).toEqual(path) | ||
|
|
||
| // validate the de-serialization via the BinaryParser | ||
| const parser = new BinaryParser(serializedHexRepr) | ||
| expect(coreTypes.PathSet.fromParser(parser)).toEqual( | ||
| coreTypes.PathSet.from(path), | ||
| ) | ||
| }) |
Contributor
There was a problem hiding this comment.
Fix typo in test description.
Minor spelling issue in the test name: “issaunce” → “issuance”.
✏️ Typo fix
- it(`Two paths with mpt_issaunce_id hops`, () => {
+ it(`Two paths with mpt_issuance_id hops`, () => {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/ripple-binary-codec/test/path-set.test.ts` around lines 130 - 155,
Rename the test description string in the test case that currently reads "Two
paths with mpt_issaunce_id hops" to correct the typo "mpt_issaunce_id" →
"mpt_issuance_id" (the test defined with it(...) containing the path with
mpt_issuance_id fields); ensure the test name and any other occurrences in this
file (e.g., the it(...) description in
packages/ripple-binary-codec/test/path-set.test.ts) are updated consistently.
Comment on lines
+100
to
116
| validateRequiredField(tx, 'Asset', isCurrency) | ||
|
|
||
| const asset = tx.Asset | ||
|
|
||
| if (tx.Holder === asset.issuer) { | ||
| throw new ValidationError( | ||
| 'AMMClawback: Holder and Asset.issuer must be distinct', | ||
| ) | ||
| } | ||
| if (isIssuedCurrency(asset)) { | ||
| if (tx.Holder === asset.issuer) { | ||
| throw new ValidationError( | ||
| 'AMMClawback: Holder and Asset.issuer must be distinct', | ||
| ) | ||
| } | ||
|
|
||
| if (tx.Account !== asset.issuer) { | ||
| throw new ValidationError( | ||
| 'AMMClawback: Account must be the same as Asset.issuer', | ||
| ) | ||
| if (tx.Account !== asset.issuer) { | ||
| throw new ValidationError( | ||
| 'AMMClawback: Account must be the same as Asset.issuer', | ||
| ) | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
Reject XRP assets explicitly.
isCurrency now permits { currency: 'XRP' }, which can slip through without issuer checks. AMMClawback should only allow issued currencies or MPT assets. Add a guard to prevent XRP assets from validating.
🛠️ Suggested fix
validateRequiredField(tx, 'Asset', isCurrency)
const asset = tx.Asset
+
+ if (!isIssuedCurrency(asset) && !('mpt_issuance_id' in asset)) {
+ throw new ValidationError(
+ 'AMMClawback: Asset must be an issued currency or MPT',
+ )
+ }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/xrpl/src/models/transactions/AMMClawback.ts` around lines 100 - 116,
The AMMClawback validation allows plain XRP objects because isCurrency now
permits {currency: 'XRP'}; to fix, add a guard in the AMMClawback validation
(before the isIssuedCurrency(asset) branch) that checks if tx.Asset is a
currency with currency === 'XRP' and throw a ValidationError rejecting XRP
assets so only issued currencies (isIssuedCurrency) or MPT assets are accepted;
update the logic around validateRequiredField, isCurrency, and isIssuedCurrency
to enforce this check and reference tx.Asset, Holder, and Account in the error
path.
Collaborator
There was a problem hiding this comment.
If you add this change, make sure you include coverage in AMMClawback.test.ts
Contributor
There was a problem hiding this comment.
@cybele-ripple, noted! If the XRP validation guard is implemented, I'll make sure to include corresponding test coverage in AMMClawback.test.ts to verify that XRP assets are properly rejected.
Would you like me to go ahead and implement this change along with the test coverage, or is this guidance for the PR author to address?
🧠 Learnings used
Learnt from: ckeshava
Repo: XRPLF/xrpl.js PR: 3081
File: packages/xrpl/src/models/transactions/MPTokenIssuanceCreate.ts:298-298
Timestamp: 2025-10-30T12:09:55.784Z
Learning: In XRPL's MPTokenIssuanceCreate transaction validation, MaximumAmount must be greater than 0 (not greater than or equal to). The rippled C++ implementation at MPTokenIssuanceCreate.cpp explicitly rejects MaximumAmount == 0 with temMALFORMED, so the JavaScript validation should use `<= 0` to match this behavior.
Learnt from: ckeshava
Repo: XRPLF/xrpl.js PR: 3214
File: packages/xrpl/src/models/transactions/payment.ts:281-291
Timestamp: 2026-02-25T19:37:37.836Z
Learning: In PathStep validation within `packages/xrpl/src/models/transactions/payment.ts`, the `mpt_issuance_id` field can coexist with the `issuer` field in a path step, but cannot be present alongside `currency` or `account` fields. This reflects the XLS-82D specification for MPT-DEX PathSet support.
Learnt from: shawnxie999
Repo: XRPLF/xrpl.js PR: 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.
Learnt from: shawnxie999
Repo: XRPLF/xrpl.js PR: 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.
Learnt from: achowdhry-ripple
Repo: XRPLF/xrpl.js PR: 2661
File: packages/xrpl/src/models/transactions/MPTokenIssuanceCreate.ts:69-102
Timestamp: 2024-12-05T16:48:12.951Z
Learning: When adding validation in `validate*` functions in `packages/xrpl/src/models/transactions/`, utilize existing helper functions (e.g., `validateOptionalField`, `validateType`, `isNumber`, `isInteger`) for type checking and validation where appropriate.
Learnt from: mvadari
Repo: XRPLF/xrpl.js PR: 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.
Learnt from: ckeshava
Repo: XRPLF/xrpl.js PR: 3032
File: packages/xrpl/src/models/transactions/common.ts:689-698
Timestamp: 2025-07-10T22:04:59.994Z
Learning: In the XRPL.js library, the validateDomainID function in `packages/xrpl/src/models/transactions/common.ts` should only validate the length (64 characters) of domain IDs, not hex encoding. The rippled C++ implementation does not enforce any regex check on domain ID values, so additional hex validation is not required in the JS implementation.
Learnt from: ckeshava
Repo: XRPLF/xrpl.js PR: 2873
File: packages/xrpl/src/models/transactions/trustSet.ts:33-36
Timestamp: 2025-01-08T13:08:52.688Z
Learning: For trust-set transactions in XRPL, validation rules for flags should be implemented comprehensively rather than cherry-picking specific rules, as there are many interdependent validation rules associated with these flags.
Learnt from: ckeshava
Repo: XRPLF/xrpl.js PR: 2874
File: packages/xrpl/test/integration/transactions/permissionedDomain.test.ts:25-80
Timestamp: 2025-01-08T02:12:28.489Z
Learning: The rippled C++ implementation (PR `#5161`) includes comprehensive test coverage for PermissionedDomain (XLS-80d) error cases. The JS SDK tests focus on the happy path since the error cases are already validated at the rippled level, following the principle of not duplicating complex validation testing across SDK implementations.
Learnt from: ckeshava
Repo: XRPLF/xrpl.js PR: 2812
File: packages/xrpl/test/integration/transactions/payment.test.ts:41-43
Timestamp: 2024-10-30T01:05:33.583Z
Learning: In tests involving fee calculations for XRPL, avoid using default fee amounts. If unable to retrieve fee values from the latest validated ledger, throw an error instead of defaulting to a specific amount.
Learnt from: Patel-Raj11
Repo: XRPLF/xrpl.js PR: 3117
File: packages/xrpl/src/models/transactions/common.ts:1133-1180
Timestamp: 2025-10-28T14:16:24.585Z
Learning: The MPTokenMetadata TypeScript interface in xrpl.js reflects the XLS-89 standard (long-form fields only) to provide correct type hints for IDEs and TypeScript users, even though JavaScript users may provide non-standard structures with compact fields or both forms present. This design provides proper tooling support while maintaining runtime flexibility.
Learnt from: Patel-Raj11
Repo: XRPLF/xrpl.js PR: 3117
File: packages/xrpl/src/models/transactions/common.ts:1133-1180
Timestamp: 2025-10-28T14:16:24.585Z
Learning: In xrpl.js MPTokenMetadata handling: encodeMPTokenMetadata and decodeMPTokenMetadata are designed to preserve the original structure of the input, only converting between compact and long forms when a single form is present (not both). If both compact (e.g., 'us') and long (e.g., 'uris') forms exist in the input, both are preserved in the output. This is intentional to avoid modifying the user's data. Validation (validateMPTokenMetadata) separately checks and reports XLS-89 compliance.
Learnt from: ckeshava
Repo: XRPLF/xrpl.js PR: 3027
File: packages/xrpl/src/models/ledger/MPTokenIssuance.ts:13-16
Timestamp: 2025-06-26T17:25:36.429Z
Learning: In the XRPL ecosystem, type choices for amount fields (like `number` vs `string`) in ledger objects such as `LockedAmount` vs `OutstandingAmount` in `MPTokenIssuance` are deliberate design decisions made across multiple products and cannot be changed unilaterally by individual contributors.
Collaborator
Author
There was a problem hiding this comment.
To enforce the non-XRP currency check, I will need to do the following:
- Check for all possible case typings of XRP in the
currencyfield of theAsset. - Check for the non-standard currency code for all the case-sensitive typing of the XRP asset.
a. This increases the complexity of the validation.
b. This check is already performed on every Clio and rippled server.
This validation is not a part of the changes introduced in XLS-82D MPT-DEX. I would prefer to retain the validation code as-is since all incorrect transactions will be rejected by the rippled server
Contributor
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!
Comment on lines
+281
to
+291
| if (pathStep.mpt_issuance_id !== undefined) { | ||
| if (typeof pathStep.mpt_issuance_id !== 'string') { | ||
| return false | ||
| } | ||
|
|
||
| if (!/^[A-F0-9]{48}$/iu.test(pathStep.mpt_issuance_id)) { | ||
| return false | ||
| } | ||
|
|
||
| return true | ||
| } |
Contributor
There was a problem hiding this comment.
Validate mpt_issuance_id whenever present and reject mixed fields.
Right now, any path step with currency/issuer returns true before mpt_issuance_id is checked. That means invalid mpt_issuance_id values pass if currency/issuer are set, and it also allows mpt_issuance_id to coexist with currency/issuer (likely invalid per PathSet rules).
🐛 Proposed fix
function isPathStep(pathStep: Record<string, unknown>): boolean {
if (pathStep.account !== undefined && typeof pathStep.account !== 'string') {
return false
}
if (
pathStep.currency !== undefined &&
typeof pathStep.currency !== 'string'
) {
return false
}
if (pathStep.issuer !== undefined && typeof pathStep.issuer !== 'string') {
return false
}
- if (
- pathStep.account !== undefined &&
- pathStep.currency === undefined &&
- pathStep.issuer === undefined
- ) {
- return true
- }
- if (pathStep.currency !== undefined || pathStep.issuer !== undefined) {
- return true
- }
- if (pathStep.mpt_issuance_id !== undefined) {
- if (typeof pathStep.mpt_issuance_id !== 'string') {
- return false
- }
-
- if (!/^[A-F0-9]{48}$/iu.test(pathStep.mpt_issuance_id)) {
- return false
- }
-
- return true
- }
- return false
+ const hasCurrencyOrIssuer =
+ pathStep.currency !== undefined || pathStep.issuer !== undefined
+ const hasMpt = pathStep.mpt_issuance_id !== undefined
+
+ if (hasMpt) {
+ if (typeof pathStep.mpt_issuance_id !== 'string') {
+ return false
+ }
+ if (!/^[A-F0-9]{48}$/iu.test(pathStep.mpt_issuance_id)) {
+ return false
+ }
+ if (hasCurrencyOrIssuer) {
+ return false
+ }
+ }
+
+ if (pathStep.account !== undefined && !hasCurrencyOrIssuer && !hasMpt) {
+ return true
+ }
+ if (hasCurrencyOrIssuer || hasMpt) {
+ return true
+ }
+ return false
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/xrpl/src/models/transactions/payment.ts` around lines 281 - 291,
Ensure any path step that includes mpt_issuance_id is always validated and never
allowed to coexist with currency/issuer: when pathStep.mpt_issuance_id is
present, first reject if pathStep.currency or pathStep.issuer are defined, then
validate type is string and that it matches /^[A-F0-9]{48}$/iu, returning false
on any failure and true only if all checks pass; conversely, if currency/issuer
are present ensure mpt_issuance_id is absent before returning true. Use the
existing pathStep.mpt_issuance_id, pathStep.currency and pathStep.issuer checks
in the validation function to enforce these mutual-exclusion and validation
rules.
Collaborator
Author
There was a problem hiding this comment.
mpt_issuance_id can coexist with issuer field only. It cannot be passed with currency or the account fields. To reduce the complexity of the validation code, I feel we can leave the code as is. If there is an error in the mpt_issuance_id field, it will fail in the Payment engine
Contributor
There was a problem hiding this comment.
@ckeshava, thank you for the clarification! That's an important distinction—I understand now that mpt_issuance_id is used instead of currency but can coexist with the issuer field.
I appreciate your pragmatic approach to keeping the validation logic simpler and relying on the Payment engine for comprehensive validation. This aligns with the pattern we've seen in other areas of the codebase where complex validation is deferred to rippled.
One suggestion: since this combination rule is non-obvious, would you consider adding a brief comment in the code documenting that mpt_issuance_id can appear with issuer but not with currency or account? This would help future maintainers understand the intended behavior.
✏️ Learnings added
Learnt from: ckeshava
Repo: XRPLF/xrpl.js PR: 3214
File: packages/xrpl/src/models/transactions/payment.ts:281-291
Timestamp: 2026-02-25T19:37:29.955Z
Learning: In PathStep validation within `packages/xrpl/src/models/transactions/payment.ts`, the `mpt_issuance_id` field can coexist with the `issuer` field in a path step, but cannot be present alongside `currency` or `account` fields. This reflects the XLS-82D specification for MPT-DEX PathSet support.
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.
🧠 Learnings used
Learnt from: shawnxie999
Repo: XRPLF/xrpl.js PR: 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.
Learnt from: mvadari
Repo: XRPLF/xrpl.js PR: 2690
File: packages/xrpl/tools/generateModels.js:52-52
Timestamp: 2024-10-02T15:47:02.491Z
Learning: In `generateModels.js`, the regex used to match `SubmittableTransaction` in `transaction.ts` is expected to always succeed because the pattern is present in the source code. If it fails, the code needs to be updated.
Learnt from: mvadari
Repo: XRPLF/xrpl.js PR: 2690
File: packages/xrpl/tools/generateModels.js:52-52
Timestamp: 2024-10-08T16:29:11.194Z
Learning: In `generateModels.js`, the regex used to match `SubmittableTransaction` in `transaction.ts` is expected to always succeed because the pattern is present in the source code. If it fails, the code needs to be updated.
Learnt from: ckeshava
Repo: XRPLF/xrpl.js PR: 3081
File: packages/xrpl/src/models/transactions/MPTokenIssuanceCreate.ts:298-298
Timestamp: 2025-10-30T12:09:55.784Z
Learning: In XRPL's MPTokenIssuanceCreate transaction validation, MaximumAmount must be greater than 0 (not greater than or equal to). The rippled C++ implementation at MPTokenIssuanceCreate.cpp explicitly rejects MaximumAmount == 0 with temMALFORMED, so the JavaScript validation should use `<= 0` to match this behavior.
Learnt from: ckeshava
Repo: XRPLF/xrpl.js PR: 3032
File: packages/xrpl/src/models/transactions/common.ts:689-698
Timestamp: 2025-07-10T22:04:59.994Z
Learning: In the XRPL.js library, the validateDomainID function in `packages/xrpl/src/models/transactions/common.ts` should only validate the length (64 characters) of domain IDs, not hex encoding. The rippled C++ implementation does not enforce any regex check on domain ID values, so additional hex validation is not required in the JS implementation.
Learnt from: achowdhry-ripple
Repo: XRPLF/xrpl.js PR: 2661
File: packages/xrpl/src/models/transactions/MPTokenIssuanceCreate.ts:69-102
Timestamp: 2024-12-05T16:48:12.951Z
Learning: When adding validation in `validate*` functions in `packages/xrpl/src/models/transactions/`, utilize existing helper functions (e.g., `validateOptionalField`, `validateType`, `isNumber`, `isInteger`) for type checking and validation where appropriate.
Learnt from: ckeshava
Repo: XRPLF/xrpl.js PR: 2873
File: packages/xrpl/src/models/transactions/trustSet.ts:33-36
Timestamp: 2025-01-08T13:08:52.688Z
Learning: For trust-set transactions in XRPL, validation rules for flags should be implemented comprehensively rather than cherry-picking specific rules, as there are many interdependent validation rules associated with these flags.
Learnt from: Patel-Raj11
Repo: XRPLF/xrpl.js PR: 3117
File: packages/xrpl/src/models/transactions/common.ts:1133-1180
Timestamp: 2025-10-28T14:16:24.585Z
Learning: The MPTokenMetadata TypeScript interface in xrpl.js reflects the XLS-89 standard (long-form fields only) to provide correct type hints for IDEs and TypeScript users, even though JavaScript users may provide non-standard structures with compact fields or both forms present. This design provides proper tooling support while maintaining runtime flexibility.
Learnt from: ckeshava
Repo: XRPLF/xrpl.js PR: 2874
File: packages/xrpl/test/integration/transactions/permissionedDomain.test.ts:25-80
Timestamp: 2025-01-08T02:12:28.489Z
Learning: The rippled C++ implementation (PR `#5161`) includes comprehensive test coverage for PermissionedDomain (XLS-80d) error cases. The JS SDK tests focus on the happy path since the error cases are already validated at the rippled level, following the principle of not duplicating complex validation testing across SDK implementations.
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/xrpl/test/models/offerCreate.test.ts (1)
345-385: Consider adding symmetric negative tests forTakerPaysMPT validation.The invalid-ID tests (non-hex, too-short, too-long) only exercise
TakerGets. Corresponding cases forTakerPaysare absent, leaving an asymmetry in test coverage for the validation path.✏️ Example additions for TakerPays invalid MPT coverage
assertInvalid(offerTx, 'OfferCreate: invalid TakerGets') }) + + it(`throws w/ MPT TakerPays mpt_issuance_id contains non-hex characters`, function () { + const offerTx = { + Account: 'r3rhWeE31Jt5sWmi4QiGLMZnY3ENgqw96W', + TakerGets: '12928290425', + TakerPays: { + mpt_issuance_id: 'Z'.repeat(MPTID_LENGTH), + value: '30', + }, + TransactionType: 'OfferCreate', + } as any + + assertInvalid(offerTx, 'OfferCreate: invalid TakerPays') + }) + + it(`throws w/ MPT TakerPays mpt_issuance_id too short`, function () { + const offerTx = { + Account: 'r3rhWeE31Jt5sWmi4QiGLMZnY3ENgqw96W', + TakerGets: '12928290425', + TakerPays: { + mpt_issuance_id: 'A'.repeat(MPTID_LENGTH - 1), + value: '30', + }, + TransactionType: 'OfferCreate', + } as any + + assertInvalid(offerTx, 'OfferCreate: invalid TakerPays') + }) + + it(`throws w/ MPT TakerPays mpt_issuance_id too long`, function () { + const offerTx = { + Account: 'r3rhWeE31Jt5sWmi4QiGLMZnY3ENgqw96W', + TakerGets: '12928290425', + TakerPays: { + mpt_issuance_id: 'A'.repeat(MPTID_LENGTH + 1), + value: '30', + }, + TransactionType: 'OfferCreate', + } as any + + assertInvalid(offerTx, 'OfferCreate: invalid TakerPays') + }) })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/xrpl/test/models/offerCreate.test.ts` around lines 345 - 385, Add three mirror tests for TakerPays MPT validation that match the existing TakerGets negative cases: create tests in the same suite that build offerTx objects with TransactionType 'OfferCreate' and a valid TakerGets (e.g., numeric string) while setting TakerPays to an object containing mpt_issuance_id with (1) non-hex characters using 'Z'.repeat(MPTID_LENGTH), (2) too short using 'A'.repeat(MPTID_LENGTH - 1), and (3) too long using 'A'.repeat(MPTID_LENGTH + 1); for each test call assertInvalid(offerTx, 'OfferCreate: invalid TakerPays') so the validation for TakerPays MPT IDs is exercised just like the existing TakerGets tests.
🤖 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/test/models/offerCreate.test.ts`:
- Around line 345-385: Add three mirror tests for TakerPays MPT validation that
match the existing TakerGets negative cases: create tests in the same suite that
build offerTx objects with TransactionType 'OfferCreate' and a valid TakerGets
(e.g., numeric string) while setting TakerPays to an object containing
mpt_issuance_id with (1) non-hex characters using 'Z'.repeat(MPTID_LENGTH), (2)
too short using 'A'.repeat(MPTID_LENGTH - 1), and (3) too long using
'A'.repeat(MPTID_LENGTH + 1); for each test call assertInvalid(offerTx,
'OfferCreate: invalid TakerPays') so the validation for TakerPays MPT IDs is
exercised just like the existing TakerGets tests.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
packages/xrpl/test/models/AMMClawback.test.tspackages/xrpl/test/models/AMMCreate.test.tspackages/xrpl/test/models/AMMDeposit.test.tspackages/xrpl/test/models/AMMWithdraw.test.tspackages/xrpl/test/models/checkCash.test.tspackages/xrpl/test/models/checkCreate.test.tspackages/xrpl/test/models/offerCreate.test.tspackages/xrpl/test/models/payment.test.tspackages/xrpl/test/testUtils.ts
🚧 Files skipped from review as they are similar to previous changes (6)
- packages/xrpl/test/models/checkCreate.test.ts
- packages/xrpl/test/models/AMMClawback.test.ts
- packages/xrpl/test/models/AMMDeposit.test.ts
- packages/xrpl/test/models/AMMWithdraw.test.ts
- packages/xrpl/test/testUtils.ts
- packages/xrpl/test/models/AMMCreate.test.ts
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
packages/ripple-binary-codec/test/path-set.test.ts (1)
130-130:⚠️ Potential issue | 🟡 MinorFix typo in test description: "mpt_issaunce_id" → "mpt_issuance_id".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/ripple-binary-codec/test/path-set.test.ts` at line 130, Update the test description string in the failing test case: change the typo "mpt_issaunce_id" to "mpt_issuance_id" inside the it(...) call (the test titled `Two paths with mpt_issaunce_id hops`) in path-set.test.ts so the description reads `Two paths with mpt_issuance_id hops`.
🤖 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/ripple-binary-codec/test/path-set.test.ts`:
- Line 97: Fix the minor typo in the comment near the 'FF' token in the test
file: change "seperated" to "separated" in the comment that reads "'FF' /* Two
path elements are seperated by 0xFF type code */" in
packages/ripple-binary-codec/test/path-set.test.ts (look for the 'FF' string in
the PathSet tests).
---
Duplicate comments:
In `@packages/ripple-binary-codec/test/path-set.test.ts`:
- Line 130: Update the test description string in the failing test case: change
the typo "mpt_issaunce_id" to "mpt_issuance_id" inside the it(...) call (the
test titled `Two paths with mpt_issaunce_id hops`) in path-set.test.ts so the
description reads `Two paths with mpt_issuance_id hops`.
| expect(serializedHexRepr).toEqual( | ||
| '20' /* Issuer has a type code of 20 */ + | ||
| AccountID.from(issuerAccount).toHex().toUpperCase() + | ||
| 'FF' /* Two path elements are seperated by 0xFF type code */ + |
Contributor
There was a problem hiding this comment.
Minor comment typo: "seperated" → "separated".
Line 97 comment spells it incorrectly; line 142 in the same file already spells it correctly.
✏️ Fix
- 'FF' /* Two path elements are seperated by 0xFF type code */ +
+ 'FF' /* Two path elements are separated by 0xFF type code */ +📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| 'FF' /* Two path elements are seperated by 0xFF type code */ + | |
| 'FF' /* Two path elements are separated by 0xFF type code */ + |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/ripple-binary-codec/test/path-set.test.ts` at line 97, Fix the minor
typo in the comment near the 'FF' token in the test file: change "seperated" to
"separated" in the comment that reads "'FF' /* Two path elements are seperated
by 0xFF type code */" in packages/ripple-binary-codec/test/path-set.test.ts
(look for the 'FF' string in the PathSet tests).
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
packages/xrpl/src/models/transactions/AMMClawback.ts (2)
139-143:⚠️ Potential issue | 🟡 MinorTypo in error message: "Amount.issuer must match Amount.issuer"
Should read
Asset.issueron the right-hand side.Proposed fix
if (tx.Amount.issuer !== asset.issuer) { throw new ValidationError( - 'AMMClawback: Amount.issuer must match Amount.issuer', + 'AMMClawback: Amount.issuer must match Asset.issuer', ) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/xrpl/src/models/transactions/AMMClawback.ts` around lines 139 - 143, The error message thrown in AMMClawback when tx.Amount.issuer and asset.issuer differ contains a typo; update the ValidationError message in the throw inside AMMClawback.ts so it reads that Amount.issuer must match Asset.issuer (e.g., "AMMClawback: Amount.issuer must match Asset.issuer") to correctly reference asset.issuer and aid clarity when the check fails.
128-144:⚠️ Potential issue | 🟠 MajorMissing cross-validation for MPT: Amount.mpt_issuance_id should match Asset.mpt_issuance_id.
For
IssuedCurrencyassets, you validate thatAmount.currencyandAmount.issuermatch theAssetfields. The analogous check is absent when bothAssetisMPTCurrencyandAmountisMPTAmount— a mismatchedmpt_issuance_idbetween the two will silently pass validation.Proposed fix — add an MPT branch
if ( tx.Amount != null && isIssuedCurrencyAmount(tx.Amount) && isIssuedCurrency(asset) ) { if (tx.Amount.currency !== asset.currency) { throw new ValidationError( 'AMMClawback: Amount.currency must match Asset.currency', ) } if (tx.Amount.issuer !== asset.issuer) { throw new ValidationError( - 'AMMClawback: Amount.issuer must match Amount.issuer', + 'AMMClawback: Amount.issuer must match Asset.issuer', ) } } + + if ( + tx.Amount != null && + isMPTAmount(tx.Amount) && + !isIssuedCurrency(asset) && + 'mpt_issuance_id' in (asset as MPTCurrency) + ) { + if (tx.Amount.mpt_issuance_id !== (asset as MPTCurrency).mpt_issuance_id) { + throw new ValidationError( + 'AMMClawback: Amount.mpt_issuance_id must match Asset.mpt_issuance_id', + ) + } + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/xrpl/src/models/transactions/AMMClawback.ts` around lines 128 - 144, Add a parallel branch in AMMClawback.ts that, when tx.Amount is an MPT amount and asset is an MPT currency (e.g. use isMPTAmount(tx.Amount) && isMPTCurrency(asset) or the project’s equivalent type checks), verifies tx.Amount.mpt_issuance_id === asset.mpt_issuance_id and throws a ValidationError (e.g. 'AMMClawback: Amount.mpt_issuance_id must match Asset.mpt_issuance_id') if they differ; place this check alongside the existing issued-currency checks that use isIssuedCurrencyAmount/isIssuedCurrency so mismatched MPT issuance IDs fail validation.
♻️ Duplicate comments (2)
packages/xrpl/src/models/transactions/AMMClawback.ts (1)
106-122:isCurrencypermits bare XRP ({ currency: 'XRP' }) forAsset, which should be rejected.This was already flagged in a prior review. AMMClawback's
Assetmust be either an issued currency (withissuer) or an MPT — native XRP should not be valid here. TheisIssuedCurrency(asset)guard on Line 110 simply skips the issuer checks for XRP, letting it through silently.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/xrpl/src/models/transactions/AMMClawback.ts` around lines 106 - 122, The current validation lets bare native XRP through because isCurrency permits { currency: 'XRP' } and the isIssuedCurrency check only skips issuer-specific checks; fix this in AMMClawback by rejecting native XRP explicitly: after validateRequiredField(tx, 'Asset', isCurrency) and const asset = tx.Asset, add a guard that throws a ValidationError when asset.currency === 'XRP' and asset.issuer is undefined (i.e., a bare XRP object), ensuring Asset must be an issued currency (isIssuedCurrency(asset)) or the expected MPT form rather than native XRP; keep the existing isIssuedCurrency-based issuer/account checks for issued currencies.packages/xrpl/test/integration/transactions/payment.test.ts (1)
260-261:⚠️ Potential issue | 🟡 MinorStale comment — "two alternative paths" and "Path 2:" still present.
Line 260 still reads "with two alternative paths" and line 261 is prefixed "Path 2:", yet the test only ever builds one path. The previous fix removed the old "Path 1" line but left these fragments behind.
✏️ Suggested correction
- // Cross-currency payment: XRP → MPT_B with two alternative paths - // Path 2: XRP → MPT_B (via XRP/MPT_B pool) + // Cross-currency payment: XRP → MPT_B via XRP/MPT_B AMM pool🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/xrpl/test/integration/transactions/payment.test.ts` around lines 260 - 261, Update the stale inline comment in the payment integration test that still says "with two alternative paths" and prefixes a comment with "Path 2:" despite only building one path; locate the comment strings ("with two alternative paths" and "Path 2:") in the test (payment.test.ts) and either remove the "Path 2:" prefix and change "with two alternative paths" to accurately describe the single path (e.g., "single alternative path" or just "Path:"), or add the missing second path construction if that was intended—adjust the surrounding comment to correctly reflect the actual test flow in the test case.
🧹 Nitpick comments (1)
packages/xrpl/test/integration/transactions/payment.test.ts (1)
249-258: Sender MPT_B authorization is likely unnecessary here.
testContext.walletsends XRP; the AMM pool converts and delivers MPT_B directly todestination. The sender never holds MPT_B at any point in the path, soauthSenderMptBappears redundant and implies a requirement that doesn't exist for this payment pattern. Removing it also reduces the number of network roundtrips in the test.Please confirm against the XLS-82D spec / rippled implementation whether a sender authorization for the destination-currency MPT is required when the sender's source currency is XRP and the conversion is fully handled by an AMM.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/xrpl/test/integration/transactions/payment.test.ts` around lines 249 - 258, Remove the unnecessary sender authorization: delete the MPTokenAuthorize object authSenderMptB and the subsequent await testTransaction(...) invocation that uses it (symbols: authSenderMptB, MPTokenAuthorize, testTransaction, testContext.wallet, mptIdB), since the sender transmits XRP and the AMM performs conversion/delivery to destination so sender-side MPT_B authorization is not required; run the test and, if desired, add a short comment referencing XLS-82D/rippled verification that sender authorization for the destination token is not needed when conversion is fully handled by an AMM.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@packages/xrpl/src/models/transactions/AMMClawback.ts`:
- Around line 139-143: The error message thrown in AMMClawback when
tx.Amount.issuer and asset.issuer differ contains a typo; update the
ValidationError message in the throw inside AMMClawback.ts so it reads that
Amount.issuer must match Asset.issuer (e.g., "AMMClawback: Amount.issuer must
match Asset.issuer") to correctly reference asset.issuer and aid clarity when
the check fails.
- Around line 128-144: Add a parallel branch in AMMClawback.ts that, when
tx.Amount is an MPT amount and asset is an MPT currency (e.g. use
isMPTAmount(tx.Amount) && isMPTCurrency(asset) or the project’s equivalent type
checks), verifies tx.Amount.mpt_issuance_id === asset.mpt_issuance_id and throws
a ValidationError (e.g. 'AMMClawback: Amount.mpt_issuance_id must match
Asset.mpt_issuance_id') if they differ; place this check alongside the existing
issued-currency checks that use isIssuedCurrencyAmount/isIssuedCurrency so
mismatched MPT issuance IDs fail validation.
---
Duplicate comments:
In `@packages/xrpl/src/models/transactions/AMMClawback.ts`:
- Around line 106-122: The current validation lets bare native XRP through
because isCurrency permits { currency: 'XRP' } and the isIssuedCurrency check
only skips issuer-specific checks; fix this in AMMClawback by rejecting native
XRP explicitly: after validateRequiredField(tx, 'Asset', isCurrency) and const
asset = tx.Asset, add a guard that throws a ValidationError when asset.currency
=== 'XRP' and asset.issuer is undefined (i.e., a bare XRP object), ensuring
Asset must be an issued currency (isIssuedCurrency(asset)) or the expected MPT
form rather than native XRP; keep the existing isIssuedCurrency-based
issuer/account checks for issued currencies.
In `@packages/xrpl/test/integration/transactions/payment.test.ts`:
- Around line 260-261: Update the stale inline comment in the payment
integration test that still says "with two alternative paths" and prefixes a
comment with "Path 2:" despite only building one path; locate the comment
strings ("with two alternative paths" and "Path 2:") in the test
(payment.test.ts) and either remove the "Path 2:" prefix and change "with two
alternative paths" to accurately describe the single path (e.g., "single
alternative path" or just "Path:"), or add the missing second path construction
if that was intended—adjust the surrounding comment to correctly reflect the
actual test flow in the test case.
---
Nitpick comments:
In `@packages/xrpl/test/integration/transactions/payment.test.ts`:
- Around line 249-258: Remove the unnecessary sender authorization: delete the
MPTokenAuthorize object authSenderMptB and the subsequent await
testTransaction(...) invocation that uses it (symbols: authSenderMptB,
MPTokenAuthorize, testTransaction, testContext.wallet, mptIdB), since the sender
transmits XRP and the AMM performs conversion/delivery to destination so
sender-side MPT_B authorization is not required; run the test and, if desired,
add a short comment referencing XLS-82D/rippled verification that sender
authorization for the destination token is not needed when conversion is fully
handled by an AMM.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/xrpl/src/models/transactions/AMMClawback.tspackages/xrpl/test/integration/transactions/payment.test.ts
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
packages/xrpl/src/models/transactions/AMMClawback.ts (2)
143-144:⚠️ Potential issue | 🟡 MinorTypo in error message:
Amount.issuer must match Amount.issuer.Both sides of the message say
Amount.issuer; the right-hand side should beAsset.issuer.✏️ Proposed fix
- 'AMMClawback: Amount.issuer must match Amount.issuer', + 'AMMClawback: Amount.issuer must match Asset.issuer',🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/xrpl/src/models/transactions/AMMClawback.ts` around lines 143 - 144, The error message in the AMMClawback validation is wrong: it currently reads "Amount.issuer must match Amount.issuer"; update the message to "Amount.issuer must match Asset.issuer" so it correctly references the expected field. Locate the check in the AMMClawback model (search for the string 'AMMClawback: Amount.issuer must match Amount.issuer' or the validation function within AMMClawback.ts) and replace the right-hand "Amount.issuer" with "Asset.issuer" in the processLogger/throw/error message.
130-160:⚠️ Potential issue | 🔴 CriticalMissing type-mismatch guard:
IssuedCurrencyasset +MPTAmountcombination silently passes.When
assetisIssuedCurrencyandtx.AmountisMPTAmount:
- Lines 130–146 are skipped because
isIssuedCurrencyAmount(tx.Amount)isfalse.- Lines 148–160 are skipped because
isMPTCurrency(asset)isfalse.The invalid combination is accepted. The reverse case (MPT asset +
IssuedCurrencyAmount) is correctly rejected at Line 149, so this gap is asymmetric.🐛 Proposed fix
if ( tx.Amount != null && isIssuedCurrencyAmount(tx.Amount) && isIssuedCurrency(asset) ) { + // amount type is already guarded by isClawbackAmountValid above; + // enforce that asset/amount types are consistent + } else if (tx.Amount != null && isIssuedCurrency(asset) && !isIssuedCurrencyAmount(tx.Amount)) { + throw new ValidationError( + 'AMMClawback: Amount must be an IssuedCurrencyAmount when Asset is an IssuedCurrency', + ) + }Or, more concisely, restructure the
IssuedCurrencyblock:if (tx.Amount != null && isIssuedCurrency(asset)) { + if (!isIssuedCurrencyAmount(tx.Amount)) { + throw new ValidationError( + 'AMMClawback: Amount must be an IssuedCurrencyAmount when Asset is an IssuedCurrency', + ) + } - if ( - tx.Amount != null && - isIssuedCurrencyAmount(tx.Amount) && - isIssuedCurrency(asset) - ) { if (tx.Amount.currency !== asset.currency) { throw new ValidationError( 'AMMClawback: Amount.currency must match Asset.currency', ) } if (tx.Amount.issuer !== asset.issuer) { throw new ValidationError( 'AMMClawback: Amount.issuer must match Asset.issuer', ) } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/xrpl/src/models/transactions/AMMClawback.ts` around lines 130 - 160, In AMMClawback validation, add explicit cross-type guards so an IssuedCurrency asset cannot accept an MPTAmount and an MPTCurrency asset cannot accept an IssuedCurrencyAmount: inside the block that checks tx.Amount and isIssuedCurrency(asset) use isMPTAmount(tx.Amount) to throw a ValidationError when an MPTAmount is provided for an IssuedCurrency asset (and conversely within the isMPTCurrency(asset) branch ensure isIssuedCurrencyAmount(tx.Amount) triggers a ValidationError); update the messages to reference AMMClawback, tx.Amount and the offending type mismatch and keep existing checks (currency/issuer and mpt_issuance_id) intact.
♻️ Duplicate comments (1)
packages/xrpl/src/models/transactions/AMMClawback.ts (1)
108-108:⚠️ Potential issue | 🟠 MajorXRP assets still not rejected for
Asset.
isCurrencyaccepts{ currency: 'XRP' }, so an XRP object passesvalidateRequiredFieldand then falls through both theisIssuedCurrencyandisMPTCurrencybranches with no error.Assetmust be restricted to issued currencies or MPT assets only.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/xrpl/src/models/transactions/AMMClawback.ts` at line 108, validateRequiredField currently uses isCurrency which allows {currency: 'XRP'}; change the Asset validation in AMMClawback.ts to require only issued or MPT assets by replacing the isCurrency predicate with a composite check (e.g. validateRequiredField(tx, 'Asset', v => isIssuedCurrency(v) || isMPTCurrency(v))) or equivalent logic; ensure that Asset that is XRP is rejected and that existing code paths that call isIssuedCurrency and isMPTCurrency remain consistent so a clear error is raised when neither predicate matches.
🧹 Nitpick comments (1)
packages/xrpl/test/models/payment.test.ts (1)
266-316: Missing positive test for thempt_issuance_id+issuervalid combination.The XLS-82D spec (and the project's own validated learning) explicitly permits
mpt_issuance_idto coexist withissuerin a PathStep. There is no test confirming this valid combination is accepted byvalidatePayment, leaving a gap that would let a regression (e.g. accidentally rejectingissueralongsidempt_issuance_id) go undetected.✅ Suggested test to add
+ it(`verifies valid Payment with MPT PathStep containing issuer`, function () { + payment.Paths = [ + [ + { + mpt_issuance_id: 'A'.repeat(MPTID_LENGTH), + issuer: 'r9vbV3EHvXWjSkeQ6CAcYVPGeq7TuiXY2X', + }, + ], + ] + assertValid(payment) + })Based on learnings: "the
mpt_issuance_idfield can coexist with theissuerfield in a path step, but cannot be present alongsidecurrencyoraccountfields."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/xrpl/test/models/payment.test.ts` around lines 266 - 316, Add a positive unit test in payment.test.ts to assert that a PathStep containing both mpt_issuance_id and issuer is valid: set payment.Paths to a nested array with an object containing mpt_issuance_id: 'A'.repeat(MPTID_LENGTH) and a valid issuer string, then call assertValid(payment); this verifies validatePayment (and related logic) accepts mpt_issuance_id alongside issuer while still rejecting combinations like mpt_issuance_id with account or currency.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@packages/xrpl/src/models/transactions/AMMClawback.ts`:
- Around line 143-144: The error message in the AMMClawback validation is wrong:
it currently reads "Amount.issuer must match Amount.issuer"; update the message
to "Amount.issuer must match Asset.issuer" so it correctly references the
expected field. Locate the check in the AMMClawback model (search for the string
'AMMClawback: Amount.issuer must match Amount.issuer' or the validation function
within AMMClawback.ts) and replace the right-hand "Amount.issuer" with
"Asset.issuer" in the processLogger/throw/error message.
- Around line 130-160: In AMMClawback validation, add explicit cross-type guards
so an IssuedCurrency asset cannot accept an MPTAmount and an MPTCurrency asset
cannot accept an IssuedCurrencyAmount: inside the block that checks tx.Amount
and isIssuedCurrency(asset) use isMPTAmount(tx.Amount) to throw a
ValidationError when an MPTAmount is provided for an IssuedCurrency asset (and
conversely within the isMPTCurrency(asset) branch ensure
isIssuedCurrencyAmount(tx.Amount) triggers a ValidationError); update the
messages to reference AMMClawback, tx.Amount and the offending type mismatch and
keep existing checks (currency/issuer and mpt_issuance_id) intact.
---
Duplicate comments:
In `@packages/xrpl/src/models/transactions/AMMClawback.ts`:
- Line 108: validateRequiredField currently uses isCurrency which allows
{currency: 'XRP'}; change the Asset validation in AMMClawback.ts to require only
issued or MPT assets by replacing the isCurrency predicate with a composite
check (e.g. validateRequiredField(tx, 'Asset', v => isIssuedCurrency(v) ||
isMPTCurrency(v))) or equivalent logic; ensure that Asset that is XRP is
rejected and that existing code paths that call isIssuedCurrency and
isMPTCurrency remain consistent so a clear error is raised when neither
predicate matches.
---
Nitpick comments:
In `@packages/xrpl/test/models/payment.test.ts`:
- Around line 266-316: Add a positive unit test in payment.test.ts to assert
that a PathStep containing both mpt_issuance_id and issuer is valid: set
payment.Paths to a nested array with an object containing mpt_issuance_id:
'A'.repeat(MPTID_LENGTH) and a valid issuer string, then call
assertValid(payment); this verifies validatePayment (and related logic) accepts
mpt_issuance_id alongside issuer while still rejecting combinations like
mpt_issuance_id with account or currency.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
packages/xrpl/src/models/transactions/AMMClawback.tspackages/xrpl/src/models/transactions/common.tspackages/xrpl/src/models/transactions/payment.tspackages/xrpl/test/models/AMMClawback.test.tspackages/xrpl/test/models/payment.test.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- packages/xrpl/test/models/AMMClawback.test.ts
- packages/xrpl/src/models/transactions/payment.ts
- packages/xrpl/src/models/transactions/common.ts
| /** | ||
| * Specifies the other asset in the AMM's pool. In JSON, this is an object with currency and | ||
| * issuer fields (omit issuer for XRP). | ||
| * issuer fields (omit issuer for XRP), or mpt_issuance_id for MPT. |
Collaborator
There was a problem hiding this comment.
nit: make this note clearer like Specifies the other asset in the AMM's pool. In JSON, this can be XRP (no issuer), an issued currency (with issuer), or an MPT (with mpt_issuance_id).
Collaborator
Author
There was a problem hiding this comment.
Thanks cybele, I copied this text from opensource.ripple.com (or) xrpl-dev-portal github repository.
If you have other suggestions, feel free to create issues in the above github repo. The xrpl.org documentation is read by a larger number of people.
| TIMEOUT, | ||
| ) | ||
|
|
||
| it( |
Collaborator
There was a problem hiding this comment.
Consider adding a test case when account is undefined but currency is defined
| type: 'check', | ||
| }) | ||
| assert.lengthOf( | ||
| response1.result.account_objects, |
Collaborator
There was a problem hiding this comment.
Maybe add another test with response1.result to verify the contents
Collaborator
There was a problem hiding this comment.
Consider adding a test where MPT is on taker_pays side and XRP on sell side
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
2 participants
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.
High Level Overview of Change
This PR implements the following XLS specification requirements for the xrpl-py client library: XRPLF/XRPL-Standards#231
The reference cpp implementation for rippled can be found here: XRPLF/rippled#5285
This PR includes updates to the transaction and request models to accommodate MPTAmount, integration and unit tests for all the affected transaction-types and RPCs.
The PR also updates the binary-codec of the
PathSetrippled type. The amendment allows formpt_issuance_idinputs in the Path specification.Type of Change
Did you update HISTORY.md?
Test Plan
Appropriate unit and integration tests have been added.