-
Notifications
You must be signed in to change notification settings - Fork 22
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
Upgrade test path #172
base: main
Are you sure you want to change the base?
Upgrade test path #172
Changes from 5 commits
820ed62
0ffb1eb
3eb89d4
557a015
6730d4e
a508d8c
9169188
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,121 @@ | ||
# 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 | ||
|
||
<!-- Most (if not all) documents should have a summary. | ||
While the length will likely be proportional to the length of the full document, | ||
the summary should be as succinct as possible. --> | ||
|
||
# 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 | ||
|
||
## Deploying Superchain contracts with `op-deployer` | ||
|
||
We propose to extend `op-deployer bootstrap` to enable deploying superchain contracts. | ||
|
||
```shell | ||
op-deployer bootstrap superchain <artifacts-locator> --outdir <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()"] | ||
C --> D["DeployImplementations.run()"] | ||
C --> E["DeploySuperchain.run()"] | ||
C --> F["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 `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 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()"] | ||
C --> D["DeployImplementations.run()"] | ||
C --> E["OPCM.upgrade()"] | ||
``` | ||
|
||
A description of what new components would do is: | ||
|
||
- **`Upgrade.run()`:** | ||
- Calls `op-deployer bootstrap superchain` to deploy new superchain contracts (`SuperchainConfig` | ||
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 introduce a circular dependency issue at all? 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 say more about what you think that might be? 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. Originally OPCM and it's solidity tests were self-contained, and op-deployer wrapped it. Now, OPCM requires op-deployer for it's own testing. I am not familiar enough with the architecture to know if there is actually a problem here, and I don't have a better solution that feels less circular 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. hmmm, I think that this is OK because |
||
and `ProtocolVersions`), corresponding to the previous release. | ||
Comment on lines
+88
to
+89
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.
@mslipper This might be a gotcha I've miseed. ie. since 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 long as a tagged release exists that can deploy all the prerequisite contracts, then yes. |
||
- Calls `op-deployer bootstrap opcm` to deploy release OPCM, corresponding to the previous release. | ||
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. Should this be merged with 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 don't have a strong opinion here, maybe @mslipper does? For the purposes of this design, I'm happy to make two 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. No strong opinion either, happy to defer |
||
- 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). | ||
|
||
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. | ||
|
||
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. | ||
Comment on lines
+102
to
+104
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. Am I correct that the flow here would be for our forge tests to invoke 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.
Yes.
I had envisioned calling 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 am not certain but it's possible the I don't think there should be any other differences. There are some |
||
|
||
# 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 | ||
|
||
<!-- An overview of what could go wrong. | ||
Also any open questions that need more work to resolve. --> |
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.
Is executing this command analogous to running both
DeploySuperchain.s.sol
andDeployImplementations.s.sol
, but only having those scripts deploy contracts that changed from the last release? Don't need to get into implementation here, but one idea I've had for doing this is have those scripts use create2, precompute deploy addresses, and skip when code already exists. This requires a one-time redeploy of all contracts using this method, which we have to do for isthmus anyway since we're bumping to 0.8.25There 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.
In my mind this command would only do what
DeploySuperchain.s.sol
currently does (but for a given release).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.
Ah, and
op-deployer bootstrap opcm
deploys the new implementations before deploying OPCM?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.
Yes, afaict that is what it does, as it creates this output: