From 23eee6192f6d0144e9e8b912b13e938729ba8dc8 Mon Sep 17 00:00:00 2001 From: Vito <5780819+Tapanito@users.noreply.github.com> Date: Wed, 11 Mar 2026 15:22:12 +0100 Subject: [PATCH 1/7] feat: Make VaultID conditional on LoanBrokerSet MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit When updating an existing LoanBroker (LoanBrokerID present), VaultID must not be provided — the vault association is read from the broker object on ledger. VaultID remains required when creating a new LoanBroker. This change is gated behind fixLendingProtocolV1_1; pre-amendment behavior is preserved for historical transaction replay. - Change sfVaultID from soeREQUIRED to soeOPTIONAL - Gate VaultID field presence rules in preflight by amendment - Refactor preclaim into readVault/preclaimUpdate/preclaimCreate - Add pre- and post-amendment unit test coverage --- .../xrpl/protocol/detail/transactions.macro | 2 +- .../tx/transactors/lending/LoanBrokerSet.cpp | 190 ++++++++++---- src/test/app/LoanBroker_test.cpp | 237 +++++++++++++++--- src/test/jtx/TestHelpers.h | 4 + src/test/jtx/impl/TestHelpers.cpp | 10 + 5 files changed, 360 insertions(+), 83 deletions(-) diff --git a/include/xrpl/protocol/detail/transactions.macro b/include/xrpl/protocol/detail/transactions.macro index 15881effc8f..4e6390d4fa1 100644 --- a/include/xrpl/protocol/detail/transactions.macro +++ b/include/xrpl/protocol/detail/transactions.macro @@ -936,7 +936,7 @@ TRANSACTION(ttLOAN_BROKER_SET, 74, LoanBrokerSet, Delegation::delegable, featureLendingProtocol, createPseudoAcct | mayAuthorizeMPT, ({ - {sfVaultID, soeREQUIRED}, + {sfVaultID, soeOPTIONAL}, {sfLoanBrokerID, soeOPTIONAL}, {sfData, soeOPTIONAL}, {sfManagementFeeRate, soeOPTIONAL}, diff --git a/src/libxrpl/tx/transactors/lending/LoanBrokerSet.cpp b/src/libxrpl/tx/transactors/lending/LoanBrokerSet.cpp index c825a7fafe6..8d963019476 100644 --- a/src/libxrpl/tx/transactors/lending/LoanBrokerSet.cpp +++ b/src/libxrpl/tx/transactors/lending/LoanBrokerSet.cpp @@ -1,5 +1,6 @@ #include // +#include #include #include @@ -29,7 +30,9 @@ LoanBrokerSet::preflight(PreflightContext const& ctx) if (!validNumericRange(tx[~sfDebtMaximum], Number(maxMPTokenAmount), Number(0))) return temINVALID; - if (tx.isFieldPresent(sfLoanBrokerID)) + auto const isLoanBrokerUpdate = tx.isFieldPresent(sfLoanBrokerID); + + if (isLoanBrokerUpdate) { // Fixed fields can not be specified if we're modifying an existing // LoanBroker Object @@ -41,9 +44,27 @@ LoanBrokerSet::preflight(PreflightContext const& ctx) return temINVALID; } - if (auto const vaultID = tx.at(~sfVaultID)) + // Amendment-specific field presence rules + if (ctx.rules.enabled(fixLendingProtocolV1_1)) + { + if (isLoanBrokerUpdate) + { + if (tx.isFieldPresent(sfVaultID)) + return temINVALID; + } + else + { + if (!tx.isFieldPresent(sfVaultID) || tx[sfVaultID] == beast::zero) + return temINVALID; + } + } + else { - if (*vaultID == beast::zero) + // Pre-amendment: VaultID was soeREQUIRED, must always be present + if (!tx.isFieldPresent(sfVaultID)) + return temINVALID; + + if (tx[sfVaultID] == beast::zero) return temINVALID; } @@ -68,77 +89,152 @@ LoanBrokerSet::getValueFields() return valueFields; } -TER -LoanBrokerSet::preclaim(PreclaimContext const& ctx) +/** Read and validate a vault, checking existence and ownership. + * + * @param ctx The preclaim context. + * @param account The expected vault owner. + * @param id The vault ID to look up. + * @return The vault SLE on success, or a TER error. + */ +static Expected, TER> +readVault(PreclaimContext const& ctx, AccountID const& account, uint256 const& id) { - auto const& tx = ctx.tx; - - auto const account = tx[sfAccount]; - auto const vaultID = tx[sfVaultID]; - - auto const sleVault = ctx.view.read(keylet::vault(vaultID)); - if (!sleVault) + auto const sle = ctx.view.read(keylet::vault(id)); + if (!sle) { JLOG(ctx.j.warn()) << "Vault does not exist."; - return tecNO_ENTRY; + return Unexpected(tecNO_ENTRY); } - Asset const asset = sleVault->at(sfAsset); - - if (account != sleVault->at(sfOwner)) + if (account != sle->at(sfOwner)) { JLOG(ctx.j.warn()) << "Account is not the owner of the Vault."; - return tecNO_PERMISSION; + return Unexpected(tecNO_PERMISSION); } + return sle; +} - if (auto const brokerID = tx[~sfLoanBrokerID]) - { - // Updating an existing Broker +/** Preclaim validation for updating an existing LoanBroker. + * + * @param ctx The preclaim context. + * @param account The transaction submitter. + * @param brokerID The LoanBroker ID to update. + * @return The vault SLE on success, or a TER error. + */ +static Expected, TER> +preclaimUpdate(PreclaimContext const& ctx, AccountID const& account, uint256 const& brokerID) +{ + auto const& tx = ctx.tx; + bool const fixEnabled = ctx.view.rules().enabled(fixLendingProtocolV1_1); - auto const sleBroker = ctx.view.read(keylet::loanbroker(*brokerID)); + std::shared_ptr sleBroker; + std::shared_ptr sleVault; + + if (fixEnabled) + { + // Post-amendment: VaultID is not in the tx, read it from broker + sleBroker = ctx.view.read(keylet::loanbroker(brokerID)); if (!sleBroker) { JLOG(ctx.j.warn()) << "LoanBroker does not exist."; - return tecNO_ENTRY; + return Unexpected(tecNO_ENTRY); } - if (vaultID != sleBroker->at(sfVaultID)) - { - JLOG(ctx.j.warn()) << "Can not change VaultID on an existing LoanBroker."; - return tecNO_PERMISSION; - } - if (account != sleBroker->at(sfOwner)) + + auto const vault = readVault(ctx, account, sleBroker->at(sfVaultID)); + if (!vault) + return vault; + sleVault = *vault; + } + else + { + XRPL_ASSERT( + tx.isFieldPresent(sfVaultID), + "xrpl::LoanBrokerSet::preclaimUpdate : VaultID is present in the transaction"); + // Pre-amendment: vault is validated before broker to preserve + // the original error ordering for historical transaction replay. + auto const vault = readVault(ctx, account, tx[sfVaultID]); + if (!vault) + return vault; + sleVault = *vault; + + sleBroker = ctx.view.read(keylet::loanbroker(brokerID)); + if (!sleBroker) { - JLOG(ctx.j.warn()) << "Account is not the owner of the LoanBroker."; - return tecNO_PERMISSION; + JLOG(ctx.j.warn()) << "LoanBroker does not exist."; + return Unexpected(tecNO_ENTRY); } - - if (auto const debtMax = tx[~sfDebtMaximum]) + if (tx[sfVaultID] != sleBroker->at(sfVaultID)) { - // Can't reduce the debt maximum below the current total debt - auto const currentDebtTotal = sleBroker->at(sfDebtTotal); - if (*debtMax != 0 && *debtMax < currentDebtTotal) - { - JLOG(ctx.j.warn()) << "Cannot reduce DebtMaximum below current DebtTotal."; - return tecLIMIT_EXCEEDED; - } + JLOG(ctx.j.warn()) << "Can not change VaultID on an existing LoanBroker."; + return Unexpected(tecNO_PERMISSION); } } - else + + if (account != sleBroker->at(sfOwner)) { - if (auto const ter = canAddHolding(ctx.view, asset)) - return ter; + JLOG(ctx.j.warn()) << "Account is not the owner of the LoanBroker."; + return Unexpected(tecNO_PERMISSION); + } - if (auto const ter = checkFrozen(ctx.view, sleVault->at(sfAccount), sleVault->at(sfAsset))) + if (auto const debtMax = tx[~sfDebtMaximum]) + { + auto const currentDebtTotal = sleBroker->at(sfDebtTotal); + if (*debtMax != 0 && *debtMax < currentDebtTotal) { - JLOG(ctx.j.warn()) << "Vault pseudo-account is frozen."; - return ter; + JLOG(ctx.j.warn()) << "Cannot reduce DebtMaximum below current DebtTotal."; + return Unexpected(tecLIMIT_EXCEEDED); } } + return sleVault; +} + +/** Preclaim validation for creating a new LoanBroker. + * + * @param ctx The preclaim context. + * @param account The transaction submitter (vault owner). + * @return The vault SLE on success, or a TER error. + */ +static Expected, TER> +preclaimCreate(PreclaimContext const& ctx, AccountID const& account) +{ + auto const vault = readVault(ctx, account, ctx.tx[sfVaultID]); + if (!vault) + return vault; + auto const& sleVault = *vault; + + Asset const asset = sleVault->at(sfAsset); + if (auto const ter = canAddHolding(ctx.view, asset)) + return Unexpected(ter); + + if (auto const ter = checkFrozen(ctx.view, sleVault->at(sfAccount), sleVault->at(sfAsset))) + { + JLOG(ctx.j.warn()) << "Vault pseudo-account is frozen."; + return Unexpected(ter); + } + + return sleVault; +} + +TER +LoanBrokerSet::preclaim(PreclaimContext const& ctx) +{ + auto const account = ctx.tx[sfAccount]; + + auto const vault = [&]() -> Expected, TER> { + if (auto const brokerID = ctx.tx[~sfLoanBrokerID]) + return preclaimUpdate(ctx, account, *brokerID); + return preclaimCreate(ctx, account); + }(); + + if (!vault) + return vault.error(); + // Check that relevant values can be represented as the vault asset - // type. This is mostly only relevant for integral (non-IOU) types + // type. This is mostly only relevant for integral (non-IOU) types. + Asset const asset = (*vault)->at(sfAsset); for (auto const& field : getValueFields()) { - if (auto const value = tx[field]; value && STAmount{asset, *value} != *value) + if (auto const value = ctx.tx[field]; value && STAmount{asset, *value} != *value) { JLOG(ctx.j.warn()) << field.f->getName() << " (" << *value << ") can not be represented as a(n) " << to_string(asset) << "."; diff --git a/src/test/app/LoanBroker_test.cpp b/src/test/app/LoanBroker_test.cpp index 4c108d9007f..0cee023d96e 100644 --- a/src/test/app/LoanBroker_test.cpp +++ b/src/test/app/LoanBroker_test.cpp @@ -397,7 +397,7 @@ class LoanBroker_test : public beast::unit_test::suite } // no-op - env(set(alice, vault.vaultID), loanBrokerID(keylet.key)); + env(set(alice), loanBrokerID(keylet.key)); env.close(); // Make modifications to the broker @@ -413,10 +413,7 @@ class LoanBroker_test : public beast::unit_test::suite // Verify that fields get removed when set to default values // Debt maximum: explicit 0 // Data: explicit empty - env(set(alice, vault.vaultID), - loanBrokerID(broker->key()), - debtMaximum(Number(0)), - data("")); + env(set(alice), loanBrokerID(broker->key()), debtMaximum(Number(0)), data("")); env.close(); // Check the updated fields @@ -673,35 +670,29 @@ class LoanBroker_test : public beast::unit_test::suite auto const nextKeylet = keylet::loanbroker(alice.id(), env.seq(alice)); // fields that can't be changed - // LoanBrokerID + // LoanBrokerID (non-existent) + env(set(alice), loanBrokerID(nextKeylet.key), ter(tecNO_ENTRY), THISLINE); + // VaultID is rejected on update (post-amendment) env(set(alice, vault.vaultID), - loanBrokerID(nextKeylet.key), - ter(tecNO_ENTRY), - THISLINE); - // VaultID - env(set(alice, nextKeylet.key), loanBrokerID(broker->key()), - ter(tecNO_ENTRY), + ter(temINVALID), THISLINE); // Owner - env(set(evan, vault.vaultID), - loanBrokerID(broker->key()), - ter(tecNO_PERMISSION), - THISLINE); + env(set(evan), loanBrokerID(broker->key()), ter(tecNO_PERMISSION), THISLINE); // ManagementFeeRate - env(set(alice, vault.vaultID), + env(set(alice), loanBrokerID(broker->key()), managementFeeRate(maxManagementFeeRate), ter(temINVALID), THISLINE); // CoverRateMinimum - env(set(alice, vault.vaultID), + env(set(alice), loanBrokerID(broker->key()), coverRateMinimum(maxManagementFeeRate), ter(temINVALID), THISLINE); // CoverRateLiquidation - env(set(alice, vault.vaultID), + env(set(alice), loanBrokerID(broker->key()), coverRateLiquidation(maxManagementFeeRate), ter(temINVALID), @@ -710,14 +701,14 @@ class LoanBroker_test : public beast::unit_test::suite // fields that can be changed testData = "Test Data 1234"; // Bad data: too long - env(set(alice, vault.vaultID), + env(set(alice), loanBrokerID(broker->key()), data(std::string(maxDataPayloadLength + 1, 'W')), ter(temINVALID), THISLINE); // Bad debt maximum - env(set(alice, vault.vaultID), + env(set(alice), loanBrokerID(broker->key()), debtMaximum(Number(-175, -1)), ter(temINVALID), @@ -725,7 +716,7 @@ class LoanBroker_test : public beast::unit_test::suite Number debtMax{175, -1}; if (vault.asset.integral()) { - env(set(alice, vault.vaultID), + env(set(alice), loanBrokerID(broker->key()), data(testData), debtMaximum(debtMax), @@ -734,7 +725,7 @@ class LoanBroker_test : public beast::unit_test::suite roundToAsset(vault.asset, debtMax); } // Data & Debt maximum - env(set(alice, vault.vaultID), + env(set(alice), loanBrokerID(broker->key()), data(testData), debtMaximum(debtMax), @@ -781,10 +772,7 @@ class LoanBroker_test : public beast::unit_test::suite }, [&](SLE::const_ref broker) { // Reset Data & Debt maximum to default values - env(set(alice, vault.vaultID), - loanBrokerID(broker->key()), - data(""), - debtMaximum(Number(0))); + env(set(alice), loanBrokerID(broker->key()), data(""), debtMaximum(Number(0))); }, [&](SLE::const_ref broker) { // Check the updated fields @@ -1035,13 +1023,8 @@ class LoanBroker_test : public beast::unit_test::suite if (brokerTest == Set) { // preflight: temINVALID (empty/zero broker id) - testZeroBrokerID([&]() { - return env.json(set(alice, vaultInfo.vaultID), loanBrokerID(brokerKeylet.key)); - }); - // preflight: temINVALID (empty/zero vault id) - testZeroVaultID([&]() { - return env.json(set(alice, vaultInfo.vaultID), loanBrokerID(brokerKeylet.key)); - }); + testZeroBrokerID( + [&]() { return env.json(set(alice), loanBrokerID(brokerKeylet.key)); }); if (asset.holds()) { @@ -1392,7 +1375,7 @@ class LoanBroker_test : public beast::unit_test::suite BEAST_EXPECT(broker->at(sfDebtTotal) == 50); auto debtTotal = broker->at(sfDebtTotal); - auto tx2 = set(alice, vaultInfo.vaultID); + auto tx2 = set(alice); tx2[sfLoanBrokerID] = to_string(brokerKeylet.key); tx2[sfDebtMaximum] = debtTotal - 1; env(tx2, ter(tecLIMIT_EXCEEDED), THISLINE); @@ -1797,11 +1780,195 @@ class LoanBroker_test : public beast::unit_test::suite BEAST_EXPECT(env.enabled(fixLendingProtocolV1_1)); } + void + testLoanBrokerSetVaultIDAmendment() + { + using namespace jtx; + using namespace loanBroker; + + Account const issuer{"issuer"}; + Account const alice{"alice"}; + Account const evan{"evan"}; + + // Helper to set up a vault and broker for testing + auto const setup = [&](Env& env) { + Vault vault{env}; + env.fund(XRP(100'000), issuer, alice, evan); + env.close(); + + env(trust(alice, issuer["IOU"](1'000'000)), THISLINE); + env.close(); + PrettyAsset const asset = issuer["IOU"]; + env(pay(issuer, alice, asset(100'000)), THISLINE); + env.close(); + + auto [tx, vaultKeylet] = vault.create({.owner = alice, .asset = asset}); + env(tx, THISLINE); + env.close(); + + env(vault.deposit({.depositor = alice, .id = vaultKeylet.key, .amount = asset(50)}), + THISLINE); + env.close(); + + auto const brokerKeylet = keylet::loanbroker(alice.id(), env.seq(alice)); + env(set(alice, vaultKeylet.key), THISLINE); + env.close(); + + struct Result + { + uint256 vaultID; + Keylet brokerKeylet; + }; + return Result{vaultKeylet.key, brokerKeylet}; + }; + + // Post-amendment: VaultID must not be present on update + { + testcase("LoanBrokerSet post-amendment: VaultID rejected on update"); + Env env(*this); + BEAST_EXPECT(env.enabled(fixLendingProtocolV1_1)); + auto const [vaultID, brokerKL] = setup(env); + + // Update with VaultID → temINVALID + env(set(alice, vaultID), loanBrokerID(brokerKL.key), ter(temINVALID), THISLINE); + + // Update without VaultID succeeds + env(set(alice), loanBrokerID(brokerKL.key), data("post-amendment update"), THISLINE); + env.close(); + + auto const broker = env.le(brokerKL); + BEAST_EXPECT(broker); + if (broker) + BEAST_EXPECT(checkVL(broker->at(sfData), "post-amendment update")); + } + + // Post-amendment: Create requires VaultID + { + testcase("LoanBrokerSet post-amendment: VaultID required on create"); + Env env(*this); + BEAST_EXPECT(env.enabled(fixLendingProtocolV1_1)); + env.fund(XRP(100'000), issuer, alice); + env.close(); + + // Create without VaultID → temINVALID + env(set(alice), ter(temINVALID), THISLINE); + + // Create with zero VaultID → temINVALID + env(set(alice, uint256{}), ter(temINVALID), THISLINE); + } + + // Post-amendment: Update by wrong owner → tecNO_PERMISSION + { + testcase("LoanBrokerSet post-amendment: wrong owner rejected"); + Env env(*this); + auto const [vaultID, brokerKL] = setup(env); + + env(set(evan), loanBrokerID(brokerKL.key), ter(tecNO_PERMISSION), THISLINE); + } + + // Post-amendment: Update non-existent broker → tecNO_ENTRY + { + testcase("LoanBrokerSet post-amendment: non-existent broker"); + Env env(*this); + auto const [vaultID, brokerKL] = setup(env); + + env(set(alice), loanBrokerID(uint256{1}), ter(tecNO_ENTRY), THISLINE); + } + + // Pre-amendment: VaultID required on both create and update + { + testcase("LoanBrokerSet pre-amendment: VaultID required on update"); + Env env(*this); + env.disableFeature(fixLendingProtocolV1_1); + BEAST_EXPECT(!env.enabled(fixLendingProtocolV1_1)); + auto const [vaultID, brokerKL] = setup(env); + + // Update without VaultID → temINVALID (pre-amendment requires it) + env(set(alice), loanBrokerID(brokerKL.key), ter(temINVALID), THISLINE); + + // Update with matching VaultID succeeds (old behavior) + env(set(alice, vaultID), + loanBrokerID(brokerKL.key), + data("pre-amendment update"), + THISLINE); + env.close(); + + auto const broker = env.le(brokerKL); + BEAST_EXPECT(broker); + if (broker) + BEAST_EXPECT(checkVL(broker->at(sfData), "pre-amendment update")); + } + + // Pre-amendment: mismatched VaultID on update → tecNO_PERMISSION + { + testcase("LoanBrokerSet pre-amendment: mismatched VaultID"); + Env env(*this); + env.disableFeature(fixLendingProtocolV1_1); + + Vault vault{env}; + env.fund(XRP(100'000), issuer, alice); + env.close(); + env(trust(alice, issuer["IOU"](1'000'000)), THISLINE); + env.close(); + PrettyAsset const asset = issuer["IOU"]; + env(pay(issuer, alice, asset(100'000)), THISLINE); + env.close(); + + // Create two vaults + auto [tx1, vaultKL1] = vault.create({.owner = alice, .asset = asset}); + env(tx1, THISLINE); + env.close(); + auto [tx2, vaultKL2] = vault.create({.owner = alice, .asset = asset}); + env(tx2, THISLINE); + env.close(); + + env(vault.deposit({.depositor = alice, .id = vaultKL1.key, .amount = asset(50)}), + THISLINE); + env.close(); + + auto const brokerKL = keylet::loanbroker(alice.id(), env.seq(alice)); + env(set(alice, vaultKL1.key), THISLINE); + env.close(); + + // Update with different vault → tecNO_PERMISSION + env(set(alice, vaultKL2.key), + loanBrokerID(brokerKL.key), + ter(tecNO_PERMISSION), + THISLINE); + } + + // Pre-amendment: Create without VaultID → temINVALID + { + testcase("LoanBrokerSet pre-amendment: create requires VaultID"); + Env env(*this); + env.disableFeature(fixLendingProtocolV1_1); + env.fund(XRP(100'000), issuer, alice); + env.close(); + + env(set(alice), ter(temINVALID), THISLINE); + } + + // Pre-amendment: immutable fields still rejected on update + { + testcase("LoanBrokerSet pre-amendment: immutable fields on update"); + Env env(*this); + env.disableFeature(fixLendingProtocolV1_1); + auto const [vaultID, brokerKL] = setup(env); + + env(set(alice, vaultID), + loanBrokerID(brokerKL.key), + managementFeeRate(TenthBips16(1)), + ter(temINVALID), + THISLINE); + } + } + public: void run() override { testFixAmendmentEnabled(); + testLoanBrokerSetVaultIDAmendment(); testLoanBrokerSetDebtMaximum(); testLoanBrokerCoverDepositNullVault(); diff --git a/src/test/jtx/TestHelpers.h b/src/test/jtx/TestHelpers.h index 2836748ec3c..cada18d896d 100644 --- a/src/test/jtx/TestHelpers.h +++ b/src/test/jtx/TestHelpers.h @@ -682,6 +682,10 @@ namespace loanBroker { Json::Value set(AccountID const& account, uint256 const& vaultId, std::uint32_t flags = 0); +// Overload for modifying an existing LoanBroker (use with loanBrokerID()) +Json::Value +set(AccountID const& account, std::uint32_t flags = 0); + // Use "del" because "delete" is a reserved word in C++. Json::Value del(AccountID const& account, uint256 const& brokerID, std::uint32_t flags = 0); diff --git a/src/test/jtx/impl/TestHelpers.cpp b/src/test/jtx/impl/TestHelpers.cpp index 5e7a31adcd5..5401fbd927f 100644 --- a/src/test/jtx/impl/TestHelpers.cpp +++ b/src/test/jtx/impl/TestHelpers.cpp @@ -343,6 +343,16 @@ set(AccountID const& account, uint256 const& vaultId, uint32_t flags) return jv; } +Json::Value +set(AccountID const& account, uint32_t flags) +{ + Json::Value jv; + jv[sfTransactionType] = jss::LoanBrokerSet; + jv[sfAccount] = to_string(account); + jv[sfFlags] = flags; + return jv; +} + Json::Value del(AccountID const& account, uint256 const& brokerID, uint32_t flags) { From 3314c21542dc0a706497c35f9ca29f4937829fc2 Mon Sep 17 00:00:00 2001 From: Vito <5780819+Tapanito@users.noreply.github.com> Date: Wed, 11 Mar 2026 16:44:36 +0100 Subject: [PATCH 2/7] removes unused variables --- src/test/app/LoanBroker_test.cpp | 14 ++------------ 1 file changed, 2 insertions(+), 12 deletions(-) diff --git a/src/test/app/LoanBroker_test.cpp b/src/test/app/LoanBroker_test.cpp index 0cee023d96e..aa4cf59421c 100644 --- a/src/test/app/LoanBroker_test.cpp +++ b/src/test/app/LoanBroker_test.cpp @@ -848,17 +848,6 @@ class LoanBroker_test : public beast::unit_test::suite // test env(jv, txflags(tfFullyCanonicalSig), ter(temINVALID), THISLINE); }; - auto testZeroVaultID = [&](auto&& getTxJv) { - auto jv = getTxJv(); - // empty broker ID - jv[sfVaultID] = ""; - env(jv, ter(temINVALID), THISLINE); - // zero broker ID - jv[sfVaultID] = to_string(uint256{}); - // needs a flag to distinguish the parsed STTx from the prior - // test - env(jv, txflags(tfFullyCanonicalSig), ter(temINVALID), THISLINE); - }; if (brokerTest == CoverDeposit) { @@ -1870,7 +1859,8 @@ class LoanBroker_test : public beast::unit_test::suite { testcase("LoanBrokerSet post-amendment: non-existent broker"); Env env(*this); - auto const [vaultID, brokerKL] = setup(env); + env.fund(XRP(100'000), alice); + env.close(); env(set(alice), loanBrokerID(uint256{1}), ter(tecNO_ENTRY), THISLINE); } From df1a55d11a193ccd9afa7223d4c9df44cf59b2af Mon Sep 17 00:00:00 2001 From: Vito <5780819+Tapanito@users.noreply.github.com> Date: Thu, 12 Mar 2026 14:51:40 +0100 Subject: [PATCH 3/7] adds additional unit-tests --- src/libxrpl/tx/transactors/lending/LoanBrokerSet.cpp | 2 ++ src/test/app/LoanBroker_test.cpp | 10 ++++++++++ 2 files changed, 12 insertions(+) diff --git a/src/libxrpl/tx/transactors/lending/LoanBrokerSet.cpp b/src/libxrpl/tx/transactors/lending/LoanBrokerSet.cpp index 8d963019476..ca898ca307d 100644 --- a/src/libxrpl/tx/transactors/lending/LoanBrokerSet.cpp +++ b/src/libxrpl/tx/transactors/lending/LoanBrokerSet.cpp @@ -169,6 +169,8 @@ preclaimUpdate(PreclaimContext const& ctx, AccountID const& account, uint256 con } } + XRPL_ASSERT(sleVault, "xrpl::LoanBrokerSet::preclaimUpdate : sleVault is initialized"); + if (account != sleBroker->at(sfOwner)) { JLOG(ctx.j.warn()) << "Account is not the owner of the LoanBroker."; diff --git a/src/test/app/LoanBroker_test.cpp b/src/test/app/LoanBroker_test.cpp index aa4cf59421c..e7892fd68f4 100644 --- a/src/test/app/LoanBroker_test.cpp +++ b/src/test/app/LoanBroker_test.cpp @@ -1951,6 +1951,16 @@ class LoanBroker_test : public beast::unit_test::suite ter(temINVALID), THISLINE); } + + // Pre-amendment: zero VaultID on update → temINVALID + { + testcase("LoanBrokerSet pre-amendment: zero VaultID on update"); + Env env(*this); + env.disableFeature(fixLendingProtocolV1_1); + auto const [vaultID, brokerKL] = setup(env); + + env(set(alice, uint256{}), loanBrokerID(brokerKL.key), ter(temINVALID), THISLINE); + } } public: From 20d6e93b573de9f3bf2986c36a4019d4d612984e Mon Sep 17 00:00:00 2001 From: Vito <5780819+Tapanito@users.noreply.github.com> Date: Tue, 31 Mar 2026 14:09:33 +0200 Subject: [PATCH 4/7] fix: bugs from merge --- .../transactions/LoanBrokerSet.h | 29 +++++--- .../tx/transactors/lending/LoanBrokerSet.cpp | 4 +- src/test/app/LoanBroker_test.cpp | 72 ++++++++----------- .../transactions/LoanBrokerSetTests.cpp | 23 +++--- 4 files changed, 67 insertions(+), 61 deletions(-) diff --git a/include/xrpl/protocol_autogen/transactions/LoanBrokerSet.h b/include/xrpl/protocol_autogen/transactions/LoanBrokerSet.h index ba6ca062666..1347cc63ef5 100644 --- a/include/xrpl/protocol_autogen/transactions/LoanBrokerSet.h +++ b/include/xrpl/protocol_autogen/transactions/LoanBrokerSet.h @@ -48,14 +48,29 @@ class LoanBrokerSet : public TransactionBase // Transaction-specific field getters /** - * @brief Get sfVaultID (soeREQUIRED) - * @return The field value. + * @brief Get sfVaultID (soeOPTIONAL) + * @return The field value, or std::nullopt if not present. */ [[nodiscard]] - SF_UINT256::type::value_type + protocol_autogen::Optional getVaultID() const { - return this->tx_->at(sfVaultID); + if (hasVaultID()) + { + return this->tx_->at(sfVaultID); + } + return std::nullopt; + } + + /** + * @brief Check if sfVaultID is present. + * @return True if the field is present, false otherwise. + */ + [[nodiscard]] + bool + hasVaultID() const + { + return this->tx_->isFieldPresent(sfVaultID); } /** @@ -228,17 +243,15 @@ class LoanBrokerSetBuilder : public TransactionBuilderBase /** * @brief Construct a new LoanBrokerSetBuilder with required fields. * @param account The account initiating the transaction. - * @param vaultID The sfVaultID field value. * @param sequence Optional sequence number for the transaction. * @param fee Optional fee for the transaction. */ LoanBrokerSetBuilder(SF_ACCOUNT::type::value_type account, - std::decay_t const& vaultID, std::optional sequence = std::nullopt, + std::optional sequence = std::nullopt, std::optional fee = std::nullopt ) : TransactionBuilderBase(ttLOAN_BROKER_SET, account, sequence, fee) { - setVaultID(vaultID); } /** @@ -258,7 +271,7 @@ class LoanBrokerSetBuilder : public TransactionBuilderBase /** @brief Transaction-specific field setters */ /** - * @brief Set sfVaultID (soeREQUIRED) + * @brief Set sfVaultID (soeOPTIONAL) * @return Reference to this builder for method chaining. */ LoanBrokerSetBuilder& diff --git a/src/libxrpl/tx/transactors/lending/LoanBrokerSet.cpp b/src/libxrpl/tx/transactors/lending/LoanBrokerSet.cpp index d27738cddef..4ef0d0aa30b 100644 --- a/src/libxrpl/tx/transactors/lending/LoanBrokerSet.cpp +++ b/src/libxrpl/tx/transactors/lending/LoanBrokerSet.cpp @@ -46,7 +46,7 @@ LoanBrokerSet::preflight(PreflightContext const& ctx) } // Amendment-specific field presence rules - if (ctx.rules.enabled(fixLendingProtocolV1_1)) + if (ctx.rules.enabled(featureLendingProtocolV1_1)) { if (isLoanBrokerUpdate) { @@ -125,7 +125,7 @@ static Expected, TER> preclaimUpdate(PreclaimContext const& ctx, AccountID const& account, uint256 const& brokerID) { auto const& tx = ctx.tx; - bool const fixEnabled = ctx.view.rules().enabled(fixLendingProtocolV1_1); + bool const fixEnabled = ctx.view.rules().enabled(featureLendingProtocolV1_1); std::shared_ptr sleBroker; std::shared_ptr sleVault; diff --git a/src/test/app/LoanBroker_test.cpp b/src/test/app/LoanBroker_test.cpp index 62163e34f76..e2213d9fade 100644 --- a/src/test/app/LoanBroker_test.cpp +++ b/src/test/app/LoanBroker_test.cpp @@ -1775,22 +1775,21 @@ class LoanBroker_test : public beast::unit_test::suite env.fund(XRP(100'000), issuer, alice, evan); env.close(); - env(trust(alice, issuer["IOU"](1'000'000)), THISLINE); + env(trust(alice, issuer["IOU"](1'000'000))); env.close(); PrettyAsset const asset = issuer["IOU"]; - env(pay(issuer, alice, asset(100'000)), THISLINE); + env(pay(issuer, alice, asset(100'000))); env.close(); auto [tx, vaultKeylet] = vault.create({.owner = alice, .asset = asset}); - env(tx, THISLINE); + env(tx); env.close(); - env(vault.deposit({.depositor = alice, .id = vaultKeylet.key, .amount = asset(50)}), - THISLINE); + env(vault.deposit({.depositor = alice, .id = vaultKeylet.key, .amount = asset(50)})); env.close(); auto const brokerKeylet = keylet::loanbroker(alice.id(), env.seq(alice)); - env(set(alice, vaultKeylet.key), THISLINE); + env(set(alice, vaultKeylet.key)); env.close(); struct Result @@ -1805,14 +1804,14 @@ class LoanBroker_test : public beast::unit_test::suite { testcase("LoanBrokerSet post-amendment: VaultID rejected on update"); Env env(*this); - BEAST_EXPECT(env.enabled(fixLendingProtocolV1_1)); + BEAST_EXPECT(env.enabled(featureLendingProtocolV1_1)); auto const [vaultID, brokerKL] = setup(env); // Update with VaultID → temINVALID - env(set(alice, vaultID), loanBrokerID(brokerKL.key), ter(temINVALID), THISLINE); + env(set(alice, vaultID), loanBrokerID(brokerKL.key), ter(temINVALID)); // Update without VaultID succeeds - env(set(alice), loanBrokerID(brokerKL.key), data("post-amendment update"), THISLINE); + env(set(alice), loanBrokerID(brokerKL.key), data("post-amendment update")); env.close(); auto const broker = env.le(brokerKL); @@ -1825,15 +1824,15 @@ class LoanBroker_test : public beast::unit_test::suite { testcase("LoanBrokerSet post-amendment: VaultID required on create"); Env env(*this); - BEAST_EXPECT(env.enabled(fixLendingProtocolV1_1)); + BEAST_EXPECT(env.enabled(featureLendingProtocolV1_1)); env.fund(XRP(100'000), issuer, alice); env.close(); // Create without VaultID → temINVALID - env(set(alice), ter(temINVALID), THISLINE); + env(set(alice), ter(temINVALID)); // Create with zero VaultID → temINVALID - env(set(alice, uint256{}), ter(temINVALID), THISLINE); + env(set(alice, uint256{}), ter(temINVALID)); } // Post-amendment: Update by wrong owner → tecNO_PERMISSION @@ -1842,7 +1841,7 @@ class LoanBroker_test : public beast::unit_test::suite Env env(*this); auto const [vaultID, brokerKL] = setup(env); - env(set(evan), loanBrokerID(brokerKL.key), ter(tecNO_PERMISSION), THISLINE); + env(set(evan), loanBrokerID(brokerKL.key), ter(tecNO_PERMISSION)); } // Post-amendment: Update non-existent broker → tecNO_ENTRY @@ -1852,25 +1851,22 @@ class LoanBroker_test : public beast::unit_test::suite env.fund(XRP(100'000), alice); env.close(); - env(set(alice), loanBrokerID(uint256{1}), ter(tecNO_ENTRY), THISLINE); + env(set(alice), loanBrokerID(uint256{1}), ter(tecNO_ENTRY)); } // Pre-amendment: VaultID required on both create and update { testcase("LoanBrokerSet pre-amendment: VaultID required on update"); Env env(*this); - env.disableFeature(fixLendingProtocolV1_1); - BEAST_EXPECT(!env.enabled(fixLendingProtocolV1_1)); + env.disableFeature(featureLendingProtocolV1_1); + BEAST_EXPECT(!env.enabled(featureLendingProtocolV1_1)); auto const [vaultID, brokerKL] = setup(env); // Update without VaultID → temINVALID (pre-amendment requires it) - env(set(alice), loanBrokerID(brokerKL.key), ter(temINVALID), THISLINE); + env(set(alice), loanBrokerID(brokerKL.key), ter(temINVALID)); // Update with matching VaultID succeeds (old behavior) - env(set(alice, vaultID), - loanBrokerID(brokerKL.key), - data("pre-amendment update"), - THISLINE); + env(set(alice, vaultID), loanBrokerID(brokerKL.key), data("pre-amendment update")); env.close(); auto const broker = env.le(brokerKL); @@ -1883,73 +1879,68 @@ class LoanBroker_test : public beast::unit_test::suite { testcase("LoanBrokerSet pre-amendment: mismatched VaultID"); Env env(*this); - env.disableFeature(fixLendingProtocolV1_1); + env.disableFeature(featureLendingProtocolV1_1); Vault vault{env}; env.fund(XRP(100'000), issuer, alice); env.close(); - env(trust(alice, issuer["IOU"](1'000'000)), THISLINE); + env(trust(alice, issuer["IOU"](1'000'000))); env.close(); PrettyAsset const asset = issuer["IOU"]; - env(pay(issuer, alice, asset(100'000)), THISLINE); + env(pay(issuer, alice, asset(100'000))); env.close(); // Create two vaults auto [tx1, vaultKL1] = vault.create({.owner = alice, .asset = asset}); - env(tx1, THISLINE); + env(tx1); env.close(); auto [tx2, vaultKL2] = vault.create({.owner = alice, .asset = asset}); - env(tx2, THISLINE); + env(tx2); env.close(); - env(vault.deposit({.depositor = alice, .id = vaultKL1.key, .amount = asset(50)}), - THISLINE); + env(vault.deposit({.depositor = alice, .id = vaultKL1.key, .amount = asset(50)})); env.close(); auto const brokerKL = keylet::loanbroker(alice.id(), env.seq(alice)); - env(set(alice, vaultKL1.key), THISLINE); + env(set(alice, vaultKL1.key)); env.close(); // Update with different vault → tecNO_PERMISSION - env(set(alice, vaultKL2.key), - loanBrokerID(brokerKL.key), - ter(tecNO_PERMISSION), - THISLINE); + env(set(alice, vaultKL2.key), loanBrokerID(brokerKL.key), ter(tecNO_PERMISSION)); } // Pre-amendment: Create without VaultID → temINVALID { testcase("LoanBrokerSet pre-amendment: create requires VaultID"); Env env(*this); - env.disableFeature(fixLendingProtocolV1_1); + env.disableFeature(featureLendingProtocolV1_1); env.fund(XRP(100'000), issuer, alice); env.close(); - env(set(alice), ter(temINVALID), THISLINE); + env(set(alice), ter(temINVALID)); } // Pre-amendment: immutable fields still rejected on update { testcase("LoanBrokerSet pre-amendment: immutable fields on update"); Env env(*this); - env.disableFeature(fixLendingProtocolV1_1); + env.disableFeature(featureLendingProtocolV1_1); auto const [vaultID, brokerKL] = setup(env); env(set(alice, vaultID), loanBrokerID(brokerKL.key), managementFeeRate(TenthBips16(1)), - ter(temINVALID), - THISLINE); + ter(temINVALID)); } // Pre-amendment: zero VaultID on update → temINVALID { testcase("LoanBrokerSet pre-amendment: zero VaultID on update"); Env env(*this); - env.disableFeature(fixLendingProtocolV1_1); + env.disableFeature(featureLendingProtocolV1_1); auto const [vaultID, brokerKL] = setup(env); - env(set(alice, uint256{}), loanBrokerID(brokerKL.key), ter(temINVALID), THISLINE); + env(set(alice, uint256{}), loanBrokerID(brokerKL.key), ter(temINVALID)); } } @@ -1957,7 +1948,6 @@ class LoanBroker_test : public beast::unit_test::suite void run() override { - testFixAmendmentEnabled(); testLoanBrokerSetVaultIDAmendment(); testFeatureLendingProtocolV1_1enabled(); testLoanBrokerSetDebtMaximum(); diff --git a/src/tests/libxrpl/protocol_autogen/transactions/LoanBrokerSetTests.cpp b/src/tests/libxrpl/protocol_autogen/transactions/LoanBrokerSetTests.cpp index e270332f427..318530b4de0 100644 --- a/src/tests/libxrpl/protocol_autogen/transactions/LoanBrokerSetTests.cpp +++ b/src/tests/libxrpl/protocol_autogen/transactions/LoanBrokerSetTests.cpp @@ -39,12 +39,12 @@ TEST(TransactionsLoanBrokerSetTests, BuilderSettersRoundTrip) LoanBrokerSetBuilder builder{ accountValue, - vaultIDValue, sequenceValue, feeValue }; // Set optional fields + builder.setVaultID(vaultIDValue); builder.setLoanBrokerID(loanBrokerIDValue); builder.setData(dataValue); builder.setManagementFeeRate(managementFeeRateValue); @@ -67,13 +67,15 @@ TEST(TransactionsLoanBrokerSetTests, BuilderSettersRoundTrip) EXPECT_EQ(tx.getFee(), feeValue); // Verify required fields + // Verify optional fields { auto const& expected = vaultIDValue; - auto const actual = tx.getVaultID(); - expectEqualField(expected, actual, "sfVaultID"); + auto const actualOpt = tx.getVaultID(); + ASSERT_TRUE(actualOpt.has_value()) << "Optional field sfVaultID should be present"; + expectEqualField(expected, *actualOpt, "sfVaultID"); + EXPECT_TRUE(tx.hasVaultID()); } - // Verify optional fields { auto const& expected = loanBrokerIDValue; auto const actualOpt = tx.getLoanBrokerID(); @@ -149,11 +151,11 @@ TEST(TransactionsLoanBrokerSetTests, BuilderFromStTxRoundTrip) // Build an initial transaction LoanBrokerSetBuilder initialBuilder{ accountValue, - vaultIDValue, sequenceValue, feeValue }; + initialBuilder.setVaultID(vaultIDValue); initialBuilder.setLoanBrokerID(loanBrokerIDValue); initialBuilder.setData(dataValue); initialBuilder.setManagementFeeRate(managementFeeRateValue); @@ -177,13 +179,14 @@ TEST(TransactionsLoanBrokerSetTests, BuilderFromStTxRoundTrip) EXPECT_EQ(rebuiltTx.getFee(), feeValue); // Verify required fields + // Verify optional fields { auto const& expected = vaultIDValue; - auto const actual = rebuiltTx.getVaultID(); - expectEqualField(expected, actual, "sfVaultID"); + auto const actualOpt = rebuiltTx.getVaultID(); + ASSERT_TRUE(actualOpt.has_value()) << "Optional field sfVaultID should be present"; + expectEqualField(expected, *actualOpt, "sfVaultID"); } - // Verify optional fields { auto const& expected = loanBrokerIDValue; auto const actualOpt = rebuiltTx.getLoanBrokerID(); @@ -269,11 +272,9 @@ TEST(TransactionsLoanBrokerSetTests, OptionalFieldsReturnNullopt) auto const feeValue = canonical_AMOUNT(); // Transaction-specific required field values - auto const vaultIDValue = canonical_UINT256(); LoanBrokerSetBuilder builder{ accountValue, - vaultIDValue, sequenceValue, feeValue }; @@ -283,6 +284,8 @@ TEST(TransactionsLoanBrokerSetTests, OptionalFieldsReturnNullopt) auto tx = builder.build(publicKey, secretKey); // Verify optional fields are not present + EXPECT_FALSE(tx.hasVaultID()); + EXPECT_FALSE(tx.getVaultID().has_value()); EXPECT_FALSE(tx.hasLoanBrokerID()); EXPECT_FALSE(tx.getLoanBrokerID().has_value()); EXPECT_FALSE(tx.hasData()); From 929d15b380a4cf109211eff05582f0f5e153578d Mon Sep 17 00:00:00 2001 From: Vito <5780819+Tapanito@users.noreply.github.com> Date: Tue, 31 Mar 2026 14:28:19 +0200 Subject: [PATCH 5/7] fix: deduplicate loanBroker::set and fix Lifecycle test failures MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Merge two `loanBroker::set` overloads into one using `std::optional` for the VaultID parameter. Fix variable rename typo (`vault` → `maybeVault`) in LoanBrokerSet::preclaim. Update Lifecycle tests to match featureLendingProtocolV1_1 semantics: update transactions must not include VaultID. --- .../tx/transactors/lending/LoanBrokerSet.cpp | 8 ++++---- src/test/app/LoanBroker_test.cpp | 10 ++++------ src/test/jtx/TestHelpers.h | 8 +++----- src/test/jtx/impl/TestHelpers.cpp | 15 +++------------ 4 files changed, 14 insertions(+), 27 deletions(-) diff --git a/src/libxrpl/tx/transactors/lending/LoanBrokerSet.cpp b/src/libxrpl/tx/transactors/lending/LoanBrokerSet.cpp index 4ef0d0aa30b..e291d3c166a 100644 --- a/src/libxrpl/tx/transactors/lending/LoanBrokerSet.cpp +++ b/src/libxrpl/tx/transactors/lending/LoanBrokerSet.cpp @@ -223,18 +223,18 @@ LoanBrokerSet::preclaim(PreclaimContext const& ctx) { auto const account = ctx.tx[sfAccount]; - auto const vault = [&]() -> Expected, TER> { + auto const maybeVault = [&]() -> Expected, TER> { if (auto const brokerID = ctx.tx[~sfLoanBrokerID]) return preclaimUpdate(ctx, account, *brokerID); return preclaimCreate(ctx, account); }(); - if (!vault) - return vault.error(); + if (!maybeVault) + return maybeVault.error(); // Check that relevant values can be represented as the vault asset // type. This is mostly only relevant for integral (non-IOU) types. - Asset const asset = (*vault)->at(sfAsset); + Asset const asset = (*maybeVault)->at(sfAsset); for (auto const& field : getValueFields()) { if (auto const value = ctx.tx[field]; value && STAmount{asset, *value} != *value) diff --git a/src/test/app/LoanBroker_test.cpp b/src/test/app/LoanBroker_test.cpp index e2213d9fade..f05ab1af094 100644 --- a/src/test/app/LoanBroker_test.cpp +++ b/src/test/app/LoanBroker_test.cpp @@ -684,13 +684,11 @@ class LoanBroker_test : public beast::unit_test::suite // fields that can't be changed // LoanBrokerID - env(set(alice, vault.vaultID), loanBrokerID(nextKeylet.key), ter(tecNO_ENTRY)); - // VaultID - env(set(alice, nextKeylet.key), loanBrokerID(broker->key()), ter(tecNO_ENTRY)); + env(set(alice), loanBrokerID(nextKeylet.key), ter(tecNO_ENTRY)); + // VaultID (rejected in preflight when amendment is active) + env(set(alice, nextKeylet.key), loanBrokerID(broker->key()), ter(temINVALID)); // Owner - env(set(evan, vault.vaultID), - loanBrokerID(broker->key()), - ter(tecNO_PERMISSION)); + env(set(evan), loanBrokerID(broker->key()), ter(tecNO_PERMISSION)); // ManagementFeeRate env(set(alice), loanBrokerID(broker->key()), diff --git a/src/test/jtx/TestHelpers.h b/src/test/jtx/TestHelpers.h index 4aa7c915d44..c21d0771c9d 100644 --- a/src/test/jtx/TestHelpers.h +++ b/src/test/jtx/TestHelpers.h @@ -680,11 +680,9 @@ checkMetrics( namespace loanBroker { Json::Value -set(AccountID const& account, uint256 const& vaultId, std::uint32_t flags = 0); - -// Overload for modifying an existing LoanBroker (use with loanBrokerID()) -Json::Value -set(AccountID const& account, std::uint32_t flags = 0); +set(AccountID const& account, + std::optional const& vaultId = std::nullopt, + std::uint32_t flags = 0); // Use "del" because "delete" is a reserved word in C++. Json::Value diff --git a/src/test/jtx/impl/TestHelpers.cpp b/src/test/jtx/impl/TestHelpers.cpp index 9091f33c676..775e1bebe43 100644 --- a/src/test/jtx/impl/TestHelpers.cpp +++ b/src/test/jtx/impl/TestHelpers.cpp @@ -334,22 +334,13 @@ allPathElements(AccountID const& a, Issue const& iss) namespace loanBroker { Json::Value -set(AccountID const& account, uint256 const& vaultId, uint32_t flags) -{ - Json::Value jv; - jv[sfTransactionType] = jss::LoanBrokerSet; - jv[sfAccount] = to_string(account); - jv[sfVaultID] = to_string(vaultId); - jv[sfFlags] = flags; - return jv; -} - -Json::Value -set(AccountID const& account, uint32_t flags) +set(AccountID const& account, std::optional const& vaultId, uint32_t flags) { Json::Value jv; jv[sfTransactionType] = jss::LoanBrokerSet; jv[sfAccount] = to_string(account); + if (vaultId) + jv[sfVaultID] = to_string(*vaultId); jv[sfFlags] = flags; return jv; } From 5c414eb396c8708f38c778bf2b91b9c5fdd72f3a Mon Sep 17 00:00:00 2001 From: Vito <5780819+Tapanito@users.noreply.github.com> Date: Tue, 31 Mar 2026 14:39:16 +0200 Subject: [PATCH 6/7] fix: add [[nodiscard]] to preclaim helpers and cover pre-amendment path Mark readVault, preclaimUpdate, and preclaimCreate with [[nodiscard]] to match project convention for error-bearing return types. Add test for the pre-amendment preclaimUpdate path where the vault doesn't exist (readVault returning tecNO_ENTRY). --- src/libxrpl/tx/transactors/lending/LoanBrokerSet.cpp | 10 +++++++--- src/test/app/LoanBroker_test.cpp | 11 +++++++++++ 2 files changed, 18 insertions(+), 3 deletions(-) diff --git a/src/libxrpl/tx/transactors/lending/LoanBrokerSet.cpp b/src/libxrpl/tx/transactors/lending/LoanBrokerSet.cpp index e291d3c166a..7355e2c20f5 100644 --- a/src/libxrpl/tx/transactors/lending/LoanBrokerSet.cpp +++ b/src/libxrpl/tx/transactors/lending/LoanBrokerSet.cpp @@ -7,6 +7,7 @@ #include namespace xrpl { + bool LoanBrokerSet::checkExtraFeatures(PreflightContext const& ctx) { @@ -97,7 +98,7 @@ LoanBrokerSet::getValueFields() * @param id The vault ID to look up. * @return The vault SLE on success, or a TER error. */ -static Expected, TER> +[[nodiscard]] static Expected, TER> readVault(PreclaimContext const& ctx, AccountID const& account, uint256 const& id) { auto const sle = ctx.view.read(keylet::vault(id)); @@ -121,7 +122,7 @@ readVault(PreclaimContext const& ctx, AccountID const& account, uint256 const& i * @param brokerID The LoanBroker ID to update. * @return The vault SLE on success, or a TER error. */ -static Expected, TER> +[[nodiscard]] static Expected, TER> preclaimUpdate(PreclaimContext const& ctx, AccountID const& account, uint256 const& brokerID) { auto const& tx = ctx.tx; @@ -197,9 +198,12 @@ preclaimUpdate(PreclaimContext const& ctx, AccountID const& account, uint256 con * @param account The transaction submitter (vault owner). * @return The vault SLE on success, or a TER error. */ -static Expected, TER> +[[nodiscard]] static Expected, TER> preclaimCreate(PreclaimContext const& ctx, AccountID const& account) { + XRPL_ASSERT( + ctx.tx.isFieldPresent(sfVaultID), + "xrpl::LoanBrokerSet::preclaimCreate : VaultID is present in the transaction"); auto const vault = readVault(ctx, account, ctx.tx[sfVaultID]); if (!vault) return vault; diff --git a/src/test/app/LoanBroker_test.cpp b/src/test/app/LoanBroker_test.cpp index f05ab1af094..75ff7fb03b9 100644 --- a/src/test/app/LoanBroker_test.cpp +++ b/src/test/app/LoanBroker_test.cpp @@ -1907,6 +1907,17 @@ class LoanBroker_test : public beast::unit_test::suite env(set(alice, vaultKL2.key), loanBrokerID(brokerKL.key), ter(tecNO_PERMISSION)); } + // Pre-amendment: non-existent vault on update → tecNO_ENTRY + { + testcase("LoanBrokerSet pre-amendment: non-existent vault on update"); + Env env(*this); + env.disableFeature(featureLendingProtocolV1_1); + auto const [vaultID, brokerKL] = setup(env); + + // Update with a VaultID that doesn't exist + env(set(alice, uint256{1}), loanBrokerID(brokerKL.key), ter(tecNO_ENTRY)); + } + // Pre-amendment: Create without VaultID → temINVALID { testcase("LoanBrokerSet pre-amendment: create requires VaultID"); From b921570a0fb7ae86014fffafa5a4497ca0146c66 Mon Sep 17 00:00:00 2001 From: Vito <5780819+Tapanito@users.noreply.github.com> Date: Mon, 8 Jun 2026 13:58:18 +0200 Subject: [PATCH 7/7] fix: post-merge --- .../tx/transactors/lending/LoanBrokerSet.cpp | 4 ++ src/test/app/LoanBroker_test.cpp | 37 +++++++++---------- 2 files changed, 22 insertions(+), 19 deletions(-) diff --git a/src/libxrpl/tx/transactors/lending/LoanBrokerSet.cpp b/src/libxrpl/tx/transactors/lending/LoanBrokerSet.cpp index 9df269d316e..7412ac324e1 100644 --- a/src/libxrpl/tx/transactors/lending/LoanBrokerSet.cpp +++ b/src/libxrpl/tx/transactors/lending/LoanBrokerSet.cpp @@ -1,13 +1,17 @@ #include +#include #include #include +#include #include +#include #include #include #include #include #include +#include #include #include #include diff --git a/src/test/app/LoanBroker_test.cpp b/src/test/app/LoanBroker_test.cpp index 9ccf2874cbb..e6754b6c64b 100644 --- a/src/test/app/LoanBroker_test.cpp +++ b/src/test/app/LoanBroker_test.cpp @@ -2232,7 +2232,7 @@ class LoanBroker_test : public beast::unit_test::Suite // Helper to set up a vault and broker for testing auto const setup = [&](Env& env) { - Vault vault{env}; + Vault const vault{env}; env.fund(XRP(100'000), issuer, alice, evan); env.close(); @@ -2258,7 +2258,7 @@ class LoanBroker_test : public beast::unit_test::Suite uint256 vaultID; Keylet brokerKeylet; }; - return Result{vaultKeylet.key, brokerKeylet}; + return Result{.vaultID = vaultKeylet.key, .brokerKeylet = brokerKeylet}; }; // Post-amendment: VaultID must not be present on update @@ -2269,10 +2269,10 @@ class LoanBroker_test : public beast::unit_test::Suite auto const [vaultID, brokerKL] = setup(env); // Update with VaultID → temINVALID - env(set(alice, vaultID), loanBrokerID(brokerKL.key), ter(temINVALID)); + env(set(alice, vaultID), kLoanBrokerId(brokerKL.key), Ter(temINVALID)); // Update without VaultID succeeds - env(set(alice), loanBrokerID(brokerKL.key), data("post-amendment update")); + env(set(alice), kLoanBrokerId(brokerKL.key), kData("post-amendment update")); env.close(); auto const broker = env.le(brokerKL); @@ -2290,10 +2290,10 @@ class LoanBroker_test : public beast::unit_test::Suite env.close(); // Create without VaultID → temINVALID - env(set(alice), ter(temINVALID)); + env(set(alice), Ter(temINVALID)); // Create with zero VaultID → temINVALID - env(set(alice, uint256{}), ter(temINVALID)); + env(set(alice, uint256{}), Ter(temINVALID)); } // Post-amendment: Update by wrong owner → tecNO_PERMISSION @@ -2302,7 +2302,7 @@ class LoanBroker_test : public beast::unit_test::Suite Env env(*this); auto const [vaultID, brokerKL] = setup(env); - env(set(evan), loanBrokerID(brokerKL.key), ter(tecNO_PERMISSION)); + env(set(evan), kLoanBrokerId(brokerKL.key), Ter(tecNO_PERMISSION)); } // Post-amendment: Update non-existent broker → tecNO_ENTRY @@ -2312,7 +2312,7 @@ class LoanBroker_test : public beast::unit_test::Suite env.fund(XRP(100'000), alice); env.close(); - env(set(alice), loanBrokerID(uint256{1}), ter(tecNO_ENTRY)); + env(set(alice), kLoanBrokerId(uint256{1}), Ter(tecNO_ENTRY)); } // Pre-amendment: VaultID required on both create and update @@ -2324,10 +2324,10 @@ class LoanBroker_test : public beast::unit_test::Suite auto const [vaultID, brokerKL] = setup(env); // Update without VaultID → temINVALID (pre-amendment requires it) - env(set(alice), loanBrokerID(brokerKL.key), ter(temINVALID)); + env(set(alice), kLoanBrokerId(brokerKL.key), Ter(temINVALID)); // Update with matching VaultID succeeds (old behavior) - env(set(alice, vaultID), loanBrokerID(brokerKL.key), data("pre-amendment update")); + env(set(alice, vaultID), kLoanBrokerId(brokerKL.key), kData("pre-amendment update")); env.close(); auto const broker = env.le(brokerKL); @@ -2342,7 +2342,7 @@ class LoanBroker_test : public beast::unit_test::Suite Env env(*this); env.disableFeature(featureLendingProtocolV1_1); - Vault vault{env}; + Vault const vault{env}; env.fund(XRP(100'000), issuer, alice); env.close(); env(trust(alice, issuer["IOU"](1'000'000))); @@ -2367,7 +2367,7 @@ class LoanBroker_test : public beast::unit_test::Suite env.close(); // Update with different vault → tecNO_PERMISSION - env(set(alice, vaultKL2.key), loanBrokerID(brokerKL.key), ter(tecNO_PERMISSION)); + env(set(alice, vaultKL2.key), kLoanBrokerId(brokerKL.key), Ter(tecNO_PERMISSION)); } // Pre-amendment: non-existent vault on update → tecNO_ENTRY @@ -2378,7 +2378,7 @@ class LoanBroker_test : public beast::unit_test::Suite auto const [vaultID, brokerKL] = setup(env); // Update with a VaultID that doesn't exist - env(set(alice, uint256{1}), loanBrokerID(brokerKL.key), ter(tecNO_ENTRY)); + env(set(alice, uint256{1}), kLoanBrokerId(brokerKL.key), Ter(tecNO_ENTRY)); } // Pre-amendment: Create without VaultID → temINVALID @@ -2389,7 +2389,7 @@ class LoanBroker_test : public beast::unit_test::Suite env.fund(XRP(100'000), issuer, alice); env.close(); - env(set(alice), ter(temINVALID)); + env(set(alice), Ter(temINVALID)); } // Pre-amendment: immutable fields still rejected on update @@ -2400,9 +2400,9 @@ class LoanBroker_test : public beast::unit_test::Suite auto const [vaultID, brokerKL] = setup(env); env(set(alice, vaultID), - loanBrokerID(brokerKL.key), - managementFeeRate(TenthBips16(1)), - ter(temINVALID)); + kLoanBrokerId(brokerKL.key), + kManagementFeeRate(TenthBips16(1)), + Ter(temINVALID)); } // Pre-amendment: zero VaultID on update → temINVALID @@ -2412,7 +2412,7 @@ class LoanBroker_test : public beast::unit_test::Suite env.disableFeature(featureLendingProtocolV1_1); auto const [vaultID, brokerKL] = setup(env); - env(set(alice, uint256{}), loanBrokerID(brokerKL.key), ter(temINVALID)); + env(set(alice, uint256{}), kLoanBrokerId(brokerKL.key), Ter(temINVALID)); } } @@ -2421,7 +2421,6 @@ class LoanBroker_test : public beast::unit_test::Suite run() override { testLoanBrokerSetVaultIDAmendment(); - testFeatureLendingProtocolV1_1enabled(); testCoverPrecisionGuard(); testLoanBrokerSetDebtMaximum();