[prajwalu142]

Problem

The Payment explanation returned only Destination, Amount and Fee, so SendMax, Paths and DeliverMin were not visible to consumers.

Changes (modules/sdk-coin-xrp)

• Both explainers (xrp.ts and lib/transaction.ts) now return sendMax, hasPaths and deliverMin when present, and list them in displayOrder.
• Payments without these fields produce the same explanation as before.
• No signing or verification behaviour changes.

Verification

• New unit tests cover explainer output in both explainers and plain payments carrying no extra fields.
• sdk-coin-xrp unit suite: 162 passing. tsc and eslint are clean.

Ticket: CSHLD-2162

[linear-code]

CSHLD-2162

[github-actions]

⚠️ Compatibility checks are failing on Node.js 27-nightly (experimental, non-blocking). This does not block merge, but it signals an incompatibility to fix before this release becomes supported.

View run

[prajwalu142]

@claude review for logical correctness, implementation completeness and regressions

[claude]

Nothing blocking. The comments below are optional suggestions. There is no need to push a fix for them before merging.

[claude]

Claude Code Review

This repository is configured for manual code reviews. Comment @claude review for a one-time review, or @claude review always to subscribe this PR to a review on every future push.

_{Tip: disable this comment in your organization's Code Review settings.}

[mukeshsp]

@claude review

[github-actions]

Claude finished @mukeshsp's task in 24s —— View job

---

Review of #10161

No blocking issues. The change is small and does what the description says. I reviewed the diff against the current HEAD (05c7e45). I did not run the tests; I'm relying on the reported 162 passing.

Correctness

• utils.getPaymentSpendLimits (lib/utils.ts) returns sendMax, hasPaths and deliverMin only when the field is defined. It uses !== undefined, so a legitimate DeliverMin: '0' is still reported.
• Plain payments produce the same explanation as before, because the spread is empty and Object.keys({}) adds nothing to displayOrder.
• displayOrder is built from the same object that is spread into the result, so the two can't drift apart.
• Both explainers (xrp.ts and lib/transaction.ts) use the one helper, so they stay consistent.
• hasPaths is a boolean rather than the raw Paths array. That is a reasonable choice for an explanation and keeps the payload small. If a consumer needs the actual path steps, they would have to decode the transaction themselves.

Tests

• The tests cover the JSON-fallback explainer, the lib-level Transaction explainer, and the plain-payment case with no extra fields.
• The lib-level test omits DeliverMin, because xrpl rejects it without tfPartialPayment. So deliverMin is only tested through the xrp.ts path. Adding a lib-level case with tfPartialPayment set would close that gap. This is optional.
• The test doesn't cover a partial payment that has both deliverMin and delivered_amount in the meta. That seems fine, since the two code paths are independent.

Regressions

• This is a purely additive change to the explanation output, and signing and verification are untouched.
• The new optional fields on PaymentTransactionExplanation don't break existing consumers.
• One caution: any consumer that treats displayOrder as an exhaustive list of known keys will now see up to three new entries for cross-currency payments.

Earlier inline comments are stale

• The two previous inline comments refer to assertNoCrossCurrencyPaymentFields and utils.hasCrossCurrencyPaymentFields. Neither exists in this branch (no matches in modules/sdk-coin-xrp), so those comments were written against different code.
• The underlying concern is still reasonable but separate from this PR. signTransaction has no transaction-type allow-list and no cross-currency Payment guard. This PR only makes SendMax, Paths and DeliverMin visible, which matches its title and description ("No signing or verification behaviour changes"). I'd track a signing guard as a follow-up ticket rather than fold it in here.

Optional

• Add a short doc comment on the three new interface fields, as partialPayment already has. For example, say what hasPaths means.

[claude]

Nothing blocking. The comments below are optional suggestions. There is no need to push a fix for them before merging.

Still open from earlier reviews (1):

• Unresolved: 1 minor or pre-existing.

Additional findings (outside the current diff — GitHub can't attach inline comments there):

• 🟣 modules/sdk-coin-xrp/src/xrp.ts — Pre-existing, security-relevant: a send can still pass verification when the prebuild spends a different asset than the one being sent. verifyTransaction now receives sendMax and hasPaths from the explainer but never reads them. It rejects only partialPayment (xrp.ts:552) and compares Destination and Amount (xrp.ts:583). Fix: for verified sends, reject hasPaths, deliverMin, and any sendMax whose asset differs from Amount. Keep accepting the same-mpt_issuance_id SendMax that mptTransferBuilder.ts:132 sets to cover the MPT TransferFee.

  Why this was flagged

  The trigger is a txPrebuild.txHex from the server (or a tampered one) for a payment send. Its Destination and Amount match txParams.recipients[0]. It also carries SendMax set to the wallet's whole issued-currency balance (for example 100000 RLUSD) and Paths routed through offers the attacker controls. Xrp.verifyTransaction (xrp.ts:514) calls explainTransaction, which now returns sendMax and hasPaths: true (xrp.ts:356, xrp.ts:384). verifyTransaction only checks partialPayment (xrp.ts:552) and then runs the comparator on outputs[0] (xrp.ts:574-585), so it returns true. Once signed, the payment can spend up to SendMax of the other asset to deliver Amount, which drains that balance into the attacker's offers. The base branch behaves the same way: it has no SendMax/Paths check, and the PR says verification is unchanged. But this PR is what puts these fields into the explanation that verifyTransaction already reads. The partial-payment rejection follows the same logic ("BitGo never builds partial payments"), and BitGo's own builders never set Paths or a cross-asset SendMax.

  Verification: Nothing in verifyTransaction reads sendMax, hasPaths or deliverMin. It rejects only explanation.partialPayment === true (xrp.ts:552-554) and then runs comparator(output, expectedOutput) (xrp.ts:583), which checks only the address and the numeric value of Amount. So a cross-currency Payment that can spend up to SendMax of another asset passes verification. Base 6aef749 has the same verifyTransaction body.