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

remove parameter and cleanup names from calc_fees functions #656

Merged
merged 9 commits into from
Nov 16, 2023

Conversation

wakamex
Copy link
Contributor

@wakamex wakamex commented Nov 8, 2023

  • remove unused governanceFlatFee return varaiable
  • further rename totalCurveFee -> curveFee, totalFlatFee -> flatFee and _amount to _shareAmount and _bondAmount as appropriate (a few tests still use totalCurveFee = curveFee * amount)

Copy link

github-actions bot commented Nov 8, 2023

Hyperdrive Gas Benchmark

Benchmark suite Current: 962af42 Previous: 6f067f9 Deviation Status
addLiquidity: min 1622 gas 755 gas 114.8344% 🚨
addLiquidity: avg 54483 gas 52224 gas 4.3256% 🚨
addLiquidity: max 98352 gas 97600 gas 0.7705% 🚨
checkpoint: min 1216 gas 558 gas 117.9211% 🚨
checkpoint: avg 48284 gas 47626 gas 1.3816% 🚨
checkpoint: max 99770 gas 99410 gas 0.3621% 🚨
closeLong: min 1690 gas 755 gas 123.8411% 🚨
closeLong: avg 24903 gas 24301 gas 2.4773% 🚨
closeLong: max 114138 gas 114894 gas -0.6580%
closeShort: min 1693 gas 713 gas 137.4474% 🚨
closeShort: avg 27869 gas 27140 gas 2.6861% 🚨
closeShort: max 108882 gas 113274 gas -3.8773%
initialize: min 1605 gas 706 gas 127.3371% 🚨
initialize: avg 179603 gas 178066 gas 0.8632% 🚨
initialize: max 254125 gas 252327 gas 0.7126% 🚨
openLong: min 736 gas 757 gas -2.7741%
openLong: avg 55954 gas 57575 gas -2.8155%
openLong: max 195381 gas 219128 gas -10.8370%
openShort: min 702 gas 712 gas -1.4045%
openShort: avg 55385 gas 57068 gas -2.9491%
openShort: max 194872 gas 218660 gas -10.8790%
redeemWithdrawalShares: min 1598 gas
redeemWithdrawalShares: avg 22248 gas
redeemWithdrawalShares: max 49538 gas
removeLiquidity: min 1661 gas 777 gas 113.7709% 🚨
removeLiquidity: avg 79069 gas 78310 gas 0.9692% 🚨
removeLiquidity: max 205229 gas 204374 gas 0.4184% 🚨

This comment was automatically generated by workflow using github-action-benchmark.

@coveralls
Copy link
Collaborator

coveralls commented Nov 8, 2023

Coverage Status

coverage: 96.217% (-0.004%) from 96.221%
when pulling 962af42 on remove_extra_return_param_from_calc_fees
into c9d52c1 on main.

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.

Nice work overall - the notes in the PR description are great! Some nits to address. I am going to wait to approve until @jalextowle can review and the issues have been resolved.

Copy link
Contributor

@jalextowle jalextowle left a comment

Choose a reason for hiding this comment

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

Nice cleanup! These changes are great. Let me know once the nits are addressed, and I'll take another look.

jalextowle
jalextowle previously approved these changes Nov 10, 2023
Copy link
Contributor

@jalextowle jalextowle left a comment

Choose a reason for hiding this comment

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

LGTM

@wakamex wakamex requested a review from jrhea November 11, 2023 16:03
@wakamex wakamex force-pushed the remove_extra_return_param_from_calc_fees branch from 5ce5778 to 7971e5e Compare November 14, 2023 20:54
@jalextowle jalextowle self-requested a review November 14, 2023 20:58
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.

Looks good now. i'll hold off until alex approves

Copy link
Contributor

@jalextowle jalextowle left a comment

Choose a reason for hiding this comment

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

LGTM

@wakamex wakamex merged commit 6a4edcf into main Nov 16, 2023
@wakamex wakamex deleted the remove_extra_return_param_from_calc_fees branch November 16, 2023 00:46
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.

4 participants