update calc_open_short to match solidity
#29
Assignees
Labels
bug
Something isn't working
Comments
|
This has to be fixed before we can make progress on #21 This also might explain the issues I found when trying to update |
This was referenced Apr 16, 2024
dpaiton
changed the title
calc_open_short to include price backdatingcalc_open_short to match solidity
6 tasks
|
However, the test currently requires us to add 100 million bonds to the max amount before This might be an indication that calc open short is not implemented correctly. |
|
Possibly also related. |
6 tasks
dpaiton
added a commit
that referenced
this issue
Jun 4, 2024
# Resolved Issues Partially addresses #29 and #21 # Description These are steps towards implementing a parity implementation of open short as well as targeted short. - Add a function to calc short proceeds & round up, which is used in the Open Short flow. - Modifies calculate short proceeds down to look more like the up version. Since these functions are implemented on State, we preferentially use State member variables (e.g. `vault_share_price`) instead of passing arguments. The short proceeds is given by: $$ P_{\text{short}}(\Delta y) = \tfrac{\Delta y \cdot c_{1}}{c_{0} \cdot c} + \tfrac{\Delta y}{c} \cdot \phi_{f} - \Delta z(\Delta y) $$
This was referenced Jun 17, 2024
dpaiton
added a commit
that referenced
this issue
Jun 20, 2024
… `Result<Option<T>>` (#143) # Resolved Issues working towards #29 syncs changes with hyperdrive when the version bumped from `1.0.11` to `1.0.12`. # Description The solvency checkers used to return `Option<T>`, but were later updated to return `Result<Option<T>>` so they could handle underlying function calls that returned `Resault<T>`. This leads to downstream checks that are confusing at best, and wrong at worst. For example, something like this in `short/max.rs`: ```rs let mut best_valid_max_bond_amount = match self.solvency_after_short(max_bond_amount, checkpoint_exposure)? { Some(_) => max_bond_amount, None => fixed!(0), }; ``` is meant as a sanity check on `max_bond_amount` and should not cause the calling function to fail. However the `?` operator would indeed cause failure even though the call is wrapped in a match statement. Updating it to ```rs let mut best_valid_max_bond_amount = match self.solvency_after_short(max_bond_amount, checkpoint_exposure) { Ok(_) => max_bond_amount, Err(_) => fixed!(0), }; ``` means that we don't care _why_ or _where_ the solvency check failed. Any underlying failure leads to assigning the value to 0, and a complete success leads to assigning it to the variable being checked. Note: I had to increase the tolerance for the trade amount delta in `fuzz_error_open_short_max_txn_amount`. The previous amount was already absurdly high, though, so either the test or the thing it's testing (max short & open short) is not working. I am actively investigating this in the process of completing #29. Given this, I think the tolerance increase is acceptible.
|
#142 bumps |
Merged
dpaiton
added a commit
that referenced
this issue
Jun 22, 2024
# Resolved Issues Making testing more consistent to help narrow problems found when investigating #29 # Description - 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.
dpaiton
added a commit
that referenced
this issue
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.
dpaiton
added a commit
that referenced
this issue
Jun 28, 2024
# Resolved Issues #29 #121 # Description This PR updates the Rust open short and derivative implementations to match Solidity. With help from @sentilesdal I uncovered a bug with sol differential tests where the current vault share price was not correctly used on the rust end. Fixing this allowed us to drop several test tolerances as well. I'll follow up with another PR that applies this fix elsewhere. I also did some minor variable renames, added & modified max short checks, and resolved a few lingering FIXMEs. I ran all of the tests with 10x fuzz constants locally without any failures. ## 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`). - `fuzz_sol_calculate_max_short_without_budget_then_open_short` lowered base test tolerance due to both the open short fix and the vault share price fix. ## Increased tolerances on tests 😭 - I had to increase tolerance on `fuzz_calculate_spot_price_after_short` from `fixed!(0)` to `fixed!(100)`. However, the test is more difficult now because it uses a random variable rate instead of 0%. - note: `test_sol_calculate_pool_deltas_after_open_short` looks like it has a new tolerance, but this was simply assigning an existing tolerance to a variable at the top of the test. - `fuzz_sol_calculate_max_short_without_budget_then_open_short` raised bond test tolerance from 1e2 to 1e10, but 1e2 wasn't holding up to extensive fuzz testing in the first place. ## Math outline The below formulation follows the logic outlined in `HyperdriveShort.sol`. I made some slight changes to variable names and ordering of presentation.   
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
This issue originally raised a concern that the rust and solidity implementations for open_short were different. I've added tests to determine that they are indeed acting the same, within an error of
1e11.However there is still an issue that the
calc_open_shortoutput is implemented differently from the equivalent smart contract call. We'd like to have the rust mirror the solidity as much as possible. Differences in implementation can lead to unintended bugs and difficulty managing changes that must be applied to both code bases.The error (
1e11) is also pretty high, so it's still possible that the implementations are different in some small way.The text was updated successfully, but these errors were encountered: