From bd014e270a45ff605a122f1c3831bd308b95f9ef Mon Sep 17 00:00:00 2001 From: Ed Hennis Date: Fri, 9 May 2025 00:27:52 -0400 Subject: [PATCH] Expand LoanPay test cases, and fix rounding errors - At least rounding errors for a full payoff --- include/xrpl/protocol/STAmount.h | 10 +++--- src/test/app/Loan_test.cpp | 48 +++++++++++++++++++------ src/test/jtx/amount.h | 14 ++++---- src/xrpld/app/misc/LendingHelpers.h | 56 ++++++++++++++++------------- src/xrpld/app/tx/detail/LoanPay.cpp | 32 ++++++++++++++--- 5 files changed, 109 insertions(+), 51 deletions(-) diff --git a/include/xrpl/protocol/STAmount.h b/include/xrpl/protocol/STAmount.h index 5566ca049f..27722babac 100644 --- a/include/xrpl/protocol/STAmount.h +++ b/include/xrpl/protocol/STAmount.h @@ -697,12 +697,14 @@ getRate(STAmount const& offerOut, STAmount const& offerIn); template 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; + return amount; } //------------------------------------------------------------------------------ diff --git a/src/test/app/Loan_test.cpp b/src/test/app/Loan_test.cpp index 034968f8e8..88af1d8b30 100644 --- a/src/test/app/Loan_test.cpp +++ b/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); - // 20 seconds since the loan started (last env.close call) + interval * Number(12, -2) / (365 * 24 * 60 * 60); + 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 diff --git a/src/test/jtx/amount.h b/src/test/jtx/amount.h index 80424cd626..d0b21d31ce 100644 --- a/src/test/jtx/amount.h +++ b/src/test/jtx/amount.h @@ -216,13 +216,13 @@ public: PrettyAmount operator()(T v) const { - bool negative = false; - if (v < 0) - { - v = -v; - negative = true; - } - STAmount amount{asset_, v * scale_, 0, negative}; + return operator()(Number(v)); + } + + PrettyAmount + operator()(Number v) const + { + STAmount amount{asset_, v * scale_}; return {amount, ""}; } diff --git a/src/xrpld/app/misc/LendingHelpers.h b/src/xrpld/app/misc/LendingHelpers.h index 4dae16db19..861bc603b9 100644 --- a/src/xrpld/app/misc/LendingHelpers.h +++ b/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( - asset, - detail::loanPeriodicPayment( - principalOutstandingField, periodicRate, paymentRemainingField)); + // Don't round the payment amount. Only round the final computations using + // it. + Number const periodicPaymentAmount = detail::loanPeriodicPayment( + 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( - periodicPaymentAmount, paymentRemainingField); + Number const totalValueOutstanding = roundToAsset( + 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 + - accruedInterest + closePrepaymentInterest + closePaymentFee; + auto const totalInterest = accruedInterest + closePrepaymentInterest; + 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; - XRPL_ASSERT( - valueChange <= 0, - "ripple::loanComputePaymentParts : valid full payment " - "value change"); - return LoanPaymentParts{ + auto const valueChange = totalInterest - totalInterestOutstanding; + + LoanPaymentParts const result{ principalOutstandingField, - accruedInterest, + totalInterest, valueChange, closePaymentFee}; + + paymentRemainingField = 0; + principalOutstandingField = 0; + + return result; } } @@ -384,7 +388,8 @@ loanComputePaymentParts( // periodic one, with possible overpayments std::optional 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 diff --git a/src/xrpld/app/tx/detail/LoanPay.cpp b/src/xrpld/app/tx/detail/LoanPay.cpp index bf1ad9d296..c20b9acca3 100644 --- a/src/xrpld/app/tx/detail/LoanPay.cpp +++ b/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 >= - tenthBipsOfValue(debtTotalField.value(), coverRateMinimum); + bool const sufficientCover = + 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,