Reduce liquidation penalty #157
Conversation
bingen
force-pushed
the
reduce_liquidation_penalty
branch
from
199b51b
to
30171fa
Compare
Before it was impossible to claim, as NFT wouldn’t exist, and therefore we couldn’t check its owner.
bingen
force-pushed
the
reduce_liquidation_penalty
branch
from
30171fa
to
b89f18e
Compare
bingen
force-pushed
the
reduce_liquidation_penalty
branch
from
c166491
to
8d74744
Compare
bingen
force-pushed
the
reduce_liquidation_penalty
branch
from
8d74744
to
57446c9
Compare
| constructor(address _ETHAddress) { | ||
| checkContract(_ETHAddress); | ||
| ETH = IERC20(_ETHAddress); | ||
| constructor(IERC20 _ETH, ITroveManager _troveManager) { |
There was a problem hiding this comment.
Is moving TM into constructor related to the reduced liq penalty, or an independent improvement? (fine if so, just curious)
There was a problem hiding this comment.
Yes, the reason is to fetch MCR from TroveManager, thus avoiding to define it twice (and making sure it’s consistent).
| address stabilityPoolAddress; | ||
| address gasPoolAddress; | ||
| ICollSurplusPool collSurplusPool; | ||
| IBoldToken public boldToken; | ||
| // A doubly linked list of Troves, sorted by their collateral ratios | ||
| ISortedTroves public sortedTroves; | ||
|
|
||
| // Minimum collateral ratio for individual troves | ||
| uint256 public immutable MCR; |
There was a problem hiding this comment.
Why not use a constant here - it would save (a bit) of gas when we read the MCR value right?
Though I guess this approach makes deployment more robust as we can pass those Trove params.
There was a problem hiding this comment.
Yes, specially because we plan to have different MCRs for different collaterals.
contracts/src/TroveManager.sol
Outdated
| (collToRedistribute, collSurplus) = _getCollPenaltyAndSurplus( | ||
| collRedistributionPortion + collSurplus, // Coll surplus from offset can be eaten up by red. penalty | ||
| debtToRedistribute, | ||
| LIQUIDATION_PENALTY_REDISTRIBUTION, |
There was a problem hiding this comment.
We're passing the redistribution penalty here as the penaltyRatio param, is that correct?
There was a problem hiding this comment.
Yes. Should I change the name? For now I’m adding a comment to make it clearer.
| if (debtToRedistribute > 0) { | ||
| uint256 collRedistributionPortion = _collToLiquidate - collSPPortion; | ||
| if (collRedistributionPortion > 0) { | ||
| (collToRedistribute, collSurplus) = _getCollPenaltyAndSurplus( |
There was a problem hiding this comment.
Hmm what if there's a partial offset and redistribution?
We assign collSurplus above on L545, and then again here on L556. Does that leave us with the correct final collateral surplus?
There was a problem hiding this comment.
I hope so 😉
The rationale is the following (and that’s what the comment in the line below means):
- First priority is offset. It can result with an outstanding coll to be redistributed and some surplus.
- When we proceed to redistribution, we assimilate the surplus into the pending coll, so implicitly we reset surplus to zero. This is because redistribution is harsher, so it may need to eat up part of the previous surplus, i.e., the outstanding collateral may not be enough for the redistribution penalty.
- We do the math again for redistribution and get the final surplus, which may be lower or greater than the one coming from offsetting, depending if the redistribution net result was positive or negative for the borrower.
Does that make sense?
There are actually a couple of tests with mixed liquidations and redistributions, one with fuzzing. Besides all the old tests are passing.
But still this is new and sensitive, so let’s make sure my logic is sound.
There was a problem hiding this comment.
Yes, that makes sense. Thanks!
| @@ -575,7 +651,10 @@ contract TroveManager is ERC721, LiquityBase, Ownable, ITroveManager { | |||
| ); | |||
|
|
|||
| // Move liquidated ETH and Bold to the appropriate pools | |||
| stabilityPoolCached.offset(totals.totalDebtToOffset, totals.totalCollToSendToSP); | |||
| if (totals.totalDebtToOffset > 0 || totals.totalCollToSendToSP > 0) { | |||
There was a problem hiding this comment.
Can it be that one is 0 and the other is not? (Ah, I guess because the redist. penalty can eat the coll to send to SP?)
| assertEq(WETH.balanceOf(A) - AInitialETHBalance, collSurplusAmount, "A collateral balance mismatch"); | ||
| } | ||
|
|
||
| function testLiquidationOffsetNoSurplus() public { |
There was a problem hiding this comment.
Hmm why is there no surplus in this test, but there is one in the previous test?
There was a problem hiding this comment.
Because the final price is 1,030, leaving the trove at 103% CR, so below the penalty threshold, while the previous one was 1,099.999..., so leaving the trove just below 110%.
Addressing PR #157 comments.
Closes #77