Single Asset Vault #5224
Single Asset Vault #5224
Conversation
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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 |
Bronek
force-pushed
the
vault
branch
3 times, most recently
from
2bcc6ad to
17af6e7
Compare
Also remove potential ODR violation from json_value.h
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #5224 +/- ##
=========================================
- Coverage 78.1% 78.0% -0.1%
=========================================
Files 790 802 +12
Lines 67908 68648 +740
Branches 8230 8334 +104
=========================================
+ Hits 53034 53541 +507
- Misses 14874 15107 +233
... and 10 files with indirect coverage changes 🚀 New features to boost your workflow:
|
|
Hello, I'm unable to build the tip of this branch. I see this error message: This is my environment configuration: |
|
Perhaps, you meant to do this: Bronek#4 ? |
| // No `LossUnrealized`. | ||
| view().insert(vault); | ||
|
|
||
| return tesSUCCESS; |
There was a problem hiding this comment.
After the successful execution of VaultCreate transaction, how can I fetch the ledger-object index? Are there any helpful fields in the transaction's meta-data?
I can iterate through all Vault-type objects in the response of AccountObjects RPC. If I want to use the LedgerEntry RPC, how can I get the ledger-index of the Vault object?
There was a problem hiding this comment.
This problem is also highlighted by this test utility method: https://github.com/Bronek/rippled/blob/04503c9fa4ce1582946c8875e832ed26d5ddb4a5/src/test/jtx/impl/vault.cpp#L36
Are users expected to calculate the vault_id by themselves?
There was a problem hiding this comment.
it will be returned in metadata as LedgerIndex of the newly created Vault object, like e.g. here
"CreatedNode" :
{
"LedgerEntryType" : "Vault",
"LedgerIndex" : "C043BB1B350FFC5FED21E40535609D3D95BC0E3CE252E2F69F85BE0157020A52",
"NewFields" :
{ . . .
I will add unit test to verify the metadata returned from VaultCreate
There was a problem hiding this comment.
This problem is also highlighted by this test utility method: https://github.com/Bronek/rippled/blob/04503c9fa4ce1582946c8875e832ed26d5ddb4a5/src/test/jtx/impl/vault.cpp#L36
Are users expected to calculate the
vault_idby themselves?
The sdks must have the ability to generate ALL hashes/indexes because in a batch transaction, you will not have the response. So yes, the user will need to be able to generate this from whatever fields are required to create the index.
There was a problem hiding this comment.
The Vault ID can be pre-generated by the user, it is a Sha512-half of the Vault space key, the account ID creating the vault, and the sequence number of the transaction creating the vault. https://github.com/XRPLF/XRPL-Standards/tree/master/XLS-0065d-single-asset-vault#211-object-identifier
| {sfDestination, soeOPTIONAL}, | ||
| })) | ||
|
|
||
| /** This transaction trades shares for assets with a vault. */ |
There was a problem hiding this comment.
Perhaps this comment needs to be modified
| TRANSACTION(ttVAULT_CLAWBACK, 69, VaultClawback, ({ | ||
| {sfVaultID, soeREQUIRED}, | ||
| {sfHolder, soeREQUIRED}, | ||
| {sfAmount, soeDEFAULT, soeMPTSupported}, |
There was a problem hiding this comment.
If I do not specify the sfAmount field in the VaultClawback transaction, I get a tecWRONG_ASSET error from this code: https://github.com/Bronek/rippled/blob/04503c9fa4ce1582946c8875e832ed26d5ddb4a5/src/xrpld/app/tx/detail/VaultClawback.cpp#L66
Does the user have the option of not specifying the Amount field? How is the default value determined?
There was a problem hiding this comment.
I observed this behavior with IOUs
There was a problem hiding this comment.
Great point - I checked the spec and indeed we do allow Amount to be unspecified, in which case I should default to 0 which will clawback all the funds. I will fix it now.
There was a problem hiding this comment.
ohhh I see how this works.
@mvadari I need your help here; basically we have {sfAmount, soeDEFAULT, soeMPTSupported}, in the body of ttVAULT_CLAWBACK which means that, when not specified, we get all zeroes. The amount is not just a number, it is also an asset. When we get all zeroes as an asset I guess that means XRP or whatever, which clearly is not the asset being traded in the vault, hence this block fails
STAmount const amount = ctx.tx[sfAmount];
if (asset != amount.asset())
return tecWRONG_ASSET;
The problem here is that, had the users explicitly specified all zeroes (number and asset, and again I guess that would be XRP) we would have no way of knowing that it was manually selected by the user rather than auto-populated by the soeDEFAULT behaviour.
I guess in this case I need to switch from soeDEFAULT to soeOPTIONAL (and then refer to this field as tx[~sfAmount]) ? If so, do I also need soeMPTSupported or not ?
There was a problem hiding this comment.
FWIW I did as discussed above (switched to soeOPTIONAL and adjusted references to tx[~sfAmount] as needed
There was a problem hiding this comment.
I guess in this case I need to switch from soeDEFAULT to soeOPTIONAL (and then refer to this field as tx[~sfAmount]) ?
That sounds right.
If so, do I also need soeMPTSupported or not ?
Is an MPT a valid value for sfAmount here?
There was a problem hiding this comment.
Is an MPT a valid value for sfAmount here?
Yes, asset can be IOU or MPT.
| std::size_t constexpr maxVaultDataLength = 256; | ||
|
|
||
| /** Vault withdrawal policies */ | ||
| std::uint8_t constexpr vaultStrategyFirstComeFirstServe = 1; |
There was a problem hiding this comment.
According to XLS-0065d-single-asset-vault specification this field should be called strFirstComeFirstServe .
There was a problem hiding this comment.
the name in spec is potentially misleading. Also it is internal, hence it does not affect the protocol in anyway.
| if (issuance->getFlags() & lsfMPTLocked) | ||
| return tecLOCKED; | ||
| if ((issuance->getFlags() & lsfMPTCanTransfer) == 0) | ||
| return tecLOCKED; |
There was a problem hiding this comment.
The TEC code for lsfMPTCanTransfer should be the asset is not transferable, instead of asset is locked.
There was a problem hiding this comment.
kind-of. The return code from canTransfer , as used and reported e.g. in Payment::doApply is tecNO_AUTH. I will switch to this one. We do not have "asset is not transferable" error and adding it now would cause inconsistency with Payment.
There was a problem hiding this comment.
this is now changed to tecNO_AUTH
| } | ||
|
|
||
| TER | ||
| VaultCreate::doApply() |
There was a problem hiding this comment.
According to the spec:
If Vault.Asset is an IOU:
Create a RippleState object between the pseudo-account AccountRoot and Issuer AccountRoot.
If Vault.Asset is an MPT:
Create MPToken object for the pseudo-account for the Asset.MPTokenIssuance.
At the moment only MPT is supported.
There was a problem hiding this comment.
I think you confused MPTokenIssuanceCreate::create (which creates MPTokenIssuance to hold the shares of the vault) with addEmptyHolding (which creates either RippleState or MPToken for the assets held in the vault, as per spec)
Look for addEmptyHolding in View.cpp
|
|
||
| // A deposit must not push the vault over its limit. | ||
| auto const maximum = *vault->at(sfAssetMaximum); | ||
| if (maximum != 0 && *vault->at(sfAssetTotal) > maximum) |
There was a problem hiding this comment.
Should this check be done before vault assets are incremented (Lines 154...155)?
Otherwise, we push vault over its limits and then check this condition afterwards.
This should be now fixed (thanks @vlntb) |
| } | ||
|
|
||
| [[nodiscard]] TER | ||
| enforceMPTokenAuthorization( |
There was a problem hiding this comment.
Is this function to be used anywhere that one needs to check if a MPToken is authorized? Or is this only used by Vault?
I do a similar check in escrow, I will do a similar one in paychan.
There was a problem hiding this comment.
partial. just suggestions.
Adding functions to the View that will only be used by one Transactor isn't a good approach imo and if you intend for them to be used agnostically then you shouldn't assume the guards (frozen, auth, canTransfer, global freeze, etc).
I also see A LOT of refactoring, some of which I'm certain will cause a fork. Please, think about if you should do that refactoring here in the PR or another PR.
Also, lol handling permissioned domains in this PR seems like it really increased the complexity, I would think about maybe just implementing this without it, without clawback, so that this PR is easier to review.
You don't have to do any of this, they are just suggestions.
Didn't review Deposit/Set/Withdraw.
|
|
||
| // if account has no MPToken, fail | ||
| if (!sleToken) |
There was a problem hiding this comment.
This isnt correct imo. If the token does not require authorization, and I dont have the token then technically the response sound be successful. See Escrow and MPTDex. Also notice how this is done for IOU and is there a similar function you are creating for IOU? Also this code here is exactly like requireAuth...
| @@ -1088,6 +1223,121 @@ trustDelete( | |||
| return tesSUCCESS; | |||
| } | |||
|
|
|||
| [[nodiscard]] TER | |||
| addEmptyHolding( | |||
There was a problem hiding this comment.
You check frozen in preclaim, and then here you check globalFrozen. This makes this PR really difficult to review. I think keeping it all in one place is better. Also you're checking global freeze way late in the stack, this could be done in preclaim right?
Here is the implementation in Escrow, stolen from Greg and Shawn. Its clean, easy to read, easy to understand. Also if this function will only ever be used by vault then putting it here in View doesn't make sense to me.
rippled/src/xrpld/app/tx/detail/Escrow.cpp
Line 209 in b08c1c7
| // Note, this is not amendment-gated because we do not want to maintain in | ||
| // this file the list of all the amendments which can write to this field. | ||
| // Without additional amendments this field is always empty. | ||
| auto const maybeDomainID = sleIssuance->at(~sfDomainID); |
There was a problem hiding this comment.
This is used by multiple existing functions, therefore unless you gated them all this will cause a fork I believe.
| auto const& mptIssue = asset.get<MPTIssue>(); | ||
| auto const& mptID = mptIssue.getMptID(); | ||
| // `MPTokenAuthorize::authorize` asserts that the balance is 0. | ||
| return MPTokenAuthorize::authorize( |
There was a problem hiding this comment.
This function checks requireAuth for MPT but I don't see you checking that for IOU?
| auto asset = ctx.tx[sfAsset]; | ||
| auto account = ctx.tx[sfAccount]; | ||
|
|
||
| if (asset.holds<MPTIssue>()) |
There was a problem hiding this comment.
I really like what Greg and Shawn did with the visit where they separated the checks for IOU vs MPT. Makes it very easy to review. Please think about implementing it. See my comment about EscrowToken for implementation details.
| if (txFlags & tfVaultPrivate) | ||
| mptFlags |= lsfMPTRequireAuth; | ||
|
|
||
| auto maybeShare = MPTokenIssuanceCreate::create( |
There was a problem hiding this comment.
Same here, I'm guessing this isn't finished though because this only handles MPT tokens.
|
|
||
| if (auto const domain = ctx.tx[~sfDomainID]) | ||
| { | ||
| if (!ctx.rules.enabled(featurePermissionedDomains)) |
There was a problem hiding this comment.
Aren't we checking the permissioned domain ID twice? We already check on lines 39-40 whether the field is present and whether the feature is enabled?
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
.gitignore, formatting, dropping support for older tooling)API Impact
libxrplchange (any change that may affectlibxrplor dependents oflibxrpl)