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

update calc_open_short to match solidity #116

Closed
wants to merge 12 commits into from
Closed

update calc_open_short to match solidity #116

wants to merge 12 commits into from

Conversation

dpaiton
Copy link
Member

@dpaiton dpaiton commented May 27, 2024 •
edited
Loading

Resolved Issues

#29
#121

Description

This PR updates the Rust open short and derivative implementations to more closely match Solidity.

Lower tolerances on tests 🎉

  • After the fix, I was able to completely remove the error tolerance on fuzz_calc_open_short.
  • I also updated the epsilon & tolerance values for the fuzz_short_deposit_derivative and fuzz_short_principal_derivative tests. I use a larger epsilon and smaller tolerance, which I think is what we prefer (more distant points should match linearity assumptions better due to the lack of monotonicity coming from pow).

Increased tolerances on tests 😭

  • I had to increase error tolerance for fuzz_calculate_max_short_no_budget from 1e12 to 1e16. I compared the rust & solidity implementations, but they're too different from each other to find any bugs by pattern matching. I'm not sure what is going on here, and would suggest we open a new issue to investigate. (more on this below)
  • I was hoping that this PR would allow me to reduce the additional bond amount in fuzz_error_open_short_max_txn_amount (per this comment), but could not (it is still 100M). This is probably related to lingering issues with calculating the max short.
  • I had initially thought this PR fixed fuzz_calculate_max_short, but I think the more likely scenario is the test wasn't set up to consistently fail for it's condition. I modified the test in my last commit and now it is consistently passing.

(relatively) Inconsequential updates

  • I updated some of the derivative tests to use an equivalent, but simpler method for numerical differentiation that only applies the epsilon once and therefore makes the distance between points easier to understand from the empirical_derivative_epsilon parameter. I also changed the tolerance check to be <= instead of < for the same reason.
  • I removed the use of calc_max_short in the preamble & open short tests. While on working this PR I was getting frustrated because the open short tests were failing due to errors in max short. This is a better setup in general, so that our tests behave more like unit tests that are easier to debug than integration tests.
  • I renamed long_amount_derivative to calculate_open_long_derivative and short_deposit_derivative to calculate_open_short_derivative.
  • I moved some arguments out of fn signatures and instead use state member variables. This is a departure from Solidity but (imo) justified because the Rust fns are all implemented on the state struct. The preferred pattern (and what we do elsewhere) is to update state and call the fn instead of passing different values as fn args.

Important items to check

  • My changes to the short deposit derivative and calc open short were not algebraic -- I believe my function is entirely different from what was there. My guess is calc open short now matches solidity, but the derivative does not. I think it's worth comparing this against solidity to make sure there aren't errors in the solidity implementation of the derivative. It could explain the next point:
  • The error in fuzz_calculate_max_short_no_budget might be due to a difference in max_short_guess or an error in the solidity short derivative fn. I still think this is best investigated in a follow-up issue, but I'm noting it here as a clue for the future.

Math outline

The below formulation follows the logic outlined in HyperdriveShort.sol. I made some slight changes to variable names and ordering of presentation.

image
image
image

@dpaiton dpaiton mentioned this pull request May 27, 2024
Closed
6 tasks
@dpaiton dpaiton linked an issue May 27, 2024 that may be closed by this pull request
update calc_open_short to match solidity #29
Closed
@dpaiton dpaiton force-pushed the dpaiton/calc-open-short branch 3 times, most recently from f046c80 to 0972415 Compare May 29, 2024 17:19
@dpaiton dpaiton force-pushed the dpaiton/calc-open-short branch 5 times, most recently from 7fa84a4 to 760d443 Compare June 5, 2024 22:59
@dpaiton dpaiton linked an issue Jun 5, 2024 that may be closed by this pull request
fuzz_calculate_max_short occasionally failing #121
Closed
@dpaiton dpaiton marked this pull request as ready for review June 5, 2024 23:37
@dpaiton dpaiton force-pushed the dpaiton/calc-open-short branch 2 times, most recently from 0c23fd0 to 2ff193d Compare June 11, 2024 03:48
@dpaiton
Copy link
Member Author

dpaiton commented Jun 11, 2024 •
edited
Loading

The fuzz_calculate_max_short test is failing again after I pushed up a rebased version of the branch. So that problem is not fixed. I'm going to remove it from the PR desc as "solved".

Looking at the current failure & the one reported in #121 I'm thinking the amount it is off is quite large -- not just a matter of bumping some error tolerance.

update: it looks like the logic changed in calc_max_short quite some time ago (#537) to something we don't actually want, where it is returning a valid bond amount even if calculate_open_short throws an error. However, fixing this does not solve the problem.

update 2: After quite a bit of digging I was able to get the test to pass consistently after increasing the number of iterations. I think it just doesn't reach the solution in 7 iterations some of the time.

@dpaiton dpaiton mentioned this pull request Jun 12, 2024
Merged
dpaiton added a commit that referenced this pull request Jun 12, 2024
@dpaiton @jalextowle
max short fixes (#137)
ce169d6 
# Resolved Issues
Partially addresses #121 & #136


# Description
These changes arose in my other pr (#116) while trying to debug issues
and high tolerances in `short::max`. There are a lot of commits in order
to separate out meaningful changes from trivial ones. A majority of the
changes are code reorganization, docstring fixes, and some additional
sanity checks.

Major changes were:
- `fuzz_calculate_max_short`
- was renamed to more accurately reflect what it is testing -- max short
in the budget constrained regime
- was rewritten to ensure the budget is constrained. I believe the
reason the failures were "intermittent" before was because it was not
always hitting the failure case. It hits it a lot now.
- the tolerance was updated so that it consistently passes, to give us
an idea of how much it is off from what we want.
  
- `calculate_max_short`
this code was incorrect:
 ```
          let absolute_max_deposit =
match self.calculate_open_short(max_bond_amount, open_vault_share_price)
{
                Ok(d) => d,
                Err(_) => return Ok(max_bond_amount),
``` 

we do **not** want to return the bond amount if `calculate_open_short` is throwing an error. I updated it.

I ran the tests locally with `FUZZ_RUNS=1_000` and `FAST_FUZZ_RUNS=50_000` without any tests failing.

---------

Co-authored-by: Alex Towle <jalextowle@gmail.com>
@dpaiton dpaiton force-pushed the dpaiton/calc-open-short branch from 036fba9 to cb7290a Compare June 12, 2024 23:09
@dpaiton dpaiton force-pushed the dpaiton/calc-open-short branch from 110e3a3 to 3bcdd03 Compare June 13, 2024 04:49
@dpaiton dpaiton marked this pull request as draft June 17, 2024 19:07
@dpaiton dpaiton mentioned this pull request Jun 22, 2024
Merged
dpaiton added a commit that referenced this pull request Jun 25, 2024
@dpaiton
short code cleanup (#151)
689b073 
# 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.
@dpaiton
Copy link
Member Author

dpaiton commented Jun 25, 2024

closing in favor of #152

@dpaiton dpaiton closed this Jun 25, 2024
@dpaiton dpaiton deleted the dpaiton/calc-open-short branch June 28, 2024 17:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jalextowle jalextowle Awaiting requested review from jalextowle

@jrhea jrhea Awaiting requested review from jrhea

@mcclurejt mcclurejt Awaiting requested review from mcclurejt

@sentilesdal sentilesdal Awaiting requested review from sentilesdal

@ryangoree ryangoree Awaiting requested review from ryangoree

@slundqui slundqui Awaiting requested review from slundqui

@wakamex wakamex Awaiting requested review from wakamex

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

fuzz_calculate_max_short occasionally failing update calc_open_short to match solidity
1 participant
@dpaiton