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.5%
=======================================
Files 998 998
Lines 74443 74450 +7
Branches 7563 7558 -5
=======================================
+ Hits 60632 60640 +8
+ Misses 13811 13810 -1
... and 1 file 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.
| auto const sharesBefore = env.balance(depositor, shares); | ||
|
|
There was a problem hiding this comment.
sharesBefore is computed but never used in this test case, which can trigger an unused-variable warning (often treated as an error in CI). Please remove it or use it in an assertion (e.g., verify the expected reduction in shares).
| auto const sharesBefore = env.balance(depositor, shares); |
| // Pre-fixSecurity3_1_3: zero-amount clawback returned early without | ||
| // clamping to assetsAvailable, allowing more assets to be recovered | ||
| // than available when there was an outstanding loan. Retained for | ||
| // ledger replay compatibility. | ||
| if (!ctx_.view().rules().enabled(fixSecurity3_1_3) && clawbackAmount == beast::zero) |
There was a problem hiding this comment.
The PR description mentions keeping the legacy behavior behind a !fixAssortedFixes gate, but the code gates it on !fixSecurity3_1_3. Since fixAssortedFixes doesn't appear to exist in the codebase, please either update the PR description to match the actual feature flag or switch the gate to the intended amendment if different.
| BEAST_EXPECT(vaultSle != nullptr); | ||
| if (!vaultSle) | ||
| return; |
There was a problem hiding this comment.
nit - alternatively could be (same for places down below)
if (!BEAST_EXPECT(vaultSle))
return;
shawnxie999
left a comment
shawnxie999
left a comment
There was a problem hiding this comment.
LGTM - left minor comment
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.
High Level Overview of Change
Context of Change
API Impact
libxrplchange (any change that may affectlibxrplor dependents oflibxrpl)