Skip to content
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

Merged
merged 23 commits into from
Mar 18, 2022
Merged

Improve Token Math #114

merged 23 commits into from
Mar 18, 2022

Conversation

Namaskar-1F64F
Copy link
Contributor

@Namaskar-1F64F Namaskar-1F64F commented Mar 18, 2022

This PR uses FixedPointMathLib - for calculations.

This PR removes totalCollateral - we don't need it closes #89

This PR refactors redeem - I think it was broken before

This PR adds tests to mint, repay, and redeem - now with more token-decimals support

🙌

@Namaskar-1F64F Namaskar-1F64F enabled auto-merge (squash) March 18, 2022 01:48
@Namaskar-1F64F Namaskar-1F64F requested a review from RusseII March 18, 2022 01:48
Copy link
Contributor

@RusseII RusseII left a 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

contracts/SimpleBond.sol Show resolved Hide resolved
contracts/SimpleBond.sol Show resolved Hide resolved
function _upscale(uint256 amount) internal view returns (uint256) {
return (amount * repaymentScalingFactor) / ONE;
return amount.mulDivUp(repaymentScalingFactor, ONE);
Copy link
Contributor

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?

Copy link
Contributor

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 ?

Copy link
Contributor Author

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.

contracts/SimpleBond.sol Show resolved Hide resolved
contracts/SimpleBond.sol Show resolved Hide resolved
contracts/SimpleBond.sol Show resolved Hide resolved
{
if (block.timestamp < maturityDate) {
// Immature bond redeems for nothing
Copy link
Contributor

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

Copy link
Contributor Author

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?

Copy link
Contributor

@RusseII RusseII Mar 18, 2022

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

@Namaskar-1F64F Namaskar-1F64F merged commit e3792ad into main Mar 18, 2022
@Namaskar-1F64F Namaskar-1F64F deleted the improve-token-math branch March 18, 2022 14:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Consider removing totalCollateral and state function
2 participants