From b2453b626e2227ff86733dce3f26c232fbfa341e Mon Sep 17 00:00:00 2001 From: Vito Tumas <5780819+Tapanito@users.noreply.github.com> Date: Tue, 1 Sep 2026 14:06:17 +0000 Subject: [PATCH] fix: Unblock VaultSet and cash-basis LoanSet at AssetsMaximum (#8143) --- src/libxrpl/tx/invariants/VaultInvariant.cpp | 31 ++- .../tx/transactors/lending/LoanSet.cpp | 13 +- .../app/invariants/InvariantsVault_test.cpp | 36 +++ src/test/app/lending/LoanCashBasis_test.cpp | 260 +++++++++++++++++- src/test/app/lending/LoanTestBase.h | 5 + 5 files changed, 327 insertions(+), 18 deletions(-) diff --git a/src/libxrpl/tx/invariants/VaultInvariant.cpp b/src/libxrpl/tx/invariants/VaultInvariant.cpp index a8ef0d3157..c0f98a8128 100644 --- a/src/libxrpl/tx/invariants/VaultInvariant.cpp +++ b/src/libxrpl/tx/invariants/VaultInvariant.cpp @@ -380,7 +380,7 @@ ValidVault::finalize( beast::Journal const& j) { bool const enforce = view.rules().enabled(featureSingleAssetVault); - bool const fixEnabled = view.rules().enabled(fixCleanup3_4_0); + bool const fix340Enabled = view.rules().enabled(fixCleanup3_4_0); if (!isTesSuccess(ret)) return true; // Do not perform checks @@ -572,7 +572,7 @@ ValidVault::finalize( else { bool const gapExceeded = [&] { - if (!fixEnabled) + if (!fix340Enabled) { return afterVault.lossUnrealized > afterVault.assetsTotal - afterVault.assetsAvailable; @@ -594,7 +594,7 @@ ValidVault::finalize( } } - if (fixEnabled && afterVault.lossUnrealized < kZero) + if (fix340Enabled && afterVault.lossUnrealized < kZero) { JLOG(j.fatal()) << "Invariant failed: loss unrealized must not be negative"; result = false; @@ -765,8 +765,13 @@ ValidVault::finalize( result = false; } + // AssetsTotal may exceed AssetsMaximum when the excess is interest. After + // fixCleanup3_4_0, only reject a VaultSet that supplies sfAssetsMaximum or + // otherwise changes the cap to a nonzero value still below AssetsTotal. if (afterVault.assetsMaximum > kZero && - afterVault.assetsTotal > afterVault.assetsMaximum) + afterVault.assetsTotal > afterVault.assetsMaximum && + (!fix340Enabled || tx.isFieldPresent(sfAssetsMaximum) || + beforeVault.assetsMaximum != afterVault.assetsMaximum)) { JLOG(j.fatal()) << // "Invariant failed: set assets outstanding must not " @@ -880,7 +885,7 @@ ValidVault::finalize( result = false; } - bool const acctVaultAddsUp = fixEnabled + bool const acctVaultAddsUp = fix340Enabled ? agreesWithinOneUnit( localVaultDeltaAssets * -1, accountDeltaAssets, @@ -935,7 +940,7 @@ ValidVault::finalize( auto const assetTotalDelta = roundToAsset( vaultAsset, afterVault.assetsTotal - beforeVault.assetsTotal, minScale); - bool const totalAddsUp = fixEnabled + bool const totalAddsUp = fix340Enabled ? agreesWithinOneUnit(assetTotalDelta, vaultDeltaAssets, vaultAsset, minScale) : assetTotalDelta == vaultDeltaAssets; if (!totalAddsUp) @@ -947,7 +952,7 @@ ValidVault::finalize( auto const assetAvailableDelta = roundToAsset( vaultAsset, afterVault.assetsAvailable - beforeVault.assetsAvailable, minScale); - bool const availableAddsUp = fixEnabled + bool const availableAddsUp = fix340Enabled ? agreesWithinOneUnit( assetAvailableDelta, vaultDeltaAssets, vaultAsset, minScale) : assetAvailableDelta == vaultDeltaAssets; @@ -993,7 +998,7 @@ ValidVault::finalize( // value merely rounds down to zero, so a missing delta while // the pool still held positive effective value indicates a // real accounting bug, not this exception. - bool const zeroDeltaIsLegitimate = fixEnabled && !maybeVaultDeltaAssets && + bool const zeroDeltaIsLegitimate = fix340Enabled && !maybeVaultDeltaAssets && beforeVault.assetsTotal == beforeVault.lossUnrealized; if (!maybeVaultDeltaAssets && !zeroDeltaIsLegitimate) @@ -1100,7 +1105,7 @@ ValidVault::finalize( vaultDeltaAssets.delta * -1 - destinationDelta.delta, destinationScale, Number::RoundingMode::Downward) == kZero; - bool const withdrawAddsUp = fixEnabled + bool const withdrawAddsUp = fix340Enabled ? agreesWithinOneUnit( localPseudoDeltaAssets * -1, roundedDestinationDelta, @@ -1150,7 +1155,7 @@ ValidVault::finalize( auto const assetTotalDelta = roundToAsset( vaultAsset, afterVault.assetsTotal - beforeVault.assetsTotal, minScale); // Note, vaultBalance is negative (see check above) - bool const totalAddsUp = fixEnabled + bool const totalAddsUp = fix340Enabled ? agreesWithinOneUnit( assetTotalDelta, vaultPseudoDeltaAssets, vaultAsset, minScale) : assetTotalDelta == vaultPseudoDeltaAssets; @@ -1164,7 +1169,7 @@ ValidVault::finalize( auto const assetAvailableDelta = roundToAsset( vaultAsset, afterVault.assetsAvailable - beforeVault.assetsAvailable, minScale); - bool const availableAddsUp = fixEnabled + bool const availableAddsUp = fix340Enabled ? agreesWithinOneUnit( assetAvailableDelta, vaultPseudoDeltaAssets, vaultAsset, minScale) : assetAvailableDelta == vaultPseudoDeltaAssets; @@ -1213,7 +1218,7 @@ ValidVault::finalize( auto const assetsTotalDelta = roundToAsset( vaultAsset, afterVault.assetsTotal - beforeVault.assetsTotal, minScale); - bool const totalAddsUp = fixEnabled + bool const totalAddsUp = fix340Enabled ? agreesWithinOneUnit( assetsTotalDelta, vaultDeltaAssets, vaultAsset, minScale) : assetsTotalDelta == vaultDeltaAssets; @@ -1228,7 +1233,7 @@ ValidVault::finalize( vaultAsset, afterVault.assetsAvailable - beforeVault.assetsAvailable, minScale); - bool const availableAddsUp = fixEnabled + bool const availableAddsUp = fix340Enabled ? agreesWithinOneUnit( assetAvailableDelta, vaultDeltaAssets, vaultAsset, minScale) : assetAvailableDelta == vaultDeltaAssets; diff --git a/src/libxrpl/tx/transactors/lending/LoanSet.cpp b/src/libxrpl/tx/transactors/lending/LoanSet.cpp index 2def3d2eb2..d84f5b09b3 100644 --- a/src/libxrpl/tx/transactors/lending/LoanSet.cpp +++ b/src/libxrpl/tx/transactors/lending/LoanSet.cpp @@ -336,7 +336,12 @@ LoanSet::preclaim(PreclaimContext const& ctx) } } - if (vault->at(sfAssetsMaximum) != 0 && vault->at(sfAssetsTotal) >= vault->at(sfAssetsMaximum)) + // Accrual origination credits interestDue into AssetsTotal, so a vault + // already at AssetsMaximum cannot take another loan. Cash-basis origination + // does not change AssetsTotal (see cash_basis::loanOriginationDeltas), so + // this leftover accrual gate must not apply there. + if (getVaultVersion(vault) != VaultVersion::CashBasis && vault->at(sfAssetsMaximum) != 0 && + vault->at(sfAssetsTotal) >= vault->at(sfAssetsMaximum)) { JLOG(ctx.j.warn()) << "Vault at maximum assets limit. Can't add another loan."; return tecLIMIT_EXCEEDED; @@ -467,9 +472,11 @@ LoanSet::doApply() properties.loanState.managementFeeDue); XRPL_ASSERT_PARTS( - *vaultSle->at(sfAssetsMaximum) == 0 || *vaultSle->at(sfAssetsMaximum) > *vaultTotalProxy, + *vaultSle->at(sfAssetsMaximum) == 0 || + getVaultVersion(vaultSle) == VaultVersion::CashBasis || + *vaultSle->at(sfAssetsMaximum) > *vaultTotalProxy, "xrpl::LoanSet::doApply", - "Vault is below maximum limit"); + "accrual vault is below maximum limit"); if (loanOriginationExceedsVaultMaximum(vaultSle, vaultTotalProxy, state.interestDue)) { diff --git a/src/test/app/invariants/InvariantsVault_test.cpp b/src/test/app/invariants/InvariantsVault_test.cpp index d816c36c15..caf4e9cfb6 100644 --- a/src/test/app/invariants/InvariantsVault_test.cpp +++ b/src/test/app/invariants/InvariantsVault_test.cpp @@ -865,6 +865,42 @@ class InvariantsVault_test : public InvariantsBase precloseXrp, TxAccount::A2); + // The cap check has two post-fixCleanup3_4_0 triggers: the transaction + // supplied sfAssetsMaximum, or the cap changed. The case above covers + // the cap-changed one (its ttVAULT_SET carries no fields). This covers + // the other: the cap is left alone at 30 XRP and the transaction + // carries sfAssetsMaximum, so only the isFieldPresent disjunct can + // fire. AssetsTotal is pushed past the cap here rather than in + // preclose because VaultSet::doApply refuses to set a cap below + // AssetsTotal, so the over-cap state is only reachable by fabrication. + // Raising AssetsTotal also trips the "must not change assets + // outstanding" check, hence two expected messages. + Number const vaultCap = XRP(30).number(); + doInvariantCheck( + {"set must not change assets outstanding", + "set assets outstanding must not exceed assets maximum"}, + [&](Account const& a1, Account const& a2, ApplyContext& ac) { + auto const keylet = keylet::vault(a1.id(), SeqProxy::rawSequence(ac.view().seq())); + return kAdjust(ac.view(), keylet, kArgs(a2.id(), 0, [&](Adjustments& sample) { + sample.assetsTotal = XRP(1).value().xrp().drops(); + })); + }, + XRPAmount{}, + STTx{ttVAULT_SET, [&](STObject& tx) { tx[sfAssetsMaximum] = vaultCap; }}, + {tecINVARIANT_FAILED, tecINVARIANT_FAILED}, + [&](Account const& a1, Account const& a2, Env& env) -> bool { + env.fund(XRP(1000), a3, a4); + Vault const vault{env}; + auto [tx, keylet] = vault.create({.owner = a1, .asset = xrpIssue()}); + tx[sfAssetsMaximum] = vaultCap; + env(tx); + env(vault.deposit({.depositor = a1, .id = keylet.key, .amount = XRP(10)})); + env(vault.deposit({.depositor = a2, .id = keylet.key, .amount = XRP(10)})); + env(vault.deposit({.depositor = a3, .id = keylet.key, .amount = XRP(10)})); + return true; + }, + TxAccount::A2); + doInvariantCheck( {"assets maximum must not be negative"}, [&](Account const& a1, Account const& a2, ApplyContext& ac) { diff --git a/src/test/app/lending/LoanCashBasis_test.cpp b/src/test/app/lending/LoanCashBasis_test.cpp index a3ca28437d..e238838306 100644 --- a/src/test/app/lending/LoanCashBasis_test.cpp +++ b/src/test/app/lending/LoanCashBasis_test.cpp @@ -4,10 +4,12 @@ #include #include #include +#include #include #include #include +#include #include #include #include @@ -25,6 +27,7 @@ #include #include #include +#include #include namespace xrpl::test { @@ -42,8 +45,9 @@ class LoanCashBasis_test : public LoanTestBase { private: // 1. LoanSet origination: Vault.AssetsTotal/LoanBroker.DebtTotal deltas, - // and the AssetsMaximum/DebtMaximum guards (which always check against - // principal + interestDue, regardless of the amendment). + // and the AssetsMaximum/DebtMaximum guards. Accrual AssetsMaximum still + // requires headroom for interestDue; cash-basis AssetsMaximum does not, + // because origination does not credit interest into AssetsTotal. void testCashBasisLoanSetOrigination() { @@ -226,6 +230,10 @@ private: // Even far less headroom than interestDue still succeeds, since // cash-basis origination never adds interest to AssetsTotal. runVaultGuard(all_ | featureLendingProtocolV1_1, oneDrop, tesSUCCESS); + // Fully subscribed: AssetsTotal == AssetsMaximum. Accrual preclaim + // used to refuse this; origination must still succeed because it + // does not change AssetsTotal. + runVaultGuard(all_ | featureLendingProtocolV1_1, Number{0}, tesSUCCESS); } // DebtMaximum guard: cash-basis projects principal-only DebtTotal; @@ -489,6 +497,252 @@ private: } } + // VaultSet must still succeed when cash-basis LoanPay has already pushed + // AssetsTotal above a nonzero AssetsMaximum. Before fixCleanup3_4_0, + // ValidVault rejects that with tecINVARIANT_FAILED even though + // VaultSet::doApply and the product rule allow the over-cap state when + // the excess is interest. + void + testVaultSetWhileAssetsTotalExceedsMaximum() + { + using namespace jtx; + using namespace loan; + + PrettyAsset const xrpAsset{xrpIssue(), 1'000'000}; + BrokerParameters const brokerParams{ + .vaultDeposit = 1'000'000, + .debtMax = 0, + .coverRateMin = TenthBips32{0}, + .coverDeposit = 0, + .managementFeeRate = TenthBips16{0}, + .coverRateLiquidation = TenthBips32{0}}; + + auto run = + [&](FeatureBitset features, TER expectedOverCapSet, bool native, bool vaultPrivate) { + bool const fix340Enabled = features[fixCleanup3_4_0]; + testcase( + std::string("cash-basis: VaultSet while AssetsTotal exceeds AssetsMaximum") + + (native ? " XRP" : " IOU") + (vaultPrivate ? " private" : "") + + (fix340Enabled ? " (fixCleanup3_4_0)" : " (pre-fix)")); + + Account const issuer{"issuer"}; + Account const lender{"lender"}; + Account const borrower{"borrower"}; + Env env(*this, features); + + BrokerParameters params = brokerParams; + if (vaultPrivate) + params.vaultFlags = tfVaultPrivate; + + PrettyAsset vaultAsset = xrpAsset; + if (native) + { + env.fund(XRP(10'000'000), lender, borrower); + env.close(); + } + else + { + vaultAsset = createFundedIouAsset(env, issuer, lender, borrower); + } + + BrokerInfo const broker{createVaultAndBroker(env, vaultAsset, lender, params)}; + auto const vaultBefore = env.le(broker.vaultKeylet()); + BEAST_EXPECT(vaultBefore); + // One unit at the vault's asset scale so the stored cap is + // strictly above AssetsTotal (a smaller ULP rounds away). + // Cash-basis origination does not credit interest, so LoanSet + // still succeeds. + Number const slack{1, -static_cast(vaultBefore->at(sfScale))}; + Number const assetsMaximum = Number(vaultBefore->at(sfAssetsTotal)) + slack; + + Vault const vault{env}; + { + auto tx = vault.set({.owner = lender, .id = broker.vaultID}); + tx[sfAssetsMaximum] = assetsMaximum; + env(tx); + env.close(); + } + + { + auto tx = vault.set({.owner = lender, .id = broker.vaultID}); + tx[sfData] = "AA"; + env(tx, Ter(tesSUCCESS)); + env.close(); + } + + auto const brokerBeforeLoan = env.le(broker.brokerKeylet()); + BEAST_EXPECT(brokerBeforeLoan); + auto const loanKeylet = keylet::loan( + broker.brokerID, SeqProxy::rawSequence(brokerBeforeLoan->at(sfLoanSequence))); + + LoanParameters const loanParams{ + .account = borrower, + .counter = lender, + .principalRequest = 12'000, + .interest = TenthBips32{percentageToTenthBips(12)}, + .payTotal = 4, + .payInterval = 600, + .gracePd = 300, + }; + env(loanParams(env, broker)); + env.close(); + + auto const vaultAfterLoan = env.le(broker.vaultKeylet()); + BEAST_EXPECT(vaultAfterLoan); + BEAST_EXPECT(vaultAfterLoan->at(sfAssetsTotal) <= assetsMaximum); + + LoanState const state = getCurrentState(env, broker, loanKeylet); + STAmount const payment{ + vaultAsset, + roundPeriodicPayment(vaultAsset, state.periodicPayment, state.loanScale) * + Number{3, -1} * 5}; + env(pay(borrower, loanKeylet.key, payment), Ter(tesSUCCESS)); + env.close(); + + auto const vaultAboveMaximum = env.le(broker.vaultKeylet()); + BEAST_EXPECT(vaultAboveMaximum); + BEAST_EXPECT(vaultAboveMaximum->at(sfAssetsTotal) > assetsMaximum); + BEAST_EXPECT(vaultAboveMaximum->at(sfAssetsMaximum) == assetsMaximum); + + { + auto tx = vault.set({.owner = lender, .id = broker.vaultID}); + tx[sfData] = "BB"; + env(tx, Ter(expectedOverCapSet)); + env.close(); + } + + if (vaultPrivate) + { + pdomain::Credentials const credentials{ + {.issuer = lender, .credType = "credential"}}; + env(pdomain::setTx(lender, credentials)); + auto const domainId = pdomain::getNewDomain(env.meta()); + auto tx = vault.set({.owner = lender, .id = broker.vaultID}); + tx[sfDomainID] = to_string(domainId); + env(tx, Ter(expectedOverCapSet)); + env.close(); + } + + if (!fix340Enabled) + return; + + { + auto tx = vault.set({.owner = lender, .id = broker.vaultID}); + tx[sfAssetsMaximum] = assetsMaximum; + env(tx, Ter(tecLIMIT_EXCEEDED)); + env.close(); + } + + { + auto tx = vault.set({.owner = lender, .id = broker.vaultID}); + tx[sfAssetsMaximum] = Number{0}; + env(tx, Ter(tesSUCCESS)); + env.close(); + } + }; + + FeatureBitset const withFix = all_ | featureLendingProtocolV1_1; + FeatureBitset const withoutFix = withFix - fixCleanup3_4_0; + + run(withFix, tesSUCCESS, true, true); + run(withoutFix, tecINVARIANT_FAILED, true, true); + run(withFix, tesSUCCESS, false, false); + run(withoutFix, tecINVARIANT_FAILED, false, false); + } + + void + testCashBasisLoanSetAfterInterestExceedsCap() + { + testcase("cash-basis: LoanSet after interest pushes AssetsTotal past AssetsMaximum"); + + using namespace jtx; + using namespace loan; + + PrettyAsset const xrpAsset{xrpIssue(), 1'000'000}; + BrokerParameters const brokerParams{ + .vaultDeposit = 1'000'000, + .debtMax = 0, + .coverRateMin = TenthBips32{0}, + .coverDeposit = 0, + .managementFeeRate = TenthBips16{0}, + .coverRateLiquidation = TenthBips32{0}}; + + Account const lender{"lender"}; + Account const borrower{"borrower"}; + Env env(*this, all_ | featureLendingProtocolV1_1); + env.fund(XRP(10'000'000), lender, borrower); + env.close(); + + BrokerInfo const broker{createVaultAndBroker(env, xrpAsset, lender, brokerParams)}; + auto const vaultBefore = env.le(broker.vaultKeylet()); + BEAST_EXPECT(vaultBefore); + Number const assetsMaximum = Number(vaultBefore->at(sfAssetsTotal)); + + Vault const vault{env}; + { + auto tx = vault.set({.owner = lender, .id = broker.vaultID}); + tx[sfAssetsMaximum] = assetsMaximum; + env(tx); + env.close(); + } + + auto const brokerBeforeLoan = env.le(broker.brokerKeylet()); + BEAST_EXPECT(brokerBeforeLoan); + auto const firstLoanKeylet = keylet::loan( + broker.brokerID, SeqProxy::rawSequence(brokerBeforeLoan->at(sfLoanSequence))); + + Number const firstPrincipal = xrpAsset(12'000).value(); + env(set(borrower, broker.brokerID, firstPrincipal), + kCounterparty(lender), + kInterestRate(TenthBips32{percentageToTenthBips(12)}), + kPaymentTotal(4), + kPaymentInterval(600), + Sig(sfCounterpartySignature, lender), + Fee(env.current()->fees().base * 2), + Ter(tesSUCCESS)); + env.close(); + + auto const vaultAfterFirst = env.le(broker.vaultKeylet()); + BEAST_EXPECT(vaultAfterFirst); + BEAST_EXPECT(vaultAfterFirst->at(sfAssetsTotal) == assetsMaximum); + BEAST_EXPECT(vaultAfterFirst->at(sfAssetsAvailable) == assetsMaximum - firstPrincipal); + + LoanState const state = getCurrentState(env, broker, firstLoanKeylet); + STAmount const payment{ + xrpAsset, + roundPeriodicPayment(xrpAsset, state.periodicPayment, state.loanScale) * Number{3, -1} * + 5}; + env(pay(borrower, firstLoanKeylet.key, payment), Ter(tesSUCCESS)); + env.close(); + + auto const vaultAfterPay = env.le(broker.vaultKeylet()); + BEAST_EXPECT(vaultAfterPay); + BEAST_EXPECT(vaultAfterPay->at(sfAssetsTotal) > assetsMaximum); + BEAST_EXPECT(vaultAfterPay->at(sfAssetsAvailable) > beast::kZero); + + auto const brokerAfterPay = env.le(broker.brokerKeylet()); + BEAST_EXPECT(brokerAfterPay); + auto const secondLoanKeylet = keylet::loan( + broker.brokerID, SeqProxy::rawSequence(brokerAfterPay->at(sfLoanSequence))); + + Number const secondPrincipal = xrpAsset(1'000).value(); + env(set(borrower, broker.brokerID, secondPrincipal), + kCounterparty(lender), + kInterestRate(TenthBips32{percentageToTenthBips(12)}), + kPaymentTotal(4), + kPaymentInterval(600), + Sig(sfCounterpartySignature, lender), + Fee(env.current()->fees().base * 2), + Ter(tesSUCCESS)); + env.close(); + + auto const vaultAfterSecond = env.le(broker.vaultKeylet()); + auto const secondLoan = env.le(secondLoanKeylet); + BEAST_EXPECT(vaultAfterSecond && secondLoan); + BEAST_EXPECT(vaultAfterSecond->at(sfAssetsTotal) == vaultAfterPay->at(sfAssetsTotal)); + BEAST_EXPECT(secondLoan->at(sfPrincipalOutstanding) == secondPrincipal); + } + // 3. LoanManage: impair, unimpair, and default. void testCashBasisLoanManage() @@ -1013,6 +1267,8 @@ public: { testCashBasisLoanSetOrigination(); testCashBasisLoanPay(); + testVaultSetWhileAssetsTotalExceedsMaximum(); + testCashBasisLoanSetAfterInterestExceedsCap(); testCashBasisLoanManage(); testLegacyVaultKeepsAccrualAfterAmendmentEnabled(); testCashBasisEndToEndTrajectory(); diff --git a/src/test/app/lending/LoanTestBase.h b/src/test/app/lending/LoanTestBase.h index c9d4a3185b..a1241d804e 100644 --- a/src/test/app/lending/LoanTestBase.h +++ b/src/test/app/lending/LoanTestBase.h @@ -101,6 +101,10 @@ protected: TenthBips32 coverRateLiquidation = percentageToTenthBips(25); std::string data = {}; // NOLINT(readability-redundant-member-init) std::uint32_t flags = 0; + // VaultCreate flags (e.g. tfVaultPrivate). Distinct from `flags`, + // which are passed to LoanBrokerSet. + std::optional vaultFlags = + std::nullopt; // NOLINT(readability-redundant-member-init) // 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 = @@ -526,6 +530,7 @@ protected: auto [tx, vaultKeylet] = vault.create( {.owner = lender, .asset = asset, + .flags = params.vaultFlags, .vaultKind = effectiveVaultKind == VaultKind::OpenEnded ? std::optional{} : std::optional{std::to_underlying(effectiveVaultKind)},