-
Notifications
You must be signed in to change notification settings - Fork 3.7k
feat: Simplify op-deployer scripts: Switch from legacy to the new scripts for DeploySuperchain & DeployImplementations [17/N] #15551
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
Conversation
5ae48b0
to
b333ffe
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## develop #15551 +/- ##
============================================
+ Coverage 47.06% 96.24% +49.17%
============================================
Files 1360 106 -1254
Lines 109447 4575 -104872
============================================
- Hits 51512 4403 -47109
+ Misses 54279 172 -54107
+ Partials 3656 0 -3656
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
252a54f
to
e5604f9
Compare
e5604f9
to
6335f6c
Compare
6335f6c
to
15d6f8d
Compare
15d6f8d
to
535a007
Compare
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.
Reviewed by manually comparing the new Deploy*
scripts with their 2
counterparts on develop
.
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.
Will re-approve once compilation errors are fixed.
…s & contract names of the new scripts
…filenames & types of the new wrappers
…uct. This step might not be necessary anymore
…ployment per CLI target so the Scripts struct needs to be created for every CLI target
…ract artifacts, after which the tests need to be updated and regression tests unskipped
aff9a86
to
adb450d
Compare
…ipts for DeploySuperchain & DeployImplementations [17/N] (ethereum-optimism#15551) * step 1: Delete old scripts & tests; Remove the "2" from the file names & contract names of the new scripts * step 2: Update the downstream scripts * step 3: Delete the old opcm script wrappers; Remove the "2" from the filenames & types of the new wrappers * step 4.1: Create OPCM scripts struct in pipeline and ad it to Env struct. This step might not be necessary anymore * step 4.2: Adjust the pipeline scripts to use the new scripts * step 4.3: Adjust op-chain-ops * step 4.4: Adjust bootstrap scripts. In this case there is only one deployment per CLI target so the Scripts struct needs to be created for every CLI target * step 5: Skip the regression tests so that we can publish the new contract artifacts, after which the tests need to be updated and regression tests unskipped * rebase: Remove unused imports * fix: Misrebase * fix: Misrebase * fix: Misrebase * fix test --------- Co-authored-by: Matthew Slipper <me@matthewslipper.com>
Description
This PR serves as a step-by-step guide for the
op-deployer
script migration. To follow the steps, please follow the commits and their messages. Some of the steps might not apply in your particular case.Questions
Q: There are regression tests that point to incompatible artifacts for existing networks.
@mslipper any opinions on this? Should we keep the tests (which also means we can't delete the old implementations of the scripts)? In general we are introducing a breaking change so at some point we'll drop the support for the old artifacts, the question is when.
A: The tests will be marked as skipped and will be unskipped once the new contract artifacts have been published
Notes
There was one particular concerning find in this PR, you can see it in this CI run. The legacy code in
deploy.go
was not being passed theSuperchainProxyAdmin
that was supposed to be required by the contracts (this should have failed) - seeimplementations.go
for an example of correct usage.After upgrading to the new script, we got a correct EVM revert and a fix was introduced. The concerning thing is that this has not been caught in the CI, and no extra
require
statements have been added to trigger this.