Nadir/deploy refactor #790
Conversation
|
|
| IEigenPodManager(newEigenPodManager), // update EigenPodManager address | ||
| params.EIGENPOD_GENESIS_TIME | ||
| )), | ||
| envToUpdate: "eigenPod.pendingImpl" |
There was a problem hiding this comment.
Interesting -- this seems like it's leaking a bit of complexity. I figured zeus would take care of this given the name field?
There was a problem hiding this comment.
If zeus isn't able to handle this for us, we should have constants defined or something that makes it harder to supply the wrong value here.
And I guess I'd question whether the name field is relevant
There was a problem hiding this comment.
Yeah, I think a better way to do this is to have that envToUpdate field be a boolean, either to update the name reserved config value's pending impl or not. Good callout
There was a problem hiding this comment.
Discussion:
- Unclear whether we're passing deployments between scripts via env variables or as updated values in JSON config. Should clarify with Justin what he expects/has been building - Nadir/I are fine with either.
- Regardless of whether we're using env variables or updating JSON, the script writer should not have to pass in raw strings to reference storage locations.
- If using env variables, we will need a library mapping strings to env variables
- If using JSON, we should be able to discern the location in the JSON by referencing the field getting updated (e.g. by passing in
addrs.eigenPod.pendingImpland the abstraction resolves this to the correct JSON key)
Other observations:
- Moving solely to env variables might be the play eventually - possibly less brittle.
- Backwards-compatibility issues will probably exist regardless of using JSON or ENV variables, so different versions may require different env/json schemas
|
|
||
| vm.stopBroadcast(); | ||
|
|
||
| return _deployments; |
There was a problem hiding this comment.
Do we need to return this if it's in a state variable the parent method has access to?
There was a problem hiding this comment.
- I prefer explicitly returning up values rather than implicitly modifying state variables.
- I actually meant to have this not be in the parent contract 😅 and instead have every contract inheriting
EOADeployerdefine their own_deploymentsarray local to their own contract to avoid 1).
Will update this in the templates branch and this PR
| function setUp() public { | ||
| (Addresses memory addrs, Environment memory env, Params memory params) = _readConfigFile("script/configs/zipzoop.json"); | ||
| _deploy(addrs, env, params); | ||
| } |
There was a problem hiding this comment.
Is this for testing purposes? (if so, that's fine - just wondering!)
There was a problem hiding this comment.
Yep. Though one big concern with this is that it actually modifies the state variable to call _deploy() twice even when not running the tests specifically -- wondering if it makes more sense to separate out tests into a different file, or to simply not use setUp()
|
|
||
| MultisigCall[] internal _executorCalls; | ||
|
|
||
| function queue(Addresses memory addrs, Environment memory env, Params memory params) public override returns (MultisigCall[] memory) { |
There was a problem hiding this comment.
Weird that this method in particular is public rather than the internal pattern the other two scripts use
There was a problem hiding this comment.
ah, i see now why it's public, used in script 3.
eesh, that feels like a weird pattern!
There was a problem hiding this comment.
2 main reasons:
- this is the only time we do this pattern (deploy a script and call it from another script)
- deploys/calls like this should be minimized because then you have to be careful how/when you use start/stop broadcast
There was a problem hiding this comment.
would much rather make this an inheritance relationship somehow
| _multisigCalls.append({ | ||
| to: addrs.timelock, | ||
| value: 0, | ||
| data: abi.encodeWithSelector( | ||
| ITimelock.executeTransaction.selector, | ||
| executorCalldata | ||
| ) | ||
| }); | ||
|
|
||
| // after queued transaction, renounce ownership from eigenPodManager | ||
| _multisigCalls.append({ | ||
| to: addrs.eigenPodManager.proxy, | ||
| value: 0, | ||
| data: abi.encodeWithSelector( | ||
| EigenPodManager(addrs.eigenPodManager.proxy).renounceOwnership.selector | ||
| ) | ||
| }); | ||
|
|
||
| return _multisigCalls; |
There was a problem hiding this comment.
Kind of a strange mix:
- when we write the
queuescript, we don't worry about the ops flow and the fact that we'll be callingqueueTransactionis abstracted away - when we write the
execute, we do worry about the ops flow and explicitly callexecuteTransaction
This distinction doesn't feel apparent given the structure of the code/templates.
The PR adds
script/Release_Template.sol, a template file providing abstract contracts for a variety of scripting use cases. A release script can inherit and override relevant functions with lightweight and straightforward syntax. Below are the contexts for using each script:EOABuilder: An EOA performing a deploy, returning aDeploymentobject.MultisigBuilder: A multisig performing some direct action (e.g. sending tokens, acting as a role on a contract), returning a Safe MetaTransactionData object.MultisigBuilderscript transforms calls into MultiSendCallOnly calls, meaning any number of calls (not accounting for gas) can be batched into one transaction by a multisig.OpsTimelockBuilder: A specialized nested multisig action performing aqueue()andexecute()queue()refers to queueing a transaction in the Timelock andexecute()refers to forwarding that transaction after some delay to the Executor Multisig. You can see more details on the EigenLayer admin structure here.Furthermore,
script/releases/v0.1-eigenpod/contains an example set of scripts inheriting and implementing functions for each aforementioned abstract contract. These scripts are purely for demonstration.DeployEigenPod.s.sol: Deploys a single contract,EigenPod.UpgradeEigenPod.s.sol: Sends a dummy call to the Timelock, plus an upgrade to theeigenPodto some pending implementation.UpgradeViaTimelock.s.sol: Queues up a transaction in the Timelock, then later allows for the same transaction to be executed.=======================================
To run this locally, try the following commands.
Deploy
forge script DeployEigenPodAndManager -s "deploy(string memory)" "script/configs/zipzoop.json"Example Output
Execute
forge script UpgradeEigenPod.s.sol -s "execute(string memory)" "script/configs/zipzoop.json"Example Output
Queue
forge script UpgradeEigenPodAndManager -s "queue(string memory)" "script/configs/zipzoop.json"Example Output
Execute after Queue
forge script UpgradeEigenPodAndManager -s "execute(string memory)" "script/configs/zipzoop.json"Example Output