diff --git a/contracts/contracts/FleekAccessControl.sol b/contracts/contracts/FleekAccessControl.sol index 2c7c389..8b5cf4f 100644 --- a/contracts/contracts/FleekAccessControl.sol +++ b/contracts/contracts/FleekAccessControl.sol @@ -5,172 +5,170 @@ pragma solidity ^0.8.7; import "@openzeppelin/contracts/utils/Counters.sol"; import "@openzeppelin/contracts-upgradeable/proxy/utils/Initializable.sol"; +error MustHaveCollectionRole(uint8 role); +error MustHaveTokenRole(uint256 tokenId, uint8 role); +error MustHaveAtLeastOneOwner(); +error RoleAlreadySet(); + contract FleekAccessControl is Initializable { using Counters for Counters.Counter; - enum Roles { - Owner, + /** + * @dev All available collection roles. + */ + enum CollectionRoles { + Owner + } + + /** + * @dev All available token roles. + */ + enum TokenRoles { Controller } - event TokenRoleGranted(uint256 indexed tokenId, Roles indexed role, address indexed toAddress, address byAddress); - event TokenRoleRevoked(uint256 indexed tokenId, Roles indexed role, address indexed toAddress, address byAddress); - event CollectionRoleGranted(Roles indexed role, address indexed toAddress, address byAddress); - event CollectionRoleRevoked(Roles indexed role, address indexed toAddress, address byAddress); + /** + * @dev Emitted when a token role is changed. + */ + event TokenRoleChanged( + uint256 indexed tokenId, + TokenRoles indexed role, + address indexed toAddress, + bool status, + address byAddress + ); - struct Role { - mapping(address => uint256) indexes; - address[] members; - } + /** + * @dev Emitted when token roles version is increased and all token roles are cleared. + */ + event TokenRolesCleared(uint256 indexed tokenId, address byAddress); - Counters.Counter private _collectionRolesVersion; - // _collectionRoles[version][role] - mapping(uint256 => mapping(Roles => Role)) private _collectionRoles; + /** + * @dev Emitted when a collection role is changed. + */ + event CollectionRoleChanged( + CollectionRoles indexed role, + address indexed toAddress, + bool status, + address byAddress + ); + /** + * @dev _collectionRolesCounter[role] is the number of addresses that have the role. + * This is prevent Owner role to go to 0. + */ + mapping(CollectionRoles => Counters.Counter) private _collectionRolesCounter; + + /** + * @dev _collectionRoles[role][address] is the mapping of addresses that have the role. + */ + mapping(CollectionRoles => mapping(address => bool)) private _collectionRoles; + + /** + * @dev _tokenRolesVersion[tokenId] is the version of the token roles. + * The version is incremented every time the token roles are cleared. + * Should be incremented every token transfer. + */ mapping(uint256 => Counters.Counter) private _tokenRolesVersion; - // _tokenRoles[tokenId][version][role] - mapping(uint256 => mapping(uint256 => mapping(Roles => Role))) private _tokenRoles; + + /** + * @dev _tokenRoles[tokenId][version][role][address] is the mapping of addresses that have the role. + */ + mapping(uint256 => mapping(uint256 => mapping(TokenRoles => mapping(address => bool)))) private _tokenRoles; /** * @dev Initializes the contract by granting the `Owner` role to the deployer. */ function __FleekAccessControl_init() internal onlyInitializing { - _grantCollectionRole(Roles.Owner, msg.sender); + _grantCollectionRole(CollectionRoles.Owner, msg.sender); } /** * @dev Checks if the `msg.sender` has a certain role. */ - modifier requireCollectionRole(Roles role) { - require( - hasCollectionRole(role, msg.sender) || hasCollectionRole(Roles.Owner, msg.sender), - "FleekAccessControl: must have collection role" - ); - _; + function _requireCollectionRole(CollectionRoles role) internal view { + if (!hasCollectionRole(role, msg.sender)) revert MustHaveCollectionRole(uint8(role)); } /** * @dev Checks if the `msg.sender` has the `Token` role for a certain `tokenId`. */ - modifier requireTokenRole(uint256 tokenId, Roles role) { - require( - hasTokenRole(tokenId, role, msg.sender) || hasTokenRole(tokenId, Roles.Owner, msg.sender), - "FleekAccessControl: must have token role" - ); - _; + function _requireTokenRole(uint256 tokenId, TokenRoles role) internal view { + if (!hasTokenRole(tokenId, role, msg.sender)) revert MustHaveTokenRole(tokenId, uint8(role)); } /** * @dev Returns `True` if a certain address has the collection role. */ - function hasCollectionRole(Roles role, address account) public view returns (bool) { - uint256 currentVersion = _collectionRolesVersion.current(); - - return _collectionRoles[currentVersion][role].indexes[account] != 0; + function hasCollectionRole(CollectionRoles role, address account) public view returns (bool) { + return _collectionRoles[role][account]; } /** * @dev Returns `True` if a certain address has the token role. */ - function hasTokenRole(uint256 tokenId, Roles role, address account) public view returns (bool) { + function hasTokenRole(uint256 tokenId, TokenRoles role, address account) public view returns (bool) { uint256 currentVersion = _tokenRolesVersion[tokenId].current(); - return _tokenRoles[tokenId][currentVersion][role].indexes[account] != 0; - } - - /** - * @dev Returns an array of addresses that all have the collection role. - */ - function getCollectionRoleMembers(Roles role) public view returns (address[] memory) { - uint256 currentVersion = _collectionRolesVersion.current(); - return _collectionRoles[currentVersion][role].members; - } - - /** - * @dev Returns an array of addresses that all have the same token role for a certain tokenId. - */ - function getTokenRoleMembers(uint256 tokenId, Roles role) public view returns (address[] memory) { - uint256 currentVersion = _tokenRolesVersion[tokenId].current(); - return _tokenRoles[tokenId][currentVersion][role].members; + return _tokenRoles[tokenId][currentVersion][role][account]; } /** * @dev Grants the collection role to an address. */ - function _grantCollectionRole(Roles role, address account) internal { - uint256 currentVersion = _collectionRolesVersion.current(); - _grantRole(_collectionRoles[currentVersion][role], account); - emit CollectionRoleGranted(role, account, msg.sender); + function _grantCollectionRole(CollectionRoles role, address account) internal { + if (hasCollectionRole(role, account)) revert RoleAlreadySet(); + + _collectionRoles[role][account] = true; + _collectionRolesCounter[role].increment(); + + emit CollectionRoleChanged(role, account, true, msg.sender); } /** * @dev Revokes the collection role of an address. */ - function _revokeCollectionRole(Roles role, address account) internal { - uint256 currentVersion = _collectionRolesVersion.current(); - _revokeRole(_collectionRoles[currentVersion][role], account); - emit CollectionRoleRevoked(role, account, msg.sender); + function _revokeCollectionRole(CollectionRoles role, address account) internal { + if (!hasCollectionRole(role, account)) revert RoleAlreadySet(); + if (role == CollectionRoles.Owner && _collectionRolesCounter[role].current() == 1) + revert MustHaveAtLeastOneOwner(); + + _collectionRoles[role][account] = false; + _collectionRolesCounter[role].decrement(); + + emit CollectionRoleChanged(role, account, false, msg.sender); } /** * @dev Grants the token role to an address. */ - function _grantTokenRole(uint256 tokenId, Roles role, address account) internal { + function _grantTokenRole(uint256 tokenId, TokenRoles role, address account) internal { + if (hasTokenRole(tokenId, role, account)) revert RoleAlreadySet(); + uint256 currentVersion = _tokenRolesVersion[tokenId].current(); - _grantRole(_tokenRoles[tokenId][currentVersion][role], account); - emit TokenRoleGranted(tokenId, role, account, msg.sender); + _tokenRoles[tokenId][currentVersion][role][account] = true; + + emit TokenRoleChanged(tokenId, role, account, true, msg.sender); } /** * @dev Revokes the token role of an address. */ - function _revokeTokenRole(uint256 tokenId, Roles role, address account) internal { + function _revokeTokenRole(uint256 tokenId, TokenRoles role, address account) internal { + if (!hasTokenRole(tokenId, role, account)) revert RoleAlreadySet(); + uint256 currentVersion = _tokenRolesVersion[tokenId].current(); - _revokeRole(_tokenRoles[tokenId][currentVersion][role], account); - emit TokenRoleRevoked(tokenId, role, account, msg.sender); - } + _tokenRoles[tokenId][currentVersion][role][account] = false; - /** - * @dev Grants a certain role to a certain address. - */ - function _grantRole(Role storage role, address account) internal { - if (role.indexes[account] == 0) { - role.members.push(account); - role.indexes[account] = role.members.length; - } - } - - /** - * @dev Revokes a certain role from a certain address. - */ - 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]; - } - } - - /** - * @dev Clears all token roles for a certain tokenId. - * Should only be used for burning tokens. - */ - function _clearAllTokenRoles(uint256 tokenId) internal { - _tokenRolesVersion[tokenId].increment(); + emit TokenRoleChanged(tokenId, role, account, false, msg.sender); } /** * @dev Clears all token roles for a certain tokenId and grants the owner role to a new address. * Should only be used for transferring tokens. */ - function _clearAllTokenRoles(uint256 tokenId, address newOwner) internal { - _clearAllTokenRoles(tokenId); - _grantTokenRole(tokenId, Roles.Owner, newOwner); + function _clearTokenRoles(uint256 tokenId) internal { + _tokenRolesVersion[tokenId].increment(); + emit TokenRolesCleared(tokenId, msg.sender); } /** diff --git a/contracts/contracts/FleekERC721.sol b/contracts/contracts/FleekERC721.sol index 47fbbb8..9c2e948 100644 --- a/contracts/contracts/FleekERC721.sol +++ b/contracts/contracts/FleekERC721.sol @@ -10,6 +10,7 @@ import "./FleekAccessControl.sol"; import "./util/FleekStrings.sol"; import "./FleekPausable.sol"; +error MustBeTokenOwner(uint256 tokenId); error ThereIsNoTokenMinted(); contract FleekERC721 is Initializable, ERC721Upgradeable, FleekAccessControl, FleekPausable { @@ -117,7 +118,7 @@ contract FleekERC721 is Initializable, ERC721Upgradeable, FleekAccessControl, Fl string memory gitRepository, string memory logo, uint24 color - ) public payable requireCollectionRole(Roles.Owner) returns (uint256) { + ) public payable requireCollectionRole(CollectionRoles.Owner) returns (uint256) { uint256 tokenId = _appIds.current(); _mint(to, tokenId); _appIds.increment(); @@ -206,13 +207,13 @@ contract FleekERC721 is Initializable, ERC721Upgradeable, FleekAccessControl, Fl ) internal virtual override whenNotPaused { if (from != address(0) && to != address(0)) { // Transfer - _clearAllTokenRoles(tokenId, to); + _clearTokenRoles(tokenId); } else if (from == address(0)) { // Mint - _grantTokenRole(tokenId, Roles.Owner, to); + // TODO: set contract owner as controller } else if (to == address(0)) { // Burn - _clearAllTokenRoles(tokenId); + _clearTokenRoles(tokenId); } super._beforeTokenTransfer(from, to, tokenId, batchSize); } @@ -238,7 +239,7 @@ contract FleekERC721 is Initializable, ERC721Upgradeable, FleekAccessControl, Fl function setTokenExternalURL( uint256 tokenId, string memory _tokenExternalURL - ) public virtual requireTokenRole(tokenId, Roles.Controller) { + ) public virtual requireTokenRole(tokenId, TokenRoles.Controller) { _requireMinted(tokenId); _apps[tokenId].externalURL = _tokenExternalURL; emit MetadataUpdate(tokenId, "externalURL", _tokenExternalURL, msg.sender); @@ -258,7 +259,7 @@ contract FleekERC721 is Initializable, ERC721Upgradeable, FleekAccessControl, Fl function setTokenENS( uint256 tokenId, string memory _tokenENS - ) public virtual requireTokenRole(tokenId, Roles.Controller) { + ) public virtual requireTokenRole(tokenId, TokenRoles.Controller) { _requireMinted(tokenId); _apps[tokenId].ENS = _tokenENS; emit MetadataUpdate(tokenId, "ENS", _tokenENS, msg.sender); @@ -278,7 +279,7 @@ contract FleekERC721 is Initializable, ERC721Upgradeable, FleekAccessControl, Fl function setTokenName( uint256 tokenId, string memory _tokenName - ) public virtual requireTokenRole(tokenId, Roles.Controller) { + ) public virtual requireTokenRole(tokenId, TokenRoles.Controller) { _requireMinted(tokenId); _apps[tokenId].name = _tokenName; emit MetadataUpdate(tokenId, "name", _tokenName, msg.sender); @@ -298,7 +299,7 @@ contract FleekERC721 is Initializable, ERC721Upgradeable, FleekAccessControl, Fl function setTokenDescription( uint256 tokenId, string memory _tokenDescription - ) public virtual requireTokenRole(tokenId, Roles.Controller) { + ) public virtual requireTokenRole(tokenId, TokenRoles.Controller) { _requireMinted(tokenId); _apps[tokenId].description = _tokenDescription; emit MetadataUpdate(tokenId, "description", _tokenDescription, msg.sender); @@ -318,7 +319,7 @@ contract FleekERC721 is Initializable, ERC721Upgradeable, FleekAccessControl, Fl function setTokenLogo( uint256 tokenId, string memory _tokenLogo - ) public virtual requireTokenRole(tokenId, Roles.Controller) { + ) public virtual requireTokenRole(tokenId, TokenRoles.Controller) { _requireMinted(tokenId); _apps[tokenId].logo = _tokenLogo; emit MetadataUpdate(tokenId, "logo", _tokenLogo, msg.sender); @@ -338,7 +339,7 @@ contract FleekERC721 is Initializable, ERC721Upgradeable, FleekAccessControl, Fl function setTokenColor( uint256 tokenId, uint24 _tokenColor - ) public virtual requireTokenRole(tokenId, Roles.Controller) { + ) public virtual requireTokenRole(tokenId, TokenRoles.Controller) { _requireMinted(tokenId); _apps[tokenId].color = _tokenColor; emit MetadataUpdate(tokenId, "color", _tokenColor, msg.sender); @@ -476,7 +477,7 @@ contract FleekERC721 is Initializable, ERC721Upgradeable, FleekAccessControl, Fl function setAccessPointContentVerify( string memory apName, bool verified - ) public requireAP(apName) requireTokenRole(_accessPoints[apName].tokenId, Roles.Controller) { + ) public requireAP(apName) requireTokenRole(_accessPoints[apName].tokenId, TokenRoles.Controller) { _accessPoints[apName].contentVerified = verified; emit ChangeAccessPointContentVerify(apName, _accessPoints[apName].tokenId, verified, msg.sender); } @@ -495,7 +496,7 @@ contract FleekERC721 is Initializable, ERC721Upgradeable, FleekAccessControl, Fl function setAccessPointNameVerify( string memory apName, bool verified - ) public requireAP(apName) requireTokenRole(_accessPoints[apName].tokenId, Roles.Controller) { + ) public requireAP(apName) requireTokenRole(_accessPoints[apName].tokenId, TokenRoles.Controller) { _accessPoints[apName].nameVerified = verified; emit ChangeAccessPointNameVerify(apName, _accessPoints[apName].tokenId, verified, msg.sender); } @@ -515,7 +516,7 @@ contract FleekERC721 is Initializable, ERC721Upgradeable, FleekAccessControl, Fl uint256 tokenId, string memory _commitHash, string memory _gitRepository - ) public virtual requireTokenRole(tokenId, Roles.Controller) { + ) public virtual requireTokenRole(tokenId, TokenRoles.Controller) { _requireMinted(tokenId); _apps[tokenId].builds[++_apps[tokenId].currentBuild] = Build(_commitHash, _gitRepository); emit MetadataUpdate(tokenId, "build", [_commitHash, _gitRepository], msg.sender); @@ -529,11 +530,11 @@ contract FleekERC721 is Initializable, ERC721Upgradeable, FleekAccessControl, Fl * Requirements: * * - the tokenId must be minted and valid. - * - the sender must have the `tokenOwner` role. + * - the sender must be the owner of the token. * - the contract must be not paused. * */ - function burn(uint256 tokenId) public virtual requireTokenRole(tokenId, Roles.Owner) { + function burn(uint256 tokenId) public virtual requireTokenOwner(tokenId) { super._burn(tokenId); if (bytes(_apps[tokenId].externalURL).length != 0) { @@ -545,6 +546,30 @@ contract FleekERC721 is Initializable, ERC721Upgradeable, FleekAccessControl, Fl ACCESS CONTROL //////////////////////////////////////////////////////////////*/ + /** + * @dev Requires caller to have a selected collection role. + */ + modifier requireCollectionRole(CollectionRoles role) { + _requireCollectionRole(role); + _; + } + + /** + * @dev Requires caller to have a selected token role. + */ + modifier requireTokenRole(uint256 tokenId, TokenRoles role) { + if (ownerOf(tokenId) != msg.sender) _requireTokenRole(tokenId, role); + _; + } + + /** + * @dev Requires caller to be selected token owner. + */ + modifier requireTokenOwner(uint256 tokenId) { + if (ownerOf(tokenId) != msg.sender) revert MustBeTokenOwner(tokenId); + _; + } + /** * @dev Grants the collection role to an address. * @@ -553,7 +578,10 @@ contract FleekERC721 is Initializable, ERC721Upgradeable, FleekAccessControl, Fl * - the caller should have the collection role. * */ - function grantCollectionRole(Roles role, address account) public whenNotPaused requireCollectionRole(Roles.Owner) { + function grantCollectionRole( + CollectionRoles role, + address account + ) public whenNotPaused requireCollectionRole(CollectionRoles.Owner) { _grantCollectionRole(role, account); } @@ -567,9 +595,9 @@ contract FleekERC721 is Initializable, ERC721Upgradeable, FleekAccessControl, Fl */ function grantTokenRole( uint256 tokenId, - Roles role, + TokenRoles role, address account - ) public whenNotPaused requireTokenRole(tokenId, Roles.Owner) { + ) public whenNotPaused requireTokenOwner(tokenId) { _grantTokenRole(tokenId, role, account); } @@ -581,7 +609,10 @@ contract FleekERC721 is Initializable, ERC721Upgradeable, FleekAccessControl, Fl * - the caller should have the collection role. * */ - function revokeCollectionRole(Roles role, address account) public whenNotPaused requireCollectionRole(Roles.Owner) { + function revokeCollectionRole( + CollectionRoles role, + address account + ) public whenNotPaused requireCollectionRole(CollectionRoles.Owner) { _revokeCollectionRole(role, account); } @@ -595,9 +626,9 @@ contract FleekERC721 is Initializable, ERC721Upgradeable, FleekAccessControl, Fl */ function revokeTokenRole( uint256 tokenId, - Roles role, + TokenRoles role, address account - ) public whenNotPaused requireTokenRole(tokenId, Roles.Owner) { + ) public whenNotPaused requireTokenOwner(tokenId) { _revokeTokenRole(tokenId, role, account); } @@ -615,7 +646,7 @@ contract FleekERC721 is Initializable, ERC721Upgradeable, FleekAccessControl, Fl * - the contract must be not paused. * */ - function pause() public requireCollectionRole(Roles.Controller) { + function pause() public requireCollectionRole(CollectionRoles.Owner) { _pause(); } @@ -628,7 +659,7 @@ contract FleekERC721 is Initializable, ERC721Upgradeable, FleekAccessControl, Fl * - the contract must be paused. * */ - function unpause() public requireCollectionRole(Roles.Controller) { + function unpause() public requireCollectionRole(CollectionRoles.Owner) { _unpause(); } @@ -641,7 +672,7 @@ contract FleekERC721 is Initializable, ERC721Upgradeable, FleekAccessControl, Fl * - the contract must be in the oposite pausable state. * */ - function setPausable(bool pausable) public requireCollectionRole(Roles.Owner) { + function setPausable(bool pausable) public requireCollectionRole(CollectionRoles.Owner) { _setPausable(pausable); } } diff --git a/contracts/test/foundry/FleekERC721/AccessControl.t.sol b/contracts/test/foundry/FleekERC721/AccessControl.t.sol index 7bd6a19..e7d493f 100644 --- a/contracts/test/foundry/FleekERC721/AccessControl.t.sol +++ b/contracts/test/foundry/FleekERC721/AccessControl.t.sol @@ -3,12 +3,21 @@ pragma solidity ^0.8.17; import "./TestBase.sol"; -import {FleekAccessControl} from "contracts/FleekAccessControl.sol"; +import "contracts/FleekAccessControl.sol"; -contract Test_FleekERC721_AccessControl is Test_FleekERC721_Base { +contract Test_FleekERC721_AccessControlAssertions is Test { + function expectRevertWithMustHaveAtLeastOneOwner() internal { + vm.expectRevert(abi.encodeWithSelector(MustHaveAtLeastOneOwner.selector)); + } + + function expectRevertWithRoleAlreadySet() internal { + vm.expectRevert(abi.encodeWithSelector(RoleAlreadySet.selector)); + } +} + +contract Test_FleekERC721_AccessControl is Test_FleekERC721_Base, Test_FleekERC721_AccessControlAssertions { uint256 internal tokenId; address internal collectionOwner = address(1); - address internal collectionController = address(2); address internal tokenOwner = address(3); address internal tokenController = address(4); address internal anyAddress = address(5); @@ -17,42 +26,31 @@ contract Test_FleekERC721_AccessControl is Test_FleekERC721_Base { baseSetUp(); // Set collectionOwner - CuT.grantCollectionRole(FleekAccessControl.Roles.Owner, collectionOwner); - // Set collectionController - CuT.grantCollectionRole(FleekAccessControl.Roles.Controller, collectionController); + CuT.grantCollectionRole(FleekAccessControl.CollectionRoles.Owner, collectionOwner); // Mint to tokenOwner to set tokenOwner mintDefault(tokenOwner); // Set tokenController to minted token vm.prank(tokenOwner); - CuT.grantTokenRole(tokenId, FleekAccessControl.Roles.Controller, tokenController); + CuT.grantTokenRole(tokenId, FleekAccessControl.TokenRoles.Controller, tokenController); } function test_setUp() public { // Check collectionOwner - assertTrue(CuT.hasCollectionRole(FleekAccessControl.Roles.Owner, collectionOwner)); - assertFalse(CuT.hasCollectionRole(FleekAccessControl.Roles.Controller, collectionOwner)); - assertFalse(CuT.hasTokenRole(tokenId, FleekAccessControl.Roles.Owner, collectionOwner)); - assertFalse(CuT.hasTokenRole(tokenId, FleekAccessControl.Roles.Controller, collectionOwner)); - // Check collectionController - assertFalse(CuT.hasCollectionRole(FleekAccessControl.Roles.Owner, collectionController)); - assertTrue(CuT.hasCollectionRole(FleekAccessControl.Roles.Controller, collectionController)); - assertFalse(CuT.hasTokenRole(tokenId, FleekAccessControl.Roles.Owner, collectionController)); - assertFalse(CuT.hasTokenRole(tokenId, FleekAccessControl.Roles.Controller, collectionController)); + assertTrue(CuT.hasCollectionRole(FleekAccessControl.CollectionRoles.Owner, collectionOwner)); + assertFalse(CuT.ownerOf(tokenId) == collectionOwner); + assertFalse(CuT.hasTokenRole(tokenId, FleekAccessControl.TokenRoles.Controller, collectionOwner)); // Check tokenOwner - assertFalse(CuT.hasCollectionRole(FleekAccessControl.Roles.Owner, tokenOwner)); - assertFalse(CuT.hasCollectionRole(FleekAccessControl.Roles.Controller, tokenOwner)); - assertTrue(CuT.hasTokenRole(tokenId, FleekAccessControl.Roles.Owner, tokenOwner)); - assertFalse(CuT.hasTokenRole(tokenId, FleekAccessControl.Roles.Controller, tokenOwner)); + assertFalse(CuT.hasCollectionRole(FleekAccessControl.CollectionRoles.Owner, tokenOwner)); + assertTrue(CuT.ownerOf(tokenId) == tokenOwner); + assertFalse(CuT.hasTokenRole(tokenId, FleekAccessControl.TokenRoles.Controller, tokenOwner)); // Check tokenController - assertFalse(CuT.hasCollectionRole(FleekAccessControl.Roles.Owner, tokenController)); - assertFalse(CuT.hasCollectionRole(FleekAccessControl.Roles.Controller, tokenController)); - assertFalse(CuT.hasTokenRole(tokenId, FleekAccessControl.Roles.Owner, tokenController)); - assertTrue(CuT.hasTokenRole(tokenId, FleekAccessControl.Roles.Controller, tokenController)); + assertFalse(CuT.hasCollectionRole(FleekAccessControl.CollectionRoles.Owner, tokenController)); + assertFalse(CuT.ownerOf(tokenId) == tokenController); + assertTrue(CuT.hasTokenRole(tokenId, FleekAccessControl.TokenRoles.Controller, tokenController)); // Check anyAddress - assertFalse(CuT.hasCollectionRole(FleekAccessControl.Roles.Owner, anyAddress)); - assertFalse(CuT.hasCollectionRole(FleekAccessControl.Roles.Controller, anyAddress)); - assertFalse(CuT.hasTokenRole(tokenId, FleekAccessControl.Roles.Owner, anyAddress)); - assertFalse(CuT.hasTokenRole(tokenId, FleekAccessControl.Roles.Controller, anyAddress)); + assertFalse(CuT.hasCollectionRole(FleekAccessControl.CollectionRoles.Owner, anyAddress)); + assertFalse(CuT.ownerOf(tokenId) == anyAddress); + assertFalse(CuT.hasTokenRole(tokenId, FleekAccessControl.TokenRoles.Controller, anyAddress)); } function test_grantAndRevokeCollectionRole() public { @@ -60,42 +58,34 @@ contract Test_FleekERC721_AccessControl is Test_FleekERC721_Base { // CollectionOwner vm.startPrank(collectionOwner); - CuT.grantCollectionRole(FleekAccessControl.Roles.Controller, randomAddress); - assertTrue(CuT.hasCollectionRole(FleekAccessControl.Roles.Controller, randomAddress)); - CuT.revokeCollectionRole(FleekAccessControl.Roles.Controller, randomAddress); - assertFalse(CuT.hasCollectionRole(FleekAccessControl.Roles.Controller, randomAddress)); - vm.stopPrank(); - - // CollectionController - vm.startPrank(collectionController); - expectRevertWithCollectionRole(); - CuT.grantCollectionRole(FleekAccessControl.Roles.Controller, randomAddress); - expectRevertWithCollectionRole(); - CuT.revokeCollectionRole(FleekAccessControl.Roles.Controller, randomAddress); + CuT.grantCollectionRole(FleekAccessControl.CollectionRoles.Owner, randomAddress); + assertTrue(CuT.hasCollectionRole(FleekAccessControl.CollectionRoles.Owner, randomAddress)); + CuT.revokeCollectionRole(FleekAccessControl.CollectionRoles.Owner, randomAddress); + assertFalse(CuT.hasCollectionRole(FleekAccessControl.CollectionRoles.Owner, randomAddress)); vm.stopPrank(); // TokenOwner vm.startPrank(tokenOwner); - expectRevertWithCollectionRole(); - CuT.grantCollectionRole(FleekAccessControl.Roles.Controller, randomAddress); - expectRevertWithCollectionRole(); - CuT.revokeCollectionRole(FleekAccessControl.Roles.Controller, randomAddress); + expectRevertWithCollectionRole(FleekAccessControl.CollectionRoles.Owner); + CuT.grantCollectionRole(FleekAccessControl.CollectionRoles.Owner, randomAddress); + expectRevertWithCollectionRole(FleekAccessControl.CollectionRoles.Owner); + CuT.revokeCollectionRole(FleekAccessControl.CollectionRoles.Owner, randomAddress); vm.stopPrank(); // TokenController vm.startPrank(tokenController); - expectRevertWithCollectionRole(); - CuT.grantCollectionRole(FleekAccessControl.Roles.Controller, randomAddress); - expectRevertWithCollectionRole(); - CuT.revokeCollectionRole(FleekAccessControl.Roles.Controller, randomAddress); + expectRevertWithCollectionRole(FleekAccessControl.CollectionRoles.Owner); + CuT.grantCollectionRole(FleekAccessControl.CollectionRoles.Owner, randomAddress); + expectRevertWithCollectionRole(FleekAccessControl.CollectionRoles.Owner); + CuT.revokeCollectionRole(FleekAccessControl.CollectionRoles.Owner, randomAddress); vm.stopPrank(); // AnyAddress vm.startPrank(anyAddress); - expectRevertWithCollectionRole(); - CuT.grantCollectionRole(FleekAccessControl.Roles.Controller, randomAddress); - expectRevertWithCollectionRole(); - CuT.revokeCollectionRole(FleekAccessControl.Roles.Controller, randomAddress); + expectRevertWithCollectionRole(FleekAccessControl.CollectionRoles.Owner); + CuT.grantCollectionRole(FleekAccessControl.CollectionRoles.Owner, randomAddress); + expectRevertWithCollectionRole(FleekAccessControl.CollectionRoles.Owner); + CuT.revokeCollectionRole(FleekAccessControl.CollectionRoles.Owner, randomAddress); vm.stopPrank(); } @@ -104,42 +94,34 @@ contract Test_FleekERC721_AccessControl is Test_FleekERC721_Base { // CollectionOwner vm.startPrank(collectionOwner); - expectRevertWithTokenRole(); - CuT.grantTokenRole(tokenId, FleekAccessControl.Roles.Controller, randomAddress); - expectRevertWithTokenRole(); - CuT.revokeTokenRole(tokenId, FleekAccessControl.Roles.Controller, randomAddress); - vm.stopPrank(); - - // CollectionController - vm.startPrank(collectionController); - expectRevertWithTokenRole(); - CuT.grantTokenRole(tokenId, FleekAccessControl.Roles.Controller, randomAddress); - expectRevertWithTokenRole(); - CuT.revokeTokenRole(tokenId, FleekAccessControl.Roles.Controller, randomAddress); + expectRevertWithMustBeTokenOwner(tokenId); + CuT.grantTokenRole(tokenId, FleekAccessControl.TokenRoles.Controller, randomAddress); + expectRevertWithMustBeTokenOwner(tokenId); + CuT.revokeTokenRole(tokenId, FleekAccessControl.TokenRoles.Controller, randomAddress); vm.stopPrank(); // TokenOwner vm.startPrank(tokenOwner); - CuT.grantTokenRole(tokenId, FleekAccessControl.Roles.Controller, randomAddress); - assertTrue(CuT.hasTokenRole(tokenId, FleekAccessControl.Roles.Controller, randomAddress)); - CuT.revokeTokenRole(tokenId, FleekAccessControl.Roles.Controller, randomAddress); - assertFalse(CuT.hasTokenRole(tokenId, FleekAccessControl.Roles.Controller, randomAddress)); + CuT.grantTokenRole(tokenId, FleekAccessControl.TokenRoles.Controller, randomAddress); + assertTrue(CuT.hasTokenRole(tokenId, FleekAccessControl.TokenRoles.Controller, randomAddress)); + CuT.revokeTokenRole(tokenId, FleekAccessControl.TokenRoles.Controller, randomAddress); + assertFalse(CuT.hasTokenRole(tokenId, FleekAccessControl.TokenRoles.Controller, randomAddress)); vm.stopPrank(); // TokenController vm.startPrank(tokenController); - expectRevertWithTokenRole(); - CuT.grantTokenRole(tokenId, FleekAccessControl.Roles.Controller, randomAddress); - expectRevertWithTokenRole(); - CuT.revokeTokenRole(tokenId, FleekAccessControl.Roles.Controller, randomAddress); + expectRevertWithMustBeTokenOwner(tokenId); + CuT.grantTokenRole(tokenId, FleekAccessControl.TokenRoles.Controller, randomAddress); + expectRevertWithMustBeTokenOwner(tokenId); + CuT.revokeTokenRole(tokenId, FleekAccessControl.TokenRoles.Controller, randomAddress); vm.stopPrank(); // AnyAddress vm.startPrank(anyAddress); - expectRevertWithTokenRole(); - CuT.grantTokenRole(tokenId, FleekAccessControl.Roles.Controller, randomAddress); - expectRevertWithTokenRole(); - CuT.revokeTokenRole(tokenId, FleekAccessControl.Roles.Controller, randomAddress); + expectRevertWithMustBeTokenOwner(tokenId); + CuT.grantTokenRole(tokenId, FleekAccessControl.TokenRoles.Controller, randomAddress); + expectRevertWithMustBeTokenOwner(tokenId); + CuT.revokeTokenRole(tokenId, FleekAccessControl.TokenRoles.Controller, randomAddress); vm.stopPrank(); } @@ -151,27 +133,21 @@ contract Test_FleekERC721_AccessControl is Test_FleekERC721_Base { mintDefault(randomAddress); vm.stopPrank(); - // CollectionController - vm.startPrank(collectionController); - expectRevertWithCollectionRole(); - mintDefault(randomAddress); - vm.stopPrank(); - // TokenOwner vm.startPrank(tokenOwner); - expectRevertWithCollectionRole(); + expectRevertWithCollectionRole(FleekAccessControl.CollectionRoles.Owner); mintDefault(randomAddress); vm.stopPrank(); // TokenController vm.startPrank(tokenController); - expectRevertWithCollectionRole(); + expectRevertWithCollectionRole(FleekAccessControl.CollectionRoles.Owner); mintDefault(randomAddress); vm.stopPrank(); // AnyAddress vm.startPrank(anyAddress); - expectRevertWithCollectionRole(); + expectRevertWithCollectionRole(FleekAccessControl.CollectionRoles.Owner); mintDefault(randomAddress); vm.stopPrank(); } @@ -187,12 +163,7 @@ contract Test_FleekERC721_AccessControl is Test_FleekERC721_Base { // ColletionOwner vm.prank(collectionOwner); - expectRevertWithTokenRole(); - CuT.setTokenExternalURL(tokenId, externalURL); - - // CollectionController - vm.prank(collectionController); - expectRevertWithTokenRole(); + expectRevertWithTokenRole(tokenId, FleekAccessControl.TokenRoles.Controller); CuT.setTokenExternalURL(tokenId, externalURL); // TokenOwner @@ -205,7 +176,7 @@ contract Test_FleekERC721_AccessControl is Test_FleekERC721_Base { // AnyAddress vm.prank(anyAddress); - expectRevertWithTokenRole(); + expectRevertWithTokenRole(tokenId, FleekAccessControl.TokenRoles.Controller); CuT.setTokenExternalURL(tokenId, externalURL); } @@ -214,12 +185,7 @@ contract Test_FleekERC721_AccessControl is Test_FleekERC721_Base { // ColletionOwner vm.prank(collectionOwner); - expectRevertWithTokenRole(); - CuT.setTokenENS(tokenId, ens); - - // CollectionController - vm.prank(collectionController); - expectRevertWithTokenRole(); + expectRevertWithTokenRole(tokenId, FleekAccessControl.TokenRoles.Controller); CuT.setTokenENS(tokenId, ens); // TokenOwner @@ -232,7 +198,7 @@ contract Test_FleekERC721_AccessControl is Test_FleekERC721_Base { // AnyAddress vm.prank(anyAddress); - expectRevertWithTokenRole(); + expectRevertWithTokenRole(tokenId, FleekAccessControl.TokenRoles.Controller); CuT.setTokenENS(tokenId, ens); } @@ -241,12 +207,7 @@ contract Test_FleekERC721_AccessControl is Test_FleekERC721_Base { // ColletionOwner vm.prank(collectionOwner); - expectRevertWithTokenRole(); - CuT.setTokenName(tokenId, name); - - // CollectionController - vm.prank(collectionController); - expectRevertWithTokenRole(); + expectRevertWithTokenRole(tokenId, FleekAccessControl.TokenRoles.Controller); CuT.setTokenName(tokenId, name); // TokenOwner @@ -259,7 +220,7 @@ contract Test_FleekERC721_AccessControl is Test_FleekERC721_Base { // AnyAddress vm.prank(anyAddress); - expectRevertWithTokenRole(); + expectRevertWithTokenRole(tokenId, FleekAccessControl.TokenRoles.Controller); CuT.setTokenName(tokenId, name); } @@ -268,12 +229,7 @@ contract Test_FleekERC721_AccessControl is Test_FleekERC721_Base { // ColletionOwner vm.prank(collectionOwner); - expectRevertWithTokenRole(); - CuT.setTokenDescription(tokenId, description); - - // CollectionController - vm.prank(collectionController); - expectRevertWithTokenRole(); + expectRevertWithTokenRole(tokenId, FleekAccessControl.TokenRoles.Controller); CuT.setTokenDescription(tokenId, description); // TokenOwner @@ -286,7 +242,7 @@ contract Test_FleekERC721_AccessControl is Test_FleekERC721_Base { // AnyAddress vm.prank(anyAddress); - expectRevertWithTokenRole(); + expectRevertWithTokenRole(tokenId, FleekAccessControl.TokenRoles.Controller); CuT.setTokenDescription(tokenId, description); } @@ -295,12 +251,7 @@ contract Test_FleekERC721_AccessControl is Test_FleekERC721_Base { // ColletionOwner vm.prank(collectionOwner); - expectRevertWithTokenRole(); - CuT.setTokenLogo(tokenId, logo); - - // CollectionController - vm.prank(collectionController); - expectRevertWithTokenRole(); + expectRevertWithTokenRole(tokenId, FleekAccessControl.TokenRoles.Controller); CuT.setTokenLogo(tokenId, logo); // TokenOwner @@ -313,7 +264,7 @@ contract Test_FleekERC721_AccessControl is Test_FleekERC721_Base { // AnyAddress vm.prank(anyAddress); - expectRevertWithTokenRole(); + expectRevertWithTokenRole(tokenId, FleekAccessControl.TokenRoles.Controller); CuT.setTokenLogo(tokenId, logo); } @@ -322,12 +273,7 @@ contract Test_FleekERC721_AccessControl is Test_FleekERC721_Base { // ColletionOwner vm.prank(collectionOwner); - expectRevertWithTokenRole(); - CuT.setTokenColor(tokenId, color); - - // CollectionController - vm.prank(collectionController); - expectRevertWithTokenRole(); + expectRevertWithTokenRole(tokenId, FleekAccessControl.TokenRoles.Controller); CuT.setTokenColor(tokenId, color); // TokenOwner @@ -340,7 +286,7 @@ contract Test_FleekERC721_AccessControl is Test_FleekERC721_Base { // AnyAddress vm.prank(anyAddress); - expectRevertWithTokenRole(); + expectRevertWithTokenRole(tokenId, FleekAccessControl.TokenRoles.Controller); CuT.setTokenColor(tokenId, color); } @@ -350,12 +296,7 @@ contract Test_FleekERC721_AccessControl is Test_FleekERC721_Base { // ColletionOwner vm.prank(collectionOwner); - expectRevertWithTokenRole(); - CuT.setTokenLogoAndColor(tokenId, logo, color); - - // CollectionController - vm.prank(collectionController); - expectRevertWithTokenRole(); + expectRevertWithTokenRole(tokenId, FleekAccessControl.TokenRoles.Controller); CuT.setTokenLogoAndColor(tokenId, logo, color); // TokenOwner @@ -368,7 +309,7 @@ contract Test_FleekERC721_AccessControl is Test_FleekERC721_Base { // AnyAddress vm.prank(anyAddress); - expectRevertWithTokenRole(); + expectRevertWithTokenRole(tokenId, FleekAccessControl.TokenRoles.Controller); CuT.setTokenLogoAndColor(tokenId, logo, color); } @@ -378,12 +319,7 @@ contract Test_FleekERC721_AccessControl is Test_FleekERC721_Base { // ColletionOwner vm.prank(collectionOwner); - expectRevertWithTokenRole(); - CuT.setTokenBuild(tokenId, commitHash, gitRepository); - - // CollectionController - vm.prank(collectionController); - expectRevertWithTokenRole(); + expectRevertWithTokenRole(tokenId, FleekAccessControl.TokenRoles.Controller); CuT.setTokenBuild(tokenId, commitHash, gitRepository); // TokenOwner @@ -396,29 +332,24 @@ contract Test_FleekERC721_AccessControl is Test_FleekERC721_Base { // AnyAddress vm.prank(anyAddress); - expectRevertWithTokenRole(); + expectRevertWithTokenRole(tokenId, FleekAccessControl.TokenRoles.Controller); CuT.setTokenBuild(tokenId, commitHash, gitRepository); } function test_burn() public { // ColletionOwner vm.prank(collectionOwner); - expectRevertWithTokenRole(); - CuT.burn(tokenId); - - // CollectionController - vm.prank(collectionController); - expectRevertWithTokenRole(); + expectRevertWithMustBeTokenOwner(tokenId); CuT.burn(tokenId); // TokenController vm.prank(tokenController); - expectRevertWithTokenRole(); + expectRevertWithMustBeTokenOwner(tokenId); CuT.burn(tokenId); // AnyAddress vm.prank(anyAddress); - expectRevertWithTokenRole(); + expectRevertWithMustBeTokenOwner(tokenId); CuT.burn(tokenId); // TokenOwner @@ -433,33 +364,27 @@ contract Test_FleekERC721_AccessControl is Test_FleekERC721_Base { CuT.unpause(); vm.stopPrank(); - // CollectionController - vm.startPrank(collectionController); - CuT.pause(); - CuT.unpause(); - vm.stopPrank(); - // TokenOwner vm.startPrank(tokenOwner); - expectRevertWithCollectionRole(); + expectRevertWithCollectionRole(FleekAccessControl.CollectionRoles.Owner); CuT.pause(); - expectRevertWithCollectionRole(); + expectRevertWithCollectionRole(FleekAccessControl.CollectionRoles.Owner); CuT.unpause(); vm.stopPrank(); // TokenController vm.startPrank(tokenController); - expectRevertWithCollectionRole(); + expectRevertWithCollectionRole(FleekAccessControl.CollectionRoles.Owner); CuT.pause(); - expectRevertWithCollectionRole(); + expectRevertWithCollectionRole(FleekAccessControl.CollectionRoles.Owner); CuT.unpause(); vm.stopPrank(); // AnyAddress vm.startPrank(anyAddress); - expectRevertWithCollectionRole(); + expectRevertWithCollectionRole(FleekAccessControl.CollectionRoles.Owner); CuT.pause(); - expectRevertWithCollectionRole(); + expectRevertWithCollectionRole(FleekAccessControl.CollectionRoles.Owner); CuT.unpause(); vm.stopPrank(); } @@ -469,24 +394,43 @@ contract Test_FleekERC721_AccessControl is Test_FleekERC721_Base { vm.prank(collectionOwner); CuT.setPausable(false); - // CollectionController - vm.prank(collectionController); - expectRevertWithCollectionRole(); - CuT.setPausable(true); - // TokenOwner vm.prank(tokenOwner); - expectRevertWithCollectionRole(); + expectRevertWithCollectionRole(FleekAccessControl.CollectionRoles.Owner); CuT.setPausable(true); // TokenController vm.prank(tokenController); - expectRevertWithCollectionRole(); + expectRevertWithCollectionRole(FleekAccessControl.CollectionRoles.Owner); CuT.setPausable(true); // AnyAddress vm.prank(anyAddress); - expectRevertWithCollectionRole(); + expectRevertWithCollectionRole(FleekAccessControl.CollectionRoles.Owner); CuT.setPausable(true); } + + function test_cannotHaveLessThanOneCollectionOwner() public { + CuT.revokeCollectionRole(FleekAccessControl.CollectionRoles.Owner, collectionOwner); + expectRevertWithMustHaveAtLeastOneOwner(); + CuT.revokeCollectionRole(FleekAccessControl.CollectionRoles.Owner, deployer); + } + + function test_cannotGrantRoleAlreadyGaranted() public { + expectRevertWithRoleAlreadySet(); + CuT.grantCollectionRole(FleekAccessControl.CollectionRoles.Owner, collectionOwner); + + expectRevertWithRoleAlreadySet(); + vm.prank(tokenOwner); + CuT.grantTokenRole(tokenId, FleekAccessControl.TokenRoles.Controller, tokenController); + } + + function test_cannotRevokeRoleAlreadyRevoked() public { + expectRevertWithRoleAlreadySet(); + CuT.revokeCollectionRole(FleekAccessControl.CollectionRoles.Owner, anyAddress); + + expectRevertWithRoleAlreadySet(); + vm.prank(tokenOwner); + CuT.revokeTokenRole(tokenId, FleekAccessControl.TokenRoles.Controller, anyAddress); + } } diff --git a/contracts/test/foundry/FleekERC721/AccessPoints.t.sol b/contracts/test/foundry/FleekERC721/AccessPoints.t.sol index 7e798e8..df292ab 100644 --- a/contracts/test/foundry/FleekERC721/AccessPoints.t.sol +++ b/contracts/test/foundry/FleekERC721/AccessPoints.t.sol @@ -109,13 +109,13 @@ contract Test_FleekERC721_AccessPoint is Test_FleekERC721_Base { CuT.addAccessPoint(tokenId, accessPointName); vm.startPrank(randomAddress); - expectRevertWithTokenRole(); + expectRevertWithTokenRole(tokenId, FleekAccessControl.TokenRoles.Controller); CuT.setAccessPointNameVerify(accessPointName, true); - expectRevertWithTokenRole(); + expectRevertWithTokenRole(tokenId, FleekAccessControl.TokenRoles.Controller); CuT.setAccessPointContentVerify(accessPointName, true); vm.stopPrank(); - CuT.grantTokenRole(tokenId, FleekAccessControl.Roles.Controller, randomAddress); + CuT.grantTokenRole(tokenId, FleekAccessControl.TokenRoles.Controller, randomAddress); vm.startPrank(randomAddress); CuT.setAccessPointNameVerify(accessPointName, true); diff --git a/contracts/test/foundry/FleekERC721/Burn.t.sol b/contracts/test/foundry/FleekERC721/Burn.t.sol index bd35e4b..fd05447 100644 --- a/contracts/test/foundry/FleekERC721/Burn.t.sol +++ b/contracts/test/foundry/FleekERC721/Burn.t.sol @@ -20,21 +20,21 @@ contract Test_FleekERC721_Burn is Test_FleekERC721_Base { function testFuzz_cannotBurnAsNotOwner(address account) public { vm.assume(account != deployer); vm.prank(account); - expectRevertWithTokenRole(); + expectRevertWithMustBeTokenOwner(tokenId); CuT.burn(tokenId); } function testFuzz_cannotBurnAsController(address account) public { vm.assume(account != deployer); - CuT.grantTokenRole(tokenId, FleekAccessControl.Roles.Controller, account); + CuT.grantTokenRole(tokenId, FleekAccessControl.TokenRoles.Controller, account); vm.prank(account); - expectRevertWithTokenRole(); + expectRevertWithMustBeTokenOwner(tokenId); CuT.burn(tokenId); } function testFuzz_cannotBurnInexistentToken(uint256 _tokenId) public { vm.assume(_tokenId != tokenId); - expectRevertWithTokenRole(); // Token role is tested first before if token exists + expectRevertWithInvalidTokenId(); CuT.burn(_tokenId); } } diff --git a/contracts/test/foundry/FleekERC721/Deploy.t.sol b/contracts/test/foundry/FleekERC721/Deploy.t.sol index 67ceee4..323ba7d 100644 --- a/contracts/test/foundry/FleekERC721/Deploy.t.sol +++ b/contracts/test/foundry/FleekERC721/Deploy.t.sol @@ -19,7 +19,7 @@ contract Test_FleekERC721_Deploy is Test_FleekERC721_Base { } function test_deployerShouldBeCollectionOwner() public { - assertTrue(CuT.hasCollectionRole(FleekAccessControl.Roles.Owner, deployer)); + assertTrue(CuT.hasCollectionRole(FleekAccessControl.CollectionRoles.Owner, deployer)); } function testFuzz_nameAndSymbol(string memory _name, string memory _symbol) public { diff --git a/contracts/test/foundry/FleekERC721/Pausable.t.sol b/contracts/test/foundry/FleekERC721/Pausable.t.sol index 1a91310..a4bfdb2 100644 --- a/contracts/test/foundry/FleekERC721/Pausable.t.sol +++ b/contracts/test/foundry/FleekERC721/Pausable.t.sol @@ -106,15 +106,15 @@ contract Test_FleekERC721_Pausable is Test_FleekERC721_Base, Test_FleekERC721_Pa CuT.removeAccessPoint("accesspoint.com"); expectRevertWithContractIsPaused(); - CuT.grantCollectionRole(FleekAccessControl.Roles.Controller, randomAddress); + CuT.grantCollectionRole(FleekAccessControl.CollectionRoles.Owner, randomAddress); expectRevertWithContractIsPaused(); - CuT.revokeCollectionRole(FleekAccessControl.Roles.Controller, randomAddress); + CuT.revokeCollectionRole(FleekAccessControl.CollectionRoles.Owner, randomAddress); expectRevertWithContractIsPaused(); - CuT.grantTokenRole(tokenId, FleekAccessControl.Roles.Controller, randomAddress); + CuT.grantTokenRole(tokenId, FleekAccessControl.TokenRoles.Controller, randomAddress); expectRevertWithContractIsPaused(); - CuT.revokeTokenRole(tokenId, FleekAccessControl.Roles.Controller, randomAddress); + CuT.revokeTokenRole(tokenId, FleekAccessControl.TokenRoles.Controller, randomAddress); } } diff --git a/contracts/test/foundry/FleekERC721/TestBase.sol b/contracts/test/foundry/FleekERC721/TestBase.sol index 240541b..b778b99 100644 --- a/contracts/test/foundry/FleekERC721/TestBase.sol +++ b/contracts/test/foundry/FleekERC721/TestBase.sol @@ -3,16 +3,20 @@ pragma solidity ^0.8.17; import "forge-std/Test.sol"; -import {FleekERC721} from "contracts/FleekERC721.sol"; +import "contracts/FleekERC721.sol"; import {TestConstants} from "./Constants.sol"; abstract contract Test_FleekERC721_Assertions is Test { - function expectRevertWithTokenRole() public { - vm.expectRevert("FleekAccessControl: must have token role"); + function expectRevertWithTokenRole(uint256 tokenId, FleekAccessControl.TokenRoles role) public { + vm.expectRevert(abi.encodeWithSelector(MustHaveTokenRole.selector, tokenId, uint8(role))); } - function expectRevertWithCollectionRole() public { - vm.expectRevert("FleekAccessControl: must have collection role"); + function expectRevertWithCollectionRole(FleekAccessControl.CollectionRoles role) public { + vm.expectRevert(abi.encodeWithSelector(MustHaveCollectionRole.selector, uint8(role))); + } + + function expectRevertWithMustBeTokenOwner(uint256 tokenId) public { + vm.expectRevert(abi.encodeWithSelector(MustBeTokenOwner.selector, tokenId)); } function expectRevertWithAPAlreadyExists() public { diff --git a/contracts/test/hardhat/contracts/FleekERC721/collection-roles.t.ts b/contracts/test/hardhat/contracts/FleekERC721/collection-roles.t.ts index 64b0a1b..f4f5ac2 100644 --- a/contracts/test/hardhat/contracts/FleekERC721/collection-roles.t.ts +++ b/contracts/test/hardhat/contracts/FleekERC721/collection-roles.t.ts @@ -1,8 +1,8 @@ import { loadFixture } from '@nomicfoundation/hardhat-network-helpers'; import { expect } from 'chai'; -import { TestConstants, Fixtures } from './helpers'; +import { TestConstants, Fixtures, Errors } from './helpers'; -const { Roles } = TestConstants; +const { CollectionRoles } = TestConstants; describe('FleekERC721.CollectionRoles', () => { let fixture: Awaited>; @@ -14,77 +14,72 @@ describe('FleekERC721.CollectionRoles', () => { 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; + expect( + await contract.hasCollectionRole(CollectionRoles.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); + await contract.grantCollectionRole( + CollectionRoles.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; + expect( + await contract.hasCollectionRole( + CollectionRoles.Owner, + otherAccount.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' + CollectionRoles.Owner, + otherAccount.address + ); + await contract.revokeCollectionRole( + CollectionRoles.Owner, + otherAccount.address ); - expect(await contract.getCollectionRoleMembers(Roles.Controller)).to.eql([ - owner.address, - '0x7ED735b7095C05d78dF169F991f2b7f1A1F1A049', - ]); + expect( + await contract.hasCollectionRole( + CollectionRoles.Owner, + otherAccount.address + ) + ).to.be.false; }); 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, + CollectionRoles.Owner, + otherAccount.address + ); + await contract.grantCollectionRole( + CollectionRoles.Owner, '0x7ED735b7095C05d78dF169F991f2b7f1A1F1A049' ); - expect(await contract.getCollectionRoleMembers(Roles.Owner)).to.eql([ - owner.address, - otherAccount.address, - '0x7ED735b7095C05d78dF169F991f2b7f1A1F1A049', - ]); + expect( + await contract.hasCollectionRole( + CollectionRoles.Owner, + otherAccount.address + ) + ).to.be.true; + + expect( + await contract.hasCollectionRole( + CollectionRoles.Owner, + '0x7ED735b7095C05d78dF169F991f2b7f1A1F1A049' + ) + ).to.be.true; }); it('should not be able to add new owner', async () => { @@ -93,73 +88,91 @@ describe('FleekERC721.CollectionRoles', () => { 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'); + .grantCollectionRole(CollectionRoles.Owner, otherAccount.address) + ) + .to.be.revertedWithCustomError(contract, Errors.MustHaveCollectionRole) + .withArgs(CollectionRoles.Owner); }); it('should be able to add roles after owner being granted', async () => { const { otherAccount, contract } = fixture; - await contract.grantCollectionRole(Roles.Owner, otherAccount.address); + await contract.grantCollectionRole( + CollectionRoles.Owner, + otherAccount.address + ); await expect( contract .connect(otherAccount) - .grantCollectionRole(Roles.Controller, otherAccount.address) + .grantCollectionRole( + CollectionRoles.Owner, + '0x7ED735b7095C05d78dF169F991f2b7f1A1F1A049' + ) ).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) + contract.grantCollectionRole(CollectionRoles.Owner, otherAccount.address) ) - .to.emit(contract, 'CollectionRoleGranted') - .withArgs(Roles.Controller, otherAccount.address, owner.address); + .to.emit(contract, 'CollectionRoleChanged') + .withArgs( + CollectionRoles.Owner, + otherAccount.address, + true, + owner.address + ); }); it('should emit event when role is revoked', async () => { const { owner, contract, otherAccount } = fixture; - await contract.grantCollectionRole(Roles.Controller, otherAccount.address); + await contract.grantCollectionRole( + CollectionRoles.Owner, + otherAccount.address + ); await expect( - contract.revokeCollectionRole(Roles.Controller, otherAccount.address) + contract.revokeCollectionRole(CollectionRoles.Owner, otherAccount.address) ) - .to.emit(contract, 'CollectionRoleRevoked') - .withArgs(Roles.Controller, otherAccount.address, owner.address); + .to.emit(contract, 'CollectionRoleChanged') + .withArgs( + CollectionRoles.Owner, + otherAccount.address, + false, + owner.address + ); + }); + + it('should not be able to grant role if already granted', async () => { + const { otherAccount, contract } = fixture; + + await contract.grantCollectionRole( + CollectionRoles.Owner, + otherAccount.address + ); + + await expect( + contract.grantCollectionRole(CollectionRoles.Owner, otherAccount.address) + ).to.be.revertedWithCustomError(contract, Errors.RoleAlreadySet); + }); + + it('should not be able to revoke role if not granted', async () => { + const { otherAccount, contract } = fixture; + + await expect( + contract.revokeCollectionRole(CollectionRoles.Owner, otherAccount.address) + ).to.be.revertedWithCustomError(contract, Errors.RoleAlreadySet); + }); + + it('should not be able to remove all collection owners', async () => { + const { owner, contract } = fixture; + + await expect( + contract.revokeCollectionRole(CollectionRoles.Owner, owner.address) + ).to.be.revertedWithCustomError(contract, Errors.MustHaveAtLeastOneOwner); }); }); diff --git a/contracts/test/hardhat/contracts/FleekERC721/helpers/constants.ts b/contracts/test/hardhat/contracts/FleekERC721/helpers/constants.ts index ca1357e..d277b98 100644 --- a/contracts/test/hardhat/contracts/FleekERC721/helpers/constants.ts +++ b/contracts/test/hardhat/contracts/FleekERC721/helpers/constants.ts @@ -1,7 +1,9 @@ export const TestConstants = Object.freeze({ - Roles: { + CollectionRoles: { Owner: 0, - Controller: 1, + }, + TokenRoles: { + Controller: 0, }, MintParams: { name: 'Fleek Test App', diff --git a/contracts/test/hardhat/contracts/FleekERC721/helpers/errors.ts b/contracts/test/hardhat/contracts/FleekERC721/helpers/errors.ts index 033099a..78bd847 100644 --- a/contracts/test/hardhat/contracts/FleekERC721/helpers/errors.ts +++ b/contracts/test/hardhat/contracts/FleekERC721/helpers/errors.ts @@ -1,4 +1,9 @@ export const Errors = Object.freeze({ + MustHaveCollectionRole: 'MustHaveCollectionRole', + MustHaveTokenRole: 'MustHaveTokenRole', + MustHaveAtLeastOneOwner: 'MustHaveAtLeastOneOwner', + RoleAlreadySet: 'RoleAlreadySet', + MustBeTokenOwner: 'MustBeTokenOwner', ContractIsPaused: 'ContractIsPaused', ContractIsNotPaused: 'ContractIsNotPaused', ContractIsNotPausable: 'ContractIsNotPausable', diff --git a/contracts/test/hardhat/contracts/FleekERC721/minting.t.ts b/contracts/test/hardhat/contracts/FleekERC721/minting.t.ts index 34f0e0d..3b77aaf 100644 --- a/contracts/test/hardhat/contracts/FleekERC721/minting.t.ts +++ b/contracts/test/hardhat/contracts/FleekERC721/minting.t.ts @@ -1,9 +1,9 @@ import { loadFixture } from '@nomicfoundation/hardhat-network-helpers'; import { expect } from 'chai'; -import { TestConstants, Fixtures } from './helpers'; +import { TestConstants, Fixtures, Errors } from './helpers'; import { ethers } from 'hardhat'; -const { MintParams, Roles } = TestConstants; +const { MintParams, CollectionRoles } = TestConstants; describe('FleekERC721.Minting', () => { it('should be able to mint a new token', async () => { @@ -42,7 +42,9 @@ describe('FleekERC721.Minting', () => { MintParams.logo, MintParams.color ) - ).to.be.revertedWith('FleekAccessControl: must have collection role'); + ) + .to.be.revertedWithCustomError(contract, Errors.MustHaveCollectionRole) + .withArgs(CollectionRoles.Owner); }); it('should have address to as owner', async () => { @@ -65,12 +67,6 @@ describe('FleekERC721.Minting', () => { 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; }); }); diff --git a/contracts/test/hardhat/contracts/FleekERC721/pausable.t.ts b/contracts/test/hardhat/contracts/FleekERC721/pausable.t.ts index b63714a..22438d4 100644 --- a/contracts/test/hardhat/contracts/FleekERC721/pausable.t.ts +++ b/contracts/test/hardhat/contracts/FleekERC721/pausable.t.ts @@ -2,7 +2,7 @@ import { loadFixture } from '@nomicfoundation/hardhat-network-helpers'; import { expect } from 'chai'; import { TestConstants, Fixtures, Errors } from './helpers'; -const { MintParams, Roles } = TestConstants; +const { MintParams, CollectionRoles, TokenRoles } = TestConstants; describe('FleekERC721.Pausable', () => { let fixture: Awaited>; @@ -134,19 +134,27 @@ describe('FleekERC721.Pausable', () => { ).to.be.revertedWithCustomError(contract, Errors.ContractIsPaused); await expect( - contract.grantCollectionRole(Roles.Controller, otherAccount.address) + contract.grantCollectionRole(CollectionRoles.Owner, otherAccount.address) ).to.be.revertedWithCustomError(contract, Errors.ContractIsPaused); await expect( - contract.revokeCollectionRole(Roles.Controller, otherAccount.address) + contract.revokeCollectionRole(CollectionRoles.Owner, otherAccount.address) ).to.be.revertedWithCustomError(contract, Errors.ContractIsPaused); await expect( - contract.grantTokenRole(Roles.Controller, tokenId, otherAccount.address) + contract.grantTokenRole( + CollectionRoles.Owner, + tokenId, + otherAccount.address + ) ).to.be.revertedWithCustomError(contract, Errors.ContractIsPaused); await expect( - contract.revokeTokenRole(Roles.Controller, tokenId, otherAccount.address) + contract.revokeTokenRole( + CollectionRoles.Owner, + tokenId, + otherAccount.address + ) ).to.be.revertedWithCustomError(contract, Errors.ContractIsPaused); }); }); diff --git a/contracts/test/hardhat/contracts/FleekERC721/token-roles.t.ts b/contracts/test/hardhat/contracts/FleekERC721/token-roles.t.ts index ff9c20e..3bdeebf 100644 --- a/contracts/test/hardhat/contracts/FleekERC721/token-roles.t.ts +++ b/contracts/test/hardhat/contracts/FleekERC721/token-roles.t.ts @@ -1,8 +1,8 @@ import { loadFixture } from '@nomicfoundation/hardhat-network-helpers'; import { expect } from 'chai'; -import { TestConstants, Fixtures, parseTokenURI } from './helpers'; +import { TestConstants, Fixtures, parseTokenURI, Errors } from './helpers'; -const { Roles } = TestConstants; +const { TokenRoles } = TestConstants; describe('FleekERC721.TokenRoles', () => { let fixture: Awaited>; @@ -19,27 +19,23 @@ describe('FleekERC721.TokenRoles', () => { it('should match the owner role for minter', async () => { const { contract, owner, tokenId } = fixture; - const hasRole = await contract.hasTokenRole( - tokenId, - Roles.Owner, - owner.address - ); + const tokenOwner = await contract.ownerOf(tokenId); - expect(hasRole).to.be.true; + expect(tokenOwner).to.be.equal(owner.address); }); it('should add a new controller', async () => { const { contract, owner, otherAccount, tokenId } = fixture; await contract.grantTokenRole( tokenId, - Roles.Controller, + TokenRoles.Controller, otherAccount.address ); expect( await contract.hasTokenRole( tokenId, - Roles.Controller, + TokenRoles.Controller, otherAccount.address ) ).to.be.true; @@ -49,71 +45,55 @@ describe('FleekERC721.TokenRoles', () => { const { contract, tokenId } = fixture; await contract.grantTokenRole( tokenId, - Roles.Controller, + TokenRoles.Controller, '0x7ED735b7095C05d78dF169F991f2b7f1A1F1A049' ); await contract.grantTokenRole( tokenId, - Roles.Controller, + TokenRoles.Controller, '0x2FEd6Ef3c495922263B403319FA6DDB323DD49E3' ); expect( - await contract.getTokenRoleMembers(tokenId, Roles.Controller) - ).to.eql([ - '0x7ED735b7095C05d78dF169F991f2b7f1A1F1A049', - '0x2FEd6Ef3c495922263B403319FA6DDB323DD49E3', - ]); - }); - - it('should add a list of owners', async () => { - const { contract, owner, tokenId } = 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', - ]); + await contract.hasTokenRole( + tokenId, + TokenRoles.Controller, + '0x7ED735b7095C05d78dF169F991f2b7f1A1F1A049' + ) + ).to.be.true; + expect( + await contract.hasTokenRole( + tokenId, + TokenRoles.Controller, + '0x2FEd6Ef3c495922263B403319FA6DDB323DD49E3' + ) + ).to.be.true; }); it('should not match the owner role for other account', async () => { const { contract, otherAccount, tokenId } = fixture; - const hasRole = await contract.hasTokenRole( - tokenId, - Roles.Owner, - otherAccount.address - ); + const tokenOwner = await contract.ownerOf(tokenId); - expect(hasRole).to.be.false; + expect(tokenOwner).to.not.be.equal(otherAccount.address); }); it('should remove an added controller', async () => { const { contract, owner, otherAccount, tokenId } = fixture; await contract.grantTokenRole( tokenId, - Roles.Controller, + TokenRoles.Controller, otherAccount.address ); await contract.revokeTokenRole( tokenId, - Roles.Controller, + TokenRoles.Controller, otherAccount.address ); expect( await contract.hasTokenRole( tokenId, - Roles.Controller, + TokenRoles.Controller, otherAccount.address ) ).to.be.false; @@ -124,86 +104,112 @@ describe('FleekERC721.TokenRoles', () => { 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, tokenId } = fixture; await contract.grantTokenRole( tokenId, - Roles.Controller, + TokenRoles.Controller, otherAccount.address ); await contract.transferFrom(owner.address, otherAccount.address, tokenId); - expect(await contract.getTokenRoleMembers(tokenId, 1)).to.eql([]); + expect( + await contract.hasTokenRole( + tokenId, + TokenRoles.Controller, + otherAccount.address + ) + ).to.be.false; }); it('should not be able to add address role', async () => { - const { contract, owner, otherAccount, tokenId } = fixture; + const { contract, otherAccount, tokenId } = 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'); + .grantTokenRole(tokenId, TokenRoles.Controller, otherAccount.address) + ).to.be.revertedWithCustomError(contract, Errors.MustBeTokenOwner); }); it('should not be able to remove address role', async () => { const { contract, owner, otherAccount, tokenId } = 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, tokenId } = fixture; - await contract.grantTokenRole(tokenId, Roles.Owner, otherAccount.address); - - expect( - await contract - .connect(otherAccount) - .grantTokenRole(tokenId, Roles.Controller, otherAccount.address) - ).to.not.be.reverted; + .revokeTokenRole(tokenId, TokenRoles.Controller, otherAccount.address) + ).to.be.revertedWithCustomError(contract, Errors.MustBeTokenOwner); }); it('should emit event when token role is granted', async () => { const { contract, owner, otherAccount, tokenId } = fixture; await expect( - contract.grantTokenRole(tokenId, Roles.Controller, otherAccount.address) + contract.grantTokenRole( + tokenId, + TokenRoles.Controller, + otherAccount.address + ) ) - .to.emit(contract, 'TokenRoleGranted') - .withArgs(tokenId, Roles.Controller, otherAccount.address, owner.address); + .to.emit(contract, 'TokenRoleChanged') + .withArgs( + tokenId, + TokenRoles.Controller, + otherAccount.address, + true, + owner.address + ); }); it('should emit event when token role is revoked', async () => { const { contract, owner, otherAccount, tokenId } = fixture; await contract.grantTokenRole( tokenId, - Roles.Controller, + TokenRoles.Controller, otherAccount.address ); await expect( - contract.revokeTokenRole(tokenId, Roles.Controller, otherAccount.address) + contract.revokeTokenRole( + tokenId, + TokenRoles.Controller, + otherAccount.address + ) ) - .to.emit(contract, 'TokenRoleRevoked') - .withArgs(tokenId, Roles.Controller, otherAccount.address, owner.address); + .to.emit(contract, 'TokenRoleChanged') + .withArgs( + tokenId, + TokenRoles.Controller, + otherAccount.address, + false, + owner.address + ); + }); + + it('should not be able to grant role twice', async () => { + const { contract, otherAccount, tokenId } = fixture; + await contract.grantTokenRole( + tokenId, + TokenRoles.Controller, + otherAccount.address + ); + await expect( + contract.grantTokenRole( + tokenId, + TokenRoles.Controller, + otherAccount.address + ) + ).to.be.revertedWithCustomError(contract, Errors.RoleAlreadySet); + }); + + it('should not be able to revoke role twice', async () => { + const { contract, otherAccount, tokenId } = fixture; + await expect( + contract.revokeTokenRole( + tokenId, + TokenRoles.Controller, + otherAccount.address + ) + ).to.be.revertedWithCustomError(contract, Errors.RoleAlreadySet); }); });