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

Fixed rounding issue with recovery #170

Merged
merged 1 commit into from
Aug 11, 2023
Merged

Fixed rounding issue with recovery #170

merged 1 commit into from
Aug 11, 2023

Conversation

aalavandhan
Copy link
Member

The computeRedeemableTrancheAmounts should round the balances down to a number divisible by the tranche ratio.

For example, if the tranche balances are "10, 15, 7.461048491123254231" the previous implementation returned "2.984419396449301692, 4.476629094673952538, 7.461048491123254231".

However, this is not the right redemption ratio, which is expected to be precisely divisible.
https://github.com/buttonwood-protocol/tranche/blob/main/contracts/BondController.sol#L229

The proposed change rounds down to achieve amounts are consistent with the expected tranche ratio.
We get -> "2.984419396449301600, 4.476629094673952400, 7.461048491123254000"

@aalavandhan
Copy link
Member Author

aalavandhan commented Aug 10, 2023

Consider another example. This time lets just consider fixed point numbers.

For tranche ratios of 200,300,500 and granularity=1000.

Lets assume balances are "[1000, 5001, 503]"

As per the previous implementation, the redeemable tranche ratios returned will be "[201, 301, 503]".
The actual ratios are "[201.2, 301.8, 503]", but because we use fixed point math we lose the decimals and we get inconsistent ratios.

The expected solution in this case is simply ["200, 300, 500"].

The proposed solution first finds the closest balance divisible by the tranche ratios. (line 219) . We get "[1000, 4800, 500]". After this we just continue with the existing algorithm.

@aalavandhan aalavandhan force-pushed the recover-fix branch 2 times, most recently from 8422490 to c9c5516 Compare August 11, 2023 15:29
Copy link
Member

@brandoniles brandoniles left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice solution. I agree this strikes the right balance for gas efficiency.

@aalavandhan aalavandhan merged commit d0d479d into main Aug 11, 2023
@aalavandhan aalavandhan deleted the recover-fix branch August 11, 2023 19:27
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.

2 participants