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] 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 ba6ca06266..1347cc63ef 100644 --- a/include/xrpl/protocol_autogen/transactions/LoanBrokerSet.h +++ b/include/xrpl/protocol_autogen/transactions/LoanBrokerSet.h @@ -48,14 +48,29 @@ public: // 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 @@ public: /** * @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 @@ public: /** @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 d27738cdde..4ef0d0aa30 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 62163e34f7..e2213d9fad 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 @@ public: 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 e270332f42..318530b4de 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());