fix: Disable AMM creation with Vault shares#7666
Conversation
Tapanito
requested review from
a1q123456
and removed request for
Kassaking7 and
PeterChen13579
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #7666 +/- ##
=======================================
Coverage 82.3% 82.3%
=======================================
Files 1024 1024
Lines 78389 78394 +5
Branches 8973 8971 -2
=======================================
+ Hits 64503 64510 +7
+ Misses 13877 13875 -2
Partials 9 9
... and 4 files with indirect coverage changes 🚀 New features to boost your workflow:
|
| if (asset.holds<MPTIssue>() && | ||
| isVaultPseudoAccountFrozen(view, pseudoAcct, asset.get<MPTIssue>(), 0)) |
There was a problem hiding this comment.
unreachable code should probably moved as the last check, to allow necessary checks to be done earlier
![xrplf-ai-reviewer[bot]](https://avatars.githubusercontent.com/u/67929741?s=60&v=4){kind=small}
There was a problem hiding this comment.
Gave this a review
Two UNREACHABLE-wrapped freeze checks in checkWithdrawFreeze and checkDepositFreeze create a silent bypass if the "no vault shares in pseudo-account-backed objects" invariant is ever broken in production — see inline comments.
Review by ReviewBot 🤖
Review by Claude Sonnet 4.6 · Prompt: V15
There was a problem hiding this comment.
Pull request overview
Disallows using Vault share MPTs (issued by Vault pseudo-accounts) as AMM pool assets, and removes now-unreachable “transitive vault freeze” special-casing by simplifying freeze/invariant handling and updating tests accordingly.
Changes:
- AMMCreate now rejects AMM creation when either asset is a pseudo-account–issued MPT (intended to cover Vault shares), returning
tecWRONG_ASSET. - Freeze helpers and the MPT transfer invariant are simplified to rely on the unified
isFrozen()logic; legacy branching and AMM-with-shares special cases are removed and replaced with defensive guards/assertions. - Test suites are updated to reflect the new behavior (AMM-with-shares creation failure) and to exercise invariants/freeze scenarios directly without constructing impossible states.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| src/test/app/Vault_test.cpp | Refactors/splits Vault freeze tests into IOU vs MPT cases and updates expectations/case labeling. |
| src/test/app/Invariants_test.cpp | Reworks vault-share invariant tests to drive invariant checks directly; removes legacy AMM-with-shares scenario reliance. |
| src/test/app/AMMMPT_test.cpp | Updates AMM+vault-shares test to assert AMM creation fails with tecWRONG_ASSET. |
| src/libxrpl/tx/transactors/dex/AMMCreate.cpp | Adds preclaim rejection for pseudo-account–issued MPT assets (Vault shares) when creating an AMM. |
| src/libxrpl/tx/invariants/MPTInvariant.cpp | Removes legacy fixCleanup3_3_0 branching and unconditionally uses isFrozen() for transfer validity checks. |
| src/libxrpl/ledger/helpers/TokenHelpers.cpp | Simplifies deposit/withdraw freeze checks; removes transitive vault-freeze branch and adds defensive assertions/guards. |
| include/xrpl/ledger/helpers/TokenHelpers.h | Updates documentation and parameter naming to match revised freeze-check semantics. |
| include/xrpl/ledger/helpers/MPTokenHelpers.h | Clarifies isIndividualFrozen behavior and directs callers to use isFrozen for complete checks. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
a1q123456
left a comment
a1q123456
left a comment
There was a problem hiding this comment.
Looks good overall. Just a small suggestion.
| shareID = env.le(vaultKeylet)->at(sfShareMPTID); | ||
|
|
||
| // Freeze a2's IOU trustline from the issuer side. | ||
| // a2 is the receiver in the simulated AMM withdraw; the | ||
| // distinction under test is that pre-fix330 the invariant | ||
| // does not apply the transitive vault freeze to receivers. | ||
| env(trust(gw, gw["IOU"](0), a2, tfSetFreeze)); | ||
| env.close(); | ||
| vaultPseudoID = env.le(vaultKeylet)->at(sfAccount); |
There was a problem hiding this comment.
Would be cleaner if we could return those values from the lambda instead of updating local variable given that this function always returns true. I suggest something like this:
auto const setupVault = [&](Account const& a1, Account const& a2, Env& env) -> std::tuple<MPTID, AccountID> {
// ... the logic
return std::make_tuple(env.le(vaultKeylet)->at(sfShareMPTID), env.le(vaultKeylet)->at(sfAccount));
};
// And then, use the lambda in other lambdas:
auto const preclose = [&](Account const& a1, Account const& a2, Env& env) -> bool {
auto const [shareID, vaultPseudoID] = setupVault(a1, a2, env);
};
};
Disallow Vault Shares as an AMM asset
Summary
Vault shares (MPTs issued by a vault pseudo-account) must never be held inside another pseudo-account-backed object. Vaults already reject pseudo-account-issued assets at creation (VaultCreate), and LoanBrokers inherit this through the vault they wrap — but AMMs had no such guard, allowing a pool to be created with vault shares.
This led to a tangle of special-case freeze handling: the deposit/withdraw freeze helpers and the ValidMPTTransfer invariant carried a fixCleanup3_3_0-gated "transitive vault freeze" path to cope with frozen vault shares sitting inside an AMM.
This PR closes the gap at the source — vault shares are rejected at AMM creation — and removes the now-unreachable special casing, replacing it with defensive guards.
Changes
Amendment / consensus safety
The AMM rejection is gated behind featureMPTokensV2.
Test plan