diff --git a/src/libxrpl/ledger/helpers/LendingHelpers.cpp b/src/libxrpl/ledger/helpers/LendingHelpers.cpp index 1fedbb5f13..9cda5905c9 100644 --- a/src/libxrpl/ledger/helpers/LendingHelpers.cpp +++ b/src/libxrpl/ledger/helpers/LendingHelpers.cpp @@ -769,9 +769,6 @@ doOverpayment( "xrpl::detail::doOverpayment", "principal change agrees"); - // I'm not 100% sure the following asserts are correct. If in doubt, and - // everything else works, remove any that cause trouble. - JLOG(j.debug()) << "valueChange: " << loanPaymentParts.valueChange << ", totalValue before: " << *totalValueOutstandingProxy << ", totalValue after: " << newRoundedLoanState.valueOutstanding @@ -783,11 +780,28 @@ doOverpayment( << overpaymentComponents.trackedPrincipalDelta - (totalValueOutstandingProxy - newRoundedLoanState.valueOutstanding); + // The valueChange returned by tryOverpayment satisfies + // valueChange = (newInterestDue - oldInterestDue) + untrackedInterest. + // Using the loan-state identity v = p + i + m and the adjacent + // `principal change agrees` assertion (dp = oldP - newP), this + // rearranges into three independently-computable terms: + // + // 1. TVO change beyond what principal repayment alone explains: + // newTVO - (oldTVO - dp) + // 2. Management fee released by re-amortization (positive when + // mfee decreased; zero when managementFeeRate == 0): + // oldMfee - newMfee + // 3. The overpayment's penalty interest part (= untrackedInterest + // for the overpayment path; see computeOverpaymentComponents): + // trackedInterestPart() + [[maybe_unused]] Number const tvoChange = newRoundedLoanState.valueOutstanding - + (totalValueOutstandingProxy - overpaymentComponents.trackedPrincipalDelta); + [[maybe_unused]] Number const managementFeeReleased = + managementFeeOutstandingProxy - newRoundedLoanState.managementFeeDue; + [[maybe_unused]] Number const interestPart = overpaymentComponents.trackedInterestPart(); + XRPL_ASSERT_PARTS( - loanPaymentParts.valueChange == - newRoundedLoanState.valueOutstanding - - (totalValueOutstandingProxy - overpaymentComponents.trackedPrincipalDelta) + - overpaymentComponents.trackedInterestPart(), + loanPaymentParts.valueChange == tvoChange + managementFeeReleased + interestPart, "xrpl::detail::doOverpayment", "interest paid agrees"); @@ -2027,51 +2041,62 @@ loanMakePayment( // It shouldn't be possible for the overpayment to be greater than // totalValueOutstanding, because that would have been processed as // another normal payment. But cap it just in case. - Number const overpayment = std::min(roundedAmount - totalPaid, *totalValueOutstandingProxy); + Number const overpaymentRaw = + std::min(roundedAmount - totalPaid, *totalValueOutstandingProxy); - detail::ExtendedPaymentComponents const overpaymentComponents = - detail::computeOverpaymentComponents( - asset, - loanScale, - overpayment, - overpaymentInterestRate, - overpaymentFeeRate, - managementFeeRate); + bool const fixEnabled = view.rules().enabled(fixCleanup3_2_0); + Number const overpayment = fixEnabled + ? roundToAsset(asset, overpaymentRaw, loanScale, Number::RoundingMode::Downward) + : overpaymentRaw; - // Don't process an overpayment if the whole amount (or more!) - // gets eaten by fees and interest. - if (overpaymentComponents.trackedPrincipalDelta > 0) + // Post-amendment, the rounded overpayment can be zero; pre-amendment + // it's always positive given the surrounding guards. + if (!fixEnabled || overpayment > 0) { - XRPL_ASSERT_PARTS( - overpaymentComponents.untrackedInterest >= beast::kZero, - "xrpl::loanMakePayment", - "overpayment penalty did not reduce value of loan"); - // Can't just use `periodicPayment` here, because it might - // change - auto periodicPaymentProxy = loan->at(sfPeriodicPayment); - if (auto const overResult = detail::doOverpayment( - view.rules(), + detail::ExtendedPaymentComponents const overpaymentComponents = + detail::computeOverpaymentComponents( asset, loanScale, - overpaymentComponents, - totalValueOutstandingProxy, - principalOutstandingProxy, - managementFeeOutstandingProxy, - periodicPaymentProxy, - periodicRate, - paymentRemainingProxy, - managementFeeRate, - j)) + overpayment, + overpaymentInterestRate, + overpaymentFeeRate, + managementFeeRate); + + // Don't process an overpayment if the whole amount (or more!) + // gets eaten by fees and interest. + if (overpaymentComponents.trackedPrincipalDelta > 0) { - totalParts += *overResult; - } - else if (overResult.error()) - { - // error() will be the TER returned if a payment is not - // made. It will only evaluate to true if it's unsuccessful. - // Otherwise, tesSUCCESS means nothing was done, so - // continue. - return Unexpected(overResult.error()); + XRPL_ASSERT_PARTS( + overpaymentComponents.untrackedInterest >= beast::kZero, + "xrpl::loanMakePayment", + "overpayment penalty did not reduce value of loan"); + // Can't just use `periodicPayment` here, because it might + // change + auto periodicPaymentProxy = loan->at(sfPeriodicPayment); + if (auto const overResult = detail::doOverpayment( + view.rules(), + asset, + loanScale, + overpaymentComponents, + totalValueOutstandingProxy, + principalOutstandingProxy, + managementFeeOutstandingProxy, + periodicPaymentProxy, + periodicRate, + paymentRemainingProxy, + managementFeeRate, + j)) + { + totalParts += *overResult; + } + else if (overResult.error()) + { + // error() will be the TER returned if a payment is not + // made. It will only evaluate to true if it's unsuccessful. + // Otherwise, tesSUCCESS means nothing was done, so + // continue. + return Unexpected(overResult.error()); + } } } } diff --git a/src/test/app/Loan_test.cpp b/src/test/app/Loan_test.cpp index 5e8e89cefa..c380655563 100644 --- a/src/test/app/Loan_test.cpp +++ b/src/test/app/Loan_test.cpp @@ -169,6 +169,10 @@ protected: TenthBips32 coverRateLiquidation = percentageToTenthBips(25); std::string data = {}; // NOLINT(readability-redundant-member-init) std::uint32_t flags = 0; + // If set, the vault is created with this sfScale value. Useful for + // tests that need finer loanScale to exercise rounding edge cases. + std::optional vaultScale = + std::nullopt; // NOLINT(readability-redundant-member-init) [[nodiscard]] Number maxCoveredLoanValue(Number const& currentDebt) const @@ -522,6 +526,8 @@ protected: auto const coverRateMinValue = params.coverRateMin; auto [tx, vaultKeylet] = vault.create({.owner = lender, .asset = asset}); + if (params.vaultScale) + tx[sfScale] = *params.vaultScale; env(tx); env.close(); BEAST_EXPECT(env.le(vaultKeylet)); @@ -2157,21 +2163,23 @@ protected: // If the loan does not allow overpayments, send a payment that // tries to make an overpayment. Do not include `txFlags`, so we // don't end up duplicating the next test transaction. - env(pay(borrower, - loanKeylet.key, - STAmount{broker.asset, state.periodicPayment * Number{15, -1}}, - tfLoanOverpayment), - Fee(XRPAmount{baseFee * (Number{15, -1} / kLoanPaymentsPerFeeIncrement + 1)}), - Ter(tecNO_PERMISSION)); + // + // fixCleanup3_1_3 gates tfLoanOverpayment as a valid flag: + // with fix on → preflight passes, apply returns tecNO_PERMISSION; + // with fix off → preflight rejects the flag, returns temINVALID_FLAG. + bool const hasFix313 = env.current()->rules().enabled(fixCleanup3_1_3); + STAmount const overpayAmount{broker.asset, state.periodicPayment * Number{15, -1}}; + XRPAmount const overpayFee{ + baseFee * (Number{15, -1} / kLoanPaymentsPerFeeIncrement + 1)}; + env(pay(borrower, loanKeylet.key, overpayAmount, tfLoanOverpayment), + Fee(overpayFee), + Ter(hasFix313 ? TER{tecNO_PERMISSION} : TER{temINVALID_FLAG})); + if (hasFix313) { env.disableFeature(fixCleanup3_1_3); - env(pay(borrower, - loanKeylet.key, - STAmount{broker.asset, state.periodicPayment * Number{15, -1}}, - tfLoanOverpayment), - Fee(XRPAmount{ - baseFee * (Number{15, -1} / kLoanPaymentsPerFeeIncrement + 1)}), + env(pay(borrower, loanKeylet.key, overpayAmount, tfLoanOverpayment), + Fee(overpayFee), Ter(temINVALID_FLAG)); env.enableFeature(fixCleanup3_1_3); } @@ -7027,7 +7035,7 @@ protected: auto credType = "credential1"; - pdomain::Credentials const credentials1{{.issuer = issuer, .credType = credType}}; + pdomain::Credentials const credentials1 = {{.issuer = issuer, .credType = credType}}; env(pdomain::setTx(issuer, credentials1)); env.close(); @@ -7572,6 +7580,74 @@ protected: attemptWithdrawShares(depositorB, sharesLpB, tesSUCCESS); } + // An overpayment whose residual amount has more precision than loanScale + // fires the isRounded(asset, overpayment, loanScale) assertion in + // computeOverpaymentComponents (and a downstream "interest paid agrees" + // assertion in doOverpayment). fixCleanup3_2_0 rounds the residual down + // to loanScale before passing it in. The pre-amendment path can't be + // tested here because the assertion fires in Debug builds and aborts + // the test process — see the PR description for context. + void + testBugOverpayUnroundedAmount() + { + testcase("bug: computeOverpaymentComponents isRounded assertion"); + + using namespace jtx; + using namespace loan; + Env env(*this, all_); + + Account const issuer{"issuer"}; + Account const lender{"vaultOwner"}; + Account const borrower{"borrower"}; + + env.fund(XRP(1'000'000), issuer, lender, borrower); + env(fset(issuer, asfDefaultRipple)); + env.close(); + + PrettyAsset const iouAsset = issuer["USD"]; + STAmount const iouLimit{iouAsset.raw(), Number{9'999'999'999'999'999LL}}; + env(trust(lender, iouLimit)); + env(trust(borrower, iouLimit)); + env(pay(issuer, lender, iouAsset(1'000'000))); + env(pay(issuer, borrower, iouAsset(1'000'000))); + env.close(); + + auto const broker = createVaultAndBroker( + env, + iouAsset, + lender, + {.vaultDeposit = 100'000, + .debtMax = 5000, + .managementFeeRate = TenthBips16{1000}, + .vaultScale = 1}); + + auto const sleBroker = env.le(broker.brokerKeylet()); + if (!BEAST_EXPECT(sleBroker)) + return; + auto const loanSequence = sleBroker->at(sfLoanSequence); + auto const loanKeylet = keylet::loan(broker.brokerID, loanSequence); + + using namespace loan; + env(set(borrower, broker.brokerID, Number{1000}, tfLoanOverpayment), + Sig(sfCounterpartySignature, lender), + kInterestRate(TenthBips32{10000}), + kPaymentTotal(12), + kPaymentInterval(60), + kGracePeriod(60), + kOverpaymentFee(TenthBips32{1000}), + kOverpaymentInterestRate(TenthBips32{1000}), + Fee(env.current()->fees().base * 2), + Ter(tesSUCCESS)); + env.close(); + + // periodic * 1.5 at 15-sig-digit precision: 125.000154585042. This + // has too many digits to round cleanly to loanScale=-10, so the + // overpayment residual fails the isRounded check. + STAmount const payAmount{iouAsset.raw(), Number{125'000'154'585'042LL, -12}}; + env(pay(borrower, loanKeylet.key, payAmount), Txflags(tfLoanOverpayment), Ter(tesSUCCESS)); + env.close(); + } + // Regression for the dual-rounding fix at coarse (integer-MPT) scale. // // Loan: P=1, r=50% (50000 tenth-bips), n=3, yearly interval. The @@ -8280,6 +8356,9 @@ protected: testRIPD3901(); testBorrowerIsBroker(); testLimitExceeded(); + testLoanSetBlockedLoanPayAllowedWhenCanTransferCleared(); + testLendingCanTradeClearedNoImpact(); + testBugOverpayUnroundedAmount(); for (auto const flags : {0u, tfLoanOverpayment}) testYieldTheftRounding(flags); @@ -8295,11 +8374,11 @@ protected: testLoanPayLateFullPaymentBypassesPenalties(features); testLoanCoverMinimumRoundingExploit(features); #endif - // Lifecycle - testSelfLoan(features); - testLoanSet(features); testLifecycle(features); + testLoanSet(features); + testDosLoanPay(features); + testSelfLoan(features); // Payment paths testWithdrawReflectsUnrealizedLoss(features); @@ -8346,11 +8425,8 @@ public: run() override { runAmendmentIndependent(); - testLoanSetBlockedLoanPayAllowedWhenCanTransferCleared(); - testLendingCanTradeClearedNoImpact(); - testDosLoanPay(all_ | fixCleanup3_1_3); - testDosLoanPay(all_ - fixCleanup3_1_3); - for (auto const& features : amendmentCombinations({fixCleanup3_2_0, featureMPTokensV2})) + for (auto const& features : + amendmentCombinations({fixCleanup3_1_3, fixCleanup3_2_0, featureMPTokensV2})) runAmendmentSensitive(features); } };