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

Remaining Flaky Tests #88

Closed
3 tasks done
jrhea opened this issue May 9, 2024 · 4 comments
Closed
3 tasks done

Remaining Flaky Tests #88

jrhea opened this issue May 9, 2024 · 4 comments
Assignees
@dpaiton @ryangoree

Comments

@jrhea
Copy link
Contributor

jrhea commented May 9, 2024 •
edited by dpaiton
Loading

We are ignoring failures with these tests currently. This needs to be addressed ASAP

  • long::targeted::tests::test_calculate_targeted_long_with_budget
  • short::max::tests::fuzz_calculate_absolute_max_short_execute
  • short::max::tests::fuzz_calculate_max_short_no_budget
@ryangoree ryangoree self-assigned this May 10, 2024
@ryangoree ryangoree mentioned this issue May 14, 2024
Merged
6 tasks
ryangoree added a commit that referenced this issue May 14, 2024
@ryangoree
Fix open_short_curve_fee (#97)
05cec36 
# Resolved Issues

Related to #88 

# Description

We were using `mul_up` where the contracts use `mulDown`:
https://github.com/delvtech/hyperdrive/blob/33982bba72d861b3b90cb0b4083fec30accb649f/test/utils/HyperdriveUtils.sol#L1362

# Review Checklists

Please check each item **before approving** the pull request. While
going
through the checklist, it is recommended to leave comments on items that
are
referenced in the checklist to make sure that they are reviewed.

- [ ] **Testing**
    - [ ] Are there new or updated unit or integration tests?
    - [ ] Do the tests cover the happy paths?
    - [ ] Do the tests cover the unhappy paths?
- [ ] Are there an adequate number of fuzz tests to ensure that we are
          covering the full input space?
- [ ] If matching Solidity behavior, are there differential fuzz tests
that
          ensure that Rust matches Solidity?
@dpaiton
Copy link
Member

dpaiton commented May 14, 2024 •
edited
Loading

#95 resolves the remaining OP flaky tests, but unfortunately introduces a new one:

  • long::max::fuzz_calculate_max_long

I'm investigating that now

@dpaiton dpaiton self-assigned this May 15, 2024
This was referenced May 15, 2024
Closed
Merged
@dpaiton dpaiton linked a pull request May 15, 2024 that will close this issue
Fix fuzz max long test #101
Merged
6 tasks
@dpaiton
Copy link
Member

dpaiton commented May 15, 2024

another one:

failures:

---- short::max::tests::fuzz_calculate_max_short stdout ----
thread 'short::max::tests::fuzz_calculate_max_short' panicked at crates/hyperdrive-math/src/short/max.rs:723:17:
expected (base=34326359.285477447501009068) < (budget=35401424.462665845134388594) * 0.001000000000000000 = 35401.424462665845134388


failures:
    short::max::tests::fuzz_calculate_max_short

@dpaiton dpaiton removed a link to a pull request May 15, 2024
Fix fuzz max long test #101
Merged
6 tasks
@dpaiton
Copy link
Member

dpaiton commented May 15, 2024

#95 resolves the remaining OP flaky tests, but unfortunately introduces a new one:

  • long::max::fuzz_calculate_max_long

I'm investigating that now

resolved by #101

@dpaiton
Copy link
Member

dpaiton commented May 15, 2024

another one:

failures:

---- short::max::tests::fuzz_calculate_max_short stdout ----
thread 'short::max::tests::fuzz_calculate_max_short' panicked at crates/hyperdrive-math/src/short/max.rs:723:17:
expected (base=34326359.285477447501009068) < (budget=35401424.462665845134388594) * 0.001000000000000000 = 35401.424462665845134388


failures:
    short::max::tests::fuzz_calculate_max_short

I ran the fuzz test 4 * 1000 times without failure against the branch for this PR delvtech/hyperdrive#1028

I'm going to close the issue; we can re-open one later if we find new flaky tests after v1.0.7 hits with #1028 merged

@dpaiton dpaiton closed this as completed May 15, 2024
dpaiton added a commit that referenced this issue May 16, 2024
@dpaiton
Fix fuzz max long test (#101)
c28bcdd 
# Resolved Issues
Partially resolves these two:
#88
#45

# Description
The fee rounding behavior was adjusted recently to more closely match
solidity, but the order of operations was not exactly the same. This
caused an occasional slight variation in output (usually off by 1e1),
that compounded in situations where we were iteratively assessing fees
such as `max_long`. In that case, the error would grow to as much as
1e5.

During my effort to find this bug I
- modified some math to use the e.g. `.mul_down` syntax instead of `*`
where it was sufficiently complicated to require this to easily pattern
match against Solidity.
- modified `calculate_max_long` to return errors instead of panic. This
makes checks in the tests ugly, but that is a temporary problem until we
purge the panics all-together
(#20)
- purged the use of `traced` and `trace_test` which is helpful for
debugging but was not actively being used and can slow down tests.

# Review Checklists

Please check each item **before approving** the pull request. While
going
through the checklist, it is recommended to leave comments on items that
are
referenced in the checklist to make sure that they are reviewed.

- [ ] **Testing**
    - [ ] Are there new or updated unit or integration tests?
    - [ ] Do the tests cover the happy paths?
    - [ ] Do the tests cover the unhappy paths?
- [ ] Are there an adequate number of fuzz tests to ensure that we are
          covering the full input space?
- [ ] If matching Solidity behavior, are there differential fuzz tests
that
          ensure that Rust matches Solidity?
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@dpaiton dpaiton

@ryangoree ryangoree

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
3 participants
@dpaiton @ryangoree @jrhea