From f27c81520ad96a1d8d832d2f839a5d3b3f45a7a0 Mon Sep 17 00:00:00 2001 From: Chris De Leon Date: Wed, 24 Jan 2024 17:01:47 -0800 Subject: [PATCH] merges CrossDomainOwnable with CrossDomainForwarder and changes CrossDomainGovernor to inherit CrossDomainForwarder --- contracts/src/v0.8/l2ep/README.md | 4 +- .../v0.8/l2ep/dev/CrossDomainForwarder.sol | 68 +++++++++++++++- .../src/v0.8/l2ep/dev/CrossDomainGovernor.sol | 16 +--- .../src/v0.8/l2ep/dev/CrossDomainOwnable.sol | 66 ---------------- .../test/v0.8/dev/CrossDomainOwnable.test.ts | 77 ------------------- 5 files changed, 70 insertions(+), 161 deletions(-) delete mode 100644 contracts/src/v0.8/l2ep/dev/CrossDomainOwnable.sol delete mode 100644 contracts/test/v0.8/dev/CrossDomainOwnable.test.ts diff --git a/contracts/src/v0.8/l2ep/README.md b/contracts/src/v0.8/l2ep/README.md index 21a537c4fe7..992c1f56855 100644 --- a/contracts/src/v0.8/l2ep/README.md +++ b/contracts/src/v0.8/l2ep/README.md @@ -15,9 +15,9 @@ The `/dev` folder contains subfolders for each chain that has an L2EP solution implemented for it (e.g. `/scroll`, `/arbitrum`, `/optimism`). It also contains a subfolder named `/interfaces`, which stores shared interface types between all the supported -contracts. The top-level contracts (e.g. `CrossDomainOwnable.sol`) +contracts. The top-level contracts (e.g. `Validator.sol`) serve as either abstract or parent contracts that are meant -to be reused for each indiviudal chain. +to be reused for each individual chain. ## The `/test` Folder diff --git a/contracts/src/v0.8/l2ep/dev/CrossDomainForwarder.sol b/contracts/src/v0.8/l2ep/dev/CrossDomainForwarder.sol index f40814b8781..34accb57374 100644 --- a/contracts/src/v0.8/l2ep/dev/CrossDomainForwarder.sol +++ b/contracts/src/v0.8/l2ep/dev/CrossDomainForwarder.sol @@ -1,10 +1,11 @@ // SPDX-License-Identifier: MIT pragma solidity 0.8.19; +import {CrossDomainOwnableInterface} from "./interfaces/CrossDomainOwnableInterface.sol"; import {ITypeAndVersion} from "../../shared/interfaces/ITypeAndVersion.sol"; import {ForwarderInterface} from "./interfaces/ForwarderInterface.sol"; -import {CrossDomainOwnable} from "./CrossDomainOwnable.sol"; +import {ConfirmedOwner} from "../../shared/access/ConfirmedOwner.sol"; import {Address} from "../../vendor/openzeppelin-solidity/v4.7.3/contracts/utils/Address.sol"; @@ -12,16 +13,75 @@ import {Address} from "../../vendor/openzeppelin-solidity/v4.7.3/contracts/utils /// @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 consider that position to be held by the `l1Owner` -abstract contract CrossDomainForwarder is ITypeAndVersion, ForwarderInterface, CrossDomainOwnable { +abstract contract CrossDomainForwarder is + ITypeAndVersion, + ForwarderInterface, + CrossDomainOwnableInterface, + ConfirmedOwner +{ + address internal s_l1Owner; + address internal s_l1PendingOwner; + /// @param l1OwnerAddr the L1 owner address that will be allowed to call the forward fn - constructor(address l1OwnerAddr) CrossDomainOwnable(l1OwnerAddr) {} + constructor(address l1OwnerAddr) ConfirmedOwner(msg.sender) { + _setL1Owner(l1OwnerAddr); + } + + /// @notice Reverts if called by anyone other than the L1 owner. + modifier onlyL1Owner() virtual { + // solhint-disable-next-line custom-errors + require(msg.sender == s_l1Owner, "Only callable by L1 owner"); + _; + } + + /// @notice Reverts if called by anyone other than the L1 owner. + modifier onlyProposedL1Owner() virtual { + // solhint-disable-next-line custom-errors + require(msg.sender == s_l1PendingOwner, "Only callable by proposed L1 owner"); + _; + } /// @notice The address of the Cross Domain Messenger contract function crossDomainMessenger() external view virtual returns (address); /// @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 { + function forward(address target, bytes memory data) external virtual override onlyL1Owner { Address.functionCall(target, data, "Forwarder call reverted"); } + + /// @notice transfer ownership of this account to a new L1 owner + /// @param to new L1 owner that will be allowed to call the forward fn + function transferL1Ownership(address to) public virtual override onlyL1Owner { + _transferL1Ownership(to); + } + + /// @notice accept ownership of this account to a new L1 owner + function acceptL1Ownership() public virtual override onlyProposedL1Owner { + _setL1Owner(s_l1PendingOwner); + } + + /// @notice Get the current owner + function l1Owner() public view override returns (address) { + return s_l1Owner; + } + + /// @notice validate, transfer ownership, and emit relevant events + function _transferL1Ownership(address to) internal { + // solhint-disable-next-line custom-errors + require(to != msg.sender, "Cannot transfer to self"); + + s_l1PendingOwner = to; + + emit L1OwnershipTransferRequested(s_l1Owner, to); + } + + /// @notice set ownership, emit relevant events. Used in acceptOwnership() + function _setL1Owner(address to) internal { + address oldOwner = s_l1Owner; + s_l1Owner = to; + s_l1PendingOwner = address(0); + + emit L1OwnershipTransferred(oldOwner, to); + } } diff --git a/contracts/src/v0.8/l2ep/dev/CrossDomainGovernor.sol b/contracts/src/v0.8/l2ep/dev/CrossDomainGovernor.sol index 027b7b50b1b..191377912a8 100644 --- a/contracts/src/v0.8/l2ep/dev/CrossDomainGovernor.sol +++ b/contracts/src/v0.8/l2ep/dev/CrossDomainGovernor.sol @@ -2,10 +2,10 @@ pragma solidity 0.8.19; import {DelegateForwarderInterface} from "./interfaces/DelegateForwarderInterface.sol"; -import {ITypeAndVersion} from "../../shared/interfaces/ITypeAndVersion.sol"; +// solhint-disable-next-line no-unused-import import {ForwarderInterface} from "./interfaces/ForwarderInterface.sol"; -import {CrossDomainOwnable} from "./CrossDomainOwnable.sol"; +import {CrossDomainForwarder} from "./CrossDomainForwarder.sol"; import {Address} from "../../vendor/openzeppelin-solidity/v4.7.3/contracts/utils/Address.sol"; @@ -13,17 +13,9 @@ import {Address} from "../../vendor/openzeppelin-solidity/v4.7.3/contracts/utils /// @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` -abstract contract CrossDomainGovernor is - DelegateForwarderInterface, - ForwarderInterface, - CrossDomainOwnable, - ITypeAndVersion -{ +abstract contract CrossDomainGovernor is DelegateForwarderInterface, CrossDomainForwarder { /// @param l1OwnerAddr the L1 owner address that will be allowed to call the forward fn - constructor(address l1OwnerAddr) CrossDomainOwnable(l1OwnerAddr) {} - - /// @notice The address of the Cross Domain Messenger contract - function crossDomainMessenger() external view virtual returns (address); + constructor(address l1OwnerAddr) CrossDomainForwarder(l1OwnerAddr) {} /// @notice The call MUST come from either the L1 owner (via cross-chain message) or the L2 owner. Reverts otherwise. function _requireLocalOrCrossDomainOwner() internal view virtual; diff --git a/contracts/src/v0.8/l2ep/dev/CrossDomainOwnable.sol b/contracts/src/v0.8/l2ep/dev/CrossDomainOwnable.sol deleted file mode 100644 index 53f3d0b27c9..00000000000 --- a/contracts/src/v0.8/l2ep/dev/CrossDomainOwnable.sol +++ /dev/null @@ -1,66 +0,0 @@ -// SPDX-License-Identifier: MIT -pragma solidity 0.8.19; - -import {CrossDomainOwnableInterface} from "./interfaces/CrossDomainOwnableInterface.sol"; - -import {ConfirmedOwner} from "../../shared/access/ConfirmedOwner.sol"; - -/// @title The CrossDomainOwnable contract -/// @notice A contract with helpers for cross-domain contract ownership. -contract CrossDomainOwnable is CrossDomainOwnableInterface, ConfirmedOwner { - address internal s_l1Owner; - address internal s_l1PendingOwner; - - constructor(address newl1Owner) ConfirmedOwner(msg.sender) { - _setL1Owner(newl1Owner); - } - - /// @notice transfer ownership of this account to a new L1 owner - /// @param to new L1 owner that will be allowed to call the forward fn - function transferL1Ownership(address to) public virtual override onlyL1Owner { - _transferL1Ownership(to); - } - - /// @notice accept ownership of this account to a new L1 owner - function acceptL1Ownership() public virtual override onlyProposedL1Owner { - _setL1Owner(s_l1PendingOwner); - } - - /// @notice Get the current owner - function l1Owner() public view override returns (address) { - return s_l1Owner; - } - - /// @notice validate, transfer ownership, and emit relevant events - function _transferL1Ownership(address to) internal { - // solhint-disable-next-line custom-errors - require(to != msg.sender, "Cannot transfer to self"); - - s_l1PendingOwner = to; - - emit L1OwnershipTransferRequested(s_l1Owner, to); - } - - /// @notice set ownership, emit relevant events. Used in acceptOwnership() - function _setL1Owner(address to) internal { - address oldOwner = s_l1Owner; - s_l1Owner = to; - s_l1PendingOwner = address(0); - - emit L1OwnershipTransferred(oldOwner, to); - } - - /// @notice Reverts if called by anyone other than the L1 owner. - modifier onlyL1Owner() virtual { - // solhint-disable-next-line custom-errors - require(msg.sender == s_l1Owner, "Only callable by L1 owner"); - _; - } - - /// @notice Reverts if called by anyone other than the L1 owner. - modifier onlyProposedL1Owner() virtual { - // solhint-disable-next-line custom-errors - require(msg.sender == s_l1PendingOwner, "Only callable by proposed L1 owner"); - _; - } -} diff --git a/contracts/test/v0.8/dev/CrossDomainOwnable.test.ts b/contracts/test/v0.8/dev/CrossDomainOwnable.test.ts deleted file mode 100644 index 7d9d58cfbaa..00000000000 --- a/contracts/test/v0.8/dev/CrossDomainOwnable.test.ts +++ /dev/null @@ -1,77 +0,0 @@ -import { ethers } from 'hardhat' -import { assert, expect } from 'chai' -import { Contract, ContractFactory } from 'ethers' -import { SignerWithAddress } from '@nomiclabs/hardhat-ethers/signers' - -let owner: SignerWithAddress -let stranger: SignerWithAddress -let l1OwnerAddress: string -let ownableFactory: ContractFactory -let ownable: Contract - -before(async () => { - const accounts = await ethers.getSigners() - owner = accounts[0] - stranger = accounts[1] - l1OwnerAddress = owner.address - - // Contract factories - ownableFactory = await ethers.getContractFactory( - 'src/v0.8/l2ep/dev/CrossDomainOwnable.sol:CrossDomainOwnable', - owner, - ) -}) - -describe('CrossDomainOwnable', () => { - beforeEach(async () => { - ownable = await ownableFactory.deploy(l1OwnerAddress) - }) - - describe('#constructor', () => { - it('should set the l1Owner correctly', async () => { - const response = await ownable.l1Owner() - assert.equal(response, l1OwnerAddress) - }) - }) - - describe('#transferL1Ownership', () => { - it('should not be callable by non-owners', async () => { - await expect( - ownable.connect(stranger).transferL1Ownership(stranger.address), - ).to.be.revertedWith('Only callable by L1 owner') - }) - - it('should be callable by current L1 owner', async () => { - const currentL1Owner = await ownable.l1Owner() - await expect(ownable.transferL1Ownership(stranger.address)) - .to.emit(ownable, 'L1OwnershipTransferRequested') - .withArgs(currentL1Owner, stranger.address) - }) - - it('should be callable by current L1 owner to zero address', async () => { - const currentL1Owner = await ownable.l1Owner() - await expect(ownable.transferL1Ownership(ethers.constants.AddressZero)) - .to.emit(ownable, 'L1OwnershipTransferRequested') - .withArgs(currentL1Owner, ethers.constants.AddressZero) - }) - }) - - describe('#acceptL1Ownership', () => { - it('should not be callable by non pending-owners', async () => { - await expect( - ownable.connect(stranger).acceptL1Ownership(), - ).to.be.revertedWith('Only callable by proposed L1 owner') - }) - - it('should be callable by pending L1 owner', async () => { - const currentL1Owner = await ownable.l1Owner() - await ownable.transferL1Ownership(stranger.address) - await expect(ownable.connect(stranger).acceptL1Ownership()) - .to.emit(ownable, 'L1OwnershipTransferred') - .withArgs(currentL1Owner, stranger.address) - - const updatedL1Owner = await ownable.l1Owner() - assert.equal(updatedL1Owner, stranger.address) - }) - }) -})