-
Notifications
You must be signed in to change notification settings - Fork 84
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
Set MEV fee recipient to FeeAccumulator for Native Staking Strategies #2179
Conversation
Added SSV approve and setFeeRecipientAddress to the Native Staking Strategy's initialize function Correct the Natspec on how tx fees are handled.
Added fork tests to set MEV fee recipient Removed old native staking fork test file that is no longer being used
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #2179 +/- ##
==========================================
- Coverage 59.22% 59.07% -0.16%
==========================================
Files 71 71
Lines 3505 3509 +4
Branches 910 687 -223
==========================================
- Hits 2076 2073 -3
- Misses 1426 1433 +7
Partials 3 3 ☔ View full report in Codecov by Sentry. |
Set MEV fee recipient to FeeAccumulator for Native Staking Strategies
🚨 Report Summary
For more details view the full report in OpenZeppelin Code Inspector |
ISSVNetwork(SSV_NETWORK).setFeeRecipientAddress( | ||
FEE_ACCUMULATOR_ADDRESS | ||
); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should setFeeRecipient
be made public
and used here to avoid duplicate code?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought about adding an internal _setFeeRecipient
function that was called by initialize
and setFeeRecipient
but that seems overkill for one line. You're suggestion of a public setFeeRecipient
is a better approach to the internal function. But I still think it's overkill for a single line. I think it makes it clearer what initialize
does.
IERC20(SSV_TOKEN).approve(SSV_NETWORK, type(uint256).max); | ||
|
||
// Set the FeeAccumulator as the address for SSV validators to send MEV rewards to | ||
ISSVNetwork(SSV_NETWORK).setFeeRecipientAddress( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: not a big deal, but this could also be called from the constructor
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it can't as it needs to come from the proxy contract, not the implementation contract. Hence it's in the initialize
function.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
great job LGTM
Contract Changes
setFeeRecipient
toNativeStakingSSVStrategy
that callssetFeeRecipientAddress
on theSSVNetwork
contract.approve
andsetFeeRecipientAddress
to the Native Staking Strategy'sinitialize
function.Updated Processes
Update the execution rewards to show transaction priority fee go to the strategy and only the MEV rewards go to the FeeAccumulator.
Updated the
initialize
function and required governance callsTesting
Added
Anyone should be able to set the MEV fee recipient
test to the SSV Native Staking behaviour tests incontracts/test/behaviour/ssvStrategy.js
.Deployment
Holesky
Mainnet
Added new deploy script to upgrade both the Native Staking Strategies and call
setFeeRecipient
.contracts/deploy/mainnet/104_upgrade_staking_strategies.js
Code Change Checklist
To be completed before internal review begins:
Internal review: