From f3dd2a8d73eb18bf55ca60b8ceacf042a696c104 Mon Sep 17 00:00:00 2001 From: afkbyte Date: Thu, 24 Feb 2022 02:00:38 -0500 Subject: [PATCH 1/8] implement metadataUpdater role and metadataLocking function --- contracts/TimelockedERC721.sol | 61 ++++++++++++++++++++++++++-------- 1 file changed, 47 insertions(+), 14 deletions(-) diff --git a/contracts/TimelockedERC721.sol b/contracts/TimelockedERC721.sol index 65fada3..ef4b5f5 100644 --- a/contracts/TimelockedERC721.sol +++ b/contracts/TimelockedERC721.sol @@ -16,11 +16,14 @@ contract TimelockedERC721 is ERC721EnumerableUpgradeable { bytes32 public constant MINTER_ROLE = keccak256("MINTER_ROLE"); + bytes32 public constant METADATA_MODIFIER_ROLE = keccak256("METADATA_MODIFIER_ROLE"); mapping(uint256 => bool) public lockedTokens; uint256 public tokenUnlockTimestamp; + bool public metadataUpdatable = true; + string private _baseTokenURI; event TokenUnlockTimestampSet(address adminAddress, uint256 timestamp); @@ -44,11 +47,27 @@ contract TimelockedERC721 is _; } + modifier onlyMetadataModifier() { + require( + hasRole(METADATA_MODIFIER_ROLE, _msgSender()), + "Doesn't have metadata modifier role!" + ); + _; + } + modifier futureTimestamp(uint256 timestamp_) { require(timestamp_ > block.timestamp, "timestamp must be in future!"); _; } + modifier metadataLocked() { + require( + !metadataUpdatable, + "Metadata cannot be updated!" + ); + _; + } + modifier tokenLocked(uint256 tokenId_) { if (lockedTokens[tokenId_]) { if (block.timestamp >= tokenUnlockTimestamp) { @@ -80,6 +99,7 @@ contract TimelockedERC721 is _setupRole(DEFAULT_ADMIN_ROLE, _msgSender()); _setupRole(MINTER_ROLE, _msgSender()); + _setupRole(METADATA_MODIFIER_ROLE, _msgSender()); } function setTokenUnlockTimestamp(uint256 timestamp_) @@ -101,10 +121,19 @@ contract TimelockedERC721 is emit MinterAdded(minter_); } - function _baseURI() internal view virtual override returns (string memory) { - return _baseTokenURI; + function addMetadataModifier(address metadataUpdater_) external onlyAdmin { + grantRole(METADATA_MODIFIER_ROLE, metadataUpdater_); + } + + function updateBaseTokenURI(string memory newBaseTokenURI_) external onlyMetadataModifier metadataLocked { + _baseTokenURI = newBaseTokenURI_; } + function lockMetadata() external onlyMetadataModifier { + metadataUpdatable = false; + } + + function lockToken(uint256 tokenId) external { require(ownerOf(tokenId) == _msgSender(), "token not owned!"); require(!lockedTokens[tokenId], "token already locked!"); @@ -122,18 +151,6 @@ contract TimelockedERC721 is } } - function _beforeTokenTransfer( - address from_, - address to_, - uint256 tokenId_ - ) - internal - override(ERC721Upgradeable, ERC721EnumerableUpgradeable) - tokenLocked(tokenId_) - { - super._beforeTokenTransfer(from_, to_, tokenId_); - } - function supportsInterface(bytes4 interfaceId) public view @@ -147,4 +164,20 @@ contract TimelockedERC721 is { return super.supportsInterface(interfaceId); } + + function _beforeTokenTransfer( + address from_, + address to_, + uint256 tokenId_ + ) + internal + override(ERC721Upgradeable, ERC721EnumerableUpgradeable) + tokenLocked(tokenId_) + { + super._beforeTokenTransfer(from_, to_, tokenId_); + } + + function _baseURI() internal view virtual override returns (string memory) { + return _baseTokenURI; + } } From e16ef4c3bd29964170f481f9c4cd6a82cc4c5c01 Mon Sep 17 00:00:00 2001 From: afkbyte Date: Thu, 24 Feb 2022 02:12:40 -0500 Subject: [PATCH 2/8] wrote tests --- test/TimelockedERC721.test.ts | 55 +++++++++++++++++++++++++++++++++-- 1 file changed, 53 insertions(+), 2 deletions(-) diff --git a/test/TimelockedERC721.test.ts b/test/TimelockedERC721.test.ts index 89eeb60..4f84565 100644 --- a/test/TimelockedERC721.test.ts +++ b/test/TimelockedERC721.test.ts @@ -47,7 +47,7 @@ describe('Timelocked ERC721', () => { ) }) - it('[sanity] admin has DEFAULT_ADMIN_ROLE and MINTER_ROLE', async () => { + it('[sanity] admin has DEFAULT_ADMIN_ROLE, MINTER_ROLE, and METADATA_UPDATER_ROLE', async () => { expect( await timelockedERC721.hasRole( ethers.utils.keccak256( @@ -56,12 +56,20 @@ describe('Timelocked ERC721', () => { await admin.getAddress() ) ) + expect( await timelockedERC721.hasRole( ethers.utils.keccak256(ethers.utils.toUtf8Bytes('MINTER_ROLE')), await admin.getAddress() ) ) + + expect( + await timelockedERC721.hasRole( + ethers.utils.keccak256(ethers.utils.toUtf8Bytes('METADATA_UPDATER_ROLE')), + await admin.getAddress() + ) + ) }) it('Should be able to mint tokens to other addresses with minter role', async () => { @@ -75,7 +83,7 @@ describe('Timelocked ERC721', () => { } }) - it('Should be able to batchMint to an addres', async () => { + it('Should be able to batchMint to an address', async () => { await timelockedERC721 .connect(admin) .batchMint(await users[1].getAddress(), [90, 91, 92]) @@ -103,6 +111,49 @@ describe('Timelocked ERC721', () => { ).to.be.revertedWith("Doesn't have minter role!") }) + it('Should be able to update baseURI with metadata updater role', async () => { + await timelockedERC721 + .connect(admin) + .updateBaseTokenURI("0x000/") + + expect(await timelockedERC721.tokenURI(3)).to.equal("0x000/3") + }) + + it('Should not be able to update baseURI without metadata updater role', async () => { + await expect( + timelockedERC721 + .connect(users[1]) + .updateBaseTokenURI("0x000/") + ).to.be.revertedWith("Metadata cannot be updated!") + }) + + it('Should not be able to update baseURI with metadata updater role when metadata is locked', async () => { + await timelockedERC721 + .connect(admin) + .lockMetadata() + + await expect( + timelockedERC721 + .connect(admin]) + .updateBaseTokenURI("0x") + ).to.be.revertedWith("Doesn't have metadata modifier role!") + + + }) + + it('Should be able to transfer tokens that have never been locked', async () => { + await timelockedERC721 + .connect(users[0]) + .transferFrom( + await users[0].getAddress(), + await users[1].getAddress(), + 0 + ) + expect(await timelockedERC721.ownerOf(0)).to.equal( + await users[1].getAddress() + ) + }) + it('Should be able to transfer tokens that have never been locked', async () => { await timelockedERC721 .connect(users[0]) From 9448a556318644983a2e52952e3920df164edc8c Mon Sep 17 00:00:00 2001 From: afkbyte Date: Thu, 24 Feb 2022 02:16:14 -0500 Subject: [PATCH 3/8] fix typo --- test/TimelockedERC721.test.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/TimelockedERC721.test.ts b/test/TimelockedERC721.test.ts index 4f84565..ca6946b 100644 --- a/test/TimelockedERC721.test.ts +++ b/test/TimelockedERC721.test.ts @@ -134,7 +134,7 @@ describe('Timelocked ERC721', () => { await expect( timelockedERC721 - .connect(admin]) + .connect(admin) .updateBaseTokenURI("0x") ).to.be.revertedWith("Doesn't have metadata modifier role!") From ef9698aa154f06691ebe63f05e6a283b4dac54f3 Mon Sep 17 00:00:00 2001 From: afkbyte Date: Thu, 24 Feb 2022 02:21:24 -0500 Subject: [PATCH 4/8] fix error messages --- contracts/TimelockedERC721.sol | 2 +- test/TimelockedERC721.test.ts | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/contracts/TimelockedERC721.sol b/contracts/TimelockedERC721.sol index ef4b5f5..10c8867 100644 --- a/contracts/TimelockedERC721.sol +++ b/contracts/TimelockedERC721.sol @@ -62,7 +62,7 @@ contract TimelockedERC721 is modifier metadataLocked() { require( - !metadataUpdatable, + metadataUpdatable, "Metadata cannot be updated!" ); _; diff --git a/test/TimelockedERC721.test.ts b/test/TimelockedERC721.test.ts index ca6946b..552e893 100644 --- a/test/TimelockedERC721.test.ts +++ b/test/TimelockedERC721.test.ts @@ -124,7 +124,7 @@ describe('Timelocked ERC721', () => { timelockedERC721 .connect(users[1]) .updateBaseTokenURI("0x000/") - ).to.be.revertedWith("Metadata cannot be updated!") + ).to.be.revertedWith("Doesn't have metadata modifier role!") }) it('Should not be able to update baseURI with metadata updater role when metadata is locked', async () => { @@ -136,7 +136,7 @@ describe('Timelocked ERC721', () => { timelockedERC721 .connect(admin) .updateBaseTokenURI("0x") - ).to.be.revertedWith("Doesn't have metadata modifier role!") + ).to.be.revertedWith("Metadata cannot be updated!") }) From f029147106ccbde301e938df2209d4f89c4c69d8 Mon Sep 17 00:00:00 2001 From: afkbyte Date: Thu, 24 Feb 2022 02:28:10 -0500 Subject: [PATCH 5/8] batch mint --- test/TimelockedERC721.test.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/TimelockedERC721.test.ts b/test/TimelockedERC721.test.ts index 552e893..44ded3b 100644 --- a/test/TimelockedERC721.test.ts +++ b/test/TimelockedERC721.test.ts @@ -116,7 +116,7 @@ describe('Timelocked ERC721', () => { .connect(admin) .updateBaseTokenURI("0x000/") - expect(await timelockedERC721.tokenURI(3)).to.equal("0x000/3") + expect(await timelockedERC721.tokenURI(0)).to.equal("0x000/0") }) it('Should not be able to update baseURI without metadata updater role', async () => { From 3010becaf2cacf313f86e0cab4ca4ccdfe8fd813 Mon Sep 17 00:00:00 2001 From: afkbyte Date: Thu, 24 Feb 2022 02:31:00 -0500 Subject: [PATCH 6/8] remove duplicate test --- test/TimelockedERC721.test.ts | 13 ------------- 1 file changed, 13 deletions(-) diff --git a/test/TimelockedERC721.test.ts b/test/TimelockedERC721.test.ts index 44ded3b..51b5146 100644 --- a/test/TimelockedERC721.test.ts +++ b/test/TimelockedERC721.test.ts @@ -154,19 +154,6 @@ describe('Timelocked ERC721', () => { ) }) - it('Should be able to transfer tokens that have never been locked', async () => { - await timelockedERC721 - .connect(users[0]) - .transferFrom( - await users[0].getAddress(), - await users[1].getAddress(), - 0 - ) - expect(await timelockedERC721.ownerOf(0)).to.equal( - await users[1].getAddress() - ) - }) - it('Should not be able to transfer locked tokens before unlockTimestamp', async () => { await timelockedERC721.connect(users[1]).lockToken(0) await expect( From 1ff9ad403da72f48e92e485b37552374c68742c7 Mon Sep 17 00:00:00 2001 From: afkbyte Date: Thu, 24 Feb 2022 02:41:13 -0500 Subject: [PATCH 7/8] lint --- test/TimelockedERC721.test.ts | 1 - 1 file changed, 1 deletion(-) diff --git a/test/TimelockedERC721.test.ts b/test/TimelockedERC721.test.ts index 51b5146..8841520 100644 --- a/test/TimelockedERC721.test.ts +++ b/test/TimelockedERC721.test.ts @@ -131,7 +131,6 @@ describe('Timelocked ERC721', () => { await timelockedERC721 .connect(admin) .lockMetadata() - await expect( timelockedERC721 .connect(admin) From d14c897db498515bf9b54e2d445048d3bdadd27b Mon Sep 17 00:00:00 2001 From: afkbyte Date: Thu, 24 Feb 2022 02:42:56 -0500 Subject: [PATCH 8/8] npm prettier --- test/TimelockedERC721.test.ts | 28 ++++++++++------------------ 1 file changed, 10 insertions(+), 18 deletions(-) diff --git a/test/TimelockedERC721.test.ts b/test/TimelockedERC721.test.ts index 8841520..0ed2eea 100644 --- a/test/TimelockedERC721.test.ts +++ b/test/TimelockedERC721.test.ts @@ -66,7 +66,9 @@ describe('Timelocked ERC721', () => { expect( await timelockedERC721.hasRole( - ethers.utils.keccak256(ethers.utils.toUtf8Bytes('METADATA_UPDATER_ROLE')), + ethers.utils.keccak256( + ethers.utils.toUtf8Bytes('METADATA_UPDATER_ROLE') + ), await admin.getAddress() ) ) @@ -112,32 +114,22 @@ describe('Timelocked ERC721', () => { }) it('Should be able to update baseURI with metadata updater role', async () => { - await timelockedERC721 - .connect(admin) - .updateBaseTokenURI("0x000/") - - expect(await timelockedERC721.tokenURI(0)).to.equal("0x000/0") + await timelockedERC721.connect(admin).updateBaseTokenURI('0x000/') + + expect(await timelockedERC721.tokenURI(0)).to.equal('0x000/0') }) it('Should not be able to update baseURI without metadata updater role', async () => { await expect( - timelockedERC721 - .connect(users[1]) - .updateBaseTokenURI("0x000/") + timelockedERC721.connect(users[1]).updateBaseTokenURI('0x000/') ).to.be.revertedWith("Doesn't have metadata modifier role!") }) it('Should not be able to update baseURI with metadata updater role when metadata is locked', async () => { - await timelockedERC721 - .connect(admin) - .lockMetadata() + await timelockedERC721.connect(admin).lockMetadata() await expect( - timelockedERC721 - .connect(admin) - .updateBaseTokenURI("0x") - ).to.be.revertedWith("Metadata cannot be updated!") - - + timelockedERC721.connect(admin).updateBaseTokenURI('0x') + ).to.be.revertedWith('Metadata cannot be updated!') }) it('Should be able to transfer tokens that have never been locked', async () => {