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

Fix Spearbit issue where governance fees are included in Share reserves #520

Merged
merged 14 commits into from
Jul 24, 2023
Merged

Fix Spearbit issue where governance fees are included in Share reserves #520

merged 14 commits into from
Jul 24, 2023

Conversation

ControlCplusControlV
Copy link
Contributor

@ControlCplusControlV ControlCplusControlV commented Jul 21, 2023 •
edited by jrhea
Loading

Closes #420

Followed the mitigation they recommended

Due the difficulty in replicating the unwanted behavior I unfortunately had to use a constant to test up against, but this ensures that every is tracked correctly

@ControlCplusControlV ControlCplusControlV added bug Something isn't working audit labels Jul 21, 2023
@ControlCplusControlV ControlCplusControlV self-assigned this Jul 21, 2023
@github-actions
Copy link

github-actions bot commented Jul 21, 2023 •
edited
Loading

Hyperdrive Gas Benchmark

Benchmark suite Current: d092518 Previous: 71c6688 Deviation Status
addLiquidity: min 785 gas 785 gas 0% 🟰
addLiquidity: avg 43549 gas 43693 gas -0.3296% ✅
addLiquidity: max 80573 gas 80573 gas 0% 🟰
checkpoint: min 514 gas 514 gas 0% 🟰
checkpoint: avg 22776 gas 22776 gas 0% 🟰
checkpoint: max 32767 gas 32767 gas 0% 🟰
closeLong: min 852 gas 852 gas 0% 🟰
closeLong: avg 45541 gas 45415 gas 0.2774% 🚨
closeLong: max 85364 gas 77501 gas 10.1457% 🚨
closeShort: min 809 gas 809 gas 0% 🟰
closeShort: avg 40736 gas 40681 gas 0.1352% 🚨
closeShort: max 86800 gas 86800 gas 0% 🟰
initialize: min 714 gas 714 gas 0% 🟰
initialize: avg 159389 gas 159368 gas 0.0132% 🚨
initialize: max 233486 gas 233486 gas 0% 🟰
openLong: min 740 gas 740 gas 0% 🟰
openLong: avg 113317 gas 112890 gas 0.3782% 🚨
openLong: max 179472 gas 179472 gas 0% 🟰
openShort: min 782 gas 782 gas 0% 🟰
openShort: avg 150582 gas 150822 gas -0.1591% ✅
openShort: max 218974 gas 218899 gas 0.0343% 🚨
removeLiquidity: min 762 gas 762 gas 0% 🟰
removeLiquidity: avg 57509 gas 57231 gas 0.4858% 🚨
removeLiquidity: max 118973 gas 118973 gas 0% 🟰

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

@coveralls
Copy link
Collaborator

coveralls commented Jul 21, 2023 •
edited
Loading

Coverage Status

coverage: 98.248% (+0.005%) from 98.243% when pulling d092518 on controlc/fix_gov_share_issue into a3116be on main.

@ControlCplusControlV
Copy link
Contributor Author

image

jrhea
jrhea requested changes Jul 21, 2023
View reviewed changes
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.

A couple of suggestions

test/units/hyperdrive/OpenShortTest.t.sol Outdated Show resolved Hide resolved
jrhea
jrhea approved these changes Jul 24, 2023
View reviewed changes
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, just add a comment and i think it is good to go

test/units/hyperdrive/OpenShortTest.t.sol Show resolved Hide resolved
jalextowle
jalextowle approved these changes Jul 24, 2023
View reviewed changes
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.

Looks great! I have a few stylistic nits, but once those are done let's :shipit:.

@ControlCplusControlV ControlCplusControlV enabled auto-merge (squash) July 24, 2023 22:23
@ControlCplusControlV ControlCplusControlV merged commit 4730953 into main Jul 24, 2023
5 checks passed
@ControlCplusControlV ControlCplusControlV deleted the controlc/fix_gov_share_issue branch July 24, 2023 22:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jrhea jrhea jrhea approved these changes

@jalextowle jalextowle jalextowle approved these changes

Assignees
No one assigned
Labels
audit bug Something isn't working
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Governance fees shouldn't be part of share reserves when opening shorts
4 participants
@ControlCplusControlV @coveralls @jrhea @jalextowle