adds sfMemoData field to VaultDelete transaction#6356
adds sfMemoData field to VaultDelete transaction#6356Tapanito merged 14 commits intotapanito/lending-fix-amendmentfrom
Conversation
Tapanito
force-pushed
the
tapanito/vault-delete-data
branch
from
df4e319 to
f8b555d
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## tapanito/lending-fix-amendment #6356 +/- ##
==============================================================
Coverage 79.8% 79.8%
==============================================================
Files 848 848
Lines 67763 67767 +4
Branches 7561 7558 -3
==============================================================
+ Hits 54064 54072 +8
+ Misses 13699 13695 -4
... and 4 files with indirect coverage changes 🚀 New features to boost your workflow:
|
| // The sfData field is an optional field used to record the deletion reason. | ||
| if (auto const data = ctx.tx[~sfData]; data && !validDataLength(data, maxDataPayloadLength)) | ||
| return temMALFORMED; | ||
|
|
There was a problem hiding this comment.
How is this not redundant with sfMemos/sfMemo?
There was a problem hiding this comment.
I haven't considered sfMemos / sfMemo. Looking at it, I have reservations with the field, simply based on how big / complex the field is.
I don't have a strong opinion either way. The benefit I see, in sfData is that it does not prescribe a particular structure. Other than that, either option works. That said, I think we only use sfMemos field, never sfMemo.
There was a problem hiding this comment.
I don't have a strong opinion either way. The benefit I see, in
sfDatais that it does not prescribe a particular structure. Other than that, either option works. That said, I think we only usesfMemosfield, neversfMemo.
Yes, the structure of sfMemos is probably overcomplicated. The JSON for even a simple one can look like
"Memos" : [
{
"Memo" : {
"MemoData" : "7274312E312E302D31",
}
}
],
Note that sfMemo is used as the first level inner object.
Whereas sfData would just be
"Data" : "7274312E312E302D31",
Despite the complication, sfMemos is fit to purpose for what you're using sfData for in this PR. I think it would be confusing to have sfData used in some transactions to be written to a ledger object, and used in others as "information only" that will not be written.
But I have a proposal: We have memo fields to represent text available already, and not used for any other purpose.
TYPED_SFIELD(sfMemoType, VL, 12)
TYPED_SFIELD(sfMemoData, VL, 13)
TYPED_SFIELD(sfMemoFormat, VL, 14)
TYPED_SFIELD(sfData, VL, 27)
If you search for any of them, you'll see that they're only used in tests and STTx.cpp where the formatting is checked.
Option 1 (easier): Instead of adding sfData to ttVAULT_DELETE, add sfMemoData. Do all the same checks in VaultDelete, but using sfMemoData instead.
Option 2 (more involved, but so much cooler): Add sfMemoData to the commonFields in TxFormats.cpp. Move the checks in VaultDelete to the corresponding location in the base Transactor class (most likely preflight0, since that's called by every transactor). Realistically, this might need a separate amendment (e.g. SimpleMemo), though that could easily be written to be backward compatible with Option 1.
There was a problem hiding this comment.
Thank you for the detailed response Ed. I understand, and agree with your reasoning.
The proposal is great, I think what we need to do is apply both options.
Option 1, in this PR, gated with the lending protocol fix amendment.
Option 2, as a separate PR against develop branch, as it would introduce a separate amendment, and I don't think it's a great idea to introduce two separate amendments in this PR. What do you think?
There was a problem hiding this comment.
I added option 1, let me know your thoughts about Option 2! 🙇
There was a problem hiding this comment.
Thank you for the detailed response Ed. I understand, and agree with your reasoning.
👍
The proposal is great, I think what we need to do is apply both options.
Option 1, in this PR, gated with the lending protocol fix amendment.
Woot!
Option 2, as a separate PR against
developbranch, as it would introduce a separate amendment, and I don't think it's a great idea to introduce two separate amendments in this PR. What do you think?
Yes, agreed! Sounds good.
ximinez
left a comment
ximinez
left a comment
There was a problem hiding this comment.
I left one small suggestion that is not enough to hang this PR up if you don't want to do it.
src/test/app/Vault_test.cpp
Outdated
| { | ||
| testcase("VaultDelete data fixLendingProtocolV1_1 disabled"); | ||
| env.disableFeature(fixLendingProtocolV1_1); | ||
| auto tx = vault.del({.owner = owner, .id = keylet.key}); |
There was a problem hiding this comment.
This restructuring is great. The only feedback I have is minor. You can build the vault.del(...) transaction once at the outer scope and reuse it in each test case, setting a different sfMemoData every time. Call it delTx to distinguish it from the creation tx in the last test case.
There was a problem hiding this comment.
This is a nice clean up! I removed the duplicate constructions of the transaction.
ximinez
changed the title
Tapanito
merged commit ba53026
into
tapanito/lending-fix-amendment
3 participants
This PR adds an optional
Datafield to theVaultDeletetransaction to register the reason for deletion.Specification: XRPLF/XRPL-Standards#470
High Level Overview of Change
Context of Change
Type of Change
.gitignore, formatting, dropping support for older tooling)API Impact
libxrplchange (any change that may affectlibxrplor dependents oflibxrpl)