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 70.09569% with 250 lines in your changes missing coverage. Please review.

Project coverage is 78.0%. Comparing base (d9e4009) to head (9d619b9).

Files with missing lines	Patch %	Lines
src/xrpld/ledger/detail/View.cpp	59.9%	85 Missing ⚠️
src/xrpld/app/misc/CredentialHelpers.cpp	20.4%	43 Missing ⚠️
src/xrpld/app/tx/detail/VaultCreate.cpp	78.0%	20 Missing ⚠️
src/xrpld/app/tx/detail/VaultDeposit.cpp	74.3%	19 Missing ⚠️
src/xrpld/app/tx/detail/VaultClawback.cpp	70.9%	16 Missing ⚠️
src/xrpld/app/tx/detail/VaultWithdraw.cpp	73.2%	15 Missing ⚠️
src/xrpld/app/tx/detail/VaultSet.cpp	71.1%	13 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.2%   78.0%   -0.1%     
=========================================
  Files          790     802     +12     
  Lines        67619   68314    +695     
  Branches      8161    8278    +117     
=========================================
+ Hits         52848   53309    +461     
- Misses       14771   15005    +234

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 47 more

... and 6 files with indirect coverage changes

vlntb · 2025-01-29T16:07:30Z

src/test/basics/IOUAmount_test.cpp

@@ -44,6 +44,11 @@ class IOUAmount_test : public beast::unit_test::suite

        IOUAmount const zz(beast::zero);
        BEAST_EXPECT(z == zz);
+
+        // https://github.com/XRPLF/rippled/issues/5170
+        IOUAmount const zzz{};


Suggested change

IOUAmount const zzz{};

IOUAmount const zzz{0, -100};

vlntb · 2025-01-29T16:09:54Z

src/test/basics/IOUAmount_test.cpp

+        // https://github.com/XRPLF/rippled/issues/5170
+        IOUAmount const zzz{};
+        BEAST_EXPECT(zzz == beast::zero);
+        // BEAST_EXPECT(zzz == zz);


The suggestion above fixes this test. The discrepancy comes from our custom assignment operator:

inline IOUAmount& IOUAmount::operator=(beast::Zero) { // The -100 is used to allow 0 to sort less than small positive values // which will have a large negative exponent. mantissa_ = 0; exponent_ = -100; return *this; }

vlntb · 2025-01-29T17:24:45Z

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

-    JLOG(j.fatal()) << "Invariant failed: account root created "
-                       "by a non-Payment, by an unsuccessful transaction, "
-                       "or by AMM";
+    JLOG(j.fatal()) << "Invariant failed: account root created illegally";


Since there are multiple reasons for this failure, it makes sense to add transaction details into this log (i.e. tx.getFullText()).

Since there are multiple reasons for this failure, it makes sense to add transaction details into this log (i.e. tx.getFullText()).

The transaction ID should be sufficient. In almost all cases, a transaction that fails invariant checks will be applied to the ledger with a tec code, so it can be looked up using the ID.

gregtatcam · 2025-01-30T15:18:04Z

src/xrpld/ledger/View.h

+ *   if needed, on the basis of non-expired credentals found.
+ */
+[[nodiscard]] TER
+verifyAuth(


Not sure if it's a good name since it doesn't just verify auth but also creates MPToken.

renamed to enforceMPTokenAuthorization . A bit clumsy but that's best I could come up with :-|

gregtatcam · 2025-01-30T16:27:01Z

src/xrpld/ledger/detail/View.cpp

+    else if (!authorizedByDomain && domainCheck)  // && sleToken, implied
+    {
+        // We will only delete MPToken here if it has 0 balance
+        [[maybe_unused]] auto _ = MPTokenAuthorize::authorize(


Do we need to delete it? Later if permissions change then we'd have to create it again. Is there any harm in keeping MPToken if it has 0 balance?

I think we do - otherwise it won't be ever removed by anyone else. Unless MPTokens are all deleted when MPTokenIssuance is deleted ?

No, we don't delete all MPToken when MPTokenIssuance is deleted. This could be computationally unbound. The argument in this case is that it's up to the holders to delete MPTToken. In case of SAV, MPToken is automatically created. So yes, it makes sense automatically delete even though there is some inefficiency.

This reverts commit 06a8a61.

- Also make MPTID more consistent

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

vlntb reviewed Jan 29, 2025

View reviewed changes

Implement MPT domain checks

3eebdae

Bronek force-pushed the vault branch from b32701f to 3eebdae Compare January 30, 2025 12:42

gregtatcam reviewed Jan 30, 2025

View reviewed changes

MPT permissioned domain checks

ad68074

Bronek force-pushed the vault branch from 2edb100 to ad68074 Compare January 31, 2025 19:27

WIP trying to fix MSVC compilation error

06a8a61

Bronek force-pushed the vault branch from f2da240 to 06a8a61 Compare January 31, 2025 20:13

ximinez added 4 commits January 31, 2025 18:06

Revert "WIP trying to fix MSVC compilation error"

a98e59a

This reverts commit 06a8a61.

Fix ICE errors by reusing a global static const object

21e0e7e

Remove unnecessary ValueProxy friend operator !=

eed18d1

Fix MSVC build: Get the intended const& for some fields

c3f9a4c

- Also make MPTID more consistent

ximinez mentioned this pull request Jan 31, 2025

DRAFT: Ximinez/lending xls 66 #5270

Draft

13 tasks

Bronek added 5 commits February 3, 2025 10:11

Merge branch 'develop' into vault

d1c7d1a

Fix clang build error

8baadd5

Rename getSequence to getSeqValue

9decc05

Check permissioned domain enabled if DomainID is used

6765c10

Check for frozen assets in preclaim

1884ec5

Bronek force-pushed the vault branch from ce7ed65 to 1884ec5 Compare February 4, 2025 16:44

Bronek added 4 commits February 4, 2025 16:49

Prevent deposit when asset is frozen for depositor

acaed58

Improve checks in Vault

ffaa8bd

Remove unnecessary check

b7c9577

Merge branch 'develop' into vault

87d8881

Bronek marked this pull request as ready for review February 7, 2025 15:18

Bronek added 3 commits February 7, 2025 19:29

Enforce freeze of pseudo-account

c1d8bc4

Optimise isFrozen check for pair of accounts

e9ed800

Merge branch 'develop' into vault

9d619b9

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

vlntb Jan 29, 2025

vlntb Jan 29, 2025

vlntb Jan 29, 2025

ximinez Jan 30, 2025

gregtatcam Jan 30, 2025

Bronek Feb 7, 2025

gregtatcam Jan 30, 2025

Bronek Jan 31, 2025

gregtatcam Jan 31, 2025

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

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

codecov bot commented Dec 18, 2024 •

edited

Loading