Dry up test preamble; code cleanup #142
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
dpaiton
force-pushed
the
dpaiton/single-preamble
branch
6 times, most recently
from
4c93a58
to
2579329
Compare
dpaiton
force-pushed
the
dpaiton/single-preamble
branch
4 times, most recently
from
d3fd141
to
f925806
Compare
dpaiton
force-pushed
the
dpaiton/single-preamble
branch
2 times, most recently
from
b8c65ef
to
b722fa3
Compare
dpaiton
force-pushed
the
dpaiton/single-preamble
branch
from
66b0119
to
2d17692
Compare
dpaiton
requested review from
jalextowle,
jrhea,
mcclurejt,
sentilesdal and
ryangoree
as code owners
mcclurejt
approved these changes
Jun 22, 2024
Merged
dpaiton
added a commit
that referenced
this pull request
Jun 25, 2024
# Resolved Issues Working towards #29 # Description This pulls a bunch of cleanup changes out of #116 to be reviewed independently. - fix a bug introduced in #142 where test tolerances were high because the variable rate was causing short amounts to change between the rust & sol calls. - improve docstrings & error messaging throughout - modify `solvency_after_short_derivative` to return `Result<T>` instead of `Result<Option<T>>` - modify max behavior to throw errors if there is no valid max, instead of returning `0` - add safety bounds on max short guesses to ensure it is always >= the min txn amount - modify sol differential max short tests to use `calculate_absolute_max_short` instead of `calculate_max_short` since the solidity implementation does not consider budget - removed the `fuzz_calculate_absolute_max_short_execute` in favor of a test that does the same but additionally checks that the pool is drained when that absolute max short is executed. It also includes some differential checks for rust vs solidity. This one is now called `fuzz_calculate_max_short_without_budget_then_open_short ` - adds a new `SLOW_FUZZ_RUNS` constant for one of the max tests bc it was slow as hell.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Resolved Issues
Making testing more consistent to help narrow problems found when investigating #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 fix
BlockOutOfRangeErrorin fuzz tests #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.