fix: Unify freeze checks for pseudo-account deposit/withdraw#7382
Open
Tapanito wants to merge 22 commits into
Open
fix: Unify freeze checks for pseudo-account deposit/withdraw#7382Tapanito wants to merge 22 commits into
Tapanito wants to merge 22 commits into
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.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #7382 +/- ##
=========================================
- Coverage 82.0% 82.0% -0.0%
=========================================
Files 1007 1007
Lines 76854 76885 +31
Branches 8984 8984
=========================================
+ Hits 62984 63009 +25
- Misses 13861 13867 +6
Partials 9 9
... and 6 files with indirect coverage changes 🚀 New features to boost your workflow:
|
bthomee
approved these changes
Jun 2, 2026
vvysokikh1
requested 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
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.
Tapanito
changed the title
Tapanito
requested review from
gregtatcam
and removed request for
a1q123456 and
bthomee
Tapanito
changed the title
Extract all IOU freeze and MPT lock tests from testWithIOU and testWithMPT into two focused test functions covering VaultDeposit and VaultWithdraw respectively. Each function tests both IOU (global, depositor regular/deep, vault-account regular/deep) and MPT (global lock, depositor lock, vault-account lock) for both pre- and post-fixCleanup3_3_0, plus clawback-while-frozen assertions.
![xrplf-ai-reviewer[bot]](https://avatars.githubusercontent.com/u/67929741?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Went through the changes
One high-severity bug: checkDepositFreeze uses a shallow freeze check on the destination pseudo-account instead of checkDeepFrozen, allowing deposits into a deep-frozen vault that can never be withdrawn — see inline.
Review by ReviewBot 🤖
Review by Claude Sonnet 4.6 · Prompt: V15
Tapanito
changed the title
Contributor
There was a problem hiding this comment.
Pull request overview
This PR centralizes freeze/lock semantics for pseudo-account deposit/withdraw flows by introducing shared helpers and refactoring pseudo-account-backed transactors to use them under the fixCleanup3_3_0 amendment. It also restructures Vault/LoanBroker tests to concentrate freeze/lock coverage into dedicated test cases, and adjusts the Nix devshell to avoid flaky Conan test builds on Darwin.
Changes:
- Add
checkWithdrawFreezesandcheckDepositFreezehelpers (public API) and use them in Vault and LoanBroker cover deposit/withdraw preclaim logic (gated byfixCleanup3_3_0). - Update
VaultWithdrawshare-balance checking to ignore freeze post-fix (since preclaim enforces the new freeze rules). - Refactor/move freeze-related assertions into new dedicated tests for Vault and LoanBroker; update the Nix devshell to skip Conan checks on Darwin.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| src/libxrpl/ledger/helpers/TokenHelpers.cpp | Implements centralized pseudo-account deposit/withdraw freeze helpers. |
| include/xrpl/ledger/helpers/TokenHelpers.h | Exposes the new helpers as public API (docs need to match semantics). |
| src/libxrpl/tx/transactors/vault/VaultDeposit.cpp | Switches deposit preclaim freeze logic to checkDepositFreeze under fixCleanup3_3_0. |
| src/libxrpl/tx/transactors/vault/VaultWithdraw.cpp | Switches withdraw preclaim freeze logic to checkWithdrawFreezes under fixCleanup3_3_0 and uses IgnoreFreeze for share balance post-fix. |
| src/libxrpl/tx/transactors/lending/LoanBrokerCoverDeposit.cpp | Switches cover deposit preclaim freeze logic to checkDepositFreeze under fixCleanup3_3_0. |
| src/libxrpl/tx/transactors/lending/LoanBrokerCoverWithdraw.cpp | Switches cover withdraw preclaim freeze logic to checkWithdrawFreezes under fixCleanup3_3_0. |
| src/test/app/Vault_test.cpp | Removes inline freeze/lock tests and adds dedicated testVaultDepositFreeze / testVaultWithdrawFreeze. |
| src/test/app/LoanBroker_test.cpp | Removes inline freeze/lock checks and adds dedicated testCoverDepositFreezes / testCoverWithdrawFreezes; reorders some run() calls. |
| nix/devshell.nix | On Darwin, disables Conan’s test suite in the dev shell to avoid unreliable sandbox builds. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…/checkWithdrawFreeze Migrate AMMDeposit and AMMWithdraw to use the unified freeze helpers introduced for Vault and LoanBroker, gated on fixCleanup3_3_0. AMMDeposit: checkDepositFreeze for both pool assets replaces the two-layer checkAsset + checkAmount freeze logic. If either pool asset is frozen (depositor or AMM account), all deposits are blocked. AMMWithdraw: checkWithdrawFreeze per withdrawn amount replaces checkFrozen(AMM) + checkIndividualFrozen(user). Regular IOU freeze no longer blocks self-withdrawal; only deep freeze does. The issuer exemption allows the token issuer to withdraw from a frozen pool; issuerFreezeHandling() ensures doApply uses IgnoreFreeze so pool math does not divide by zero. Rename checkWithdrawFreezes -> checkWithdrawFreeze (singular) and update all callers. Document both helpers with full freeze semantics.
Restore unconditional IgnoreFreeze in VaultWithdraw::doApply when fixCleanup3_3_0 is enabled. That accountHolds reads the submitter's share balance, which recurses into the underlying asset, so freeze gating must stay in preclaim; a dstAcct==issuer guard there wrongly blocked self-withdrawal under a regular freeze. Apply issuer-aware freeze handling to LoanBrokerCoverWithdraw::preclaim so the issuer can redeem their own token from the broker pseudo-account even under a global freeze, matching AMM and Vault. Here accountHolds reads the pseudo-account's holdings, so the issuer-only IgnoreFreeze is both necessary and sufficient. Rename the checkWithdrawFreeze source parameter to srcAcct to match its definition, drop a misleading comment, and hoist the fixCleanup feature flags into named locals.
clang-tidy include-cleaner flagged amendmentCombinations using std::initializer_list without a direct include.
| isPseudoAccount(view, dstAcct), | ||
| "xrpl::checkDepositFreeze : destination must be a pseudo-account"); | ||
|
|
||
| if (auto const ret = checkFrozen(view, srcAcct, asset)) |
Contributor
There was a problem hiding this comment.
checkDepositFreeze never calls checkDeepFrozen on srcAcct, so a deep-frozen depositor can still move funds into the pseudo-account. Add a deep-frozen check on the source before the regular freeze check on the destination:
if (auto const ret = checkDeepFrozen(view, srcAcct, asset))
return ret;
if (auto const ret = checkFrozen(view, srcAcct, asset))
return ret;
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).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.