-
Notifications
You must be signed in to change notification settings - Fork 3
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
Improve Token Math #114
Improve Token Math #114
Conversation
… improve-repayment-token
… improve-repayment-token
…RC20Metadata for decimals querying, fix the tests and add some comments
… improve-repayment-token
…inance/v1-core into improve-token-math
… improve-token-math
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.
YAY! Great additions. Left a few questions to better understand how it works. I didn't think too hard about the different math pieces but I will when doing the comprehensive security review.
Let's merge this and follow up if there end up being any action items from my comments
function _upscale(uint256 amount) internal view returns (uint256) { | ||
return (amount * repaymentScalingFactor) / ONE; | ||
return amount.mulDivUp(repaymentScalingFactor, ONE); |
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 you explain the specific benefit that using fixedpointmath here gives us over doing it the native way?
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.
seems like it's just safer
?
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.
There's control over rounding down or up. I think we need to figure out which parts of the contract are potentially rounding and decide which should round up or down. I took a stab at it in the contract as well as testing, but for minting for example I couldn't get a test case that was affected by either using
mulDivUp
mulDivDown
(a * b) / c
but I did notice some rounding errors in repayment maybe? The place where I'm adding or removing .sub(1)
.add(1)
a single "unit" which does get rounded away.
{ | ||
if (block.timestamp < maturityDate) { | ||
// Immature bond redeems for nothing |
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 liked peteris' idea here https://canary.discord.com/channels/903094151002857492/930939338425008208/954303204609372191 - I'd recommend a follow up PR for that
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.
peteris suggested a few things there - my takeaway was rename to previewRedeemAtMaturity
and remove the maturityDate business. Is that what you would want to see in a follow up PR?
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.
peteris suggested a few things there
I'm pretty sure he suggested one thing and outlined exactly how that one thing would work with 4 example use cases
Is that what you would want to see in a follow up PR
Yes
This PR uses
FixedPointMathLib
- for calculations.This PR removes
totalCollateral
- we don't need it closes #89This PR refactors redeem - I think it was broken before
This PR adds tests to mint, repay, and redeem - now with more token-decimals support
🙌