single preamble
We had 3x duplicates of a preamble function that would execute a series of trades on the deployed test pool in order to simulate testing from a random state. I consolidated it into one location that is imported elsewhere. I also added some checks on upper trade bounds so that it is guaranteed to be valid, even if calc_max fails.
code cleanup
This seemingly innocuous change caused a difficult-to-track Hisenbug where the ERC20 contract would throw an overflow/underflow error whenever we'd call hyperdrive.open_long or hyperdrive.open_short. This was due to lack of approval, which was due to the approval call not going through before the open call, which was due to anvil not being able to keep up with the rust calls coming in.
I fixed that bug (thanks to @mcclurejt for the help!) and in the process of hunting down that bug did a bunch of code cleanup stuff like improved error messaging, variable names, organization, etc.
I'm was able to consistently reproduce #4 while tracking this bug, and the fix we implemented seems to have also fixed that one.
new tests
I also wrote a couple of new tests while hunting the Heisenbug.
NOTE:
I had to increase test tolerance for fuzz_sol_calc_open_short from 1e9 to 10e18. My best guess as to why this happened is because we're now testing a wider variety of valid states and we're hitting error modes. I know that calculate_open_short does not match solidity exactly, so most likely this is the source of the error. I added a comment as a reminder in #29, but for now I think we should live with the high tolerance.
That being said, I encourage the reviewer to look carefully. I may have missed a change in this PR that is causing such a big error.
Resolved Issues
Making testing more consistent to help narrow problems found when investigating https://github.com/delvtech/hyperdrive-rs/issues/29
Description
single preamble We had 3x duplicates of a
preamblefunction that would execute a series of trades on the deployed test pool in order to simulate testing from a random state. I consolidated it into one location that is imported elsewhere. I also added some checks on upper trade bounds so that it is guaranteed to be valid, even if calc_max fails.code cleanup This seemingly innocuous change caused a difficult-to-track Hisenbug where the ERC20 contract would throw an overflow/underflow error whenever we'd call
hyperdrive.open_longorhyperdrive.open_short. This was due to lack of approval, which was due to the approval call not going through before the open call, which was due to anvil not being able to keep up with the rust calls coming in. I fixed that bug (thanks to @mcclurejt for the help!) and in the process of hunting down that bug did a bunch of code cleanup stuff like improved error messaging, variable names, organization, etc. I'm was able to consistently reproduce #4 while tracking this bug, and the fix we implemented seems to have also fixed that one.new tests I also wrote a couple of new tests while hunting the Heisenbug.
NOTE:
I had to increase test tolerance for
fuzz_sol_calc_open_shortfrom 1e9 to 10e18. My best guess as to why this happened is because we're now testing a wider variety of valid states and we're hitting error modes. I know thatcalculate_open_shortdoes not match solidity exactly, so most likely this is the source of the error. I added a comment as a reminder in #29, but for now I think we should live with the high tolerance.That being said, I encourage the reviewer to look carefully. I may have missed a change in this PR that is causing such a big error.