Skip to content

short code cleanup#151

Merged
dpaiton merged 8 commits intomainfrom
dpaiton/short-cleanup
Jun 25, 2024
Merged

short code cleanup#151
dpaiton merged 8 commits intomainfrom
dpaiton/short-cleanup

Conversation

@dpaiton
Copy link
Contributor

@dpaiton dpaiton commented Jun 22, 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 Dry up test preamble; code cleanup #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 dpaiton force-pushed the dpaiton/short-cleanup branch 3 times, most recently from b61ac8d to feba4c9 Compare June 22, 2024 01:20
@dpaiton dpaiton force-pushed the dpaiton/short-cleanup branch 4 times, most recently from 610c8c2 to f73ddb6 Compare June 22, 2024 07:35
@dpaiton dpaiton force-pushed the dpaiton/short-cleanup branch from f73ddb6 to 800c690 Compare June 22, 2024 08:52
@dpaiton dpaiton force-pushed the dpaiton/short-cleanup branch from 800c690 to 7a4ceaa Compare June 23, 2024 04:28
Copy link
Contributor

@sentilesdal sentilesdal left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

shouldnt need to set variable to zero when advancng time.

Copy link
Contributor

@sentilesdal sentilesdal left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just add a comment that setting the variable rate to zero is necessary to get the test to pass and that this is very bad and needs to be fixed

@dpaiton dpaiton enabled auto-merge (squash) June 25, 2024 20:13
@dpaiton dpaiton merged commit 689b073 into main Jun 25, 2024
@dpaiton dpaiton deleted the dpaiton/short-cleanup branch June 25, 2024 20:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants