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.

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

fee overhaul & targeted long fix #95

Merged
merged 7 commits into from
May 14, 2024
Merged

fee overhaul & targeted long fix #95

merged 7 commits into from
May 14, 2024

Conversation

dpaiton
Copy link
Contributor

@dpaiton dpaiton commented May 13, 2024

Description

This PR is the result of an audit of the fee functions. The following changes were made (one commit per bullet):

  1. revert the rounding adjustment from Fix open_short_curve_fee #97. We realized that the bug actually lies in the solidity test utils. This has been fixed so I reverted the rounding behavior to match.
  2. update docstrings to use a more consistent format. I introduce a convention of my own to use upper-case $\Phi_{c,ol}(...)$ for the fee function and lower-case $\phi_{c}$ for the fee multiplier constant. The subscript on the fee function is the fee type $\in c, g, f$ for curve, gov, and flat; as well as the trade type $\in ol, cl, os, cs$ for opening or closing a long or short. This replaces the inconsistent method of using e.g. c(x) at some times and curve_fee at other times, or using repeat symbols for different equations.
  3. add maybe_curve_fee arg to gov fee calculations to fix Avoid redundant calculations for calculate_close_short_flat_plus_curve #43 and reduce computation.
  4. change the rounding behavior to match solidity (note it's matching the eqs in internal/HyperdriveBase.sol
  5. the above changes revealed a unit issue in calculate_targeted_long where the gov fee (and derivative) was not converted to shares. I updated my overleaf doc, the docstrings, and the code itself.

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 dpaiton force-pushed the dpaiton/fees-audit branch 2 times, most recently from 336007b to 2aeca1b Compare May 13, 2024 17:54
@dpaiton dpaiton linked an issue May 13, 2024 that may be closed by this pull request
@dpaiton dpaiton changed the title docstring & var name updates fee audit & fixes May 13, 2024
@dpaiton dpaiton force-pushed the dpaiton/fees-audit branch from 29135ed to 426fb54 Compare May 14, 2024 05:40
@dpaiton dpaiton changed the title fee audit & fixes fee audit & targeted long fix May 14, 2024
@dpaiton dpaiton changed the title fee audit & targeted long fix fee overhaul & targeted long fix May 14, 2024
@dpaiton dpaiton force-pushed the dpaiton/fees-audit branch from 426fb54 to 2d40100 Compare May 14, 2024 06:05
@dpaiton dpaiton force-pushed the dpaiton/fees-audit branch 2 times, most recently from 027124b to b832625 Compare May 14, 2024 07:04
@dpaiton dpaiton force-pushed the dpaiton/fees-audit branch from b832625 to 06e27a5 Compare May 14, 2024 16:05
@dpaiton dpaiton force-pushed the dpaiton/fees-audit branch from 06e27a5 to f2abae8 Compare May 14, 2024 17:01
@dpaiton dpaiton marked this pull request as ready for review May 14, 2024 17:16
Copy link
Contributor

@jrhea jrhea left a comment

Choose a reason for hiding this comment

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

lgtm

@dpaiton dpaiton merged commit 8958336 into main May 14, 2024
8 checks passed
@dpaiton dpaiton deleted the dpaiton/fees-audit branch May 14, 2024 18:14
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.

Avoid redundant calculations for calculate_close_short_flat_plus_curve
3 participants