Review feedbak from @tequdev, plus helpers

- Fix LoanSet.calculateBaseFee with multisign.
- Cleanups.
- Add View helper functions for Loan and Vault transactions
  - preflightDestinationAndTag
  - checkDestinationAndTag
  - canSendToAccount
- Used the helpers in appropriate Loan and Vault transactions.
  - They could also be used in older transactions, I'll save that for
    later.

include/xrpl/ledger/View.h

@@ -694,7 +694,7 @@ isPseudoAccount(std::shared_ptr<SLE const> sleAcct);
 getPseudoAccountFields();
 
 [[nodiscard]] inline bool
-isPseudoAccount(ReadView const& view, AccountID accountId)
+isPseudoAccount(ReadView const& view, AccountID const& accountId)
 {
     return isPseudoAccount(view.read(keylet::account(accountId)));
 }
@@ -702,6 +702,51 @@ isPseudoAccount(ReadView const& view, AccountID accountId)
 [[nodiscard]] TER
 canAddHolding(ReadView const& view, Asset const& asset);
 
+/** Checks that the destination and destination tag are valid.
+
+   - If destination is set, it must not be zero.
+   - If not set, there must not be a destination tag.
+*/
+[[nodiscard]] NotTEC
+preflightDestinationAndTag(
+    std::optional<AccountID> const& destination,
+    bool hasDestinationTag);
+
+/** Validates that the destination SLE and tag are valid
+
+   - Checks that the SLE is not null.
+   - If the SLE requires a destination tag, checks that there is a tag.
+*/
+[[nodiscard]] TER
+checkDestinationAndTag(SLE::const_ref toSle, bool hasDestinationTag);
+
+/** Checks that from can sent to to (with an SLE)
+
+   - Does the checkDestinationAndTag checks, plus:
+   - if the destination requires deposit auth, checks that the from account has
+     that auth.
+*/
+[[nodiscard]] TER
+canSendToAccount(
+    AccountID const& from,
+    ReadView const& view,
+    SLE::const_ref toSle,
+    bool hasDestinationTag);
+
+/** Checks that from can sent to to (with an AccountID)
+
+   - Loads the SLE for "to"
+   - Checks whether the account is trying to send to itself.
+     Succeeds if the SLE exists, fails if not.
+   - Then does the checkDestinationAndTag check with an SLE.
+*/
+[[nodiscard]] TER
+canSendToAccount(
+    AccountID const& from,
+    ReadView const& view,
+    AccountID const& to,
+    bool hasDestinationTag);
+
 /// 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

@@ -1340,6 +1340,73 @@ canAddHolding(ReadView const& view, Asset const& asset)
         asset.value());
 }
 
+[[nodiscard]] NotTEC
+preflightDestinationAndTag(
+    std::optional<AccountID> const& destination,
+    bool hasDestinationTag)
+{
+    if (destination)
+    {
+        if (*destination == beast::zero)
+        {
+            return temMALFORMED;
+        }
+    }
+    else if (hasDestinationTag)
+    {
+        return temMALFORMED;
+    }
+
+    return tesSUCCESS;
+}
+
+[[nodiscard]] TER
+checkDestinationAndTag(SLE::const_ref toSle, bool hasDestinationTag)
+{
+    if (toSle == nullptr)
+        return tecNO_DST;
+
+    // The tag is basically account-specific information we don't
+    // understand, but we can require someone to fill it in.
+    if (toSle->isFlag(lsfRequireDestTag) && !hasDestinationTag)
+        return tecDST_TAG_NEEDED;  // Cannot send without a tag
+
+    return tesSUCCESS;
+}
+
+[[nodiscard]] TER
+canSendToAccount(
+    AccountID const& from,
+    ReadView const& view,
+    SLE::const_ref toSle,
+    bool hasDestinationTag)
+{
+    if (auto const ret = checkDestinationAndTag(toSle, hasDestinationTag))
+        return ret;
+
+    if (toSle->isFlag(lsfDepositAuth))
+    {
+        if (!view.exists(keylet::depositPreauth(toSle->at(sfAccount), from)))
+            return tecNO_PERMISSION;
+    }
+
+    return tesSUCCESS;
+}
+
+[[nodiscard]] TER
+canSendToAccount(
+    AccountID const& from,
+    ReadView const& view,
+    AccountID const& to,
+    bool hasDestinationTag)
+{
+    auto const toSle = view.read(keylet::account(to));
+    if (from == to)
+        return toSle ? (TER)tesSUCCESS : (TER)tecINTERNAL;
+
+    return canSendToAccount(from, view, toSle, hasDestinationTag);
+}
+
 [[nodiscard]] TER
 addEmptyHolding(
     ApplyView& view,

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

@@ -43,23 +43,9 @@ LoanBrokerCoverWithdraw::preflight(PreflightContext const& ctx)
     if (!isLegalNet(dstAmount))
         return temBAD_AMOUNT;
 
-    if (auto const destination = ctx.tx[~sfDestination];
+    if (auto const ret = preflightDestinationAndTag(
-        destination.has_value())
+            ctx.tx[~sfDestination], ctx.tx.isFieldPresent(sfDestinationTag)))
-    {
+        return ret;
-        if (*destination == beast::zero)
-        {
-            JLOG(ctx.j.debug())
-                << "LoanBrokerCoverWithdraw: zero/empty destination account.";
-            return temMALFORMED;
-        }
-    }
-    else if (ctx.tx.isFieldPresent(sfDestinationTag))
-    {
-        JLOG(ctx.j.debug())
-            << "LoanBrokerCoverWithdraw: sfDestinationTag is set but "
-               "sfDestination is not";
-        return temMALFORMED;
-    }
 
     return tesSUCCESS;
 }
@@ -73,11 +59,7 @@ LoanBrokerCoverWithdraw::preclaim(PreclaimContext const& ctx)
     auto const brokerID = tx[sfLoanBrokerID];
     auto const amount = tx[sfAmount];
 
-    auto const dstAcct = [&]() -> AccountID {
+    auto const dstAcct = tx[~sfDestination].value_or(account);
-        if (auto const dst = tx[~sfDestination])
-            return *dst;
-        return account;
-    }();
 
     auto const sleBroker = ctx.view.read(keylet::loanbroker(brokerID));
     if (!sleBroker)
@@ -103,19 +85,13 @@ LoanBrokerCoverWithdraw::preclaim(PreclaimContext const& ctx)
     AuthType authType = AuthType::Legacy;
     if (account != dstAcct)
     {
-        auto const sleDst = ctx.view.read(keylet::account(dstAcct));
+        if (auto const ret = canSendToAccount(
-        if (sleDst == nullptr)
+                account,
-            return tecNO_DST;
+                ctx.view,
+                dstAcct,
+                tx.isFieldPresent(sfDestinationTag)))
+            return ret;
 
-        if (sleDst->isFlag(lsfRequireDestTag) &&
-            !tx.isFieldPresent(sfDestinationTag))
-            return tecDST_TAG_NEEDED;  // Cannot send without a tag
-
-        if (sleDst->isFlag(lsfDepositAuth))
-        {
-            if (!ctx.view.exists(keylet::depositPreauth(dstAcct, account)))
-                return tecNO_PERMISSION;
-        }
         // The destination account must have consented to receive the asset by
         // creating a RippleState or MPToken
         authType = AuthType::StrongAuth;
@@ -171,11 +147,7 @@ LoanBrokerCoverWithdraw::doApply()
 
     auto const brokerID = tx[sfLoanBrokerID];
     auto const amount = tx[sfAmount];
-    auto const dstAcct = [&]() -> AccountID {
+    auto const dstAcct = tx[~sfDestination].value_or(account_);
-        if (auto const dst = tx[~sfDestination])
-            return *dst;
-        return account_;
-    }();
 
     auto broker = view().peek(keylet::loanbroker(brokerID));
     if (!broker)

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

@@ -172,8 +172,8 @@ LoanSet::calculateBaseFee(ReadView const& view, STTx const& tx)
     // for the transaction. Note that unlike the base class, the single signer
     // is counted if present. It will only be absent in a batch inner
     // transaction.
-    std::size_t const signerCount = tx.isFieldPresent(sfSigners)
+    std::size_t const signerCount = counterSig.isFieldPresent(sfSigners)
-        ? tx.getFieldArray(sfSigners).size()
+        ? counterSig.getFieldArray(sfSigners).size()
         : (counterSig.isFieldPresent(sfTxnSignature) ? 1 : 0);
 
     return normalCost + (signerCount * baseFee);
@@ -513,7 +513,7 @@ LoanSet::doApply()
                 brokerOwnerSle->at(sfBalance).value().xrp(),
                 vaultAsset,
                 j_);
-            !isTesSuccess(ter) && ter != tecDUPLICATE)
+            ter && ter != tecDUPLICATE)
             // ignore tecDUPLICATE. That means the holding already exists,
             // and is fine here
             return ter;

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

@@ -42,16 +42,9 @@ VaultWithdraw::preflight(PreflightContext const& ctx)
     if (ctx.tx[sfAmount] <= beast::zero)
         return temBAD_AMOUNT;
 
-    if (auto const destination = ctx.tx[~sfDestination];
+    if (auto const ret = preflightDestinationAndTag(
-        destination.has_value())
+            ctx.tx[~sfDestination], ctx.tx.isFieldPresent(sfDestinationTag)))
-    {
+        return ret;
-        if (*destination == beast::zero)
-        {
-            JLOG(ctx.j.debug())
-                << "VaultWithdraw: zero/empty destination account.";
-            return temMALFORMED;
-        }
-    }
 
     return tesSUCCESS;
 }
@@ -111,21 +104,13 @@ VaultWithdraw::preclaim(PreclaimContext const& ctx)
 
     auto const account = ctx.tx[sfAccount];
     auto const dstAcct = ctx.tx[~sfDestination].value_or(account);
-    auto const sleDst = ctx.view.read(keylet::account(dstAcct));
-    if (sleDst == nullptr)
-        return account == dstAcct ? tecINTERNAL : tecNO_DST;
 
-    if (sleDst->isFlag(lsfRequireDestTag) &&
+    if (auto const ret = canSendToAccount(
-        !ctx.tx.isFieldPresent(sfDestinationTag))
+            account,
-        return tecDST_TAG_NEEDED;  // Cannot send without a tag
+            ctx.view,
-
+            dstAcct,
-    // Withdrawal to a 3rd party destination account is essentially a transfer,
+            ctx.tx.isFieldPresent(sfDestinationTag)))
-    // via shares in the vault. Enforce all the usual asset transfer checks.
+        return ret;
-    if (account != dstAcct && sleDst->isFlag(lsfDepositAuth))
-    {
-        if (!ctx.view.exists(keylet::depositPreauth(dstAcct, account)))
-            return tecNO_PERMISSION;
-    }
 
     // If sending to Account (i.e. not a transfer), we will also create (only
     // if authorized) a trust line or MPToken as needed, in doApply().