Skip to content

adds sfMemoData field to VaultDelete transaction #6356

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
Tapanito merged 14 commits into from
Feb 26, 2026
Merged

adds sfMemoData field to VaultDelete transaction #6356

Show file tree
Hide file tree
Changes from 10 commits
Commits
Show all changes
14 commits
Select commit Hold shift + click to select a range
f8b555d
adds sfData field to VaultDelete transaction
Tapanito Feb 12, 2026
60fce3c
improves feature gating
Tapanito Feb 12, 2026
3338ec3
Merge branch 'tapanito/lending-fix-amendment' into tapanito/vault-del…
Tapanito Feb 12, 2026
6a34a66
Refactor VaultDelete tests for clarity
Tapanito Feb 12, 2026
a14541a
Update test comment for VaultDelete behavior
Tapanito Feb 12, 2026
0bc67c8
formatting
Tapanito Feb 12, 2026
667b559
Merge branch 'tapanito/lending-fix-amendment' into tapanito/vault-del…
Tapanito Feb 17, 2026
edfc41f
Merge branch 'tapanito/lending-fix-amendment' into tapanito/vault-del…
Tapanito Feb 23, 2026
d67c520
Merge branch 'tapanito/lending-fix-amendment' into tapanito/vault-del…
Tapanito Feb 24, 2026
cb93f02
changes to use sfMemoData instead
Tapanito Feb 24, 2026
597c3e3
improvements to vault tests
Tapanito Feb 25, 2026
5cd9d18
fixes a dumb mistake
Tapanito Feb 25, 2026
3d4c11e
fix dumb mistake pt2
Tapanito Feb 25, 2026
5805372
Merge branch 'tapanito/lending-fix-amendment' into tapanito/vault-del…
Tapanito Feb 25, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions include/xrpl/protocol/detail/transactions.macro
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -868,6 +868,7 @@ TRANSACTION(ttVAULT_DELETE, 67, VaultDelete,
mustDeleteAcct | destroyMPTIssuance | mustModifyVault,
({
{sfVaultID, soeREQUIRED},
{sfMemoData, soeOPTIONAL},
}))

/** This transaction trades assets for shares with a vault. */
Expand Down
7 changes: 7 additions & 0 deletions src/libxrpl/tx/transactors/Vault/VaultDelete.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,13 @@ VaultDelete::preflight(PreflightContext const& ctx)
return temMALFORMED;
}

if (ctx.tx.isFieldPresent(sfMemoData) && !ctx.rules.enabled(fixLendingProtocolV1_1))
return temDISABLED;

// The sfMemoData field is an optional field used to record the deletion reason.
if (auto const data = ctx.tx[~sfMemoData]; data && !validDataLength(data, maxDataPayloadLength))
return temMALFORMED;

Copy link
Collaborator

@ximinez ximinez Feb 19, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How is this not redundant with sfMemos/sfMemo?

Copy link
Collaborator Author

@Tapanito Tapanito Feb 23, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator

@ximinez ximinez Feb 23, 2026 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

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.

Copy link
Collaborator Author

@Tapanito Tapanito Feb 24, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Collaborator Author

@Tapanito Tapanito Feb 24, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added option 1, let me know your thoughts about Option 2! 🙇

Copy link
Collaborator

@ximinez ximinez Feb 24, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 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?

Yes, agreed! Sounds good.

return tesSUCCESS;
}

Expand Down
90 changes: 74 additions & 16 deletions src/test/app/Vault_test.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1064,14 +1064,13 @@ class Vault_test : public beast::unit_test::suite
{
using namespace test::jtx;

auto testCase = [this](
std::function<void(
Env & env,
Account const& issuer,
Account const& owner,
Account const& depositor,
Asset const& asset,
Vault& vault)> test) {
auto testCase = [this](std::function<void(
Env & env,
Account const& issuer,
Account const& owner,
Account const& depositor,
Asset const& asset,
Vault& vault)> test) {
Env env{*this, testable_amendments() | featureSingleAssetVault};
Account issuer{"issuer"};
Account owner{"owner"};
Expand Down Expand Up @@ -1354,14 +1353,13 @@ class Vault_test : public beast::unit_test::suite
{
using namespace test::jtx;

auto testCase = [this](
std::function<void(
Env & env,
Account const& issuer,
Account const& owner,
Account const& depositor,
Asset const& asset,
Vault& vault)> test) {
auto testCase = [this](std::function<void(
Env & env,
Account const& issuer,
Account const& owner,
Account const& depositor,
Asset const& asset,
Vault& vault)> test) {
Env env{*this, testable_amendments() | featureSingleAssetVault};
Account issuer{"issuer"};
Account owner{"owner"};
Expand Down Expand Up @@ -5357,6 +5355,65 @@ class Vault_test : public beast::unit_test::suite
}
}

void
testVaultDeleteData()
{
using namespace test::jtx;

Env env{*this};

Account const owner{"owner"};
env.fund(XRP(1'000'000), owner);
env.close();

Vault vault{env};

auto const keylet = keylet::vault(owner.id(), 1);

// Test VaultDelete with fixLendingProtocolV1_1 disabled
// Transaction fails if the data field is provided
{
testcase("VaultDelete data fixLendingProtocolV1_1 disabled");
env.disableFeature(fixLendingProtocolV1_1);
auto tx = vault.del({.owner = owner, .id = keylet.key});
Copy link
Collaborator

@ximinez ximinez Feb 24, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator Author

@Tapanito Tapanito Feb 25, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a nice clean up! I removed the duplicate constructions of the transaction.

tx[sfMemoData] = strHex(std::string(maxDataPayloadLength, 'A'));
env(tx, ter(temDISABLED), THISLINE);
env.close();
env.enableFeature(fixLendingProtocolV1_1);
}

// Transaction fails if the data field is too large
{
testcase("VaultDelete data fixLendingProtocolV1_1 enabled data too large");
auto tx = vault.del({.owner = owner, .id = keylet.key});
tx[sfMemoData] = strHex(std::string(maxDataPayloadLength + 1, 'A'));
env(tx, ter(temMALFORMED), THISLINE);
env.close();
}

// Transaction fails if the data field is set, but is empty
{
testcase("VaultDelete data fixLendingProtocolV1_1 enabled data empty");
auto tx = vault.del({.owner = owner, .id = keylet.key});
tx[sfMemoData] = strHex(std::string(0, 'A'));
env(tx, ter(temMALFORMED), THISLINE);
env.close();
}

{
testcase("VaultDelete data fixLendingProtocolV1_1 enabled data valid");
PrettyAsset const xrpAsset = xrpIssue();
auto [tx, keylet] = vault.create({.owner = owner, .asset = xrpAsset});
env(tx, ter(tesSUCCESS), THISLINE);
env.close();

tx = vault.del({.owner = owner, .id = keylet.key});
tx[sfMemoData] = strHex(std::string(maxDataPayloadLength, 'A'));
env(tx, ter(tesSUCCESS), THISLINE);
env.close();
}
}

public:
void
run() override
Expand All @@ -5378,6 +5435,7 @@ class Vault_test : public beast::unit_test::suite
testVaultClawbackBurnShares();
testVaultClawbackAssets();
testAssetsMaximum();
testVaultDeleteData();
}
};

Expand Down
Loading