From 71bb08cd78d0718421b68f063ce32ff5bee5f654 Mon Sep 17 00:00:00 2001 From: Ed Hennis Date: Mon, 24 Nov 2025 20:36:57 -0500 Subject: [PATCH] Review feedback from @Tapanito, @gregtatcam, and @shawnxie999 - Created a common doWithdraw function for VaultWithdraw and LoanBrokerCoverWithdraw. Added verifyDepositPreauth to it, so that both transactions will get the check. - Add a missing null check to LoanBrokerSet, and add log messages to the existing checks. --- include/xrpl/ledger/View.h | 11 ++++ src/libxrpl/ledger/View.cpp | 49 +++++++++++++++++ .../app/tx/detail/LoanBrokerCoverWithdraw.cpp | 44 ++++------------ src/xrpld/app/tx/detail/LoanBrokerSet.cpp | 24 ++++++++- src/xrpld/app/tx/detail/VaultWithdraw.cpp | 52 ++++--------------- 5 files changed, 100 insertions(+), 80 deletions(-) diff --git a/include/xrpl/ledger/View.h b/include/xrpl/ledger/View.h index 3bad22c321..fc16d53e61 100644 --- a/include/xrpl/ledger/View.h +++ b/include/xrpl/ledger/View.h @@ -753,6 +753,17 @@ canWithdraw( [[nodiscard]] TER canWithdraw(ReadView const& view, STTx const& tx); +[[nodiscard]] TER +doWithdraw( + ApplyView& view, + STTx const& tx, + AccountID const& senderAcct, + AccountID const& dstAcct, + AccountID const& sourceAcct, + XRPAmount priorBalance, + STAmount const& amount, + beast::Journal j); + /// Any transactors that call addEmptyHolding() in doApply must call /// canAddHolding() in preflight with the same View and Asset [[nodiscard]] TER diff --git a/src/libxrpl/ledger/View.cpp b/src/libxrpl/ledger/View.cpp index b3625bc596..a91a0c6468 100644 --- a/src/libxrpl/ledger/View.cpp +++ b/src/libxrpl/ledger/View.cpp @@ -1386,6 +1386,55 @@ canWithdraw(ReadView const& view, STTx const& tx) return canWithdraw(from, view, to, tx.isFieldPresent(sfDestinationTag)); } +TER +doWithdraw( + ApplyView& view, + STTx const& tx, + AccountID const& senderAcct, + AccountID const& dstAcct, + AccountID const& sourceAcct, + XRPAmount priorBalance, + STAmount const& amount, + beast::Journal j) +{ + // Create trust line or MPToken for the receiving account + if (dstAcct == senderAcct) + { + if (auto const ter = addEmptyHolding( + view, senderAcct, priorBalance, amount.asset(), j); + !isTesSuccess(ter) && ter != tecDUPLICATE) + return ter; + } + else + { + auto dstSle = view.peek(keylet::account(dstAcct)); + if (auto err = + verifyDepositPreauth(tx, view, senderAcct, dstAcct, dstSle, j)) + return err; + } + + // Sanity check + if (accountHolds( + view, + sourceAcct, + amount.asset(), + FreezeHandling::fhIGNORE_FREEZE, + AuthHandling::ahIGNORE_AUTH, + j) < amount) + { + // LCOV_EXCL_START + JLOG(j.error()) << "LoanBrokerCoverWithdraw: negative balance of " + "broker cover assets."; + return tefINTERNAL; + // LCOV_EXCL_STOP + } + + // Move the funds directly from the broker's pseudo-account to the + // dstAcct + return accountSend( + view, sourceAcct, dstAcct, amount, j, WaiveTransferFee::Yes); +} + [[nodiscard]] TER addEmptyHolding( ApplyView& view, diff --git a/src/xrpld/app/tx/detail/LoanBrokerCoverWithdraw.cpp b/src/xrpld/app/tx/detail/LoanBrokerCoverWithdraw.cpp index 2b54e28295..1fd5a1a471 100644 --- a/src/xrpld/app/tx/detail/LoanBrokerCoverWithdraw.cpp +++ b/src/xrpld/app/tx/detail/LoanBrokerCoverWithdraw.cpp @@ -157,41 +157,15 @@ LoanBrokerCoverWithdraw::doApply() broker->at(sfCoverAvailable) -= amount; view().update(broker); - // Create trust line or MPToken for the receiving account - if (dstAcct == account_) - { - if (auto const ter = addEmptyHolding( - view(), account_, mPriorBalance, amount.asset(), j_); - !isTesSuccess(ter) && ter != tecDUPLICATE) - return ter; - } - else - { - auto dstSle = view().peek(keylet::account(dstAcct)); - if (auto err = - verifyDepositPreauth(tx, view(), account_, dstAcct, dstSle, j_)) - return err; - } - - // Sanity check - if (accountHolds( - view(), - brokerPseudoID, - amount.asset(), - FreezeHandling::fhIGNORE_FREEZE, - AuthHandling::ahIGNORE_AUTH, - j_) < amount) - { - // LCOV_EXCL_START - JLOG(j_.error()) << "LoanBrokerCoverWithdraw: negative balance of " - "broker cover assets."; - return tefINTERNAL; - // LCOV_EXCL_STOP - } - - // Move the funds directly from the broker's pseudo-account to the dstAcct - return accountSend( - view(), brokerPseudoID, dstAcct, amount, j_, WaiveTransferFee::Yes); + return doWithdraw( + view(), + tx, + account_, + dstAcct, + brokerPseudoID, + mPriorBalance, + amount, + j_); } //------------------------------------------------------------------------------ diff --git a/src/xrpld/app/tx/detail/LoanBrokerSet.cpp b/src/xrpld/app/tx/detail/LoanBrokerSet.cpp index beb8bf141b..c2e6effd7a 100644 --- a/src/xrpld/app/tx/detail/LoanBrokerSet.cpp +++ b/src/xrpld/app/tx/detail/LoanBrokerSet.cpp @@ -120,7 +120,13 @@ LoanBrokerSet::doApply() // Modify an existing LoanBroker auto broker = view.peek(keylet::loanbroker(*brokerID)); if (!broker) - return tefBAD_LEDGER; // LCOV_EXCL_LINE + { + // This should be impossible + // LCOV_EXCL_START + JLOG(j_.fatal()) << "LoanBroker does not exist."; + return tefBAD_LEDGER; + // LCOV_EXCL_STOP + } if (auto const data = tx[~sfData]) broker->at(sfData) = *data; @@ -134,12 +140,26 @@ LoanBrokerSet::doApply() // Create a new LoanBroker pointing back to the given Vault auto const vaultID = tx[sfVaultID]; auto const sleVault = view.read(keylet::vault(vaultID)); + if (!sleVault) + { + // This should be impossible + // LCOV_EXCL_START + JLOG(j_.fatal()) << "Vault does not exist."; + return tefBAD_LEDGER; + // LCOV_EXCL_STOP + } auto const vaultPseudoID = sleVault->at(sfAccount); auto const sequence = tx.getSeqValue(); auto owner = view.peek(keylet::account(account_)); if (!owner) - return tefBAD_LEDGER; // LCOV_EXCL_LINE + { + // This should be impossible + // LCOV_EXCL_START + JLOG(j_.fatal()) << "Account does not exist."; + return tefBAD_LEDGER; + // LCOV_EXCL_STOP + } auto broker = std::make_shared(keylet::loanbroker(account_, sequence)); diff --git a/src/xrpld/app/tx/detail/VaultWithdraw.cpp b/src/xrpld/app/tx/detail/VaultWithdraw.cpp index adab7eb3a6..99a5f5f721 100644 --- a/src/xrpld/app/tx/detail/VaultWithdraw.cpp +++ b/src/xrpld/app/tx/detail/VaultWithdraw.cpp @@ -237,49 +237,15 @@ VaultWithdraw::doApply() auto const dstAcct = ctx_.tx[~sfDestination].value_or(account_); - // Create trust line or MPToken for the receiving account - if (dstAcct == account_) - { - if (auto const ter = addEmptyHolding( - view(), account_, mPriorBalance, vaultAsset, j_); - !isTesSuccess(ter) && ter != tecDUPLICATE) - return ter; - } - else - { - auto dstSle = view().peek(keylet::account(dstAcct)); - if (auto err = verifyDepositPreauth( - ctx_.tx, view(), account_, dstAcct, dstSle, j_)) - return err; - } - - // Transfer assets from vault to depositor or destination account. - if (auto const ter = accountSend( - view(), - vaultAccount, - dstAcct, - assetsWithdrawn, - j_, - WaiveTransferFee::Yes); - !isTesSuccess(ter)) - return ter; - - // Sanity check - if (accountHolds( - view(), - vaultAccount, - assetsWithdrawn.asset(), - FreezeHandling::fhIGNORE_FREEZE, - AuthHandling::ahIGNORE_AUTH, - j_) < beast::zero) - { - // LCOV_EXCL_START - JLOG(j_.error()) << "VaultWithdraw: negative balance of vault assets."; - return tefINTERNAL; - // LCOV_EXCL_STOP - } - - return tesSUCCESS; + return doWithdraw( + view(), + ctx_.tx, + account_, + dstAcct, + vaultAccount, + mPriorBalance, + assetsWithdrawn, + j_); } } // namespace ripple