ARCHIVE/rippled
1
0
Fork 0
mirror of https://github.com/XRPLF/rippled.git synced 2026-09-27 15:28:03 +00:00
Code Issues Packages Projects Releases Wiki Activity

fix: Skip canAddHolding in LoanSet when the holding already exists

Browse Source
This commit is contained in:
Vito
2026-09-02 10:55:57 +02:00
parent 02b6e2b653
commit 6428435882
6 changed files with 216 additions and 26 deletions
Download Patch File Download Diff File Expand all files Collapse all files

14
include/xrpl/ledger/helpers/RippleStateHelpers.h
View File

@@ -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(

5
src/libxrpl/ledger/helpers/RippleStateHelpers.cpp
View File

@@ -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.

14
src/libxrpl/tx/transactors/lending/LoanSet.cpp
View File

@@ -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))

65
src/test/app/lending/LoanPay_test.cpp
View File

@@ -37,6 +37,7 @@
#include <cstdint>
#include <limits>
#include <memory>
#include <string>
#include <type_traits>
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<Issue>())));
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().

61
src/test/app/lending/LoanSet_test.cpp
View File

@@ -31,6 +31,7 @@
#include <array>
#include <cstdint>
#include <functional>
#include <string>
#include <utility>
#include <vector>
@@ -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<Issue>())));
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();
}
};

83
src/test/app/vault/VaultBugs_test.cpp
View File

@@ -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<Issue>();
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