fix: Unify freeze checks for pseudo-account deposit/withdraw - #7382
Merged
Merged
Conversation
Under fixCleanup3_2_0, VaultWithdraw::preclaim now explicitly checks that the vault pseudo-account (sender) is not frozen, and replaces the destination checkFrozen with checkDeepFrozen — a frozen account can still receive assets, only a deep-frozen one cannot. Pre-amendment behaviour is preserved via the else branch. Tests are updated to reflect the changed error codes and new pre/post amendment cases are added for the vault-account and 3rd-party freeze scenarios.
![xrplf-ai-reviewer[bot]](https://avatars.githubusercontent.com/u/67929741?s=60&v=4){kind=small}
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #7382 +/- ##
========================================
Coverage 82.0% 82.0%
========================================
Files 1007 1007
Lines 76888 76996 +108
Branches 8971 8969 -2
========================================
+ Hits 63042 63147 +105
- Misses 13837 13840 +3
Partials 9 9
... and 3 files with indirect coverage changes 🚀 New features to boost your workflow:
|
bthomee
previously approved these changes
Jun 2, 2026
vvysokikh1
suggested changes
Jun 3, 2026
Verify the dstAcct != vaultAsset.getIssuer() guard introduced under fixCleanup3_2_0: frozen vault trust line and MPT global lock both block non-issuer withdrawals, while the issuer-destination path bypasses preclaim's freeze checks (redemption path). Preclaim passes for the issuer destination, but doApply::accountHolds(ZeroIfFrozen) still returns 0 for locked shares — a matching doApply fix is tracked via TODO comments. Also updates expected error codes in existing freeze tests affected by the new vault-sender and deep-frozen-destination checks.
Hoist dstAcct into doApply and use FreezeHandling::IgnoreFreeze for accountHolds when dstAcct == vaultAsset.getIssuer() under fixCleanup3_2_0. Without this the locked-share balance read as zero and the redemption failed with tecINSUFFICIENT_FUNDS despite preclaim passing. Update tests: issuer-redemption now succeeds end-to-end for both IOU and MPT vaults.
The previous edit dropped the assertion that clawback still works when the MPT asset is globally locked. Restore it before the issuer-redemption step, claw back 50 of the 100 deposited units, then withdraw the remaining 50 to the issuer so both behaviours are exercised in the same test.
Mirror the MPT and trust-line-freeze variants: under global freeze the redemption path (dstAcct == vaultAsset.getIssuer()) should still succeed.
vvysokikh1
reviewed
Jun 3, 2026
vvysokikh1
self-requested a review
June 3, 2026 16:15
vvysokikh1
previously approved these changes
Jun 3, 2026
…ezes Introduce checkWithdrawFreezes helper gated behind fixCleanup3_3_0 that consolidates source, submitter, and destination freeze checks for pseudo-account withdrawals. The issuer-redemption exemption is now handled in one place rather than scattered across callers. Add MPT individual-lock tests for depositor and 3rd-party destination withdrawal scenarios, and extend LoanBrokerCoverWithdraw with the same consolidated freeze logic.
Signed-off-by: Pratik Mankawde <3397372+pratikmankawde@users.noreply.github.com>
…kDepositFreeze Reflect the new deposit freeze semantics: only a deep-frozen pseudo-account blocks deposits; a regular freeze on the vault account's trust line no longer does. Also account for the checkWithdrawFreezes change where submitter == destination skips the submitter freeze check. Parameterize the pre/post-fix test variants via runTest lambdas instead of withFix ternaries. Add a dedicated deep-freeze deposit test.
Add checkDepositFreeze that checks the depositor for regular freeze and the destination pseudo-account for deep freeze only. A regular freeze on the vault account's trust line no longer blocks deposits. Update checkWithdrawFreezes to skip the submitter freeze check when submitter == destination (withdrawing to self). Gate VaultDeposit with fixCleanup3_3_0; pre-fix path preserves the old isFrozen-based checks. Use checkDepositFreeze in LoanBrokerCoverDeposit.
|
This PR has conflicts, please resolve them in order for the PR to be reviewed. |
gregtatcam
previously approved these changes
Jun 26, 2026
…eze-check # Conflicts: # src/libxrpl/ledger/helpers/MPTokenHelpers.cpp # src/test/app/Vault_test.cpp
|
All conflicts have been resolved. Assigned reviewers can now start or resume their review. |
shawnxie999
previously approved these changes
Jun 26, 2026
gregtatcam
previously approved these changes
Jun 26, 2026
Tapanito
added
the
Ready to merge
shawnxie999
approved these changes
Jun 26, 2026
gregtatcam
approved these changes
Jun 26, 2026
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
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
Introduces two centralized freeze-checking helpers —
checkWithdrawFreezeandcheckDepositFreeze— and refactors every pseudo-account-backed transactor (VaultDeposit,VaultWithdraw,AMMDeposit,AMMWithdraw,LoanBrokerCoverDeposit,LoanBrokerCoverWithdraw) to use them. All behavioral changes are gated behindfixCleanup3_3_0.Context of Change
Pre-
fixCleanup3_3_0, freeze checks across pseudo-account transactors were ad-hoc and inconsistent:VaultWithdrawchecked the destination and the submitter's shares, but never checked the vault pseudo-account (source) itself.VaultDepositchecked the depositor and share freeze but not the vault pseudo-account's freeze directly (it was caught indirectly via the transitive share check, yieldingtecLOCKEDinstead oftecFROZEN).LoanBrokerCoverWithdrawchecked the broker pseudo-account and destination, but skipped the submitter's individual freeze entirely (a bug — frozen accounts could withdraw to third parties).LoanBrokerCoverDepositusedcheckDeepFrozenfor the pseudo-account, allowing deposits when the pseudo-account was merely regular-frozen.AMMDeposit/AMMWithdrawhad their own inline freeze logic duplicating the same patterns.Pseudo-Account Deposit / Withdraw Freeze Semantics
Pseudo-accounts (Vaults, AMM pools, LoanBrokers) hold pooled assets on behalf of participants. Their freeze semantics differ from regular account-to-account transfers because:
checkFrozen(sourceAcct, asset)separately before callingcheckWithdrawFreeze.checkWithdrawFreeze(view, sourceAcct, submitterAcct, dstAcct, asset)Models the flow source (pseudo) → submitter → destination:
dstAcct == asset.getIssuer()→tesSUCCESScheckFrozen(sourceAcct, asset)checkFrozen(submitterAcct, asset)— skipped when submitter == dstcheckDeepFrozen(dstAcct, asset)For MPTs, "locked" is equivalent to deep-frozen, so locked MPT holders are always blocked (including self-withdrawal).
checkDepositFreeze(view, srcAcct, dstAcct, asset)Models the flow depositor → pseudo-account:
checkFrozen(srcAcct, asset)checkFrozen(dstAcct, asset)Both functions assert that the pseudo-account argument (
sourceAcct/dstAcct) is a pseudo-account viaXRPL_ASSERT.Before / After
All behavioral changes are gated behind
fixCleanup3_3_0.Withdrawals
tecFROZEN/tecLOCKEDtesSUCCESStecFROZENtesSUCCESStecFROZENtesSUCCESStecFROZENLoanBrokerCoverWithdrawwith frozen submitter → 3rd partytesSUCCESS(bug)tecFROZENtecFROZENtesSUCCESSammHoldsusesIgnoreFreezeDeposits
tecLOCKED(transitive share check)tecFROZEN(directcheckFrozen)tecLOCKED(transitive share check)tecFROZEN(directcheckFrozen)checkDepositFreezedirectlyLoanBrokerCoverDepositto regularly-frozen pseudo (IOU)tesSUCCESS(onlycheckDeepFrozen)tecFROZENcheckDepositFreezeon bothsfAsset/sfAsset2Vault share-level freeze removal (post-fix)
VaultDepositpreviously checkedisFrozen(depositor, vaultShare). Post-fix this check is removed: vault shares are issued by the vault pseudo-account, which cannot submitMPTokenIssuanceSetto individually lock a holder's MPToken. The only way shares become locked is transitively via the underlying asset, whichcheckDepositFreeze/checkWithdrawFreezealready covers.VaultWithdraw::doApply—IgnoreFreezefor balance checkPost-fix,
accountHoldsindoApplyusesFreezeHandling::IgnoreFreezebecausepreclaimalready validated all freeze constraints viacheckWithdrawFreeze. This prevents spurious "insufficient shares" failures for the relaxed self-withdrawal case (where the submitter's regular freeze would otherwise zero out the balance).Authorization for AMM pseudo-accounts holding vault shares
When a vault share is one of an AMM pool's two assets, the AMM pseudo-account holds that share.
requireAuthdescends from a vault share to its underlying asset to verify the holder is authorized for the underlying. The AMM holds the share but not the underlying, so the descent wrongly returnedtecNO_AUTH(MPT underlying) /tecNO_LINE(IOU underlying).(Only AMM pools hold vault shares: a vault issues its shares from the vault pseudo-account, and LoanBroker cover is denominated in the vault's underlying asset — neither holds shares.)
On released behavior (SAV enabled,
fixCleanup3_3_0disabled) this left AMM-held shares stuck: an AMM could be created holding vault shares, butaccountHolds(ZeroIfUnauthorized)read the pool's share balance as zero, so withdrawals failed withtecAMM_FAILED.requireAuth: pseudo-accounts are exempted before the recursive underlying-asset check, gated byfixCleanup3_3_0. Pre-amendment behavior is preserved (the exemption stays after the recursion).ValidMPTTransferinvariant: pseudo-accounts are treated as authorized inisAuthorized. This is ungated but fork-safe — the invariant only affects consensus underfeatureMPTokensV2(unreleased).Result: AMM pools can be created with, hold, and (post-
fixCleanup3_3_0) withdraw vault shares.API Impact
libxrplchange (any change that may affectlibxrplor dependents oflibxrpl)Test Plan
All tests run with both
fixCleanup3_3_0enabled and disabled to verify pre/post-amendment behavior:Vault_test::testVaultDepositFreeze: IOU global freeze, depositor regular/deep freeze, vault-account regular/deep freeze, clawback-while-frozen. MPT global lock, depositor individual lock, vault pseudo-account individual lock.Vault_test::testVaultWithdrawFreeze: IOU global freeze, vault-account regular freeze, depositor regular/deep freeze (self + 3rd party), destination regular/deep freeze, clawback-while-frozen, issuer bypass under global lock. MPT equivalents including issuer bypass.LoanBroker_test::testCoverDepositFreezes: IOU source/pseudo regular/deep freeze, MPT global/individual locks.LoanBroker_test::testCoverWithdrawFreezes: IOU source/submitter/destination freeze at regular and deep levels, submitter-to-self, issuer bypass. MPT equivalents.AMM_test: Global/individual freeze deposit and withdraw tests updated withamendmentCombinations({fixCleanup3_3_0})to cover both amendment states. Issuer withdrawal from frozen pool.AMMMPT_test::testAMMWithVaultShares: AMM with an IOU-backed and an MPT-backed vault share as a pool asset; deposit/withdraw under underlying freeze/lock, pre- and post-fixCleanup3_3_0(pre-amendment the shares are inaccessible to the pseudo-account, so withdraw fails withtecAMM_FAILED).Invariants_test: side-specificAMM_WITHDRAWvault-share freeze invariant cases (pseudo-account as sender vs. receiver, pre/post-fixCleanup3_3_0).