search

hifi-finance / hifi

Monorepo implementing the Hifi fixed-rate, fixed-term lending protocol
https://app.hifi.finance
Other
106 stars 15 forks source link

feat(protocol): add repay bond amount calculation #38

Closed scorpion9979 closed 3 years ago

scorpion9979 commented 3 years ago

Adds an on-chain getter method to the balance sheet to calculate the amount of bond needed to be repaid in order to liquidate a specified amount of a specified collateral.

PaulRBerg commented 3 years ago

I just started to review this and noticed that this PR does not test the function it adds. I'm afraid you will have to write some tests, @scorpion9979 😬

A comment about the name of the repayBondAmount variable. It can be interpreted as as a number of bonds, because there is another storage variable in the protocol called maxBonds. I would rather define the parameter as simply repayAmount, since that variable is ubiquitously considered an amount of hTokens.

And a question about the logic itself. Didn't you copy-paste this from somewhere else? If yes, shouldn't we call this new function from that place, so that we keep the code base DRY?

scorpion9979 commented 3 years ago

I just started to review this and noticed that this PR does not test the function it adds. I'm afraid you will have to write some tests, @scorpion9979

A comment about the name of the repayBondAmount variable. It can be interpreted as as a number of bonds, because there is another storage variable in the protocol called maxBonds. I would rather define the parameter as simply repayAmount, since that variable is ubiquitously considered an amount of hTokens.

And a question about the logic itself. Didn't you copy-paste this from somewhere else? If yes, shouldn't we call this new function from that place, so that we keep the code base DRY?

I didn't actually do much copy-pasting. I basically used the math formula in getSeizableCollateralAmount(...)'s interface definition, but refactored it to solve for repay amount instead of collateral amount.

PaulRBerg commented 3 years ago

There is a thorough change log in my recent commit message.

PaulRBerg commented 3 years ago

Besides the fixes we discussed above, I made two other big changes: