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

Fix/div zero #176

Merged
merged 10 commits into from
Jun 10, 2024
Merged

Fix/div zero #176

merged 10 commits into from
Jun 10, 2024

Conversation

0xDmtri
Copy link
Contributor

@0xDmtri 0xDmtri commented May 31, 2024

Motivation

Fix annoying bug related to not enough bits that results in div zero due to very small price number.

Solution

  1. Convert to Q128 before sqrtPrice.
  2. Add two forge tests.

PR Checklist

  • [+] Added Tests
  • [-] Added Documentation
  • [-] Breaking changes

@0xDmtri
Copy link
Contributor Author

0xDmtri commented Jun 3, 2024

"We should really add some unit tests on this function after these changes. Maybe we could separate all peripheral functions into a deployable inherited contract that we can then easily unit test in foundry"

Just for this contract? Should we do it in this pull request or make a new one? U wanna make an abstract contract right?

@0xDmtri
Copy link
Contributor Author

0xDmtri commented Jun 4, 2024

Another edge case was found:

It will still produce a div 0 error if sqrtPriceX96 == 0. I dont think we can fix that except of doing:

require(sqrtPriceX96 != 0)

@0xOsiris thoughts?

@0xDmtri
Copy link
Contributor Author

0xDmtri commented Jun 4, 2024

Change log:

  1. Added another test case in Foundry.
  2. Fixed bug with Uni V3 pools when sqrtPriceX96 == 0.
  3. Fixed bug with Uni V3 pools when Token does not impl decimals().
  4. Added weth value test in Rust.
  5. Specified target EVM version in Foundry config.

@0xOsiris
Copy link
Member

0xOsiris commented Jun 8, 2024

"We should really add some unit tests on this function after these changes. Maybe we could separate all peripheral functions into a deployable inherited contract that we can then easily unit test in foundry"

Just for this contract? Should we do it in this pull request or make a new one? U wanna make an abstract contract right?

Yeah, we can isolate this into a different PR though

Copy link
Member

@0xOsiris 0xOsiris left a comment

Choose a reason for hiding this comment

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

Nice work on this! LGTM

@0xDmtri
Copy link
Contributor Author

0xDmtri commented Jun 9, 2024

ah shit, theres a conflict with .gitignore, will resolve now

@0xDmtri
Copy link
Contributor Author

0xDmtri commented Jun 9, 2024

done!

@0xKitsune 0xKitsune merged commit 7d9980a into darkforestry:main Jun 10, 2024
1 of 2 checks passed
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.

3 participants