fix erc721

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 {
+    struct Role {
-    bytes32 public constant COLLECTION_OWNER_ROLE =
+        mapping(address => uint256) indexes;
-        keccak256("COLLECTION_OWNER_ROLE");
+        address[] members;
-    bytes32 public constant COLLECTION_CONTROLLER_ROLE =
+    }
-        keccak256("COLLECTION_CONTROLLER_ROLE");
+
+    // _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);
+        _grantCollectionRole(Roles.Owner, msg.sender);
-        _grantRole(COLLECTION_OWNER_ROLE, msg.sender);
     }
 
-    modifier requireCollectionOwner() {
+    modifier requireCollectionRole(Roles role) {
         require(
-            hasRole(COLLECTION_OWNER_ROLE, msg.sender),
+            hasCollectionRole(role, msg.sender) ||
-            "FleekAccessControl: must have collection owner role"
+                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) ||
+            hasTokenRole(tokenId, role, msg.sender) ||
-                hasRole(COLLECTION_CONTROLLER_ROLE, msg.sender),
+                hasTokenRole(tokenId, Roles.Owner, msg.sender),
-            "FleekAccessControl: must have collection controller role"
-        );
-        _;
-    }
-
-    modifier requireTokenController(uint256 tokenId) {
-        require(
-            hasRole(_tokenRole(tokenId, "CONTROLLER"), 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
+        Roles role,
-    ) internal pure returns (bytes32) {
+        address account
-        return keccak256(abi.encodePacked("TOKEN_", role, tokenId));
+    ) public view returns (bool) {
+        return _tokenRoles[tokenId][role].indexes[account] != 0;
     }
 
-    function _clearTokenControllers(uint256 tokenId) internal {
+    function getCollectionRoleMembers(
-        // TODO: Remove token controllers from AccessControl
+        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];
     }
 }

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);
+            _clearAllTokenRoles(tokenId);
-            _grantRole(_tokenRole(tokenId, "CONTROLLER"), to);
+            _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) {

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(
+      expect(await contract.hasCollectionRole(0, owner.address)).to.equal(true);
-        await contract.hasRole(COLLECTION_OWNER_ROLE, 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(
+      ).to.be.revertedWith('FleekAccessControl: must have collection role');
-        'FleekAccessControl: must have collection owner 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<ReturnType<typeof defaultFixture>>;
 
@@ -158,11 +183,98 @@ describe('FleekERC721', () => {
         ],
       });
     });
+  });
+
+  describe('Token Roles', () => {
+    let tokenId: number;
+    let fixture: Awaited<ReturnType<typeof defaultFixture>>;
+
+    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;
+    });
   });
 });