-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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.
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
base: ximinez/lending-XLS-66-ongoing
Are you sure you want to change the base?
adds a check to ensure vault asset cap is not exceeded #6124
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## ximinez/lending-XLS-66-ongoing #6124 +/- ##
================================================================
- Coverage 79.1% 79.1% -0.0%
================================================================
Files 839 839
Lines 71385 71389 +4
Branches 8317 8338 +21
================================================================
- Hits 56481 56460 -21
- Misses 14904 14929 +25
🚀 New features to boost your workflow:
|
gregtatcam
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
| 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; | ||
| } |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
| 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); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
Adds a check to ensure vault asset cap is not exceeded when creating a Loan.