-
Notifications
You must be signed in to change notification settings - Fork 229
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
Loan Contract #1903
Loan Contract #1903
Conversation
0356d20
to
e3714bd
Compare
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 is nicely structured. I appreciate all the distinct source files, each well-commented.
This looks like a good approach. The main thing (other than just unfinished stuff) is that when liquidation is triggered, or the collateral changes, the code needs to recalculate the trigger levels and reschedule the trigger.
I've finished reading the contract code, and have the types and tests still to read.
Oh good catch, I will fix |
Remaining to-do: address liquidations re: comments from @Chris-Hibbert |
a46d996
to
58b7412
Compare
Adjust to allow loan to take mocked price from Chainlink instead of internal autoswap price authority |
006a370
to
60ebe8a
Compare
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.
mostly minor enhancements to doc or type info.
I suggest a few additional tests, some of which might point to code that doesn't exist. Would you look at these and see if any are needed before people start using the contract?
// TODO: implement | ||
getPriceNotifier: () => { | ||
const { notifier } = makeNotifierKit(); | ||
return notifier; | ||
}, |
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 has been added in #1985.
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.
Looks like your version will get in first, and I'll integrate when I merge 1985.
|
||
import { trade } from '../../../../src/contractSupport'; | ||
|
||
test('test doLiquidation with mocked autoswap', async t => { |
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.
additional test:
- autoswap fails
- multiple scheduled liquidations don't interfere
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.
'Multiple scheduled liquidations' is already tested.
For 'autoswap fails' - do you mean the trade fails? That is not tested.
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.
Sorry, I don't see a test that verifies that the two liquidations from an initial borrow and adding collateral don't interfere. Which test are you referring to?
For 'autoswap fails' - do you mean the trade fails?
Yeah, with autoswap, that pair might not have been initialized, or all the collateral could have been removed. There's no guarantee that autoswap will always succeed. Particularly in the start-up sequence, it's conceivable that a pair of currencies could be missing.
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.
Sorry, I don't see a test that verifies that the two liquidations from an initial borrow and adding collateral don't interfere. Which test are you referring to?
'borrow, then addCollateral, then getLiquidationPromise'
performAddCollateral, | ||
} from './helpers'; | ||
|
||
test.todo('makeAddCollateralInvitation - test bad proposal'); |
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.
additionally:
- can we verify that a new liquidation trigger was rescheduled?
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.
Yes, that is tested already.
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.
See 'borrow, then addCollateral, then getLiquidationPromise'
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 tests ends with t.falsy(liquidated);
, so I think it shows that neither was triggered. I'm interested in the case where both price levels are exceeded, but only the rescheduled one should cause liquidation.
value is at 12
borrow 100 with 16 of collateral. (12 * 16 > 1.5 * 100)
value goes to 10 (10 * 16 > 1.5 * 100)
addCollateral of 10
value goes to 8; with 16 collateral, would have liquidated (8 * 16 < 1.5 * 100)
value goes to 5; now we should actually liquidate. (5 * 26 < 1.5 * 100)
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.
Oh, good point. That liquidated
is old - the liquidation code doesn't call that anymore. Let me revise that test.
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.
Liquidation is added to that test
@Chris-Hibbert Ready for another review |
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.
Thanks for the updates. This looks good now.
// TODO: implement | ||
getPriceNotifier: () => { | ||
const { notifier } = makeNotifierKit(); | ||
return notifier; | ||
}, |
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.
Looks like your version will get in first, and I'll integrate when I merge 1985.
Co-authored-by: Chris Hibbert <[email protected]>
Co-authored-by: Chris Hibbert <[email protected]>
Co-authored-by: Chris Hibbert <[email protected]>
Adds a basic loan contract using a priceAuthority, with tests.
Add collateral of a particular brand and get a loan of another brand. Collateral (as known as margin) must be greater than the loan value, at an amount set by the Maintenance Margin Requirement (mmr) in the terms of the contract. The loan does not have a distinct end time. Rather, if the value of the collateral changes such that insufficient margin is provided, the collateral is liquidated, and the loan is closed. At any time, the borrower can add collateral or repay the loan with interest, closing the loan.
The borrower can set up their own margin calls by getting the
priceAuthority
from the terms and callingE(priceAuthority).quoteWhenLT(allCollateral, x)
where x is the value of the collateral in the Loan brand at which they want a reminder to addCollateral.Note that all collateral must be of the same brand and all of the
loaned amount and interest must be of the same (separate) brand.
Terms:
percent. The default is 150, meaning that collateral should be
worth at least 150% of the loan. If the value of the collateral
drops below mmr, liquidation occurs.
collateral and setting liquidation triggers.
or Multipool Autoswap installation. The publicFacet of the
instance is used for producing an invitation to sell the
collateral on liquidation.
that a period has passed, on which compound interest will be
calculated using the interestRate.
with the debt on every period to compound interest.
IssuerKeywordRecord:
Keyword: 'Collateral' - The issuer for the digital assets to be
escrowed as collateral.
Keyword: 'Loan' - The issuer for the digital assets to be loaned
out.
Closes #1942
Closes #1939