feat(ct): overhaul deployment process by smartcontracts · Pull Request #2298 · ethereum-optimism/optimism

smartcontracts · 2022-03-09T17:37:27Z

Description
Overhauls the contract deployment process to use a simpler typed
deployment method. Removes the need for deployment bash scripts and
makes review of deployment configurations much easier.

TODO:

Add parsing/validation for config files
Add checks to confirm environment variables are properly set
Check if anything needs to be modified in stackman
Update README to reflect new deploy process

changeset-bot · 2022-03-09T17:37:30Z

⚠️ No Changeset found

Latest commit: 4c486d7

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

packages/contracts/deploy/004-OVM_CanonicalTransactionChain.deploy.ts

tynes · 2022-03-09T17:44:41Z

packages/contracts/tasks/take-dump.ts

    L2StandardBridge: {
-      l1TokenBridge: cfg.l1StandardBridgeAddress,
+      l1TokenBridge: (
+        await getContractFromArtifact(


This is no longer configurable?

Correct, it gets pulled automatically from the deployment folder based on the current network name. This is effectively what we did anyway, except we did it manually which is error prone.

tynes · 2022-03-09T17:48:22Z

I would say at this point, the most confusing part of the deploy setup is this:

optimism/packages/contracts/hardhat.config.ts

Lines 137 to 140 in 5c83435

    
           if ( 
        
             process.env.CONTRACTS_TARGET_NETWORK && 
        
             process.env.CONTRACTS_DEPLOYER_KEY && 
        
             process.env.CONTRACTS_RPC_URL

These env vars modifying the networks is a bit confusing

smartcontracts · 2022-03-09T17:52:46Z

I would say at this point, the most confusing part of the deploy setup is this:

optimism/packages/contracts/hardhat.config.ts

Lines 137 to 140 in 5c83435

if (

process.env.CONTRACTS_TARGET_NETWORK &&

process.env.CONTRACTS_DEPLOYER_KEY &&

process.env.CONTRACTS_RPC_URL

These env vars modifying the networks is a bit confusing

I agree. We might as well use a more standard setup (different RPC URLs for different networks? API keys? idk).

codecov-commenter · 2022-03-09T22:29:57Z

Codecov Report

Merging #2298 (3cd8d0b) into develop (779709f) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff            @@
##           develop    #2298   +/-   ##
========================================
  Coverage    80.08%   80.08%           
========================================
  Files           77       77           
  Lines         2460     2460           
  Branches       450      450           
========================================
  Hits          1970     1970           
  Misses         490      490

Flag	Coverage Δ
contracts	`99.29% <ø> (ø)`
core-utils	`86.77% <ø> (ø)`
data-transport-layer	`49.72% <ø> (ø)`
sdk	`55.75% <ø> (ø)`

Flags with carried forward coverage won't be shown. Click here to find out more.

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 779709f...3cd8d0b. Read the comment docs.

Overhauls the contract deployment process to use a simpler typed deployment method. Removes the need for deployment bash scripts and makes review of deployment configurations much easier.

tynes · 2022-03-14T17:26:15Z

packages/contracts/README.md

 The output is most easily viewable by opening the html file in your browser:
+
 ```shell
 open ./coverage/index.html


open is a mac specific thing, one particular flavor of linux uses xdg-open

I'll fix this in another PR

mslipper · 2022-03-23T18:02:35Z

packages/contracts/deploy-config/mainnet.ts

@@ -0,0 +1,23 @@
+import { DeployConfig } from '../src/deploy-config'
+


We'll have to add one of these for nightly, but we can do this later.

mslipper · 2022-03-23T18:04:00Z

Do we still have the ability to automatically set the chugsplash proxy owner? Without that capability, deployments get stuck during automated but non-CI builds like nightly.

mslipper

Approved conditionally on my above comment.

smartcontracts · 2022-03-23T19:03:17Z

Do we still have the ability to automatically set the chugsplash proxy owner? Without that capability, deployments get stuck during automated but non-CI builds like nightly.

I haven't changed anything on that front. I think as long as you set the AUTOMATICALLY_TRANSFER_OWNERSHIP environment variable you should be fine.

mslipper · 2022-03-23T19:32:50Z

Great, then I'm happy to merge this if you are.

maurelian

I see that this was merged... but I had a question about the tags flag not being include in mainnet.ts.
If the point of these scripts is to make previous deployments reproducible, I think that's necessary.

maurelian · 2022-03-23T20:06:54Z

packages/contracts/.env.example

+# Private key that will send deployment transactions
+CONTRACTS_DEPLOYER_KEY=
+
+# RPC URL connected to the L1 chain we're deploying to
+CONTRACTS_RPC_URL=
+
+# Your Etherscan API key for the L1 network
+ETHERSCAN_API_KEY=


After a quick text search, I don't see any of these 3 values used anywhere outside the deleted bash scripts.

Can these be deleted? Where would these values now be specified? I don't see where that would be done in the deploy configs.

Ah, looks like they're already in hardhat.config.ts

maurelian · 2022-03-23T20:20:55Z

packages/contracts/scripts/deploy-scripts/mainnet.sh

- --ovm-address-manager-owner 0x9BA6e03D8B90dE867373Db8cF1A58d2F7F006b3A \
- --gasprice 150000000000 \
- --num-deploy-confirmations 4 \
- --tags upgrade \


I don't see this --tags argument reflected in the mainnet.ts flag that replaces this. Should it be?

smartcontracts requested review from elenadimitrova, maurelian, mslipper, optimisticben and tynes as code owners March 9, 2022 17:37

github-actions bot added 2-reviewers A-ops Area: ops labels Mar 9, 2022

smartcontracts marked this pull request as draft March 9, 2022 17:38

tynes reviewed Mar 9, 2022

View reviewed changes

packages/contracts/deploy/004-OVM_CanonicalTransactionChain.deploy.ts Outdated Show resolved Hide resolved

smartcontracts force-pushed the sc/ct-update-deploy branch 2 times, most recently from 7ea02e3 to 90dc13f Compare March 9, 2022 17:44

tynes reviewed Mar 9, 2022

View reviewed changes

smartcontracts marked this pull request as ready for review March 9, 2022 20:27

smartcontracts marked this pull request as draft March 9, 2022 20:28

smartcontracts force-pushed the sc/ct-update-deploy branch 4 times, most recently from ef5b859 to a433656 Compare March 9, 2022 21:30

smartcontracts marked this pull request as ready for review March 9, 2022 21:33

smartcontracts force-pushed the sc/ct-update-deploy branch from a433656 to 3cd8d0b Compare March 9, 2022 21:42

smartcontracts force-pushed the sc/ct-update-deploy branch from 3cd8d0b to bed0c1f Compare March 9, 2022 23:52

feat(ct): overhaul deployment process

4c486d7

Overhauls the contract deployment process to use a simpler typed deployment method. Removes the need for deployment bash scripts and makes review of deployment configurations much easier.

smartcontracts force-pushed the sc/ct-update-deploy branch from bed0c1f to 4c486d7 Compare March 9, 2022 23:58

tynes reviewed Mar 14, 2022

View reviewed changes

maurelian removed their request for review March 17, 2022 13:21

mslipper reviewed Mar 23, 2022

View reviewed changes

mslipper approved these changes Mar 23, 2022

View reviewed changes

mslipper merged commit 3b82da3 into develop Mar 23, 2022

mslipper deleted the sc/ct-update-deploy branch March 23, 2022 19:36

maurelian reviewed Mar 23, 2022

View reviewed changes

		@@ -0,0 +1,23 @@
		import { DeployConfig } from '../src/deploy-config'

Conversation

smartcontracts commented Mar 9, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

changeset-bot bot commented Mar 9, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

⚠️ No Changeset found

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tynes commented Mar 9, 2022

Uh oh!

smartcontracts commented Mar 9, 2022

Uh oh!

codecov-commenter commented Mar 9, 2022

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mslipper commented Mar 23, 2022

Uh oh!

mslipper left a comment

Choose a reason for hiding this comment

Uh oh!

smartcontracts commented Mar 23, 2022

Uh oh!

mslipper commented Mar 23, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

maurelian left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

smartcontracts commented Mar 9, 2022 •

edited

Loading

changeset-bot bot commented Mar 9, 2022 •

edited

Loading

mslipper commented Mar 23, 2022 •

edited

Loading