Skip to content
This repository was archived by the owner on Mar 11, 2025. It is now read-only.

lending: Use fair obligation health factor calculation #1119

Merged
merged 2 commits into from
Jan 28, 2021

Conversation

jstarry
Copy link
Contributor

@jstarry jstarry commented Jan 22, 2021

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

  • Use the best price in the order book to determine the health of an obligation but continue using order book depth to calculate a fair market value of of collateral to send to liquidators. This leaves large obligations much less vulnerable to order book manipulation and provides an efficient way to determine the health of a loan. An additional win is that the amount of liquidity a liquidator wants to pay off has no impact on the determination about an obligation's health.
  • Add special handling for liquidating dust obligations. When an obligation has less than a single token unit of borrowed value remaining, allow a liquidator to pay it off in return for the remaining collateral.

joncinque
joncinque previously approved these changes Jan 22, 2021
Copy link
Contributor

@joncinque joncinque left a 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.

@mergify mergify bot dismissed joncinque’s stale review January 27, 2021 09:20

Pull request has been modified.

@jstarry
Copy link
Contributor Author

jstarry commented Jan 27, 2021

@joncinque do you mind taking a look at the latest changes? In writing your requested tests I found some holes.

  • Repay amount for dust accounts was hard coded to 1 but should be the ceiling of the decimal borrow amount
  • If decimal borrow amount was between 1 and 2, we weren't enforcing that the input liquidity amount was greater than 1.
  • There were some edge cases where unhealthy loans couldn't be liquidated, I fixed those and refactored the test to ensure that all unhealthy loans can be liquidated.
  • Decided that splitting off the bonus amount wasn't worth the compute cost and by combining the bonus and withdraw amounts, we're now giving liquidators a more fair amount of collateral (rather than rounding down separately)

@joncinque
Copy link
Contributor

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 {
Copy link
Contributor

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?

Copy link
Contributor Author

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);
Copy link
Contributor

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 😄

Copy link
Contributor Author

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);
Copy link
Contributor

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?

Copy link
Contributor Author

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.

Copy link
Contributor

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

Comment on lines 142 to 145
let max_repay_amount = obligation
.borrowed_liquidity_wads
.try_mul(Rate::from_percent(LIQUIDATION_CLOSE_FACTOR))?
.try_floor_u64()?;
Copy link
Contributor

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()?

Copy link
Contributor Author

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,
Copy link
Contributor

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, fixed

Copy link
Contributor Author

@jstarry jstarry left a 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!

@jstarry jstarry merged commit f61d7a8 into solana-labs:master Jan 28, 2021
@jstarry jstarry deleted the obligation-health branch January 28, 2021 07:56
@jstarry
Copy link
Contributor Author

jstarry commented Jan 28, 2021

feedback changes can be found here: ca5e43f

@joncinque
Copy link
Contributor

Looks good

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants