Add `Scale` to SingleAssetVault #5652

Bronek · 2025-08-06T16:20:18Z

High Level Overview of Change

This change adds Scale to SAV when asset is an IOU, per XRPLF/XRPL-Standards#301

It also removes empty MPToken for vault shares when VaultClawback or VaultWithdraw zeroes the holders shares balance.

Context of Change

SAV shares are always expressed as an integer, since they are MPTs. We need to ensure that this loss of precision does not cause problems when asset has fractional part, like IOUs do.

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

codecov · 2025-08-07T12:09:01Z

Codecov Report

❌ Patch coverage is 97.92531% with 5 lines in your changes missing coverage. Please review.
✅ Project coverage is 78.7%. Comparing base (c38f2a3) to head (011c3c6).
⚠️ Report is 1 commits behind head on develop.

Files with missing lines	Patch %	Lines
src/xrpld/app/tx/detail/VaultCreate.cpp	89.7%	3 Missing ⚠️
src/xrpld/app/tx/detail/VaultClawback.cpp	98.5%	1 Missing ⚠️
src/xrpld/app/tx/detail/VaultDeposit.cpp	97.2%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff            @@
##           develop   #5652     +/-   ##
=========================================
+ Coverage     78.7%   78.7%   +0.1%     
=========================================
  Files          816     816             
  Lines        71748   71924    +176     
  Branches      8477    8466     -11     
=========================================
+ Hits         56440   56627    +187     
+ Misses       15308   15297     -11

Files with missing lines	Coverage Δ
include/xrpl/basics/Number.h	`100.0% <100.0%> (ø)`
include/xrpl/protocol/detail/ledger_entries.macro	`100.0% <ø> (ø)`
include/xrpl/protocol/detail/transactions.macro	`100.0% <ø> (ø)`
src/xrpld/app/tx/detail/InvariantCheck.cpp	`89.2% <100.0%> (+0.1%)`	⬆️
src/xrpld/app/tx/detail/VaultDelete.cpp	`92.4% <100.0%> (+0.8%)`	⬆️
src/xrpld/app/tx/detail/VaultSet.cpp	`100.0% <100.0%> (ø)`
src/xrpld/app/tx/detail/VaultWithdraw.cpp	`96.8% <100.0%> (+2.7%)`	⬆️
src/xrpld/ledger/View.h	`100.0% <ø> (ø)`
src/xrpld/ledger/detail/View.cpp	`91.7% <100.0%> (+0.9%)`	⬆️
src/xrpld/app/tx/detail/VaultClawback.cpp	`95.5% <98.5%> (+3.5%)`	⬆️
... and 2 more

... and 4 files with indirect coverage changes

🚀 New features to boost your workflow:

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

shawnxie999 · 2025-08-26T16:57:24Z

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

+    if (auto const ter = accountSend(
+            view(),
+            vaultAccount,
+            dstAcct,
+            assetsWithdrawn,
+            j_,
+            WaiveTransferFee::Yes))


some other places in this PR uses isTesSuccess to check the result. I think we'd prefer to use this for explicitness and consistency

thanks, added explicit isTesSuccess(ter) . I also prefer it, only missed in few places, thanks for spotting it !

shawnxie999 · 2025-08-26T16:58:14Z

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

+    if (auto const ter = accountSend(
+            view(),
+            holder,
+            vaultAccount,
+            sharesDestroyed,
+            j_,
+            WaiveTransferFee::Yes))


shawnxie999 · 2025-08-26T16:58:40Z

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

+    if (auto const ter = accountSend(
+            view(),
+            account_,
+            vaultAccount,
+            sharesRedeemed,
+            j_,
+            WaiveTransferFee::Yes))


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

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

src/xrpld/ledger/detail/View.cpp

src/test/app/Vault_test.cpp

Co-authored-by: Ed Hennis <[email protected]>

… account check

ximinez

I have one suggestion about a log message, but it's so minor that I'm going to approve this whether you add it or not.

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

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

gregtatcam · 2025-09-02T11:14:54Z

include/xrpl/basics/Number.h

        return (mantissa_ < 0) ? -1 : (mantissa_ ? 1 : 0);
    }

+    Number


Can simplify or maybe just use combination of towards_zero rounding and static_cast since truncate() usage is limited.

inline Number Number::truncate() const noexcept { if (exponent_ >= 0 || mantissa_ == 0) return *this; NumberRoundModeGuard mg(Number::towards_zero); return static_cast<rep>(*this); }

I am not sure if that would be an improvement, given the complexity of operator rep(). On the other hand, normalize() is also quite complex. However with the current implementation we can guarantee noexcept and I am not sure how this would look like if we used operator rep()

I will add a comment about noexcept and leave it like this for now.

gregtatcam · 2025-09-02T14:09:12Z

include/xrpl/protocol/detail/transactions.macro

    {sfDomainID, soeOPTIONAL},
    {sfWithdrawalPolicy, soeOPTIONAL},
    {sfData, soeOPTIONAL},
+    {sfScale, soeOPTIONAL},


Should this be sfShareScale to stress that it applies to the shares?

The sfScale is used to similar effect elsewhere already; I think it's better to stick to this name.

gregtatcam · 2025-09-02T14:23:02Z

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


-    // Compute exchange before transferring any amounts.
-    auto const shares = assetsToSharesDeposit(vault, sleIssuance, assets);
+    STAmount sharesCreated = {vault->at(sfShareMPTID)}, assetsDeposited;


Consider defining assetsDeposited on a separate line. I don't think we really do this chained definition. It's also easier to read if defined on a separate line.

gregtatcam · 2025-09-02T14:38:14Z

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

    auto const share = MPTIssue(mptIssuanceID);
-    STAmount shares, assets;
-    if (amount.asset() == asset)
+    STAmount sharesRedeemed = {vault->at(sfShareMPTID)}, assetsWithdrawn;


Consider defining assetsWithdraw on a separate line for a better readability and style consistency.

gregtatcam · 2025-09-02T15:02:32Z

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

+
+            if (sharesRedeemed == beast::zero)
+                return tecPRECISION_LOSS;
+            auto const maybeAssets =


Because of the truncation/recalculation, which is done on deposit and recalculation here then is it possible that if I deposit A and get S; if I withdraw A then I'll redeem more than S and likewise, if I withdraw S then I'll get less than A? Another case would be also bad; i.e. I withdraw A and redeem less than S and I withdraw S and get more than A. The same concern applies to VaultClawback.

If we are given an A (number of assets) then we always (i.e. in VaultDeposit VaultClawback VaultWithdraw) do roundtrip calculation : first from assets to shares, then from such rounded shares to assets again. This guarantees that the number of assets always corresponds with the number of shares, bar rounding of assets (for MPT or XRP) which can go either way. This is similar how DEX works when one side is XRP - which also can go either way due to rounding. If the asset is an IOU then there is no rounding of assets (other than epsilon) which is also similar to how DEX works.

gregtatcam · 2025-09-02T15:03:35Z

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

-        assets = sharesToAssetsWithdraw(vault, sleIssuance, shares);
-    }
-    else
+    STAmount sharesDestroyed = {vault->at(sfShareMPTID)}, assetsRecovered;


Consider defining assetsRecovered on a separate line.

gregtatcam · 2025-09-02T15:22:05Z

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

+    {
+        if (auto const ter =
+                removeEmptyHolding(view(), holder, sharesDestroyed.asset(), j_);
+            isTesSuccess(ter))


Shouldn't error be returned on failure?

Maybe. We are opportunistically trying to remove MPToken in case if its balance is zero. This is expected to fail if the balance is not zero, and we want to ignore this error.

On the other hand, reporting other errors would be useful here. I will add it.

gregtatcam

👍 LGTM

ximinez

I have couple of stylistic-ish nits, but otherwise looks good.

ximinez · 2025-09-03T15:39:26Z

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

    auto const mptIssuanceID = (*vault)[sfShareMPTID];
-    auto const sleIssuance = view().read(keylet::mptIssuance(mptIssuanceID));
+    auto const sleIssuance = view().read(keylet::mptIssuance(*mptIssuanceID));


Rather than using the * operator every time you access mptIssuanceID, you could do it when you do the initial lookup:

Suggested change

auto const mptIssuanceID = (*vault)[sfShareMPTID];

auto const sleIssuance = view().read(keylet::mptIssuance(mptIssuanceID));

auto const sleIssuance = view().read(keylet::mptIssuance(*mptIssuanceID));

auto const mptIssuanceID = *(*vault)[sfShareMPTID];

auto const sleIssuance = view().read(keylet::mptIssuance(mptIssuanceID));

thanks, implemented in 6893015 (with extra parents for readability)

ximinez · 2025-09-03T15:43:07Z

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

-        if (auto const ter =
-                removeEmptyHolding(view(), holder, sharesDestroyed.asset(), j_);
-            isTesSuccess(ter))
+        auto const ter =
+            removeEmptyHolding(view(), holder, sharesDestroyed.asset(), j_);
+        if (isTesSuccess(ter))


Even with the else if, ter is only used in the context of the if, so you can put it back.

Suggested change

if (auto const ter =

removeEmptyHolding(view(), holder, sharesDestroyed.asset(), j_);

isTesSuccess(ter))

auto const ter =

removeEmptyHolding(view(), holder, sharesDestroyed.asset(), j_);

if (isTesSuccess(ter))

if (auto const ter =

removeEmptyHolding(view(), holder, sharesDestroyed.asset(), j_);

isTesSuccess(ter))

thanks, implemented in 6893015

ximinez · 2025-09-03T15:44:24Z

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

    auto const mptIssuanceID = (*vault)[sfShareMPTID];
-    auto const sleIssuance = view().read(keylet::mptIssuance(mptIssuanceID));
+    auto const sleIssuance = view().read(keylet::mptIssuance(*mptIssuanceID));


Same as in VaultClawback, mptIssuanceID can be declared with the *.

thanks, implemented in 6893015 (with extra parents for readability)

ximinez · 2025-09-03T15:44:53Z

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

+        auto const ter =
+            removeEmptyHolding(view(), account_, sharesRedeemed.asset(), j_);
+        if (isTesSuccess(ter))


Same as in VaultClawback, ter can be put back into the if statement.

thanks, implemented in 6893015

ximinez

I'm realizing there's a lot of duplicated code between VaultClawback and VaultWithdraw that can be refactored into helper functions, but that can be done later.

Bronek added 3 commits August 4, 2025 14:17

Add AssetScale to VaultCreate

7d9dffc

Add AssetScale to VaultDeposit

b17b167

Add round-trip calculation to VaultWithdraw and VaultClawback

a38d7bf

Bronek added the DraftRunCI Normally CI does not run on draft PRs. This opts in. label Aug 6, 2025

Bronek added 2 commits August 6, 2025 17:20

Merge branch 'develop' into Bronek/Vault_add_AssetScale

622ef6e

Enable IOU permission test again

f374d5a

Bronek added 7 commits August 7, 2025 15:57

Implement Number::truncate()

87a9558

Fixes for truncation and unit tests

b302670

Merge branch 'develop' into Bronek/Vault_add_AssetScale

76fd2e8

Merge branch 'develop' into Bronek/Vault_add_AssetScale

6c31556

Only apply AssetScale to IOU

c788564

Rounding in DepositWithdraw

ff48d67

Merge branch 'develop' into Bronek/Vault_add_AssetScale

2b75960

Bronek changed the title ~~Add AssetScale to SingleAssetVault~~ Add ScaleFactor to SingleAssetVault Aug 13, 2025

Bronek added 2 commits August 15, 2025 15:40

More unit tests

a484b6e

Disallow zero shares withdraw or deposit

44b60fe

Bronek force-pushed the Bronek/Vault_add_AssetScale branch from 44422a5 to 44b60fe Compare August 15, 2025 15:11

Withdraw/clawback last of shares

09190db

Bronek force-pushed the Bronek/Vault_add_AssetScale branch 2 times, most recently from ecfbf80 to 5f54dae Compare August 15, 2025 20:11

Fix handling of AssetsAvailable in Clawback

97e44c7

Bronek force-pushed the Bronek/Vault_add_AssetScale branch from 5f54dae to 97e44c7 Compare August 15, 2025 20:23

Bronek added 3 commits August 15, 2025 21:27

Remve redundant check

35671ff

Merge branch 'develop' into Bronek/Vault_add_AssetScale

65dce68

Rename AssetScale to Scale, more tests

2047661

Bronek force-pushed the Bronek/Vault_add_AssetScale branch from 50a393d to 2047661 Compare August 18, 2025 14:31

Bronek changed the title ~~Add ScaleFactor to SingleAssetVault~~ Add Scale to SingleAssetVault Aug 18, 2025

Bronek added 2 commits August 19, 2025 09:37

Merge branch 'develop' into Bronek/Vault_add_AssetScale

d365a2d

Fix unit tests

f0e83dc

shawnxie999 reviewed Aug 26, 2025

View reviewed changes

Merge branch 'develop' into Bronek/Vault_add_AssetScale

93df6ce

ximinez requested changes Aug 27, 2025

View reviewed changes

shawnxie999 approved these changes Aug 27, 2025

View reviewed changes

Merge branch 'develop' into Bronek/Vault_add_AssetScale

1d7992b

Bronek force-pushed the Bronek/Vault_add_AssetScale branch from 7f6e13a to f28ce76 Compare August 28, 2025 11:53

Bronek and others added 2 commits August 28, 2025 12:58

Review feedback: do not ignore error from removeEmptyHolding

8c8e027

Co-authored-by: Ed Hennis <[email protected]>

Review feedback: add !isTesSuccess(ter)

d24632d

Bronek force-pushed the Bronek/Vault_add_AssetScale branch from f28ce76 to d24632d Compare August 28, 2025 11:58

Bronek added 3 commits August 28, 2025 13:11

Review feedback: use lsfVaultPrivate as appropriate, remove redundant…

0d046da

… account check

Review feedback: defensive checks in shares/assets calculations

d145666

Review feedback: comment update

37196b7

Bronek requested a review from ximinez August 28, 2025 13:45

ximinez approved these changes Aug 28, 2025

View reviewed changes

src/xrpld/app/tx/detail/VaultDelete.cpp Outdated Show resolved Hide resolved

src/xrpld/app/tx/detail/VaultDeposit.cpp Outdated Show resolved Hide resolved

Bronek added 2 commits August 29, 2025 12:32

Merge branch 'develop' into Bronek/Vault_add_AssetScale

c9e4b4a

Review feedback: logging

76c3cff

gregtatcam reviewed Sep 2, 2025

View reviewed changes

Bronek added 2 commits September 3, 2025 09:26

Merge branch 'develop' into Bronek/Vault_add_AssetScale

3689246

Review feedback

a763642

Bronek requested a review from gregtatcam September 3, 2025 09:47

gregtatcam approved these changes Sep 3, 2025

View reviewed changes

ximinez requested changes Sep 3, 2025

View reviewed changes

Minor improvements from review

6893015

Bronek requested a review from ximinez September 3, 2025 16:33

ximinez approved these changes Sep 3, 2025

View reviewed changes

Merge branch 'develop' into Bronek/Vault_add_AssetScale

011c3c6

Bronek enabled auto-merge (squash) September 3, 2025 17:16

Bronek 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 Sep 3, 2025

Bronek merged commit cf5f65b into develop Sep 4, 2025
67 of 70 checks passed

Bronek deleted the Bronek/Vault_add_AssetScale branch September 4, 2025 08:54

Add Scale to SingleAssetVault #5652

Add Scale to SingleAssetVault #5652

Uh oh!

Conversation

Bronek commented Aug 6, 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

Uh oh!

codecov bot commented Aug 7, 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!

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!

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!

gregtatcam left a comment

Choose a reason for hiding this comment

Uh oh!

ximinez left a comment

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!

ximinez left a comment

Choose a reason for hiding this comment

Uh oh!

Add `Scale` to SingleAssetVault #5652

Add `Scale` to SingleAssetVault #5652

Bronek commented Aug 6, 2025 •

edited

Loading

codecov bot commented Aug 7, 2025 •

edited

Loading