Skip to content

Comments

feat: update mainnet deploy scripts#1723

Merged
smartcontracts merged 1 commit intoregenesis/0.5.0from
sc/update-mainnet-deploy-scripts
Nov 10, 2021
Merged

feat: update mainnet deploy scripts#1723
smartcontracts merged 1 commit intoregenesis/0.5.0from
sc/update-mainnet-deploy-scripts

Conversation

@smartcontracts
Copy link
Contributor

Description
Updates the mainnet deploy scripts with what I believe to be the correct values for each variable. Must be merged by today.

@changeset-bot
Copy link

changeset-bot bot commented Nov 9, 2021

⚠️ No Changeset found

Latest commit: b17c6e5

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

export L1_FEE_WALLET_ADDRESS=0x391716d440c151c42cdf1c95c1d83a5427bca52c
export L1_CROSS_DOMAIN_MESSENGER_ADDRESS=0x25ace71c97B33Cc4729CF772ae268934F7ab5fA1
export WHITELIST_OWNER=0x648E3e8101BFaB7bf5997Bd007Fb473786019159
export GAS_PRICE_ORACLE_OWNER=0x648E3e8101BFaB7bf5997Bd007Fb473786019159
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the only value that I'm not confident in

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

verified that this matches the k8s settings, but not sure if it should be different

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We shouldn't change the gas oracle owner, so its good

@codecov-commenter
Copy link

codecov-commenter commented Nov 9, 2021

Codecov Report

Merging #1723 (0002116) into regenesis/0.5.0 (923d9d9) will decrease coverage by 0.51%.
The diff coverage is n/a.

❗ Current head 0002116 differs from pull request most recent head b17c6e5. Consider uploading reports for the commit b17c6e5 to get more accurate results
Impacted file tree graph

@@                 Coverage Diff                 @@
##           regenesis/0.5.0    #1723      +/-   ##
===================================================
- Coverage            72.64%   72.12%   -0.52%     
===================================================
  Files                   69       67       -2     
  Lines                 2274     2167     -107     
  Branches               337      324      -13     
===================================================
- Hits                  1652     1563      -89     
+ Misses                 622      604      -18     
Flag Coverage Δ
batch-submitter 61.56% <ø> (ø)
contracts 87.96% <ø> (ø)
core-utils 57.72% <ø> (ø)
data-transport-layer 38.23% <ø> (ø)
message-relayer ?

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

Impacted Files Coverage Δ
packages/message-relayer/src/relay-tx.ts
packages/message-relayer/hardhat.config.ts

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 923d9d9...b17c6e5. Read the comment docs.

@tynes
Copy link
Contributor

tynes commented Nov 9, 2021

Replaces #1693

export L1_FEE_WALLET_ADDRESS=0x391716d440c151c42cdf1c95c1d83a5427bca52c
export L1_CROSS_DOMAIN_MESSENGER_ADDRESS=0x25ace71c97B33Cc4729CF772ae268934F7ab5fA1
export WHITELIST_OWNER=0x648E3e8101BFaB7bf5997Bd007Fb473786019159
export GAS_PRICE_ORACLE_OWNER=0x648E3e8101BFaB7bf5997Bd007Fb473786019159
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

verified that this matches the k8s settings, but not sure if it should be different

Comment on lines +28 to +30
--ovm-sequencer-address 0x6887246668a3b87F54DeB3b94Ba47a6f63F32985 \
--ovm-proposer-address 0x473300df21D047806A082244b417f96b32f13A33 \
--ovm-address-manager-owner 0x9BA6e03D8B90dE867373Db8cF1A58d2F7F006b3A \
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what should these be compared against?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

They aren't really changed, the diff is just a bit messed up.
But I believe the right way to check is to ensure that they match what's in the AddressManager now.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should be compared against the existing mainnet configuration values

--ovm-sequencer-address 0x6887246668a3b87F54DeB3b94Ba47a6f63F32985 \
--ovm-proposer-address 0x473300df21D047806A082244b417f96b32f13A33 \
--ovm-address-manager-owner 0x9BA6e03D8B90dE867373Db8cF1A58d2F7F006b3A \
--gasprice 150000000000 \
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This may not be the best value to use at the time, will need to determine what it should be at deploy time

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This could be determined at runtime using seth but unsure if we want to add another dependency to this script

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

seth basefee or seth gas-price

--ovm-sequencer-address 0x6887246668a3b87F54DeB3b94Ba47a6f63F32985 \
--ovm-proposer-address 0x473300df21D047806A082244b417f96b32f13A33 \
--ovm-address-manager-owner 0x9BA6e03D8B90dE867373Db8cF1A58d2F7F006b3A \
--gasprice 150000000000 \
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This could be determined at runtime using seth but unsure if we want to add another dependency to this script

--ovm-sequencer-address 0x6887246668a3b87F54DeB3b94Ba47a6f63F32985 \
--ovm-proposer-address 0x473300df21D047806A082244b417f96b32f13A33 \
--ovm-address-manager-owner 0x9BA6e03D8B90dE867373Db8cF1A58d2F7F006b3A \
--gasprice 150000000000 \
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

seth basefee or seth gas-price

Comment on lines +28 to +30
--ovm-sequencer-address 0x6887246668a3b87F54DeB3b94Ba47a6f63F32985 \
--ovm-proposer-address 0x473300df21D047806A082244b417f96b32f13A33 \
--ovm-address-manager-owner 0x9BA6e03D8B90dE867373Db8cF1A58d2F7F006b3A \
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

They aren't really changed, the diff is just a bit messed up.
But I believe the right way to check is to ensure that they match what's in the AddressManager now.

--ovm-proposer-address 0x473300df21D047806A082244b417f96b32f13A33 \
--ovm-address-manager-owner 0x9BA6e03D8B90dE867373Db8cF1A58d2F7F006b3A \
--gasprice 150000000000 \
--num-deploy-confirmations 12 \
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This might be overkill?

It's about 3 minutes. I count 9 files with the upgrade tag, each of which has at least one transaction. So with no delay getting a tx mined, and not counting time for validation and signing, this will take half an hour.

Some quick searching suggests that reorgs greater than 2 blocks are extremely rare
https://ethereum.stackexchange.com/questions/62000/distribution-of-chain-reorganization-events/73007
https://etherscan.io/blocks_forked

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe 3 or 4 confs then?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reduced to 4 confs.

Comment on lines 36 to +39
CONTRACTS_TARGET_NETWORK=mainnet \
npx hardhat etherscan-verify --network mainnet
npx hardhat etherscan-verify \
--network mainnet \
--sleep
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We shouldn't need any of this with #1722.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Might be worth keeping for now given that we don't throw if the etherscan validation fails the first time around.

@smartcontracts smartcontracts force-pushed the sc/update-mainnet-deploy-scripts branch from 7e97eb5 to 0002116 Compare November 10, 2021 17:29
@smartcontracts smartcontracts force-pushed the sc/update-mainnet-deploy-scripts branch from 0002116 to b17c6e5 Compare November 10, 2021 17:40
@smartcontracts smartcontracts merged commit 02cfd2d into regenesis/0.5.0 Nov 10, 2021
@smartcontracts smartcontracts deleted the sc/update-mainnet-deploy-scripts branch November 10, 2021 20:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants