Single Asset Vault #5224

Bronek · 2024-12-17T12:13:31Z

High Level Overview of Change

This is implementation of XLS-65 Single Asset Vault. First draft was implemented by @thejohnfreeman in #5147

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)

gregtatcam

I got compile error on MAC M1 Sequoia 15.1.1, apple clang 16.0.0

xrpl/json/json_value.h:705:5: error: constexpr function's return type 'Value' is not a literal type
705 |     operator()(T&& t) const
xrpl/json/json_value.h:148:7: note: 'Value' is not literal because it is not an aggregate and has no constexpr constructors other than copy or move constructors
148 | class Value

gregtatcam

There are a few warnings, of which the most common is:

xrpl/protocol/STIssue.h:49:5: warning: definition of implicit copy constructor for 'STIssue' is deprecated because it has a user-declared copy assignment operator [-Wdeprecated-copy]
   49 |     operator=(STIssue const& rhs) = default;

And once the compile error is fixed (remove constexpr in to_json_fn::operator() return value), there are VaultDelete unit-tests failures.

Bronek · 2024-12-18T15:03:12Z

There are a few warnings, of which the most common is:
xrpl/protocol/STIssue.h:49:5: warning: definition of implicit copy constructor for 'STIssue' is deprecated because it has a user-declared copy assignment operator [-Wdeprecated-copy]
   49 |     operator=(STIssue const& rhs) = default; 
And once the compile error is fixed (remove constexpr in to_json_fn::operator() return value), there are VaultDelete unit-tests failures.

This is fixed now. The VaultDelete still needs to be fixed, but for the time being I've commented the failing test out. Compilation errors in clang 16 fixed, also simplified json_value.h a little.

Also remove potential ODR violation from json_value.h

codecov · 2024-12-18T17:52:20Z

Codecov Report

Attention: Patch coverage is 75.51582% with 178 lines in your changes missing coverage. Please review.

Project coverage is 78.0%. Comparing base (ed4870c) to head (02dec4f).

Files with missing lines	Patch %	Lines
src/xrpld/app/misc/CredentialHelpers.cpp	21.6%	40 Missing ⚠️
src/xrpld/ledger/detail/View.cpp	85.1%	21 Missing ⚠️
src/xrpld/app/tx/detail/VaultDeposit.cpp	70.8%	19 Missing ⚠️
src/xrpld/app/tx/detail/VaultCreate.cpp	78.8%	18 Missing ⚠️
src/xrpld/app/tx/detail/VaultClawback.cpp	69.8%	16 Missing ⚠️
src/xrpld/app/tx/detail/VaultSet.cpp	68.9%	14 Missing ⚠️
src/xrpld/app/tx/detail/VaultWithdraw.cpp	70.7%	12 Missing ⚠️
src/xrpld/app/tx/detail/VaultDelete.cpp	80.0%	9 Missing ⚠️
src/libxrpl/protocol/STNumber.cpp	77.1%	8 Missing ⚠️
src/xrpld/app/tx/detail/MPTokenAuthorize.cpp	78.9%	4 Missing ⚠️
... and 6 more

Additional details and impacted files

@@            Coverage Diff            @@
##           develop   #5224     +/-   ##
=========================================
- Coverage     78.0%   78.0%   -0.0%     
=========================================
  Files          789     801     +12     
  Lines        67007   67593    +586     
  Branches      8110    8175     +65     
=========================================
+ Hits         52274   52707    +433     
- Misses       14733   14886    +153

Files with missing lines	Coverage Δ
include/xrpl/json/json_value.h	`98.5% <ø> (ø)`
include/xrpl/protocol/AMMCore.h	`100.0% <ø> (ø)`
include/xrpl/protocol/Asset.h	`95.2% <100.0%> (+0.5%)`	⬆️
include/xrpl/protocol/Feature.h	`100.0% <ø> (ø)`
include/xrpl/protocol/IOUAmount.h	`100.0% <ø> (ø)`
include/xrpl/protocol/Indexes.h	`100.0% <100.0%> (ø)`
include/xrpl/protocol/MPTAmount.h	`100.0% <ø> (ø)`
include/xrpl/protocol/MPTIssue.h	`100.0% <100.0%> (ø)`
include/xrpl/protocol/SField.h	`100.0% <ø> (ø)`
include/xrpl/protocol/STAmount.h	`95.5% <100.0%> (+0.3%)`	⬆️
... and 44 more

... and 6 files with indirect coverage changes

gregtatcam

I took a first pass at the review. It looks overall pretty good.
I still need to review the Vault unit-tests.

include/xrpl/protocol/detail/ledger_entries.macro

gregtatcam · 2024-12-18T19:44:27Z

include/xrpl/protocol/detail/ledger_entries.macro

+    {sfAccount,              soeREQUIRED},
+    {sfData,                 soeDEFAULT},
+    {sfAsset,                soeREQUIRED},
+    {sfAssetTotal,           soeDEFAULT},


AssetTotal, AssetAvailable, LossUnrealized are marked as required in the specifications.

Also, AssetTotal, AssetMaximum, and AssetAvailable are plural in the specifications - AssetsTotal, AssetsMaximum, and AssetsAvailable.

NVM the first comment. These fields have a default value.

gregtatcam · 2024-12-18T19:45:00Z

include/xrpl/protocol/detail/ledger_entries.macro

+    {sfAssetAvailable,       soeDEFAULT},
+    {sfAssetMaximum,         soeDEFAULT},
+    {sfLossUnrealized,       soeDEFAULT},
+    {sfMPTokenIssuanceID,    soeREQUIRED}, // sfShare


Are these three fields slated for SAV v1 and just not implemented yet?

ShareTotal is not going to be implemented. We decided to just use the bookkeeping in mptIssuance(vault.sfMPTIssuanceID).sfOutstandingAmount. However, the RPC for ledger_object (or whatever it's called) needs to join that field in the result.

Right now, there is only one WithdrawalPolicy. IMO, it can be added (and tested) once a second is introduced.

PermissionedDomainID is planned for v1, but isn't yet available in develop. Its PR (Permissioned Domains (XLS-80d) #5161) needs to be merged with this branch.

gregtatcam · 2024-12-18T20:58:10Z

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

+        if ((issuance->getFlags() & lsfMPTCanTransfer) == 0)
+            return tecLOCKED;
+    }
+


According to the specs there should be a check added for a frozen trustline.

src/xrpld/ledger/detail/View.cpp

gregtatcam · 2024-12-19T20:16:06Z

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

+    vault->at(sfAssetAvailable) -= assets;
+    view().update(vault);
+
+    // auto const& vaultAccount = vault->at(sfAccount);


Why is this block of code commented out?

I was in the middle of tracking down a bug in this transaction.

gregtatcam · 2024-12-19T20:21:41Z

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

    }
+    auto& account = *maybeAccount;
+    auto const accountId = (*account)[sfAccount];


consider renaming to ammAccountId to emphasize that this AMM's pseudo account.

gregtatcam · 2024-12-19T20:22:45Z

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

@@ -327,7 +327,8 @@ AccountRootsNotDeleted::finalize(
    // A successful AccountDelete or AMMDelete MUST delete exactly
    // one account root.


Should add comments for the Vault.

gregtatcam · 2024-12-19T20:32:08Z

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

-            auto const sleMpt = view.peek(mptokenKey);
-            if (!sleMpt || (*sleMpt)[sfMPTAmount] != 0)
-                return tecINTERNAL;
+            if (!sleMpt)


If we do decide to go ahead with these changes (here and in MPTokenIssuance[Create,Destroy] then any return code changes have to be amendment gated. It makes sense for @shawnxie999 to review.

gregtatcam · 2024-12-19T20:45:57Z

src/libxrpl/protocol/STNumber.cpp

@@ -108,4 +110,101 @@ operator<<(std::ostream& out, STNumber const& rhs)
    return out << rhs.getText();
 }

+NumberParts
+partsFromString(std::string const& number)


This is copied from the original implementation of amountFromString() but this check is missing:

if ((match[2].length() + match[4].length()) > 32) Throw<std::runtime_error>("Number '" + amount + "' is overlong");

I don't have access to the Slack any more, but I had asked about this there and explained why I left it out.

thanks @thejohnfreeman , I found it

separately, that function has a CHECKME about its length check. it checks that the non-fractional and fractional strings do not concatenate longer than 32 characters. the CHECKME asks if that should be 16 characters. I'm guessing this check is to make sure the base-10 digit string can be parsed into an unsigned integer 64 bits long, but both of those lengths are wrong. 2^64 - 1 has a decimal representation that is 20 digits long. of course you can't put just any digits in those 20 places, but this length check seems misguided anyway, because the parsing is handled by beast::lexicalCastThrow, which will throw if the string overflows 64 bits unsigned. am I reading this right?

gregtatcam

I think the following unit-tests are missing:

Vault feature disabled for all transactors
Invalid flags for all transactors (except for set:tfVaultPrivate)
Insufficient fee (except for create)
Vault is not found (except for delete)
Set:
- Data too large
Deposit:
- Deposit into private vault
- Amount to deposit is not the vault's asset
- Checking for the reserve if the depositor doesn't have MPToken for the Vault shares
Withdraw
- Amount to withdraw is not the vault's asset
- Vault is private
Clawback:
- Amount to clawback is not the vault's asset
- Insufficient funds to withdraw

thejohnfreeman added 16 commits December 17, 2024 11:10

Integrate STNumber with STParsedJSON

1a032f0

Start vault implementation

ff8c649

wip

a8ec8e7

format

d09e74e

progress

1680477

progress

ebc97ae

progress

286612c

progress

6046fa2

progress

1255216

AND_THEN

1ff1274

wip

ea6c040

fix

ea30f44

revise tests

12646cb

remove blob constants

54d511b

bless sfAsset to carry MPT

b5619fb

wip clawback

ad58166

gregtatcam reviewed Dec 17, 2024

View reviewed changes

Fix failing MPToken payment test

ecec6e5

Bronek force-pushed the vault branch from f2dd852 to ecec6e5 Compare December 17, 2024 18:05

gregtatcam reviewed Dec 17, 2024

View reviewed changes

Fix compilation errors

eae2cd3

Bronek force-pushed the vault branch 3 times, most recently from 2bcc6ad to 17af6e7 Compare December 18, 2024 17:23

Bronek added 3 commits December 18, 2024 17:30

Comment out failing test

526f715

Fix formatting

b9f1200

Simplify conversions to Json::Value

241429b

Also remove potential ODR violation from json_value.h

Bronek force-pushed the vault branch from 17af6e7 to 241429b Compare December 18, 2024 17:34

gregtatcam reviewed Dec 19, 2024

View reviewed changes

gregtatcam reviewed Dec 20, 2024

View reviewed changes

Merge branch 'develop' into vault

8d5cfb9

Bronek mentioned this pull request Jan 15, 2025

fix: Use the same compiler options for xrpl.libxrpl and all its submodules #5228

Merged

1 task

Bronek added 3 commits January 15, 2025 15:49

Merge branch 'develop' into vault

bf8bbff

Merge branch 'develop' into vault

375614d

Fix Vault unit tests

c325b6c

Bronek force-pushed the vault branch 5 times, most recently from 296cdec to 201a310 Compare January 17, 2025 19:09

WIP permissioned domain support

dbaa12a

Bronek force-pushed the vault branch from 201a310 to dbaa12a Compare January 17, 2025 19:16

WIP support adding domainID in VaultSet

580a85f

Bronek force-pushed the vault branch from e4bf999 to 580a85f Compare January 21, 2025 13:39

Remove temSTRING_TOO_LARGE, fix authorizedDomain, refactor Vault_test

c08f86d

Bronek force-pushed the vault branch from 13edf9d to c08f86d Compare January 22, 2025 18:38

Add VaultID to account

25a20ad

Bronek force-pushed the vault branch from dcd5eda to 8c14fd0 Compare January 23, 2025 20:47

Verify credentials in VaultDeposit

2c4a224

Bronek force-pushed the vault branch from 8c14fd0 to 2c4a224 Compare January 23, 2025 20:50

Bronek added 4 commits January 24, 2025 11:06

Merge branch 'develop' into vault

9bf9a1e

Fix checo of expired credential

86d249a

Unit tests naming fix

65d6c48

Merge branch 'develop' into vault

02dec4f

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Single Asset Vault #5224

Single Asset Vault #5224

Bronek commented Dec 17, 2024

gregtatcam left a comment

gregtatcam left a comment

Bronek commented Dec 18, 2024

codecov bot commented Dec 18, 2024 •

edited

Loading

gregtatcam left a comment

gregtatcam Dec 18, 2024

gregtatcam Dec 18, 2024

gregtatcam Dec 20, 2024

gregtatcam Dec 18, 2024

thejohnfreeman Dec 23, 2024

gregtatcam Dec 18, 2024

gregtatcam Dec 19, 2024

thejohnfreeman Dec 23, 2024

gregtatcam Dec 19, 2024

gregtatcam Dec 19, 2024

gregtatcam Dec 19, 2024

gregtatcam Dec 19, 2024

thejohnfreeman Dec 23, 2024

Bronek Jan 15, 2025

gregtatcam left a comment

		@@ -327,7 +327,8 @@ AccountRootsNotDeleted::finalize(
		// A successful AccountDelete or AMMDelete MUST delete exactly
		// one account root.

Single Asset Vault #5224

Are you sure you want to change the base?

Single Asset Vault #5224

Conversation

Bronek commented Dec 17, 2024

High Level Overview of Change

Context of Change

Type of Change

API Impact

gregtatcam left a comment

Choose a reason for hiding this comment

gregtatcam left a comment

Choose a reason for hiding this comment

Bronek commented Dec 18, 2024

codecov bot commented Dec 18, 2024 • edited Loading

Codecov Report

gregtatcam left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gregtatcam left a comment

Choose a reason for hiding this comment

codecov bot commented Dec 18, 2024 •

edited

Loading