fix(xrpl): use deep amount comparison in handleDeliverMax (#3313)

[satyakwok]

Closes #3313.

Summary

handleDeliverMax in src/sugar/autofill.ts compared the two fields with !== (reference equality). For IOU and MPT amounts — objects like {currency, issuer, value} — two distinct object literals with identical fields always compare not-equal, so every IOU payment that set both `Amount` and `DeliverMax` as separate object literals threw a spurious validation error:

PaymentTransaction: Amount and DeliverMax fields must be identical when both are provided

This silently broke any IOU payment flow that built the two fields independently (e.g. one constructed by the caller, the other autofilled from a higher-level helper).

Fix

Reuse the existing amountsEqual helper from src/client/partialPayment.ts. It already handles all three amount shapes correctly:

• XRP (string drops): ===
• IOU: currency + issuer + BigNumber(value) (so "1.0" and "1" compare equal)
• MPT: mpt_issuance_id + BigNumber(value)

Only change to partialPayment.ts is making amountsEqual exported (was previously file-local). Same implementation, just visible to other modules.

Tests

Two new tests in packages/xrpl/test/client/autofill.test.ts:

1. Regression — identical IOU Amount and DeliverMax as separate object literals → must autofill cleanly. Fails on main (throws the spurious ValidationError), passes after.
2. Companion — different value with same currency / issuer → must still throw. Pins that the fix didn't loosen validation.

The pre-existing tests for the === string path and the same-reference IOU case (paymentTx.DeliverMax = paymentTx.Amount) still pass.

Smoke gate

• npm run build from repo root — all 6 workspaces build clean.
• npx jest --config=jest.config.unit.js --testPathPatterns autofill — 23 passed, 0 failed.

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 5fb9b19c-7ebd-475d-ac0d-98e8d45468ae

📥 Commits

Reviewing files that changed from the base of the PR and between 2636d6c and d1519c0.

📒 Files selected for processing (1)

• packages/xrpl/test/client/autofill.test.ts

✅ Files skipped from review due to trivial changes (1)

• packages/xrpl/test/client/autofill.test.ts

---

Walkthrough

Exports a semantic equality utility amountsEqual, uses it in handleDeliverMax to compare Amount and DeliverMax by value (not reference), and adds regression tests for IOU object comparisons.

Changes

DeliverMax Validation

Layer / File(s)	Summary
Semantic Equality Utility Export packages/xrpl/src/client/partialPayment.ts	Exports amountsEqual with JSDoc documenting cross-type comparison for XRP, IOU, and MPT amounts using BigNumber equality for value.
DeliverMax Validation Logic packages/xrpl/src/sugar/autofill.ts	Imports amountsEqual and replaces reference inequality check with amountsEqual(...) in handleDeliverMax, preserving ValidationError on mismatch and deleting DeliverMax after validation.
Regression Test Coverage packages/xrpl/test/client/autofill.test.ts	Adds two tests: one ensures identical IOU-style Amount and DeliverMax object literals are accepted (and DeliverMax omitted), the other ensures differing value still triggers ValidationError.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Suggested reviewers

• Patel-Raj11
• mvadari
• ckeshava

Poem

🐰 Two carrots, same taste, different bowls,
Objects apart but equal in souls,
amountsEqual hops in to see,
Values matched — let payments be,
A tiny hop fixes ledger goals. ✨

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title clearly summarizes the main change: using deep amount comparison instead of reference equality in handleDeliverMax.
Description check	✅ Passed	The description provides clear context (the bug and why it occurs), explains the fix with implementation details, lists the changes made, and documents test coverage thoroughly.
Linked Issues check	✅ Passed	The PR fully addresses #3313 by fixing the reference equality bug, implementing deep amount comparison for all shapes (XRP/IOU/MPT), and adding regression tests.
Out of Scope Changes check	✅ Passed	All changes are directly scoped to fixing issue #3313: exporting amountsEqual, updating handleDeliverMax logic, and adding targeted regression tests.
Docstring Coverage	✅ Passed	Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

Actionable comments posted: 1

🧹 Nitpick comments (1)

packages/xrpl/test/client/autofill.test.ts (1)

108-131: ⚡ Quick win

Add MPT regression coverage for the same bug path.

Nice IOU coverage. Since issue scope also includes MPT amount objects, add one matching/non-matching MPT pair to guard handleDeliverMax against future regressions there too.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@packages/xrpl/test/client/autofill.test.ts` around lines 108 - 131, Add two
new test cases mirroring the existing IOU tests but using MPT-format amount
objects to cover the MPT code path in handleDeliverMax: one where
paymentTx.Amount and paymentTx.DeliverMax are distinct MPT amount objects with
identical fields (assert autofill returns Amount and drops DeliverMax), and a
companion where the MPT objects differ in value (assert autofill rejects with
ValidationError). Place them alongside the existing "Validate Payment
transaction API v2" tests and reference the same testContext.client.autofill
call and the handleDeliverMax behavior so future changes to handleDeliverMax/MPT
handling are guarded.

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@packages/xrpl/test/client/autofill.test.ts`:
- Around line 109-112: The comment describing the regression is incorrect about
reference-equality behavior; update the wording around the `tx.Amount !==
tx.DeliverMax` mention (in the autofill test) to state that distinct
IssuedCurrencyAmount object literals evaluate true with `!==` (i.e., different
references cause the comparison to be true), and clarify that this caused IOU
payments to be rejected when both fields were set even though their contents
matched; locate the comment that includes `tx.Amount !== tx.DeliverMax` and
rewrite it to correctly describe the inverted result.

---

Nitpick comments:
In `@packages/xrpl/test/client/autofill.test.ts`:
- Around line 108-131: Add two new test cases mirroring the existing IOU tests
but using MPT-format amount objects to cover the MPT code path in
handleDeliverMax: one where paymentTx.Amount and paymentTx.DeliverMax are
distinct MPT amount objects with identical fields (assert autofill returns
Amount and drops DeliverMax), and a companion where the MPT objects differ in
value (assert autofill rejects with ValidationError). Place them alongside the
existing "Validate Payment transaction API v2" tests and reference the same
testContext.client.autofill call and the handleDeliverMax behavior so future
changes to handleDeliverMax/MPT handling are guarded.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 564d4571-aedb-4c46-9c8c-4aa8a759a10c

📥 Commits

Reviewing files that changed from the base of the PR and between d739d47 and 2636d6c.

📒 Files selected for processing (3)

• packages/xrpl/src/client/partialPayment.ts
• packages/xrpl/src/sugar/autofill.ts
• packages/xrpl/test/client/autofill.test.ts

[satyakwok]

Thanks @coderabbitai — both fair.

1. Comment wording (autofill.test.ts:112) — fixed in d1519c0. You're right, the operand semantics were inverted: tx.Amount !== tx.DeliverMax returns true for distinct object references (different identity), which is what tripped the validation throw. Updated the comment to match.

2. MPT regression coverage — held off intentionally. Payment.Amount / DeliverMax are typed as Amount = IssuedCurrencyAmount | string today (see models/common/index.ts — there's a // TODO: add MPTAmount to Amount once MPTv2 is released next to it), so an MPT pair would need an as any cast to compile. The shared amountsEqual helper already has direct MPT coverage in its own call site (partialPayment.ts). Happy to add the Payment MPT coverage as a follow-up the moment MPT lands in the Amount union.

[coderabbitai]

Tip

For best results, initiate chat on the files or code changes.

@satyakwok: Thanks for the detailed follow-up!

1. Comment wording fix — confirmed, that reads correctly now. Appreciated.

2. MPT regression coverage — the reasoning is solid. Since MPTAmount isn't in the Amount union yet and the amountsEqual helper already covers the MPT path at its own call site, deferring this makes sense. Would you like me to open a follow-up GitHub issue to track adding Payment MPT regression tests once MPTv2 lands in the Amount union?

---

✏️ Learnings added

Learnt from: satyakwok
URL: https://github.com/XRPLF/xrpl.js/pull/3338

Timestamp: 2026-05-12T01:04:33.978Z
Learning: In `packages/xrpl/src/models/common/index.ts`, the `Amount` type is currently `IssuedCurrencyAmount | string` (XRP drops as string, or IOU object). There is a `// TODO: add MPTAmount to Amount once MPTv2 is released` comment next to it. As of this PR, `MPTAmount` is NOT included in the `Amount` union, so using an MPT amount for `Payment.Amount` or `Payment.DeliverMax` requires an `as any` cast. MPT coverage for `handleDeliverMax` in `autofill.ts` is intentionally deferred until MPTv2 is released and `MPTAmount` is added to the `Amount` union.

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.