From 8e34f3a121972191bacf75cb617633ec658b6c32 Mon Sep 17 00:00:00 2001 From: ashwinYardi Date: Tue, 1 Mar 2022 19:37:25 +0530 Subject: [PATCH 1/4] add ERC721 and ERC1155 receiver support in Governor, Timelock and MinimalForwarder (#3174) --- contracts/governance/Governor.sol | 55 +++++++++++++++ contracts/governance/TimelockController.sol | 53 ++++++++++++++ contracts/metatx/MinimalForwarder.sol | 53 ++++++++++++++ test/governance/Governor.test.js | 67 ++++++++++++++++++ test/governance/TimelockController.test.js | 76 +++++++++++++++++++- test/metatx/MinimalForwarder.test.js | 77 ++++++++++++++++++++- 6 files changed, 379 insertions(+), 2 deletions(-) diff --git a/contracts/governance/Governor.sol b/contracts/governance/Governor.sol index 0da3f11706a..97eeab16b51 100644 --- a/contracts/governance/Governor.sol +++ b/contracts/governance/Governor.sol @@ -438,4 +438,59 @@ abstract contract Governor is Context, ERC165, EIP712, IGovernor { function _executor() internal view virtual returns (address) { return address(this); } + + + // Hooks + // IMPORTANT: When inheriting this contract, you must include a way to use the received tokens, otherwise they will be stuck. + + /** + * @dev See {IERC721Receiver-onERC721Received}. + * + * Always returns `onERC721Received.selector`. + * The reason for adding this hook here is to enable Governor contract to accept ERC-721 tokens if required after + * proposal has been executed. + */ + function onERC721Received( + address, + address, + uint256, + bytes memory + ) public virtual returns (bytes4) { + return this.onERC721Received.selector; + } + + /** + * @dev See {IERC1155Receiver-onERC1155Received}. + * + * Always returns `onERC1155Received.selector`. + * The reason for adding this hook here is to enable Governor contract to accept ERC-1155 tokens if required after + * proposal has been executed. + */ + function onERC1155Received( + address, + address, + uint256, + uint256, + bytes memory + ) public virtual returns (bytes4) { + return this.onERC1155Received.selector; + } + + /** + * @dev See {IERC1155Receiver-onERC1155BatchReceived}. + * + * Always returns `onERC1155BatchReceived.selector`. + * The reason for adding this hook here is to enable Governor contract to accept batch ERC-1155 tokens if required after + * proposal has been executed. + */ + function onERC1155BatchReceived( + address, + address, + uint256[] memory, + uint256[] memory, + bytes memory + ) public virtual returns (bytes4) { + return this.onERC1155BatchReceived.selector; + } + } diff --git a/contracts/governance/TimelockController.sol b/contracts/governance/TimelockController.sol index c375f074456..3160d768083 100644 --- a/contracts/governance/TimelockController.sol +++ b/contracts/governance/TimelockController.sol @@ -353,4 +353,57 @@ contract TimelockController is AccessControl { emit MinDelayChange(_minDelay, newDelay); _minDelay = newDelay; } + + // Hooks + + /** + * @dev See {IERC721Receiver-onERC721Received}. + * + * Always returns `onERC721Received.selector`. + * The reason for adding this hook here is to enable Timelock contract to accept ERC-721 tokens if required after + * proposal has been executed. + */ + function onERC721Received( + address, + address, + uint256, + bytes memory + ) public virtual returns (bytes4) { + return this.onERC721Received.selector; + } + + /** + * @dev See {IERC1155Receiver-onERC1155Received}. + * + * Always returns `onERC1155Received.selector`. + * The reason for adding this hook here is to enable Timelock contract to accept ERC-1155 tokens if required after + * proposal has been executed. + */ + function onERC1155Received( + address, + address, + uint256, + uint256, + bytes memory + ) public virtual returns (bytes4) { + return this.onERC1155Received.selector; + } + + /** + * @dev See {IERC1155Receiver-onERC1155BatchReceived}. + * + * Always returns `onERC1155BatchReceived.selector`. + * The reason for adding this hook here is to enable Timelock contract to accept batch ERC-1155 tokens if required after + * proposal has been executed. + */ + function onERC1155BatchReceived( + address, + address, + uint256[] memory, + uint256[] memory, + bytes memory + ) public virtual returns (bytes4) { + return this.onERC1155BatchReceived.selector; + } + } diff --git a/contracts/metatx/MinimalForwarder.sol b/contracts/metatx/MinimalForwarder.sol index a7a1899f4d4..a8946b41be6 100644 --- a/contracts/metatx/MinimalForwarder.sol +++ b/contracts/metatx/MinimalForwarder.sol @@ -64,4 +64,57 @@ contract MinimalForwarder is EIP712 { return (success, returndata); } + + // Hooks + + /** + * @dev See {IERC721Receiver-onERC721Received}. + * + * Always returns `onERC721Received.selector`. + * The reason for adding this hook here is to enable Forwarder contract to accept ERC-721 tokens if required after + * proposal has been executed. + */ + function onERC721Received( + address, + address, + uint256, + bytes memory + ) public virtual returns (bytes4) { + return this.onERC721Received.selector; + } + + /** + * @dev See {IERC1155Receiver-onERC1155Received}. + * + * Always returns `onERC1155Received.selector`. + * The reason for adding this hook here is to enable Forwarder contract to accept ERC-1155 tokens if required after + * proposal has been executed. + */ + function onERC1155Received( + address, + address, + uint256, + uint256, + bytes memory + ) public virtual returns (bytes4) { + return this.onERC1155Received.selector; + } + + /** + * @dev See {IERC1155Receiver-onERC1155BatchReceived}. + * + * Always returns `onERC1155BatchReceived.selector`. + * The reason for adding this hook here is to enable Forwarder contract to accept batch ERC-1155 tokens if required after + * proposal has been executed. + */ + function onERC1155BatchReceived( + address, + address, + uint256[] memory, + uint256[] memory, + bytes memory + ) public virtual returns (bytes4) { + return this.onERC1155BatchReceived.selector; + } + } diff --git a/test/governance/Governor.test.js b/test/governance/Governor.test.js index 6017db0507b..05c36a918d0 100644 --- a/test/governance/Governor.test.js +++ b/test/governance/Governor.test.js @@ -16,6 +16,8 @@ const { const Token = artifacts.require('ERC20VotesMock'); const Governor = artifacts.require('GovernorMock'); const CallReceiver = artifacts.require('CallReceiverMock'); +const ERC721Mock = artifacts.require('ERC721Mock'); +const ERC1155Mock = artifacts.require('ERC1155Mock'); contract('Governor', function (accounts) { const [ owner, proposer, voter1, voter2, voter3, voter4 ] = accounts; @@ -943,4 +945,69 @@ contract('Governor', function (accounts) { }); }); }); + + describe('onERC721Received', function () { + const name = 'Non Fungible Token'; + const symbol = 'NFT'; + + it('receives an ERC721 token', async function () { + const token = await ERC721Mock.new(name, symbol); + const tokenId = new BN(1); + await token.mint(owner, tokenId); + + await token.safeTransferFrom(owner, this.mock.address, tokenId, { from: owner }); + + expect(await token.ownerOf(tokenId)).to.be.equal(this.mock.address); + }); + }); + + describe('onERC1155Received', function () { + const uri = 'https://token-cdn-domain/{id}.json'; + const multiTokenIds = [new BN(1), new BN(2), new BN(3)]; + const multiTokenAmounts = [new BN(1000), new BN(2000), new BN(3000)]; + const transferData = '0x12345678'; + + beforeEach(async function () { + this.multiToken = await ERC1155Mock.new(uri, { from: owner }); + await this.multiToken.mintBatch(owner, multiTokenIds, multiTokenAmounts, '0x', { from: owner }); + }); + + it('receives ERC1155 tokens from a single ID', async function () { + await this.multiToken.safeTransferFrom( + owner, + this.mock.address, + multiTokenIds[0], + multiTokenAmounts[0], + transferData, + { from: owner }, + ); + + expect(await this.multiToken.balanceOf(this.mock.address, multiTokenIds[0])) + .to.be.bignumber.equal(multiTokenAmounts[0]); + + for (let i = 1; i < multiTokenIds.length; i++) { + expect(await this.multiToken.balanceOf(this.mock.address, multiTokenIds[i])).to.be.bignumber.equal(new BN(0)); + } + }); + + it('receives ERC1155 tokens from a multiple IDs', async function () { + for (let i = 0; i < multiTokenIds.length; i++) { + expect(await this.multiToken.balanceOf(this.mock.address, multiTokenIds[i])).to.be.bignumber.equal(new BN(0)); + }; + + await this.multiToken.safeBatchTransferFrom( + owner, + this.mock.address, + multiTokenIds, + multiTokenAmounts, + transferData, + { from: owner }, + ); + + for (let i = 0; i < multiTokenIds.length; i++) { + expect(await this.multiToken.balanceOf(this.mock.address, multiTokenIds[i])) + .to.be.bignumber.equal(multiTokenAmounts[i]); + } + }); + }); }); diff --git a/test/governance/TimelockController.test.js b/test/governance/TimelockController.test.js index f39b963481d..905dbf22876 100644 --- a/test/governance/TimelockController.test.js +++ b/test/governance/TimelockController.test.js @@ -1,4 +1,4 @@ -const { constants, expectEvent, expectRevert, time } = require('@openzeppelin/test-helpers'); +const { BN, constants, expectEvent, expectRevert, time } = require('@openzeppelin/test-helpers'); const { ZERO_BYTES32 } = constants; const { expect } = require('chai'); @@ -6,6 +6,9 @@ const { expect } = require('chai'); const TimelockController = artifacts.require('TimelockController'); const CallReceiverMock = artifacts.require('CallReceiverMock'); const Implementation2 = artifacts.require('Implementation2'); +const ERC721Mock = artifacts.require('ERC721Mock'); +const ERC1155Mock = artifacts.require('ERC1155Mock'); + const MINDELAY = time.duration.days(1); function genOperation (target, value, data, predecessor, salt) { @@ -1029,4 +1032,75 @@ contract('TimelockController', function (accounts) { expect(await web3.eth.getBalance(this.callreceivermock.address)).to.be.bignumber.equal(web3.utils.toBN(0)); }); }); + + describe('onERC721Received', function () { + const name = 'Non Fungible Token'; + const symbol = 'NFT'; + + it('receives an ERC721 token', async function () { + const token = await ERC721Mock.new(name, symbol); + const tokenId = new BN(1); + await token.mint(other, tokenId); + + await token.safeTransferFrom(other, this.timelock.address, tokenId, { from: other }); + + expect(await token.ownerOf(tokenId)).to.be.equal(this.timelock.address); + }); + }); + + describe('onERC1155Received', function () { + const uri = 'https://token-cdn-domain/{id}.json'; + const multiTokenIds = [new BN(1), new BN(2), new BN(3)]; + const multiTokenAmounts = [new BN(1000), new BN(2000), new BN(3000)]; + const transferData = '0x12345678'; + + beforeEach(async function () { + this.multiToken = await ERC1155Mock.new(uri, { from: other }); + await this.multiToken.mintBatch(other, multiTokenIds, multiTokenAmounts, '0x', { from: other }); + }); + + it('receives ERC1155 tokens from a single ID', async function () { + await this.multiToken.safeTransferFrom( + other, + this.timelock.address, + multiTokenIds[0], + multiTokenAmounts[0], + transferData, + { from: other }, + ); + + expect(await this.multiToken.balanceOf(this.timelock.address, multiTokenIds[0])).to.be.bignumber.equal( + multiTokenAmounts[0], + ); + + for (let i = 1; i < multiTokenIds.length; i++) { + expect(await this.multiToken.balanceOf(this.timelock.address, multiTokenIds[i])).to.be.bignumber.equal( + new BN(0), + ); + } + }); + + it('receives ERC1155 tokens from a multiple IDs', async function () { + for (let i = 0; i < multiTokenIds.length; i++) { + expect(await this.multiToken.balanceOf(this.timelock.address, multiTokenIds[i])).to.be.bignumber.equal( + new BN(0), + ); + } + + await this.multiToken.safeBatchTransferFrom( + other, + this.timelock.address, + multiTokenIds, + multiTokenAmounts, + transferData, + { from: other }, + ); + + for (let i = 0; i < multiTokenIds.length; i++) { + expect(await this.multiToken.balanceOf(this.timelock.address, multiTokenIds[i])).to.be.bignumber.equal( + multiTokenAmounts[i], + ); + } + }); + }); }); diff --git a/test/metatx/MinimalForwarder.test.js b/test/metatx/MinimalForwarder.test.js index b8984e431e3..8404aeb76b7 100644 --- a/test/metatx/MinimalForwarder.test.js +++ b/test/metatx/MinimalForwarder.test.js @@ -2,11 +2,13 @@ const ethSigUtil = require('eth-sig-util'); const Wallet = require('ethereumjs-wallet').default; const { EIP712Domain } = require('../helpers/eip712'); -const { expectRevert, constants } = require('@openzeppelin/test-helpers'); +const { BN, expectRevert, constants } = require('@openzeppelin/test-helpers'); const { expect } = require('chai'); const MinimalForwarder = artifacts.require('MinimalForwarder'); const CallReceiverMock = artifacts.require('CallReceiverMock'); +const ERC721Mock = artifacts.require('ERC721Mock'); +const ERC1155Mock = artifacts.require('ERC1155Mock'); const name = 'MinimalForwarder'; const version = '0.0.1'; @@ -181,4 +183,77 @@ contract('MinimalForwarder', function (accounts) { }); }); }); + + describe('onERC721Received', function () { + const owner = accounts[0]; + const name = 'Non Fungible Token'; + const symbol = 'NFT'; + + it('receives an ERC721 token', async function () { + const token = await ERC721Mock.new(name, symbol); + const tokenId = new BN(1); + await token.mint(owner, tokenId); + + await token.safeTransferFrom(owner, this.forwarder.address, tokenId, { from: owner }); + + expect(await token.ownerOf(tokenId)).to.be.equal(this.forwarder.address); + }); + }); + + describe('onERC1155Received', function () { + const owner = accounts[0]; + const uri = 'https://token-cdn-domain/{id}.json'; + const multiTokenIds = [new BN(1), new BN(2), new BN(3)]; + const multiTokenAmounts = [new BN(1000), new BN(2000), new BN(3000)]; + const transferData = '0x12345678'; + + beforeEach(async function () { + this.multiToken = await ERC1155Mock.new(uri, { from: owner }); + await this.multiToken.mintBatch(owner, multiTokenIds, multiTokenAmounts, '0x', { from: owner }); + }); + + it('receives ERC1155 tokens from a single ID', async function () { + await this.multiToken.safeTransferFrom( + owner, + this.forwarder.address, + multiTokenIds[0], + multiTokenAmounts[0], + transferData, + { from: owner }, + ); + + expect(await this.multiToken.balanceOf(this.forwarder.address, multiTokenIds[0])).to.be.bignumber.equal( + multiTokenAmounts[0], + ); + + for (let i = 1; i < multiTokenIds.length; i++) { + expect(await this.multiToken.balanceOf(this.forwarder.address, multiTokenIds[i])).to.be.bignumber.equal( + new BN(0), + ); + } + }); + + it('receives ERC1155 tokens from a multiple IDs', async function () { + for (let i = 0; i < multiTokenIds.length; i++) { + expect(await this.multiToken.balanceOf(this.forwarder.address, multiTokenIds[i])).to.be.bignumber.equal( + new BN(0), + ); + } + + await this.multiToken.safeBatchTransferFrom( + owner, + this.forwarder.address, + multiTokenIds, + multiTokenAmounts, + transferData, + { from: owner }, + ); + + for (let i = 0; i < multiTokenIds.length; i++) { + expect(await this.multiToken.balanceOf(this.forwarder.address, multiTokenIds[i])).to.be.bignumber.equal( + multiTokenAmounts[i], + ); + } + }); + }); }); From c341a58b6bb05a49bcdef4dec22a6e70f60a71cf Mon Sep 17 00:00:00 2001 From: ashwinYardi Date: Wed, 2 Mar 2022 14:00:23 +0530 Subject: [PATCH 2/4] revert the nft receiver hooks from MinimalForwarder and linting updates --- contracts/governance/Governor.sol | 8 +-- contracts/governance/TimelockController.sol | 5 +- contracts/metatx/MinimalForwarder.sol | 53 -------------- test/metatx/MinimalForwarder.test.js | 77 +-------------------- 4 files changed, 6 insertions(+), 137 deletions(-) diff --git a/contracts/governance/Governor.sol b/contracts/governance/Governor.sol index 97eeab16b51..07879f9c560 100644 --- a/contracts/governance/Governor.sol +++ b/contracts/governance/Governor.sol @@ -439,11 +439,10 @@ abstract contract Governor is Context, ERC165, EIP712, IGovernor { return address(this); } - // Hooks // IMPORTANT: When inheriting this contract, you must include a way to use the received tokens, otherwise they will be stuck. - - /** + + /** * @dev See {IERC721Receiver-onERC721Received}. * * Always returns `onERC721Received.selector`. @@ -458,7 +457,7 @@ abstract contract Governor is Context, ERC165, EIP712, IGovernor { ) public virtual returns (bytes4) { return this.onERC721Received.selector; } - + /** * @dev See {IERC1155Receiver-onERC1155Received}. * @@ -492,5 +491,4 @@ abstract contract Governor is Context, ERC165, EIP712, IGovernor { ) public virtual returns (bytes4) { return this.onERC1155BatchReceived.selector; } - } diff --git a/contracts/governance/TimelockController.sol b/contracts/governance/TimelockController.sol index 3160d768083..f15e23488b0 100644 --- a/contracts/governance/TimelockController.sol +++ b/contracts/governance/TimelockController.sol @@ -355,8 +355,8 @@ contract TimelockController is AccessControl { } // Hooks - - /** + + /** * @dev See {IERC721Receiver-onERC721Received}. * * Always returns `onERC721Received.selector`. @@ -405,5 +405,4 @@ contract TimelockController is AccessControl { ) public virtual returns (bytes4) { return this.onERC1155BatchReceived.selector; } - } diff --git a/contracts/metatx/MinimalForwarder.sol b/contracts/metatx/MinimalForwarder.sol index a8946b41be6..a7a1899f4d4 100644 --- a/contracts/metatx/MinimalForwarder.sol +++ b/contracts/metatx/MinimalForwarder.sol @@ -64,57 +64,4 @@ contract MinimalForwarder is EIP712 { return (success, returndata); } - - // Hooks - - /** - * @dev See {IERC721Receiver-onERC721Received}. - * - * Always returns `onERC721Received.selector`. - * The reason for adding this hook here is to enable Forwarder contract to accept ERC-721 tokens if required after - * proposal has been executed. - */ - function onERC721Received( - address, - address, - uint256, - bytes memory - ) public virtual returns (bytes4) { - return this.onERC721Received.selector; - } - - /** - * @dev See {IERC1155Receiver-onERC1155Received}. - * - * Always returns `onERC1155Received.selector`. - * The reason for adding this hook here is to enable Forwarder contract to accept ERC-1155 tokens if required after - * proposal has been executed. - */ - function onERC1155Received( - address, - address, - uint256, - uint256, - bytes memory - ) public virtual returns (bytes4) { - return this.onERC1155Received.selector; - } - - /** - * @dev See {IERC1155Receiver-onERC1155BatchReceived}. - * - * Always returns `onERC1155BatchReceived.selector`. - * The reason for adding this hook here is to enable Forwarder contract to accept batch ERC-1155 tokens if required after - * proposal has been executed. - */ - function onERC1155BatchReceived( - address, - address, - uint256[] memory, - uint256[] memory, - bytes memory - ) public virtual returns (bytes4) { - return this.onERC1155BatchReceived.selector; - } - } diff --git a/test/metatx/MinimalForwarder.test.js b/test/metatx/MinimalForwarder.test.js index 8404aeb76b7..b8984e431e3 100644 --- a/test/metatx/MinimalForwarder.test.js +++ b/test/metatx/MinimalForwarder.test.js @@ -2,13 +2,11 @@ const ethSigUtil = require('eth-sig-util'); const Wallet = require('ethereumjs-wallet').default; const { EIP712Domain } = require('../helpers/eip712'); -const { BN, expectRevert, constants } = require('@openzeppelin/test-helpers'); +const { expectRevert, constants } = require('@openzeppelin/test-helpers'); const { expect } = require('chai'); const MinimalForwarder = artifacts.require('MinimalForwarder'); const CallReceiverMock = artifacts.require('CallReceiverMock'); -const ERC721Mock = artifacts.require('ERC721Mock'); -const ERC1155Mock = artifacts.require('ERC1155Mock'); const name = 'MinimalForwarder'; const version = '0.0.1'; @@ -183,77 +181,4 @@ contract('MinimalForwarder', function (accounts) { }); }); }); - - describe('onERC721Received', function () { - const owner = accounts[0]; - const name = 'Non Fungible Token'; - const symbol = 'NFT'; - - it('receives an ERC721 token', async function () { - const token = await ERC721Mock.new(name, symbol); - const tokenId = new BN(1); - await token.mint(owner, tokenId); - - await token.safeTransferFrom(owner, this.forwarder.address, tokenId, { from: owner }); - - expect(await token.ownerOf(tokenId)).to.be.equal(this.forwarder.address); - }); - }); - - describe('onERC1155Received', function () { - const owner = accounts[0]; - const uri = 'https://token-cdn-domain/{id}.json'; - const multiTokenIds = [new BN(1), new BN(2), new BN(3)]; - const multiTokenAmounts = [new BN(1000), new BN(2000), new BN(3000)]; - const transferData = '0x12345678'; - - beforeEach(async function () { - this.multiToken = await ERC1155Mock.new(uri, { from: owner }); - await this.multiToken.mintBatch(owner, multiTokenIds, multiTokenAmounts, '0x', { from: owner }); - }); - - it('receives ERC1155 tokens from a single ID', async function () { - await this.multiToken.safeTransferFrom( - owner, - this.forwarder.address, - multiTokenIds[0], - multiTokenAmounts[0], - transferData, - { from: owner }, - ); - - expect(await this.multiToken.balanceOf(this.forwarder.address, multiTokenIds[0])).to.be.bignumber.equal( - multiTokenAmounts[0], - ); - - for (let i = 1; i < multiTokenIds.length; i++) { - expect(await this.multiToken.balanceOf(this.forwarder.address, multiTokenIds[i])).to.be.bignumber.equal( - new BN(0), - ); - } - }); - - it('receives ERC1155 tokens from a multiple IDs', async function () { - for (let i = 0; i < multiTokenIds.length; i++) { - expect(await this.multiToken.balanceOf(this.forwarder.address, multiTokenIds[i])).to.be.bignumber.equal( - new BN(0), - ); - } - - await this.multiToken.safeBatchTransferFrom( - owner, - this.forwarder.address, - multiTokenIds, - multiTokenAmounts, - transferData, - { from: owner }, - ); - - for (let i = 0; i < multiTokenIds.length; i++) { - expect(await this.multiToken.balanceOf(this.forwarder.address, multiTokenIds[i])).to.be.bignumber.equal( - multiTokenAmounts[i], - ); - } - }); - }); }); From 5ea382e77ebf2d51dbd4e6f5b43a85abedd86bbf Mon Sep 17 00:00:00 2001 From: Hadrien Croubois Date: Mon, 21 Mar 2022 21:33:54 +0100 Subject: [PATCH 3/4] add ERC165 support & simplify test --- contracts/governance/Governor.sol | 30 +-- contracts/governance/TimelockController.sol | 31 +-- test/governance/Governor.test.js | 96 ++++---- test/governance/TimelockController.test.js | 259 ++++++++++---------- 4 files changed, 189 insertions(+), 227 deletions(-) diff --git a/contracts/governance/Governor.sol b/contracts/governance/Governor.sol index 07879f9c560..fd996f56bf3 100644 --- a/contracts/governance/Governor.sol +++ b/contracts/governance/Governor.sol @@ -3,6 +3,8 @@ pragma solidity ^0.8.0; +import "../token/ERC721/IERC721Receiver.sol"; +import "../token/ERC1155/IERC1155Receiver.sol"; import "../utils/cryptography/ECDSA.sol"; import "../utils/cryptography/draft-EIP712.sol"; import "../utils/introspection/ERC165.sol"; @@ -24,7 +26,7 @@ import "./IGovernor.sol"; * * _Available since v4.3._ */ -abstract contract Governor is Context, ERC165, EIP712, IGovernor { +abstract contract Governor is Context, ERC165, EIP712, IGovernor, IERC721Receiver, IERC1155Receiver { using DoubleEndedQueue for DoubleEndedQueue.Bytes32Deque; using SafeCast for uint256; using Timers for Timers.BlockNumber; @@ -86,7 +88,10 @@ abstract contract Governor is Context, ERC165, EIP712, IGovernor { * @dev See {IERC165-supportsInterface}. */ function supportsInterface(bytes4 interfaceId) public view virtual override(IERC165, ERC165) returns (bool) { - return interfaceId == type(IGovernor).interfaceId || super.supportsInterface(interfaceId); + return + interfaceId == type(IGovernor).interfaceId || + interfaceId == type(IERC1155Receiver).interfaceId || + super.supportsInterface(interfaceId); } /** @@ -439,31 +444,20 @@ abstract contract Governor is Context, ERC165, EIP712, IGovernor { return address(this); } - // Hooks - // IMPORTANT: When inheriting this contract, you must include a way to use the received tokens, otherwise they will be stuck. - /** * @dev See {IERC721Receiver-onERC721Received}. - * - * Always returns `onERC721Received.selector`. - * The reason for adding this hook here is to enable Governor contract to accept ERC-721 tokens if required after - * proposal has been executed. */ function onERC721Received( address, address, uint256, bytes memory - ) public virtual returns (bytes4) { + ) public virtual override returns (bytes4) { return this.onERC721Received.selector; } /** * @dev See {IERC1155Receiver-onERC1155Received}. - * - * Always returns `onERC1155Received.selector`. - * The reason for adding this hook here is to enable Governor contract to accept ERC-1155 tokens if required after - * proposal has been executed. */ function onERC1155Received( address, @@ -471,16 +465,12 @@ abstract contract Governor is Context, ERC165, EIP712, IGovernor { uint256, uint256, bytes memory - ) public virtual returns (bytes4) { + ) public virtual override returns (bytes4) { return this.onERC1155Received.selector; } /** * @dev See {IERC1155Receiver-onERC1155BatchReceived}. - * - * Always returns `onERC1155BatchReceived.selector`. - * The reason for adding this hook here is to enable Governor contract to accept batch ERC-1155 tokens if required after - * proposal has been executed. */ function onERC1155BatchReceived( address, @@ -488,7 +478,7 @@ abstract contract Governor is Context, ERC165, EIP712, IGovernor { uint256[] memory, uint256[] memory, bytes memory - ) public virtual returns (bytes4) { + ) public virtual override returns (bytes4) { return this.onERC1155BatchReceived.selector; } } diff --git a/contracts/governance/TimelockController.sol b/contracts/governance/TimelockController.sol index f15e23488b0..7140792d572 100644 --- a/contracts/governance/TimelockController.sol +++ b/contracts/governance/TimelockController.sol @@ -4,6 +4,8 @@ pragma solidity ^0.8.0; import "../access/AccessControl.sol"; +import "../token/ERC721/IERC721Receiver.sol"; +import "../token/ERC1155/IERC1155Receiver.sol"; /** * @dev Contract module which acts as a timelocked controller. When set as the @@ -20,7 +22,7 @@ import "../access/AccessControl.sol"; * * _Available since v3.3._ */ -contract TimelockController is AccessControl { +contract TimelockController is AccessControl, IERC721Receiver, IERC1155Receiver { bytes32 public constant TIMELOCK_ADMIN_ROLE = keccak256("TIMELOCK_ADMIN_ROLE"); bytes32 public constant PROPOSER_ROLE = keccak256("PROPOSER_ROLE"); bytes32 public constant EXECUTOR_ROLE = keccak256("EXECUTOR_ROLE"); @@ -105,6 +107,13 @@ contract TimelockController is AccessControl { */ receive() external payable {} + /** + * @dev See {IERC165-supportsInterface}. + */ + function supportsInterface(bytes4 interfaceId) public view virtual override(IERC165, AccessControl) returns (bool) { + return interfaceId == type(IERC1155Receiver).interfaceId || super.supportsInterface(interfaceId); + } + /** * @dev Returns whether an id correspond to a registered operation. This * includes both Pending, Ready and Done operations. @@ -354,30 +363,20 @@ contract TimelockController is AccessControl { _minDelay = newDelay; } - // Hooks - /** * @dev See {IERC721Receiver-onERC721Received}. - * - * Always returns `onERC721Received.selector`. - * The reason for adding this hook here is to enable Timelock contract to accept ERC-721 tokens if required after - * proposal has been executed. */ function onERC721Received( address, address, uint256, bytes memory - ) public virtual returns (bytes4) { + ) public virtual override returns (bytes4) { return this.onERC721Received.selector; } /** * @dev See {IERC1155Receiver-onERC1155Received}. - * - * Always returns `onERC1155Received.selector`. - * The reason for adding this hook here is to enable Timelock contract to accept ERC-1155 tokens if required after - * proposal has been executed. */ function onERC1155Received( address, @@ -385,16 +384,12 @@ contract TimelockController is AccessControl { uint256, uint256, bytes memory - ) public virtual returns (bytes4) { + ) public virtual override returns (bytes4) { return this.onERC1155Received.selector; } /** * @dev See {IERC1155Receiver-onERC1155BatchReceived}. - * - * Always returns `onERC1155BatchReceived.selector`. - * The reason for adding this hook here is to enable Timelock contract to accept batch ERC-1155 tokens if required after - * proposal has been executed. */ function onERC1155BatchReceived( address, @@ -402,7 +397,7 @@ contract TimelockController is AccessControl { uint256[] memory, uint256[] memory, bytes memory - ) public virtual returns (bytes4) { + ) public virtual override returns (bytes4) { return this.onERC1155BatchReceived.selector; } } diff --git a/test/governance/Governor.test.js b/test/governance/Governor.test.js index 05c36a918d0..c19fed5878a 100644 --- a/test/governance/Governor.test.js +++ b/test/governance/Governor.test.js @@ -42,6 +42,7 @@ contract('Governor', function (accounts) { shouldSupportInterfaces([ 'ERC165', + 'ERC1155Receiver', 'Governor', ]); @@ -946,68 +947,55 @@ contract('Governor', function (accounts) { }); }); - describe('onERC721Received', function () { - const name = 'Non Fungible Token'; - const symbol = 'NFT'; - - it('receives an ERC721 token', async function () { - const token = await ERC721Mock.new(name, symbol); + describe('safe receive', function () { + describe('ERC721', function () { + const name = 'Non Fungible Token'; + const symbol = 'NFT'; const tokenId = new BN(1); - await token.mint(owner, tokenId); - - await token.safeTransferFrom(owner, this.mock.address, tokenId, { from: owner }); - - expect(await token.ownerOf(tokenId)).to.be.equal(this.mock.address); - }); - }); - describe('onERC1155Received', function () { - const uri = 'https://token-cdn-domain/{id}.json'; - const multiTokenIds = [new BN(1), new BN(2), new BN(3)]; - const multiTokenAmounts = [new BN(1000), new BN(2000), new BN(3000)]; - const transferData = '0x12345678'; - - beforeEach(async function () { - this.multiToken = await ERC1155Mock.new(uri, { from: owner }); - await this.multiToken.mintBatch(owner, multiTokenIds, multiTokenAmounts, '0x', { from: owner }); - }); - - it('receives ERC1155 tokens from a single ID', async function () { - await this.multiToken.safeTransferFrom( - owner, - this.mock.address, - multiTokenIds[0], - multiTokenAmounts[0], - transferData, - { from: owner }, - ); - - expect(await this.multiToken.balanceOf(this.mock.address, multiTokenIds[0])) - .to.be.bignumber.equal(multiTokenAmounts[0]); + beforeEach(async function () { + this.token = await ERC721Mock.new(name, symbol); + await this.token.mint(owner, tokenId); + }); - for (let i = 1; i < multiTokenIds.length; i++) { - expect(await this.multiToken.balanceOf(this.mock.address, multiTokenIds[i])).to.be.bignumber.equal(new BN(0)); - } + it('can receive an ERC721 safeTransfer', async function () { + await this.token.safeTransferFrom(owner, this.mock.address, tokenId, { from: owner }); + }); }); - it('receives ERC1155 tokens from a multiple IDs', async function () { - for (let i = 0; i < multiTokenIds.length; i++) { - expect(await this.multiToken.balanceOf(this.mock.address, multiTokenIds[i])).to.be.bignumber.equal(new BN(0)); + describe('ERC1155', function () { + const uri = 'https://token-cdn-domain/{id}.json'; + const tokenIds = { + 1: new BN(1000), + 2: new BN(2000), + 3: new BN(3000), }; - await this.multiToken.safeBatchTransferFrom( - owner, - this.mock.address, - multiTokenIds, - multiTokenAmounts, - transferData, - { from: owner }, - ); + beforeEach(async function () { + this.token = await ERC1155Mock.new(uri); + await this.token.mintBatch(owner, Object.keys(tokenIds), Object.values(tokenIds), '0x'); + }); - for (let i = 0; i < multiTokenIds.length; i++) { - expect(await this.multiToken.balanceOf(this.mock.address, multiTokenIds[i])) - .to.be.bignumber.equal(multiTokenAmounts[i]); - } + it('can receive ERC1155 safeTransfer', async function () { + await this.token.safeTransferFrom( + owner, + this.mock.address, + ...Object.entries(tokenIds)[0], // id + amount + '0x', + { from: owner }, + ); + }); + + it('can receive ERC1155 safeBatchTransfer', async function () { + await this.token.safeBatchTransferFrom( + owner, + this.mock.address, + Object.keys(tokenIds), + Object.values(tokenIds), + '0x', + { from: owner }, + ); + }); }); }); }); diff --git a/test/governance/TimelockController.test.js b/test/governance/TimelockController.test.js index 905dbf22876..752c75ef700 100644 --- a/test/governance/TimelockController.test.js +++ b/test/governance/TimelockController.test.js @@ -3,6 +3,10 @@ const { ZERO_BYTES32 } = constants; const { expect } = require('chai'); +const { + shouldSupportInterfaces, +} = require('../utils/introspection/SupportsInterface.behavior'); + const TimelockController = artifacts.require('TimelockController'); const CallReceiverMock = artifacts.require('CallReceiverMock'); const Implementation2 = artifacts.require('Implementation2'); @@ -50,22 +54,26 @@ contract('TimelockController', function (accounts) { beforeEach(async function () { // Deploy new timelock - this.timelock = await TimelockController.new( + this.mock = await TimelockController.new( MINDELAY, [ proposer ], [ executor ], { from: admin }, ); - this.TIMELOCK_ADMIN_ROLE = await this.timelock.TIMELOCK_ADMIN_ROLE(); - this.PROPOSER_ROLE = await this.timelock.PROPOSER_ROLE(); - this.EXECUTOR_ROLE = await this.timelock.EXECUTOR_ROLE(); + this.TIMELOCK_ADMIN_ROLE = await this.mock.TIMELOCK_ADMIN_ROLE(); + this.PROPOSER_ROLE = await this.mock.PROPOSER_ROLE(); + this.EXECUTOR_ROLE = await this.mock.EXECUTOR_ROLE(); // Mocks this.callreceivermock = await CallReceiverMock.new({ from: admin }); this.implementation2 = await Implementation2.new({ from: admin }); }); + shouldSupportInterfaces([ + 'ERC1155Receiver', + ]); + it('initial state', async function () { - expect(await this.timelock.getMinDelay()).to.be.bignumber.equal(MINDELAY); + expect(await this.mock.getMinDelay()).to.be.bignumber.equal(MINDELAY); }); describe('methods', function () { @@ -78,7 +86,7 @@ contract('TimelockController', function (accounts) { '0xba41db3be0a9929145cfe480bd0f1f003689104d275ae912099f925df424ef94', '0x60d9109846ab510ed75c15f979ae366a8a2ace11d34ba9788c13ac296db50e6e', ); - expect(await this.timelock.hashOperation( + expect(await this.mock.hashOperation( this.operation.target, this.operation.value, this.operation.data, @@ -95,7 +103,7 @@ contract('TimelockController', function (accounts) { '0xce8f45069cc71d25f71ba05062de1a3974f9849b004de64a70998bca9d29c2e7', '0x8952d74c110f72bfe5accdf828c74d53a7dfb71235dfa8a1e8c75d8576b372ff', ); - expect(await this.timelock.hashOperationBatch( + expect(await this.mock.hashOperationBatch( this.operation.targets, this.operation.values, this.operation.datas, @@ -117,7 +125,7 @@ contract('TimelockController', function (accounts) { }); it('proposer can schedule', async function () { - const receipt = await this.timelock.schedule( + const receipt = await this.mock.schedule( this.operation.target, this.operation.value, this.operation.data, @@ -138,12 +146,12 @@ contract('TimelockController', function (accounts) { const block = await web3.eth.getBlock(receipt.receipt.blockHash); - expect(await this.timelock.getTimestamp(this.operation.id)) + expect(await this.mock.getTimestamp(this.operation.id)) .to.be.bignumber.equal(web3.utils.toBN(block.timestamp).add(MINDELAY)); }); it('prevent overwritting active operation', async function () { - await this.timelock.schedule( + await this.mock.schedule( this.operation.target, this.operation.value, this.operation.data, @@ -154,7 +162,7 @@ contract('TimelockController', function (accounts) { ); await expectRevert( - this.timelock.schedule( + this.mock.schedule( this.operation.target, this.operation.value, this.operation.data, @@ -169,7 +177,7 @@ contract('TimelockController', function (accounts) { it('prevent non-proposer from commiting', async function () { await expectRevert( - this.timelock.schedule( + this.mock.schedule( this.operation.target, this.operation.value, this.operation.data, @@ -184,7 +192,7 @@ contract('TimelockController', function (accounts) { it('enforce minimum delay', async function () { await expectRevert( - this.timelock.schedule( + this.mock.schedule( this.operation.target, this.operation.value, this.operation.data, @@ -211,7 +219,7 @@ contract('TimelockController', function (accounts) { it('revert if operation is not scheduled', async function () { await expectRevert( - this.timelock.execute( + this.mock.execute( this.operation.target, this.operation.value, this.operation.data, @@ -225,7 +233,7 @@ contract('TimelockController', function (accounts) { describe('with scheduled operation', function () { beforeEach(async function () { - ({ receipt: this.receipt, logs: this.logs } = await this.timelock.schedule( + ({ receipt: this.receipt, logs: this.logs } = await this.mock.schedule( this.operation.target, this.operation.value, this.operation.data, @@ -238,7 +246,7 @@ contract('TimelockController', function (accounts) { it('revert if execution comes too early 1/2', async function () { await expectRevert( - this.timelock.execute( + this.mock.execute( this.operation.target, this.operation.value, this.operation.data, @@ -251,11 +259,11 @@ contract('TimelockController', function (accounts) { }); it('revert if execution comes too early 2/2', async function () { - const timestamp = await this.timelock.getTimestamp(this.operation.id); + const timestamp = await this.mock.getTimestamp(this.operation.id); await time.increaseTo(timestamp - 5); // -1 is too tight, test sometime fails await expectRevert( - this.timelock.execute( + this.mock.execute( this.operation.target, this.operation.value, this.operation.data, @@ -269,12 +277,12 @@ contract('TimelockController', function (accounts) { describe('on time', function () { beforeEach(async function () { - const timestamp = await this.timelock.getTimestamp(this.operation.id); + const timestamp = await this.mock.getTimestamp(this.operation.id); await time.increaseTo(timestamp); }); it('executor can reveal', async function () { - const receipt = await this.timelock.execute( + const receipt = await this.mock.execute( this.operation.target, this.operation.value, this.operation.data, @@ -293,7 +301,7 @@ contract('TimelockController', function (accounts) { it('prevent non-executor from revealing', async function () { await expectRevert( - this.timelock.execute( + this.mock.execute( this.operation.target, this.operation.value, this.operation.data, @@ -322,7 +330,7 @@ contract('TimelockController', function (accounts) { }); it('proposer can schedule', async function () { - const receipt = await this.timelock.scheduleBatch( + const receipt = await this.mock.scheduleBatch( this.operation.targets, this.operation.values, this.operation.datas, @@ -345,12 +353,12 @@ contract('TimelockController', function (accounts) { const block = await web3.eth.getBlock(receipt.receipt.blockHash); - expect(await this.timelock.getTimestamp(this.operation.id)) + expect(await this.mock.getTimestamp(this.operation.id)) .to.be.bignumber.equal(web3.utils.toBN(block.timestamp).add(MINDELAY)); }); it('prevent overwritting active operation', async function () { - await this.timelock.scheduleBatch( + await this.mock.scheduleBatch( this.operation.targets, this.operation.values, this.operation.datas, @@ -361,7 +369,7 @@ contract('TimelockController', function (accounts) { ); await expectRevert( - this.timelock.scheduleBatch( + this.mock.scheduleBatch( this.operation.targets, this.operation.values, this.operation.datas, @@ -376,7 +384,7 @@ contract('TimelockController', function (accounts) { it('length of batch parameter must match #1', async function () { await expectRevert( - this.timelock.scheduleBatch( + this.mock.scheduleBatch( this.operation.targets, [], this.operation.datas, @@ -391,7 +399,7 @@ contract('TimelockController', function (accounts) { it('length of batch parameter must match #1', async function () { await expectRevert( - this.timelock.scheduleBatch( + this.mock.scheduleBatch( this.operation.targets, this.operation.values, [], @@ -406,7 +414,7 @@ contract('TimelockController', function (accounts) { it('prevent non-proposer from commiting', async function () { await expectRevert( - this.timelock.scheduleBatch( + this.mock.scheduleBatch( this.operation.targets, this.operation.values, this.operation.datas, @@ -421,7 +429,7 @@ contract('TimelockController', function (accounts) { it('enforce minimum delay', async function () { await expectRevert( - this.timelock.scheduleBatch( + this.mock.scheduleBatch( this.operation.targets, this.operation.values, this.operation.datas, @@ -448,7 +456,7 @@ contract('TimelockController', function (accounts) { it('revert if operation is not scheduled', async function () { await expectRevert( - this.timelock.executeBatch( + this.mock.executeBatch( this.operation.targets, this.operation.values, this.operation.datas, @@ -462,7 +470,7 @@ contract('TimelockController', function (accounts) { describe('with scheduled operation', function () { beforeEach(async function () { - ({ receipt: this.receipt, logs: this.logs } = await this.timelock.scheduleBatch( + ({ receipt: this.receipt, logs: this.logs } = await this.mock.scheduleBatch( this.operation.targets, this.operation.values, this.operation.datas, @@ -475,7 +483,7 @@ contract('TimelockController', function (accounts) { it('revert if execution comes too early 1/2', async function () { await expectRevert( - this.timelock.executeBatch( + this.mock.executeBatch( this.operation.targets, this.operation.values, this.operation.datas, @@ -488,11 +496,11 @@ contract('TimelockController', function (accounts) { }); it('revert if execution comes too early 2/2', async function () { - const timestamp = await this.timelock.getTimestamp(this.operation.id); + const timestamp = await this.mock.getTimestamp(this.operation.id); await time.increaseTo(timestamp - 5); // -1 is to tight, test sometime fails await expectRevert( - this.timelock.executeBatch( + this.mock.executeBatch( this.operation.targets, this.operation.values, this.operation.datas, @@ -506,12 +514,12 @@ contract('TimelockController', function (accounts) { describe('on time', function () { beforeEach(async function () { - const timestamp = await this.timelock.getTimestamp(this.operation.id); + const timestamp = await this.mock.getTimestamp(this.operation.id); await time.increaseTo(timestamp); }); it('executor can reveal', async function () { - const receipt = await this.timelock.executeBatch( + const receipt = await this.mock.executeBatch( this.operation.targets, this.operation.values, this.operation.datas, @@ -532,7 +540,7 @@ contract('TimelockController', function (accounts) { it('prevent non-executor from revealing', async function () { await expectRevert( - this.timelock.executeBatch( + this.mock.executeBatch( this.operation.targets, this.operation.values, this.operation.datas, @@ -546,7 +554,7 @@ contract('TimelockController', function (accounts) { it('length mismatch #1', async function () { await expectRevert( - this.timelock.executeBatch( + this.mock.executeBatch( [], this.operation.values, this.operation.datas, @@ -560,7 +568,7 @@ contract('TimelockController', function (accounts) { it('length mismatch #2', async function () { await expectRevert( - this.timelock.executeBatch( + this.mock.executeBatch( this.operation.targets, [], this.operation.datas, @@ -574,7 +582,7 @@ contract('TimelockController', function (accounts) { it('length mismatch #3', async function () { await expectRevert( - this.timelock.executeBatch( + this.mock.executeBatch( this.operation.targets, this.operation.values, [], @@ -609,7 +617,7 @@ contract('TimelockController', function (accounts) { '0x8ac04aa0d6d66b8812fb41d39638d37af0a9ab11da507afd65c509f8ed079d3e', ); - await this.timelock.scheduleBatch( + await this.mock.scheduleBatch( operation.targets, operation.values, operation.datas, @@ -620,7 +628,7 @@ contract('TimelockController', function (accounts) { ); await time.increase(MINDELAY); await expectRevert( - this.timelock.executeBatch( + this.mock.executeBatch( operation.targets, operation.values, operation.datas, @@ -643,7 +651,7 @@ contract('TimelockController', function (accounts) { ZERO_BYTES32, '0xa2485763600634800df9fc9646fb2c112cf98649c55f63dd1d9c7d13a64399d9', ); - ({ receipt: this.receipt, logs: this.logs } = await this.timelock.schedule( + ({ receipt: this.receipt, logs: this.logs } = await this.mock.schedule( this.operation.target, this.operation.value, this.operation.data, @@ -655,20 +663,20 @@ contract('TimelockController', function (accounts) { }); it('proposer can cancel', async function () { - const receipt = await this.timelock.cancel(this.operation.id, { from: proposer }); + const receipt = await this.mock.cancel(this.operation.id, { from: proposer }); expectEvent(receipt, 'Cancelled', { id: this.operation.id }); }); it('cannot cancel invalid operation', async function () { await expectRevert( - this.timelock.cancel(constants.ZERO_BYTES32, { from: proposer }), + this.mock.cancel(constants.ZERO_BYTES32, { from: proposer }), 'TimelockController: operation cannot be cancelled', ); }); it('prevent non-proposer from canceling', async function () { await expectRevert( - this.timelock.cancel(this.operation.id, { from: other }), + this.mock.cancel(this.operation.id, { from: other }), `AccessControl: account ${other.toLowerCase()} is missing role ${this.PROPOSER_ROLE}`, ); }); @@ -678,7 +686,7 @@ contract('TimelockController', function (accounts) { describe('maintenance', function () { it('prevent unauthorized maintenance', async function () { await expectRevert( - this.timelock.updateDelay(0, { from: other }), + this.mock.updateDelay(0, { from: other }), 'TimelockController: caller must be timelock', ); }); @@ -686,14 +694,14 @@ contract('TimelockController', function (accounts) { it('timelock scheduled maintenance', async function () { const newDelay = time.duration.hours(6); const operation = genOperation( - this.timelock.address, + this.mock.address, 0, - this.timelock.contract.methods.updateDelay(newDelay.toString()).encodeABI(), + this.mock.contract.methods.updateDelay(newDelay.toString()).encodeABI(), ZERO_BYTES32, '0xf8e775b2c5f4d66fb5c7fa800f35ef518c262b6014b3c0aee6ea21bff157f108', ); - await this.timelock.schedule( + await this.mock.schedule( operation.target, operation.value, operation.data, @@ -703,7 +711,7 @@ contract('TimelockController', function (accounts) { { from: proposer }, ); await time.increase(MINDELAY); - const receipt = await this.timelock.execute( + const receipt = await this.mock.execute( operation.target, operation.value, operation.data, @@ -713,7 +721,7 @@ contract('TimelockController', function (accounts) { ); expectEvent(receipt, 'MinDelayChange', { newDuration: newDelay.toString(), oldDuration: MINDELAY }); - expect(await this.timelock.getMinDelay()).to.be.bignumber.equal(newDelay); + expect(await this.mock.getMinDelay()).to.be.bignumber.equal(newDelay); }); }); @@ -733,7 +741,7 @@ contract('TimelockController', function (accounts) { this.operation1.id, '0x036e1311cac523f9548e6461e29fb1f8f9196b91910a41711ea22f5de48df07d', ); - await this.timelock.schedule( + await this.mock.schedule( this.operation1.target, this.operation1.value, this.operation1.data, @@ -742,7 +750,7 @@ contract('TimelockController', function (accounts) { MINDELAY, { from: proposer }, ); - await this.timelock.schedule( + await this.mock.schedule( this.operation2.target, this.operation2.value, this.operation2.data, @@ -756,7 +764,7 @@ contract('TimelockController', function (accounts) { it('cannot execute before dependency', async function () { await expectRevert( - this.timelock.execute( + this.mock.execute( this.operation2.target, this.operation2.value, this.operation2.data, @@ -769,7 +777,7 @@ contract('TimelockController', function (accounts) { }); it('can execute after dependency', async function () { - await this.timelock.execute( + await this.mock.execute( this.operation1.target, this.operation1.value, this.operation1.data, @@ -777,7 +785,7 @@ contract('TimelockController', function (accounts) { this.operation1.salt, { from: executor }, ); - await this.timelock.execute( + await this.mock.execute( this.operation2.target, this.operation2.value, this.operation2.data, @@ -800,7 +808,7 @@ contract('TimelockController', function (accounts) { '0x8043596363daefc89977b25f9d9b4d06c3910959ef0c4d213557a903e1b555e2', ); - await this.timelock.schedule( + await this.mock.schedule( operation.target, operation.value, operation.data, @@ -810,7 +818,7 @@ contract('TimelockController', function (accounts) { { from: proposer }, ); await time.increase(MINDELAY); - await this.timelock.execute( + await this.mock.execute( operation.target, operation.value, operation.data, @@ -831,7 +839,7 @@ contract('TimelockController', function (accounts) { '0xb1b1b276fdf1a28d1e00537ea73b04d56639128b08063c1a2f70a52e38cba693', ); - await this.timelock.schedule( + await this.mock.schedule( operation.target, operation.value, operation.data, @@ -842,7 +850,7 @@ contract('TimelockController', function (accounts) { ); await time.increase(MINDELAY); await expectRevert( - this.timelock.execute( + this.mock.execute( operation.target, operation.value, operation.data, @@ -863,7 +871,7 @@ contract('TimelockController', function (accounts) { '0xe5ca79f295fc8327ee8a765fe19afb58f4a0cbc5053642bfdd7e73bc68e0fc67', ); - await this.timelock.schedule( + await this.mock.schedule( operation.target, operation.value, operation.data, @@ -874,7 +882,7 @@ contract('TimelockController', function (accounts) { ); await time.increase(MINDELAY); await expectRevert( - this.timelock.execute( + this.mock.execute( operation.target, operation.value, operation.data, @@ -895,7 +903,7 @@ contract('TimelockController', function (accounts) { '0xf3274ce7c394c5b629d5215723563a744b817e1730cca5587c567099a14578fd', ); - await this.timelock.schedule( + await this.mock.schedule( operation.target, operation.value, operation.data, @@ -906,7 +914,7 @@ contract('TimelockController', function (accounts) { ); await time.increase(MINDELAY); await expectRevert( - this.timelock.execute( + this.mock.execute( operation.target, operation.value, operation.data, @@ -927,7 +935,7 @@ contract('TimelockController', function (accounts) { '0x5ab73cd33477dcd36c1e05e28362719d0ed59a7b9ff14939de63a43073dc1f44', ); - await this.timelock.schedule( + await this.mock.schedule( operation.target, operation.value, operation.data, @@ -938,10 +946,10 @@ contract('TimelockController', function (accounts) { ); await time.increase(MINDELAY); - expect(await web3.eth.getBalance(this.timelock.address)).to.be.bignumber.equal(web3.utils.toBN(0)); + expect(await web3.eth.getBalance(this.mock.address)).to.be.bignumber.equal(web3.utils.toBN(0)); expect(await web3.eth.getBalance(this.callreceivermock.address)).to.be.bignumber.equal(web3.utils.toBN(0)); - await this.timelock.execute( + await this.mock.execute( operation.target, operation.value, operation.data, @@ -950,7 +958,7 @@ contract('TimelockController', function (accounts) { { from: executor, value: 1 }, ); - expect(await web3.eth.getBalance(this.timelock.address)).to.be.bignumber.equal(web3.utils.toBN(0)); + expect(await web3.eth.getBalance(this.mock.address)).to.be.bignumber.equal(web3.utils.toBN(0)); expect(await web3.eth.getBalance(this.callreceivermock.address)).to.be.bignumber.equal(web3.utils.toBN(1)); }); @@ -963,7 +971,7 @@ contract('TimelockController', function (accounts) { '0xb78edbd920c7867f187e5aa6294ae5a656cfbf0dea1ccdca3751b740d0f2bdf8', ); - await this.timelock.schedule( + await this.mock.schedule( operation.target, operation.value, operation.data, @@ -974,11 +982,11 @@ contract('TimelockController', function (accounts) { ); await time.increase(MINDELAY); - expect(await web3.eth.getBalance(this.timelock.address)).to.be.bignumber.equal(web3.utils.toBN(0)); + expect(await web3.eth.getBalance(this.mock.address)).to.be.bignumber.equal(web3.utils.toBN(0)); expect(await web3.eth.getBalance(this.callreceivermock.address)).to.be.bignumber.equal(web3.utils.toBN(0)); await expectRevert( - this.timelock.execute( + this.mock.execute( operation.target, operation.value, operation.data, @@ -989,7 +997,7 @@ contract('TimelockController', function (accounts) { 'TimelockController: underlying transaction reverted', ); - expect(await web3.eth.getBalance(this.timelock.address)).to.be.bignumber.equal(web3.utils.toBN(0)); + expect(await web3.eth.getBalance(this.mock.address)).to.be.bignumber.equal(web3.utils.toBN(0)); expect(await web3.eth.getBalance(this.callreceivermock.address)).to.be.bignumber.equal(web3.utils.toBN(0)); }); @@ -1002,7 +1010,7 @@ contract('TimelockController', function (accounts) { '0xdedb4563ef0095db01d81d3f2decf57cf83e4a72aa792af14c43a792b56f4de6', ); - await this.timelock.schedule( + await this.mock.schedule( operation.target, operation.value, operation.data, @@ -1013,11 +1021,11 @@ contract('TimelockController', function (accounts) { ); await time.increase(MINDELAY); - expect(await web3.eth.getBalance(this.timelock.address)).to.be.bignumber.equal(web3.utils.toBN(0)); + expect(await web3.eth.getBalance(this.mock.address)).to.be.bignumber.equal(web3.utils.toBN(0)); expect(await web3.eth.getBalance(this.callreceivermock.address)).to.be.bignumber.equal(web3.utils.toBN(0)); await expectRevert( - this.timelock.execute( + this.mock.execute( operation.target, operation.value, operation.data, @@ -1028,79 +1036,60 @@ contract('TimelockController', function (accounts) { 'TimelockController: underlying transaction reverted', ); - expect(await web3.eth.getBalance(this.timelock.address)).to.be.bignumber.equal(web3.utils.toBN(0)); + expect(await web3.eth.getBalance(this.mock.address)).to.be.bignumber.equal(web3.utils.toBN(0)); expect(await web3.eth.getBalance(this.callreceivermock.address)).to.be.bignumber.equal(web3.utils.toBN(0)); }); }); - describe('onERC721Received', function () { - const name = 'Non Fungible Token'; - const symbol = 'NFT'; - - it('receives an ERC721 token', async function () { - const token = await ERC721Mock.new(name, symbol); + describe('safe receive', function () { + describe('ERC721', function () { + const name = 'Non Fungible Token'; + const symbol = 'NFT'; const tokenId = new BN(1); - await token.mint(other, tokenId); - await token.safeTransferFrom(other, this.timelock.address, tokenId, { from: other }); + beforeEach(async function () { + this.token = await ERC721Mock.new(name, symbol); + await this.token.mint(other, tokenId); + }); - expect(await token.ownerOf(tokenId)).to.be.equal(this.timelock.address); + it('can receive an ERC721 safeTransfer', async function () { + await this.token.safeTransferFrom(other, this.mock.address, tokenId, { from: other }); + }); }); - }); - describe('onERC1155Received', function () { - const uri = 'https://token-cdn-domain/{id}.json'; - const multiTokenIds = [new BN(1), new BN(2), new BN(3)]; - const multiTokenAmounts = [new BN(1000), new BN(2000), new BN(3000)]; - const transferData = '0x12345678'; + describe('ERC1155', function () { + const uri = 'https://token-cdn-domain/{id}.json'; + const tokenIds = { + 1: new BN(1000), + 2: new BN(2000), + 3: new BN(3000), + }; - beforeEach(async function () { - this.multiToken = await ERC1155Mock.new(uri, { from: other }); - await this.multiToken.mintBatch(other, multiTokenIds, multiTokenAmounts, '0x', { from: other }); - }); - - it('receives ERC1155 tokens from a single ID', async function () { - await this.multiToken.safeTransferFrom( - other, - this.timelock.address, - multiTokenIds[0], - multiTokenAmounts[0], - transferData, - { from: other }, - ); - - expect(await this.multiToken.balanceOf(this.timelock.address, multiTokenIds[0])).to.be.bignumber.equal( - multiTokenAmounts[0], - ); - - for (let i = 1; i < multiTokenIds.length; i++) { - expect(await this.multiToken.balanceOf(this.timelock.address, multiTokenIds[i])).to.be.bignumber.equal( - new BN(0), - ); - } - }); + beforeEach(async function () { + this.token = await ERC1155Mock.new(uri); + await this.token.mintBatch(other, Object.keys(tokenIds), Object.values(tokenIds), '0x'); + }); - it('receives ERC1155 tokens from a multiple IDs', async function () { - for (let i = 0; i < multiTokenIds.length; i++) { - expect(await this.multiToken.balanceOf(this.timelock.address, multiTokenIds[i])).to.be.bignumber.equal( - new BN(0), + it('can receive ERC1155 safeTransfer', async function () { + await this.token.safeTransferFrom( + other, + this.mock.address, + ...Object.entries(tokenIds)[0], // id + amount + '0x', + { from: other }, ); - } - - await this.multiToken.safeBatchTransferFrom( - other, - this.timelock.address, - multiTokenIds, - multiTokenAmounts, - transferData, - { from: other }, - ); + }); - for (let i = 0; i < multiTokenIds.length; i++) { - expect(await this.multiToken.balanceOf(this.timelock.address, multiTokenIds[i])).to.be.bignumber.equal( - multiTokenAmounts[i], + it('can receive ERC1155 safeBatchTransfer', async function () { + await this.token.safeBatchTransferFrom( + other, + this.mock.address, + Object.keys(tokenIds), + Object.values(tokenIds), + '0x', + { from: other }, ); - } + }); }); }); }); From 7802d2dc7fd471469182e1b586af4561e4efddf2 Mon Sep 17 00:00:00 2001 From: Hadrien Croubois Date: Thu, 24 Mar 2022 01:09:17 +0100 Subject: [PATCH 4/4] add changelog entry --- CHANGELOG.md | 2 ++ 1 file changed, 2 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index a64abe50308..3cd1a889cdd 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -11,6 +11,8 @@ * `Governor`: Add a way to parameterize votes. This can be used to implement voting systems such as fractionalized voting, ERC721 based voting, or any number of other systems. The `params` argument added to `_countVote` method, and included in the newly added `_getVotes` method, can be used by counting and voting modules respectively for such purposes. * `TimelockController`: Add a separate canceller role for the ability to cancel. ([#3165](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/3165)) * `draft-ERC20Permit`: replace `immutable` with `constant` for `_PERMIT_TYPEHASH` since the `keccak256` of string literals is treated specially and the hash is evaluated at compile time. ([#3196](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/3196)) + * `Governor`: Implement `IERC721Receiver` and `IERC1155Receiver` to improve token custody by governors. ([#3230](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/3230)) + * `TimelockController`: Implement `IERC721Receiver` and `IERC1155Receiver` to improve token custody by timelocks. ([#3230](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/3230)) ### Breaking changes