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.

Sign up for GitHub

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

Jump to bottom

Tests fail when using large (but valid) share_adjustment #171

Open
dpaiton opened this issue Jul 11, 2024 · 0 comments
Open

Tests fail when using large (but valid) share_adjustment #171

dpaiton opened this issue Jul 11, 2024 · 0 comments
Labels
bug Something isn't working

Comments

@dpaiton
Copy link
Member

dpaiton commented Jul 11, 2024

#170 highlights that using the upper-bound on share_adjustment when generating a random state causes a set of tests to fail:

test lp::math::tests::fuzz_calculate_net_curve_trade_safe
test lp::math::tests::fuzz_calculate_present_value
test long::max::tests::fuzz_sol_calculate_max_long
test short::open::tests::test_defaults_calculate_spot_price_after_short

After an initial inspection, @jalextowle concluded that at least the fuzz_sol_calculate_max_long, fuzz_calculate_present_value, and fuzz_calculate_net_curve_trade_safe failures indicate a discrepancy between Rust and Solidity.

We need to dig into each test and correct it or the underlying code it is testing.
@dpaiton dpaiton mentioned this issue Jul 11, 2024
Merged
dpaiton added a commit that referenced this issue Jul 11, 2024
@dpaiton
Inclusive & valid random share reserve range; dynamic fuzz bugfix (#170)
c469d2b 
# Description
- share adjustment (zeta) was using a non-inclusive range, but the
requirement (`<=`) implies inclusive should be valid.
- I tested this with the bounds (`share_adjustment = 0` and
`share_adjustment = share_reserves - min_share_reserves -
fixed!(10e18)`) and all tests passed with both extremes
- note that I had to reduce the upper-bound by `fixed!(10e18)` for the
tests to pass, which I guess means we were just getting lucky before. I
made an issue for this:
#171
- dynamic fuzz is supposed to do 10x fuzz runs on new tests, but was 1x
with `FAST_FUZZ_RUNS`
- fix a check in an `open.rs` test where the allowable max bond amount
was too low
@dpaiton dpaiton added the bug Something isn't working label Jul 11, 2024
@dpaiton dpaiton mentioned this issue Jul 16, 2024
Merged
dpaiton added a commit that referenced this issue Jul 22, 2024
@dpaiton
fix open long delta calculations and add tests (#178)
2d56ace 
# Resolved Issues
working towards #171 and
#21

# Description
- The code for calculating pool deltas & state after opening a long did
not correctly account for the gov fee's impact on the share reserves. I
fixed this and added a test to demonstrate the failure
- I added a similar test on the short side, which almost passes with
equality (seems there is a rounding error)
- I renamed some functions, cleaned up some comments, fixed some typos
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
bug Something isn't working
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
1 participant
@dpaiton