search

darkforestry / amms-rs

A Rust library to interact with automated market makers across EVM chains.
438 stars 117 forks source link

Fix/div zero #176

Closed 0xDmtri closed 3 months ago

0xDmtri commented 4 months ago

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

0xDmtri commented 4 months ago

"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 commented 4 months ago

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 commented 4 months ago

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 commented 3 months ago

"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

0xDmtri commented 3 months ago

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

0xDmtri commented 3 months ago

done!