Upgrade test path #172
Upgrade test path #172
Conversation
| - Calls `op-deployer bootstrap superchain` to deploy new superchain contracts (`SuperchainConfig` | ||
| and `ProtocolVersions`), corresponding to the previous release. |
There was a problem hiding this comment.
corresponding to the previous release.
@mslipper This might be a gotcha I've miseed. ie. since op-deployer now only deploys a single release version, we'd need to use the previous release of op-deployer, not the one that exists on develop. Can we easily install and run a previous version?
There was a problem hiding this comment.
As long as a tagged release exists that can deploy all the prerequisite contracts, then yes.
| 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.
Is executing this command analogous to running both DeploySuperchain.s.sol and DeployImplementations.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.25
There was a problem hiding this comment.
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.
Ah, and op-deployer bootstrap opcm deploys the new implementations before deploying OPCM?
There was a problem hiding this comment.
Yes, afaict that is what it does, as it creates this output:
{
"Opcm": "0xe3ef345391654121f385679613cea79a692c2dd8",
"DelayedWETHImpl": "0x71e966ae981d1ce531a7b6d23dc0f27b38409087",
"OptimismPortalImpl": "0xe2f826324b2faf99e513d16d266c3f80ae87832b",
"PreimageOracleSingleton": "0x9c065e11870b891d214bc2da7ef1f9ddfa1be277",
"MipsSingleton": "0x16e83ce5ce29bf90ad9da06d2fe6a15d5f344ce4",
"SystemConfigImpl": "0xf56d96b2535b932656d3c04ebf51babff241d886",
"L1CrossDomainMessengerImpl": "0xd3494713a5cfad3f5359379dfa074e2ac8c6fd65",
"L1ERC721BridgeImpl": "0xae2af01232a6c4a4d3012c5ec5b1b35059caf10d",
"L1StandardBridgeImpl": "0x64b5a5ed26dcb17370ff4d33a8d503f0fbd06cff",
"OptimismMintableERC20FactoryImpl": "0xe01efbeb1089d1d1db9c6c8b135c934c0734c846",
"DisputeGameFactoryImpl": "0xc641a33cab81c559f2bd4b21ea34c290e2440c2b"
}
| 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.
Does this introduce a circular dependency issue at all?
There was a problem hiding this comment.
Can you say more about what you think that might be?
There was a problem hiding this comment.
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.
hmmm, I think that this is OK because op-deployer is only being used to setup a system from a previous release, and then the testing will act directly on the OPCM and updated implementations which are on develop
| - **`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. |
There was a problem hiding this comment.
Should this be merged with op-deployer bootstrap superchain? It seems like any time we want to deploy new superchain implementation contracts we'd also want to deploy a corresponding new OPCM
There was a problem hiding this comment.
I don't have a strong opinion here, maybe @mslipper does?
For the purposes of this design, I'm happy to make two op-deployer bootstrap calls or just the one if we do combine then.
There was a problem hiding this comment.
No strong opinion either, happy to defer
| 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. |
There was a problem hiding this comment.
Am I correct that the flow here would be for our forge tests to invoke op-deployer against an anvil RPC URL, then after op-deployer runs it would vm.createSelectFork(anvilRpcUrl)?
There was a problem hiding this comment.
invoke
op-deployeragainst an anvil RPC URL
Yes.
then after op-deployer runs it would
vm.createSelectFork(anvilRpcUrl)?
I had envisioned calling forge test --fork-url anvilRpcUrl. Creating the fork in the script seems less error prone so I like this, are there any other differences I should be aware of between those two approaches?
There was a problem hiding this comment.
I had envisioned calling
forge test --fork-url anvilRpcUrl. Creating the fork in the script seems less error prone so I like this
I am not certain but it's possible the forge test --fork-url anvilRpcUrl approach might not work because forge may always make requests at whatever the initial block is at fork time, i.e. I don't know if it follows the chain live, if that makes sense. So starting our fork from within the test/script itself after anvil is properly configured seems safer anyway
I don't think there should be any other differences. There are some createSelectFork things to be aware of if you switch between forks in the tests, like vm.makePersistent
No description provided.