Worldchain sepolia key handback over #1208
Conversation
…n-ops into worldchain-sepolia-key-handback-over
![graphite-app[bot]](https://avatars.githubusercontent.com/in/158384?s=60&v=4){kind=small}
| interface OptimismPortal { | ||
| function depositTransaction(address _to, uint256 _value, uint64 _gasLimit, bool _isCreation, bytes memory _data) | ||
| external | ||
| payable; | ||
| } | ||
|
|
||
| interface ProxyAdmin { | ||
| function owner() external view returns (address); | ||
| function transferOwnership(address newOwner) external; | ||
| } |
There was a problem hiding this comment.
The OptimismPortal and ProxyAdmin interfaces are missing natspec documentation. According to the Solidity style guide, all interfaces should use triple-slash natspec comment style with @notice tags to document their purpose and functions. Add proper natspec documentation above each interface and their functions to explain what they do.
| interface OptimismPortal { | |
| function depositTransaction(address _to, uint256 _value, uint64 _gasLimit, bool _isCreation, bytes memory _data) | |
| external | |
| payable; | |
| } | |
| interface ProxyAdmin { | |
| function owner() external view returns (address); | |
| function transferOwnership(address newOwner) external; | |
| } | |
| /// @notice Interface for interacting with the Optimism Portal, which handles cross-chain transactions from L1 to L2 | |
| interface OptimismPortal { | |
| /// @notice Deposits a transaction from L1 to L2 | |
| /// @param _to The address to send the transaction to on L2 | |
| /// @param _value The amount of ETH to send with the transaction | |
| /// @param _gasLimit The gas limit for the L2 transaction | |
| /// @param _isCreation Whether the transaction is a contract creation | |
| /// @param _data The calldata for the transaction | |
| function depositTransaction(address _to, uint256 _value, uint64 _gasLimit, bool _isCreation, bytes memory _data) | |
| external | |
| payable; | |
| } | |
| /// @notice Interface for interacting with a ProxyAdmin contract that manages proxy contracts | |
| interface ProxyAdmin { | |
| /// @notice Returns the current owner of the ProxyAdmin | |
| /// @return The address of the current owner | |
| function owner() external view returns (address); | |
| /// @notice Transfers ownership of the ProxyAdmin to a new address | |
| /// @param newOwner The address of the new owner | |
| function transferOwnership(address newOwner) external; | |
| } | |
Spotted by Diamond (based on custom rule: Custom rules)
Is this helpful? React 👍 or 👎 to let us know.
There was a problem hiding this comment.
Because there's a new template being added, we also need to include a Regression test in Regression.t.sol and a test task in test/example/....
There is a PR opened that allows arbitrary L2 executions (see here: src/improvements/template/L1PortalExecuteL2Call.sol) maybe we can get this merged and use it for your owner change? I've just made this connection?
| { | ||
| require(_preExecutionSnapshot == 0, "MultisigTask: already initialized"); | ||
| templateConfig.safeAddressString = loadSafeAddressString(MultisigTask(address(this)), _taskConfigFilePath); | ||
| // Commented out - address(this) not allowed in scripts |
There was a problem hiding this comment.
not following why these changes are needed in MultisigTask. Can you explain them to me some more?
There was a problem hiding this comment.
keeping address(this) in MultisigTask is causing this error for me:
Error: script failed: Usage of
address(this)detected in script contract. Script contracts are ephemeral and their addresses should not be relied upon.
error: Recipesimulatefailed with exit code 1
it seems like keeping address(this) doesnt work in scripts and we use it in two places. one is for getting the safeAddressString, which i changed to just calling safeAddressString() and the other uses address(this) to do debugging w vm.label.
There was a problem hiding this comment.
you need to make sure you're using mise to install the correct version of foundry. Can you follow the installation steps in the readme and try again? This change is not necessary.
src/improvements/tasks/sep/030-worldchain-l2pao-key-handback-over/config.toml
Show resolved
Hide resolved
![almanax-ai[bot]](https://avatars.githubusercontent.com/in/934833?s=60&v=4){kind=small}
| /// 1. Find the L2 deposit transaction sent from the L1 caller. | ||
| /// 2. The transaction should be interacting with the L2 ProxyAdmin at 0x4200000000000000000000000000000000000018. | ||
| /// 3. Verify that the OwnershipTransferred event was emitted with the correct new EOA owner. | ||
| contract TransferL2PAOFromL1ToEOA is L2TaskBase { |
There was a problem hiding this comment.
The contract TransferL2PAOFromL1ToEOA violates the Solidity style guide by not using triple-slash natspec comment style. The contract has a multi-line comment block using /// but should use the proper natspec format with @notice tags. According to the Solidity Guide, comments should use triple-slash solidity natspec comment style and always use @notice instead of @dev. The contract documentation should be reformatted to use proper natspec tags like /// @notice for the contract description.
| contract TransferL2PAOFromL1ToEOA is L2TaskBase { | |
| /// @notice A contract that handles transferring L2 PAO tokens from L1 to an EOA (Externally Owned Account) | |
| contract TransferL2PAOFromL1ToEOA is L2TaskBase { | |
Spotted by Diamond (based on custom rule: Custom rules)
Is this helpful? React 👍 or 👎 to let us know.
| SuperchainAddressRegistry.ChainInfo[] memory chains = superchainAddrRegistry.getChains(); | ||
|
|
||
| // See this Tenderly simulation for an example of this gas limit working: https://www.tdly.co/shared/simulation/d5028138-469c-4bb2-97fd-50f5f4bb8515 | ||
| uint64 gasLimit = 200000; |
There was a problem hiding this comment.
The hardcoded gas limit of 200000 reduces robustness and safety. Consider making this value configurable through the config file or adding validation to ensure the gas limit is sufficient for the specific L2 operation. While a Tenderly simulation is referenced in the comments, the template would be more robust with either runtime validation of gas sufficiency or configuration-based flexibility to handle different scenarios.
| uint64 gasLimit = 200000; | |
| uint64 gasLimit = config.getL2TransferGasLimit(); // Read from config instead of hardcoding |
Spotted by Diamond (based on custom rule: Superchain Ops task/template review)
Is this helpful? React 👍 or 👎 to let us know.
3 participants
Description
Tests
Additional context
Metadata