From 42eadf04167c0fd70087bb4edc4f0432a1c900ca Mon Sep 17 00:00:00 2001 From: Vito <5780819+Tapanito@users.noreply.github.com> Date: Thu, 14 May 2026 15:04:37 +0200 Subject: [PATCH] review: Address code review feedback on cover precision guard - Capitalize guard helper comments in Deposit and Withdraw transactors - Fix canApplyToBrokerCover signature to use SLE::const_ref (matches header) - Add alice balance invariant check to Withdraw no-op test case - Add MPT positive test: verify 1-unit MPT passes the precision guard --- include/xrpl/ledger/helpers/LendingHelpers.h | 1 + src/libxrpl/ledger/helpers/LendingHelpers.cpp | 2 +- .../lending/LoanBrokerCoverDeposit.cpp | 2 +- .../lending/LoanBrokerCoverWithdraw.cpp | 2 +- src/test/app/LoanBroker_test.cpp | 47 +++++++++++++++++++ 5 files changed, 51 insertions(+), 3 deletions(-) diff --git a/include/xrpl/ledger/helpers/LendingHelpers.h b/include/xrpl/ledger/helpers/LendingHelpers.h index 6196b90745..acdcaf0809 100644 --- a/include/xrpl/ledger/helpers/LendingHelpers.h +++ b/include/xrpl/ledger/helpers/LendingHelpers.h @@ -5,6 +5,7 @@ #include #include + namespace xrpl { /** diff --git a/src/libxrpl/ledger/helpers/LendingHelpers.cpp b/src/libxrpl/ledger/helpers/LendingHelpers.cpp index 2e50c935b6..8343dc69f8 100644 --- a/src/libxrpl/ledger/helpers/LendingHelpers.cpp +++ b/src/libxrpl/ledger/helpers/LendingHelpers.cpp @@ -31,7 +31,7 @@ namespace xrpl { [[nodiscard]] TER canApplyToBrokerCover( ReadView const& view, - std::shared_ptr const& sleBroker, + SLE::const_ref sleBroker, Asset const& vaultAsset, STAmount const& amount, beast::Journal j, diff --git a/src/libxrpl/tx/transactors/lending/LoanBrokerCoverDeposit.cpp b/src/libxrpl/tx/transactors/lending/LoanBrokerCoverDeposit.cpp index 7e93675e22..010830dacb 100644 --- a/src/libxrpl/tx/transactors/lending/LoanBrokerCoverDeposit.cpp +++ b/src/libxrpl/tx/transactors/lending/LoanBrokerCoverDeposit.cpp @@ -73,7 +73,7 @@ LoanBrokerCoverDeposit::preclaim(PreclaimContext const& ctx) if (amount.asset() != vaultAsset) return tecWRONG_ASSET; - // helper handles both IOU and MPT correctly without explicit branching. + // Helper handles both IOU and MPT correctly without explicit branching. if (auto const ret = canApplyToBrokerCover( ctx.view, sleBroker, vaultAsset, amount, ctx.j, "LoanBrokerCoverDeposit")) return ret; diff --git a/src/libxrpl/tx/transactors/lending/LoanBrokerCoverWithdraw.cpp b/src/libxrpl/tx/transactors/lending/LoanBrokerCoverWithdraw.cpp index 059e865ebf..dd02f573b2 100644 --- a/src/libxrpl/tx/transactors/lending/LoanBrokerCoverWithdraw.cpp +++ b/src/libxrpl/tx/transactors/lending/LoanBrokerCoverWithdraw.cpp @@ -93,7 +93,7 @@ LoanBrokerCoverWithdraw::preclaim(PreclaimContext const& ctx) if (amount.asset() != vaultAsset) return tecWRONG_ASSET; - // helper handles both IOU and MPT correctly without explicit branching. + // Helper handles both IOU and MPT correctly without explicit branching. if (auto const ret = canApplyToBrokerCover( ctx.view, sleBroker, vaultAsset, amount, ctx.j, "LoanBrokerCoverWithdraw")) return ret; diff --git a/src/test/app/LoanBroker_test.cpp b/src/test/app/LoanBroker_test.cpp index a8c6b72b30..eeb0d6d2e9 100644 --- a/src/test/app/LoanBroker_test.cpp +++ b/src/test/app/LoanBroker_test.cpp @@ -1901,12 +1901,14 @@ class LoanBroker_test : public beast::unit_test::Suite auto const [brokerKeylet, iou] = setup(env); PrettyAmount const subUlpAmt = iou(Number{1, -16}); auto const coverBefore = env.le(brokerKeylet)->at(sfCoverAvailable); + auto const aliceBalanceBefore = env.balance(alice, iou); env(coverWithdraw(alice, brokerKeylet.key, subUlpAmt), Ter(expected)); env.close(); if (expected == tesSUCCESS) { if (auto const broker = env.le(brokerKeylet); BEAST_EXPECT(broker)) BEAST_EXPECT(broker->at(sfCoverAvailable) == coverBefore); + BEAST_EXPECT(env.balance(alice, iou) == aliceBalanceBefore); } } @@ -1931,6 +1933,51 @@ class LoanBroker_test : public beast::unit_test::Suite runTestCases(all_); runTestCases(all_ - fixCleanup3_2_0); + + // MPT amounts are integers; scale is 0; the guard never rejects a + // positive integer amount. Verify all three callsites pass with amendment on. + { + testcase("Cover precision guard: MPT min amount passes"); + Env env{*this, all_}; + + env.fund(XRP(100'000), issuer, alice); + env.close(); + + MPTTester mptt{env, issuer, kMPT_INIT_NO_FUND}; + mptt.create({.flags = tfMPTCanClawback | tfMPTCanTransfer | tfMPTCanLock}); + env.close(); + + PrettyAsset const mptAsset = mptt["MPT"]; + mptt.authorize({.account = alice}); + env.close(); + + env(pay(issuer, alice, mptAsset(100))); + env.close(); + + Vault vault{env}; + auto [createTx, vaultKeylet] = vault.create({.owner = alice, .asset = mptAsset}); + env(createTx); + env.close(); + + auto const brokerKeylet = keylet::loanbroker(alice.id(), env.seq(alice)); + env(set(alice, vaultKeylet.key)); + env.close(); + + env(coverDeposit(alice, brokerKeylet.key, mptAsset(10))); + env.close(); + + env(coverDeposit(alice, brokerKeylet.key, mptAsset(1)), Ter(tesSUCCESS)); + env.close(); + + env(coverWithdraw(alice, brokerKeylet.key, mptAsset(1)), Ter(tesSUCCESS)); + env.close(); + + env(coverClawback(issuer), + kLOAN_BROKER_ID(brokerKeylet.key), + kAMOUNT(mptAsset(1)), + Ter(tesSUCCESS)); + env.close(); + } } public: