-
Notifications
You must be signed in to change notification settings - Fork 359
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
feat: rewards v2 upgrade script #888
Conversation
7cf6917
to
c36c54a
Compare
b34c492
to
ad4a0bc
Compare
This PR LGTM. I added one (fairly minor) note above about correctness/consistency checks. |
@MinisculeTarantula - if the above looks good (modulo the feedback), please approve or reject ty <3 |
4c5a4af
to
8c62537
Compare
5337c88
to
5663b7c
Compare
5663b7c
to
579bb1e
Compare
@MinisculeTarantula - please reconfirm and +1 once you've reproduced the tests with zeus independently shared zeus with you via DM |
bytes memory executorMultisigCalldata = address(this._timelock()).calldataToExecTransaction( | ||
this._proxyAdmin(), | ||
proxyAdminCalldata, | ||
EncGnosisSafe.Operation.Call | ||
); | ||
|
||
return (executorMultisigCalldata); | ||
} | ||
|
||
function _runAsMultisig() internal virtual override { | ||
bytes memory executorMultisigCalldata = _getMultisigTransactionCalldata(); | ||
|
||
TimelockController timelock = TimelockController(payable(this._timelock())); | ||
timelock.schedule( | ||
this._executorMultisig(), | ||
0 /* value */, | ||
executorMultisigCalldata, | ||
0 /* predecessor */, | ||
bytes32(0) /* salt */, | ||
timelock.getMinDelay() | ||
); |
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.
Our typical pattern when writing calls with multiple (~ 3+) args is to use solidity's named argument syntax, e.g:
bytes memory executorMultisigCalldata = address(this._timelock()).calldataToExecTransaction({
to: this._proxyAdmin(),
data: proxyAdminCalldata,
op: EncGnosisSafe.Operation.Call
});
Helps disambiguate long lists of params. Same goes for timelock.schedule
!
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.
Not able to run Zeus, but the scripts look correct, and deployed impl looks correct.
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.
LGTM. Gave it some thorough manual review and local testing. The tests are super helpful at providing strong assurance that the scripts do what we expect them to do.
Comments regard primarily aesthetic concerns, so accepting not making those changes now in favor of minimizing diffs at this stage.
import {ITimelock} from "zeus-templates/interfaces/ITimelock.sol"; | ||
import {console} from "forge-std/console.sol"; | ||
import {EncGnosisSafe} from "zeus-templates/utils/EncGnosisSafe.sol"; | ||
import {MultisigCallUtils, MultisigCall} from "zeus-templates/utils/MultisigCallUtils.sol"; |
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.
Can remove this line, imported above
import {MultisigCallUtils, MultisigCall} from "zeus-templates/utils/MultisigCallUtils.sol"; |
using MultisigCallUtils for MultisigCall[]; | ||
using EigenLabsUpgrade for *; | ||
using EncGnosisSafe for *; | ||
using MultisigCallUtils for *; |
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.
Declared above
using MultisigCallUtils for *; |
MultisigCall[] private _executorCalls; | ||
MultisigCall[] private _opsCalls; |
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.
Unused
MultisigCall[] private _executorCalls; | |
MultisigCall[] private _opsCalls; |
Summary
Testing
Run tests with:
zeus test --verbose script/releases/v0.1.0-rewardsv2/*.s.sol --env preprod