Skip to content

adds a check to ensure vault asset cap is not exceeded #6124

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
Tapanito wants to merge 4 commits into
base: ximinez/lending-XLS-66-ongoing
Choose a base branch
from
+31 −5
Open

adds a check to ensure vault asset cap is not exceeded #6124

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
4109cb5
adds a check to ensure vault asset cap is not exceeded
Tapanito Dec 8, 2025
52948a9
fixes clang-format..
Tapanito Dec 8, 2025
bd1cea0
Merge branch 'ximinez/lending-XLS-66-ongoing' into tapanito/loan-pay-…
Tapanito Dec 15, 2025
9570222
Merge branch 'ximinez/lending-XLS-66-ongoing' into tapanito/loan-pay-…
Tapanito Dec 17, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 20 additions & 0 deletions src/test/app/Loan_test.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3590,6 +3590,26 @@ class Loan_test : public beast::unit_test::suite
fee(env.current()->fees().base * 5));
},
CaseArgs{.requireAuth = true, .authorizeBorrower = true});

testCase(
[&, this](Env& env, BrokerInfo const& broker, auto&) {
using namespace loan;
Number const principalRequest = broker.asset(1'000).value();
Vault vault{env};
auto tx = vault.set({.owner = lender, .id = broker.vaultID});
tx[sfAssetsMaximum] = BrokerParameters::defaults().vaultDeposit;
env(tx);
env.close();

testcase("Vault maximum value exceeded");
env(set(issuer, broker.brokerID, principalRequest),
counterparty(lender),
interestRate(TenthBips32(10'000)),
sig(sfCounterpartySignature, lender),
fee(env.current()->fees().base * 5),
ter(tecLIMIT_EXCEEDED));
},
nullptr);
Comment on lines +3594 to +3612
Copy link
Collaborator

@ximinez ximinez Dec 17, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it would be good to have another test case where the loan has interest, and the principal request is under the limit, but the interest drives it over.

Copy link
Collaborator Author

@Tapanito Tapanito Dec 17, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This testcase is exactly what you describe.

We set the Vault max assets to 1,000,000
tx[sfAssetsMaximum] = BrokerParameters::defaults().vaultDeposit; <=== 1,000,000

We request principal of 1,000, but the interest pushes the TotalValue above the limit of 1,000,000

}

void
Expand Down
16 changes: 11 additions & 5 deletions src/xrpld/app/tx/detail/LoanSet.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -407,6 +407,17 @@ LoanSet::doApply()
vaultScale,
j_);

LoanState const state = constructLoanState(
properties.totalValueOutstanding,
principalRequested,
properties.managementFeeOwedToBroker);

if (vaultSle->at(sfAssetsMaximum) != 0 &&
vaultTotalProxy + state.interestDue > vaultSle->at(sfAssetsMaximum))
{
JLOG(j_.warn()) << "Loan would exceed the maximum assets of the vault";
return tecLIMIT_EXCEEDED;
}
Comment on lines +415 to +420
Copy link
Collaborator

@ximinez ximinez Dec 17, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we want to leave some extra buffer here? A late payment, for example, could increase the vault total over assets maximum. Those things are hard to predict, though, so I'm not sure what a reasonable amount would be.

Copy link
Collaborator Author

@Tapanito Tapanito Dec 17, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's okay, a LoanPay transaction is allowed to exceed the Vault asset limit, precisely for the reason you mention: it's unpredictable.

This is similar to freezing logic, where if the LoanBroker is frozen during the loan creation, the transaction fails, but if the LoanBroker is frozen during LoanPay, then we redirect the funds to the LoanBrokerCover.

// Check that relevant values won't lose precision. This is mostly only
// relevant for IOU assets.
{
Expand Down Expand Up @@ -449,11 +460,6 @@ LoanSet::doApply()
// LCOV_EXCL_STOP
}

LoanState const state = constructLoanState(
properties.totalValueOutstanding,
principalRequested,
properties.managementFeeOwedToBroker);

auto const originationFee = tx[~sfLoanOriginationFee].value_or(Number{});

auto const loanAssetsToBorrower = principalRequested - originationFee;
Expand Down
Loading