fix: Decide LoanPay fee forgiveness from stored balances

The terminal fee forgiveness read the payee's spendable balance. That is a
placeholder maximum for an IOU issuer, so any fee to an issuer-owned broker
was forgiven, and it adds the opposite trust limit, so a collectible small
fee could look like it rounds away. It also ignored the borrower's side.

Decide from the stored trust line balance of each holder side, skip the
issuer side, and forgive the fee if either side would round away. Credit
the cover only after that decision. The conservation checks use stored
balances for holders too, and accept an issuer payee whose spendable
balance does not move.

src/libxrpl/tx/transactors/lending/LoanPay.cpp

@@ -46,14 +46,12 @@ isPaymentAmountInvalid(STAmount const& amount)
     return amount <= beast::kZero;
 }
 
-// Returns the account's true, unclamped balance in `asset`, for use only in
-// fund-conservation checks. accountHolds(..., SpendableHandling::FullBalance)
-// cannot be used for this: for XRP it always defers to xrpLiquid, which
-// subtracts the account's reserve, so a payee sitting below its own reserve
-// would appear to receive nothing even though its raw ledger balance grew.
-// That mismatch is exactly what a conservation check must not see.
+// Returns the balance stored on the ledger for a holder of `asset`: the raw
+// XRP balance (not xrpLiquid, which subtracts the reserve), or the signed
+// trust line or MPToken balance with no trust limit added. Not meaningful for
+// the issuer, which holds no balance of its own.
 STAmount
-conservationBalance(ReadView const& view, AccountID const& id, Asset const& asset, beast::Journal j)
+storedBalance(ReadView const& view, AccountID const& id, Asset const& asset, beast::Journal j)
 {
     if (isXRP(asset))
     {
@@ -62,6 +60,25 @@ conservationBalance(ReadView const& view, AccountID const& id, Asset const& asse
             return STAmount{asset};  // LCOV_EXCL_LINE
         return view.balanceHookIOU(id, xrpAccount(), sle->getFieldAmount(sfBalance));
     }
+    return accountHolds(
+        view,
+        id,
+        asset,
+        FreezeHandling::IgnoreFreeze,
+        AuthHandling::IgnoreAuth,
+        j,
+        SpendableHandling::SimpleBalance);
+}
+
+// Returns the account's balance in `asset`, for use only in fund-conservation
+// checks. A holder's is its stored balance: a spendable balance would add the
+// opposite trust limit, which can hide a small credit in rounding. An issuer
+// has no stored balance, so its spendable balance is used instead.
+STAmount
+conservationBalance(ReadView const& view, AccountID const& id, Asset const& asset, beast::Journal j)
+{
+    if (id != asset.getIssuer())
+        return storedBalance(view, id, asset, j);
     return accountHolds(
         view,
         id,
@@ -72,18 +89,25 @@ conservationBalance(ReadView const& view, AccountID const& id, Asset const& asse
         SpendableHandling::FullBalance);
 }
 
-// Returns true if crediting `amount` to `id` would change its balance. An IOU
-// balance keeps 16 significant digits, so a credit below its last digit
-// rounds away.
+// Returns true if sending `amount` from `from` to `to` changes every holder
+// balance it touches. An IOU balance keeps 16 significant digits, so an amount
+// below a balance's last digit rounds away. The issuer holds no balance, so
+// its side always takes effect.
 bool
-creditChangesBalance(
+transferChangesBalances(
     ReadView const& view,
-    AccountID const& id,
+    AccountID const& from,
+    AccountID const& to,
     STAmount const& amount,
     beast::Journal j)
 {
-    auto const before = conservationBalance(view, id, amount.asset(), j);
-    return before + amount != before;
+    auto const changes = [&](AccountID const& id, STAmount const& delta) {
+        if (id == amount.asset().getIssuer())
+            return true;
+        auto const before = storedBalance(view, id, amount.asset(), j);
+        return before + delta != before;
+    };
+    return changes(from, -amount) && changes(to, amount);
 }
 }  // namespace
 
@@ -581,16 +605,6 @@ LoanPay::doApply()
         "xrpl::LoanPay::doApply",
         "amount is sufficient");
 
-    if (!sendBrokerFeeToOwner)
-    {
-        // If there is not enough first-loss capital, add the fee to First Loss
-        // Cover Pool. FixedPrecision rounds the redirected fee at the
-        // posterior cover scale; any sub-unit remainder is forgiven. The
-        // broker can Withdraw the credited amount later or leave it for future
-        // needs.
-        brokerSle->at(sfCoverAvailable) += totalPaidToBroker;
-    }
-
     associateAsset(*loanSle, asset);
     associateAsset(*brokerSle, asset);
     associateAsset(*vaultSle, asset);
@@ -617,15 +631,13 @@ LoanPay::doApply()
 
             // Nothing moves for this terminal close when the sub-unit remainder is forgiven and
             // either no fee is owed, or the fee goes to the broker owner who is also the borrower
-            // (the owner would pay itself), or the fee would round away on the owner's balance
-            // (it is forgiven like the Vault remainder). Return before the transfer and its
-            // conservation checks, since they assume some transfer happens.
+            // (the owner would pay itself), or the fee would round away on the borrower's or the
+            // payee's balance (it is forgiven like the Vault remainder). Return before the
+            // transfer and its conservation checks, since they assume some transfer happens.
             if (totalPaidToVaultRounded == beast::kZero &&
-                (totalPaidToBroker == beast::kZero ||
-                 (sendBrokerFeeToOwner &&
-                  (brokerPayee == accountID_ ||
-                   !creditChangesBalance(
-                       view, brokerPayee, STAmount{asset, totalPaidToBroker}, j_)))))
+                (totalPaidToBroker == beast::kZero || brokerPayee == accountID_ ||
+                 !transferChangesBalances(
+                     view, accountID_, brokerPayee, STAmount{asset, totalPaidToBroker}, j_)))
             {
                 return tesSUCCESS;
             }
@@ -684,6 +696,16 @@ LoanPay::doApply()
         // LCOV_EXCL_STOP
     }
 
+    if (!sendBrokerFeeToOwner)
+    {
+        // If there is not enough first-loss capital, add the fee to First Loss
+        // Cover Pool. FixedPrecision rounds the redirected fee at the
+        // posterior cover scale; any sub-unit remainder is forgiven. The
+        // broker can Withdraw the credited amount later or leave it for future
+        // needs.
+        brokerSle->at(sfCoverAvailable) += totalPaidToBroker;
+    }
+
     // These three values are used to check that funds are conserved after the transfers
     auto const accountBalanceBefore = conservationBalance(view, accountID_, asset, j_);
     auto const vaultBalanceBefore = accountID_ == vaultPseudoAccount
@@ -874,7 +896,9 @@ LoanPay::doApply()
         "xrpl::LoanPay::doApply",
         "broker balance did not decrease");
     XRPL_ASSERT_PARTS(
-        vaultBalanceAfter > vaultBalanceBefore || brokerBalanceAfter > brokerBalanceBefore,
+        // An IOU issuer's spendable balance does not change when it is paid.
+        vaultBalanceAfter > vaultBalanceBefore || brokerBalanceAfter > brokerBalanceBefore ||
+            brokerPayee == asset.getIssuer(),
         "xrpl::LoanPay::doApply",
         "vault and/or broker balance increased");
 

src/test/app/lending/LoanPayFixedPrecision_test.cpp

@@ -32,6 +32,7 @@
 #include <chrono>
 #include <cstdint>
 #include <string>
+#include <utility>
 
 namespace xrpl {
 
@@ -642,35 +643,62 @@ class LoanPayFixedPrecision_test : public LoanPayFixedPrecisionBase
 
         // The dust principal floors to a zero vault credit, but the flat service
         // fee is non-zero and goes to the broker owner. It is also the terminal
-        // payment. When the owner is also the borrower, the fee is the owner
-        // paying itself, so nothing moves at all. When the owner's balance is so
-        // large that the fee would round away on its trust line, the fee is
-        // forgiven and nothing moves either.
+        // payment. The fee moves unless the owner is also the borrower (the owner
+        // would pay itself) or it would round away on the borrower's or the
+        // owner's trust line, in which case it is forgiven and nothing moves.
         struct Row
         {
             char const* name = nullptr;
             bool ownerIsBorrower = false;
+            bool ownerIsIssuer = false;
             Number fee{1};
-            bool feeRoundsAway = false;
+            // Extra USD paid to the owner or the borrower before the payment.
+            Number ownerFunds{0};
+            Number borrowerFunds{0};
+            // The issuer's own limit on the owner's trust line.
+            Number issuerLimitOnOwner{0};
+            bool feeMoves = true;
         };
+        Number const smallFee{1, -10};
+        // At 1e7 a 16-digit balance cannot hold a 1e-10 change.
+        Number const largeFunds{10'000'000};
         Row const rows[] = {
             {
                 .name = "Lending: terminal close with a zero vault credit but a non-zero fee needs "
                         "no exemption",
-                .ownerIsBorrower = false,
             },
             {
                 .name =
                     "Lending: terminal close with a zero vault credit and a fee the broker owner "
                     "pays to itself moves nothing",
                 .ownerIsBorrower = true,
+                .ownerFunds = Number{10},
+                .feeMoves = false,
             },
             {
                 .name = "Lending: terminal close with a zero vault credit and a fee that rounds "
                         "away on the broker owner's trust line moves nothing",
-                .ownerIsBorrower = false,
-                .fee = Number{1, -10},
-                .feeRoundsAway = true,
+                .fee = smallFee,
+                .ownerFunds = largeFunds,
+                .feeMoves = false,
+            },
+            {
+                .name = "Lending: terminal close with a zero vault credit and a fee that rounds "
+                        "away on the borrower's trust line moves nothing",
+                .fee = smallFee,
+                .borrowerFunds = largeFunds,
+                .feeMoves = false,
+            },
+            {
+                .name = "Lending: terminal close with a zero vault credit pays a small fee to an "
+                        "owner whose trust line has a large issuer limit",
+                .fee = smallFee,
+                .issuerLimitOnOwner = largeFunds,
+            },
+            {
+                .name = "Lending: terminal close with a zero vault credit pays the fee to an "
+                        "issuer that owns the broker",
+                .ownerIsIssuer = true,
             },
         };
         for (auto const& row : rows)
@@ -679,7 +707,8 @@ class LoanPayFixedPrecision_test : public LoanPayFixedPrecisionBase
 
             Env env(*this, features());
             auto const accounts = setupCoarsenIou(env);
-            auto const& [issuer, owner, depositor, asset] = accounts;
+            auto const& [issuer, holderOwner, depositor, asset] = accounts;
+            Account const& owner = row.ownerIsIssuer ? issuer : holderOwner;
             Account const dustBorrower{"dustBorrower"};
             fundIouHolder(env, dustBorrower, accounts, kDustBorrowerTrustLimit, kDustBorrowerFunds);
             Account const& borrower = row.ownerIsBorrower ? owner : dustBorrower;
@@ -711,16 +740,19 @@ class LoanPayFixedPrecision_test : public LoanPayFixedPrecisionBase
                 return;
             BEAST_EXPECT(getVaultScale(vaultSleBefore) > getVaultBaseScale(vaultSleBefore));
 
-            if (row.ownerIsBorrower)
+            for (auto const& [holder, funds] :
+                 {std::pair{&owner, row.ownerFunds}, std::pair{&borrower, row.borrowerFunds}})
             {
-                env(test::jtx::pay(issuer, owner, asset(Number{10})));
+                if (funds == beast::kZero)
+                    continue;
+                // Keep the default 1,000 limit's headroom.
+                env(trust(*holder, asset(funds + Number{1'000})));
+                env(test::jtx::pay(issuer, *holder, asset(funds)));
                 env.close();
             }
-            if (row.feeRoundsAway)
+            if (row.issuerLimitOnOwner != beast::kZero)
             {
-                // At 1e7 the owner's 16-digit balance cannot hold a 1e-10 credit.
-                env(trust(owner, asset(Number{20'000'000})));
-                env(test::jtx::pay(issuer, owner, asset(Number{10'000'000})));
+                env(trust(issuer, PrettyAsset{owner["USD"]}(row.issuerLimitOnOwner)));
                 env.close();
             }
 
@@ -747,15 +779,15 @@ class LoanPayFixedPrecision_test : public LoanPayFixedPrecisionBase
             Number const ownerBalanceAfter = env.balance(owner, asset).value();
             Number const borrowerBalanceAfter = env.balance(borrower, asset).value();
 
-            // No vault credit. A separate borrower pays the owner the fee; an
-            // owner-borrower pays nothing net, and a fee that rounds away is
-            // forgiven.
-            bool const feeMoves = !row.ownerIsBorrower && !row.feeRoundsAway;
+            // No vault credit. The borrower pays the owner the fee unless it is
+            // forgiven. The issuer holds no balance of its own to check.
+            Number const feeMoved = row.feeMoves ? fee : Number{0};
             BEAST_EXPECT(after.available == before.available);
             BEAST_EXPECT(after.vaultBalance == before.vaultBalance);
-            BEAST_EXPECT(ownerBalanceAfter - ownerBalanceBefore == (feeMoves ? fee : Number{0}));
-            if (row.feeRoundsAway)
-                BEAST_EXPECT(borrowerBalanceAfter == borrowerBalanceBefore);
+            if (!row.ownerIsBorrower)
+                BEAST_EXPECT(borrowerBalanceBefore - borrowerBalanceAfter == feeMoved);
+            if (!row.ownerIsIssuer)
+                BEAST_EXPECT(ownerBalanceAfter - ownerBalanceBefore == feeMoved);
 
             checkVaultLoanSums(env, coarsened, row.name);
         }