From 6cf35daa3bcf760f1b3927514e438caf087bfc17 Mon Sep 17 00:00:00 2001 From: Blaine Malone Date: Fri, 13 Sep 2024 20:24:10 -0400 Subject: [PATCH] feat: Front OPSM with Proxy and Initialize (#11875) * fix: getting stack underflow error. * feat: adding proxy infront of OPStackManager. * fix: PR comments, we're landing on using the initialize function over setRelease. * fix: rename function. * fix: nit * fix: infering proxy admin from superchain config. * fix: ran command: just pre-pr-no-build * fix: nits * fix: using CommonBase in DeployImplementations.s.sol. * op-chain-ops: pass superchain proxy admin address as input to deployments script --------- Co-authored-by: Matt Solomon Co-authored-by: protolambda --- op-chain-ops/interopgen/deploy.go | 1 + .../interopgen/deployers/implementations.go | 1 + .../scripts/DeployImplementations.s.sol | 75 ++++--- .../scripts/DeployOPChain.s.sol | 1 + .../scripts/libraries/DeployUtils.sol | 12 ++ packages/contracts-bedrock/semver-lock.json | 4 +- .../snapshots/abi/OPStackManager.json | 183 ++++++++++-------- .../snapshots/abi/OPStackManagerInterop.json | 183 ++++++++++-------- .../storageLayout/OPStackManager.json | 22 ++- .../storageLayout/OPStackManagerInterop.json | 22 ++- .../src/L1/OPStackManager.sol | 61 +++--- .../src/L1/OPStackManagerInterop.sol | 7 +- .../test/DeployImplementations.t.sol | 25 ++- .../test/L1/OPStackManager.t.sol | 21 +- packages/contracts-bedrock/test/Specs.t.sol | 4 +- .../test/vendor/Initializable.t.sol | 5 +- 16 files changed, 384 insertions(+), 243 deletions(-) diff --git a/op-chain-ops/interopgen/deploy.go b/op-chain-ops/interopgen/deploy.go index fc77cf53ed06..89982a876337 100644 --- a/op-chain-ops/interopgen/deploy.go +++ b/op-chain-ops/interopgen/deploy.go @@ -168,6 +168,7 @@ func deploySuperchainToL1(l1Host *script.Host, superCfg *SuperchainConfig) (*Sup Release: superCfg.Implementations.Release, SuperchainConfigProxy: superDeployment.SuperchainConfigProxy, ProtocolVersionsProxy: superDeployment.ProtocolVersionsProxy, + SuperchainProxyAdmin: superDeployment.SuperchainProxyAdmin, UseInterop: superCfg.Implementations.UseInterop, }) if err != nil { diff --git a/op-chain-ops/interopgen/deployers/implementations.go b/op-chain-ops/interopgen/deployers/implementations.go index a9baf7718598..29a1fb6dee99 100644 --- a/op-chain-ops/interopgen/deployers/implementations.go +++ b/op-chain-ops/interopgen/deployers/implementations.go @@ -19,6 +19,7 @@ type DeployImplementationsInput struct { Release string SuperchainConfigProxy common.Address ProtocolVersionsProxy common.Address + SuperchainProxyAdmin common.Address UseInterop bool // if true, deploy Interop implementations } diff --git a/packages/contracts-bedrock/scripts/DeployImplementations.s.sol b/packages/contracts-bedrock/scripts/DeployImplementations.s.sol index 49de2bc2e7da..f68db7511f5a 100644 --- a/packages/contracts-bedrock/scripts/DeployImplementations.s.sol +++ b/packages/contracts-bedrock/scripts/DeployImplementations.s.sol @@ -2,6 +2,7 @@ pragma solidity 0.8.15; import { Script } from "forge-std/Script.sol"; +import { CommonBase } from "forge-std/Base.sol"; import { LibString } from "@solady/utils/LibString.sol"; @@ -37,7 +38,7 @@ import { DeployUtils } from "scripts/libraries/DeployUtils.sol"; import { Solarray } from "scripts/libraries/Solarray.sol"; // See DeploySuperchain.s.sol for detailed comments on the script architecture used here. -contract DeployImplementationsInput { +contract DeployImplementationsInput is CommonBase { uint256 internal _withdrawalDelaySeconds; uint256 internal _minProposalSizeBytes; uint256 internal _challengePeriodSeconds; @@ -130,6 +131,15 @@ contract DeployImplementationsInput { require(address(_protocolVersionsProxy) != address(0), "DeployImplementationsInput: not set"); return _protocolVersionsProxy; } + + function superchainProxyAdmin() public returns (ProxyAdmin) { + SuperchainConfig proxy = this.superchainConfigProxy(); + // Can infer the superchainProxyAdmin from the superchainConfigProxy. + vm.prank(address(0)); + ProxyAdmin proxyAdmin = ProxyAdmin(Proxy(payable(address(proxy))).admin()); + require(address(proxyAdmin) != address(0), "DeployImplementationsInput: not set"); + return proxyAdmin; + } } contract DeployImplementationsOutput { @@ -169,7 +179,7 @@ contract DeployImplementationsOutput { require(false, "DeployImplementationsOutput: not implemented"); } - function checkOutput() public view { + function checkOutput() public { address[] memory addrs = Solarray.addresses( address(this.opsm()), address(this.optimismPortalImpl()), @@ -186,8 +196,9 @@ contract DeployImplementationsOutput { DeployUtils.assertValidContractAddresses(addrs); } - function opsm() public view returns (OPStackManager) { + function opsm() public returns (OPStackManager) { DeployUtils.assertValidContractAddress(address(_opsm)); + DeployUtils.assertImplementationSet(address(_opsm)); return _opsm; } @@ -292,24 +303,34 @@ contract DeployImplementations is Script { }); } + // Deploy and initialize a proxied OPStackManager. function createOPSMContract( DeployImplementationsInput _dii, DeployImplementationsOutput, - OPStackManager.Blueprints memory blueprints + OPStackManager.Blueprints memory blueprints, + string memory release, + OPStackManager.ImplementationSetter[] memory setters ) internal virtual - returns (OPStackManager opsm_) + returns (OPStackManager opsmProxy_) { SuperchainConfig superchainConfigProxy = _dii.superchainConfigProxy(); ProtocolVersions protocolVersionsProxy = _dii.protocolVersionsProxy(); + ProxyAdmin proxyAdmin = _dii.superchainProxyAdmin(); - vm.broadcast(msg.sender); - opsm_ = new OPStackManager({ - _superchainConfig: superchainConfigProxy, - _protocolVersions: protocolVersionsProxy, - _blueprints: blueprints - }); + vm.startBroadcast(msg.sender); + Proxy proxy = new Proxy(address(msg.sender)); + OPStackManager opsm = new OPStackManager(superchainConfigProxy, protocolVersionsProxy); + + OPStackManager.InitializerInputs memory initializerInputs = + OPStackManager.InitializerInputs(blueprints, setters, release, true); + proxy.upgradeToAndCall(address(opsm), abi.encodeWithSelector(opsm.initialize.selector, initializerInputs)); + + proxy.changeAdmin(address(proxyAdmin)); // transfer ownership of Proxy contract to the ProxyAdmin contract + vm.stopBroadcast(); + + opsmProxy_ = OPStackManager(address(proxy)); } function deployOPStackManager(DeployImplementationsInput _dii, DeployImplementationsOutput _dio) public virtual { @@ -329,9 +350,6 @@ contract DeployImplementations is Script { vm.stopBroadcast(); // forgefmt: disable-end - // This call contains a broadcast to deploy OPSM. - OPStackManager opsm = createOPSMContract(_dii, _dio, blueprints); - OPStackManager.ImplementationSetter[] memory setters = new OPStackManager.ImplementationSetter[](6); setters[0] = OPStackManager.ImplementationSetter({ name: "L1ERC721Bridge", @@ -359,8 +377,8 @@ contract DeployImplementations is Script { info: OPStackManager.Implementation(address(_dio.l1StandardBridgeImpl()), L1StandardBridge.initialize.selector) }); - vm.broadcast(msg.sender); - opsm.setRelease({ _release: release, _isLatest: true, _setters: setters }); + // This call contains a broadcast to deploy OPSM which is proxied. + OPStackManager opsm = createOPSMContract(_dii, _dio, blueprints, release, setters); vm.label(address(opsm), "OPStackManager"); _dio.set(_dio.opsm.selector, address(opsm)); @@ -571,21 +589,30 @@ contract DeployImplementationsInterop is DeployImplementations { function createOPSMContract( DeployImplementationsInput _dii, DeployImplementationsOutput, - OPStackManager.Blueprints memory blueprints + OPStackManager.Blueprints memory blueprints, + string memory release, + OPStackManager.ImplementationSetter[] memory setters ) internal override - returns (OPStackManager opsm_) + returns (OPStackManager opsmProxy_) { SuperchainConfig superchainConfigProxy = _dii.superchainConfigProxy(); ProtocolVersions protocolVersionsProxy = _dii.protocolVersionsProxy(); + ProxyAdmin proxyAdmin = _dii.superchainProxyAdmin(); - vm.broadcast(msg.sender); - opsm_ = new OPStackManagerInterop({ - _superchainConfig: superchainConfigProxy, - _protocolVersions: protocolVersionsProxy, - _blueprints: blueprints - }); + vm.startBroadcast(msg.sender); + Proxy proxy = new Proxy(address(msg.sender)); + OPStackManager opsm = new OPStackManagerInterop(superchainConfigProxy, protocolVersionsProxy); + + OPStackManager.InitializerInputs memory initializerInputs = + OPStackManager.InitializerInputs(blueprints, setters, release, true); + proxy.upgradeToAndCall(address(opsm), abi.encodeWithSelector(opsm.initialize.selector, initializerInputs)); + + proxy.changeAdmin(address(proxyAdmin)); // transfer ownership of Proxy contract to the ProxyAdmin contract + vm.stopBroadcast(); + + opsmProxy_ = OPStackManagerInterop(address(proxy)); } function deployOptimismPortalImpl( diff --git a/packages/contracts-bedrock/scripts/DeployOPChain.s.sol b/packages/contracts-bedrock/scripts/DeployOPChain.s.sol index 9aac4aefc5c2..fa306edbd3dd 100644 --- a/packages/contracts-bedrock/scripts/DeployOPChain.s.sol +++ b/packages/contracts-bedrock/scripts/DeployOPChain.s.sol @@ -115,6 +115,7 @@ contract DeployOPChainInput { return _l2ChainId; } + // TODO: Check that opsm is proxied and it has an implementation. function opsm() public view returns (OPStackManager) { require(address(_opsm) != address(0), "DeployOPChainInput: not set"); return _opsm; diff --git a/packages/contracts-bedrock/scripts/libraries/DeployUtils.sol b/packages/contracts-bedrock/scripts/libraries/DeployUtils.sol index d690f0b6df6d..d0346ea0d72d 100644 --- a/packages/contracts-bedrock/scripts/libraries/DeployUtils.sol +++ b/packages/contracts-bedrock/scripts/libraries/DeployUtils.sol @@ -1,9 +1,13 @@ // SPDX-License-Identifier: MIT pragma solidity 0.8.15; +import { Proxy } from "src/universal/Proxy.sol"; import { LibString } from "@solady/utils/LibString.sol"; +import { Vm } from "forge-std/Vm.sol"; library DeployUtils { + Vm internal constant vm = Vm(address(uint160(uint256(keccak256("hevm cheat code"))))); + // This takes a sender and an identifier and returns a deterministic address based on the two. // The result is used to etch the input and output contracts to a deterministic address based on // those two values, where the identifier represents the input or output contract, such as @@ -18,6 +22,14 @@ library DeployUtils { require(_who.code.length > 0, string.concat("DeployUtils: no code at ", LibString.toHexStringChecksummed(_who))); } + function assertImplementationSet(address _proxy) internal { + // We prank as the zero address due to the Proxy's `proxyCallIfNotAdmin` modifier. + // Pranking inside this function also means it can no longer be considered `view`. + vm.prank(address(0)); + address implementation = Proxy(payable(_proxy)).implementation(); + assertValidContractAddress(implementation); + } + function assertValidContractAddresses(address[] memory _addrs) internal view { // Assert that all addresses are non-zero and have code. // We use LibString to avoid the need for adding cheatcodes to this contract. diff --git a/packages/contracts-bedrock/semver-lock.json b/packages/contracts-bedrock/semver-lock.json index 966fdf33a6bb..8e5def6450d1 100644 --- a/packages/contracts-bedrock/semver-lock.json +++ b/packages/contracts-bedrock/semver-lock.json @@ -32,8 +32,8 @@ "sourceCodeHash": "0xde4df0f9633dc0cdb1c9f634003ea5b0f7c5c1aebc407bc1b2f44c0ecf938649" }, "src/L1/OPStackManager.sol": { - "initCodeHash": "0xe1eab75651e3d81ad20ca01b1e7d373b25d716ee5f8841a56e56b4531a6e0e70", - "sourceCodeHash": "0x5182a2678dadb200dd255ecdfa395e5f7b1e1e27288e78ddf8802ab51ed2dd81" + "initCodeHash": "0x8081ca5dd48497b74758d1425ad6f025d6fd3cb144b4c5d4335b9a04e78b8474", + "sourceCodeHash": "0xb5fb50a9ddf8c0aee6d0e545f8ef4528f27698f3522cab744cd44ffaef6364d2" }, "src/L1/OptimismPortal.sol": { "initCodeHash": "0xb7a7a28d5b3b88334e7cb4bc1c5fbbf9f691d934e907a2fed6a30e461eb1c0f6", diff --git a/packages/contracts-bedrock/snapshots/abi/OPStackManager.json b/packages/contracts-bedrock/snapshots/abi/OPStackManager.json index 8180e2799c96..00cc5980be0a 100644 --- a/packages/contracts-bedrock/snapshots/abi/OPStackManager.json +++ b/packages/contracts-bedrock/snapshots/abi/OPStackManager.json @@ -10,38 +10,6 @@ "internalType": "contract ProtocolVersions", "name": "_protocolVersions", "type": "address" - }, - { - "components": [ - { - "internalType": "address", - "name": "addressManager", - "type": "address" - }, - { - "internalType": "address", - "name": "proxy", - "type": "address" - }, - { - "internalType": "address", - "name": "proxyAdmin", - "type": "address" - }, - { - "internalType": "address", - "name": "l1ChugSplashProxy", - "type": "address" - }, - { - "internalType": "address", - "name": "resolvedDelegateProxy", - "type": "address" - } - ], - "internalType": "struct OPStackManager.Blueprints", - "name": "_blueprints", - "type": "tuple" } ], "stateMutability": "nonpayable", @@ -271,6 +239,92 @@ "stateMutability": "view", "type": "function" }, + { + "inputs": [ + { + "components": [ + { + "components": [ + { + "internalType": "address", + "name": "addressManager", + "type": "address" + }, + { + "internalType": "address", + "name": "proxy", + "type": "address" + }, + { + "internalType": "address", + "name": "proxyAdmin", + "type": "address" + }, + { + "internalType": "address", + "name": "l1ChugSplashProxy", + "type": "address" + }, + { + "internalType": "address", + "name": "resolvedDelegateProxy", + "type": "address" + } + ], + "internalType": "struct OPStackManager.Blueprints", + "name": "blueprints", + "type": "tuple" + }, + { + "components": [ + { + "internalType": "string", + "name": "name", + "type": "string" + }, + { + "components": [ + { + "internalType": "address", + "name": "logic", + "type": "address" + }, + { + "internalType": "bytes4", + "name": "initializer", + "type": "bytes4" + } + ], + "internalType": "struct OPStackManager.Implementation", + "name": "info", + "type": "tuple" + } + ], + "internalType": "struct OPStackManager.ImplementationSetter[]", + "name": "setters", + "type": "tuple[]" + }, + { + "internalType": "string", + "name": "release", + "type": "string" + }, + { + "internalType": "bool", + "name": "isLatest", + "type": "bool" + } + ], + "internalType": "struct OPStackManager.InitializerInputs", + "name": "_initializerInputs", + "type": "tuple" + } + ], + "name": "initialize", + "outputs": [], + "stateMutability": "nonpayable", + "type": "function" + }, { "inputs": [], "name": "latestRelease", @@ -297,53 +351,6 @@ "stateMutability": "view", "type": "function" }, - { - "inputs": [ - { - "internalType": "string", - "name": "_release", - "type": "string" - }, - { - "internalType": "bool", - "name": "_isLatest", - "type": "bool" - }, - { - "components": [ - { - "internalType": "string", - "name": "name", - "type": "string" - }, - { - "components": [ - { - "internalType": "address", - "name": "logic", - "type": "address" - }, - { - "internalType": "bytes4", - "name": "initializer", - "type": "bytes4" - } - ], - "internalType": "struct OPStackManager.Implementation", - "name": "info", - "type": "tuple" - } - ], - "internalType": "struct OPStackManager.ImplementationSetter[]", - "name": "_setters", - "type": "tuple[]" - } - ], - "name": "setRelease", - "outputs": [], - "stateMutability": "nonpayable", - "type": "function" - }, { "inputs": [], "name": "superchainConfig", @@ -408,6 +415,19 @@ "name": "Deployed", "type": "event" }, + { + "anonymous": false, + "inputs": [ + { + "indexed": false, + "internalType": "uint8", + "name": "version", + "type": "uint8" + } + ], + "name": "Initialized", + "type": "event" + }, { "inputs": [ { @@ -477,6 +497,11 @@ "name": "InvalidRoleAddress", "type": "error" }, + { + "inputs": [], + "name": "LatestReleaseNotSet", + "type": "error" + }, { "inputs": [], "name": "NotABlueprint", diff --git a/packages/contracts-bedrock/snapshots/abi/OPStackManagerInterop.json b/packages/contracts-bedrock/snapshots/abi/OPStackManagerInterop.json index 8180e2799c96..00cc5980be0a 100644 --- a/packages/contracts-bedrock/snapshots/abi/OPStackManagerInterop.json +++ b/packages/contracts-bedrock/snapshots/abi/OPStackManagerInterop.json @@ -10,38 +10,6 @@ "internalType": "contract ProtocolVersions", "name": "_protocolVersions", "type": "address" - }, - { - "components": [ - { - "internalType": "address", - "name": "addressManager", - "type": "address" - }, - { - "internalType": "address", - "name": "proxy", - "type": "address" - }, - { - "internalType": "address", - "name": "proxyAdmin", - "type": "address" - }, - { - "internalType": "address", - "name": "l1ChugSplashProxy", - "type": "address" - }, - { - "internalType": "address", - "name": "resolvedDelegateProxy", - "type": "address" - } - ], - "internalType": "struct OPStackManager.Blueprints", - "name": "_blueprints", - "type": "tuple" } ], "stateMutability": "nonpayable", @@ -271,6 +239,92 @@ "stateMutability": "view", "type": "function" }, + { + "inputs": [ + { + "components": [ + { + "components": [ + { + "internalType": "address", + "name": "addressManager", + "type": "address" + }, + { + "internalType": "address", + "name": "proxy", + "type": "address" + }, + { + "internalType": "address", + "name": "proxyAdmin", + "type": "address" + }, + { + "internalType": "address", + "name": "l1ChugSplashProxy", + "type": "address" + }, + { + "internalType": "address", + "name": "resolvedDelegateProxy", + "type": "address" + } + ], + "internalType": "struct OPStackManager.Blueprints", + "name": "blueprints", + "type": "tuple" + }, + { + "components": [ + { + "internalType": "string", + "name": "name", + "type": "string" + }, + { + "components": [ + { + "internalType": "address", + "name": "logic", + "type": "address" + }, + { + "internalType": "bytes4", + "name": "initializer", + "type": "bytes4" + } + ], + "internalType": "struct OPStackManager.Implementation", + "name": "info", + "type": "tuple" + } + ], + "internalType": "struct OPStackManager.ImplementationSetter[]", + "name": "setters", + "type": "tuple[]" + }, + { + "internalType": "string", + "name": "release", + "type": "string" + }, + { + "internalType": "bool", + "name": "isLatest", + "type": "bool" + } + ], + "internalType": "struct OPStackManager.InitializerInputs", + "name": "_initializerInputs", + "type": "tuple" + } + ], + "name": "initialize", + "outputs": [], + "stateMutability": "nonpayable", + "type": "function" + }, { "inputs": [], "name": "latestRelease", @@ -297,53 +351,6 @@ "stateMutability": "view", "type": "function" }, - { - "inputs": [ - { - "internalType": "string", - "name": "_release", - "type": "string" - }, - { - "internalType": "bool", - "name": "_isLatest", - "type": "bool" - }, - { - "components": [ - { - "internalType": "string", - "name": "name", - "type": "string" - }, - { - "components": [ - { - "internalType": "address", - "name": "logic", - "type": "address" - }, - { - "internalType": "bytes4", - "name": "initializer", - "type": "bytes4" - } - ], - "internalType": "struct OPStackManager.Implementation", - "name": "info", - "type": "tuple" - } - ], - "internalType": "struct OPStackManager.ImplementationSetter[]", - "name": "_setters", - "type": "tuple[]" - } - ], - "name": "setRelease", - "outputs": [], - "stateMutability": "nonpayable", - "type": "function" - }, { "inputs": [], "name": "superchainConfig", @@ -408,6 +415,19 @@ "name": "Deployed", "type": "event" }, + { + "anonymous": false, + "inputs": [ + { + "indexed": false, + "internalType": "uint8", + "name": "version", + "type": "uint8" + } + ], + "name": "Initialized", + "type": "event" + }, { "inputs": [ { @@ -477,6 +497,11 @@ "name": "InvalidRoleAddress", "type": "error" }, + { + "inputs": [], + "name": "LatestReleaseNotSet", + "type": "error" + }, { "inputs": [], "name": "NotABlueprint", diff --git a/packages/contracts-bedrock/snapshots/storageLayout/OPStackManager.json b/packages/contracts-bedrock/snapshots/storageLayout/OPStackManager.json index 3ef385443c3d..e19eff235994 100644 --- a/packages/contracts-bedrock/snapshots/storageLayout/OPStackManager.json +++ b/packages/contracts-bedrock/snapshots/storageLayout/OPStackManager.json @@ -1,30 +1,44 @@ [ + { + "bytes": "1", + "label": "_initialized", + "offset": 0, + "slot": "0", + "type": "uint8" + }, + { + "bytes": "1", + "label": "_initializing", + "offset": 1, + "slot": "0", + "type": "bool" + }, { "bytes": "160", "label": "blueprint", "offset": 0, - "slot": "0", + "slot": "1", "type": "struct OPStackManager.Blueprints" }, { "bytes": "32", "label": "latestRelease", "offset": 0, - "slot": "5", + "slot": "6", "type": "string" }, { "bytes": "32", "label": "implementations", "offset": 0, - "slot": "6", + "slot": "7", "type": "mapping(string => mapping(string => struct OPStackManager.Implementation))" }, { "bytes": "32", "label": "systemConfigs", "offset": 0, - "slot": "7", + "slot": "8", "type": "mapping(uint256 => contract SystemConfig)" } ] \ No newline at end of file diff --git a/packages/contracts-bedrock/snapshots/storageLayout/OPStackManagerInterop.json b/packages/contracts-bedrock/snapshots/storageLayout/OPStackManagerInterop.json index 3ef385443c3d..e19eff235994 100644 --- a/packages/contracts-bedrock/snapshots/storageLayout/OPStackManagerInterop.json +++ b/packages/contracts-bedrock/snapshots/storageLayout/OPStackManagerInterop.json @@ -1,30 +1,44 @@ [ + { + "bytes": "1", + "label": "_initialized", + "offset": 0, + "slot": "0", + "type": "uint8" + }, + { + "bytes": "1", + "label": "_initializing", + "offset": 1, + "slot": "0", + "type": "bool" + }, { "bytes": "160", "label": "blueprint", "offset": 0, - "slot": "0", + "slot": "1", "type": "struct OPStackManager.Blueprints" }, { "bytes": "32", "label": "latestRelease", "offset": 0, - "slot": "5", + "slot": "6", "type": "string" }, { "bytes": "32", "label": "implementations", "offset": 0, - "slot": "6", + "slot": "7", "type": "mapping(string => mapping(string => struct OPStackManager.Implementation))" }, { "bytes": "32", "label": "systemConfigs", "offset": 0, - "slot": "7", + "slot": "8", "type": "mapping(uint256 => contract SystemConfig)" } ] \ No newline at end of file diff --git a/packages/contracts-bedrock/src/L1/OPStackManager.sol b/packages/contracts-bedrock/src/L1/OPStackManager.sol index 5c96cd6aeb8b..29413e6388a8 100644 --- a/packages/contracts-bedrock/src/L1/OPStackManager.sol +++ b/packages/contracts-bedrock/src/L1/OPStackManager.sol @@ -3,9 +3,13 @@ pragma solidity 0.8.15; import { Blueprint } from "src/libraries/Blueprint.sol"; +import { Initializable } from "@openzeppelin/contracts/proxy/utils/Initializable.sol"; + import { ISemver } from "src/universal/interfaces/ISemver.sol"; import { Proxy } from "src/universal/Proxy.sol"; import { ProxyAdmin } from "src/universal/ProxyAdmin.sol"; +import { SuperchainConfig } from "src/L1/SuperchainConfig.sol"; +import { ProtocolVersions } from "src/L1/ProtocolVersions.sol"; import { L1ChugSplashProxy } from "src/legacy/L1ChugSplashProxy.sol"; import { ResolvedDelegateProxy } from "src/legacy/ResolvedDelegateProxy.sol"; @@ -29,7 +33,7 @@ import { L1StandardBridge } from "src/L1/L1StandardBridge.sol"; import { OptimismMintableERC20Factory } from "src/universal/OptimismMintableERC20Factory.sol"; /// @custom:proxied true -contract OPStackManager is ISemver { +contract OPStackManager is ISemver, Initializable { // -------- Structs -------- /// @notice Represents the roles that can be set when deploying a standard OP Stack chain. @@ -97,6 +101,15 @@ contract OPStackManager is ISemver { address resolvedDelegateProxy; } + /// @notice Inputs required when initializing the OPStackManager. To avoid 'StackTooDeep' errors, + /// all necessary inputs (excluding immutables) for initialization are bundled together in this struct. + struct InitializerInputs { + Blueprints blueprints; + ImplementationSetter[] setters; + string release; + bool isLatest; + } + // -------- Constants and Variables -------- /// @custom:semver 1.0.0-beta.3 @@ -110,7 +123,8 @@ contract OPStackManager is ISemver { /// @notice Addresses of the Blueprint contracts. /// This is internal because if public the autogenerated getter method would return a tuple of - /// addresses, but we want it to return a struct. + /// addresses, but we want it to return a struct. This is also set via `initialize` because + /// we can't make this an immutable variable as it is a non-value type. Blueprints internal blueprint; /// @notice The latest release of the OP Stack Manager, as a string of the format `op-contracts/vX.Y.Z`. @@ -149,46 +163,35 @@ contract OPStackManager is ISemver { /// @notice Thrown when a role's address is not valid. error InvalidRoleAddress(string role); + /// @notice Thrown when the latest release is not set upon initialization. + error LatestReleaseNotSet(); + // -------- Methods -------- - /// @notice OPSM is intended to be proxied when used in production. Since we are initially - /// focused on an OPSM version that unblocks interop, we are not proxying OPSM for simplicity. - /// Later, we will `_disableInitializers` in the constructor and replace any constructor logic - /// with an `initialize` function, which will be a breaking change to the OPSM interface. - constructor(SuperchainConfig _superchainConfig, ProtocolVersions _protocolVersions, Blueprints memory _blueprints) { - // TODO uncomment these as we add more validations to this contract, currently this will break a few tests. - // assertValidContractAddress(address(_superchainConfig)); - // assertValidContractAddress(address(_protocolVersions)); - // assertValidContractAddress(_blueprints.addressManager); - // assertValidContractAddress(_blueprints.proxy); - // assertValidContractAddress(_blueprints.proxyAdmin); - // assertValidContractAddress(_blueprints.l1ChugSplashProxy); - // assertValidContractAddress(_blueprints.resolvedDelegateProxy); + /// @notice OPSM is proxied. Therefore the `initialize` function replaces most constructor logic for this contract. + constructor(SuperchainConfig _superchainConfig, ProtocolVersions _protocolVersions) { + assertValidContractAddress(address(_superchainConfig)); + assertValidContractAddress(address(_protocolVersions)); superchainConfig = _superchainConfig; protocolVersions = _protocolVersions; - blueprint = _blueprints; + _disableInitializers(); } - /// @notice Callable by the OPSM owner to release a set of implementation contracts for a given - /// release version. This must be called with `_isLatest` set to true before any chains can be deployed. - /// @param _release The release version to set implementations for, of the format `op-contracts/vX.Y.Z`. - /// @param _isLatest Whether the release version is the latest released version. This is - /// significant because the latest version is used to deploy chains in the `deploy` function. - /// @param _setters The set of implementations to set for the release version. - function setRelease(string memory _release, bool _isLatest, ImplementationSetter[] calldata _setters) external { - // TODO Add auth to this method. + function initialize(InitializerInputs memory _initializerInputs) public initializer { + if (_initializerInputs.isLatest) latestRelease = _initializerInputs.release; + if (keccak256(bytes(latestRelease)) == keccak256("")) revert LatestReleaseNotSet(); - if (_isLatest) latestRelease = _release; - - for (uint256 i = 0; i < _setters.length; i++) { - ImplementationSetter calldata setter = _setters[i]; - Implementation storage impl = implementations[_release][setter.name]; + for (uint256 i = 0; i < _initializerInputs.setters.length; i++) { + ImplementationSetter memory setter = _initializerInputs.setters[i]; + Implementation storage impl = implementations[_initializerInputs.release][setter.name]; if (impl.logic != address(0)) revert AlreadyReleased(); impl.initializer = setter.info.initializer; impl.logic = setter.info.logic; } + + blueprint = _initializerInputs.blueprints; } function deploy(DeployInput calldata _input) external returns (DeployOutput memory) { diff --git a/packages/contracts-bedrock/src/L1/OPStackManagerInterop.sol b/packages/contracts-bedrock/src/L1/OPStackManagerInterop.sol index e9fa44c90f9b..53d33fc9522d 100644 --- a/packages/contracts-bedrock/src/L1/OPStackManagerInterop.sol +++ b/packages/contracts-bedrock/src/L1/OPStackManagerInterop.sol @@ -8,14 +8,13 @@ import { ResourceMetering } from "src/L1/ResourceMetering.sol"; import { SystemConfig } from "src/L1/SystemConfig.sol"; import { SystemConfigInterop } from "src/L1/SystemConfigInterop.sol"; -/// @custom:proxied TODO this is not proxied yet. +/// @custom:proxied true contract OPStackManagerInterop is OPStackManager { constructor( SuperchainConfig _superchainConfig, - ProtocolVersions _protocolVersions, - Blueprints memory _blueprints + ProtocolVersions _protocolVersions ) - OPStackManager(_superchainConfig, _protocolVersions, _blueprints) + OPStackManager(_superchainConfig, _protocolVersions) { } // The `SystemConfigInterop` contract has an extra `address _dependencyManager` argument diff --git a/packages/contracts-bedrock/test/DeployImplementations.t.sol b/packages/contracts-bedrock/test/DeployImplementations.t.sol index 4b00813836ee..5a713999495b 100644 --- a/packages/contracts-bedrock/test/DeployImplementations.t.sol +++ b/packages/contracts-bedrock/test/DeployImplementations.t.sol @@ -17,6 +17,8 @@ import { L1CrossDomainMessenger } from "src/L1/L1CrossDomainMessenger.sol"; import { L1ERC721Bridge } from "src/L1/L1ERC721Bridge.sol"; import { L1StandardBridge } from "src/L1/L1StandardBridge.sol"; import { OptimismMintableERC20Factory } from "src/universal/OptimismMintableERC20Factory.sol"; +import { ProxyAdmin } from "src/universal/ProxyAdmin.sol"; +import { Proxy } from "src/universal/Proxy.sol"; import { DeployImplementationsInput, @@ -89,7 +91,12 @@ contract DeployImplementationsOutput_Test is Test { } function test_set_succeeds() public { - OPStackManager opsm = OPStackManager(makeAddr("opsm")); + Proxy opsmProxy = new Proxy(address(0)); + address opsmImpl = address(makeAddr("opsm")); + vm.prank(address(0)); + opsmProxy.upgradeTo(opsmImpl); + + OPStackManager opsm = OPStackManager(address(opsmProxy)); OptimismPortal2 optimismPortalImpl = OptimismPortal2(payable(makeAddr("optimismPortalImpl"))); DelayedWETH delayedWETHImpl = DelayedWETH(payable(makeAddr("delayedWETHImpl"))); PreimageOracle preimageOracleSingleton = PreimageOracle(makeAddr("preimageOracleSingleton")); @@ -103,7 +110,8 @@ contract DeployImplementationsOutput_Test is Test { OptimismMintableERC20Factory(makeAddr("optimismMintableERC20FactoryImpl")); DisputeGameFactory disputeGameFactoryImpl = DisputeGameFactory(makeAddr("disputeGameFactoryImpl")); - vm.etch(address(opsm), hex"01"); + vm.etch(address(opsmProxy), address(opsmProxy).code); + vm.etch(address(opsmImpl), hex"01"); vm.etch(address(optimismPortalImpl), hex"01"); vm.etch(address(delayedWETHImpl), hex"01"); vm.etch(address(preimageOracleSingleton), hex"01"); @@ -255,9 +263,20 @@ contract DeployImplementations_Test is Test { proofMaturityDelaySeconds = uint256(hash(_seed, 3)); disputeGameFinalityDelaySeconds = uint256(hash(_seed, 4)); release = string(bytes.concat(hash(_seed, 5))); - superchainConfigProxy = SuperchainConfig(address(uint160(uint256(hash(_seed, 6))))); protocolVersionsProxy = ProtocolVersions(address(uint160(uint256(hash(_seed, 7))))); + // Must configure the ProxyAdmin contract which is used to upgrade the OPSM's proxy contract. + ProxyAdmin superchainProxyAdmin = new ProxyAdmin(msg.sender); + superchainConfigProxy = SuperchainConfig(address(new Proxy(payable(address(superchainProxyAdmin))))); + + SuperchainConfig superchainConfigImpl = SuperchainConfig(address(uint160(uint256(hash(_seed, 6))))); + vm.prank(address(superchainProxyAdmin)); + Proxy(payable(address(superchainConfigProxy))).upgradeTo(address(superchainConfigImpl)); + + vm.etch(address(superchainProxyAdmin), address(superchainProxyAdmin).code); + vm.etch(address(superchainConfigProxy), address(superchainConfigProxy).code); + vm.etch(address(protocolVersionsProxy), hex"01"); + dii.set(dii.withdrawalDelaySeconds.selector, withdrawalDelaySeconds); dii.set(dii.minProposalSizeBytes.selector, minProposalSizeBytes); dii.set(dii.challengePeriodSeconds.selector, challengePeriodSeconds); diff --git a/packages/contracts-bedrock/test/L1/OPStackManager.t.sol b/packages/contracts-bedrock/test/L1/OPStackManager.t.sol index 4d5929021b5d..289911b06c05 100644 --- a/packages/contracts-bedrock/test/L1/OPStackManager.t.sol +++ b/packages/contracts-bedrock/test/L1/OPStackManager.t.sol @@ -14,10 +14,9 @@ import { ProtocolVersions } from "src/L1/ProtocolVersions.sol"; contract OPStackManager_Harness is OPStackManager { constructor( SuperchainConfig _superchainConfig, - ProtocolVersions _protocolVersions, - Blueprints memory _blueprints + ProtocolVersions _protocolVersions ) - OPStackManager(_superchainConfig, _protocolVersions, _blueprints) + OPStackManager(_superchainConfig, _protocolVersions) { } function chainIdToBatchInboxAddress_exposed(uint256 l2ChainId) public pure returns (address) { @@ -92,16 +91,14 @@ contract OPStackManager_InternalMethods_Test is Test { OPStackManager_Harness opsmHarness; function setUp() public { + SuperchainConfig superchainConfigProxy = SuperchainConfig(makeAddr("superchainConfig")); + ProtocolVersions protocolVersionsProxy = ProtocolVersions(makeAddr("protocolVersions")); + vm.etch(address(superchainConfigProxy), hex"01"); + vm.etch(address(protocolVersionsProxy), hex"01"); + opsmHarness = new OPStackManager_Harness({ - _superchainConfig: SuperchainConfig(makeAddr("superchainConfig")), - _protocolVersions: ProtocolVersions(makeAddr("protocolVersions")), - _blueprints: OPStackManager.Blueprints({ - addressManager: makeAddr("addressManager"), - proxy: makeAddr("proxy"), - proxyAdmin: makeAddr("proxyAdmin"), - l1ChugSplashProxy: makeAddr("l1ChugSplashProxy"), - resolvedDelegateProxy: makeAddr("resolvedDelegateProxy") - }) + _superchainConfig: superchainConfigProxy, + _protocolVersions: protocolVersionsProxy }); } diff --git a/packages/contracts-bedrock/test/Specs.t.sol b/packages/contracts-bedrock/test/Specs.t.sol index 15e20f4d01f1..e9217d655074 100644 --- a/packages/contracts-bedrock/test/Specs.t.sol +++ b/packages/contracts-bedrock/test/Specs.t.sol @@ -843,7 +843,7 @@ contract Specification_Test is CommonTest { _addSpec({ _name: "OPStackManager", _sel: _getSel("latestRelease()") }); _addSpec({ _name: "OPStackManager", _sel: _getSel("implementations(string,string)") }); _addSpec({ _name: "OPStackManager", _sel: _getSel("systemConfigs(uint256)") }); - _addSpec({ _name: "OPStackManager", _sel: OPStackManager.setRelease.selector }); + _addSpec({ _name: "OPStackManager", _sel: OPStackManager.initialize.selector }); _addSpec({ _name: "OPStackManager", _sel: OPStackManager.deploy.selector }); _addSpec({ _name: "OPStackManager", _sel: OPStackManager.blueprints.selector }); @@ -854,7 +854,7 @@ contract Specification_Test is CommonTest { _addSpec({ _name: "OPStackManagerInterop", _sel: _getSel("latestRelease()") }); _addSpec({ _name: "OPStackManagerInterop", _sel: _getSel("implementations(string,string)") }); _addSpec({ _name: "OPStackManagerInterop", _sel: _getSel("systemConfigs(uint256)") }); - _addSpec({ _name: "OPStackManagerInterop", _sel: OPStackManager.setRelease.selector }); + _addSpec({ _name: "OPStackManagerInterop", _sel: OPStackManager.initialize.selector }); _addSpec({ _name: "OPStackManagerInterop", _sel: OPStackManager.deploy.selector }); _addSpec({ _name: "OPStackManagerInterop", _sel: OPStackManager.blueprints.selector }); diff --git a/packages/contracts-bedrock/test/vendor/Initializable.t.sol b/packages/contracts-bedrock/test/vendor/Initializable.t.sol index df9231a27627..f6ef25608e34 100644 --- a/packages/contracts-bedrock/test/vendor/Initializable.t.sol +++ b/packages/contracts-bedrock/test/vendor/Initializable.t.sol @@ -400,7 +400,7 @@ contract Initializer_Test is Bridge_Initializer { /// 3. The `initialize()` function of each contract cannot be called again. function test_cannotReinitialize_succeeds() public { // Collect exclusions. - string[] memory excludes = new string[](6); + string[] memory excludes = new string[](8); // TODO: Neither of these contracts are labeled properly in the deployment script. Both are // currently being labeled as their non-interop versions. Remove these exclusions once // the deployment script is fixed. @@ -416,6 +416,9 @@ contract Initializer_Test is Bridge_Initializer { // don't work properly. Remove these exclusions once the deployment script is fixed. excludes[4] = "src/dispute/FaultDisputeGame.sol"; excludes[5] = "src/dispute/PermissionedDisputeGame.sol"; + // TODO: Eventually remove this exclusion. Same reason as above dispute contracts. + excludes[6] = "src/L1/OPStackManager.sol"; + excludes[7] = "src/L1/OPStackManagerInterop.sol"; // Get all contract names in the src directory, minus the excluded contracts. string[] memory contractNames = ForgeArtifacts.getContractNames("src/*", excludes);