Skip to content

feat[contracts]: enable initiating L2 upgrade via L1 to L2 message#854

Closed
smartcontracts wants to merge 2 commits intov0.4.0-rcfrom
kelvin/l2-upgrade-xdomain-init
Closed

feat[contracts]: enable initiating L2 upgrade via L1 to L2 message#854
smartcontracts wants to merge 2 commits intov0.4.0-rcfrom
kelvin/l2-upgrade-xdomain-init

Conversation

@smartcontracts
Copy link
Contributor

Description
Pretty straightforward. Makes L2ChugSplashDeployer inherit from OVM_CrossDomainEnabled and adds a new modifier that allows functions to be called either directly by the owner or via a cross-domain message. Adds tests to cover this new functionality.

@changeset-bot
Copy link

changeset-bot bot commented May 12, 2021

🦋 Changeset detected

Latest commit: 4b5de02

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
@eth-optimism/contracts Patch

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

* via an L1 => L2 message.
*/
modifier onlyOwnerOrCrossDomainOwner() {
require(
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this a gas savings thing to pack so much in a single expression? If so, I understand why there is utility but otherwise readability is more important

Copy link
Contributor Author

Choose a reason for hiding this comment

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

How would you reformat this to make it more legible?

Copy link
Contributor

Choose a reason for hiding this comment

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

Doesn't the cross domains enabled lib have something that does half of this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, but I don't believe it's possible to compose two function modifiers with an "OR".

Copy link
Collaborator

@ben-chain ben-chain left a comment

Choose a reason for hiding this comment

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

This looks good as-is, but I do think there is one way to beef up security which we should talk through. As I understand it there will be environments in which we want the owner to be an L2 account, and other environments where we want it to be an L1 account. However, I don't believe that we will ever have both holding true simultaneously. In this case, the onlyOwnerOrCrossDomainOwner modifier does open up an additional attack surface by having the ||. If we have the contract set up to be owned by L1, then the modifier would still allow an upgrade through if someone were able to impersonate an OVM account (i.e. if they could somehow arbitarily set the ovmCALLER. Unless we think the configuration overhead is significantly higher, I would suggest that we add an ownedByL1OrL2 storage variable which triggers one of the two checks, instead of always doing both.

@smartcontracts
Copy link
Contributor Author

This looks good as-is, but I do think there is one way to beef up security which we should talk through. As I understand it there will be environments in which we want the owner to be an L2 account, and other environments where we want it to be an L1 account

Hmmm after further consideration, I think that we should initially only support L1 accounts and require that upgrades be initiated via an L1 => L2 message. This is how we're going to be performing upgrades on mainnet, so it seems prudent to force ourselves to use the same exact flow for our testnets.

@smartcontracts
Copy link
Contributor Author

After further review of the changes necessary here, I think it might be best to have the owner be another contract that itself can only be called by an L1 => L2 message. Adding this functionality from within this contract is actually quite difficult to test.

@smartcontracts
Copy link
Contributor Author

Going to close this PR and open up another one with that alternative approach.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

C-feature Category: features

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants