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

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 79.1%. Comparing base (f816ffa) to head (2af722f).
⚠️ Report is 1 commits behind head on develop.

Additional details and impacted files

@@           Coverage Diff           @@
##           develop   #6120   +/-   ##
=======================================
  Coverage     79.1%   79.1%           
=======================================
  Files          839     839           
  Lines        71386   71419   +33     
  Branches      8343    8334    -9     
=======================================
+ Hits         56447   56485   +38     
+ Misses       14939   14934    -5

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.cpp	`95.8% <100.0%> (+0.8%)`	⬆️
src/xrpld/app/tx/detail/VaultClawback.h	`100.0% <ø> (ø)`

... and 8 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])

ximinez · 2025-12-16T22:48:24Z

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

-        else if (isXRP(amount->asset()))
-        {
-            JLOG(ctx.j.debug()) << "VaultClawback: cannot clawback XRP.";
-            return temMALFORMED;
-        }


I think it makes sense to leave the XRP check here in preflight.

To summarize 3.3.1.1 from the proposed spec changes:

If the transaction is submitted by the vault owner, then amount must be specified as the share type (MPT).

If the transaction is submitted by the asset issuer, the amount must be specified as the asset type (IOU, MPT).

Thus, specifying sfAmount as XRP is never valid. May as well check it here, and save the effort later.

ximinez · 2025-12-16T22:54:05Z

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

    Asset const vaultAsset = vault->at(sfAsset);
    if (auto const amount = ctx.tx[~sfAmount];
        amount && vaultAsset != amount->asset())
        return tecWRONG_ASSET;


You need two cases for this check:

When the vault is worthless, and the tx is submitted by the vault owner. I'd suggest checking that the asset is the shares MPT in the block below where you check for that.

When the tx is submitted by the asset issuer. You should move this check to after the worthless vault block.

ximinez · 2025-12-16T22:56:40Z

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

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


What should happen if the holder has no shares, and sfAmount is 0?

ximinez · 2025-12-16T23:15:14Z

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

+        // Clamp to maximum.
+        if (assetsRecovered > *assetsAvailable)
+        {
+            assetsRecovered = *assetsAvailable;


Is it necessary to do two passes when assetsRecovered > *assetsAvailable?

Intuitively, it seems like you could clamp assetsRecovered as the first step, then do one set of calls to assetsToSharesWithdraw and sharesToAssetsWithdraw, probably truncating either way, and get the same result with less work.

ximinez · 2025-12-16T23:17:31Z

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

+    std::shared_ptr<SLE> const& vault,
+    std::shared_ptr<SLE const> const& sleShareIssuance,
+    AccountID const& holder,
+    STAmount const& clawbackAmount)


Unless I'm mistaken, clawbackAmount must be the vault asset type in this function, but there's nothing in here that checks. There is an assert followed by a check in assetsToSharesWithdraw, but that will crash nodes running in debug, and we want to avoid that.

ximinez · 2025-12-16T23:57:25Z

src/test/app/Vault_test.cpp

-            {
-                auto tx = vault.clawback(
-                    {.issuer = owner,
-                     .id = keylet.key,
-                     .holder = issuer,
-                     .amount = asset(50)});
-                env(tx, ter(temMALFORMED));
-            }


I'm not usually a fan of removing test cases unless they're redundant or mess up later tests. I think it's usually better to update the test with the new expected result. Do you think that is appropriate here?

ximinez · 2025-12-17T00:05:33Z

src/test/app/Vault_test.cpp

+            auto const [availablePreDefault, totalPreDefault] =
+                vaultAssetBalance(vaultKeylet);
+            BEAST_EXPECT(availablePreDefault == totalPreDefault);
+            BEAST_EXPECT(availablePreDefault == asset(100).value());


You should do an owner clawback here, before the loan to show that it doesn't work when there are assets available.

ximinez · 2025-12-17T00:06:17Z

src/test/app/Vault_test.cpp

+            // Create a simple Loan for the full amount of Vault assets
+            env(set(depositor, brokerKeylet.key, asset(100).value()),
+                loan::interestRate(TenthBips32(0)),
+                gracePeriod(10),
+                paymentInterval(120),
+                paymentTotal(10),
+                sig(sfCounterpartySignature, owner),
+                fee(env.current()->fees().base * 2),
+                ter(tesSUCCESS),
+                THISLINE);
+            env.close();


You should also do an owner clawback here, before the default to show that it doesn't work when assets total is not zero, even if available is.

ximinez · 2025-12-17T00:07:37Z

src/test/app/Vault_test.cpp

+            // The owner can clawback explicitly all shares, burning them
+            {
+                auto [vault, vaultKeylet] = setupVault(asset);
+                env(vault.clawback({
+                        .issuer = owner,
+                        .id = vaultKeylet.key,
+                        .holder = depositor,
+                        .amount = asset(vaultShareBalance(vaultKeylet)),
+                    }),
+                    ter(tesSUCCESS),
+                    THISLINE);
+                env.close();
+                BEAST_EXPECT(vaultShareBalance(vaultKeylet) == 0);
+            }


It would be worth another test where amount is explicitly zero.

.amount = shareasset(0),

ximinez · 2025-12-17T00:29:55Z

src/test/app/Vault_test.cpp

+                env(vault.clawback({
+                        .issuer = owner,
+                        .id = vaultKeylet.key,
+                        .holder = depositor,
+                        .amount = asset(vaultShareBalance(vaultKeylet)),


This is subtle. The spec is very clear that the amount must be the shares asset type. But this is the vault asset type. This test should be failing.

However, as I noted in VaultClawback, if you pass the shares type, you'll get tecWRONG_ASSET. So you've got two mistakes that work together to make it look as if they're doing the right thing.

(When you fix the issues, you should keep these transactions to verify they fail.)

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

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Tapanito commented Dec 8, 2025 •

edited

Loading

codecov bot commented Dec 11, 2025 •

edited

Loading