diff --git a/include/xrpl/ledger/helpers/RippleStateHelpers.h b/include/xrpl/ledger/helpers/RippleStateHelpers.h index 5350182cff..9ee96b0060 100644 --- a/include/xrpl/ledger/helpers/RippleStateHelpers.h +++ b/include/xrpl/ledger/helpers/RippleStateHelpers.h @@ -239,12 +239,14 @@ canTransfer(ReadView const& view, Issue const& issue, AccountID const& from, Acc //------------------------------------------------------------------------------ /** - * If the destination already holds this IOU, returns tecDUPLICATE and does - * not consult issuer freeze or DefaultRipple (post-fixCleanup3_4_0). - * Transactors that may create a new holding in doApply must call - * canAddHolding() in preclaim with the same View and Asset. Do not call - * canAddHolding() merely because addEmptyHolding() is invoked: that helper - * does not check whether a holding already exists. + * After fixCleanup3_4_0, if the destination already holds this IOU, returns + * tecDUPLICATE and does not consult issuer freeze or DefaultRipple. Freeze + * and DefaultRipple still apply on the create path (DefaultRipple off is + * terNO_RIPPLE). Transactors that may create a new holding in doApply must + * call canAddHolding() in preclaim only when that destination does not + * already hold the asset. Do not call canAddHolding() merely because + * addEmptyHolding() is invoked: that helper does not check whether a + * holding already exists. */ [[nodiscard]] TER addEmptyHolding( diff --git a/src/libxrpl/ledger/helpers/RippleStateHelpers.cpp b/src/libxrpl/ledger/helpers/RippleStateHelpers.cpp index b1e6a47f19..cc02b56305 100644 --- a/src/libxrpl/ledger/helpers/RippleStateHelpers.cpp +++ b/src/libxrpl/ledger/helpers/RippleStateHelpers.cpp @@ -669,6 +669,11 @@ addEmptyHolding( auto const sleDst = ctx.view.peek(keylet::account(dstId)); if (!sleDst || !sleSrc) return tefINTERNAL; // LCOV_EXCL_LINE + // Create path: DefaultRipple is still required. terNO_RIPPLE is + // intentional so VaultWithdraw / CoverWithdraw fail in preclaim via + // canAddHolding (retryable, no fee) rather than claiming a tec* fee + // in doApply. Transactor::operator() will not apply and will not + // convert it to tefINTERNAL. if (!sleSrc->isFlag(lsfDefaultRipple)) return fix340Enabled ? TER{terNO_RIPPLE} : tecINTERNAL; // If the line already exists, don't create it again. diff --git a/src/libxrpl/tx/transactors/lending/LoanSet.cpp b/src/libxrpl/tx/transactors/lending/LoanSet.cpp index b67c244bac..9ad76212f6 100644 --- a/src/libxrpl/tx/transactors/lending/LoanSet.cpp +++ b/src/libxrpl/tx/transactors/lending/LoanSet.cpp @@ -372,8 +372,18 @@ LoanSet::preclaim(PreclaimContext const& ctx) } } - if (auto const ter = canAddHolding(ctx.view, asset)) - return ter; + // canAddHolding does not look at existing lines. After + // fixCleanup3_4_0, addEmptyHolding is a no-op when the destination + // already holds the asset, so skip this gate unless a holding would + // actually be created (borrower always; broker owner if there is an + // origination fee). + auto const originationFee = tx[~sfLoanOriginationFee].value_or(Number{}); + if (!ctx.view.rules().enabled(fixCleanup3_4_0) || !holdingExists(ctx.view, borrower, asset) || + (originationFee != beast::kZero && !holdingExists(ctx.view, brokerOwner, asset))) + { + if (auto const ter = canAddHolding(ctx.view, asset)) + return ter; + } // vaultPseudo is going to send funds, so it can't be frozen. if (auto const ret = checkFrozen(ctx.view, vaultPseudo, asset)) diff --git a/src/test/app/lending/LoanPay_test.cpp b/src/test/app/lending/LoanPay_test.cpp index 038ef4067b..3360e68272 100644 --- a/src/test/app/lending/LoanPay_test.cpp +++ b/src/test/app/lending/LoanPay_test.cpp @@ -37,6 +37,7 @@ #include #include #include +#include #include namespace xrpl::test { @@ -1419,6 +1420,69 @@ private: BEAST_EXPECT(stateAfter.nextPaymentDate == exactDueDate); } + // LoanPay does not call canAddHolding. addEmptyHolding recreates the + // broker-owner holding when the borrower is also the broker owner. After + // fixCleanup3_4_0 an existing line is a no-op even if DefaultRipple is + // off; pre-fix that path dies with tecINTERNAL. + void + testLoanPaySelfBrokerExistingLineDefaultRipple() + { + using namespace jtx; + using namespace loan; + + auto run = [this](FeatureBitset features, TER expected) { + testcase( + std::string( + "LoanPay broker-owner borrower existing line after " + "issuer clears asfDefaultRipple (") + + (features[fixCleanup3_4_0] ? "post" : "pre") + "-fixCleanup3_4_0)"); + + Env env(*this, features); + Account const issuer{"issuer"}; + Account const alice{"alice"}; + + env.fund(XRP(10'000), issuer, alice); + env.close(); + env(fset(issuer, asfDefaultRipple)); + env.close(); + + PrettyAsset const usd{issuer["USD"]}; + env(trust(alice, usd(100'000))); + env.close(); + env(pay(issuer, alice, usd(50'000))); + env.close(); + + auto const broker = createVaultAndBroker(env, usd, alice); + auto const brokerSle = env.le(keylet::loanBroker(broker.brokerID)); + if (!BEAST_EXPECT(brokerSle)) + return; + auto const loanKeylet = + keylet::loan(broker.brokerID, SeqProxy::rawSequence(brokerSle->at(sfLoanSequence))); + + Number const serviceFee = usd(2).value(); + env(set(alice, broker.brokerID, usd(1'000).value()), + Sig(sfCounterpartySignature, alice), + kLoanServiceFee(serviceFee), + Fee(env.current()->fees().base * 2)); + env.close(); + + env(fclear(issuer, asfDefaultRipple)); + env.close(); + BEAST_EXPECT(env.le(keylet::trustLine(alice.id(), usd.raw().get()))); + + auto const state = getCurrentState(env, broker, loanKeylet); + STAmount const payment{ + usd, + roundPeriodicPayment(usd, state.periodicPayment + serviceFee, state.loanScale)}; + + env(pay(alice, loanKeylet.key, payment), Ter(expected)); + env.close(); + }; + + run(all_ - fixCleanup3_4_0, tecINTERNAL); + run(all_, tesSUCCESS); + } + void runAmendmentIndependent() { @@ -1429,6 +1493,7 @@ private: testLoanPayCatchUpFeeAtExactDueDatePostAmendment(); testLoanPayCatchUpFeeAtExactDueDatePreAmendment(); testRepayIntoUnauthorizedVault(); + testLoanPaySelfBrokerExistingLineDefaultRipple(); } // Tests run under each entry in amendmentCombinations(). diff --git a/src/test/app/lending/LoanSet_test.cpp b/src/test/app/lending/LoanSet_test.cpp index 9829f23138..9e2202a2bb 100644 --- a/src/test/app/lending/LoanSet_test.cpp +++ b/src/test/app/lending/LoanSet_test.cpp @@ -31,6 +31,7 @@ #include #include #include +#include #include #include @@ -764,6 +765,65 @@ private: }); } + // LoanSet used to call canAddHolding unconditionally, so an existing + // borrower line still failed with terNO_RIPPLE after the issuer cleared + // DefaultRipple. After fixCleanup3_4_0, skip that gate when the holding + // already exists. + void + testLoanSetExistingLineAfterIssuerClearsDefaultRipple() + { + using namespace jtx; + using namespace loan; + + auto run = [this](FeatureBitset features, TER expected) { + testcase( + std::string( + "LoanSet existing borrower line after issuer " + "clears asfDefaultRipple (") + + (features[fixCleanup3_4_0] ? "post" : "pre") + "-fixCleanup3_4_0)"); + + Env env(*this, features); + Account const issuer{"issuer"}; + Account const lender{"lender"}; + Account const borrower{"borrower"}; + + env.fund(XRP(10'000), issuer, lender, borrower); + env.close(); + env(fset(issuer, asfDefaultRipple)); + env.close(); + + PrettyAsset const usd{issuer["USD"]}; + env(trust(lender, usd(100'000))); + env(trust(borrower, usd(100'000))); + env.close(); + env(pay(issuer, lender, usd(50'000))); + env(pay(issuer, borrower, usd(1'000))); + env.close(); + BEAST_EXPECT(env.le(keylet::trustLine(borrower.id(), usd.raw().get()))); + + auto const broker = createVaultAndBroker(env, usd, lender); + + env(fclear(issuer, asfDefaultRipple)); + env.close(); + + Number const destBefore = env.balance(borrower, usd.raw()).number(); + env(set(borrower, broker.brokerID, usd(100).value()), + Sig(sfCounterpartySignature, lender), + Fee(env.current()->fees().base * 2), + Ter(expected)); + env.close(); + + Number const destAfter = env.balance(borrower, usd.raw()).number(); + if (isTesSuccess(expected)) + BEAST_EXPECT(destAfter == destBefore + Number{100}); + else + BEAST_EXPECT(destAfter == destBefore); + }; + + run(all_ - fixCleanup3_4_0, terNO_RIPPLE); + run(all_, tesSUCCESS); + } + public: void run() override @@ -773,6 +833,7 @@ public: testLoanSet(features); testLoanSetClosedEnded(); + testLoanSetExistingLineAfterIssuerClearsDefaultRipple(); } }; diff --git a/src/test/app/vault/VaultBugs_test.cpp b/src/test/app/vault/VaultBugs_test.cpp index c0983511ad..5b6e756e59 100644 --- a/src/test/app/vault/VaultBugs_test.cpp +++ b/src/test/app/vault/VaultBugs_test.cpp @@ -1869,23 +1869,67 @@ private: } }; - auto runPrivateVault = [this](FeatureBitset features, TER selfExpected, TER destExpected) { + auto runDeletedCoverWithdraw = [this](FeatureBitset features, TER selfExpected) { + using namespace loan_broker; + + Env env(*this, features); + Account const issuer{"issuer"}; + Account const alice{"alice"}; + + env.fund(XRP(10'000), issuer, alice); + env.close(); + env(fset(issuer, asfDefaultRipple)); + env.close(); + + PrettyAsset const usd{issuer["USD"]}; + Issue const usdIssue = usd.raw().get(); + env(trust(alice, usd(10'000))); + env.close(); + env(pay(issuer, alice, usd(600))); + env.close(); + + Vault const vault{env}; + auto const [createTx, vaultKeylet, subscriptionDate] = vault.createClosedEnded( + {.owner = alice, .asset = usd, .subscriptionOffset = std::chrono::seconds{60}}); + (void)subscriptionDate; + env(createTx); + env.close(); + + env(vault.deposit({.depositor = alice, .id = vaultKeylet.key, .amount = usd(500)})); + env.close(); + + auto const brokerKeylet = + keylet::loanBroker(alice.id(), SeqProxy::rawSequence(env.seq(alice))); + env(set(alice, vaultKeylet.key)); + env.close(); + env(coverDeposit(alice, brokerKeylet.key, usd(100).value())); + env.close(); + + env(trust(alice, usd(0))); + env.close(); + BEAST_EXPECT(!env.le(keylet::trustLine(alice.id(), usdIssue))); + env(fclear(issuer, asfDefaultRipple)); + env.close(); + + env(coverWithdraw(alice, brokerKeylet.key, usd(50).value()), Ter(selfExpected)); + env.close(); + }; + + auto runPrivateVault = [this](FeatureBitset features, TER selfExpected) { Env env(*this, features); Account const issuer{"issuer"}; Account const alice{"alice"}; - Account const bob{"bob"}; Account const pdOwner{"pdOwner"}; Account const credIssuer{"credIssuer"}; std::string const credType = "credential"; - env.fund(XRP(10'000), issuer, alice, bob, pdOwner, credIssuer); + env.fund(XRP(10'000), issuer, alice, pdOwner, credIssuer); env.close(); env(fset(issuer, asfDefaultRipple)); env.close(); PrettyAsset const usd{issuer["USD"]}; env(trust(alice, usd(10'000))); - env(trust(bob, usd(10'000))); env.close(); env(pay(issuer, alice, usd(1'000))); env.close(); @@ -1919,14 +1963,6 @@ private: env(vault.withdraw({.depositor = alice, .id = vaultKeylet.key, .amount = usd(50)}), Ter(selfExpected)); env.close(); - - // Bob has a USD line but is not in the vault's domain; post-fixCleanup3_4_0 - // that is not a usable destination. - auto destTx = - vault.withdraw({.depositor = alice, .id = vaultKeylet.key, .amount = usd(50)}); - destTx[sfDestination] = bob.human(); - env(destTx, Ter(destExpected)); - env.close(); }; testcase( @@ -1969,16 +2005,27 @@ private: runCoverWithdraw(all_, tesSUCCESS); testcase( - "bug: private VaultWithdraw to self fails with tecINTERNAL after " - "issuer clears asfDefaultRipple; third-party dest still works " + "bug: LoanBrokerCoverWithdraw to self fails with tecINTERNAL after " + "issuer clears asfDefaultRipple and the trust line was deleted " "(pre-fixCleanup3_4_0)"); - runPrivateVault(all_ - fixCleanup3_4_0, tecINTERNAL, tesSUCCESS); + runDeletedCoverWithdraw(all_ - fixCleanup3_4_0, tecINTERNAL); + + testcase( + "bug: LoanBrokerCoverWithdraw to self fails with terNO_RIPPLE after " + "issuer clears asfDefaultRipple and the trust line was deleted " + "(post-fixCleanup3_4_0)"); + runDeletedCoverWithdraw(all_, terNO_RIPPLE); + + testcase( + "bug: private VaultWithdraw to self fails with tecINTERNAL after " + "issuer clears asfDefaultRipple even though the trust line exists " + "(pre-fixCleanup3_4_0)"); + runPrivateVault(all_ - fixCleanup3_4_0, tecINTERNAL); testcase( "bug: private VaultWithdraw to self succeeds after issuer clears " - "asfDefaultRipple; third-party dest is tecNO_AUTH " - "(post-fixCleanup3_4_0)"); - runPrivateVault(all_, tesSUCCESS, tecNO_AUTH); + "asfDefaultRipple when the trust line exists (post-fixCleanup3_4_0)"); + runPrivateVault(all_, tesSUCCESS); } // Bug 1: a sponsored XRP VaultWithdraw to a distinct destination is