-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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
[DEPLOY-178]: Adds Scroll L2EP Contracts #11405
Merged
Merged
Changes from 19 commits
Commits
Show all changes
23 commits
Select commit
Hold shift + click to select a range
ab23752
Adds scroll L2EP contracts and tests
chris-de-leon-cll 74d7a91
fixes comments, adds fixed solidity compiler version, and adds scroll…
chris-de-leon-cll 2443b2d
renames SCROLL_CROSS_DOMAIN_MESSENGER to i_SCROLL_CROSS_DOMAIN_MESSEN…
chris-de-leon-cll 6ec4f81
removes unnecessary solhint disable rule
chris-de-leon-cll a6e29b7
moves scroll mocks from tests folder to l2ep folder
chris-de-leon-cll c26330f
resolve solhint errors for scroll
chris-de-leon-cll 7a849cc
proposed restructure to reduce inheritance
RensR 785fc51
Merge pull request #11471 from smartcontractkit/l2ep-fixes
chris-de-leon-cll e81403d
Merge branch 'develop' into feat/DEPLOY-178-l2ep-scroll
chris-de-leon-cll 1405c41
removes extraneous comments from scroll contracts and refactors typeA…
chris-de-leon-cll 6786173
use named parameters in mappings for scroll contracts
chris-de-leon-cll b6df5fe
removes extraneous empty comments from scroll contracts (again)
chris-de-leon-cll a006199
removes unnecessary comment from scroll cross domain governor
chris-de-leon-cll 9db4bb9
Merge branch 'develop' into feat/DEPLOY-178-l2ep-scroll
chris-de-leon-cll 3c38e37
adds minor formatting updates to scroll mocks
chris-de-leon-cll ced25df
adds onlyL1Owner modifier back to ScrollCrossDomainGovernor
chris-de-leon-cll e0076a6
Merge branch 'develop' into feat/DEPLOY-178-l2ep-scroll
chris-de-leon-cll 5f58f39
adds style and formatting improvements
chris-de-leon-cll f281a4b
adds formatting updates
chris-de-leon-cll e1402b9
Merge branch 'develop' into feat/DEPLOY-178-l2ep-scroll
chris-de-leon-cll 0a30bc9
Merge branch 'develop' into feat/DEPLOY-178-l2ep-scroll
chris-de-leon-cll ebbb791
refactors scroll sequencer uptime feed to reduce gas and updates test…
chris-de-leon-cll 1b8c936
Merge branch 'develop' into feat/DEPLOY-178-l2ep-scroll
chris-de-leon-cll File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Oops, something went wrong.
6 changes: 6 additions & 0 deletions
6
contracts/src/v0.8/l2ep/dev/interfaces/ScrollSequencerUptimeFeedInterface.sol
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,6 @@ | ||
// SPDX-License-Identifier: MIT | ||
pragma solidity 0.8.19; | ||
|
||
interface ScrollSequencerUptimeFeedInterface { | ||
function updateStatus(bool status, uint64 timestamp) external; | ||
} |
65 changes: 65 additions & 0 deletions
65
contracts/src/v0.8/l2ep/dev/scroll/ScrollCrossDomainForwarder.sol
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,65 @@ | ||
// SPDX-License-Identifier: MIT | ||
pragma solidity 0.8.19; | ||
|
||
import {TypeAndVersionInterface} from "../../../interfaces/TypeAndVersionInterface.sol"; | ||
import {ForwarderInterface} from "../interfaces/ForwarderInterface.sol"; | ||
|
||
import {CrossDomainForwarder} from "../CrossDomainForwarder.sol"; | ||
import {CrossDomainOwnable} from "../CrossDomainOwnable.sol"; | ||
|
||
import {IScrollMessenger} from "@scroll-tech/contracts/libraries/IScrollMessenger.sol"; | ||
import {Address} from "../../../vendor/openzeppelin-solidity/v4.7.3/contracts/utils/Address.sol"; | ||
|
||
/// @title ScrollCrossDomainForwarder - L1 xDomain account representation | ||
/// @notice L2 Contract which receives messages from a specific L1 address and transparently forwards them to the destination. | ||
/// @dev Any other L2 contract which uses this contract's address as a privileged position, | ||
/// can be considered to be owned by the `l1Owner` | ||
contract ScrollCrossDomainForwarder is TypeAndVersionInterface, CrossDomainForwarder { | ||
// solhint-disable-next-line chainlink-solidity/all-caps-constant-storage-variables | ||
string public constant override typeAndVersion = "ScrollCrossDomainForwarder 1.0.0"; | ||
|
||
address internal immutable i_scrollCrossDomainMessenger; | ||
|
||
/// @param crossDomainMessengerAddr the xDomain bridge messenger (Scroll bridge L2) contract address | ||
/// @param l1OwnerAddr the L1 owner address that will be allowed to call the forward fn | ||
constructor(IScrollMessenger crossDomainMessengerAddr, address l1OwnerAddr) CrossDomainOwnable(l1OwnerAddr) { | ||
// solhint-disable-next-line custom-errors | ||
require(address(crossDomainMessengerAddr) != address(0), "Invalid xDomain Messenger address"); | ||
i_scrollCrossDomainMessenger = address(crossDomainMessengerAddr); | ||
} | ||
|
||
/// @dev forwarded only if L2 Messenger calls with `xDomainMessageSender` being the L1 owner address | ||
/// @inheritdoc ForwarderInterface | ||
function forward(address target, bytes memory data) external override onlyL1Owner { | ||
Address.functionCall(target, data, "Forwarder call reverted"); | ||
} | ||
|
||
/// @notice This is always the address of the Scroll Cross Domain Messenger contract | ||
function crossDomainMessenger() external view returns (address) { | ||
return address(i_scrollCrossDomainMessenger); | ||
} | ||
|
||
/// @notice The call MUST come from the L1 owner (via cross-chain message.) Reverts otherwise. | ||
modifier onlyL1Owner() override { | ||
RensR marked this conversation as resolved.
Show resolved
Hide resolved
|
||
// solhint-disable-next-line custom-errors | ||
require(msg.sender == i_scrollCrossDomainMessenger, "Sender is not the L2 messenger"); | ||
// solhint-disable-next-line custom-errors | ||
require( | ||
IScrollMessenger(i_scrollCrossDomainMessenger).xDomainMessageSender() == l1Owner(), | ||
"xDomain sender is not the L1 owner" | ||
); | ||
_; | ||
} | ||
|
||
/// @notice The call MUST come from the proposed L1 owner (via cross-chain message.) Reverts otherwise. | ||
modifier onlyProposedL1Owner() override { | ||
// solhint-disable-next-line custom-errors | ||
require(msg.sender == i_scrollCrossDomainMessenger, "Sender is not the L2 messenger"); | ||
// solhint-disable-next-line custom-errors | ||
require( | ||
IScrollMessenger(i_scrollCrossDomainMessenger).xDomainMessageSender() == s_l1PendingOwner, | ||
"Must be proposed L1 owner" | ||
); | ||
_; | ||
} | ||
} |
92 changes: 92 additions & 0 deletions
92
contracts/src/v0.8/l2ep/dev/scroll/ScrollCrossDomainGovernor.sol
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,92 @@ | ||
// SPDX-License-Identifier: MIT | ||
pragma solidity 0.8.19; | ||
|
||
import {TypeAndVersionInterface} from "../../../interfaces/TypeAndVersionInterface.sol"; | ||
import {DelegateForwarderInterface} from "../interfaces/DelegateForwarderInterface.sol"; | ||
// solhint-disable-next-line no-unused-import | ||
import {ForwarderInterface} from "../interfaces/ForwarderInterface.sol"; | ||
|
||
import {CrossDomainForwarder} from "../CrossDomainForwarder.sol"; | ||
import {CrossDomainOwnable} from "../CrossDomainOwnable.sol"; | ||
|
||
import {IScrollMessenger} from "@scroll-tech/contracts/libraries/IScrollMessenger.sol"; | ||
import {Address} from "../../../vendor/openzeppelin-solidity/v4.7.3/contracts/utils/Address.sol"; | ||
|
||
/// @title ScrollCrossDomainGovernor - L1 xDomain account representation (with delegatecall support) for Scroll | ||
/// @notice L2 Contract which receives messages from a specific L1 address and transparently forwards them to the destination. | ||
/// @dev Any other L2 contract which uses this contract's address as a privileged position, | ||
/// can be considered to be simultaneously owned by the `l1Owner` and L2 `owner` | ||
contract ScrollCrossDomainGovernor is DelegateForwarderInterface, TypeAndVersionInterface, CrossDomainForwarder { | ||
// solhint-disable-next-line chainlink-solidity/all-caps-constant-storage-variables | ||
string public constant override typeAndVersion = "ScrollCrossDomainGovernor 1.0.0"; | ||
|
||
address internal immutable i_scrollCrossDomainMessenger; | ||
|
||
/// @param crossDomainMessengerAddr the xDomain bridge messenger (Scroll bridge L2) contract address | ||
/// @param l1OwnerAddr the L1 owner address that will be allowed to call the forward fn | ||
constructor(IScrollMessenger crossDomainMessengerAddr, address l1OwnerAddr) CrossDomainOwnable(l1OwnerAddr) { | ||
// solhint-disable-next-line custom-errors | ||
require(address(crossDomainMessengerAddr) != address(0), "Invalid xDomain Messenger address"); | ||
i_scrollCrossDomainMessenger = address(crossDomainMessengerAddr); | ||
} | ||
|
||
/// @inheritdoc ForwarderInterface | ||
/// @dev forwarded only if L2 Messenger calls with `msg.sender` being the L1 owner address, or called by the L2 owner | ||
function forward(address target, bytes memory data) external override onlyLocalOrCrossDomainOwner { | ||
Address.functionCall(target, data, "Governor call reverted"); | ||
} | ||
|
||
/// @inheritdoc DelegateForwarderInterface | ||
/// @dev forwarded only if L2 Messenger calls with `msg.sender` being the L1 owner address, or called by the L2 owner | ||
function forwardDelegate(address target, bytes memory data) external override onlyLocalOrCrossDomainOwner { | ||
Address.functionDelegateCall(target, data, "Governor delegatecall reverted"); | ||
} | ||
|
||
/// @notice The address of the Scroll Cross Domain Messenger contract | ||
function crossDomainMessenger() external view returns (address) { | ||
return address(i_scrollCrossDomainMessenger); | ||
} | ||
|
||
/// @notice The call MUST come from the L1 owner (via cross-chain message.) Reverts otherwise. | ||
modifier onlyL1Owner() override { | ||
// solhint-disable-next-line custom-errors | ||
require(msg.sender == i_scrollCrossDomainMessenger, "Sender is not the L2 messenger"); | ||
// solhint-disable-next-line custom-errors | ||
require( | ||
IScrollMessenger(i_scrollCrossDomainMessenger).xDomainMessageSender() == l1Owner(), | ||
"xDomain sender is not the L1 owner" | ||
); | ||
_; | ||
} | ||
|
||
/// @notice The call MUST come from either the L1 owner (via cross-chain message) or the L2 owner. Reverts otherwise. | ||
modifier onlyLocalOrCrossDomainOwner() { | ||
// 1. The delegatecall MUST come from either the L1 owner (via cross-chain message) or the L2 owner | ||
// solhint-disable-next-line custom-errors | ||
require( | ||
msg.sender == i_scrollCrossDomainMessenger || msg.sender == owner(), | ||
"Sender is not the L2 messenger or owner" | ||
); | ||
// 2. The L2 Messenger's caller MUST be the L1 Owner | ||
if (msg.sender == i_scrollCrossDomainMessenger) { | ||
// solhint-disable-next-line custom-errors | ||
require( | ||
IScrollMessenger(i_scrollCrossDomainMessenger).xDomainMessageSender() == l1Owner(), | ||
"xDomain sender is not the L1 owner" | ||
); | ||
} | ||
_; | ||
} | ||
|
||
/// @notice The call MUST come from the proposed L1 owner (via cross-chain message.) Reverts otherwise. | ||
modifier onlyProposedL1Owner() override { | ||
// solhint-disable-next-line custom-errors | ||
require(msg.sender == i_scrollCrossDomainMessenger, "Sender is not the L2 messenger"); | ||
// solhint-disable-next-line custom-errors | ||
require( | ||
IScrollMessenger(i_scrollCrossDomainMessenger).xDomainMessageSender() == s_l1PendingOwner, | ||
"Must be proposed L1 owner" | ||
); | ||
_; | ||
} | ||
} |
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
Do we need the require statements for backwards compatibility?
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.
I'm not too sure about the backwards compatibility portion (if any other reviewer has more insights on this please feel free to follow up here)
However, I don't think it hurts to include the additional check here (Optimism also had one as well). It also helps avoid errors on this line as well.
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.
I meant require over custom errors. We generally don't write new code that uses require statements
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.
@mohamed-mehany any thoughts here?
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.
@RensR Would it be possible to produce the same interface (including the errors) using custom errors?
Because Optimism and Arbitrum L2EP validators also have the same error handling logic and it would be better if we can maintain the same interface across all chains contracts.
If it's possible then we can do that in this PR, if not we can create a ticket to refactor all L2EP contracts (for all chains) to use custom errors.
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.
Yes but that completely defeats the purpose of custom errors. If you need it for backwards compatibility reasons, that valid. We can create a ticket to "modernise" all L2EP contracts, that would be great
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.
Sounds good! I will leave the require statements alone in this PR, and we can update them for the L2EP contracts at another time. @mohamed-mehany have any preferences on where we should track the ticket? I believe the Scroll work is being tracked here, but not sure if you have another place in mind.