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.

Author: ximinez

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

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,

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_);
 }
 
 //------------------------------------------------------------------------------

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<SLE>(keylet::loanbroker(account_, sequence));
 

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