Expand LoanPay test cases, and fix rounding errors

- At least rounding errors for a full payoff

include/xrpl/protocol/STAmount.h

@@ -697,12 +697,14 @@ getRate(STAmount const& offerOut, STAmount const& offerIn);
 
 template <AssetType A>
 Number
-roundToAsset(A const& asset, Number const& value)
+roundToAsset(
+    A const& asset,
+    Number const& value,
+    Number::rounding_mode rounding = Number::getround())
 {
+    NumberRoundModeGuard mg(rounding);
     STAmount const amount{asset, value};
-    if (amount != value)
     return amount;
-    return value;
 }
 
 //------------------------------------------------------------------------------

src/test/app/Loan_test.cpp

@@ -1229,7 +1229,7 @@ class Loan_test : public beast::unit_test::suite
                     // toEndOfLife
                     //
                     auto state = currentState(loanKeylet, verifyLoanStatus);
-                    BEAST_EXPECT(state.flags == lsfLoanOverpayment);
+                    BEAST_EXPECT(state.flags == 0);
                     auto const borrowerStartingBalance =
                         env.balance(borrower, broker.asset);
 
@@ -1246,7 +1246,7 @@ class Loan_test : public beast::unit_test::suite
                     PrettyAmount adjustment = broker.asset(0);
                     if (broker.asset.raw().native())
                     {
-                        adjustment = env.current()->fees().base;
+                        adjustment = 2 * env.current()->fees().base;
                     }
 
                     // Draw the entire available balance
@@ -1256,6 +1256,8 @@ class Loan_test : public beast::unit_test::suite
                         broker.asset, state.assetsAvailable};
                     env(draw(borrower, loanKeylet.key, drawAmount));
                     env.close(state.startDate + 20s);
+                    auto const loanAge = (env.now() - state.startDate).count();
+                    BEAST_EXPECT(loanAge == 30);
 
                     state.assetsAvailable -= drawAmount;
                     verifyLoanStatus(state);
@@ -1283,8 +1285,14 @@ class Loan_test : public beast::unit_test::suite
                             ter(tecWRONG_ASSET));
                     }
 
+                    // Amount doesn't cover a single payment
+                    env(pay(borrower,
+                            loanKeylet.key,
+                            STAmount{broker.asset, 1}),
+                        ter(tecINSUFFICIENT_PAYMENT));
+
                     // Get the balance after these failed transactions take fees
-                    auto const borrowerBeforePaymentBalance =
+                    auto const borrowerBalanceBeforePayment =
                         env.balance(borrower, broker.asset);
 
                     // Full payoff amount will consist of
@@ -1294,27 +1302,43 @@ class Loan_test : public beast::unit_test::suite
                     // 4. close payment fee (4)
                     // Calculate these values without the helper functions to
                     // verify they're working correctly
+                    // The numbers in the below BEAST_EXPECTs may not hold
+                    // across assets.
                     Number const interval = state.paymentInterval;
                     auto const periodicRate =
-                        interval * (12 / 100) / (365 * 24 * 60 * 60);
+                        interval * Number(12, -2) / (365 * 24 * 60 * 60);
-                    // 20 seconds since the loan started (last env.close call)
+                    BEAST_EXPECT(
+                        periodicRate ==
+                        Number(2283105022831050, -21, Number::unchecked{}));
                     STAmount const accruedInterest{
                         broker.asset,
-                        state.principalOutstanding * periodicRate * 20 /
+                        state.principalOutstanding * periodicRate * loanAge /
                             interval};
+                    BEAST_EXPECT(
+                        accruedInterest ==
+                        broker.asset(Number(1141552511415525, -19)));
                     STAmount const prepaymentPenalty{
                         broker.asset,
-                        state.principalOutstanding * 36 / 10 / 100};
+                        state.principalOutstanding * Number(36, -3)};
+                    BEAST_EXPECT(prepaymentPenalty == broker.asset(36));
                     STAmount const closePaymentFee = broker.asset(4);
                     auto const payoffAmount =
                         STAmount{broker.asset, state.principalOutstanding} +
                         accruedInterest + prepaymentPenalty + closePaymentFee;
+                    BEAST_EXPECT(
+                        payoffAmount ==
+                        broker.asset(Number(1040000114155251, -12)));
+                    BEAST_EXPECT(payoffAmount > drawAmount);
                     // Try to pay a little extra to show that it's _not_ taken
                     auto const transactionAmount =
                         payoffAmount + broker.asset(10);
-                    BEAST_EXPECT(payoffAmount > drawAmount);
+                    BEAST_EXPECT(
+                        transactionAmount ==
+                        broker.asset(Number(1050000114155251, -12)));
                     env(pay(borrower, loanKeylet.key, transactionAmount));
 
+                    env.close();
+
                     // Need to account for fees if the loan is in XRP
                     adjustment = broker.asset(0);
                     if (broker.asset.raw().native())
@@ -1328,10 +1352,14 @@ class Loan_test : public beast::unit_test::suite
 
                     BEAST_EXPECT(
                         env.balance(borrower, broker.asset) ==
-                        borrowerBeforePaymentBalance - payoffAmount -
+                        borrowerBalanceBeforePayment - payoffAmount -
                             adjustment);
 
-                    // TODO: Try to impair a paid off loan
+                    // Can't impair or default a paid off loan
+                    env(manage(lender, loanKeylet.key, tfLoanImpair),
+                        ter(tecNO_PERMISSION));
+                    env(manage(lender, loanKeylet.key, tfLoanDefault),
+                        ter(tecNO_PERMISSION));
                 });
 
 #if 0 

src/test/jtx/amount.h

@@ -216,13 +216,13 @@ public:
     PrettyAmount
     operator()(T v) const
     {
-        bool negative = false;
+        return operator()(Number(v));
-        if (v < 0)
-        {
-            v = -v;
-            negative = true;
     }
-        STAmount amount{asset_, v * scale_, 0, negative};
+
+    PrettyAmount
+    operator()(Number v) const
+    {
+        STAmount amount{asset_, v * scale_};
         return {amount, ""};
     }
 

src/xrpld/app/misc/LendingHelpers.h

@@ -127,10 +127,11 @@ computePeriodicPaymentParts(
     Number const interest =
         roundToAsset(asset, principalOutstanding * periodicRate);
     XRPL_ASSERT(
-        interest > 0,
+        interest >= 0,
         "ripple::detail::computePeriodicPayment : valid interest");
 
-    Number const principal = periodicPaymentAmount - interest;
+    Number const principal =
+        roundToAsset(asset, periodicPaymentAmount - interest);
     XRPL_ASSERT(
         principal > 0 && principal <= principalOutstanding,
         "ripple::detail::computePeriodicPayment : valid principal");
@@ -233,7 +234,8 @@ loanComputePaymentParts(
 
     Number const serviceFee = loan->at(sfLoanServiceFee);
     Number const latePaymentFee = loan->at(sfLatePaymentFee);
-    Number const closePaymentFee = loan->at(sfClosePaymentFee);
+    Number const closePaymentFee =
+        roundToAsset(asset, loan->at(sfClosePaymentFee));
     TenthBips32 const overpaymentFee{loan->at(sfOverpaymentFee)};
 
     std::uint32_t const paymentInterval = loan->at(sfPaymentInterval);
@@ -257,10 +259,10 @@ loanComputePaymentParts(
     XRPL_ASSERT(
         periodicRate > 0, "ripple::loanComputePaymentParts : valid rate");
 
-    Number const periodicPaymentAmount = roundToAsset(
+    // Don't round the payment amount. Only round the final computations using
-        asset,
+    // it.
-        detail::loanPeriodicPayment(
+    Number const periodicPaymentAmount = detail::loanPeriodicPayment(
-            principalOutstandingField, periodicRate, paymentRemainingField));
+        principalOutstandingField, periodicRate, paymentRemainingField);
     XRPL_ASSERT(
         periodicPaymentAmount > 0,
         "ripple::computePeriodicPayment : valid payment");
@@ -268,8 +270,10 @@ loanComputePaymentParts(
     auto const periodic = detail::computePeriodicPaymentParts(
         asset, principalOutstandingField, periodicPaymentAmount, periodicRate);
 
-    Number const totalValueOutstanding = detail::loanTotalValueOutstanding(
+    Number const totalValueOutstanding = roundToAsset(
-        periodicPaymentAmount, paymentRemainingField);
+        asset,
+        detail::loanTotalValueOutstanding(
+            periodicPaymentAmount, paymentRemainingField));
     XRPL_ASSERT(
         totalValueOutstanding > 0,
         "ripple::loanComputePaymentParts : valid total value");
@@ -277,7 +281,7 @@ loanComputePaymentParts(
         detail::loanTotalInterestOutstanding(
             principalOutstandingField, totalValueOutstanding);
     XRPL_ASSERT(
-        totalInterestOutstanding > 0,
+        totalInterestOutstanding >= 0,
         "ripple::loanComputePaymentParts : valid total interest");
 
     view.update(loan);
@@ -351,29 +355,29 @@ loanComputePaymentParts(
             closePrepaymentInterest >= 0,
             "ripple::loanComputePaymentParts : valid prepayment "
             "interest");
-        auto const closeFullPayment = principalOutstandingField +
+        auto const totalInterest = accruedInterest + closePrepaymentInterest;
-            accruedInterest + closePrepaymentInterest + closePaymentFee;
+        auto const closeFullPayment =
+            principalOutstandingField + totalInterest + closePaymentFee;
 
         // if the payment is equal or higher than full payment amount, make a
         // full payment
         if (amount >= closeFullPayment)
         {
-            paymentRemainingField = 0;
-            principalOutstandingField = 0;
-
             // A full payment decreases the value of the loan by the
             // difference between the interest paid and the expected
             // outstanding interest return
-            auto const valueChange = accruedInterest - totalInterestOutstanding;
+            auto const valueChange = totalInterest - totalInterestOutstanding;
-            XRPL_ASSERT(
+
-                valueChange <= 0,
+            LoanPaymentParts const result{
-                "ripple::loanComputePaymentParts : valid full payment "
-                "value change");
-            return LoanPaymentParts{
                 principalOutstandingField,
-                accruedInterest,
+                totalInterest,
                 valueChange,
                 closePaymentFee};
+
+            paymentRemainingField = 0;
+            principalOutstandingField = 0;
+
+            return result;
         }
     }
 
@@ -384,7 +388,8 @@ loanComputePaymentParts(
     // periodic one, with possible overpayments
 
     std::optional<NumberRoundModeGuard> mg(Number::downward);
-    std::int64_t const fullPeriodicPayments{amount / periodicPaymentAmount};
+    std::int64_t const fullPeriodicPayments{
+        amount / roundToAsset(asset, periodicPaymentAmount, Number::upward)};
     mg.reset();
     // Temporary asserts
     XRPL_ASSERT(
@@ -441,13 +446,14 @@ loanComputePaymentParts(
             principalOutstandingField.value(),
             amount - periodicPaymentAmount * fullPeriodicPayments);
 
-        if (overpayment > 0)
+        if (roundToAsset(asset, overpayment) > 0)
         {
             Number const interestPortion = roundToAsset(
                 asset, tenthBipsOfValue(overpayment, overpaymentInterestRate));
             Number const feePortion = roundToAsset(
                 asset, tenthBipsOfValue(overpayment, overpaymentFee));
-            Number const remainder = overpayment - interestPortion - feePortion;
+            Number const remainder =
+                roundToAsset(asset, overpayment - interestPortion - feePortion);
 
             // Don't process an overpayment if the whole amount (or more!) gets
             // eaten by fees

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

@@ -188,6 +188,19 @@ LoanPay::doApply()
     // If the loan was impaired, it isn't anymore.
     loanSle->clearFlag(lsfLoanImpaired);
 
+    XRPL_ASSERT_PARTS(
+        paymentParts->principalPaid > 0,
+        "ripple::LoanPay::doApply",
+        "valid principal paid");
+    XRPL_ASSERT_PARTS(
+        paymentParts->interestPaid >= 0,
+        "ripple::LoanPay::doApply",
+        "valid interest paid");
+    XRPL_ASSERT_PARTS(
+        paymentParts->feePaid >= 0,
+        "ripple::LoanPay::doApply",
+        "valid fee paid");
+
     //------------------------------------------------------
     // LoanBroker object state changes
     view.update(brokerSle);
@@ -206,8 +219,10 @@ LoanPay::doApply()
     auto debtTotalField = brokerSle->at(sfDebtTotal);
     TenthBips32 const coverRateMinimum{brokerSle->at(sfCoverRateMinimum)};
 
-    bool const sufficientCover = coverAvailableField >=
+    bool const sufficientCover =
-        tenthBipsOfValue(debtTotalField.value(), coverRateMinimum);
+        coverAvailableField >=
+        roundToAsset(
+            asset, tenthBipsOfValue(debtTotalField.value(), coverRateMinimum));
     if (!sufficientCover)
     {
         // Add the fee to to First Loss Cover Pool
@@ -217,7 +232,9 @@ LoanPay::doApply()
     // Decrease LoanBroker Debt by the amount paid, add the Loan value change,
     // and subtract the change in the management fee
     auto const vaultValueChange = paymentParts->valueChange -
-        tenthBipsOfValue(paymentParts->valueChange, managementFeeRate);
+        roundToAsset(asset,
+                     tenthBipsOfValue(
+                         paymentParts->valueChange, managementFeeRate));
     debtTotalField += vaultValueChange - totalPaidToVault;
 
     //------------------------------------------------------
@@ -231,9 +248,14 @@ LoanPay::doApply()
     STAmount const paidToVault(asset, totalPaidToVault);
     STAmount const paidToBroker(asset, totalFee);
     XRPL_ASSERT_PARTS(
-        paidToVault + paidToBroker == amount,
+        paidToVault + paidToBroker <= amount,
         "ripple::LoanPay::doApply",
-        "correct payment totals");
+        "amount is sufficient");
+    XRPL_ASSERT_PARTS(
+        paidToVault + paidToBroker <= paymentParts->principalPaid +
+                paymentParts->interestPaid + paymentParts->feePaid,
+        "ripple::LoanPay::doApply",
+        "payment agreement");
 
     if (auto const ter = accountSend(
             view,