feat!: liquidation directly rewards base assets #1118
Conversation
Codecov Report
@@ Coverage Diff @@
## main #1118 +/- ##
==========================================
- Coverage 51.54% 51.17% -0.37%
==========================================
Files 65 67 +2
Lines 6585 6654 +69
==========================================
+ Hits 3394 3405 +11
- Misses 2928 3009 +81
+ Partials 263 240 -23
|
There was a problem hiding this comment.
Actually, I think we should not reduce available_supply by directly giving a tokenA (instead of uToken) to the liquidator. We want available supply to stay high (to optimize collateral_utilization(tokenA)).
So, I think we should:
- keep the liquidation logic as is (liquidator receives uTokens)
- round down when distributing rewards.
| string liquidator = 1; | ||
| string borrower = 2; | ||
| cosmos.base.v1beta1.Coin repayment = 3 [(gogoproto.nullable) = false]; | ||
| string reward_denom = 4; |
There was a problem hiding this comment.
We need to think more about this change and this PR. In particular, I think this should be repeated Coin, see: #1129
There was a problem hiding this comment.
Considering the level of mathematical complexity that single-denom liquidations involve (see below), I prefer avoiding sdk.Coins in either repayment or reward.
It does increase the number of MsgLiquidate required for complex positions unfortunately, but keeps the edge cases much more understandable.
There was a problem hiding this comment.
In that case the complexity is moved to liquidator... he will need to calculate which denom he can liquidate and how much.
We are already adjusting the repayment. Instead we can go one by one of the collateral list and sum up to the value related to the repayment. We could use denom list to make it simpler.
There was a problem hiding this comment.
Yes, the liquidator can do that - I had a script doing as much during the testnet, and what it does is proceed in order of preference for repay and reward denoms by offering the maximum amount it is willing to repay.
For most borrowers, even with multiple borrowed and collateral types, the first MsgLiquidate brings them back to health, so the liquidator's initial preference for reward and repay denoms is enough.
For borrowers where multiple transactions are needed, the liquidator only needs to follow their ranked order of preferences, because the first transaction will have either exhausted a repay denom or a reward denom by succeeding.
Note: Exhausted a denom can mean:
- Repaid a borrow completely in one token denom
- Reduced liquidator's token balance to zero (or consumed maximum offered amount) for a token denom
- Reduced borrower's collateral position in a uToken denom to zero
- Exhausted available supply of a reward token
There was a problem hiding this comment.
Do we have a test verifying if the liquidation of a single asset, even if it doesn't bring the account to a healthy state, is possible?
There was a problem hiding this comment.
Liquidation logic is independent of final state (except for the collateral == 0 check for bad debt flagging) so that isn't a problem.
In particular, the test case labeled repayLimited covers this scenario in liquidate_test.go
Suppose we have a situation where collateral utilization has reached its limit. In this case, However, there is no way to distinguish between a supplier and a liquidator if they both have to use |
This change to rounding up is related to a dust problem I witnesses on chain during umeemania. Consider the following scenario
Note the microatom and microosmo, which cannot be subdivided further. The transaction
The priority for rounding during liquidation in my opinion should be to not leave dust. This is because
That said, I'll look into the Since that looks like it might be the case, I'll change the code to round up for collateral consumed, and down for base tokens rewarded. I'll await further discussion on the rest. edit: Use of The above was a scenario where both the borrowed and the collateral value were tiny "dust" quantities, and the collateral denom was worth more in USD than the borrowed denom. Here are my opinions on other permutations:
|
Co-authored-by: Robert Zaremba <robert@zaremba.ch>
Co-authored-by: Robert Zaremba <robert@zaremba.ch>
There was a problem hiding this comment.
Part 1. Will finish tomorrow.
- need to validate (in
ValidateBasic) thatMsgLiquidate.RewardDenomis not a uToken.... On the other hand, we should supportuTokenin case liquidator will like to keepuTokens- but that's probably a case for another task. Please confirm.
| string liquidator = 1; | ||
| string borrower = 2; | ||
| cosmos.base.v1beta1.Coin repayment = 3 [(gogoproto.nullable) = false]; | ||
| string reward_denom = 4; |
There was a problem hiding this comment.
Do we have a test verifying if the liquidation of a single asset, even if it doesn't bring the account to a healthy state, is possible?
Co-authored-by: Robert Zaremba <robert@zaremba.ch>
In the current code, this is handled in After #1202 both options will be valid (but lead to different outcomes) |
There was a problem hiding this comment.
pre-approving. Left few comments.
Importantly, let's verify if our interest rate logic is applied correctly. In the setBorrow and getBorrow I don't see how time ingredient is applied. Interest rates are defined over year, so need to do few checks:
- to not apply the interest rate in the same block twice
- add time component to set/get Borrow.
| if remainingCollateral.IsZero() { | ||
| for _, coin := range k.GetBorrowerBorrows(ctx, borrowerAddr) { | ||
| // set a bad debt flag for each borrowed denom | ||
| if err := k.setBadDebtAddress(ctx, borrowerAddr, coin.Denom, true); err != nil { |
There was a problem hiding this comment.
why don't we store amount of tokens in bad debt?
There was a problem hiding this comment.
Bad debt is currently just something to iterate over, so storing 0x01 is sufficient.
We could store the amount - but then it would be redundant with adjusted borrow which also stores amount. Adjusted borrow would also continue to accrue interest while this does not. Additionally, any operation (e.g. partial repay) that updated one of the two would have to keep the other in sync, while the bad debt existed.
Overall, it's simpler to avoid storing amount here.
Overall, try to keep discussions within the bounds of the PR. The interest rate logic you're looking for is in |
Co-authored-by: Robert Zaremba <robert@zaremba.ch>
Co-authored-by: Robert Zaremba <robert@zaremba.ch>
This PR uses get/set Borrow. So it make sense to look there and eventually could be fixed in other PR if needed. |
Description
Significant Change:
MsgLiquidate, liquidator receives equivalent base assets.Reasoning:
We're about to add
MaxCollateralUtilizationwhich restrictsMsgWithdrawat critical times, soAvailableSupplycan be used for liquidation.But if liquidators receive their rewards as uTokens, then the
MsgWithdrawrestriction will apply to them just like any other user, and they will be unable to receive their base tokens.Switching to base assets received eliminates makes
MaxCollateralUtilizationwork as intended, and also simplifies theMsgLiquidate -> MsgWithdraw -> IBC (to sell reward)liquidation workflow to justMsgLiquidate -> IBCAPI Breaking:
MsgLiquidate's rewardsdk.Coinfield to reward_denomstringReasoning:
That field was for rare cases where liquidators do not trust the umee
x/oracleand want to put their own price floors. Such cases should be handled off-chain (just query prices) rather than adding a computation and required field for all users. See #828closes: #898
closes: #828
relevant to #831 - because rounding up repayment and reward eliminates most "dust" liquidation remainders
Author Checklist
All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.
I have...
!to the type prefix if API or client breaking changeCHANGELOG.mdReviewers Checklist
All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items.
I have...