Commit be4647ec authored by smartcontracts's avatar smartcontracts Committed by GitHub

feat(ctb): use ERC721Enumerable for standard token (#2917)

Replaces ERC721 with ERC721Enumerable for the standard version of the
OptimismMintableERC721 token. Enumerable is generally less useful on L1
because it consumes more gas but it makes indexing significantly easier
and gas is cheap on L2. Also removes reinitializer functions because we
don't want to be initializing contracts every time we re-deploy.
parent bfbb08e2
...@@ -21,7 +21,7 @@ contract L1ERC721Bridge is CrossDomainEnabled, OwnableUpgradeable { ...@@ -21,7 +21,7 @@ contract L1ERC721Bridge is CrossDomainEnabled, OwnableUpgradeable {
/** /**
* @notice Contract version number. * @notice Contract version number.
*/ */
uint8 public constant VERSION = 1; uint256 public constant VERSION = 2;
/** /**
* @notice Emitted when an ERC721 bridge to the other network is initiated. * @notice Emitted when an ERC721 bridge to the other network is initiated.
...@@ -85,7 +85,7 @@ contract L1ERC721Bridge is CrossDomainEnabled, OwnableUpgradeable { ...@@ -85,7 +85,7 @@ contract L1ERC721Bridge is CrossDomainEnabled, OwnableUpgradeable {
* @param _messenger Address of the CrossDomainMessenger on this network. * @param _messenger Address of the CrossDomainMessenger on this network.
* @param _otherBridge Address of the ERC721 bridge on the other network. * @param _otherBridge Address of the ERC721 bridge on the other network.
*/ */
function initialize(address _messenger, address _otherBridge) public reinitializer(VERSION) { function initialize(address _messenger, address _otherBridge) public initializer {
messenger = _messenger; messenger = _messenger;
otherBridge = _otherBridge; otherBridge = _otherBridge;
......
...@@ -23,7 +23,7 @@ contract L2ERC721Bridge is CrossDomainEnabled, OwnableUpgradeable { ...@@ -23,7 +23,7 @@ contract L2ERC721Bridge is CrossDomainEnabled, OwnableUpgradeable {
/** /**
* @notice Contract version number. * @notice Contract version number.
*/ */
uint8 public constant VERSION = 1; uint256 public constant VERSION = 2;
/** /**
* @notice Emitted when an ERC721 bridge to the other network is initiated. * @notice Emitted when an ERC721 bridge to the other network is initiated.
...@@ -99,7 +99,7 @@ contract L2ERC721Bridge is CrossDomainEnabled, OwnableUpgradeable { ...@@ -99,7 +99,7 @@ contract L2ERC721Bridge is CrossDomainEnabled, OwnableUpgradeable {
* @param _messenger Address of the CrossDomainMessenger on this network. * @param _messenger Address of the CrossDomainMessenger on this network.
* @param _otherBridge Address of the ERC721 bridge on the other network. * @param _otherBridge Address of the ERC721 bridge on the other network.
*/ */
function initialize(address _messenger, address _otherBridge) public reinitializer(VERSION) { function initialize(address _messenger, address _otherBridge) public initializer {
messenger = _messenger; messenger = _messenger;
otherBridge = _otherBridge; otherBridge = _otherBridge;
......
// SPDX-License-Identifier: MIT // SPDX-License-Identifier: MIT
pragma solidity ^0.8.9; pragma solidity ^0.8.9;
import { IERC721 } from "@openzeppelin/contracts/token/ERC721/IERC721.sol"; import {
IERC721Enumerable
} from "@openzeppelin/contracts/token/ERC721/extensions/IERC721Enumerable.sol";
/** /**
* @title IOptimismMintableERC721 * @title IOptimismMintableERC721
* @notice Interface for contracts that are compatible with the OptimismMintableERC721 standard. * @notice Interface for contracts that are compatible with the OptimismMintableERC721 standard.
* Tokens that follow this standard can be easily transferred across the ERC721 bridge. * Tokens that follow this standard can be easily transferred across the ERC721 bridge.
*/ */
interface IOptimismMintableERC721 is IERC721 { interface IOptimismMintableERC721 is IERC721Enumerable {
/** /**
* @notice Emitted when a token is minted. * @notice Emitted when a token is minted.
* *
......
// SPDX-License-Identifier: MIT // SPDX-License-Identifier: MIT
pragma solidity ^0.8.9; pragma solidity ^0.8.9;
import {
ERC721Enumerable
} from "@openzeppelin/contracts/token/ERC721/extensions/ERC721Enumerable.sol";
import { ERC721 } from "@openzeppelin/contracts/token/ERC721/ERC721.sol"; import { ERC721 } from "@openzeppelin/contracts/token/ERC721/ERC721.sol";
import { IERC165 } from "@openzeppelin/contracts/utils/introspection/IERC165.sol"; import { IERC165 } from "@openzeppelin/contracts/utils/introspection/IERC165.sol";
import { Strings } from "@openzeppelin/contracts/utils/Strings.sol"; import { Strings } from "@openzeppelin/contracts/utils/Strings.sol";
...@@ -12,7 +15,7 @@ import { IOptimismMintableERC721 } from "./IOptimismMintableERC721.sol"; ...@@ -12,7 +15,7 @@ import { IOptimismMintableERC721 } from "./IOptimismMintableERC721.sol";
* typically an Optimism representation of an Ethereum-based token. Standard reference * typically an Optimism representation of an Ethereum-based token. Standard reference
* implementation that can be extended or modified according to your needs. * implementation that can be extended or modified according to your needs.
*/ */
contract OptimismMintableERC721 is ERC721, IOptimismMintableERC721 { contract OptimismMintableERC721 is ERC721Enumerable, IOptimismMintableERC721 {
/** /**
* @inheritdoc IOptimismMintableERC721 * @inheritdoc IOptimismMintableERC721
*/ */
...@@ -100,7 +103,7 @@ contract OptimismMintableERC721 is ERC721, IOptimismMintableERC721 { ...@@ -100,7 +103,7 @@ contract OptimismMintableERC721 is ERC721, IOptimismMintableERC721 {
function supportsInterface(bytes4 _interfaceId) function supportsInterface(bytes4 _interfaceId)
public public
view view
override(ERC721, IERC165) override(ERC721Enumerable, IERC165)
returns (bool) returns (bool)
{ {
bytes4 iface1 = type(IERC165).interfaceId; bytes4 iface1 = type(IERC165).interfaceId;
......
/* Imports */ /* Imports */
import { ethers } from 'hardhat' import { ethers } from 'hardhat'
import { Signer, ContractFactory, Contract, constants } from 'ethers' import { Signer, ContractFactory, Contract, constants } from 'ethers'
import { smock, FakeContract, MockContract } from '@defi-wonderland/smock' import { smock, FakeContract } from '@defi-wonderland/smock'
import ICrossDomainMessenger from '@eth-optimism/contracts/artifacts/contracts/libraries/bridge/ICrossDomainMessenger.sol/ICrossDomainMessenger.json' import ICrossDomainMessenger from '@eth-optimism/contracts/artifacts/contracts/libraries/bridge/ICrossDomainMessenger.sol/ICrossDomainMessenger.json'
import { toRpcHexString } from '@eth-optimism/core-utils'
import { expect } from '../../../setup' import { expect } from '../../../setup'
import { import {
...@@ -20,7 +21,6 @@ const DUMMY_L1ERC721_ADDRESS: string = ...@@ -20,7 +21,6 @@ const DUMMY_L1ERC721_ADDRESS: string =
'0x2234223412342234223422342234223422342234' '0x2234223412342234223422342234223422342234'
const ERR_INVALID_WITHDRAWAL: string = const ERR_INVALID_WITHDRAWAL: string =
'Withdrawal is not being initiated by NFT owner' 'Withdrawal is not being initiated by NFT owner'
const ALICE_INITIAL_BALANCE: number = 10
const TOKEN_ID: number = 10 const TOKEN_ID: number = 10
describe('L2ERC721Bridge', () => { describe('L2ERC721Bridge', () => {
...@@ -219,32 +219,35 @@ describe('L2ERC721Bridge', () => { ...@@ -219,32 +219,35 @@ describe('L2ERC721Bridge', () => {
}) })
describe('withdrawals', () => { describe('withdrawals', () => {
let Mock__L2Token: MockContract<Contract> let L2Token: Contract
beforeEach(async () => { beforeEach(async () => {
Mock__L2Token = await ( L2Token = await (
await smock.mock('OptimismMintableERC721') await ethers.getContractFactory('OptimismMintableERC721')
).deploy( ).deploy(
L2ERC721Bridge.address, L2ERC721Bridge.address,
100, 100,
DUMMY_L1ERC721_ADDRESS, DUMMY_L1ERC721_ADDRESS,
'L2Token', 'L2Token',
'L2T', 'L2T'
{ gasLimit: 4_000_000 } // Necessary to avoid an out-of-gas error
) )
await Mock__L2Token.setVariable('_owners', { await ethers.provider.send('hardhat_impersonateAccount', [
[TOKEN_ID]: aliceAddress, L2ERC721Bridge.address,
}) ])
await Mock__L2Token.setVariable('_balances', {
[aliceAddress]: ALICE_INITIAL_BALANCE, await ethers.provider.send('hardhat_setBalance', [
}) L2ERC721Bridge.address,
toRpcHexString(ethers.utils.parseEther('1')),
])
const signer = await ethers.getSigner(L2ERC721Bridge.address)
await L2Token.connect(signer).mint(aliceAddress, TOKEN_ID)
}) })
it('bridgeERC721() reverts when called by non-owner of nft', async () => { it('bridgeERC721() reverts when called by non-owner of nft', async () => {
await expect( await expect(
L2ERC721Bridge.connect(bob).bridgeERC721( L2ERC721Bridge.connect(bob).bridgeERC721(
Mock__L2Token.address, L2Token.address,
DUMMY_L1ERC721_ADDRESS, DUMMY_L1ERC721_ADDRESS,
TOKEN_ID, TOKEN_ID,
0, 0,
...@@ -256,7 +259,7 @@ describe('L2ERC721Bridge', () => { ...@@ -256,7 +259,7 @@ describe('L2ERC721Bridge', () => {
it('bridgeERC721() reverts if called by a contract', async () => { it('bridgeERC721() reverts if called by a contract', async () => {
await expect( await expect(
L2ERC721Bridge.connect(l2MessengerImpersonator).bridgeERC721( L2ERC721Bridge.connect(l2MessengerImpersonator).bridgeERC721(
Mock__L2Token.address, L2Token.address,
DUMMY_L1ERC721_ADDRESS, DUMMY_L1ERC721_ADDRESS,
TOKEN_ID, TOKEN_ID,
0, 0,
...@@ -267,12 +270,12 @@ describe('L2ERC721Bridge', () => { ...@@ -267,12 +270,12 @@ describe('L2ERC721Bridge', () => {
it('bridgeERC721() burns and sends the correct withdrawal message', async () => { it('bridgeERC721() burns and sends the correct withdrawal message', async () => {
// Make sure that alice begins as the NFT owner // Make sure that alice begins as the NFT owner
expect(await Mock__L2Token.ownerOf(TOKEN_ID)).to.equal(aliceAddress) expect(await L2Token.ownerOf(TOKEN_ID)).to.equal(aliceAddress)
// Initiates a successful withdrawal. // Initiates a successful withdrawal.
const expectedResult = expect( const expectedResult = expect(
L2ERC721Bridge.connect(alice).bridgeERC721( L2ERC721Bridge.connect(alice).bridgeERC721(
Mock__L2Token.address, L2Token.address,
DUMMY_L1ERC721_ADDRESS, DUMMY_L1ERC721_ADDRESS,
TOKEN_ID, TOKEN_ID,
0, 0,
...@@ -284,7 +287,7 @@ describe('L2ERC721Bridge', () => { ...@@ -284,7 +287,7 @@ describe('L2ERC721Bridge', () => {
await expectedResult.to await expectedResult.to
.emit(L2ERC721Bridge, 'ERC721BridgeInitiated') .emit(L2ERC721Bridge, 'ERC721BridgeInitiated')
.withArgs( .withArgs(
Mock__L2Token.address, L2Token.address,
DUMMY_L1ERC721_ADDRESS, DUMMY_L1ERC721_ADDRESS,
aliceAddress, aliceAddress,
aliceAddress, aliceAddress,
...@@ -295,15 +298,15 @@ describe('L2ERC721Bridge', () => { ...@@ -295,15 +298,15 @@ describe('L2ERC721Bridge', () => {
// A withdrawal also causes a Transfer event to be emitted the L2 ERC721, signifying that the L2 token // A withdrawal also causes a Transfer event to be emitted the L2 ERC721, signifying that the L2 token
// has been burnt. // has been burnt.
await expectedResult.to await expectedResult.to
.emit(Mock__L2Token, 'Transfer') .emit(L2Token, 'Transfer')
.withArgs(aliceAddress, constants.AddressZero, TOKEN_ID) .withArgs(aliceAddress, constants.AddressZero, TOKEN_ID)
// Assert Alice's balance went down // Assert Alice's balance went down
const aliceBalance = await Mock__L2Token.balanceOf(aliceAddress) const aliceBalance = await L2Token.balanceOf(aliceAddress)
expect(aliceBalance).to.equal(ALICE_INITIAL_BALANCE - 1) expect(aliceBalance).to.equal(0)
// Assert that the token isn't owned by anyone // Assert that the token isn't owned by anyone
await expect(Mock__L2Token.ownerOf(TOKEN_ID)).to.be.revertedWith( await expect(L2Token.ownerOf(TOKEN_ID)).to.be.revertedWith(
'ERC721: owner query for nonexistent token' 'ERC721: owner query for nonexistent token'
) )
...@@ -319,7 +322,7 @@ describe('L2ERC721Bridge', () => { ...@@ -319,7 +322,7 @@ describe('L2ERC721Bridge', () => {
'finalizeBridgeERC721', 'finalizeBridgeERC721',
[ [
DUMMY_L1ERC721_ADDRESS, DUMMY_L1ERC721_ADDRESS,
Mock__L2Token.address, L2Token.address,
aliceAddress, aliceAddress,
aliceAddress, aliceAddress,
TOKEN_ID, TOKEN_ID,
...@@ -334,7 +337,7 @@ describe('L2ERC721Bridge', () => { ...@@ -334,7 +337,7 @@ describe('L2ERC721Bridge', () => {
it('bridgeERC721To() reverts when called by non-owner of nft', async () => { it('bridgeERC721To() reverts when called by non-owner of nft', async () => {
await expect( await expect(
L2ERC721Bridge.connect(bob).bridgeERC721To( L2ERC721Bridge.connect(bob).bridgeERC721To(
Mock__L2Token.address, L2Token.address,
DUMMY_L1ERC721_ADDRESS, DUMMY_L1ERC721_ADDRESS,
bobsAddress, bobsAddress,
TOKEN_ID, TOKEN_ID,
...@@ -346,12 +349,12 @@ describe('L2ERC721Bridge', () => { ...@@ -346,12 +349,12 @@ describe('L2ERC721Bridge', () => {
it('bridgeERC721To() burns and sends the correct withdrawal message', async () => { it('bridgeERC721To() burns and sends the correct withdrawal message', async () => {
// Make sure that alice begins as the NFT owner // Make sure that alice begins as the NFT owner
expect(await Mock__L2Token.ownerOf(TOKEN_ID)).to.equal(aliceAddress) expect(await L2Token.ownerOf(TOKEN_ID)).to.equal(aliceAddress)
// Initiates a successful withdrawal. // Initiates a successful withdrawal.
const expectedResult = expect( const expectedResult = expect(
L2ERC721Bridge.connect(alice).bridgeERC721To( L2ERC721Bridge.connect(alice).bridgeERC721To(
Mock__L2Token.address, L2Token.address,
DUMMY_L1ERC721_ADDRESS, DUMMY_L1ERC721_ADDRESS,
bobsAddress, bobsAddress,
TOKEN_ID, TOKEN_ID,
...@@ -364,7 +367,7 @@ describe('L2ERC721Bridge', () => { ...@@ -364,7 +367,7 @@ describe('L2ERC721Bridge', () => {
await expectedResult.to await expectedResult.to
.emit(L2ERC721Bridge, 'ERC721BridgeInitiated') .emit(L2ERC721Bridge, 'ERC721BridgeInitiated')
.withArgs( .withArgs(
Mock__L2Token.address, L2Token.address,
DUMMY_L1ERC721_ADDRESS, DUMMY_L1ERC721_ADDRESS,
aliceAddress, aliceAddress,
bobsAddress, bobsAddress,
...@@ -375,15 +378,15 @@ describe('L2ERC721Bridge', () => { ...@@ -375,15 +378,15 @@ describe('L2ERC721Bridge', () => {
// A withdrawal also causes a Transfer event to be emitted the L2 ERC721, signifying that the L2 token // A withdrawal also causes a Transfer event to be emitted the L2 ERC721, signifying that the L2 token
// has been burnt. // has been burnt.
await expectedResult.to await expectedResult.to
.emit(Mock__L2Token, 'Transfer') .emit(L2Token, 'Transfer')
.withArgs(aliceAddress, constants.AddressZero, TOKEN_ID) .withArgs(aliceAddress, constants.AddressZero, TOKEN_ID)
// Assert Alice's balance went down // Assert Alice's balance went down
const aliceBalance = await Mock__L2Token.balanceOf(aliceAddress) const aliceBalance = await L2Token.balanceOf(aliceAddress)
expect(aliceBalance).to.equal(ALICE_INITIAL_BALANCE - 1) expect(aliceBalance).to.equal(0)
// Assert that the token isn't owned by anyone // Assert that the token isn't owned by anyone
await expect(Mock__L2Token.ownerOf(TOKEN_ID)).to.be.revertedWith( await expect(L2Token.ownerOf(TOKEN_ID)).to.be.revertedWith(
'ERC721: owner query for nonexistent token' 'ERC721: owner query for nonexistent token'
) )
...@@ -399,7 +402,7 @@ describe('L2ERC721Bridge', () => { ...@@ -399,7 +402,7 @@ describe('L2ERC721Bridge', () => {
'finalizeBridgeERC721', 'finalizeBridgeERC721',
[ [
DUMMY_L1ERC721_ADDRESS, DUMMY_L1ERC721_ADDRESS,
Mock__L2Token.address, L2Token.address,
aliceAddress, aliceAddress,
bobsAddress, bobsAddress,
TOKEN_ID, TOKEN_ID,
......
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment