ARCHIVE/rippled
1
0
Fork 0
mirror of https://github.com/XRPLF/rippled.git synced 2026-07-24 07:30:30 +00:00
Code Issues Packages Projects Releases Wiki Activity

fix: address PR review feedback

Browse Source
This commit is contained in:
Vito
2026-03-21 17:43:08 +01:00
parent 7e5a4c9d04
commit b970c66a37
6 changed files with 140 additions and 69 deletions
Download Patch File Download Diff File Expand all files Collapse all files

37
include/xrpl/protocol_autogen/transactions/VaultDelete.h
View File

@@ -57,6 +57,32 @@ public:
{
return this->tx_->at(sfVaultID);
}
/**
* @brief Get sfMemoData (soeOPTIONAL)
* @return The field value, or std::nullopt if not present.
*/
[[nodiscard]]
protocol_autogen::Optional<SF_VL::type::value_type>
getMemoData() const
{
if (hasMemoData())
{
return this->tx_->at(sfMemoData);
}
return std::nullopt;
}
/**
* @brief Check if sfMemoData is present.
* @return True if the field is present, false otherwise.
*/
[[nodiscard]]
bool
hasMemoData() const
{
return this->tx_->isFieldPresent(sfMemoData);
}
};
/**
@@ -112,6 +138,17 @@ public:
return *this;
}
/**
* @brief Set sfMemoData (soeOPTIONAL)
* @return Reference to this builder for method chaining.
*/
VaultDeleteBuilder&
setMemoData(std::decay_t<typename SF_VL::type::value_type> const& value)
{
object_[sfMemoData] = value;
return *this;
}
/**
* @brief Build and return the VaultDelete wrapper.
* @param publicKey The public key for signing.

5
src/libxrpl/tx/transactors/vault/VaultCreate.cpp
View File

@@ -32,7 +32,7 @@ VaultCreate::checkExtraFeatures(PreflightContext const& ctx)
std::uint32_t
VaultCreate::getFlagsMask(PreflightContext const& ctx)
{
if (ctx.rules.enabled(fixLendingProtocolV1_1))
if (ctx.rules.enabled(featureLendingProtocolV1_1))
return tfVaultCreateMask;
return tfVaultCreateMask | tfVaultOwnerCanBlockDeposit;
@@ -193,7 +193,8 @@ VaultCreate::doApply()
if (tx.isFlag(tfVaultPrivate))
vault->setFlag(lsfVaultPrivate);
if (view().rules().enabled(fixLendingProtocolV1_1) && tx.isFlag(tfVaultOwnerCanBlockDeposit))
if (view().rules().enabled(featureLendingProtocolV1_1) &&
tx.isFlag(tfVaultOwnerCanBlockDeposit))
vault->setFlag(lsfVaultOwnerCanBlockDeposit);
vault->at(sfSequence) = sequence;

4
src/libxrpl/tx/transactors/vault/VaultDeposit.cpp
View File

@@ -77,7 +77,7 @@ VaultDeposit::preclaim(PreclaimContext const& ctx)
// LCOV_EXCL_STOP
}
if (ctx.view.rules().enabled(fixLendingProtocolV1_1))
if (ctx.view.rules().enabled(featureLendingProtocolV1_1))
{
// Perform these checks early to avoid unnecessary processing
@@ -85,7 +85,7 @@ VaultDeposit::preclaim(PreclaimContext const& ctx)
if (isVaultInsolvent(vault, sleShareIssuance))
{
JLOG(ctx.j.debug()) << "VaultDeposit: Vault is insolvent, deposits are not allowed";
return tecNO_PERMISSION;
return tecLOCKED;
}
if (vault->isFlag(lsfVaultDepositBlocked))

8
src/libxrpl/tx/transactors/vault/VaultSet.cpp
View File

@@ -23,7 +23,7 @@ VaultSet::checkExtraFeatures(PreflightContext const& ctx)
std::uint32_t
VaultSet::getFlagsMask(PreflightContext const& ctx)
{
if (ctx.rules.enabled(fixLendingProtocolV1_1))
if (ctx.rules.enabled(featureLendingProtocolV1_1))
return tfVaultSetMask;
// Add tfVaultDepositBlock and tfVaultDepositUnblock flags to indicate they are disabled
@@ -36,6 +36,8 @@ isValidVaultUpdate(PreflightContext const& ctx)
auto const atLeastOneFieldPresent = ctx.tx.isFieldPresent(sfDomainID) ||
ctx.tx.isFieldPresent(sfAssetsMaximum) || ctx.tx.isFieldPresent(sfData);
// Mask of valid, non-universal flags: any bit set here means the
// transaction is requesting a meaningful flag change.
auto const expectedFlags = ~(VaultSet::getFlagsMask(ctx) | tfUniversal);
return atLeastOneFieldPresent || (ctx.tx.getFlags() & expectedFlags);
@@ -134,7 +136,7 @@ VaultSet::preclaim(PreclaimContext const& ctx)
}
}
if (ctx.view.rules().enabled(fixLendingProtocolV1_1))
if (ctx.view.rules().enabled(featureLendingProtocolV1_1))
{
// The Vault is not configured to support deposit blocking
if (!vault->isFlag(lsfVaultOwnerCanBlockDeposit) &&
@@ -214,7 +216,7 @@ VaultSet::doApply()
view().update(sleIssuance);
}
if (view().rules().enabled(fixLendingProtocolV1_1))
if (view().rules().enabled(featureLendingProtocolV1_1))
{
if (tx.isFlag(tfVaultDepositBlock))
vault->setFlag(lsfVaultDepositBlocked);

106
src/test/app/Vault_test.cpp
View File

@@ -669,11 +669,11 @@ class Vault_test : public beast::unit_test::suite
env(tx, ter{temINVALID_FLAG});
{
env.disableFeature(fixLendingProtocolV1_1);
env.disableFeature(featureLendingProtocolV1_1);
auto [tx, keylet] = vault.create({.owner = owner, .asset = asset});
tx[sfFlags] = tfVaultOwnerCanBlockDeposit;
env(tx, ter{temINVALID_FLAG});
env.enableFeature(fixLendingProtocolV1_1);
env.enableFeature(featureLendingProtocolV1_1);
}
{
@@ -1067,18 +1067,18 @@ class Vault_test : public beast::unit_test::suite
testCase(
[&](Env& env, Account const&, Account const& owner, Asset const& asset, Vault& vault) {
testcase("set flags fail without fixLendingProtocolV1_1");
testcase("set flags fail without featureLendingProtocolV1_1");
auto [tx, keylet] = vault.create({.owner = owner, .asset = asset});
{
env.disableFeature(fixLendingProtocolV1_1);
env.disableFeature(featureLendingProtocolV1_1);
env(vault.set({.owner = owner, .id = keylet.key, .flags = tfVaultDepositBlock}),
ter(temINVALID_FLAG));
env(vault.set(
{.owner = owner, .id = keylet.key, .flags = tfVaultDepositUnblock}),
ter(temINVALID_FLAG));
env.enableFeature(fixLendingProtocolV1_1);
env.enableFeature(featureLendingProtocolV1_1);
}
});
@@ -2371,7 +2371,7 @@ class Vault_test : public beast::unit_test::suite
{
auto const tx = vault.deposit(
{.depositor = depositor, .id = vaultKeylet.key, .amount = depositAmount});
env(tx, ter{tesSUCCESS}, THISLINE);
env(tx, ter{tesSUCCESS});
env.close();
}
@@ -2383,7 +2383,7 @@ class Vault_test : public beast::unit_test::suite
using namespace loanBroker;
using namespace loan;
env(set(owner, vaultKeylet.key), THISLINE);
env(set(owner, vaultKeylet.key));
env.close();
// Create a simple Loan for the full amount of Vault assets
@@ -2393,11 +2393,10 @@ class Vault_test : public beast::unit_test::suite
paymentTotal(1),
sig(sfCounterpartySignature, owner),
fee(env.current()->fees().base * 2),
ter(tesSUCCESS),
THISLINE);
ter(tesSUCCESS));
env.close(std::chrono::seconds{120 + 60});
env(manage(owner, loanKeylet.key, tfLoanDefault), ter(tesSUCCESS), THISLINE);
env(manage(owner, loanKeylet.key, tfLoanDefault), ter(tesSUCCESS));
env.close();
auto const sleVault = env.le(vaultKeylet);
@@ -2424,7 +2423,7 @@ class Vault_test : public beast::unit_test::suite
{
auto const tx = vault.deposit(
{.depositor = depositor, .id = vaultKeylet.key, .amount = asset(20)});
env(tx, ter{tecNO_PERMISSION}, THISLINE);
env(tx, ter{tecNO_PERMISSION});
env.close();
}
@@ -2440,14 +2439,13 @@ class Vault_test : public beast::unit_test::suite
.id = vaultKeylet.key,
.holder = depositor,
.amount = share(0)}),
ter(tesSUCCESS),
THISLINE);
ter(tesSUCCESS));
env.close();
}
{
env(loan::del(owner, loanKeylet.key), ter(tesSUCCESS), THISLINE);
env(loanBroker::del(owner, brokerKeylet.key), ter(tesSUCCESS), THISLINE);
env(loan::del(owner, loanKeylet.key), ter(tesSUCCESS));
env(loanBroker::del(owner, brokerKeylet.key), ter(tesSUCCESS));
env(vault.del({.owner = owner, .id = vaultKeylet.key}));
env.close();
}
@@ -3173,7 +3171,7 @@ class Vault_test : public beast::unit_test::suite
{
auto const tx = vault.deposit(
{.depositor = issuer, .id = vaultKeylet.key, .amount = depositAmount});
env(tx, ter{tesSUCCESS}, THISLINE);
env(tx, ter{tesSUCCESS});
env.close();
}
@@ -3185,7 +3183,7 @@ class Vault_test : public beast::unit_test::suite
using namespace loanBroker;
using namespace loan;
env(set(owner, vaultKeylet.key), ter{tesSUCCESS}, THISLINE);
env(set(owner, vaultKeylet.key), ter{tesSUCCESS});
env.close();
// Create a simple Loan for the full amount of Vault assets
@@ -3195,11 +3193,10 @@ class Vault_test : public beast::unit_test::suite
paymentTotal(1),
sig(sfCounterpartySignature, owner),
fee(env.current()->fees().base * 2),
ter{tesSUCCESS},
THISLINE);
ter{tesSUCCESS});
env.close(std::chrono::seconds{120 + 60});
env(manage(owner, loanKeylet.key, tfLoanDefault), ter(tesSUCCESS), THISLINE);
env(manage(owner, loanKeylet.key, tfLoanDefault), ter(tesSUCCESS));
env.close();
auto const sleVault = env.le(vaultKeylet);
@@ -3226,7 +3223,7 @@ class Vault_test : public beast::unit_test::suite
{
auto const tx = vault.deposit(
{.depositor = issuer, .id = vaultKeylet.key, .amount = asset(20)});
env(tx, ter{tecNO_PERMISSION}, THISLINE);
env(tx, ter{tecNO_PERMISSION});
env.close();
}
@@ -3242,14 +3239,13 @@ class Vault_test : public beast::unit_test::suite
.id = vaultKeylet.key,
.holder = issuer,
.amount = share(0)}),
ter(tesSUCCESS),
THISLINE);
ter(tesSUCCESS));
env.close();
}
{
env(loan::del(owner, loanKeylet.key), ter(tesSUCCESS), THISLINE);
env(loanBroker::del(owner, brokerKeylet.key), ter(tesSUCCESS), THISLINE);
env(loan::del(owner, loanKeylet.key), ter(tesSUCCESS));
env(loanBroker::del(owner, brokerKeylet.key), ter(tesSUCCESS));
env(vault.del({.owner = owner, .id = vaultKeylet.key}));
env.close();
}
@@ -5585,30 +5581,28 @@ class Vault_test : public beast::unit_test::suite
auto const blockVault = [&](TER expectedTer, Keylet const& keylet) {
env(vault.set({.owner = owner, .id = keylet.key, .flags = tfVaultDepositBlock}),
ter(expectedTer),
THISLINE);
ter(expectedTer));
};
auto const unblockVault = [&](TER expectedTer, Keylet const& keylet) {
env(vault.set({.owner = owner, .id = keylet.key, .flags = tfVaultDepositUnblock}),
ter(expectedTer),
THISLINE);
ter(expectedTer));
};
// Blocking Vault with the amendment disabled fails
{
testcase(prefix + "block/unblock fails when amendment is disabled");
env.disableFeature(fixLendingProtocolV1_1);
env.disableFeature(featureLendingProtocolV1_1);
auto const [tx, keylet] = vault.create(
{.owner = owner, .asset = asset, .flags = tfVaultOwnerCanBlockDeposit});
env(tx, ter(temINVALID_FLAG), THISLINE);
env(tx, ter(temINVALID_FLAG));
env.close();
blockVault(temINVALID_FLAG, keylet);
unblockVault(temINVALID_FLAG, keylet);
env.enableFeature(fixLendingProtocolV1_1);
env.enableFeature(featureLendingProtocolV1_1);
}
// Block Vault deposits fails if the vault is not configured to allow blocking deposits
@@ -5621,7 +5615,7 @@ class Vault_test : public beast::unit_test::suite
blockVault(tecNO_PERMISSION, keylet);
unblockVault(tecNO_PERMISSION, keylet);
env(vault.del({.owner = owner, .id = keylet.key}), ter(tesSUCCESS), THISLINE);
env(vault.del({.owner = owner, .id = keylet.key}), ter(tesSUCCESS));
env.close();
}
@@ -5638,15 +5632,13 @@ class Vault_test : public beast::unit_test::suite
.id = keylet.key,
.amount = XRP(10'000),
}),
ter(tesSUCCESS),
THISLINE);
ter(tesSUCCESS));
env(vault.deposit({
.depositor = other,
.id = keylet.key,
.amount = XRP(10'000),
}),
ter(tesSUCCESS),
THISLINE);
ter(tesSUCCESS));
blockVault(tesSUCCESS, keylet);
@@ -5656,8 +5648,7 @@ class Vault_test : public beast::unit_test::suite
.id = keylet.key,
.amount = XRP(10'000),
}),
ter(tecNO_PERMISSION),
THISLINE);
ter(tecNO_PERMISSION));
// Other accounts are also blocked from depositing to the vault
env(vault.deposit({
@@ -5665,8 +5656,7 @@ class Vault_test : public beast::unit_test::suite
.id = keylet.key,
.amount = XRP(10'000),
}),
ter(tecNO_PERMISSION),
THISLINE);
ter(tecNO_PERMISSION));
// Block vault withdrawal works as normal
env(vault.withdraw({
@@ -5674,16 +5664,14 @@ class Vault_test : public beast::unit_test::suite
.id = keylet.key,
.amount = XRP(10'000),
}),
ter(tesSUCCESS),
THISLINE);
ter(tesSUCCESS));
env(vault.withdraw({
.depositor = other,
.id = keylet.key,
.amount = XRP(10'000),
}),
ter(tesSUCCESS),
THISLINE);
ter(tesSUCCESS));
unblockVault(tesSUCCESS, keylet);
@@ -5692,16 +5680,14 @@ class Vault_test : public beast::unit_test::suite
.id = keylet.key,
.amount = XRP(10'000),
}),
ter(tesSUCCESS),
THISLINE);
ter(tesSUCCESS));
env(vault.deposit({
.depositor = other,
.id = keylet.key,
.amount = XRP(10'000),
}),
ter(tesSUCCESS),
THISLINE);
ter(tesSUCCESS));
// Withdraw to keep the vault empty
env(vault.withdraw({
@@ -5709,30 +5695,26 @@ class Vault_test : public beast::unit_test::suite
.id = keylet.key,
.amount = XRP(10'000),
}),
ter(tesSUCCESS),
THISLINE);
ter(tesSUCCESS));
env(vault.withdraw({
.depositor = other,
.id = keylet.key,
.amount = XRP(10'000),
}),
ter(tesSUCCESS),
THISLINE);
ter(tesSUCCESS));
}
{
testcase(prefix + "block/unblock fails when caller is not owner");
env(vault.set({.owner = other, .id = keylet.key, .flags = tfVaultDepositBlock}),
ter(tecNO_PERMISSION),
THISLINE);
ter(tecNO_PERMISSION));
blockVault(tesSUCCESS, keylet);
env(vault.set({.owner = other, .id = keylet.key, .flags = tfVaultDepositUnblock}),
ter(tecNO_PERMISSION),
THISLINE);
ter(tecNO_PERMISSION));
unblockVault(tesSUCCESS, keylet);
}
@@ -5772,7 +5754,7 @@ class Vault_test : public beast::unit_test::suite
testcase("VaultDelete data featureLendingProtocolV1_1 disabled");
env.disableFeature(featureLendingProtocolV1_1);
delTx[sfMemoData] = strHex(std::string(maxDataPayloadLength, 'A'));
env(delTx, ter(temDISABLED), THISLINE);
env(delTx, ter(temDISABLED));
env.close();
env.enableFeature(featureLendingProtocolV1_1);
}
@@ -5781,7 +5763,7 @@ class Vault_test : public beast::unit_test::suite
{
testcase("VaultDelete data featureLendingProtocolV1_1 enabled data too large");
delTx[sfMemoData] = strHex(std::string(maxDataPayloadLength + 1, 'A'));
env(delTx, ter(temMALFORMED), THISLINE);
env(delTx, ter(temMALFORMED));
env.close();
}
@@ -5789,7 +5771,7 @@ class Vault_test : public beast::unit_test::suite
{
testcase("VaultDelete data featureLendingProtocolV1_1 enabled data empty");
delTx[sfMemoData] = strHex(std::string(0, 'A'));
env(delTx, ter(temMALFORMED), THISLINE);
env(delTx, ter(temMALFORMED));
env.close();
}
@@ -5797,12 +5779,12 @@ class Vault_test : public beast::unit_test::suite
testcase("VaultDelete data featureLendingProtocolV1_1 enabled data valid");
PrettyAsset const xrpAsset = xrpIssue();
auto [tx, keylet] = vault.create({.owner = owner, .asset = xrpAsset});
env(tx, ter(tesSUCCESS), THISLINE);
env(tx, ter(tesSUCCESS));
env.close();
// Recreate the transaction as the vault keylet changed
auto delTx = vault.del({.owner = owner, .id = keylet.key});
delTx[sfMemoData] = strHex(std::string(maxDataPayloadLength, 'A'));
env(delTx, ter(tesSUCCESS), THISLINE);
env(delTx, ter(tesSUCCESS));
env.close();
}
}

49
src/tests/libxrpl/protocol_autogen/transactions/VaultDeleteTests.cpp
View File

@@ -30,6 +30,7 @@ TEST(TransactionsVaultDeleteTests, BuilderSettersRoundTrip)
// Transaction-specific field values
auto const vaultIDValue = canonical_UINT256();
auto const memoDataValue = canonical_VL();
VaultDeleteBuilder builder{
accountValue,
@@ -39,6 +40,7 @@ TEST(TransactionsVaultDeleteTests, BuilderSettersRoundTrip)
};
// Set optional fields
builder.setMemoData(memoDataValue);
auto tx = builder.build(publicKey, secretKey);
@@ -62,6 +64,14 @@ TEST(TransactionsVaultDeleteTests, BuilderSettersRoundTrip)
}
// Verify optional fields
{
auto const& expected = memoDataValue;
auto const actualOpt = tx.getMemoData();
ASSERT_TRUE(actualOpt.has_value()) << "Optional field sfMemoData should be present";
expectEqualField(expected, *actualOpt, "sfMemoData");
EXPECT_TRUE(tx.hasMemoData());
}
}
// 2 & 4) Start from an STTx, construct a builder from it, build a new wrapper,
@@ -79,6 +89,7 @@ TEST(TransactionsVaultDeleteTests, BuilderFromStTxRoundTrip)
// Transaction-specific field values
auto const vaultIDValue = canonical_UINT256();
auto const memoDataValue = canonical_VL();
// Build an initial transaction
VaultDeleteBuilder initialBuilder{
@@ -88,6 +99,7 @@ TEST(TransactionsVaultDeleteTests, BuilderFromStTxRoundTrip)
feeValue
};
initialBuilder.setMemoData(memoDataValue);
auto initialTx = initialBuilder.build(publicKey, secretKey);
@@ -112,6 +124,13 @@ TEST(TransactionsVaultDeleteTests, BuilderFromStTxRoundTrip)
}
// Verify optional fields
{
auto const& expected = memoDataValue;
auto const actualOpt = rebuiltTx.getMemoData();
ASSERT_TRUE(actualOpt.has_value()) << "Optional field sfMemoData should be present";
expectEqualField(expected, *actualOpt, "sfMemoData");
}
}
// 3) Verify wrapper throws when constructed from wrong transaction type.
@@ -142,5 +161,35 @@ TEST(TransactionsVaultDeleteTests, BuilderThrowsOnWrongTxType)
EXPECT_THROW(VaultDeleteBuilder{wrongTx.getSTTx()}, std::runtime_error);
}
// 5) Build with only required fields and verify optional fields return nullopt.
TEST(TransactionsVaultDeleteTests, OptionalFieldsReturnNullopt)
{
// Generate a deterministic keypair for signing
auto const [publicKey, secretKey] =
generateKeyPair(KeyType::secp256k1, generateSeed("testVaultDeleteNullopt"));
// Common transaction fields
auto const accountValue = calcAccountID(publicKey);
std::uint32_t const sequenceValue = 3;
auto const feeValue = canonical_AMOUNT();
// Transaction-specific required field values
auto const vaultIDValue = canonical_UINT256();
VaultDeleteBuilder builder{
accountValue,
vaultIDValue,
sequenceValue,
feeValue
};
// Do NOT set optional fields
auto tx = builder.build(publicKey, secretKey);
// Verify optional fields are not present
EXPECT_FALSE(tx.hasMemoData());
EXPECT_FALSE(tx.getMemoData().has_value());
}
}