From ee520dcc73db98968ec4b11d44725e0b95559b67 Mon Sep 17 00:00:00 2001 From: EmperorOrokuSaki Date: Thu, 15 Dec 2022 21:31:59 +0330 Subject: [PATCH] fix erc721 --- contracts/FleekAccessControl.sol | 155 +++++++++++++++++++++++++------ contracts/FleekERC721.sol | 45 +++------ test/FleekERC721.ts | 124 +++++++++++++++++++++++-- 3 files changed, 254 insertions(+), 70 deletions(-) diff --git a/contracts/FleekAccessControl.sol b/contracts/FleekAccessControl.sol index 9f18772..3c904c6 100644 --- a/contracts/FleekAccessControl.sol +++ b/contracts/FleekAccessControl.sol @@ -2,59 +2,154 @@ pragma solidity ^0.8.7; -import "@openzeppelin/contracts/access/AccessControl.sol"; +abstract contract FleekAccessControl { + enum Roles { + Owner, + Controller + } -abstract contract FleekAccessControl is AccessControl { - bytes32 public constant COLLECTION_OWNER_ROLE = - keccak256("COLLECTION_OWNER_ROLE"); - bytes32 public constant COLLECTION_CONTROLLER_ROLE = - keccak256("COLLECTION_CONTROLLER_ROLE"); + struct Role { + mapping(address => uint256) indexes; + address[] members; + } + + // _collectionRoles[role] + mapping(Roles => Role) private _collectionRoles; + + // _tokenRoles[tokenId][role] + mapping(uint256 => mapping(Roles => Role)) private _tokenRoles; constructor() { - _setRoleAdmin(COLLECTION_OWNER_ROLE, DEFAULT_ADMIN_ROLE); - _grantRole(COLLECTION_OWNER_ROLE, msg.sender); + _grantCollectionRole(Roles.Owner, msg.sender); } - modifier requireCollectionOwner() { + modifier requireCollectionRole(Roles role) { require( - hasRole(COLLECTION_OWNER_ROLE, msg.sender), - "FleekAccessControl: must have collection owner role" + hasCollectionRole(role, msg.sender) || + hasCollectionRole(Roles.Owner, msg.sender), + "FleekAccessControl: must have collection role" ); _; } - modifier requireCollectionController() { + modifier requireTokenRole(uint256 tokenId, Roles role) { require( - hasRole(COLLECTION_OWNER_ROLE, msg.sender) || - hasRole(COLLECTION_CONTROLLER_ROLE, msg.sender), - "FleekAccessControl: must have collection controller role" - ); - _; - } - - modifier requireTokenController(uint256 tokenId) { - require( - hasRole(_tokenRole(tokenId, "CONTROLLER"), msg.sender), + hasTokenRole(tokenId, role, msg.sender) || + hasTokenRole(tokenId, Roles.Owner, msg.sender), "FleekAccessControl: must have token role" ); _; } - function isTokenController( + function grantCollectionRole( + Roles role, + address account + ) public requireCollectionRole(Roles.Owner) { + _grantCollectionRole(role, account); + } + + function grantTokenRole( uint256 tokenId, + Roles role, + address account + ) public requireCollectionRole(Roles.Owner) { + _grantTokenRole(tokenId, role, account); + } + + function revokeCollectionRole( + Roles role, + address account + ) public requireCollectionRole(Roles.Owner) { + _revokeCollectionRole(role, account); + } + + function revokeTokenRole( + uint256 tokenId, + Roles role, + address account + ) public requireCollectionRole(Roles.Owner) { + _revokeTokenRole(tokenId, role, account); + } + + function hasCollectionRole( + Roles role, address account ) public view returns (bool) { - return hasRole(_tokenRole(tokenId, "CONTROLLER"), account); + return _collectionRoles[role].indexes[account] != 0; } - function _tokenRole( + function hasTokenRole( uint256 tokenId, - string memory role - ) internal pure returns (bytes32) { - return keccak256(abi.encodePacked("TOKEN_", role, tokenId)); + Roles role, + address account + ) public view returns (bool) { + return _tokenRoles[tokenId][role].indexes[account] != 0; } - function _clearTokenControllers(uint256 tokenId) internal { - // TODO: Remove token controllers from AccessControl + function getCollectionRoleMembers( + Roles role + ) public view returns (address[] memory) { + return _collectionRoles[role].members; + } + + function getTokenRoleMembers( + uint256 tokenId, + Roles role + ) public view returns (address[] memory) { + return _tokenRoles[tokenId][role].members; + } + + function _grantCollectionRole(Roles role, address account) internal { + _grantRole(_collectionRoles[role], account); + } + + function _revokeCollectionRole(Roles role, address account) internal { + _revokeRole(_collectionRoles[role], account); + } + + function _grantTokenRole( + uint256 tokenId, + Roles role, + address account + ) internal { + _grantRole(_tokenRoles[tokenId][role], account); + } + + function _revokeTokenRole( + uint256 tokenId, + Roles role, + address account + ) internal { + _revokeRole(_tokenRoles[tokenId][role], account); + } + + function _grantRole(Role storage role, address account) internal { + if (role.indexes[account] == 0) { + role.members.push(account); + role.indexes[account] = role.members.length; + } + } + + function _revokeRole(Role storage role, address account) internal { + if (role.indexes[account] != 0) { + uint256 index = role.indexes[account] - 1; + uint256 lastIndex = role.members.length - 1; + address lastAccount = role.members[lastIndex]; + + role.members[index] = lastAccount; + role.indexes[lastAccount] = index + 1; + + role.members.pop(); + delete role.indexes[account]; + } + } + + function _clearTokenRole(uint256 tokenId, Roles role) internal { + delete _tokenRoles[tokenId][role]; + } + + function _clearAllTokenRoles(uint256 tokenId) internal { + delete _tokenRoles[tokenId][Roles.Owner]; + delete _tokenRoles[tokenId][Roles.Controller]; } } diff --git a/contracts/FleekERC721.sol b/contracts/FleekERC721.sol index 78b6845..80057ab 100644 --- a/contracts/FleekERC721.sol +++ b/contracts/FleekERC721.sol @@ -108,32 +108,9 @@ contract FleekERC721 is ERC721, FleekAccessControl { return string(abi.encodePacked(_baseURI(), Base64.encode((dataURI)))); } - function addTokenController( - uint256 tokenId, - address controller - ) public requireTokenOwner(tokenId) { - _addTokenController(tokenId, controller); - } - - function _addTokenController( - uint256 tokenId, - address controller - ) internal { - _requireMinted(tokenId); - _grantRole(_tokenRole(tokenId, "CONTROLLER"), controller); - } - - function removeTokenController( - uint256 tokenId, - address controller - ) public requireTokenOwner(tokenId) { - _requireMinted(tokenId); - _revokeRole(_tokenRole(tokenId, "CONTROLLER"), controller); - } - function supportsInterface( bytes4 interfaceId - ) public view virtual override(ERC721, AccessControl) returns (bool) { + ) public view virtual override(ERC721) returns (bool) { return super.supportsInterface(interfaceId); } @@ -150,14 +127,14 @@ contract FleekERC721 is ERC721, FleekAccessControl { ) internal virtual override { if (from != address(0) && to != address(0)) { // Transfer - _clearTokenControllers(tokenId); - _grantRole(_tokenRole(tokenId, "CONTROLLER"), to); + _clearAllTokenRoles(tokenId); + _grantTokenRole(tokenId, Roles.Owner, to); } else if (from == address(0)) { // Mint - _grantRole(_tokenRole(tokenId, "CONTROLLER"), to); + _grantTokenRole(tokenId, Roles.Owner, to); } else if (to == address(0)) { // Burn - _clearTokenControllers(tokenId); + _clearAllTokenRoles(tokenId); } super._beforeTokenTransfer(from, to, tokenId, batchSize); } @@ -169,7 +146,7 @@ contract FleekERC721 is ERC721, FleekAccessControl { function setTokenExternalURL( uint256 tokenId, string memory _tokenExternalURL - ) public virtual requireTokenController(tokenId) { + ) public virtual requireTokenRole(tokenId, Roles.Controller) { _requireMinted(tokenId); _apps[tokenId].external_url = _tokenExternalURL; emit NewTokenExternalURL(tokenId, _tokenExternalURL); @@ -178,7 +155,7 @@ contract FleekERC721 is ERC721, FleekAccessControl { function setTokenENS( uint256 tokenId, string memory _tokenENS - ) public virtual requireTokenController(tokenId) { + ) public virtual requireTokenRole(tokenId, Roles.Controller) { _requireMinted(tokenId); _apps[tokenId].ENS = _tokenENS; emit NewTokenENS(tokenId, _tokenENS); @@ -187,7 +164,7 @@ contract FleekERC721 is ERC721, FleekAccessControl { function setTokenName( uint256 tokenId, string memory _tokenName - ) public virtual requireTokenController(tokenId) { + ) public virtual requireTokenRole(tokenId, Roles.Controller) { _requireMinted(tokenId); _apps[tokenId].name = _tokenName; emit NewTokenName(tokenId, _tokenName); @@ -196,7 +173,7 @@ contract FleekERC721 is ERC721, FleekAccessControl { function setTokenDescription( uint256 tokenId, string memory _tokenDescription - ) public virtual requireTokenController(tokenId) { + ) public virtual requireTokenRole(tokenId, Roles.Controller) { _requireMinted(tokenId); _apps[tokenId].description = _tokenDescription; emit NewTokenDescription(tokenId, _tokenDescription); @@ -205,7 +182,7 @@ contract FleekERC721 is ERC721, FleekAccessControl { function setTokenImage( uint256 tokenId, string memory _tokenImage - ) public virtual requireTokenController(tokenId) { + ) public virtual requireTokenRole(tokenId, Roles.Controller) { _requireMinted(tokenId); _apps[tokenId].image = _tokenImage; emit NewTokenImage(tokenId, _tokenImage); @@ -223,7 +200,7 @@ contract FleekERC721 is ERC721, FleekAccessControl { function burn( uint256 tokenId - ) public virtual requireTokenOwner(tokenId) { + ) public virtual requireTokenRole(tokenId, Roles.Owner) { super._burn(tokenId); if (bytes(_apps[tokenId].external_url).length != 0) { diff --git a/test/FleekERC721.ts b/test/FleekERC721.ts index cce86ed..427bde7 100644 --- a/test/FleekERC721.ts +++ b/test/FleekERC721.ts @@ -46,9 +46,7 @@ describe('FleekERC721', () => { it('should assign the owner of the contract', async () => { const { owner, contract } = await loadFixture(defaultFixture); - expect( - await contract.hasRole(COLLECTION_OWNER_ROLE, owner.address) - ).to.equal(true); + expect(await contract.hasCollectionRole(0, owner.address)).to.equal(true); }); it('should support ERC721 interface', async () => { @@ -93,13 +91,40 @@ describe('FleekERC721', () => { MINT_PARAMS.commitHash, MINT_PARAMS.gitRepository, ) - ).to.be.revertedWith( - 'FleekAccessControl: must have collection owner role' + ).to.be.revertedWith('FleekAccessControl: must have collection role'); + }); + + it('should have address to as owner', async () => { + const { owner, otherAccount, contract } = await loadFixture( + defaultFixture ); + + const response = await contract.mint( + owner.address, + MINT_PARAMS.name, + MINT_PARAMS.description, + MINT_PARAMS.image, + MINT_PARAMS.externalUrl, + MINT_PARAMS.ens, + MINT_PARAMS.commitHash, + MINT_PARAMS.gitRepository, + MINT_PARAMS.author + ); + + const tokenId = response.value.toNumber(); + + expect(await contract.ownerOf(tokenId)).to.equal(owner.address); + expect(await contract.hasTokenRole(tokenId, 0, owner.address)).to.be.true; + + expect(await contract.ownerOf(tokenId)).not.to.equal( + otherAccount.address + ); + expect(await contract.hasTokenRole(tokenId, 0, otherAccount.address)).to + .be.false; }); }); - describe('Token', () => { + describe('Token URI', () => { let tokenId: number; let fixture: Awaited>; @@ -158,11 +183,98 @@ describe('FleekERC721', () => { ], }); }); + }); + + describe('Token Roles', () => { + let tokenId: number; + let fixture: Awaited>; + + beforeEach(async () => { + fixture = await loadFixture(defaultFixture); + const { contract } = fixture; + + const response = await contract.mint( + fixture.owner.address, + MINT_PARAMS.name, + MINT_PARAMS.description, + MINT_PARAMS.image, + MINT_PARAMS.externalUrl, + MINT_PARAMS.ens, + MINT_PARAMS.commitHash, + MINT_PARAMS.gitRepository, + MINT_PARAMS.author + ); + + tokenId = response.value.toNumber(); + }); it('should match the token owner', async () => { const { contract, owner } = fixture; const tokenOwner = await contract.ownerOf(tokenId); + expect(tokenOwner).to.equal(owner.address); }); + + it('should match the owner role for minter', async () => { + const { contract, owner } = fixture; + const hasRole = await contract.hasTokenRole(tokenId, 0, owner.address); + + expect(hasRole).to.be.true; + }); + + it('should add a list of controllers', async () => { + const { contract, owner } = fixture; + await contract.grantTokenRole( + tokenId, + 1, + '0x7ED735b7095C05d78dF169F991f2b7f1A1F1A049' + ); + await contract.grantTokenRole( + tokenId, + 1, + '0x2FEd6Ef3c495922263B403319FA6DDB323DD49E3' + ); + + expect(await contract.getTokenRoleMembers(tokenId, 1)).to.eql([ + '0x7ED735b7095C05d78dF169F991f2b7f1A1F1A049', + '0x2FEd6Ef3c495922263B403319FA6DDB323DD49E3', + ]); + }); + + it('should not match the owner role for other account', async () => { + const { contract, otherAccount } = fixture; + const hasRole = await contract.hasTokenRole( + tokenId, + 0, + otherAccount.address + ); + + expect(hasRole).to.be.false; + }); + + it('should add a new controller', async () => { + const { contract, owner, otherAccount } = fixture; + await contract.grantTokenRole(tokenId, 1, otherAccount.address); + + expect(await contract.hasTokenRole(tokenId, 1, otherAccount.address)).to + .be.true; + }); + + it('should transfer the token owner role', async () => { + // FIXME: this test is failing + const { contract, owner, otherAccount } = fixture; + await contract.transferFrom(owner.address, otherAccount.address, tokenId); + + console.log(owner.address, otherAccount.address, tokenId); + + console.log(await contract.getTokenRoleMembers(tokenId, 0)); + console.log(await contract.getTokenRoleMembers(tokenId, 1)); + + expect(await contract.ownerOf(tokenId)).to.equal(otherAccount.address); + expect(await contract.hasTokenRole(tokenId, 0, otherAccount.address)).to + .be.true; + expect(await contract.hasTokenRole(tokenId, 0, owner.address)).to.be + .false; + }); }); });