VaultClawback: Burn shares of an empty vault #6120

Tapanito · 2025-12-08T18:07:38Z

In the context of the Single Asset Vault there exists a situation where Vault.AssetsTotal and Vault.AssetsAvailable are zero, but the Vault still has shares. Such a situation may arise due to a Loan that was issued for the total of Vault's assets defaulting. The problem with such situation is that the Vault becomes permanently stuck, deposits will not be able to settle the balance, and the Vault object is non-deletable. This PR fixes this problem, namely:

The PR updates the logic of VaultClawback transaction allowing the Owner of the Vault to submit VaultClawback transaction to burn shares of a depositor only when the there are no assets in the vault.

Edit:

The link to specification PR: XRPLF/XRPL-Standards#422

High Level Overview of Change

Context of Change

Type of Change

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to not work as expected)
Refactor (non-breaking change that only restructures code)
Performance (increase or change in throughput and/or latency)
Tests (you added tests for code that already exists, or your new feature included in this PR)
Documentation update
Chore (no impact to binary, e.g. .gitignore, formatting, dropping support for older tooling)
Release

API Impact

Public API: New feature (new methods and/or new fields)
Public API: Breaking change (in general, breaking changes should only impact the next api_version)
libxrpl change (any change that may affect libxrpl or dependents of libxrpl)
Peer protocol change (must be backward compatible or bump the peer protocol version)

…lt is stuck

src/xrpld/app/tx/detail/InvariantCheck.cpp

src/xrpld/app/tx/detail/VaultClawback.cpp

gregtatcam

LGTM

Tapanito · 2025-12-11T18:22:17Z

Sorry for the force-push, the devcontainer commit wasn't signed

codecov · 2025-12-11T18:48:10Z

Codecov Report

❌ Patch coverage is 97.50000% with 4 lines in your changes missing coverage. Please review.
✅ Project coverage is 79.2%. Comparing base (f80059e) to head (8034a79).
⚠️ Report is 1 commits behind head on develop.

Files with missing lines	Patch %	Lines
src/xrpld/app/tx/detail/VaultClawback.cpp	97.0%	4 Missing ⚠️

Additional details and impacted files

@@           Coverage Diff           @@
##           develop   #6120   +/-   ##
=======================================
  Coverage     79.1%   79.2%           
=======================================
  Files          836     836           
  Lines        71257   71312   +55     
  Branches      8305    8299    -6     
=======================================
+ Hits         56393   56448   +55     
  Misses       14864   14864

Files with missing lines	Coverage Δ
src/xrpld/app/tx/detail/InvariantCheck.cpp	`92.2% <100.0%> (+<0.1%)`	⬆️
src/xrpld/app/tx/detail/InvariantCheck.h	`100.0% <ø> (ø)`
src/xrpld/app/tx/detail/VaultClawback.h	`100.0% <ø> (ø)`
src/xrpld/app/tx/detail/VaultClawback.cpp	`97.3% <97.0%> (+2.4%)`	⬆️

... and 2 files with indirect coverage changes

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

gregtatcam · 2025-12-11T19:12:58Z

src/xrpld/app/tx/detail/VaultClawback.cpp

+    if (sharesTotal > 0 && assetsTotal == 0 && assetsAvailable == 0 &&
+        account == owner)
+    {
+        if (auto const amount = ctx.tx[~sfAmount]; amount)


Don't need to have this additional amount check.

if (auto const amount = ctx.tx[~sfAmount])

src/xrpld/app/tx/detail/VaultClawback.cpp

src/test/app/Vault_test.cpp

ximinez · 2025-12-18T22:50:46Z

If this PR gets merged into develop, remember to also cherry pick it into ximinez/lending-3.1 #6156.

Tapanito · 2025-12-23T15:26:51Z

src/test/app/Vault_test.cpp

            }
        });

-        testCase([&](Env& env,


@ximinez @gregtatcam , since the Clawback transaction serves two purposes, the owner can submit the transaction to burn own shares.

This unit test checks if the issuer is trying to clawback from itself, however since now it's a valid use-case to submit the same account as "issuer" and "holder", these unit-tests are no longer valid (as the code they're testing was removed)

Tapanito · 2025-12-23T16:00:37Z

src/xrpld/app/tx/detail/VaultClawback.cpp

-    AccountID const issuer = ctx.tx[sfAccount];
-    AccountID const holder = ctx.tx[sfHolder];
-
-    if (issuer == holder)


@ximinez , @gregtatcam this check is no longer valid, as the Vault Owner may submit a clawback transaction to burn their own shares.

Co-authored-by: Ed Hennis <ed@ripple.com>

src/xrpld/app/tx/detail/VaultClawback.cpp

src/test/app/Vault_test.cpp

src/xrpld/app/tx/detail/VaultClawback.cpp

ximinez

In addition to these, there are a couple of comments from my first review still left pending.

src/xrpld/app/tx/detail/VaultClawback.cpp

src/test/app/Vault_test.cpp

Tapanito · 2025-12-24T18:28:23Z

@ximinez , @gregtatcam thank you for your time reviewing this PR 🙇

src/xrpld/app/tx/detail/VaultClawback.cpp

gregtatcam · 2026-01-05T20:16:07Z

src/xrpld/app/tx/detail/VaultClawback.cpp

+    MPTIssue const share{mptIssuanceID};
+
+    Asset const vaultAsset = vault->at(sfAsset);
+    STAmount const amount = [&]() {


Maybe makes sense to have a helper function? There is a similar block of code in preclaim.

Added a helper function.

src/xrpld/app/tx/detail/VaultClawback.cpp

gregtatcam · 2026-01-05T21:04:19Z

src/test/app/Vault_test.cpp

+        env(pay(issuer, owner, MPT(1000)));
+        env(pay(issuer, depositor, MPT(1000)));
+        env.close();
+        testCase(MPT, "MPT", owner, depositor);


Should add unit-tests when owner is the issuer.

I added the unit test and found a bug. Great call Greg!

ximinez

I left a couple of minor style suggestions, but otherwise this looks good, aside from the items that @gregtatcam has identified.

ximinez · 2026-01-05T21:22:59Z

src/test/app/Vault_test.cpp

+            }
+            else
+            {


Minor: You could return at the end of the if block, instead of having the else.

ximinez · 2026-01-05T21:38:22Z

src/test/app/Vault_test.cpp

+                BEAST_EXPECT(vaultSle != nullptr);
+                if (!vaultSle)
+                    return;


Minor: This could be written as

if (!BEAST_EXPECT(vaultSle != nullptr)) return;

gregtatcam

Looks good. I just have a couple of minor suggestions left for your consideration and a request to update the unit-test when an asset's issuer is also the vault holder.

Tapanito added 2 commits December 8, 2025 18:56

adds a mechanism for the vault owner to burn user shares when the vau…

865bbc9

…lt is stuck

minor fixes

d46ed4e

Tapanito requested review from gregtatcam and ximinez December 8, 2025 18:07

Tapanito added 4 commits December 8, 2025 19:07

Merge branch 'develop' into tapanito/vault-owner-clawback

c58724e

fixes formatting

74f75ce

Merge branch 'develop' into tapanito/vault-owner-clawback

04e1af6

formatting issues

8a935e1

gregtatcam reviewed Dec 9, 2025

View reviewed changes

Tapanito added 2 commits December 10, 2025 17:47

addresses first comment

d049434

improves invariant check

61f078d

gregtatcam approved these changes Dec 10, 2025

View reviewed changes

Tapanito added 2 commits December 11, 2025 12:55

attempt to fix optional dereference

733bb00

fixes an elusive optional dereference

2c63982

Tapanito force-pushed the tapanito/vault-owner-clawback branch from 1ba2f8d to 2c63982 Compare December 11, 2025 18:11

Merge branch 'develop' into tapanito/vault-owner-clawback

ef44ca7

Tapanito requested a review from gregtatcam December 11, 2025 18:25

gregtatcam reviewed Dec 11, 2025

View reviewed changes

Merge branch 'develop' into tapanito/vault-owner-clawback

2af722f

Tapanito added the Ready to merge *PR author* thinks it's ready to merge. Has passed code review. Perf sign-off may still be required. label Dec 16, 2025

ximinez reviewed Dec 17, 2025

View reviewed changes

minor comment changes

0ad2c70

improves vaultClawback logic

3ab6623

Tapanito requested review from gregtatcam and ximinez December 23, 2025 15:24

Tapanito commented Dec 23, 2025

View reviewed changes

Tapanito and others added 3 commits December 23, 2025 19:10

further code improvements

d3d47d3

Apply suggestions from code review

8cc6eac

Co-authored-by: Ed Hennis <ed@ripple.com>

restores a previous unit-test

ee24339

gregtatcam reviewed Dec 23, 2025

View reviewed changes

ximinez requested changes Dec 24, 2025

View reviewed changes

Tapanito added 3 commits December 24, 2025 19:19

addresses review comments

b3ae984

syntax fixes

8456cbc

additional unit test

89fe41d

more syntax

294f7bf

vvysokikh1 removed the Ready to merge *PR author* thinks it's ready to merge. Has passed code review. Perf sign-off may still be required. label Jan 2, 2026

vvysokikh1 requested a review from ximinez January 2, 2026 16:15

gregtatcam reviewed Jan 5, 2026

View reviewed changes

ximinez approved these changes Jan 5, 2026

View reviewed changes

gregtatcam approved these changes Jan 6, 2026

View reviewed changes

Tapanito added 4 commits January 6, 2026 22:00

addresses review comments pt1

07c1bb7

addresses review comments pt2

e241e50

Merge branch 'develop' into tapanito/vault-owner-clawback

cc6b9fc

Merge branch 'develop' into tapanito/vault-owner-clawback

8034a79

Tapanito added the Ready to merge *PR author* thinks it's ready to merge. Has passed code review. Perf sign-off may still be required. label Jan 7, 2026

VaultClawback: Burn shares of an empty vault #6120

Are you sure you want to change the base?

VaultClawback: Burn shares of an empty vault #6120

Conversation

Tapanito commented Dec 8, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

High Level Overview of Change

Context of Change

Type of Change

API Impact

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

gregtatcam left a comment

Choose a reason for hiding this comment

Uh oh!

Tapanito commented Dec 11, 2025

Uh oh!

codecov bot commented Dec 11, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ximinez commented Dec 18, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ximinez left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Tapanito commented Dec 24, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ximinez left a comment

Choose a reason for hiding this comment

Tapanito commented Dec 8, 2025 •

edited

Loading

codecov bot commented Dec 11, 2025 •

edited

Loading