-
Notifications
You must be signed in to change notification settings - Fork 1
Better auction creation #37
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
Conversation
Ryang-21
commented
Apr 30, 2025
- Implement search for minimum auction size that results in targeted user health factor
mootz12
left a comment
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 - mostly picky comments and a few additional test cases that would be helpful
src/liquidations.ts
Outdated
| // The factor by which we can reduce the liabilities | ||
| let liabilityReductionFactor = avgLF * 1.06 - avgCF * estIncentive; | ||
| let totalRemoveableLiabilities = liabilityReductionFactor * positions.totalBorrowed; |
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.
liabilitiesToReduce -> excessBorrowed
liabilityReductionFactor -> borrowLimitFactor
totalRemoveableLiabilities -> totalBorrowLimitRecovered
might be clearer names?
src/liquidations.ts
Outdated
| if (liqPercent === 0) { | ||
| let nextLiability = effectiveLiabilities.shift(); | ||
| if (nextLiability === undefined) { | ||
| // No more collaterals to liquidate |
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.
can we just make the assumption here that the full position needs to be liquidated?
If we have included all the liabilities and the liq percent is complaining that we still don't have enough liabilities to liquidate, the whole position can be liquidated at 100% right?
FWIW this is probably a code path that is never really hit, given the collateral would get flagged as not being sufficient first, and return a 101 liq percent.
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.
I don't think that assumption can be made because an asset with a low cf can be added which then increases the reduction factor and could make the liquidation possible
| userEstimate.totalSupplied = 2000; | ||
| const result = calculateLiquidationPercent(userEstimate); | ||
| expect(Number(result)).toBe(56); | ||
| it('should find auction subset with partial auction', () => { |
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.
we should add a test for the 1 to 1 happy path at both a partial percentage and the full percentage