feat: add L1PortalExecuteL2Call template for L2 actions through OptimismPortal#1173
feat: add L1PortalExecuteL2Call template for L2 actions through OptimismPortal#11730xiamflux wants to merge 23 commits into
Conversation
Co-authored-by: 0xOneTony <112496816+0xOneTony@users.noreply.github.com> Signed-off-by: IamFlux <175354924+0xiamflux@users.noreply.github.com>
Rehearsal: Governor Upgrade on L2 via Optimism Portal
|
|
||
| # Portal + L2 call params | ||
| portal = "0xbEb5Fc579115071764c7423A4f12eDde41f106Ed" # L1 OptimismPortal | ||
| l2Target = "0x1234567890123456789012345678901234567890" # replace with real OptimismGovernor Proxy |
There was a problem hiding this comment.
The placeholder address 0x1234567890123456789012345678901234567890 should be replaced with the actual OptimismGovernor Proxy address on L2 before using this configuration for the rehearsal. Using a real address ensures that the simulation and validation steps will accurately reflect the intended behavior when executed on-chain.
Spotted by Diamond
Is this helpful? React 👍 or 👎 to let us know.
| # Portal + L2 call params | ||
| portal = "0xbEb5Fc579115071764c7423A4f12eDde41f106Ed" # L1 OptimismPortal | ||
| l2Target = "0x1234567890123456789012345678901234567890" # replace with real OptimismGovernor Proxy | ||
| l2Data = "0x3659cfe60000000000000000000000000000000000000000000000000000000000001234" # upgradeTo(0x00..1234) |
There was a problem hiding this comment.
The placeholder address 0x00..1234 in the L2Data is not a valid implementation address for a production deployment. For a proper rehearsal, consider replacing this with either:
- The current implementation address (for a true no-op upgrade)
- A valid implementation contract address that exists on L2
This ensures the rehearsal accurately simulates a real governance action. The test example in 014-noop-call-optimismportal demonstrates this approach by using the current implementation address.
Spotted by Diamond
Is this helpful? React 👍 or 👎 to let us know.
| @@ -0,0 +1,265 @@ | |||
| # Rehearsal 4 - No-op Governor Upgrade | |||
There was a problem hiding this comment.
Why is this being put into the rehearsals directory?
There was a problem hiding this comment.
GM, our goal here was to add a rehearsal task for a no-op upgrade on the Optimism Governor Proxy on L2 using a newly added template. What would be the best place to do this?
There was a problem hiding this comment.
hm, this rehearsals directory contains templates and ceremonies related to onboarding Security Council members. I don't think this is what you want to do right?
There was a problem hiding this comment.
The task was to create a rehearsal upgrade ceremony for the Security Council to complete an end-to-end process of a no-op upgrade in a way that is repeatable.
Which would be a better suited place to put it?
There was a problem hiding this comment.
Is this simply to test this templates functionality? Or have you been in touch with the security council and agreed that this should be a new rehearsal for them to perform when onboarding a security council memeber?
If it's the former, you don't need to add this as a rehearsal. When you create a new template, you just need to follow this guide: https://github.com/ethereum-optimism/superchain-ops/blob/main/src/improvements/doc/NEW_TEMPLATE_GUIDE.md (1. make template 2. make Regression test 3. make example task
There was a problem hiding this comment.
We just removed the rehearsal docs in the rehearsal folder on this commit.
| import {Action} from "../../libraries/MultisigTypes.sol"; | ||
|
|
||
| import {IOptimismPortal2} from "lib/optimism/packages/contracts-bedrock/interfaces/L1/IOptimismPortal2.sol"; | ||
|
|
| } | ||
|
|
||
| /// @notice Validate that exactly one action to the portal with the expected calldata and value was captured. | ||
| function _validate(VmSafe.AccountAccess[] memory, Action[] memory _actions, address) internal override { |
There was a problem hiding this comment.
| function _validate(VmSafe.AccountAccess[] memory, Action[] memory _actions, address) internal override { | |
| function _validate(VmSafe.AccountAccess[] memory, Action[] memory _actions, address) internal view override { |
| /// @notice Build the portal deposit action. WARNING: State changes here are reverted after capture. | ||
| function _build(address) internal override { | ||
| // Record the L1 portal call with value for action extraction. | ||
| IOptimismPortal2(portal).depositTransaction{value: valueWei}(l2Target, valueWei, gasLimit, isCreation, l2Data); |
There was a problem hiding this comment.
does this need to take a value? I'm curious as I don't think we've fully tested if the value actually gets transferred.
There was a problem hiding this comment.
For the particular use case we had in mind when adding the template it does not need the value, but I think for a general purpose solution is good to have it there. What would be the proper way to fully test and document this?
There was a problem hiding this comment.
Can you hardcode 0 for this first version?
When performing an upgrade action i.e. portal.depositTransaction, this function is executed inside a Multicall3 contract. The safe delegatecall's to the Multicall3 contract. Right now, this will fail because of the require check inside the Multicall3 contract: https://github.com/mds1/multicall3/blob/main/src/Multicall3.sol#L160 (see: stackexchange)
There was a problem hiding this comment.
got it, do you want to close this PR in favor of #1231?
| IOptimismPortal2(portal).depositTransaction{value: valueWei}(l2Target, valueWei, gasLimit, isCreation, l2Data); | ||
| } | ||
|
|
||
| /// @notice Validate that exactly one action to the portal with the expected calldata and value was captured. |
There was a problem hiding this comment.
The _validate function is missing natspec documentation. According to the Solidity Guide, functions should use triple-slash natspec comment style with @notice tags instead of single-line comments. Replace the single-line comment with proper natspec documentation using /// @notice.
Spotted by Diamond (based on custom rule: Custom rules)
Is this helpful? React 👍 or 👎 to let us know.
| MultisigTaskPrinter.printTitle("Validated portal deposit action"); | ||
| } | ||
|
|
||
| /// @notice No code exceptions required for this template. |
There was a problem hiding this comment.
The _getCodeExceptions function is missing natspec documentation. According to the Solidity Guide, functions should use triple-slash natspec comment style with @notice tags instead of single-line comments. Replace the single-line comment with proper natspec documentation using /// @notice.
Spotted by Diamond (based on custom rule: Custom rules)
Is this helpful? React 👍 or 👎 to let us know.
| ## Creating and signing a new rehearsal ceremony | ||
|
|
||
| To create a new rehearsal ceremony, follow the instructions in the _Facilitator_ section of the README files for each of the following rehearsals: | ||
|
|
There was a problem hiding this comment.
nit, can undo this diff since we don't touch the rehearsals dir anywhere else here
| import {VmSafe} from "forge-std/Vm.sol"; | ||
| import {stdToml} from "forge-std/StdToml.sol"; | ||
|
|
||
| import {IOptimismPortal2} from "lib/optimism/packages/contracts-bedrock/interfaces/L1/IOptimismPortal2.sol"; |
There was a problem hiding this comment.
Instead of importing the interface, let's define the relevant interface in this file with interface IOptimismPortal2 { ... }. The reason for this is so that, in the future, the template can keep working for chains running older versions of the OP stack, even if the monorepo submodule version is bumped. In other words, we don't want templates to suddenly break when we bump the submodule version (i.e. if we change the portal interface in the future) because not all chains are necessarily on the latest version at all times, so we prefer to make sure templates continue to work
| // -------- Config inputs -------- | ||
| address payable public portal; // L1 OptimismPortal address | ||
| address public l2Target; // L2 target address | ||
| bytes public l2Data; // Inner L2 calldata |
There was a problem hiding this comment.
nit, just to be more precise, assuming this is correct
| bytes public l2Data; // Inner L2 calldata | |
| bytes public l2Data; // Inner L2 calldata, i.e. the calldata to execute on `l2Target` |
|
|
||
| isCreation = false; | ||
| try vm.parseTomlBool(_toml, ".isCreation") returns (bool _b) { | ||
| isCreation = _b; |
There was a problem hiding this comment.
The portal has this check so it's arguably unnecessary, but might be better UX to check and revert here if isCreation && l2Target != address(0). Up to you
There was a problem hiding this comment.
I do like this, makes sense to early revert before reaching the portal.
| // Resolve portal from registry first if available, else read explicit field. | ||
| try simpleAddrRegistry.get("OptimismPortal") returns (address p) { | ||
| portal = payable(p); | ||
| } catch { | ||
| portal = payable(_toml.readAddress(".portal")); | ||
| } |
There was a problem hiding this comment.
For the revshare use case, it might be better UX (fewer playbooks, fewer signatures) to have this template support doing a deposit transaction for many L2s in the same transaction. In that case, we would want to inherit from L2TaskBas instead and have the config inputs be arrays corresponding to the deposit. What do you think here?
There was a problem hiding this comment.
Do you think it makes sense to have 2 separate templates one for simpler use cases and the other for multi chain calls?
There was a problem hiding this comment.
As we continued working on the Rev Share use case, we have came to the conclusion it should have it’s own, separate template and we can keep this as a general purpose template for more simple cases. Wdyt?
There was a problem hiding this comment.
Having a separate template is good with me!
| // Read hex string and parse to bytes. | ||
| string memory _dataHex = _toml.readString(".l2Data"); | ||
| l2Data = vm.parseBytes(_dataHex); | ||
| require(l2Data.length > 0, "l2Data must be set"); |
There was a problem hiding this comment.
Could just do _toml.readBytes here
| bytes memory _expected = abi.encodeWithSelector( | ||
| IOptimismPortal2.depositTransaction.selector, l2Target, valueWei, gasLimit, isCreation, l2Data | ||
| ); |
There was a problem hiding this comment.
This task was recently changed to CANCELLED so we should be able to undo the diff in this task's dir
| contract L1PortalExecuteL2Call is SimpleTaskBase { | ||
| using stdToml for string; | ||
|
|
||
| // -------- Config inputs -------- |
There was a problem hiding this comment.
The comment // -------- Config inputs -------- should use triple-slash Solidity NatSpec format: /// -------- Config inputs -------- per the Solidity style guide requirements.
| // -------- Config inputs -------- | |
| /// -------- Config inputs -------- |
Spotted by Diamond (based on custom rule: Custom rules)
Is this helpful? React 👍 or 👎 to let us know.
d26ced1 to
3f3b8d9
Compare
|
Continue review in #1231, leaving this open for now for future reference. |
Description
src/improvements/template/L1PortalExecuteL2Call.solIOptimismPortal2(depositTransaction) to dispatch an L2 call from an L1 Safeconfig.toml:portal,l2Target,l2Data,gasLimit(uint64),value(default 0),isCreation(default false)SimpleTaskBasefor L1 multisig flows;safeAddressString()defaults toProxyAdminOwnerOptimismPortalsrc/improvements/interfaces/IOptimismPortal.sol(completeIOptimismPortal2)src/improvements/tasks/eth/rehearsals/2025-08-11-l1-to-l2-noop/config.tomlplaceholders and signerREADME.mdwith simulate/validate/sign stepsTests