From d7196a8e56b2ceae4b0b1d7d6d0df1e5a587f1ba Mon Sep 17 00:00:00 2001 From: Ed Hennis Date: Sun, 12 Oct 2025 19:18:14 -0400 Subject: [PATCH] Get remaining Loan unit tests working - Rename some of the helper functions / lambdas. - Update tracked interestOwed field better at final payoff. - Add checks in LoanSet that the fields can be represented in the asset type, and update test that fails those checks (testLoanPayComputePeriodicPaymentValidRateInvariant) - Also check that the computed periodic payment can be represented as the asset type, and doesn't round _UP_ to 0. - Update asserts to account for more scenarios, including initial loan computation. --- src/test/app/Loan_test.cpp | 61 ++++++------- src/xrpld/app/misc/LendingHelpers.h | 33 +++++-- src/xrpld/app/tx/detail/LoanSet.cpp | 136 +++++++++++++++++++--------- src/xrpld/app/tx/detail/LoanSet.h | 3 + 4 files changed, 146 insertions(+), 87 deletions(-) diff --git a/src/test/app/Loan_test.cpp b/src/test/app/Loan_test.cpp index 4dce52995f..c067dea02e 100644 --- a/src/test/app/Loan_test.cpp +++ b/src/test/app/Loan_test.cpp @@ -1369,7 +1369,7 @@ class Loan_test : public beast::unit_test::suite ter(tecNO_PERMISSION)); }; - auto immediatePayoff = [&](std::uint32_t baseFlag) { + auto fullPayment = [&](std::uint32_t baseFlag) { return [&, baseFlag]( Keylet const& loanKeylet, VerifyLoanStatus const& verifyLoanStatus) { @@ -1432,7 +1432,7 @@ class Loan_test : public beast::unit_test::suite }; }; - auto multiplePayoff = [&](std::uint32_t baseFlag) { + auto combineAllPayments = [&](std::uint32_t baseFlag) { return [&, baseFlag]( Keylet const& loanKeylet, VerifyLoanStatus const& verifyLoanStatus) { @@ -1536,7 +1536,7 @@ class Loan_test : public beast::unit_test::suite broker, pseudoAcct, 0, - immediatePayoff(0)); + fullPayment(0)); lifecycle( caseLabel, @@ -1550,11 +1550,11 @@ class Loan_test : public beast::unit_test::suite broker, pseudoAcct, tfLoanOverpayment, - immediatePayoff(lsfLoanOverpayment)); + fullPayment(lsfLoanOverpayment)); lifecycle( caseLabel, - "Loan overpayment prohibited - Make multiple payments", + "Loan overpayment prohibited - Combine all payments", env, loanAmount, interestExponent, @@ -1564,11 +1564,11 @@ class Loan_test : public beast::unit_test::suite broker, pseudoAcct, 0, - multiplePayoff(0)); + combineAllPayments(0)); lifecycle( caseLabel, - "Loan overpayment allowed - Make multiple payments", + "Loan overpayment allowed - Combine all payments", env, loanAmount, interestExponent, @@ -1578,7 +1578,7 @@ class Loan_test : public beast::unit_test::suite broker, pseudoAcct, tfLoanOverpayment, - multiplePayoff(lsfLoanOverpayment)); + combineAllPayments(lsfLoanOverpayment)); lifecycle( caseLabel, @@ -1775,20 +1775,21 @@ class Loan_test : public beast::unit_test::suite if (paymentComponents.final) { state.paymentRemaining = 0; + state.interestOwed = 0; } else { state.nextPaymentDate += state.paymentInterval; + state.interestOwed -= valueMinusFee( + broker.asset.raw(), + paymentComponents.roundedInterest, + managementFeeRateParameter, + state.loanScale); } state.principalOutstanding -= paymentComponents.roundedPrincipal; state.totalValue -= paymentComponents.roundedPrincipal + paymentComponents.roundedInterest; - state.interestOwed -= valueMinusFee( - broker.asset.raw(), - paymentComponents.roundedInterest, - managementFeeRateParameter, - state.loanScale); verifyLoanStatus(state); } @@ -2214,38 +2215,28 @@ class Loan_test : public beast::unit_test::suite createJson["OverpaymentInterestRate"] = 1360; createJson["PaymentInterval"] = 727; - Number const actualPrincipal{6}; - auto const brokerStateBefore = env.le(keylet::loanbroker(broker.brokerID)); auto const loanSequence = brokerStateBefore->at(sfLoanSequence); auto const keylet = keylet::loan(broker.brokerID, loanSequence); createJson = env.json(createJson, sig(sfCounterpartySignature, lender)); - env(createJson, ter(tesSUCCESS)); + // Fails in preclaim because principal requested can't be represented as + // XRP + env(createJson, ter(tecPRECISION_LOSS)); env.close(); - if (auto const loan = env.le(keylet); BEAST_EXPECT(loan)) - { - // Verify the payment decreased the principal - BEAST_EXPECT(loan->at(sfPaymentRemaining) == numPayments); - BEAST_EXPECT(loan->at(sfLoanScale) == actualPrincipal.exponent()); - BEAST_EXPECT(loan->at(sfPrincipalOutstanding) == actualPrincipal); - } + BEAST_EXPECT(!env.le(keylet)); - auto loanPayTx = env.json( - pay(borrower, keylet.key, STAmount{broker.asset, serviceFee + 6})); - env(loanPayTx, ter(tesSUCCESS)); + Number const actualPrincipal{6}; + + createJson[sfPrincipalRequested] = actualPrincipal; + createJson.removeMember(sfSequence.jsonName); + createJson = env.json(createJson, sig(sfCounterpartySignature, lender)); + // Fails in doApply because the payment is too small to be represented + // as XRP. + env(createJson, ter(tecPRECISION_LOSS)); env.close(); - - if (auto const loan = env.le(keylet); BEAST_EXPECT(loan)) - { - // Verify the payment decreased the principal - BEAST_EXPECT(loan->at(sfPaymentRemaining) == numPayments - 1); - BEAST_EXPECT(loan->at(sfLoanScale) == actualPrincipal.exponent()); - BEAST_EXPECT( - loan->at(sfPrincipalOutstanding) == actualPrincipal - 1); - } } void diff --git a/src/xrpld/app/misc/LendingHelpers.h b/src/xrpld/app/misc/LendingHelpers.h index b9cd00de83..af056ffbd9 100644 --- a/src/xrpld/app/misc/LendingHelpers.h +++ b/src/xrpld/app/misc/LendingHelpers.h @@ -225,16 +225,19 @@ computePaymentComponents( auto const p = roundToAsset( asset, // Compute the delta that will get the tracked principalOutstanding - // amount as close to the raw principal amount after the payment as + // amount as close to the true principal amount after the payment as // possible. principalOutstanding - (rawPrincipalOutstanding - rawPrincipal), scale, Number::downward); + // The principal part can only be 0 during intial loan validation. If it + // is 0, the Loan will not be created, but we don't want an assert + // aborting the process before we get that far. XRPL_ASSERT_PARTS( - p > 0, + p >= 0, "rippled::detail::computePaymentComponents", - "principal part positive"); + "principal part not negative"); XRPL_ASSERT_PARTS( p <= principalOutstanding, "rippled::detail::computePaymentComponents", @@ -266,7 +269,7 @@ computePaymentComponents( auto const iDiff = roundedPeriodicPayment - roundedPrincipal; // Compute the delta that will get the untracked interestOutstanding - // amount as close as possible to the raw interest amount after the + // amount as close as possible to the true interest amount after the // payment as possible. auto const iSync = interestOutstanding - (roundToAsset(asset, rawInterestOutstanding, scale) - @@ -691,7 +694,7 @@ template LoanProperties computeLoanProperties( A const& asset, - Number const& principalOutstanding, + Number principalOutstanding, TenthBips32 interestRate, std::uint32_t paymentInterval, std::uint32_t paymentsRemaining, @@ -723,8 +726,14 @@ computeLoanProperties( // over payments) auto const loanScale = totalValueOutstanding.exponent(); + // Since we just figured out the loan scale, we haven't been able to + // validate that the principal fits in it, so to allow this function to + // succeed, round it here, and let the caller do the validation. + principalOutstanding = roundToAsset( + asset, principalOutstanding, loanScale, Number::to_nearest); + auto const firstPaymentPrincipal = [&]() { - // Compute the unrounded parts for the first payment. Ensure that the + // Compute the parts for the first payment. Ensure that the // principal payment will actually change the principal. auto const paymentComponents = detail::computePaymentComponents( asset, @@ -735,9 +744,9 @@ computeLoanProperties( periodicRate, paymentsRemaining); - // The rounded principal part needs to be large enough to affect the + // The unrounded principal part needs to be large enough to affect the // principal. What to do if not is left to the caller - return paymentComponents.roundedPrincipal; + return paymentComponents.rawPrincipal; }(); auto const interestOwedToVault = valueMinusFee( @@ -1086,6 +1095,10 @@ loanMakePayment( periodicRate, paymentRemainingProxy), serviceFee}; + XRPL_ASSERT_PARTS( + periodic.roundedPrincipal > 0, + "ripple::loanMakePayment", + "regular payment pays principal"); // ------------------------------------------------------------- // late payment handling @@ -1193,6 +1206,10 @@ loanMakePayment( periodicRate, paymentRemainingProxy), periodic.fee}; + XRPL_ASSERT_PARTS( + nextPayment.roundedPrincipal > 0, + "ripple::loanMakePayment", + "additional payment pays principal"); XRPL_ASSERT( nextPayment.rawInterest <= periodic.rawInterest, "ripple::loanMakePayment : decreasing interest"); diff --git a/src/xrpld/app/tx/detail/LoanSet.cpp b/src/xrpld/app/tx/detail/LoanSet.cpp index 70dbfd668b..83f3f0a429 100644 --- a/src/xrpld/app/tx/detail/LoanSet.cpp +++ b/src/xrpld/app/tx/detail/LoanSet.cpp @@ -179,6 +179,21 @@ LoanSet::calculateBaseFee(ReadView const& view, STTx const& tx) return normalCost + (signerCount * baseFee); } +std::vector> const& +LoanSet::getValueFields() +{ + static std::vector> const valueFields{ + ~sfPrincipalRequested, + ~sfLoanOriginationFee, + ~sfLoanServiceFee, + ~sfLatePaymentFee, + ~sfClosePaymentFee + // Overpayment fee is really a rate. Don't check it here. + }; + + return valueFields; +} + TER LoanSet::preclaim(PreclaimContext const& ctx) { @@ -221,6 +236,24 @@ LoanSet::preclaim(PreclaimContext const& ctx) Asset const asset = vault->at(sfAsset); auto const vaultPseudo = vault->at(sfAccount); + // Check that relevant values can be represented as the vault asset type. + // This check is almost duplicated in doApply, but that check is done after + // the overall loan scale is known. This is mostly only relevant for + // integral (non-IOU) types + { + for (auto const& field : getValueFields()) + { + if (auto const value = tx[field]; + value && STAmount{asset, *value} != *value) + { + JLOG(ctx.j.warn()) << field.f->getName() << " (" << *value + << ") can not be represented as a(n) " + << to_string(asset) << "."; + return tecPRECISION_LOSS; + } + } + } + if (auto const ter = canAddHolding(ctx.view, asset)) return ter; @@ -310,43 +343,52 @@ LoanSet::doApply() paymentTotal, TenthBips32{brokerSle->at(sfManagementFeeRate)}); + // Check that relevant values won't lose precision. This is mostly only + // relevant for IOU assets. + { + for (auto const& field : getValueFields()) + { + if (auto const value = tx[field]; + value && !isRounded(vaultAsset, *value, properties.loanScale)) + { + JLOG(j_.warn()) + << field.f->getName() << " (" << *value + << ") has too much precision. Total loan value is " + << properties.totalValueOutstanding << " with a scale of " + << properties.loanScale; + return tecPRECISION_LOSS; + } + } + } + // Guard 1: if there is no computed total interest over the life of the loan // for a non-zero interest rate, we cannot properly amortize the loan if (interestRate > TenthBips32{0} && (properties.totalValueOutstanding - principalRequested) <= 0) { - // Unless this is a zero-interst loan, there must be some interest due + // Unless this is a zero-interest loan, there must be some interest due // on the loan, even if it's (measurable) dust JLOG(j_.warn()) << "Loan with " << interestRate << "% interest has no interest due"; return tecPRECISION_LOSS; } - - // Guard 2: if the rounded periodic payment is large enough that the loan - // can't be amortized in the specified number of payments, raise an error - if (auto const computedPayments = roundToAsset( - vaultAsset, - properties.totalValueOutstanding / - roundToAsset( - vaultAsset, - properties.periodicPayment, - properties.loanScale, - Number::upward), - properties.loanScale, - Number::upward); - computedPayments < paymentTotal) + // Guard 1a: If there is any interest computed over the life of the loan, + // for a zero interest rate, something went sideways. + if (interestRate == TenthBips32{0} && + (properties.totalValueOutstanding - principalRequested) > 0) { - JLOG(j_.warn()) << "Loan Periodic payment rounding will complete the " - "loan in less than the specified number of payments"; - return tecPRECISION_LOSS; + // LCOV_EXCL_START + JLOG(j_.warn()) << "Loan with 0% interest has interest due"; + return tecINTERNAL; + // LCOV_EXCL_STOP } - // Guard 3: if the principal portion of the first periodic payment is too + // Guard 2: if the principal portion of the first periodic payment is too // small to be accurately represented with the given rounding mode, raise an // error if (properties.firstPaymentPrincipal <= 0) { - // Check that some reference principal is paid each period. Since + // Check that some true (unrounded) principal is paid each period. Since // the first payment pays the least principal, if it's good, they'll // all be good. Note that the outstanding principal is rounded, and // may not change right away. @@ -354,6 +396,36 @@ LoanSet::doApply() return tecPRECISION_LOSS; } + // Guard 3: If the periodic payment is so small that it can't even be + // rounded to a representable value, then the loan can't be paid. Also, + // avoids dividing by 0. + auto const roundedPayment = roundPeriodicPayment( + vaultAsset, properties.periodicPayment, properties.loanScale); + if (roundedPayment == Number{}) + { + JLOG(j_.warn()) << "Loan Periodic payment (" + << properties.periodicPayment << ") rounds to 0. "; + return tecPRECISION_LOSS; + } + + // Guard 4: if the rounded periodic payment is large enough that the loan + // can't be amortized in the specified number of payments, raise an error + if (auto const computedPayments = roundToAsset( + vaultAsset, + properties.totalValueOutstanding / roundedPayment, + properties.loanScale, + Number::upward); + computedPayments < paymentTotal) + { + JLOG(j_.warn()) + << "Loan Periodic payment (" << properties.periodicPayment + << ") rounding (" << roundedPayment + << ") will complete the " + "loan in less than the specified number of payments (" + << computedPayments << " < " << paymentTotal << ")"; + return tecPRECISION_LOSS; + } + // Check that the other computed values are valid if (properties.interestOwedToVault < 0 || properties.totalValueOutstanding <= 0 || @@ -366,30 +438,6 @@ LoanSet::doApply() // LCOV_EXCL_STOP } - // Check that relevant values won't lose precision - { - static std::map> const - valueFields{ - {"Principal Requested", ~sfPrincipalRequested}, - {"Origination fee", ~sfLoanOriginationFee}, - {"Service fee", ~sfLoanServiceFee}, - {"Late Payment fee", ~sfLatePaymentFee}, - {"Close Payment fee", ~sfClosePaymentFee} - // Overpayment fee is really a rate. Don't include it. - }; - for (auto const& [name, field] : valueFields) - { - if (auto const value = tx[field]; - value && !isRounded(vaultAsset, *value, properties.loanScale)) - { - JLOG(j_.warn()) - << name << " has too much precision. Total loan value is " - << properties.totalValueOutstanding << " with a scale of " - << properties.loanScale; - return tecPRECISION_LOSS; - } - } - } auto const originationFee = tx[~sfLoanOriginationFee].value_or(Number{}); auto const loanAssetsToBorrower = principalRequested - originationFee; diff --git a/src/xrpld/app/tx/detail/LoanSet.h b/src/xrpld/app/tx/detail/LoanSet.h index cb98e361c0..68c7f4ca3c 100644 --- a/src/xrpld/app/tx/detail/LoanSet.h +++ b/src/xrpld/app/tx/detail/LoanSet.h @@ -48,6 +48,9 @@ public: static XRPAmount calculateBaseFee(ReadView const& view, STTx const& tx); + static std::vector> const& + getValueFields(); + static TER preclaim(PreclaimContext const& ctx);