Adds functionality to block vault deposits by Tapanito · Pull Request #6361 · XRPLF/rippled

Tapanito · 2026-02-12T16:43:44Z

Specification: XRPLF/XRPL-Standards#469

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)

Tapanito · 2026-02-17T12:46:43Z

            {
                auto tx = vault.set({.owner = owner, .id = keylet.key});
-                tx[sfFlags] = tfVaultPrivate;
+                tx[sfFlags] = tfVaultDepositBlock | tfVaultDepositUnblock;


Reviewer note: tfVaultPrivate and tfVaultDepositBlock have identical numerical values.

Reviewer note: tfVaultPrivate and tfVaultDepositBlock have identical numerical values.

Is that a coincidence, or does it have functional implications?

Also, if you have a reason to want this intentionally, enforce it in the code with a static_assert. (There's an example in the "VaultCreate flags".)

It's just a coincidence, there are no implications. The comment is to say that even though I changed the flags, the unit-test still tests the same logic: an invalid flag.

It's just a coincidence, there are no implications. The comment is to say that even though I changed the flags, the unit-test still tests the same logic: an invalid flag.

Oh, I see. tfVaultPrivate was never a valid flag for VaultSet, but since you added tfVaultDepositBlock with the same value, it would have looked valid, so you had to change it to an invalid combo.

…ck-deposit

codecov · 2026-02-17T13:21:43Z

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 81.5%. Comparing base (68e4fbd) to head (f2495dc).

Additional details and impacted files

@@                      Coverage Diff                       @@
##           tapanito/lending-fix-amendment   #6361   +/-   ##
==============================================================
  Coverage                            81.5%   81.5%           
==============================================================
  Files                                 999     999           
  Lines                               74550   74595   +45     
  Branches                             7560    7553    -7     
==============================================================
+ Hits                                60722   60778   +56     
+ Misses                              13828   13817   -11

Files with missing lines	Coverage Δ
include/xrpl/protocol/LedgerFormats.h	`100.0% <ø> (ø)`
include/xrpl/protocol/TxFlags.h	`100.0% <ø> (ø)`
include/xrpl/tx/transactors/vault/VaultClawback.h	`100.0% <ø> (ø)`
include/xrpl/tx/transactors/vault/VaultDeposit.h	`100.0% <ø> (ø)`
include/xrpl/tx/transactors/vault/VaultSet.h	`100.0% <ø> (ø)`
include/xrpl/tx/transactors/vault/VaultWithdraw.h	`100.0% <ø> (ø)`
src/libxrpl/ledger/helpers/VaultHelpers.cpp	`100.0% <100.0%> (ø)`
src/libxrpl/tx/transactors/vault/VaultClawback.cpp	`97.7% <100.0%> (-<0.1%)`	⬇️
src/libxrpl/tx/transactors/vault/VaultCreate.cpp	`96.6% <100.0%> (+0.1%)`	⬆️
src/libxrpl/tx/transactors/vault/VaultDelete.cpp	`91.8% <ø> (ø)`
... and 3 more

... and 6 files with indirect coverage changes

🚀 New features to boost your workflow:

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

…ck-deposit

ximinez

Partial review

Move vault share/asset conversion functions (assetsToSharesDeposit, sharesToAssetsDeposit, assetsToSharesWithdraw, sharesToAssetsWithdraw) and isVaultInsolvent from View.h/View.cpp into a dedicated VaultHelpers.h/VaultHelpers.cpp module. Reorder includes in Vault transactor .cpp files to place own header first. Fix VaultSet flag validation logic.

…ck-deposit

ximinez · 2026-02-24T22:52:21Z

+                     Vault& vault,
+                     PrettyAsset const& asset,
+                     auto&&...) {
+            testcase("IOU insolvent vault blocks deposits");


I see tests for MPT and IOU. Should you also do XRP?

I made additional changes to tests:

I moved the "general" checks from MPT/IOU unit tests to testVaultDepositBlockGeneral(). There is no benefit to test that blocking / unblocking works for all assets.

Regarding XRP tests, I noticed that there are no XRP tests for SAV. I'll add insolvency unit tests, but full XRP sav tests are out of scope.

ximinez · 2026-02-24T23:10:32Z

+    if (ctx.view.rules().enabled(fixLendingProtocolV1_1))
+    {
+        // Perform these checks early to avoid unnecessary processing
+
+        // The Vault is insolvent, deposits are not allowed
+        if (isVaultInsolvent(vault, sleShareIssuance))
+        {
+            JLOG(ctx.j.debug()) << "VaultDeposit: Vault is insolvent, deposits are not allowed";
+            return tecNO_PERMISSION;
+        }
+
+        if (vault->isFlag(lsfVaultDepositBlocked))
+        {
+            JLOG(ctx.j.debug()) << "VaultDeposit: Vault deposits are blocked";
+            return tecNO_PERMISSION;
+        }
+    }


Maybe these should both be ter results. You'll probably need to create a new one - terNO_PERMISSION, maybe. That may make some of your tests more complicated because the transactions that now fail will be retried when you close the Env.

Consider:

In both cases, the Vault owner could unblock the Vault, or make it solvent, immediately after this transaction. From TER.h:
// Cause: // Prior application of another, possibly non-existent, transaction could // allow this transaction to succeed.

The deposit transaction and the Vault owner's transaction could be in the same ledger. If both transactions survive consensus, the ordering is unpredictable.

Vault owner unblocks deposits: If the owner's unblock is ordered after the deposit, then with a tec, the deposit fee is claimed and the transaction is done. With a ter, the deposit will be tried again after the unblock, and succeed. A better user experience.

Vault owner blocks deposits: Conversely, if the owner's block is ordered after the deposit, the deposit succeeds. If it's ordered before the deposit, the transaction is not included in the validated ledger - an acceptable outcome. The depositor doesn't lose their fee. If the owner then unblocks before the deposit expires, the deposit will succeed on that ledger. OTOH, if the depositor does not unblock, the transaction may not achieve finality for a while, so there are downsides.

Attack vector? Since I'm talking about the possibility of a transaction getting broadcast, then not claim a fee, there is the possibility of a malicious user using this to waste resources. It is possible, but it would require lucky timing to have any meaningful effect. The vault owner could not launch the attack easily by themselves - the spec says there should be an exception for them. Even without an exception, they'd have to use tickets, which comes with its own unpredictability. So now you need an attacker account, and cooperation with the vault owner (or knowledge of the vault owner's plans). One account sends a bunch of VaultDeposits, then the vault owner sends a VaultSet blocking deposits. In consensus, which set of transactions goes first is unpredictable. Any deposits ordered first will succeed. If the VaultSet is deferred to the next ledger, all of the deposits in the current ledger will succeed. If the VaultSet is in the same ledger and comes first, then the deposits may not get validated. The more deposits there are, the greater the chance that the VaultSet will not be broadcast in time to be validated in the same ledger. And once the Vault is viewed as locked, no further deposits will be broadcast from the receiving node. In short, there are more effective ways to waste resources.

A simple change, yet so much subtilty. I guess a lid to a can of worms is usually plain..

Philosophy aside, in an ideal world, VaultSet transaction would be prioritised over other vault transactions to prevent race conditions, alas we do not live in such a world. Especially, if the VaultOwner is blocking deposits this may be for a good reason, perhaps to protect depositors, or prevent diluting shares of existing depositors.

Vault owner unblocks deposits: If the owner's unblock is ordered after the deposit, then with a tec, the deposit fee is claimed and the transaction is done. With a ter, the deposit will be tried again after the unblock, and succeed. A better user experience.

This is a better user experience, but at the same time, I vaguely remember us wanting to minimize ter codes as much as possible, though I don't remember the rationale. I suppose, because, as we say, sometimes you eat the bear, sometimes the bear eats you. A better UX is a compelling argument though.

Vault owner blocks deposits: Conversely, if the owner's block is ordered after the deposit, the deposit succeeds. If it's ordered before the deposit, the transaction is not included in the validated ledger - an acceptable outcome. The depositor doesn't lose their fee. If the owner then unblocks before the deposit expires, the deposit will succeed on that ledger. OTOH, if the depositor does not unblock, the transaction may not achieve finality for a while, so there are downsides. [ ... ]

How long does it take for a transaction to expire? I believe the transaction is punted to the next open ledger, where if it fails it fails. I think such behaviour is not worth it, i.e. given that transaction order is unpredictable, blocking and unblocking deposits back to back is nonsensical behaviour, as there are no guarantees that the blocking action will block the deposits that are in the same ledger, and similarly there are no guarantees about unblock.

At the end, I think the UX improvement is marginal, is it really worth adding a ter code for it? 🤔

ximinez · 2026-02-24T23:11:28Z

+        if (isVaultInsolvent(vault, sleShareIssuance))
+        {
+            JLOG(ctx.j.debug()) << "VaultDeposit: Vault is insolvent, deposits are not allowed";
+            return tecNO_PERMISSION;


The proposed updates to the spec indicate this should be tecLOCKED. XRPLF/XRPL-Standards#469 Section 6.2.2 item 7

Is it okay to use tecLOCKED in this situation? That particular tec code is only used with MPTs as far as I know. It might be confusing for users to return it in such situation.

Tapanito · 2026-02-25T12:42:50Z

@ximinez , there was a small change to the spec. We introduced a lsfVaultOwnerCanBlockDeposit flag to the Vault object.

This flag acts as a signal to depositors that the owner of this particular vault may block deposits. Thus, this flag can only be set when creating the vault.

If Vault.lsfVaultOwnerCanBlockDeposit is not set, blocking deposits is now allowed. Though, note, that deposits to an insolvent vault are still prohibited.

In contrast, if the flag is set, the owner may block / unblock the Vault deposits as they see fit.

…into tapanito/vault-block-deposit

…ck-deposit

…into tapanito/vault-block-deposit

…ck-deposit

Tapanito · 2026-03-21T13:42:30Z

/ai-review

xrplf-ai-reviewer

Critical amendment flag logic issues discovered across vault transaction handlers.

Review by Claude Opus 4.6 · Prompt: V12

…into tapanito/vault-block-deposit

github-actions · 2026-03-26T16:27:11Z

This PR has conflicts, please resolve them in order for the PR to be reviewed.

a1q123456 · 2026-03-31T10:49:52Z

+            return tecNO_PERMISSION;
+        }
+
+        if (!vault->isFlag(lsfVaultDepositBlocked) && ctx.tx.isFlag(tfVaultDepositUnblock))


I think it may make sense to allow cases where the flag we want to add already exists or the flag we want to remove doesn't exist.

We don't return an error when a transaction is updating another field and the value is the same as the current value, but we do return an error when the flag is already added, which feels inconsistent.

…into tapanito/vault-block-deposit

github-actions · 2026-03-31T12:57:25Z

All conflicts have been resolved. Assigned reviewers can now start or resume their review.

xrplf-ai-reviewer

LGTM

Review by Claude Opus 4.6 · Prompt: V12

Tapanito added 2 commits February 12, 2026 15:27

adds flag definitions

c808c46

adds BLockDeposit flagsto VaultSet

a219814

Tapanito changed the base branch from develop to tapanito/lending-fix-amendment February 12, 2026 16:43

Tapanito added 3 commits February 13, 2026 15:00

adds amendment validation in flags and better tests

1010866

add vault deposit logic and tests

087a9c1

minor code improvements

82b0d57

Tapanito requested review from a1q123456 and ximinez February 17, 2026 12:35

Tapanito changed the title ~~[W.I.P] Adds functionality to block vault deposits~~ Adds functionality to block vault deposits Feb 17, 2026

Tapanito commented Feb 17, 2026

View reviewed changes

Tapanito added 2 commits February 17, 2026 13:47

removes redundant check

f57b715

Merge branch 'tapanito/lending-fix-amendment' into tapanito/vault-blo…

71b9f98

…ck-deposit

Tapanito added 4 commits February 19, 2026 11:11

additional unit test

3cfb5fe

debug attempt

cd1e8eb

Merge branch 'tapanito/lending-fix-amendment' into tapanito/vault-blo…

d972071

…ck-deposit

adds missing env.close

884530e

ximinez requested changes Feb 19, 2026

View reviewed changes

Comment thread src/libxrpl/tx/transactors/Vault/VaultSet.cpp Outdated

Comment thread src/libxrpl/ledger/View.cpp Outdated

Tapanito added 4 commits February 24, 2026 12:56

Merge branch 'tapanito/lending-fix-amendment' into tapanito/vault-blo…

81e69b9

…ck-deposit

Merge branch 'tapanito/lending-fix-amendment' into tapanito/vault-blo…

b08451c

…ck-deposit

fixes broken unit-tests

8723472

ximinez reviewed Feb 24, 2026

View reviewed changes

adds lsfVaultOwnerCanBlockDeposit flag

2b716eb

ximinez mentioned this pull request Feb 25, 2026

adds VaultDepositBlock flag XRPLF/XRPL-Standards#469

Open

6 tasks

Tapanito added 4 commits February 26, 2026 14:43

Merge remote-tracking branch 'origin/tapanito/lending-fix-amendment' …

f034ca0

…into tapanito/vault-block-deposit

Merge branch 'tapanito/lending-fix-amendment' into tapanito/vault-blo…

3deb0de

…ck-deposit

fixes formattting issues after merge

ebfa659

fixes typos and improves test coverage

cc9dbe2

ximinez self-requested a review March 4, 2026 22:11

Tapanito added 2 commits March 9, 2026 13:03

Merge remote-tracking branch 'origin/tapanito/lending-fix-amendment' …

68d7555

…into tapanito/vault-block-deposit

Merge branch 'tapanito/lending-fix-amendment' into tapanito/vault-blo…

7e5a4c9

…ck-deposit

xrplf-ai-reviewer Bot reviewed Mar 21, 2026

View reviewed changes

Comment thread src/libxrpl/tx/transactors/vault/VaultSet.cpp

Comment thread src/libxrpl/tx/transactors/vault/VaultDeposit.cpp

Comment thread src/libxrpl/tx/transactors/vault/VaultCreate.cpp Outdated

Comment thread src/libxrpl/tx/transactors/vault/VaultSet.cpp Outdated

Tapanito added 3 commits March 21, 2026 17:43

fix: address PR review feedback

b970c66

fixes failing unit-tests

5fe99dc

Merge remote-tracking branch 'origin/tapanito/lending-fix-amendment' …

8899d34

…into tapanito/vault-block-deposit

github-actions Bot added the PR: has conflicts label Mar 26, 2026

a1q123456 reviewed Mar 31, 2026

View reviewed changes

Merge remote-tracking branch 'origin/tapanito/lending-fix-amendment' …

f2495dc

…into tapanito/vault-block-deposit

github-actions Bot removed the PR: has conflicts label Mar 31, 2026

xrplf-ai-reviewer Bot approved these changes Mar 31, 2026

View reviewed changes

Conversation

Tapanito commented Feb 12, 2026

High Level Overview of Change

Context of Change

Type of Change

API Impact

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!

codecov Bot commented Feb 17, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

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!

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!

Uh oh!

Tapanito commented Feb 25, 2026

Uh oh!

Tapanito commented Mar 21, 2026

Uh oh!

xrplf-ai-reviewer Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

github-actions Bot commented Mar 26, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

github-actions Bot commented Mar 31, 2026

Uh oh!

xrplf-ai-reviewer Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

codecov Bot commented Feb 17, 2026 •

edited

Loading