-
Notifications
You must be signed in to change notification settings - Fork 2.3k
lending: Use fair obligation health factor calculation #1119
Conversation
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 good! Just two small questions really, which don't need to be done in this PR.
Pull request has been modified.
6e8a7ca
to
04415b1
Compare
@joncinque do you mind taking a look at the latest changes? In writing your requested tests I found some holes.
|
Looks really good, just some questions and nits |
|
||
// Creates a range of reasonable collateral exchange rates | ||
prop_compose! { | ||
fn collateral_exchange_rate_range()(rate in 1..=500u64) -> CollateralExchangeRate { |
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.
Is 500 here meant to be 500%, the starting collateral exchange rate?
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, exactly. I'll rename rate
to percent
let collateral_value = collateral_exchange_rate | ||
.decimal_collateral_to_liquidity(self.deposited_collateral_tokens.into())? | ||
.try_div(borrow_token_price)?; | ||
println!("loan: {}, value: {}", loan, collateral_value); |
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.
👀 the sign of debugging a lot of proptests 😄
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.
eek, fixed
// Special handling for dust obligations | ||
if max_repayable == 0 { | ||
// Special handling for small, closeable obligations | ||
let close_amount = liquidity_amount.min(CLOSEABLE_AMOUNT); |
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 totally understand this number. Is the idea that loans with more than 1 in wad but less than 2 can still be dust accounts? And there's no case where loans of more than 2 wads could be dust accounts?
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, there was a bug here where the close_amount
could be too high.
Sorry, I don't totally understand this number
Added a new method for calculating this number and documented it like this:
/// Maximum amount of loan that can be closed out by a liquidator due
/// to the remaining balance being too small to be liquidated normally.
pub fn max_closeable_amount(&self) -> Result<u64, ProgramError> {
Is that clearer?
Is the idea that loans with more than 1 in wad but less than 2 can still be dust accounts?
Idea is that any loan less than 2 wads are dust accounts and can be closed because the can not be liquidated
And there's no case where loans of more than 2 wads could be dust accounts?
Correct, if the loan is greater than or equal to 2 wads, half of the loan can be liquidated.
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.
Right, of course, I forgot about the 50% max liquidation! Ok great, thanks for the explanation
let max_repay_amount = obligation | ||
.borrowed_liquidity_wads | ||
.try_mul(Rate::from_percent(LIQUIDATION_CLOSE_FACTOR))? | ||
.try_floor_u64()?; |
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.
nit: is this meant to use max_liquidation_amount()
?
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, not sure how that happened.
liquidation_bonus in 0..=100u8, | ||
liquidation_threshold in 2..=100u8, | ||
fn unhealthy_obligations_can_be_liquidated( | ||
obligation_collateral in 1..=u32::MAX, |
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.
Should this be u64::MAX
?
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, fixed
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 detailed feedback @joncinque!
fe3919c
to
b4fc08d
Compare
feedback changes can be found here: ca5e43f |
Looks good |
Problem
Loans that are large in value relative to the depth of the dex order book are prone to having their collateral devalued which leads to a higher LTV ratio and potential opportunity for liquidations.
Changes