refactor: remove extension AccessControl from FleekAccessControl (#28)

* refactor: remove extension AccessControl from FleekAccessControl

* refactor: add version for roles

* test: add collection roles tests

* test: add more token role hardhat tests

* refactor: remove lib/forge-std

* feat: add role grant and revoke events

* test: add access control role event emit tests

* refactor: remove abstract keyword from FleekAccessControl

* Merge conflicts for #28 (#49)

* Add msg.sender to the triggered_by field in events

* Document methods and make everything camelCase

* Make event params all camelCase

* fix: forge-std submodule

* make vars camelCase, remove baseURI header, remove addTokenController and removeTokenController, update tests

Co-authored-by: EmperorOrokuSaki <artie.eth@gmail.com>
Co-authored-by: Shredder <110225819+EmperorOrokuSaki@users.noreply.github.com>
Co-authored-by: zoruka <zo.fmendes@gmail.com>

Co-authored-by: Janison Sivarajah <jsonsivar@gmail.com>
Co-authored-by: EmperorOrokuSaki <artie.eth@gmail.com>
Co-authored-by: Shredder <110225819+EmperorOrokuSaki@users.noreply.github.com>

contracts/FleekAccessControl.sol

@@ -1,45 +1,151 @@
-// SPDX-License-Identifier: MIT
+// SPDX-License-Identifier: MIT
-
+
-pragma solidity ^0.8.7;
+pragma solidity ^0.8.7;
-
+
-import "@openzeppelin/contracts/access/AccessControl.sol";
+import "@openzeppelin/contracts/utils/Counters.sol";
-
+
-abstract contract FleekAccessControl is AccessControl {
+contract FleekAccessControl {
-    bytes32 public constant COLLECTION_OWNER_ROLE = keccak256("COLLECTION_OWNER_ROLE");
+    using Counters for Counters.Counter;
-    bytes32 public constant COLLECTION_CONTROLLER_ROLE = keccak256("COLLECTION_CONTROLLER_ROLE");
+
-
+    enum Roles {
-    constructor() {
+        Owner,
-        _setRoleAdmin(COLLECTION_OWNER_ROLE, DEFAULT_ADMIN_ROLE);
+        Controller
-        _grantRole(COLLECTION_OWNER_ROLE, msg.sender);
+    }
-    }
+
-
+    event TokenRoleGranted(uint256 indexed tokenId, Roles indexed role, address indexed toAddress, address byAddress);
-    modifier requireCollectionOwner() {
+    event TokenRoleRevoked(uint256 indexed tokenId, Roles indexed role, address indexed toAddress, address byAddress);
-        require(hasRole(COLLECTION_OWNER_ROLE, msg.sender), "FleekAccessControl: must have collection owner role");
+    event CollectionRoleGranted(Roles indexed role, address indexed toAddress, address byAddress);
-        _;
+    event CollectionRoleRevoked(Roles indexed role, address indexed toAddress, address byAddress);
-    }
+
-
+    struct Role {
-    modifier requireCollectionController() {
+        mapping(address => uint256) indexes;
-        require(
+        address[] members;
-            hasRole(COLLECTION_OWNER_ROLE, msg.sender) || hasRole(COLLECTION_CONTROLLER_ROLE, msg.sender),
+    }
-            "FleekAccessControl: must have collection controller role"
+
-        );
+    Counters.Counter private _collectionRolesVersion;
-        _;
+    // _collectionRoles[version][role]
-    }
+    mapping(uint256 => mapping(Roles => Role)) private _collectionRoles;
-
+
-    modifier requireTokenController(uint256 tokenId) {
+    mapping(uint256 => Counters.Counter) private _tokenRolesVersion;
-        require(hasRole(_tokenRole(tokenId, "CONTROLLER"), msg.sender), "FleekAccessControl: must have token role");
+    // _tokenRoles[tokenId][version][role]
-        _;
+    mapping(uint256 => mapping(uint256 => mapping(Roles => Role))) private _tokenRoles;
-    }
+
-
+    constructor() {
-    function isTokenController(uint256 tokenId, address account) public view returns (bool) {
+        _grantCollectionRole(Roles.Owner, msg.sender);
-        return hasRole(_tokenRole(tokenId, "CONTROLLER"), account);
+    }
-    }
+
-
+    modifier requireCollectionRole(Roles role) {
-    function _tokenRole(uint256 tokenId, string memory role) internal pure returns (bytes32) {
+        require(
-        return keccak256(abi.encodePacked("TOKEN_", role, tokenId));
+            hasCollectionRole(role, msg.sender) || hasCollectionRole(Roles.Owner, msg.sender),
-    }
+            "FleekAccessControl: must have collection role"
-
+        );
-    function _clearTokenControllers(uint256 tokenId) internal {
+        _;
-        // TODO: Remove token controllers from AccessControl
+    }
-    }
+
-}
+    modifier requireTokenRole(uint256 tokenId, Roles role) {
+        require(
+            hasTokenRole(tokenId, role, msg.sender) || hasTokenRole(tokenId, Roles.Owner, msg.sender),
+            "FleekAccessControl: must have token role"
+        );
+        _;
+    }
+
+    function grantCollectionRole(Roles role, address account) public requireCollectionRole(Roles.Owner) {
+        _grantCollectionRole(role, account);
+    }
+
+    function grantTokenRole(
+        uint256 tokenId,
+        Roles role,
+        address account
+    ) public requireTokenRole(tokenId, 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 requireTokenRole(tokenId, Roles.Owner) {
+        _revokeTokenRole(tokenId, role, account);
+    }
+
+    function hasCollectionRole(Roles role, address account) public view returns (bool) {
+        uint256 currentVersion = _collectionRolesVersion.current();
+
+        return _collectionRoles[currentVersion][role].indexes[account] != 0;
+    }
+
+    function hasTokenRole(uint256 tokenId, Roles role, address account) public view returns (bool) {
+        uint256 currentVersion = _tokenRolesVersion[tokenId].current();
+        return _tokenRoles[tokenId][currentVersion][role].indexes[account] != 0;
+    }
+
+    function getCollectionRoleMembers(Roles role) public view returns (address[] memory) {
+        uint256 currentVersion = _collectionRolesVersion.current();
+        return _collectionRoles[currentVersion][role].members;
+    }
+
+    function getTokenRoleMembers(uint256 tokenId, Roles role) public view returns (address[] memory) {
+        uint256 currentVersion = _tokenRolesVersion[tokenId].current();
+        return _tokenRoles[tokenId][currentVersion][role].members;
+    }
+
+    function _grantCollectionRole(Roles role, address account) internal {
+        uint256 currentVersion = _collectionRolesVersion.current();
+        _grantRole(_collectionRoles[currentVersion][role], account);
+        emit CollectionRoleGranted(role, account, msg.sender);
+    }
+
+    function _revokeCollectionRole(Roles role, address account) internal {
+        uint256 currentVersion = _collectionRolesVersion.current();
+        _revokeRole(_collectionRoles[currentVersion][role], account);
+        emit CollectionRoleRevoked(role, account, msg.sender);
+    }
+
+    function _grantTokenRole(uint256 tokenId, Roles role, address account) internal {
+        uint256 currentVersion = _tokenRolesVersion[tokenId].current();
+        _grantRole(_tokenRoles[tokenId][currentVersion][role], account);
+        emit TokenRoleGranted(tokenId, role, account, msg.sender);
+    }
+
+    function _revokeTokenRole(uint256 tokenId, Roles role, address account) internal {
+        uint256 currentVersion = _tokenRolesVersion[tokenId].current();
+        _revokeRole(_tokenRoles[tokenId][currentVersion][role], account);
+        emit TokenRoleRevoked(tokenId, role, account, msg.sender);
+    }
+
+    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 _clearAllTokenRoles(uint256 tokenId) internal {
+        _tokenRolesVersion[tokenId].increment();
+    }
+
+    function _clearAllTokenRoles(uint256 tokenId, address newOwner) internal {
+        _clearAllTokenRoles(tokenId);
+        _grantTokenRole(tokenId, Roles.Owner, newOwner);
+    }
+}

contracts/FleekERC721.sol

@@ -67,7 +67,7 @@ contract FleekERC721 is ERC721, FleekAccessControl {
         string memory ENS,
         string memory commitHash,
         string memory gitRepository
-    ) public payable requireCollectionOwner returns (uint256) {
+    ) public payable requireCollectionRole(Roles.Owner) returns (uint256) {
         uint256 tokenId = _tokenIds.current();
         _mint(to, tokenId);
         _tokenIds.increment();
@@ -121,42 +121,10 @@ contract FleekERC721 is ERC721, FleekAccessControl {
         return string(abi.encodePacked(_baseURI(), Base64.encode((dataURI))));
     }
 
-    /**
-     * @dev Adds a new address with the`tokenController` privileges to a previously minted `tokenId`.
-     *
-     * May emit a {RoleGranted} event.
-     *
-     * Requirements:
-     *
-     * - the tokenId must be minted and valid.
-     * - the sender must have the `tokenOwner` role.
-     *
-     */
-    function addTokenController(uint256 tokenId, address controller) public requireTokenOwner(tokenId) {
-        _requireMinted(tokenId);
-        _grantRole(_tokenRole(tokenId, "CONTROLLER"), controller);
-    }
-
-    /**
-     * @dev Strips an address from their `tokenController` privileges on a previously minted `tokenId`.
-     *
-     * May emit a {RoleRevoked} event.
-     *
-     * Requirements:
-     *
-     * - the tokenId must be minted and valid.
-     * - the sender must have the `tokenOwner` role.
-     *
-     */
-    function removeTokenController(uint256 tokenId, address controller) public requireTokenOwner(tokenId) {
-        _requireMinted(tokenId);
-        _revokeRole(_tokenRole(tokenId, "CONTROLLER"), controller);
-    }
-
     /**
      * @dev See {IERC165-supportsInterface}.
      */
-    function supportsInterface(bytes4 interfaceId) public view virtual override(ERC721, AccessControl) returns (bool) {
+    function supportsInterface(bytes4 interfaceId) public view virtual override(ERC721) returns (bool) {
         return super.supportsInterface(interfaceId);
     }
 
@@ -173,14 +141,13 @@ contract FleekERC721 is ERC721, FleekAccessControl {
     ) internal virtual override {
         if (from != address(0) && to != address(0)) {
             // Transfer
-            _clearTokenControllers(tokenId);
+            _clearAllTokenRoles(tokenId, to);
-            _grantRole(_tokenRole(tokenId, "CONTROLLER"), 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);
     }
@@ -206,7 +173,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].externalURL = _tokenExternalURL;
         emit NewTokenExternalURL(tokenId, _tokenExternalURL, msg.sender);
@@ -223,7 +190,10 @@ contract FleekERC721 is ERC721, FleekAccessControl {
      * - the sender must have the `tokenController` role.
      *
      */
-    function setTokenENS(uint256 tokenId, string memory _tokenENS) public virtual requireTokenController(tokenId) {
+    function setTokenENS(
+        uint256 tokenId,
+        string memory _tokenENS
+    ) public virtual requireTokenRole(tokenId, Roles.Controller) {
         _requireMinted(tokenId);
         _apps[tokenId].ENS = _tokenENS;
         emit NewTokenENS(tokenId, _tokenENS, msg.sender);
@@ -240,7 +210,10 @@ contract FleekERC721 is ERC721, FleekAccessControl {
      * - the sender must have the `tokenController` role.
      *
      */
-    function setTokenName(uint256 tokenId, string memory _tokenName) public virtual requireTokenController(tokenId) {
+    function setTokenName(
+        uint256 tokenId,
+        string memory _tokenName
+    ) public virtual requireTokenRole(tokenId, Roles.Controller) {
         _requireMinted(tokenId);
         _apps[tokenId].name = _tokenName;
         emit NewTokenName(tokenId, _tokenName, msg.sender);
@@ -260,7 +233,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, msg.sender);
@@ -277,7 +250,10 @@ contract FleekERC721 is ERC721, FleekAccessControl {
      * - the sender must have the `tokenController` role.
      *
      */
-    function setTokenImage(uint256 tokenId, string memory _tokenImage) public virtual requireTokenController(tokenId) {
+    function setTokenImage(
+        uint256 tokenId,
+        string memory _tokenImage
+    ) public virtual requireTokenRole(tokenId, Roles.Controller) {
         _requireMinted(tokenId);
         _apps[tokenId].image = _tokenImage;
         emit NewTokenImage(tokenId, _tokenImage, msg.sender);
@@ -298,7 +274,7 @@ contract FleekERC721 is ERC721, FleekAccessControl {
         uint256 tokenId,
         string memory _commitHash,
         string memory _gitRepository
-    ) public virtual requireTokenController(tokenId) {
+    ) public virtual requireTokenRole(tokenId, Roles.Controller) {
         _requireMinted(tokenId);
         _apps[tokenId].builds[++_apps[tokenId].currentBuild] = Build(_commitHash, _gitRepository);
         emit NewBuild(tokenId, _commitHash, msg.sender);
@@ -315,7 +291,7 @@ contract FleekERC721 is ERC721, FleekAccessControl {
      * - the sender must have the `tokenOwner` role.
      *
      */
-    function burn(uint256 tokenId) public virtual requireTokenOwner(tokenId) {
+     function burn(uint256 tokenId) public virtual requireTokenRole(tokenId, Roles.Owner) {
         super._burn(tokenId);
 
         if (bytes(_apps[tokenId].externalURL).length != 0) {

test/FleekERC721.ts

@@ -1,10 +1,12 @@
 import { loadFixture } from '@nomicfoundation/hardhat-network-helpers';
 import { expect } from 'chai';
 import { ethers } from 'hardhat';
-import web3 from 'web3';
 
 describe('FleekERC721', () => {
-  const COLLECTION_OWNER_ROLE = web3.utils.keccak256('COLLECTION_OWNER_ROLE');
+  const ROLES = Object.freeze({
+    OWNER: 0,
+    CONTROLLER: 1,
+  });
 
   const MINT_PARAMS = Object.freeze({
     name: 'Fleek Test App',
@@ -42,14 +44,6 @@ describe('FleekERC721', () => {
       expect(await contract.symbol()).to.equal(COLLECTION_PARAMS.symbol);
     });
 
-    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);
-    });
-
     it('should support ERC721 interface', async () => {
       const { contract } = await loadFixture(defaultFixture);
 
@@ -92,13 +86,42 @@ 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
+      );
+
+      const tokenId = response.value.toNumber();
+
+      expect(await contract.ownerOf(tokenId)).to.equal(owner.address);
+      expect(await contract.hasTokenRole(tokenId, ROLES.OWNER, owner.address))
+        .to.be.true;
+
+      expect(await contract.ownerOf(tokenId)).not.to.equal(
+        otherAccount.address
+      );
+      expect(
+        await contract.hasTokenRole(tokenId, ROLES.OWNER, otherAccount.address)
+      ).to.be.false;
     });
   });
 
-  describe('Token', () => {
+  describe('Token URI', () => {
+
     let tokenId: number;
     let fixture: Awaited<ReturnType<typeof defaultFixture>>;
 
@@ -157,11 +180,406 @@ 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
+      );
+
+      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,
+        ROLES.OWNER,
+        owner.address
+      );
+
+      expect(hasRole).to.be.true;
+    });
+
+    it('should add a new controller', async () => {
+      const { contract, owner, otherAccount } = fixture;
+      await contract.grantTokenRole(
+        tokenId,
+        ROLES.CONTROLLER,
+        otherAccount.address
+      );
+
+      expect(
+        await contract.hasTokenRole(
+          tokenId,
+          ROLES.CONTROLLER,
+          otherAccount.address
+        )
+      ).to.be.true;
+    });
+
+    it('should add a list of controllers', async () => {
+      const { contract } = fixture;
+      await contract.grantTokenRole(
+        tokenId,
+        ROLES.CONTROLLER,
+        '0x7ED735b7095C05d78dF169F991f2b7f1A1F1A049'
+      );
+      await contract.grantTokenRole(
+        tokenId,
+        ROLES.CONTROLLER,
+        '0x2FEd6Ef3c495922263B403319FA6DDB323DD49E3'
+      );
+
+      expect(
+        await contract.getTokenRoleMembers(tokenId, ROLES.CONTROLLER)
+      ).to.eql([
+        '0x7ED735b7095C05d78dF169F991f2b7f1A1F1A049',
+        '0x2FEd6Ef3c495922263B403319FA6DDB323DD49E3',
+      ]);
+    });
+
+    it('should add a list of owners', async () => {
+      const { contract, owner } = fixture;
+      await contract.grantTokenRole(
+        tokenId,
+        ROLES.OWNER,
+        '0x7ED735b7095C05d78dF169F991f2b7f1A1F1A049'
+      );
+      await contract.grantTokenRole(
+        tokenId,
+        ROLES.OWNER,
+        '0x2FEd6Ef3c495922263B403319FA6DDB323DD49E3'
+      );
+
+      expect(await contract.getTokenRoleMembers(tokenId, ROLES.OWNER)).to.eql([
+        owner.address,
+        '0x7ED735b7095C05d78dF169F991f2b7f1A1F1A049',
+        '0x2FEd6Ef3c495922263B403319FA6DDB323DD49E3',
+      ]);
+    });
+
+    it('should not match the owner role for other account', async () => {
+      const { contract, otherAccount } = fixture;
+      const hasRole = await contract.hasTokenRole(
+        tokenId,
+        ROLES.OWNER,
+        otherAccount.address
+      );
+
+      expect(hasRole).to.be.false;
+    });
+
+    it('should remove an added controller', async () => {
+      const { contract, owner, otherAccount } = fixture;
+      await contract.grantTokenRole(
+        tokenId,
+        ROLES.CONTROLLER,
+        otherAccount.address
+      );
+      await contract.revokeTokenRole(
+        tokenId,
+        ROLES.CONTROLLER,
+        otherAccount.address
+      );
+
+      expect(
+        await contract.hasTokenRole(
+          tokenId,
+          ROLES.CONTROLLER,
+          otherAccount.address
+        )
+      ).to.be.false;
+    });
+
+    it('should transfer the token owner role', async () => {
+      const { contract, owner, otherAccount } = fixture;
+      await contract.transferFrom(owner.address, otherAccount.address, tokenId);
+
+      expect(await contract.ownerOf(tokenId)).to.equal(otherAccount.address);
+      expect(
+        await contract.hasTokenRole(tokenId, ROLES.OWNER, otherAccount.address)
+      ).to.be.true;
+      expect(await contract.hasTokenRole(tokenId, ROLES.OWNER, owner.address))
+        .to.be.false;
+    });
+
+    it('should clean the token controller list after transfer', async () => {
+      const { contract, owner, otherAccount } = fixture;
+      await contract.grantTokenRole(
+        tokenId,
+        ROLES.CONTROLLER,
+        otherAccount.address
+      );
+      await contract.transferFrom(owner.address, otherAccount.address, tokenId);
+
+      expect(await contract.getTokenRoleMembers(tokenId, 1)).to.eql([]);
+    });
+
+    it('should not be able to add address role', async () => {
+      const { contract, owner, otherAccount } = fixture;
+      await expect(
+        contract
+          .connect(otherAccount)
+          .grantTokenRole(tokenId, ROLES.OWNER, otherAccount.address)
+      ).to.be.revertedWith('FleekAccessControl: must have token role');
+
+      await expect(
+        contract
+          .connect(otherAccount)
+          .grantTokenRole(tokenId, ROLES.CONTROLLER, otherAccount.address)
+      ).to.be.revertedWith('FleekAccessControl: must have token role');
+    });
+
+    it('should not be able to remove address role', async () => {
+      const { contract, owner, otherAccount } = fixture;
+      await expect(
+        contract
+          .connect(otherAccount)
+          .revokeTokenRole(tokenId, ROLES.OWNER, otherAccount.address)
+      ).to.be.revertedWith('FleekAccessControl: must have token role');
+
+      await expect(
+        contract
+          .connect(otherAccount)
+          .revokeTokenRole(tokenId, ROLES.CONTROLLER, otherAccount.address)
+      ).to.be.revertedWith('FleekAccessControl: must have token role');
+    });
+
+    it('should be able to add token role after owner role granted', async () => {
+      const { contract, owner, otherAccount } = fixture;
+      await contract.grantTokenRole(tokenId, ROLES.OWNER, otherAccount.address);
+
+      expect(
+        await contract
+          .connect(otherAccount)
+          .grantTokenRole(tokenId, ROLES.CONTROLLER, otherAccount.address)
+      ).to.not.be.reverted;
+    });
+
+    it('should emit event when token role is granted', async () => {
+      const { contract, owner, otherAccount } = fixture;
+      await expect(
+        contract.grantTokenRole(tokenId, ROLES.CONTROLLER, otherAccount.address)
+      )
+        .to.emit(contract, 'TokenRoleGranted')
+        .withArgs(
+          tokenId,
+          ROLES.CONTROLLER,
+          otherAccount.address,
+          owner.address
+        );
+    });
+
+    it('should emit event when token role is revoked', async () => {
+      const { contract, owner, otherAccount } = fixture;
+      await contract.grantTokenRole(
+        tokenId,
+        ROLES.CONTROLLER,
+        otherAccount.address
+      );
+      await expect(
+        contract.revokeTokenRole(
+          tokenId,
+          ROLES.CONTROLLER,
+          otherAccount.address
+        )
+      )
+        .to.emit(contract, 'TokenRoleRevoked')
+        .withArgs(
+          tokenId,
+          ROLES.CONTROLLER,
+          otherAccount.address,
+          owner.address
+        );
+    });
+  });
+
+  describe('Collection Roles', () => {
+    let fixture: Awaited<ReturnType<typeof defaultFixture>>;
+
+    beforeEach(async () => {
+      fixture = await loadFixture(defaultFixture);
+    });
+
+    it('should assign the owner of the contract on contract creation', async () => {
+      const { owner, contract } = fixture;
+
+      expect(await contract.hasCollectionRole(ROLES.OWNER, owner.address)).to.be
+        .true;
+    });
+
+    it('should assign owner role to address', async () => {
+      const { otherAccount, contract } = fixture;
+
+      await contract.grantCollectionRole(ROLES.OWNER, otherAccount.address);
+
+      expect(
+        await contract.hasCollectionRole(ROLES.OWNER, otherAccount.address)
+      ).to.be.true;
+    });
+
+    it('should assign controller role to address', async () => {
+      const { owner, contract } = fixture;
+
+      await contract.grantCollectionRole(ROLES.CONTROLLER, owner.address);
+
+      expect(await contract.hasCollectionRole(ROLES.CONTROLLER, owner.address))
+        .to.be.true;
+    });
+
+    it('should remove an assigned controller', async () => {
+      const { otherAccount, contract } = fixture;
+
+      await contract.grantCollectionRole(ROLES.OWNER, otherAccount.address);
+      await contract.revokeCollectionRole(ROLES.OWNER, otherAccount.address);
+
+      expect(
+        await contract.hasCollectionRole(ROLES.OWNER, otherAccount.address)
+      ).to.be.false;
+    });
+
+    it('should remove an assigned controller', async () => {
+      const { owner, contract } = fixture;
+
+      await contract.grantCollectionRole(ROLES.CONTROLLER, owner.address);
+      await contract.revokeCollectionRole(ROLES.CONTROLLER, owner.address);
+
+      expect(await contract.hasCollectionRole(ROLES.CONTROLLER, owner.address))
+        .to.be.false;
+    });
+
+    it('should fetch the list of controllers', async () => {
+      const { owner, contract } = fixture;
+
+      await contract.grantCollectionRole(ROLES.CONTROLLER, owner.address);
+      await contract.grantCollectionRole(
+        ROLES.CONTROLLER,
+        '0x7ED735b7095C05d78dF169F991f2b7f1A1F1A049'
+      );
+
+      expect(await contract.getCollectionRoleMembers(ROLES.CONTROLLER)).to.eql([
+        owner.address,
+        '0x7ED735b7095C05d78dF169F991f2b7f1A1F1A049',
+      ]);
+    });
+
+    it('should fetch the list of owners', async () => {
+      const { owner, contract, otherAccount } = fixture;
+
+      await contract.grantCollectionRole(ROLES.OWNER, otherAccount.address);
+      await contract.grantCollectionRole(
+        ROLES.OWNER,
+        '0x7ED735b7095C05d78dF169F991f2b7f1A1F1A049'
+      );
+
+      expect(await contract.getCollectionRoleMembers(ROLES.OWNER)).to.eql([
+        owner.address,
+        otherAccount.address,
+        '0x7ED735b7095C05d78dF169F991f2b7f1A1F1A049',
+      ]);
+    });
+
+    it('should not be able to add new owner', async () => {
+      const { otherAccount, contract } = fixture;
+
+      await expect(
+        contract
+          .connect(otherAccount)
+          .grantCollectionRole(ROLES.OWNER, otherAccount.address)
+      ).to.be.revertedWith('FleekAccessControl: must have collection role');
+    });
+
+    it('should not be able to add new controller', async () => {
+      const { otherAccount, contract } = fixture;
+
+      await expect(
+        contract
+          .connect(otherAccount)
+          .grantCollectionRole(ROLES.CONTROLLER, otherAccount.address)
+      ).to.be.revertedWith('FleekAccessControl: must have collection role');
+    });
+
+    it('should be able to add roles after owner being granted', async () => {
+      const { otherAccount, contract } = fixture;
+
+      await contract.grantCollectionRole(ROLES.OWNER, otherAccount.address);
+
+      await expect(
+        contract
+          .connect(otherAccount)
+          .grantCollectionRole(ROLES.CONTROLLER, otherAccount.address)
+      ).to.not.be.reverted;
+      await expect(
+        contract
+          .connect(otherAccount)
+          .revokeCollectionRole(ROLES.CONTROLLER, otherAccount.address)
+      ).to.not.be.reverted;
+    });
+
+    it('should not be able to change roles for controllers', async () => {
+      const { owner, otherAccount, contract } = fixture;
+
+      await contract.grantCollectionRole(
+        ROLES.CONTROLLER,
+        otherAccount.address
+      );
+
+      await expect(
+        contract
+          .connect(otherAccount)
+          .grantCollectionRole(ROLES.OWNER, owner.address)
+      ).to.be.revertedWith('FleekAccessControl: must have collection role');
+      await expect(
+        contract
+          .connect(otherAccount)
+          .revokeCollectionRole(ROLES.OWNER, owner.address)
+      ).to.be.revertedWith('FleekAccessControl: must have collection role');
+    });
+
+    it('should emit event when role is granted', async () => {
+      const { owner, contract, otherAccount } = fixture;
+
+      await expect(
+        contract.grantCollectionRole(ROLES.CONTROLLER, otherAccount.address)
+      )
+        .to.emit(contract, 'CollectionRoleGranted')
+        .withArgs(ROLES.CONTROLLER, otherAccount.address, owner.address);
+    });
+
+    it('should emit event when role is revoked', async () => {
+      const { owner, contract, otherAccount } = fixture;
+
+      await contract.grantCollectionRole(
+        ROLES.CONTROLLER,
+        otherAccount.address
+      );
+
+      await expect(
+        contract.revokeCollectionRole(ROLES.CONTROLLER, otherAccount.address)
+      )
+        .to.emit(contract, 'CollectionRoleRevoked')
+        .withArgs(ROLES.CONTROLLER, otherAccount.address, owner.address);
+    });
   });
 });