Review feedback from @dangell7 and @gregtatcam

- Mostly adding comments.
- Fixed some function parameter names that weren't updated after
  a copy/paste.
- LoanBrokerCoverWithdraw does not need to check for freeze when sending
  to the issuer.
- Reorder the fund transfer cases in LoanBrokerCoverWithdraw to make it
  clearer that some transfers are direct, and some use the payment
  engine.
- Only look up the vault ID once in LoanBrokerSet

include/xrpl/protocol/Indexes.h

@@ -348,18 +348,18 @@ Keylet
 loanbroker(AccountID const& owner, std::uint32_t seq) noexcept;
 
 inline Keylet
-loanbroker(uint256 const& vaultKey)
+loanbroker(uint256 const& key)
 {
-    return {ltLOAN_BROKER, vaultKey};
+    return {ltLOAN_BROKER, key};
 }
 
 Keylet
 loan(uint256 const& loanBrokerID, std::uint32_t loanSeq) noexcept;
 
 inline Keylet
-loan(uint256 const& vaultKey)
+loan(uint256 const& key)
 {
-    return {ltLOAN, vaultKey};
+    return {ltLOAN, key};
 }
 
 Keylet

src/xrpld/app/tx/detail/LoanBrokerCoverClawback.cpp

@@ -64,6 +64,7 @@ LoanBrokerCoverClawback::preflight(PreflightContext const& ctx)
 
     if (amount)
     {
+        // XRP has no counterparty, and thus nobody can claw it back
         if (amount->native())
             return temBAD_AMOUNT;
 
@@ -81,6 +82,9 @@ LoanBrokerCoverClawback::preflight(PreflightContext const& ctx)
                 return temINVALID;
 
             auto const account = ctx.tx[sfAccount];
+            // Since we don't have a LoanBrokerID, holder _should_ be the loan
+            // broker's pseudo-account, but we don't know yet whether it is, so
+            // use a generic placeholder name.
             auto const holder = amount->getIssuer();
             if (holder == account || holder == beast::zero)
                 return temINVALID;
@@ -100,6 +104,9 @@ determineBrokerID(ReadView const& view, STTx const& tx)
     if (!dstAmount || !dstAmount->holds<Issue>())
         return Unexpected{tecINTERNAL};
 
+    // Since we don't have a LoanBrokerID, holder _should_ be the loan broker's
+    // pseudo-account, but we don't know yet whether it is, so use a generic
+    // placeholder name.
     auto const holder = dstAmount->getIssuer();
     auto const sle = view.read(keylet::account(holder));
     if (!sle)

src/xrpld/app/tx/detail/LoanBrokerCoverWithdraw.cpp

@@ -145,11 +145,12 @@ LoanBrokerCoverWithdraw::preclaim(PreclaimContext const& ctx)
     // The broker's pseudo-account is the source of funds.
     auto const pseudoAccountID = sleBroker->at(sfAccount);
 
-    // Cannot send a frozen Asset
-    if (auto const ret = checkFrozen(ctx.view, pseudoAccountID, vaultAsset))
-        return ret;
+    // Check for freezes, unless sending directly to the issuer
     if (dstAcct != vaultAsset.getIssuer())
     {
+        // Cannot send a frozen Asset
+        if (auto const ret = checkFrozen(ctx.view, pseudoAccountID, vaultAsset))
+            return ret;
         // Destination account cannot receive if asset is deep frozen
         if (auto const ret = checkDeepFrozen(ctx.view, dstAcct, vaultAsset))
             return ret;
@@ -205,8 +206,21 @@ LoanBrokerCoverWithdraw::doApply()
 
     // Move the funds from the broker's pseudo-account to the dstAcct
 
-    if (dstAcct != account_ && !amount.native())
+    if (dstAcct == account_ || amount.native())
     {
+        // Transfer assets directly from pseudo-account to depositor.
+        // Because this is either a self-transfer or an XRP payment, there is no
+        // need to use the payment engine.
+        return accountSend(
+            view(), brokerPseudoID, dstAcct, amount, j_, WaiveTransferFee::Yes);
+    }
+
+    {
+        // If sending the Cover to a different account, then this is
+        // effectively a payment. Use the Payment transaction code to call
+        // the payment engine, though only a subset of the functionality is
+        // supported in this transaction. e.g. No paths, no partial
+        // payments.
         bool const mptDirect = amount.holds<MPTIssue>();
         STAmount const maxSourceAmount =
             Payment::getMaxSourceAmount(brokerPseudoID, amount);
@@ -225,11 +239,6 @@ LoanBrokerCoverWithdraw::doApply()
             .deliverMin = std::nullopt,
             .j = j_};
 
-        // If sending the Cover to a different account, then this is
-        // effectively a payment. Use the Payment transaction code to call
-        // the payment engine, though only a subset of the functionality is
-        // supported in this transaction. e.g. No paths, no partial
-        // payments.
         TER ret;
         if (mptDirect)
         {
@@ -250,10 +259,6 @@ LoanBrokerCoverWithdraw::doApply()
         }
         return ret;
     }
-
-    // Transfer assets from pseudo-account to depositor.
-    return accountSend(
-        view(), brokerPseudoID, dstAcct, amount, j_, WaiveTransferFee::Yes);
 }
 
 //------------------------------------------------------------------------------

src/xrpld/app/tx/detail/LoanBrokerSet.cpp

@@ -87,6 +87,8 @@ LoanBrokerSet::preclaim(PreclaimContext const& ctx)
     auto const& tx = ctx.tx;
 
     auto const account = tx[sfAccount];
+    auto const vaultID = tx[sfVaultID];
+
     if (auto const brokerID = tx[~sfLoanBrokerID])
     {
         auto const sleBroker = ctx.view.read(keylet::loanbroker(*brokerID));
@@ -95,7 +97,7 @@ LoanBrokerSet::preclaim(PreclaimContext const& ctx)
             JLOG(ctx.j.warn()) << "LoanBroker does not exist.";
             return tecNO_ENTRY;
         }
-        if (tx[sfVaultID] != sleBroker->at(sfVaultID))
+        if (vaultID != sleBroker->at(sfVaultID))
         {
             JLOG(ctx.j.warn())
                 << "Can not change VaultID on an existing LoanBroker.";
@@ -109,7 +111,6 @@ LoanBrokerSet::preclaim(PreclaimContext const& ctx)
     }
     else
     {
-        auto const vaultID = tx[sfVaultID];
         auto const sleVault = ctx.view.read(keylet::vault(vaultID));
         if (!sleVault)
         {
@@ -150,7 +151,6 @@ LoanBrokerSet::doApply()
     else
     {
         // Create a new LoanBroker pointing back to the given Vault
-        auto const vaultID = tx[sfVaultID];
         auto const sleVault = view.read(keylet::vault(vaultID));
         auto const vaultPseudoID = sleVault->at(sfAccount);
         auto const sequence = tx.getSeqValue();