fix: Clamp VaultClawback to assetsAvailable for zero-amount clawback#6646
fix: Clamp VaultClawback to assetsAvailable for zero-amount clawback#6646
Conversation
Zero-amount clawback (meaning "clawback all") returned early without clamping to assetsAvailable, allowing more assets to be recovered than available when there was an outstanding loan. Move the zero-amount path inside the try block so it shares the same clamping logic as non-zero clawback. The old early-return behavior is retained behind a !fixAssortedFixes gate for ledger replay compatibility.
Tapanito
added
Amendment
AI Triage
- Add test for the pre-fixAssortedFixes legacy code path where zero-amount clawback does not clamp to assetsAvailable, verifying the transaction fails with tefINTERNAL for ledger replay fidelity - Remove unused sharesBefore variable in zero-amount post-fix test - Normalize env.balance() calls to use PrettyAsset directly - Strengthen non-zero clawback share assertions from inequality checks to exact expected values matching the zero-amount test
|
This PR has conflicts, please resolve them in order for the PR to be reviewed. |
…_1_3 Resolve amendment conflict: drop placeholder fixAssortedFixes, adopt fixSecurity3_1_3 (Supported::no) from develop. Update VaultClawback and tests to reference the new amendment name and explicitly enable it in the test Env since it is disabled by default.
|
All conflicts have been resolved. Assigned reviewers can now start or resume their review. |
|
/ai-review |
![xrplf-ai-reviewer[bot]](https://avatars.githubusercontent.com/u/67929741?s=60&v=4){kind=small}
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #6646 +/- ##
=========================================
- Coverage 81.4% 81.4% -0.0%
=========================================
Files 999 999
Lines 74439 74446 +7
Branches 7564 7560 -4
=========================================
- Hits 60621 60620 -1
- Misses 13818 13826 +8
... and 2 files with indirect coverage changes 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
Fixes a VaultClawback edge case where a zero-amount clawback (“claw back all”) could recover more than sfAssetsAvailable when loans are outstanding, by ensuring the zero-amount path shares the same clamping logic as non-zero clawbacks (while retaining legacy behavior behind a feature gate for replay compatibility).
Changes:
- Update
VaultClawback::assetsToClawbackto compute and clamp recovered assets for zero-amount clawback within the same guarded path as non-zero clawback. - Add regression tests covering zero-amount and non-zero clawback clamping when
sfAssetsAvailable < sfAssetsTotaldue to an outstanding loan. - Add a legacy-path test to validate pre-fix behavior when the relevant feature is disabled.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| src/test/app/Vault_test.cpp | Adds regression and legacy-path tests for clamping behavior under outstanding loans; enables the relevant feature for the test environment. |
| src/libxrpl/tx/transactors/vault/VaultClawback.cpp | Refactors zero-amount clawback handling so it is clamped to sfAssetsAvailable, preserving legacy behavior behind a feature flag. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
shawnxie999
left a comment
shawnxie999
left a comment
There was a problem hiding this comment.
LGTM - left minor comment
Remove unused sharesBefore variable and consolidate BEAST_EXPECT + null-check into idiomatic if (!BEAST_EXPECT(..)) pattern.
The zero-amount clawback on an empty vault was hitting the tecWRONG_ASSET ambiguity check when issuer == owner. Provide an explicit share amount to test the intended tecNO_PERMISSION path.
There was a problem hiding this comment.
Took a pass through this
Two issues flagged inline: a pre-patch inverted guard that silently skipped a negative-path security test, and a pre-fix zero-amount clawback bypass of the assetsAvailable cap. Both are addressed by this MR; comments request CI validation and an audit of analogous liquidation paths.
Review by ReviewBot 🤖
Review by Claude Opus 4.6 · Prompt: V12
pratikmankawde
left a comment
pratikmankawde
left a comment
There was a problem hiding this comment.
Looks good. Left minor comments on missing tests.
Cover four boundary cases not previously tested: - Partial clawback below assetsAvailable (no clamping needed) - Clawback exactly equal to assetsAvailable (boundary) - Clawback with zero assetsAvailable (fully borrowed vault) - Non-1:1 share ratio (scale=1) with outstanding loan Increase IOU/MPT funding from 1000 to 2000 to accommodate the additional setupVault calls.
|
@Tapanito how does this handle |
|
@mvadari good question, I added vault unit tests to ensure that |
github-merge-queue
Bot
removed this pull request from the merge queue due to a conflict with the base branch
|
This PR has conflicts, please resolve them in order for the PR to be reviewed. |
|
All conflicts have been resolved. Assigned reviewers can now start or resume their review. |
Zero-amount clawback (meaning "clawback all") returned early without clamping to assetsAvailable, allowing more assets to be recovered than available when there was an outstanding loan. Move the zero-amount path inside the try block so it shares the same clamping logic as non-zero clawback.
The old early-return behavior is retained behind an amendment gate for ledger replay compatibility.
High Level Overview of Change
Context of Change
API Impact
libxrplchange (any change that may affectlibxrplor dependents oflibxrpl)