feat[contracts]: have hardhat-deploy register L1MultiMessageRelayer on mainnet#828
Conversation
🦋 Changeset detectedLatest commit: 15351ff The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
@ben-chain @K-Ho @tynes -- We need to merge this before we perform the mainnet regenesis to make sure that the message relayer is set on L1. |
karlfloersch
left a comment
karlfloersch
left a comment
There was a problem hiding this comment.
This LGTM but we should] make sure others see this (tagged some folks) & give an approval before it's merged
| }) | ||
|
|
||
| // OVM_L2MessageRelayer *must* be set to multi message relayer address on mainnet. | ||
| if (hre.network.name.includes('mainnet')) { |
There was a problem hiding this comment.
Discussed with @smartcontracts how best to only enable this for mainnet & we settled on this approach. Feedback welcome
There was a problem hiding this comment.
Looks fine but I'll pose the obvious devil's advocate: Are there situations where mainnet would be only a substring of hre.network.name, and we would still want to trigger this condition?
There was a problem hiding this comment.
This seems fine to me as it will target one of many possible mainnet deployments at the same time
There was a problem hiding this comment.
Hmmm, thinking about it more, it is probably the case that we want to trigger this logic as much as we possibly can, because the downside of not triggering it when we do want to is much greated than the downside of triggering it when we do not.
There was a problem hiding this comment.
whoops, didn't see @tynes ' comment when I followed up, but sounds good 👍
Codecov Report
@@ Coverage Diff @@
## master #828 +/- ##
=======================================
Coverage 80.66% 80.66%
=======================================
Files 48 48
Lines 1903 1903
Branches 302 302
=======================================
Hits 1535 1535
Misses 368 368 Continue to review full report at Codecov.
|
|
So the assumption is that |
|
|
||
| // OVM_L2MessageRelayer *must* be set to multi message relayer address on mainnet. | ||
| if (hre.network.name.includes('mainnet')) { | ||
| const OVM_L1MultiMessageRelayer = await getDeployedContract( |
There was a problem hiding this comment.
Does this method throw if the given string is not set in the address manager? We should make sure we explicitly avoid a situation where we end up calling setAddress, but the input is still 0x00..., so if it does not, we should add the check here.
There was a problem hiding this comment.
This function doesn't pull from the address manager -- it pulls from the local list of known/deployed contracts managed by hardhat-deploy. Will throw if the given contract name does not exist.
ben-chain
left a comment
ben-chain
left a comment
There was a problem hiding this comment.
Whoops, left individual comments by accident instead of within a review. I have a couple small questions which I want to make sure are addressed explicitly before I approve but I think this is probably good to go.
I'm not sure if it's that straightforward, since the address we want |
Right now on kovan the |
…n mainnet (ethereum-optimism#828) * feat[contracts]: have deploy register multi message relayer on L1 * chore: add changeset * fix: minor typo in comment * fix: remove unnecessary changes
5 participants
Description
Slightly modifies our deployment so that we'll set
OVM_L2MessageRelayer = L1MultiMessageRelayer.addresson mainnet.