From 820ed629879d4adb91fcc83e7e9a709a7063fb8b Mon Sep 17 00:00:00 2001 From: Maurelian Date: Wed, 27 Nov 2024 16:05:59 -0500 Subject: [PATCH 1/7] Add solidity based upgrade testing --- protocol/l1-upgrades.md | 2 +- solidity/20241126-upgrade-path-testing.md | 100 ++++++++++++++++++++++ 2 files changed, 101 insertions(+), 1 deletion(-) create mode 100644 solidity/20241126-upgrade-path-testing.md diff --git a/protocol/l1-upgrades.md b/protocol/l1-upgrades.md index c988d40b..c1a3cab2 100644 --- a/protocol/l1-upgrades.md +++ b/protocol/l1-upgrades.md @@ -224,7 +224,7 @@ This method will: In order to avoid blocking this work, if the `FaultDisputeGame` and `PermissionedDisputeGame` have not become MCP-L1 compatible in time, the OPCM upgrade path will deploy chain specific instances. -## Release process +## Upgrade process As part of the release process, the associated implementation contracts and a new OPCM, will be deployed. diff --git a/solidity/20241126-upgrade-path-testing.md b/solidity/20241126-upgrade-path-testing.md new file mode 100644 index 00000000..a0767bd8 --- /dev/null +++ b/solidity/20241126-upgrade-path-testing.md @@ -0,0 +1,100 @@ +# Purpose + +In order to safely implement the [L1 Upgrades design](../protocol/l1-upgrades.md) +the upgrade path should be tested, meaning that we should be able to: + +1. start with a system matching the previous release +2. upgrade that system (using `OPCM.upgrade()`) +3. run tests against the upgraded system. + +Put another way, we need two different methods for setting up the foundry tests which are +created by inheriting from `CommonTest`. + +# Summary + + + +# Problem Statement + Context + +Our test suite is currently not able to replicate the system from a previous release, which +prevents us from testing the upgrade path to the system currently under development. + +As we're moving towards onchain upgrades via the OPCM, we want to be able to test that: + +1. The upgrade works to move from the previous release to the system on `develop`. +2. The upgraded system passes the same set of unit test as the freshly deployed system on `develop`. + +# Proposed Solution + +## Getting artifacts with `op-deployer` + +We propose to extend `op-deployer`, which already includes functionality for downloading a set of artifacts, to expose that functionality, with a command like: + +``` +op-deployer get-artifacts --outdir +``` + +This command will write the artifacts to the outdir. + +## Setting up the system to test + +The current foundry testing uses the following high level call flow. + +```mermaid +graph LR + A["CommonTest.setUp()"] --> B["Setup.L1()"] --> C["Deploy.run()"] --> D["OPCM.deploy()"] +``` + +This is roughly what each component does: + +- **`CommonTest.setUp()`:** + - Defines system config settings (ie. `useInterop`). + - provides reusable addresses (`alice` and `bob`) + - provides reusable generic contracts (`ERC20`) +- **`Setup.L1()`:** + - Reads addresses from the deployments file stored on disk + - labels addresses to make traces more readable + - ie. + ```solidity + optimismPortal = IOptimismPortal(deploy.mustGetAddress("OptimismPortalProxy")); + vm.label(address(optimismPortal), "OptimismPortal"); + ``` +- **`Deploy.run()`:** + - Deploys all necessary contracts via calls to the `DeploySuperchain`, `DeployImplementations`, + and `OPContractsManager` + - Makes modifications to the resulting system for non-standard configurations. +- **`OPCM.deploy()`:** + - Deploys all proxies and bespoke singleton contracts as necessary for a new OP Chain. + +This work would provide an alternative testing route for the standard configuration: + +```mermaid +graph LR + A["CommonTest.setUp()"] --> B["Setup.L1()"] --> C["Upgrade.run()"] --> D["OPCM.upgrade()"] +``` + +This testing setup route would be indicated with a new `useUpgradedSystem` flag in `CommonTest`. The +new flag could only be enabled when other flags (`useAltDAOverride`, `useLegacyContracts`, +`useInteropOverride`, `customGasToken`) are disabled. + +A description of what new components would do is: + +- **`Upgrade.run()`:** + - Calls `op-deployer get-artifacts` to download the artifacts for the previous release. + - Reads bytecode from the artifacts + - Deploys all necesssary implementations and singleton contracts using that bytecode. + - Deploys contracts necessary for upgrading the the system on `develop` using `DeployImplementations`. +- **`OPCM.upgrade()`:** + - Upgrades proxies to new implementation contracts and bespoke singleton contracts as necessary for a new OP Chain. + +# Alternatives Considered + + + +# Risks & Uncertainties + + From 0ffb1eb57b011db893097f61ee98028031db7d45 Mon Sep 17 00:00:00 2001 From: Maurelian Date: Wed, 27 Nov 2024 16:20:17 -0500 Subject: [PATCH 2/7] Update to op-deployer bootstrap driven approach --- solidity/20241126-upgrade-path-testing.md | 38 ++++++++++++++--------- 1 file changed, 24 insertions(+), 14 deletions(-) diff --git a/solidity/20241126-upgrade-path-testing.md b/solidity/20241126-upgrade-path-testing.md index a0767bd8..744165d3 100644 --- a/solidity/20241126-upgrade-path-testing.md +++ b/solidity/20241126-upgrade-path-testing.md @@ -44,7 +44,10 @@ The current foundry testing uses the following high level call flow. ```mermaid graph LR - A["CommonTest.setUp()"] --> B["Setup.L1()"] --> C["Deploy.run()"] --> D["OPCM.deploy()"] + A["CommonTest.setUp()"] --> B["Setup.L1()"] --> C["Deploy.run()"] + C --> D["DeployImplementations.run()"] + C --> E["DeploySuperchain.run()"] + C --> F["OPCM.deploy()"] ``` This is roughly what each component does: @@ -62,32 +65,39 @@ This is roughly what each component does: vm.label(address(optimismPortal), "OptimismPortal"); ``` - **`Deploy.run()`:** - - Deploys all necessary contracts via calls to the `DeploySuperchain`, `DeployImplementations`, - and `OPContractsManager` - - Makes modifications to the resulting system for non-standard configurations. + - Deploys all necessary contracts via calls to `DeploySuperchain`, `DeployImplementations`. +- **`DeploySuperchain.run()`:** + - Deploys superchain contracts (`SuperchainConfig` and `ProtocolVersions`). +- **`DeployImplementations.run()`:** + - Deploys contracts (implementations and singletons) necessary for upgrading to the system on + `develop`. - **`OPCM.deploy()`:** - Deploys all proxies and bespoke singleton contracts as necessary for a new OP Chain. -This work would provide an alternative testing route for the standard configuration: +This work would replace the `Deploy` script with a new `Upgrade` script, resulting in the following +call flow: ```mermaid graph LR - A["CommonTest.setUp()"] --> B["Setup.L1()"] --> C["Upgrade.run()"] --> D["OPCM.upgrade()"] + A["CommonTest.setUp()"] --> B["Setup.L1()"] --> C["Upgrade.run()"] + C --> D["DeployImplementations.run()"] + C --> E["OPCM.upgrade()"] ``` -This testing setup route would be indicated with a new `useUpgradedSystem` flag in `CommonTest`. The -new flag could only be enabled when other flags (`useAltDAOverride`, `useLegacyContracts`, -`useInteropOverride`, `customGasToken`) are disabled. - A description of what new components would do is: - **`Upgrade.run()`:** - - Calls `op-deployer get-artifacts` to download the artifacts for the previous release. - - Reads bytecode from the artifacts - - Deploys all necesssary implementations and singleton contracts using that bytecode. - - Deploys contracts necessary for upgrading the the system on `develop` using `DeployImplementations`. + - Calls `op-deployer bootstrap superchain` to deploy new superchain contracts (`SuperchainConfig` + and `ProtocolVersions`), corresponding to the previous release. + - Calls `op-deployer bootstrap opcm` to deploy release OPCM, corresponding to the previous release. + - Calls to `DeployImplementations.run()` to deploy contracts necessary for upgrading to the system on `develop`. - **`OPCM.upgrade()`:** - Upgrades proxies to new implementation contracts and bespoke singleton contracts as necessary for a new OP Chain. + - This flow is descibed in detail in the [L1 Upgrades design](../protocol/l1-upgrades.md#release-process). + +This testing setup route would be indicated with a new `useUpgradedSystem` flag in `CommonTest`. The +new flag could only be enabled when other flags (`useAltDAOverride`, `useLegacyContracts`, +`useInteropOverride`, `customGasToken`) are disabled. # Alternatives Considered From 3eb89d4846744452fd5914d6ff2f6e055e82f109 Mon Sep 17 00:00:00 2001 From: Maurelian Date: Wed, 27 Nov 2024 16:26:42 -0500 Subject: [PATCH 3/7] Revert back to get-artifacts --- solidity/20241126-upgrade-path-testing.md | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/solidity/20241126-upgrade-path-testing.md b/solidity/20241126-upgrade-path-testing.md index 744165d3..32d669c8 100644 --- a/solidity/20241126-upgrade-path-testing.md +++ b/solidity/20241126-upgrade-path-testing.md @@ -87,10 +87,10 @@ graph LR A description of what new components would do is: - **`Upgrade.run()`:** - - Calls `op-deployer bootstrap superchain` to deploy new superchain contracts (`SuperchainConfig` - and `ProtocolVersions`), corresponding to the previous release. - - Calls `op-deployer bootstrap opcm` to deploy release OPCM, corresponding to the previous release. - - Calls to `DeployImplementations.run()` to deploy contracts necessary for upgrading to the system on `develop`. + - Calls `op-deployer get-artifacts` to download the artifacts for the previous release. + - Reads bytecode from the artifacts + - Deploys all necesssary implementations and singleton contracts using that bytecode. + - Deploys contracts necessary for upgrading the the system on `develop` using `DeployImplementations`. - **`OPCM.upgrade()`:** - Upgrades proxies to new implementation contracts and bespoke singleton contracts as necessary for a new OP Chain. - This flow is descibed in detail in the [L1 Upgrades design](../protocol/l1-upgrades.md#release-process). From 557a015c59a99aeb50e1ad5ba776aee17245f22e Mon Sep 17 00:00:00 2001 From: Maurelian Date: Wed, 27 Nov 2024 16:26:51 -0500 Subject: [PATCH 4/7] Revert "Revert back to get-artifacts" This reverts commit 3eb89d4846744452fd5914d6ff2f6e055e82f109. --- solidity/20241126-upgrade-path-testing.md | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/solidity/20241126-upgrade-path-testing.md b/solidity/20241126-upgrade-path-testing.md index 32d669c8..744165d3 100644 --- a/solidity/20241126-upgrade-path-testing.md +++ b/solidity/20241126-upgrade-path-testing.md @@ -87,10 +87,10 @@ graph LR A description of what new components would do is: - **`Upgrade.run()`:** - - Calls `op-deployer get-artifacts` to download the artifacts for the previous release. - - Reads bytecode from the artifacts - - Deploys all necesssary implementations and singleton contracts using that bytecode. - - Deploys contracts necessary for upgrading the the system on `develop` using `DeployImplementations`. + - Calls `op-deployer bootstrap superchain` to deploy new superchain contracts (`SuperchainConfig` + and `ProtocolVersions`), corresponding to the previous release. + - Calls `op-deployer bootstrap opcm` to deploy release OPCM, corresponding to the previous release. + - Calls to `DeployImplementations.run()` to deploy contracts necessary for upgrading to the system on `develop`. - **`OPCM.upgrade()`:** - Upgrades proxies to new implementation contracts and bespoke singleton contracts as necessary for a new OP Chain. - This flow is descibed in detail in the [L1 Upgrades design](../protocol/l1-upgrades.md#release-process). From 6730d4e1608c8f24cdf06a5578741d7e4a0403f9 Mon Sep 17 00:00:00 2001 From: Maurelian Date: Wed, 27 Nov 2024 16:43:28 -0500 Subject: [PATCH 5/7] Add alternatives --- solidity/20241126-upgrade-path-testing.md | 23 +++++++++++++++++------ 1 file changed, 17 insertions(+), 6 deletions(-) diff --git a/solidity/20241126-upgrade-path-testing.md b/solidity/20241126-upgrade-path-testing.md index 744165d3..75f087fc 100644 --- a/solidity/20241126-upgrade-path-testing.md +++ b/solidity/20241126-upgrade-path-testing.md @@ -28,12 +28,12 @@ As we're moving towards onchain upgrades via the OPCM, we want to be able to tes # Proposed Solution -## Getting artifacts with `op-deployer` +## Deploying Superchain contracts with `op-deployer` -We propose to extend `op-deployer`, which already includes functionality for downloading a set of artifacts, to expose that functionality, with a command like: +We propose to extend `op-deployer bootstrap` to enable deploying superchain contracts. -``` -op-deployer get-artifacts --outdir +```shell +op-deployer bootstrap superchain --outdir ``` This command will write the artifacts to the outdir. @@ -91,6 +91,8 @@ A description of what new components would do is: and `ProtocolVersions`), corresponding to the previous release. - Calls `op-deployer bootstrap opcm` to deploy release OPCM, corresponding to the previous release. - Calls to `DeployImplementations.run()` to deploy contracts necessary for upgrading to the system on `develop`. + - Parses the deployment output from `op-deployer` and writes to disk using the `Deploy.save()` + functions, so that `Setup.L1()` can read the deployment. - **`OPCM.upgrade()`:** - Upgrades proxies to new implementation contracts and bespoke singleton contracts as necessary for a new OP Chain. - This flow is descibed in detail in the [L1 Upgrades design](../protocol/l1-upgrades.md#release-process). @@ -99,10 +101,19 @@ This testing setup route would be indicated with a new `useUpgradedSystem` flag new flag could only be enabled when other flags (`useAltDAOverride`, `useLegacyContracts`, `useInteropOverride`, `customGasToken`) are disabled. +Note that this testing would need to be run against an `anvil` node, as `op-deployer bootstrap` +requires an RPC endpoint. I am not experienced with running `forge test` against an anvil node, +so appreciate any gotchas I might be missing. + # Alternatives Considered - +An alternative considered was to add a new `op-deployer download` command to get artifacts, then +make minimal modifications to `DeploySuperchain` and `DeployImplementations` to deploy those +artifacts by providing branching logic to provide a different artifacts path to `vm.getCode()`. + +The challenge with this approach is that `DeployImplementations` will need other changes from +release to release, so we would be in a position of needing branching logic to accomodate +at least the most recent release and current release in that script. # Risks & Uncertainties From a508d8ccdba51c08858a15e2739dcba0c2b523d1 Mon Sep 17 00:00:00 2001 From: Maurelian Date: Thu, 28 Nov 2024 15:53:33 -0500 Subject: [PATCH 6/7] Fix bootstrap invocation --- solidity/20241126-upgrade-path-testing.md | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/solidity/20241126-upgrade-path-testing.md b/solidity/20241126-upgrade-path-testing.md index 75f087fc..3e3ecc35 100644 --- a/solidity/20241126-upgrade-path-testing.md +++ b/solidity/20241126-upgrade-path-testing.md @@ -33,11 +33,9 @@ As we're moving towards onchain upgrades via the OPCM, we want to be able to tes We propose to extend `op-deployer bootstrap` to enable deploying superchain contracts. ```shell -op-deployer bootstrap superchain --outdir +op-deployer bootstrap superchain ``` -This command will write the artifacts to the outdir. - ## Setting up the system to test The current foundry testing uses the following high level call flow. From 916918850aae3785ca543be2c5895df88db8d161 Mon Sep 17 00:00:00 2001 From: Maurelian Date: Mon, 2 Dec 2024 10:37:56 -0500 Subject: [PATCH 7/7] Add mainnet fork testing as alternative considered. --- solidity/20241126-upgrade-path-testing.md | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/solidity/20241126-upgrade-path-testing.md b/solidity/20241126-upgrade-path-testing.md index 3e3ecc35..6481422e 100644 --- a/solidity/20241126-upgrade-path-testing.md +++ b/solidity/20241126-upgrade-path-testing.md @@ -108,11 +108,19 @@ so appreciate any gotchas I might be missing. An alternative considered was to add a new `op-deployer download` command to get artifacts, then make minimal modifications to `DeploySuperchain` and `DeployImplementations` to deploy those artifacts by providing branching logic to provide a different artifacts path to `vm.getCode()`. - The challenge with this approach is that `DeployImplementations` will need other changes from release to release, so we would be in a position of needing branching logic to accomodate at least the most recent release and current release in that script. +Another alternative would be to always test against a fork of mainnet. This was +rejected as it would prevent development from occuring on features which are +further out. For example, while the current production version is `2.0.0`, an RC +candidate OPCM (`2.1.0-rc.1`) would be deployed and submitted to governance for +approval. Developer would then wish to begin developing the upgrade from +`2.1.0` to `2.2.0` but the upgrade has not yet been activated, causing fork +based tests to pass. Enabling upgrade tests to occur against an arbitrary tag +is seen as more flexible and robust. + # Risks & Uncertainties