-
Notifications
You must be signed in to change notification settings - Fork 126
feat: add L1PortalExecuteL2Call template for L2 actions through OptimismPortal #1173
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
Changes from 15 commits
289bdd4
fb1de79
27de7e3
64e7689
a4bc70d
0f97595
dfadb1f
9b22137
212a84e
fe44a7e
b823c6a
a9b18e6
51d63c5
543b409
cc569d3
fcc89ba
fadfa08
a3ca79e
0caf083
c0cd688
0af20c9
cde2b35
3f3b8d9
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This task was recently changed to CANCELLED so we should be able to undo the diff in this task's dir |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,10 +1,11 @@ | ||
| l2chains = [{name = "Worldchain Sepolia", chainId = 4801}] | ||
| l2chains = [{name = "Worldchain Sepolia", chainId = 4801}] | ||
| templateName = "TransferOwners" | ||
| safeAddressString = "ProxyAdminOwner" | ||
|
|
||
| # Standard Sepolia L1PAO - https://github.com/ethereum-optimism/superchain-registry/blob/93c5073d233cb9011a95aebf275270fd00346400/validation/standard/standard-config-roles-sepolia.toml#L3 | ||
| newOwner = "0x1Eb2fFc903729a0F03966B917003800b145F56E2" | ||
|
|
||
| [stateOverrides] | ||
| 0x945185c01fb641ba3e63a9bdf66575e35a407837 = [ # Worldchain L1PAO | ||
| {key = "0x0000000000000000000000000000000000000000000000000000000000000005", value = 45} | ||
| {key = "0x0000000000000000000000000000000000000000000000000000000000000005", value = 51} | ||
| ] |
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
| @@ -0,0 +1,117 @@ | ||||||
| // SPDX-License-Identifier: MIT | ||||||
| pragma solidity 0.8.15; | ||||||
|
|
||||||
| 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"; | ||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Instead of importing the interface, let's define the relevant interface in this file with |
||||||
| import {MultisigTaskPrinter} from "../../libraries/MultisigTaskPrinter.sol"; | ||||||
| import {Action} from "../../libraries/MultisigTypes.sol"; | ||||||
| import {SimpleTaskBase} from "../tasks/types/SimpleTaskBase.sol"; | ||||||
|
|
||||||
| /// @notice Template to execute an L2 call via the L1 Optimism Portal from a nested L1 Safe. | ||||||
| /// Sends an L2 transaction using OptimismPortal.depositTransaction with config-driven params. | ||||||
| contract L1PortalExecuteL2Call is SimpleTaskBase { | ||||||
|
blmalone marked this conversation as resolved.
|
||||||
| using stdToml for string; | ||||||
|
|
||||||
| // -------- Config inputs -------- | ||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The comment
Suggested change
Spotted by Diamond (based on custom rule: Custom rules) |
||||||
| address payable public portal; // L1 OptimismPortal address | ||||||
| address public l2Target; // L2 target address | ||||||
| bytes public l2Data; // Inner L2 calldata | ||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit, just to be more precise, assuming this is correct
Suggested change
|
||||||
| uint256 public valueWei; // ETH value to forward to L2 (defaults to 0) | ||||||
| uint64 public gasLimit; // L2 gas limit (required) | ||||||
| bool public isCreation; // Whether to create a contract on L2 (defaults to false) | ||||||
|
|
||||||
| /// @notice Default Safe name. Can be overridden via `safeAddressString` in config.toml. | ||||||
| function safeAddressString() public pure override returns (string memory) { | ||||||
| return "ProxyAdminOwner"; | ||||||
| } | ||||||
|
|
||||||
| /// @notice The contracts expected to have storage writes during execution. | ||||||
| /// Allowlist the OptimismPortal since it will mutate state (queue/event) on deposit. | ||||||
| function _taskStorageWrites() internal pure override returns (string[] memory) { | ||||||
| string[] memory _storageWrites = new string[](1); | ||||||
| _storageWrites[0] = "OptimismPortal"; | ||||||
| return _storageWrites; | ||||||
| } | ||||||
|
|
||||||
| /// @notice The contracts expected to have balance changes during execution. | ||||||
| /// Allowlist the OptimismPortal to receive ETH (value) in the deposit call. | ||||||
| function _taskBalanceChanges() internal pure override returns (string[] memory) { | ||||||
| string[] memory _balanceChanges = new string[](1); | ||||||
| _balanceChanges[0] = "OptimismPortal"; | ||||||
| return _balanceChanges; | ||||||
| } | ||||||
|
|
||||||
| /// @notice Parse config and initialize template variables. | ||||||
| /// Expected TOML keys: | ||||||
| /// - portal: address (L1 OptimismPortal) OR addresses.OptimismPortal in [addresses] | ||||||
| /// - l2Target: address (L2 target address) | ||||||
| /// - l2Data: hex string (e.g. 0x1234...) | ||||||
| /// - gasLimit: uint (will be cast to uint64) | ||||||
| /// - value: uint (optional, default 0) | ||||||
| /// - isCreation: bool (optional, default false) | ||||||
| function _templateSetup(string memory _taskConfigFilePath, address) internal override { | ||||||
| string memory _toml = vm.readFile(_taskConfigFilePath); | ||||||
|
|
||||||
| // 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")); | ||||||
| } | ||||||
|
Comment on lines
+75
to
+80
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do you think it makes sense to have 2 separate templates one for simpler use cases and the other for multi chain calls?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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?
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Having a separate template is good with me! |
||||||
| require(portal != address(0), "portal must be set (addresses.OptimismPortal or .portal)"); | ||||||
|
|
||||||
| l2Target = _toml.readAddress(".l2Target"); | ||||||
| require(l2Target != address(0), "l2Target must be set"); | ||||||
|
|
||||||
| // 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"); | ||||||
|
Comment on lines
+86
to
+88
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Could just do |
||||||
|
|
||||||
| uint256 _gasLimitTmp = _toml.readUint(".gasLimit"); | ||||||
| require(_gasLimitTmp > 0 && _gasLimitTmp <= type(uint64).max, "invalid gasLimit"); | ||||||
| gasLimit = uint64(_gasLimitTmp); | ||||||
|
|
||||||
| // Optional fields | ||||||
| valueWei = 0; | ||||||
| try vm.parseTomlUint(_toml, ".value") returns (uint256 _v) { | ||||||
| valueWei = _v; | ||||||
| } catch {} | ||||||
|
|
||||||
| isCreation = false; | ||||||
| try vm.parseTomlBool(_toml, ".isCreation") returns (bool _b) { | ||||||
| isCreation = _b; | ||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The portal has this check so it's arguably unnecessary, but might be better UX to check and revert here if
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I do like this, makes sense to early revert before reaching the portal. |
||||||
| } catch {} | ||||||
| } | ||||||
|
|
||||||
| /// @notice Build the portal deposit action. WARNING: State changes here are reverted after capture. | ||||||
|
blmalone marked this conversation as resolved.
|
||||||
| 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); | ||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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?
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can you hardcode When performing an upgrade action i.e.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Addressed in here.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. got it, do you want to close this PR in favor of #1231? |
||||||
| } | ||||||
|
|
||||||
| /// @notice Validate that exactly one action to the portal with the expected calldata and value was captured. | ||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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) |
||||||
| function _validate(VmSafe.AccountAccess[] memory, Action[] memory _actions, address) internal view override { | ||||||
| bytes memory _expected = abi.encodeWithSelector( | ||||||
| IOptimismPortal2.depositTransaction.selector, l2Target, valueWei, gasLimit, isCreation, l2Data | ||||||
| ); | ||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit, prefer |
||||||
|
|
||||||
| bool _found; | ||||||
| uint256 _matches; | ||||||
| for (uint256 _i = 0; _i < _actions.length; _i++) { | ||||||
| if (_actions[_i].target == portal && _actions[_i].value == valueWei) { | ||||||
| if (keccak256(_actions[_i].arguments) == keccak256(_expected)) { | ||||||
| _found = true; | ||||||
| _matches++; | ||||||
| } | ||||||
| } | ||||||
| } | ||||||
| require(_found && _matches == 1, "expected one portal deposit action"); | ||||||
| MultisigTaskPrinter.printTitle("Validated portal deposit action"); | ||||||
| } | ||||||
|
|
||||||
| /// @notice No code exceptions required for this template. | ||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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) |
||||||
| function _getCodeExceptions() internal view override returns (address[] memory) {} | ||||||
| } | ||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,3 @@ | ||
| TENDERLY_GAS=10000000 | ||
| NESTED_SAFE_NAME_DEPTH_1=council | ||
| FORK_BLOCK_NUMBER=23197819 |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,13 @@ | ||
| templateName = "L1PortalExecuteL2Call" | ||
|
|
||
| # Portal + L2 call params | ||
| portal = "0xbEb5Fc579115071764c7423A4f12eDde41f106Ed" # L1 OptimismPortal | ||
| l2Target = "0xcDF27F107725988f2261Ce2256bDfCdE8B382B10" # OptimismGovernor Proxy | ||
| l2Data = "0x3659cfe6000000000000000000000000ecbf4ed9f47302f00f0f039a691e7db83bdd2624" # upgradeTo(currentImpl) -> 0xecbf4ed9f47302f00f0f039a691e7db83bdd2624 | ||
| gasLimit = 500000 | ||
| value = 0 | ||
| isCreation = false | ||
|
|
||
| [addresses] | ||
| ProxyAdminOwner = "0x5a0Aae59D09fccBdDb6C6CcEB07B7279367C3d2A" # 2-of-2 between council and foundation | ||
| OptimismPortal = "0xbEb5Fc579115071764c7423A4f12eDde41f106Ed" |
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, can undo this diff since we don't touch the rehearsals dir anywhere else here