fix: Add assorted MPT/DEX fixes - #7040
Conversation
Replace checkMPTTxAllowed() with canMPTTradeAndTransfer().
Co-authored-by: xrplf-ai-reviewer[bot] <266832837+xrplf-ai-reviewer[bot]@users.noreply.github.com>
Co-authored-by: Shawn Xie <35279399+shawnxie999@users.noreply.github.com>
* Combine frozen and reqAuth. * Check senders and receivers for reqAuth.
… not checking the reserves when creating MPToken or Trustline on partial offer crossing.
…Step. Extend unit-tests.
…it authorize/unauthorize of this token.
Add ClawbackMPT_test.
![xrplf-ai-reviewer[bot]](https://avatars.githubusercontent.com/u/67929741?s=60&v=4){kind=small}
|
All conflicts have been resolved. Assigned reviewers can now start or resume their review. |
gregtatcam
added
the
Ready to merge
|
This PR has conflicts, please resolve them in order for the PR to be reviewed. |
bthomee
changed the title
|
All conflicts have been resolved. Assigned reviewers can now start or resume their review. |
There was a problem hiding this comment.
Pull request overview
This PR tightens MPToken (MPT) semantics across DEX/path execution and AMM flows, adds new invariant enforcement, and expands test coverage to lock in the corrected behavior (transfer-fee charging, lock/freeze handling, trade/transfer permissions, and AMM bookkeeping).
Changes:
- Refactors frozen/locked checks to return correct TERs (e.g.,
tecLOCKEDfor locked MPT) and aligns transactors/steps with updated MPT permission rules. - Adds a new
ValidMPTTransferinvariant to prevent invalid MPT holder-to-holder transfers (flags/freeze/auth), and extends AMM invariants to treat AMM-held MPT as pool state. - Expands/adjusts unit tests for Offer/Payment pathing, Checks, AMM behavior, invariants, and adds a new Clawback MPT test suite.
Reviewed changes
Copilot reviewed 26 out of 26 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| src/test/app/OfferMPT_test.cpp | Adds regression tests for BookStep MPT transfer-fee charging and reserve-less auto-create behavior during partial crossing. |
| src/test/app/MPToken_test.cpp | Updates expected TERs/behaviors for locked MPT, Check create/cash rules, AMM semantics, and adds new tests for trade/transfer + owner count fix. |
| src/test/app/Invariants_test.cpp | Adds invariant tests for invalid MPT transfers and extends AMM invariant coverage to include MPT pool cases. |
| src/test/app/ClawbackMPT_test.cpp | New test suite validating MPT clawback behavior (permissions, locking, tickets, edge cases). |
| src/test/app/CheckMPT_test.cpp | Updates Check/MPT tests to expect tecLOCKED instead of tecFROZEN when locked. |
| src/test/app/AMMMPT_test.cpp | Updates AMM/MPT tests for locked semantics and revised permission TERs; adds withdraw-when-transfer-disabled scenario. |
| src/test/app/AMMExtendedMPT_test.cpp | Updates AMM extended tests to reflect new locked/unfunded behavior for offers and AMM creation. |
| src/libxrpl/tx/transactors/token/MPTokenAuthorize.cpp | Prevents (un)authorization of AMM pseudo-accounts (implicitly authorized). |
| src/libxrpl/tx/transactors/dex/OfferCreate.cpp | Uses checkGlobalFrozen and adds MPT freeze checks in accept-asset validation. |
| src/libxrpl/tx/transactors/dex/AMMWithdraw.cpp | Refactors freeze/lock checks and removes extra owner-count adjustment to fix double increment. |
| src/libxrpl/tx/transactors/dex/AMMDeposit.cpp | Refactors freeze/lock checks and replaces checkMPTTxAllowed with canMPTTradeAndTransfer. |
| src/libxrpl/tx/transactors/dex/AMMCreate.cpp | Refactors freeze/lock checks, replaces checkMPTTxAllowed, and implicitly authorizes AMM pseudo-account MPToken holdings. |
| src/libxrpl/tx/transactors/check/CheckCreate.cpp | Treats checks as non-DEX; enforces MPT transfer permission instead of trade permission. |
| src/libxrpl/tx/transactors/check/CheckCash.cpp | Switches MPT permission check from canTrade to canTransfer for checks. |
| src/libxrpl/tx/paths/MPTEndpointStep.cpp | Adjusts freeze/lock checking logic for path calculation consistency. |
| src/libxrpl/tx/paths/BookStep.cpp | Tightens canTrade checking and fixes MPT rate/parity logic to ensure transfer fees apply correctly. |
| src/libxrpl/tx/invariants/MPTInvariant.cpp | Adds new ValidMPTTransfer invariant and refactors enforcement gating under MPTokensV2. |
| src/libxrpl/tx/invariants/InvariantCheck.cpp | Fixes ValidClawback accounting for MPT by checking the holder side. |
| src/libxrpl/tx/invariants/AMMInvariant.cpp | Treats AMM-held MPToken changes as pool changes and improves invariant log clarity. |
| src/libxrpl/protocol/STPathSet.cpp | Enforces stricter STPathSet decoding (rejects invalid MPT+Currency element). |
| src/libxrpl/ledger/helpers/TokenHelpers.cpp | Adds checkGlobalFrozen/checkIndividualFrozen helpers and adds MPT amount overflow check in direct send. |
| src/libxrpl/ledger/helpers/MPTokenHelpers.cpp | Extends implicit pseudo-account authorization to AMM (under feature gating) and adds canMPTTradeAndTransfer. |
| include/xrpl/tx/invariants/MPTInvariant.h | Declares new ValidMPTTransfer invariant type. |
| include/xrpl/tx/invariants/InvariantCheck.h | Adds ValidMPTTransfer to the public invariant check tuple. |
| include/xrpl/ledger/helpers/TokenHelpers.h | Declares new checkGlobalFrozen and checkIndividualFrozen helpers. |
| include/xrpl/ledger/helpers/MPTokenHelpers.h | Declares canMPTTradeAndTransfer and removes the old checkMPTTxAllowed declaration. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // pure issue/redeem can't be frozen (issuer/holder) | ||
| // For the first step: check global freeze of the step's own asset and of | ||
| // the strand's deliver asset (if a DEX book follows and the source is not | ||
| // the deliver-asset issuer). For the last step: check only the per-holder | ||
| // MPToken lock. Global freeze of the deliver asset is not checked here | ||
| // because MPT semantics allow issuer<->holder transfers even when globally | ||
| // locked — only holder-to-holder DEX paths are restricted. | ||
| if (!(ctx.isLast && ctx.isFirst)) | ||
| { | ||
| auto const& account = ctx.isFirst ? src_ : dst_; | ||
| if (isFrozen(ctx.view, account, mptIssue_)) | ||
| bool const frozen = (ctx.isFirst && isGlobalFrozen(ctx.view, mptIssue_)) || | ||
| isIndividualFrozen(ctx.view, account, mptIssue_); | ||
| if (frozen) | ||
| return terLOCKED; | ||
| } |
There was a problem hiding this comment.
I don’t think this change is needed.
For direct MPT payments, the vault-share inherited freeze/lock is already enforced during execution via MPTEndpointPaymentStep::check() (MPTEndpointStep.cpp:362), which calls isFrozen(...) and therefore includes the vault pseudo-account/reference-holding logic.
For the cross-currency case, I added a test for the stale-offer scenario: create a vault-share offer while the underlying asset is unlocked, then lock the underlying asset, then attempt an XRP -> vault-share payment through that offer. The payment does not consume the offer. It returns tecPATH_PARTIAL, the share balances remain unchanged, and the stale offer remains on book.
So the path may be constructed, but execution still enforces the inherited lock. This matches the intended DirectStep/MPTEndpoint behavior: some conditions are checked during payment execution rather than strand construction.
I agree the comment can be clarified, but restoring isFrozen(...) in the strand check would be more than a comment fix and would change the intended execution-stage behavior.
| ValidLoanBroker, | ||
| ValidLoan, | ||
| ValidVault, | ||
| ValidMPTPayment>; | ||
| ValidMPTPayment, | ||
| ValidMPTTransfer>; |
There was a problem hiding this comment.
I don’t think a compatibility typedef is needed here.
InvariantChecks is the concrete list of invariant checks, and it is expected to change when a new invariant is added. getInvariantChecks() must return the updated tuple so the new invariant runs; preserving the old tuple shape would defeat the purpose.
This header is exported, but I don’t believe the exact tuple arity/order is intended as a stable downstream API. Treating it as stable would make adding any invariant a breaking API event. I’m fine with mentioning the new invariant in release notes, but I don’t think we should add compatibility surface for the previous tuple type.
|
|
||
| // Deposit one asset, which is the frozen token, | ||
| // then we should get tecFROZEN error. | ||
| // Deposit one asset, which is the frozen locked, |
Co-authored-by: xrplf-ai-reviewer[bot] <266832837+xrplf-ai-reviewer[bot]@users.noreply.github.com> Co-authored-by: Shawn Xie <35279399+shawnxie999@users.noreply.github.com>
High Level Overview of Change