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

bug(CL): rounding behavior is incorrect, analyze other truncations and roundings #3891

Closed
p0mvn opened this issue Dec 30, 2022 · 1 comment
Closed
Assignees
Labels
F: concentrated-liquidity Tracking the development of concentrated liquidity feature to improve filtering on the project board

Comments

@p0mvn
Copy link
Member

p0mvn commented Dec 30, 2022

Background

The comment mentions that we need to round up but does banker's rounding. Instead, we need to first take Ceil() and then TruncateInt().

// coin amounts require int values
// round amountIn up to avoid under charging
amt0 := tokenAmountInAfterFee.Sub(swapState.amountSpecifiedRemaining).RoundInt()
amt1 := swapState.amountCalculated.TruncateInt()

// coin amounts require int values
// round amountIn up to avoid under charging
amt0 := swapState.amountCalculated.TruncateInt()
amt1 := desiredTokenOut.Amount.ToDec().Sub(swapState.amountSpecifiedRemaining).RoundInt()

Suggested Design

Fix the 2 problems

Review other calls to TruncateInt and RountInt in CL module to see if truncations/roundings are safe

Acceptance Criteria

  • 2 examples fixed
  • TruncateInt in CL module are analyzed and correct
  • RountInt in CL module are analyzed and correct
@p0mvn p0mvn added the F: concentrated-liquidity Tracking the development of concentrated liquidity feature to improve filtering on the project board label Dec 30, 2022
@github-project-automation github-project-automation bot moved this to Needs Triage 🔍 in Osmosis Chain Development Apr 10, 2023
@stackman27 stackman27 self-assigned this Apr 11, 2023
@p0mvn p0mvn moved this from Needs Triage 🔍 to Todo 🕒 in Osmosis Chain Development Apr 17, 2023
@p0mvn p0mvn assigned p0mvn and unassigned stackman27 Apr 17, 2023
@p0mvn
Copy link
Member Author

p0mvn commented Apr 25, 2023

These specific examples have been fixed, the rest should be investigated in internal review

@p0mvn p0mvn closed this as completed Apr 25, 2023
@github-project-automation github-project-automation bot moved this from Todo 🕒 to Done ✅ in Osmosis Chain Development Apr 25, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
F: concentrated-liquidity Tracking the development of concentrated liquidity feature to improve filtering on the project board
Projects
Archived in project
Development

No branches or pull requests

2 participants