diff --git a/contracts/access/AccessControlled.sol b/contracts/access/AccessControlled.sol index 6ad462141..b310c0764 100644 --- a/contracts/access/AccessControlled.sol +++ b/contracts/access/AccessControlled.sol @@ -1,31 +1,28 @@ // SPDX-License-Identifier: MIT pragma solidity ^0.8.18; -import { ZNSRoles } from "./ZNSRoles.sol"; import { IZNSAccessController } from "./IZNSAccessController.sol"; -abstract contract AccessControlled is ZNSRoles { +abstract contract AccessControlled { + event AccessControllerSet(address accessController); IZNSAccessController internal accessController; - modifier onlyRole(bytes32 role) { - accessController.checkRole(role, msg.sender); + + modifier onlyAdmin() { + accessController.checkAdmin(msg.sender); _; } /** - * @dev This is here to make sure the external function is always implemented in children, - * otherwise we will not be able to reset the module (not ideal since it might - * not get to the final interface of a child). - * TODO AC: how do we make sure this gets to the final interface? + * @dev These 2 virtual functions are here to make sure they are always implemented in children, + * otherwise we will not be able to reset the module or read the AC address */ - function setAccessController(address _accessController) external virtual; + function getAccessController() external view virtual returns (address); - function getAccessController() external view returns (address) { - return address(accessController); - } + function setAccessController(address _accessController) external virtual; function _setAccessController(address _accessController) internal { require(_accessController != address(0), "AC: _accessController is 0x0 address"); diff --git a/contracts/access/IZNSAccessController.sol b/contracts/access/IZNSAccessController.sol index 65cf0952c..177d3e904 100644 --- a/contracts/access/IZNSAccessController.sol +++ b/contracts/access/IZNSAccessController.sol @@ -10,7 +10,17 @@ interface IZNSAccessController is IAccessControlUpgradeable { address[] calldata operatorAddresses ) external; - function checkRole(bytes32 role, address account) external view; - function setRoleAdmin(bytes32 role, bytes32 adminRole) external; + + function checkGovernor(address account) external view; + + function checkAdmin(address account) external view; + + function checkExecutor(address account) external view; + + function checkRegistrar(address account) external view; + + function isAdmin(address account) external view returns (bool); + + function isRegistrar(address account) external view returns (bool); } diff --git a/contracts/access/ZNSAccessController.sol b/contracts/access/ZNSAccessController.sol index 840457e3b..0975ccd09 100644 --- a/contracts/access/ZNSAccessController.sol +++ b/contracts/access/ZNSAccessController.sol @@ -16,29 +16,46 @@ contract ZNSAccessController is AccessControlUpgradeable, ZNSRoles, IZNSAccessCo _grantRoleToMany(GOVERNOR_ROLE, governorAddresses); _grantRoleToMany(ADMIN_ROLE, adminAddresses); - // all of the governors control admins TODO AC: ??? + // all of the governors control admins _setRoleAdmin(ADMIN_ROLE, GOVERNOR_ROLE); - // all of the governors control governors TODO AC: ??? + // all of the governors control governors _setRoleAdmin(GOVERNOR_ROLE, GOVERNOR_ROLE); - // all of the admins control registrar TODO AC: ??? + // all of the admins control registrar _setRoleAdmin(REGISTRAR_ROLE, ADMIN_ROLE); } - // TODO AC: should we keep this function here so that we can get standardized message? - // test this function for gas usage with a standardized message vs a custom message - // when using the recommended method of `hasRole` - function checkRole(bytes32 role, address account) external view override { - _checkRole(role, account); + // ** Access Validators ** + + function checkGovernor(address account) external view override { + _checkRole(GOVERNOR_ROLE, account); + } + + function checkAdmin(address account) external view override { + _checkRole(ADMIN_ROLE, account); + } + + function checkExecutor(address account) external view override { + _checkRole(EXECUTOR_ROLE, account); + } + + function checkRegistrar(address account) external view override { + _checkRole(REGISTRAR_ROLE, account); + } + + function isAdmin(address account) external view override returns (bool) { + return hasRole(ADMIN_ROLE, account); + } + + function isRegistrar(address account) external view override returns (bool) { + return hasRole(REGISTRAR_ROLE, account); } - // TODO AC: is this function necessary? how often will it be used? function _grantRoleToMany(bytes32 role, address[] calldata addresses) internal { for (uint256 i = 0; i < addresses.length; i++) { _grantRole(role, addresses[i]); } } - // TODO AC: how safe is this? function setRoleAdmin(bytes32 role, bytes32 adminRole) external override onlyRole(GOVERNOR_ROLE) { _setRoleAdmin(role, adminRole); } diff --git a/contracts/access/ZNSRoles.sol b/contracts/access/ZNSRoles.sol index 4f473e96b..2a19007b9 100644 --- a/contracts/access/ZNSRoles.sol +++ b/contracts/access/ZNSRoles.sol @@ -2,18 +2,16 @@ pragma solidity ^0.8.18; +// TODO: can we declare these outside of contract, just in the ZNSAccessController file? abstract contract ZNSRoles { - // TODO AC: test getting this from AC contract vs inheriting these roles in every other contract - // the highest rank, only assigns Admins + // the highest rank, assigns Admins, new roles and Role Admins bytes32 public constant GOVERNOR_ROLE = keccak256("GOVERNOR_ROLE"); // the main maintainer role, that gets access to all system functions - // TODO AC: should we split responsibilities in a better way? bytes32 public constant ADMIN_ROLE = keccak256("ADMIN_ROLE"); // operator can be here to future proof, if we need a new role // so we don't have to upgrade all contracts - // TODO AC: change name of this role - bytes32 public constant OPERATOR_ROLE = keccak256("OPERATOR_ROLE"); + // TODO AC: possibly remove executor role? + bytes32 public constant EXECUTOR_ROLE = keccak256("EXECUTOR_ROLE"); // this role is here specifically for the ZNSEthRegistrar contract bytes32 public constant REGISTRAR_ROLE = keccak256("REGISTRAR_ROLE"); - // TODO AC: what other roles do we need here? } diff --git a/contracts/distribution/IZNSEthRegistrar.sol b/contracts/distribution/IZNSEthRegistrar.sol index dc24bcc05..c8b6f8628 100644 --- a/contracts/distribution/IZNSEthRegistrar.sol +++ b/contracts/distribution/IZNSEthRegistrar.sol @@ -18,14 +18,13 @@ interface IZNSEthRegistrar { address indexed registrant ); - // TODO AC: remove ZNS from names here and in state vars - event ZnsRegistrySet(address znsRegistry); + event RegistrySet(address znsRegistry); - event ZnsTreasurySet(address znsTreasury); + event TreasurySet(address znsTreasury); - event ZnsDomainTokenSet(address znsDomainToken); + event DomainTokenSet(address znsDomainToken); - event ZnsAddressResolverSet(address znsAddressResolver); + event AddressResolverSet(address znsAddressResolver); function registerDomain( string calldata name, @@ -36,13 +35,15 @@ interface IZNSEthRegistrar { function reclaimDomain(bytes32 domainHash) external; - function setZnsRegistry(address znsRegistry_) external; + function setRegistry(address znsRegistry_) external; - function setZnsTreasury(address znsTreasury_) external; + function setTreasury(address znsTreasury_) external; - function setZnsDomainToken(address znsDomainToken_) external; + function setDomainToken(address znsDomainToken_) external; - function setZnsAddressResolver(address znsAddressResolver_) external; + function setAddressResolver(address znsAddressResolver_) external; function setAccessController(address accessController_) external; + + function getAccessController() external view returns (address); } diff --git a/contracts/distribution/IZNSPriceOracle.sol b/contracts/distribution/IZNSPriceOracle.sol index 4b64b978f..7bc5a0e6a 100644 --- a/contracts/distribution/IZNSPriceOracle.sol +++ b/contracts/distribution/IZNSPriceOracle.sol @@ -120,4 +120,8 @@ interface IZNSPriceOracle { * @param subdomainLength The length for subdomains */ function setBaseLengths(uint256 rootLength, uint256 subdomainLength) external; + + function setAccessController(address accessController) external; + + function getAccessController() external view returns (address); } diff --git a/contracts/distribution/IZNSTreasury.sol b/contracts/distribution/IZNSTreasury.sol index 8e21823d1..446fb48a2 100644 --- a/contracts/distribution/IZNSTreasury.sol +++ b/contracts/distribution/IZNSTreasury.sol @@ -2,12 +2,6 @@ pragma solidity ^0.8.18; interface IZNSTreasury { - /** - * @notice Emitted when the znsRegistrar is updated - * @param znsRegistrar The address of the new registrar - */ - event ZNSRegistrarSet(address znsRegistrar); - /** * @notice Emitted when a new stake is deposited * @param domainHash The hash of the domain name @@ -34,9 +28,9 @@ interface IZNSTreasury { uint256 indexed amount ); - event ZnsPriceOracleSet(address znsPriceOracle); + event PriceOracleSet(address znsPriceOracle); - event ZnsStakingTokenSet(address znsStakingToken); + event StakingTokenSet(address znsStakingToken); event ZeroVaultAddressSet(address zeroVault); @@ -54,4 +48,8 @@ interface IZNSTreasury { function setPriceOracle(address znsPriceOracle_) external; function setStakingToken(address stakingToken_) external; + + function setAccessController(address accessController) external; + + function getAccessController() external view returns (address); } diff --git a/contracts/distribution/ZNSEthRegistrar.sol b/contracts/distribution/ZNSEthRegistrar.sol index fef82b86f..3080b0d5e 100644 --- a/contracts/distribution/ZNSEthRegistrar.sol +++ b/contracts/distribution/ZNSEthRegistrar.sol @@ -8,17 +8,18 @@ import { IZNSDomainToken } from "../token/IZNSDomainToken.sol"; import { IZNSAddressResolver } from "../resolver/IZNSAddressResolver.sol"; import { AccessControlled } from "../access/AccessControlled.sol"; + contract ZNSEthRegistrar is AccessControlled, IZNSEthRegistrar { - IZNSRegistry public znsRegistry; - IZNSTreasury public znsTreasury; - IZNSDomainToken public znsDomainToken; - IZNSAddressResolver public znsAddressResolver; + IZNSRegistry public registry; + IZNSTreasury public treasury; + IZNSDomainToken public domainToken; + IZNSAddressResolver public addressResolver; modifier onlyNameOwner(bytes32 domainHash) { require( - msg.sender == znsRegistry.getDomainOwner(domainHash), + msg.sender == registry.getDomainOwner(domainHash), "ZNSEthRegistrar: Not the Owner of the Name" ); _; @@ -26,8 +27,8 @@ contract ZNSEthRegistrar is AccessControlled, IZNSEthRegistrar { modifier onlyTokenOwner(bytes32 domainHash) { require( - msg.sender == znsDomainToken.ownerOf(uint256(domainHash)), - "ZNSEthRegistrar: Not the owner of the Token" + msg.sender == domainToken.ownerOf(uint256(domainHash)), + "ZNSEthRegistrar: Not the Owner of the Token" ); _; } @@ -44,11 +45,10 @@ contract ZNSEthRegistrar is AccessControlled, IZNSEthRegistrar { address znsAddressResolver_ ) { _setAccessController(accessController_); - // TODO AC: should we call protected functions in the constructor/initialize? - setZnsRegistry(znsRegistry_); - setZnsTreasury(znsTreasury_); - setZnsDomainToken(znsDomainToken_); - setZnsAddressResolver(znsAddressResolver_); + setRegistry(znsRegistry_); + setTreasury(znsTreasury_); + setDomainToken(znsDomainToken_); + setAddressResolver(znsAddressResolver_); } /** @@ -70,16 +70,16 @@ contract ZNSEthRegistrar is AccessControlled, IZNSEthRegistrar { bytes32 domainHash = keccak256(bytes(name)); require( - !znsRegistry.exists(domainHash), + !registry.exists(domainHash), "ZNSEthRegistrar: Domain already exists" ); // Staking logic - znsTreasury.stakeForDomain(domainHash, name, msg.sender, true); + treasury.stakeForDomain(domainHash, name, msg.sender, true); // Get tokenId for the new token to be minted for the new domain uint256 tokenId = uint256(domainHash); - znsDomainToken.register(msg.sender, tokenId); + domainToken.register(msg.sender, tokenId); _setDomainData(domainHash, msg.sender, resolverContent); @@ -88,7 +88,7 @@ contract ZNSEthRegistrar is AccessControlled, IZNSEthRegistrar { tokenId, name, msg.sender, - address(znsAddressResolver) + address(addressResolver) ); return domainHash; @@ -104,9 +104,9 @@ contract ZNSEthRegistrar is AccessControlled, IZNSEthRegistrar { onlyTokenOwner(domainHash) { uint256 tokenId = uint256(domainHash); - znsDomainToken.revoke(tokenId); - znsTreasury.unstakeForDomain(domainHash, msg.sender); - znsRegistry.deleteRecord(domainHash); + domainToken.revoke(tokenId); + treasury.unstakeForDomain(domainHash, msg.sender); + registry.deleteRecord(domainHash); emit DomainRevoked(domainHash, msg.sender); } @@ -116,59 +116,63 @@ contract ZNSEthRegistrar is AccessControlled, IZNSEthRegistrar { override onlyTokenOwner(domainHash) { - znsRegistry.updateDomainOwner(domainHash, msg.sender); + registry.updateDomainOwner(domainHash, msg.sender); emit DomainReclaimed(domainHash, msg.sender); } - function setZnsRegistry(address znsRegistry_) public override onlyRole(ADMIN_ROLE) { + function setRegistry(address znsRegistry_) public override onlyAdmin { require( znsRegistry_ != address(0), "ZNSEthRegistrar: znsRegistry_ is 0x0 address" ); - znsRegistry = IZNSRegistry(znsRegistry_); + registry = IZNSRegistry(znsRegistry_); - emit ZnsRegistrySet(znsRegistry_); + emit RegistrySet(znsRegistry_); } - function setZnsTreasury(address znsTreasury_) public override onlyRole(ADMIN_ROLE) { + function setTreasury(address znsTreasury_) public override onlyAdmin { require( znsTreasury_ != address(0), "ZNSEthRegistrar: znsTreasury_ is 0x0 address" ); - znsTreasury = IZNSTreasury(znsTreasury_); + treasury = IZNSTreasury(znsTreasury_); - emit ZnsTreasurySet(znsTreasury_); + emit TreasurySet(znsTreasury_); } - function setZnsDomainToken(address znsDomainToken_) public override onlyRole(ADMIN_ROLE) { + function setDomainToken(address znsDomainToken_) public override onlyAdmin { require( znsDomainToken_ != address(0), "ZNSEthRegistrar: znsDomainToken_ is 0x0 address" ); - znsDomainToken = IZNSDomainToken(znsDomainToken_); + domainToken = IZNSDomainToken(znsDomainToken_); - emit ZnsDomainTokenSet(znsDomainToken_); + emit DomainTokenSet(znsDomainToken_); } - function setZnsAddressResolver(address znsAddressResolver_) public override onlyRole(ADMIN_ROLE) { + function setAddressResolver(address znsAddressResolver_) public override onlyAdmin { require( znsAddressResolver_ != address(0), "ZNSEthRegistrar: znsAddressResolver_ is 0x0 address" ); - znsAddressResolver = IZNSAddressResolver(znsAddressResolver_); + addressResolver = IZNSAddressResolver(znsAddressResolver_); - emit ZnsAddressResolverSet(znsAddressResolver_); + emit AddressResolverSet(znsAddressResolver_); } function setAccessController(address accessController_) external override(AccessControlled, IZNSEthRegistrar) - onlyRole(ADMIN_ROLE) + onlyAdmin { _setAccessController(accessController_); } + function getAccessController() external view override(AccessControlled, IZNSEthRegistrar) returns (address) { + return address(accessController); + } + /** * @notice Set domain data appropriately for a newly registered domain * @@ -183,10 +187,10 @@ contract ZNSEthRegistrar is AccessControlled, IZNSEthRegistrar { ) internal { // Set only the domain owner if no resolver content is given if (resolverContent != address(0)) { - znsRegistry.createDomainRecord(domainHash, owner, address(znsAddressResolver)); - znsAddressResolver.setAddress(domainHash, resolverContent); + registry.createDomainRecord(domainHash, owner, address(addressResolver)); + addressResolver.setAddress(domainHash, resolverContent); } else { - znsRegistry.createDomainRecord(domainHash, owner, address(0)); + registry.createDomainRecord(domainHash, owner, address(0)); } } } diff --git a/contracts/distribution/ZNSPriceOracle.sol b/contracts/distribution/ZNSPriceOracle.sol index 64b9769ee..c7e993652 100644 --- a/contracts/distribution/ZNSPriceOracle.sol +++ b/contracts/distribution/ZNSPriceOracle.sol @@ -7,7 +7,8 @@ import { IZNSPriceOracle } from "./IZNSPriceOracle.sol"; import { StringUtils } from "../utils/StringUtils.sol"; import { AccessControlled } from "../access/AccessControlled.sol"; -contract ZNSPriceOracle is AccessControlled, IZNSPriceOracle, Initializable { + +contract ZNSPriceOracle is AccessControlled, Initializable, IZNSPriceOracle { using StringUtils for string; uint256 public constant PERCENTAGE_BASIS = 10000; @@ -25,16 +26,18 @@ contract ZNSPriceOracle is AccessControlled, IZNSPriceOracle, Initializable { // TODO: rework and add more setters for every single var PriceParams public priceConfig; + // TODO: rework setters here for a better structure! + // TODO: remove subdomain logic function initialize( address accessController_, PriceParams calldata priceConfig_, uint256 regFeePercentage_ ) public override initializer { + _setAccessController(accessController_); // Set pricing and length parameters priceConfig = priceConfig_; feePercentage = regFeePercentage_; - _setAccessController(accessController_); } /** @@ -90,7 +93,7 @@ contract ZNSPriceOracle is AccessControlled, IZNSPriceOracle, Initializable { function setMaxPrice( uint256 maxPrice, bool isRootDomain - ) external override onlyRole(ADMIN_ROLE) { + ) external override onlyAdmin { if (isRootDomain) { priceConfig.maxRootDomainPrice = maxPrice; } else { @@ -113,17 +116,21 @@ contract ZNSPriceOracle is AccessControlled, IZNSPriceOracle, Initializable { * to make up for this. * @param multiplier The new price multiplier to set */ - function setPriceMultiplier(uint256 multiplier) external override onlyRole(ADMIN_ROLE) { + function setPriceMultiplier(uint256 multiplier) external override onlyAdmin { require( multiplier >= 300 && multiplier <= 400, - "ZNS: Multiplier out of range" + "ZNSPriceOracle: Multiplier out of range" ); priceConfig.priceMultiplier = multiplier; emit PriceMultiplierSet(multiplier); } - function setRegistrationFeePercentage(uint256 regFeePercentage) external override onlyRole(ADMIN_ROLE) { + function setRegistrationFeePercentage(uint256 regFeePercentage) + external + override + onlyAdmin + { feePercentage = regFeePercentage; emit FeePercentageSet(regFeePercentage); } @@ -138,7 +145,7 @@ contract ZNSPriceOracle is AccessControlled, IZNSPriceOracle, Initializable { function setBaseLength( uint256 length, bool isRootDomain - ) external override onlyRole(ADMIN_ROLE) { + ) external override onlyAdmin { if (isRootDomain) { priceConfig.baseRootDomainLength = length; } else { @@ -156,17 +163,25 @@ contract ZNSPriceOracle is AccessControlled, IZNSPriceOracle, Initializable { function setBaseLengths( uint256 rootLength, uint256 subdomainLength - ) external override onlyRole(ADMIN_ROLE) { + ) external override onlyAdmin { priceConfig.baseRootDomainLength = rootLength; priceConfig.baseSubdomainLength = subdomainLength; emit BaseLengthsSet(rootLength, subdomainLength); } - function setAccessController(address accessController) external override onlyRole(ADMIN_ROLE) { + function setAccessController(address accessController) + external + override(AccessControlled, IZNSPriceOracle) + onlyAdmin + { _setAccessController(accessController); } + function getAccessController() external view override(AccessControlled, IZNSPriceOracle) returns (address) { + return address(accessController); + } + /** * @notice Internal function to get price abstract of the base price being for * a root domain or a subdomain. diff --git a/contracts/distribution/ZNSTreasury.sol b/contracts/distribution/ZNSTreasury.sol index a1b6738c5..7962e0c77 100644 --- a/contracts/distribution/ZNSTreasury.sol +++ b/contracts/distribution/ZNSTreasury.sol @@ -11,7 +11,7 @@ contract ZNSTreasury is AccessControlled, IZNSTreasury { /** * @notice The price oracle */ - IZNSPriceOracle public znsPriceOracle; + IZNSPriceOracle public priceOracle; /** * @notice The payment/staking token @@ -32,6 +32,12 @@ contract ZNSTreasury is AccessControlled, IZNSTreasury { mapping(bytes32 domainHash => uint256 amountStaked) public stakedForDomain; + modifier onlyRegistrar() { + accessController.checkRegistrar(msg.sender); + _; + } + + constructor( address accessController_, address znsPriceOracle_, @@ -39,7 +45,7 @@ contract ZNSTreasury is AccessControlled, IZNSTreasury { address zeroVault_ ) { _setAccessController(accessController_); - _setZeroVaultAddress(zeroVault_); + setZeroVaultAddress(zeroVault_); // TODO change from mock setStakingToken(stakingToken_); setPriceOracle(znsPriceOracle_); @@ -50,9 +56,9 @@ contract ZNSTreasury is AccessControlled, IZNSTreasury { string calldata domainName, address depositor, bool isTopLevelDomain - ) external override onlyRole(REGISTRAR_ROLE) { + ) external override onlyRegistrar { // Get price and fee for the domain - (, uint256 stakeAmount, uint256 registrationFee) = znsPriceOracle.getPrice( + (, uint256 stakeAmount, uint256 registrationFee) = priceOracle.getPrice( domainName, isTopLevelDomain ); @@ -72,7 +78,7 @@ contract ZNSTreasury is AccessControlled, IZNSTreasury { function unstakeForDomain( bytes32 domainHash, address owner - ) external override onlyRole(REGISTRAR_ROLE) { + ) external override onlyRegistrar { uint256 stakeAmount = stakedForDomain[domainHash]; require(stakeAmount > 0, "ZNSTreasury: No stake for domain"); delete stakedForDomain[domainHash]; @@ -82,36 +88,39 @@ contract ZNSTreasury is AccessControlled, IZNSTreasury { emit StakeWithdrawn(domainHash, owner, stakeAmount); } - function setZeroVaultAddress(address zeroVaultAddress) external override onlyRole(ADMIN_ROLE) { - _setZeroVaultAddress(zeroVaultAddress); + function setZeroVaultAddress(address zeroVaultAddress) public override onlyAdmin { + require(zeroVaultAddress != address(0), "ZNSTreasury: zeroVault passed as 0x0 address"); + + zeroVault = zeroVaultAddress; + emit ZeroVaultAddressSet(zeroVaultAddress); } - // TODO AC: should we call a protected function in the constructor/initialize? - function setPriceOracle(address znsPriceOracle_) public override onlyRole(ADMIN_ROLE) { + function setPriceOracle(address znsPriceOracle_) public override onlyAdmin { require( znsPriceOracle_ != address(0), "ZNSTreasury: znsPriceOracle_ passed as 0x0 address" ); - znsPriceOracle = IZNSPriceOracle(znsPriceOracle_); - emit ZnsPriceOracleSet(znsPriceOracle_); + priceOracle = IZNSPriceOracle(znsPriceOracle_); + emit PriceOracleSet(znsPriceOracle_); } - function setStakingToken(address stakingToken_) public override onlyRole(ADMIN_ROLE) { + function setStakingToken(address stakingToken_) public override onlyAdmin { require(stakingToken_ != address(0), "ZNSTreasury: stakingToken_ passed as 0x0 address"); stakingToken = IERC20(stakingToken_); - emit ZnsStakingTokenSet(stakingToken_); + emit StakingTokenSet(stakingToken_); } - function setAccessController(address accessController_) external override onlyRole(ADMIN_ROLE) { + function setAccessController(address accessController_) + public + override(AccessControlled, IZNSTreasury) + onlyAdmin + { _setAccessController(accessController_); } - function _setZeroVaultAddress(address zeroVaultAddress) internal { - require(zeroVaultAddress != address(0), "ZNSTreasury: zeroVault passed as 0x0 address"); - - zeroVault = zeroVaultAddress; - emit ZeroVaultAddressSet(zeroVaultAddress); + function getAccessController() external view override(AccessControlled, IZNSTreasury) returns (address) { + return address(accessController); } } diff --git a/contracts/registry/IZNSRegistry.sol b/contracts/registry/IZNSRegistry.sol index 78f85588e..58615200a 100644 --- a/contracts/registry/IZNSRegistry.sol +++ b/contracts/registry/IZNSRegistry.sol @@ -2,6 +2,7 @@ pragma solidity 0.8.18; interface IZNSRegistry { + /** * @notice The `DomainRecord` struct is meant to hold relevant information * about a domain, such as its owner and resolver. @@ -10,6 +11,7 @@ interface IZNSRegistry { address owner; address resolver; } + /** * @notice Emit when ownership of a domain is modified * @param owner The new domain owner @@ -50,15 +52,11 @@ interface IZNSRegistry { bool allowed ); - function initialize(address owner) external; - /** - * @notice Emit when a new ZNSRegistrar address is set - * @param znsRegistrar The new address + * @notice Initialize the ZNSRegistry contract + * @param _accessController The address of the AccessController contract */ - event ZNSRegistrarSet( - address indexed znsRegistrar - ); + function initialize(address _accessController) external; /** * @notice Check if a given domain exists @@ -79,7 +77,6 @@ interface IZNSRegistry { /** * @notice Set an `operator` as `allowed` to give or remove permissions for all * domains owned by `msg.sender` - * * @param operator The account to allow/disallow * @param allowed The true/false value to set */ @@ -111,7 +108,6 @@ interface IZNSRegistry { /** * @notice Create a new domain record - * * @param domainHash The hash of the domain name * @param owner The owner of the new domain * @param resolver The resolver of the new domain @@ -124,7 +120,6 @@ interface IZNSRegistry { /** * @notice Update an existing domain record's owner or resolver - * * @param domainHash The hash of the domain * @param owner The owner or an allowed operator of that domain * @param resolver The resolver for the domain @@ -144,7 +139,6 @@ interface IZNSRegistry { /** * @notice Update the domain's default resolver - * * @param domainHash the hash of a domain's name * @param resolver The new default resolver */ @@ -153,17 +147,17 @@ interface IZNSRegistry { address resolver ) external; - /** - * @notice Change the address of the ZNSRegistrar contract we use - * - * @param znsRegistrar_ The new ZNSRegistrar - */ - function setZNSRegistrar(address znsRegistrar_) external; - /** * @notice Delete a domain's record - * * @param domainHash The hash of the domain name */ function deleteRecord(bytes32 domainHash) external; + + /** + * @notice Set the access controller contract + * @param accessController The new access controller + */ + function setAccessController(address accessController) external; + + function getAccessController() external view returns (address); } diff --git a/contracts/registry/ZNSRegistry.sol b/contracts/registry/ZNSRegistry.sol index 47e13c002..ea2dc0eb7 100644 --- a/contracts/registry/ZNSRegistry.sol +++ b/contracts/registry/ZNSRegistry.sol @@ -5,12 +5,11 @@ import { ERC1967UpgradeUpgradeable } from "@openzeppelin/contracts-upgradeable/proxy/ERC1967/ERC1967UpgradeUpgradeable.sol"; import { Initializable } from "@openzeppelin/contracts-upgradeable/proxy/utils/Initializable.sol"; import { IZNSRegistry } from "./IZNSRegistry.sol"; +import { AccessControlled } from "../access/AccessControlled.sol"; + + +contract ZNSRegistry is AccessControlled, ERC1967UpgradeUpgradeable, IZNSRegistry { -contract ZNSRegistry is IZNSRegistry, ERC1967UpgradeUpgradeable { - /** - * @notice The address of the registrar we are using - */ - address public znsRegistrar; /** * @notice Mapping `domainHash` to `DomainRecord` struct to hold information * about each domain @@ -30,6 +29,7 @@ contract ZNSRegistry is IZNSRegistry, ERC1967UpgradeUpgradeable { * @param domainHash the hash of a domain's name */ modifier onlyOwnerOrOperator(bytes32 domainHash) { + // TODO: consider using errors instead of requires with string msgs require( isOwnerOrOperator(domainHash, msg.sender), "ZNSRegistry: Not authorized" @@ -37,32 +37,29 @@ contract ZNSRegistry is IZNSRegistry, ERC1967UpgradeUpgradeable { _; } - /** - * @notice Revert if `msg.sender` is not the registrar - */ - modifier onlyRegistrar() { + modifier onlyOwner(bytes32 domainHash) { require( - msg.sender == znsRegistrar, - "ZNSRegistry: Caller is not the Registrar" + records[domainHash].owner == msg.sender, + "ZNSRegistry: Not the Name Owner" ); _; } + modifier onlyRegistrar() { + accessController.checkRegistrar(msg.sender); + _; + } + /** - * Initialize the ZNSRegistry contract, setting the owner of the `0x0` domain - * to be the account that deploys this contract + * @notice Initialize the ZNSRegistry contract + * @param _accessController The address of the AccessController contract */ - function initialize(address znsRegistrar_) public override initializer { - require( - znsRegistrar_ != address(0), - "ZNSRegistry: Registrar can not be 0x0 address" - ); - znsRegistrar = znsRegistrar_; + function initialize(address _accessController) public override initializer { + _setAccessController(_accessController); } /** * @notice Check if a given domain exists - * * @param domainHash The hash of a domain's name */ function exists(bytes32 domainHash) external view override returns (bool) { @@ -71,7 +68,6 @@ contract ZNSRegistry is IZNSRegistry, ERC1967UpgradeUpgradeable { /** * @notice Checks if provided address is an owner or an operator of the provided domain - * * @param domainHash The hash of a domain's name * @param candidate The address for which we are checking access */ @@ -83,31 +79,12 @@ contract ZNSRegistry is IZNSRegistry, ERC1967UpgradeUpgradeable { return candidate == owner || operators[owner][candidate]; } - /** - * @notice Change the address of the ZNSRegistrar contract we use - * - * @param znsRegistrar_ The new ZNSRegistrar - */ - function setZNSRegistrar(address znsRegistrar_) external override { - // TODO When we have access control, only be callable by admin!! - require( - znsRegistrar_ != address(0), - "ZNSRegistry: Cannot set Registrar to 0x0" - ); - - znsRegistrar = znsRegistrar_; - - emit ZNSRegistrarSet(znsRegistrar_); - } - /** * @notice Set an `operator` as `allowed` to give or remove permissions for all * domains owned by the owner `msg.sender` - * * @param operator The account to allow/disallow * @param allowed The true/false value to set */ - // TODO AC: add access control function setOwnerOperator(address operator, bool allowed) external override { operators[msg.sender][operator] = allowed; @@ -116,7 +93,6 @@ contract ZNSRegistry is IZNSRegistry, ERC1967UpgradeUpgradeable { /** * @notice Get a record for a domain - * * @param domainHash the hash of a domain's name */ function getDomainRecord( @@ -127,7 +103,6 @@ contract ZNSRegistry is IZNSRegistry, ERC1967UpgradeUpgradeable { /** * @notice Get the owner of the given domain - * * @param domainHash the hash of a domain's name */ function getDomainOwner( @@ -138,7 +113,6 @@ contract ZNSRegistry is IZNSRegistry, ERC1967UpgradeUpgradeable { /** * @notice Get the default resolver for the given domain - * * @param domainHash the hash of a domain's name */ function getDomainResolver( @@ -149,7 +123,6 @@ contract ZNSRegistry is IZNSRegistry, ERC1967UpgradeUpgradeable { /** * @notice Create a new domain record - * * @param domainHash The hash of the domain name * @param owner The owner of the new domain * @param resolver The resolver of the new domain @@ -169,7 +142,6 @@ contract ZNSRegistry is IZNSRegistry, ERC1967UpgradeUpgradeable { /** * @notice Update an existing domain record's owner or resolver - * * @param domainHash The hash of the domain * @param owner The owner or an allowed operator of that domain * @param resolver The resolver for the domain @@ -178,8 +150,7 @@ contract ZNSRegistry is IZNSRegistry, ERC1967UpgradeUpgradeable { bytes32 domainHash, address owner, address resolver - // TODO AC: make this so only owner can change the owner and not the operator! - ) external override onlyOwnerOrOperator(domainHash) { + ) external override onlyOwner(domainHash) { // `exists` is checked implicitly through the modifier _setDomainOwner(domainHash, owner); _setDomainResolver(domainHash, resolver); @@ -187,22 +158,26 @@ contract ZNSRegistry is IZNSRegistry, ERC1967UpgradeUpgradeable { /** * @notice Update a domain's owner - * * @param domainHash the hash of a domain's name * @param owner The account to transfer ownership to */ function updateDomainOwner( bytes32 domainHash, address owner - // TODO AC: make this so only owner can change the owner and not the operator! - ) external override onlyOwnerOrOperator(domainHash) { + ) external override { + // this can be called from ZNSRegistrar as part of `reclaim()` flow + require( + msg.sender == records[domainHash].owner || + accessController.isRegistrar(msg.sender), + "ZNSRegistry: Only Name Owner or Registrar allowed to call" + ); + // `exists` is checked implicitly through the modifier _setDomainOwner(domainHash, owner); } /** * @notice Update the domain's default resolver - * * @param domainHash the hash of a domain's name * @param resolver The new default resolver */ @@ -216,7 +191,6 @@ contract ZNSRegistry is IZNSRegistry, ERC1967UpgradeUpgradeable { /** * @notice Delete a domain's record - * * @param domainHash The hash of the domain name */ function deleteRecord(bytes32 domainHash) external override onlyRegistrar { @@ -225,9 +199,18 @@ contract ZNSRegistry is IZNSRegistry, ERC1967UpgradeUpgradeable { emit DomainRecordDeleted(domainHash); } + function setAccessController( + address accessController + ) external override(AccessControlled, IZNSRegistry) onlyAdmin { + _setAccessController(accessController); + } + + function getAccessController() external view override(AccessControlled, IZNSRegistry) returns (address) { + return address(accessController); + } + /** * @notice Check if a domain exists. True if the owner is not `0x0` - * * @param domainHash the hash of a domain's name */ function _exists(bytes32 domainHash) internal view returns (bool) { @@ -239,7 +222,6 @@ contract ZNSRegistry is IZNSRegistry, ERC1967UpgradeUpgradeable { * Note that we don't check for `address(0)` here. This is intentional * because we are not currently allowing reselling of domains and want * to enable burning them instead by transferring ownership to `address(0)` - * * @param domainHash the hash of a domain's name * @param owner The owner to set */ @@ -251,7 +233,6 @@ contract ZNSRegistry is IZNSRegistry, ERC1967UpgradeUpgradeable { /** * @notice Set a domain's resolver - * * @param domainHash the hash of a domain's name * @param resolver The resolver to set */ diff --git a/contracts/resolver/IZNSAddressResolver.sol b/contracts/resolver/IZNSAddressResolver.sol index 119568b75..8866ff795 100644 --- a/contracts/resolver/IZNSAddressResolver.sol +++ b/contracts/resolver/IZNSAddressResolver.sol @@ -9,6 +9,12 @@ interface IZNSAddressResolver { */ event AddressSet(bytes32 indexed domainHash, address indexed newAddress); + /** + * @dev Emit when the registry is set + * @param registry The address of the registry + */ + event RegistrySet(address registry); + /** * @dev ERC-165 check for implementation identifier * @dev Supports interfaces IZNSAddressResolver and IERC165 @@ -33,4 +39,10 @@ interface IZNSAddressResolver { ) external; function getInterfaceId() external pure returns (bytes4); + + function setRegistry(address _registry) external; + + function setAccessController(address accessController) external; + + function getAccessController() external view returns (address); } diff --git a/contracts/resolver/ZNSAddressResolver.sol b/contracts/resolver/ZNSAddressResolver.sol index 01d6d82ab..c2c6dbc64 100644 --- a/contracts/resolver/ZNSAddressResolver.sol +++ b/contracts/resolver/ZNSAddressResolver.sol @@ -4,13 +4,16 @@ pragma solidity ^0.8.18; import { ERC165 } from "@openzeppelin/contracts/utils/introspection/ERC165.sol"; import { IZNSAddressResolver } from "./IZNSAddressResolver.sol"; import { IZNSRegistry } from "../registry/IZNSRegistry.sol"; +import { AccessControlled } from "../access/AccessControlled.sol"; -contract ZNSAddressResolver is ERC165, IZNSAddressResolver { + +contract ZNSAddressResolver is AccessControlled, ERC165, IZNSAddressResolver { /** * @notice Address of the ZNSRegistry contract that holds all crucial data * for every domain in the system */ IZNSRegistry public registry; + /** * @notice Mapping of domain hash to address used to bind domains * to Ethereum wallets or contracts registered in ZNS @@ -18,23 +21,12 @@ contract ZNSAddressResolver is ERC165, IZNSAddressResolver { mapping(bytes32 domainHash => address resolvedAddress) private addressOf; - constructor(IZNSRegistry _registry) { - registry = _registry; - } - - /** - * @dev Revert if `msg.sender` is not the owner or an operator allowed by the owner - * @param domainHash The identifying hash of a domain's name - */ - // TODO AC: Remove this when doing access control (or adapt to work the best way here). - // A function like that can be created in Registry, but think - // deeper if we want this to be for owner in Registry or owner of the Token in DomainToken! - modifier onlyOwnerOrOperator(bytes32 domainHash) { - require( - registry.isOwnerOrOperator(domainHash, msg.sender), - "ZNSAddressResolver: Not allowed" - ); - _; + constructor( + address _accessController, + address _registry + ) { + _setAccessController(_accessController); + setRegistry(_registry); } /** @@ -55,8 +47,14 @@ contract ZNSAddressResolver is ERC165, IZNSAddressResolver { function setAddress( bytes32 domainHash, address newAddress - ) external override onlyOwnerOrOperator(domainHash) { - require(newAddress != address(0), "ZNS: Cant set address to 0"); + ) external override { + // only owner or operator of the current domain can set the address + // also, ZNSRegistrar can set the address as part of the registration process + require( + registry.isOwnerOrOperator(domainHash, msg.sender) || + accessController.isRegistrar(msg.sender), + "ZNSAddressResolver: Not authorized for this domain" + ); addressOf[domainHash] = newAddress; @@ -82,4 +80,24 @@ contract ZNSAddressResolver is ERC165, IZNSAddressResolver { function getInterfaceId() public pure override returns (bytes4) { return type(IZNSAddressResolver).interfaceId; } + + function setRegistry(address _registry) public override onlyAdmin { + require( + _registry != address(0), + "ZNSAddressResolver: _registry is 0x0 address" + ); + registry = IZNSRegistry(_registry); + + emit RegistrySet(_registry); + } + + function setAccessController( + address accessController + ) external override(AccessControlled, IZNSAddressResolver) onlyAdmin { + _setAccessController(accessController); + } + + function getAccessController() external view override(AccessControlled, IZNSAddressResolver) returns (address) { + return address(accessController); + } } diff --git a/contracts/token/IZNSDomainToken.sol b/contracts/token/IZNSDomainToken.sol index b4d466f66..a4a223bc5 100644 --- a/contracts/token/IZNSDomainToken.sol +++ b/contracts/token/IZNSDomainToken.sol @@ -1,13 +1,15 @@ // SPDX-License-Identifier: MIT pragma solidity ^0.8.18; + import { IERC721 } from "@openzeppelin/contracts/token/ERC721/IERC721.sol"; -interface IZNSDomainToken is IERC721{ - event SetAccessAuthorization(address indexed account); +interface IZNSDomainToken is IERC721 { function register(address to, uint256 tokenId) external; function revoke(uint256 tokenId) external; - function authorize(address account) external; + function setAccessController(address accessController) external; + + function getAccessController() external view returns (address); } diff --git a/contracts/token/ZNSDomainToken.sol b/contracts/token/ZNSDomainToken.sol index f00d30d6d..75762b35f 100644 --- a/contracts/token/ZNSDomainToken.sol +++ b/contracts/token/ZNSDomainToken.sol @@ -3,58 +3,54 @@ pragma solidity ^0.8.18; import { ERC721 } from "@openzeppelin/contracts/token/ERC721/ERC721.sol"; import { IZNSDomainToken } from "./IZNSDomainToken.sol"; +import { AccessControlled } from "../access/AccessControlled.sol"; + /** - * @title A contract for tokenizing domains under the ZNS Architecture + * @title A contract for tokenizing domains under ZNS */ -contract ZNSDomainToken is ERC721, IZNSDomainToken { - constructor(string memory tokenName, string memory tokenSymbol) ERC721(tokenName, tokenSymbol) { - authorized[msg.sender] = true; - } - - /** - * @notice Track authorized users or contracts - * TODO access control for the entire system - */ - mapping(address user => bool isAuthorized) public authorized; +contract ZNSDomainToken is AccessControlled, ERC721, IZNSDomainToken { - /** - * @notice Restrict a function to only be callable by authorized users - */ - modifier onlyAuthorized() { - require(authorized[msg.sender], "ZNS: Not authorized"); + modifier onlyRegistrar() { + accessController.checkRegistrar(msg.sender); _; } - /** - * @notice Authorize an address for this contract - * @param account The registrar to set - */ - function authorize(address account) external override onlyAuthorized { - require(account != address(0), "ZNS: Zero address for authorized account"); - - // Modify the access control for the given address - authorized[account] = true; - emit SetAccessAuthorization(account); + constructor( + string memory tokenName, + string memory tokenSymbol, + address accessController + ) ERC721(tokenName, tokenSymbol) { + _setAccessController(accessController); } /** * @notice Mints a token with a specified tokenId, using _safeMint, and sends it to the given address - * @dev TODO: Add Access Control * @param to The address that will recieve the newly minted domain token * @param tokenId The TokenId that the caller wishes to mint/register */ - function register(address to, uint256 tokenId) external override { + function register(address to, uint256 tokenId) external override onlyRegistrar { _safeMint(to, tokenId); } /** * @notice Burns the token with the specified tokenId - * @dev TODO: Add Access Control, replace require to also other specific contracts to revoke * @param tokenId The tokenId that the caller wishes to burn/revoke */ - function revoke(uint256 tokenId) external override onlyAuthorized { + function revoke(uint256 tokenId) external override onlyRegistrar { _burn(tokenId); } + + function setAccessController(address accessController) + external + override(AccessControlled, IZNSDomainToken) + onlyAdmin + { + _setAccessController(accessController); + } + + function getAccessController() external view override(AccessControlled, IZNSDomainToken) returns (address) { + return address(accessController); + } } diff --git a/test/ZNSAccessController.test.ts b/test/ZNSAccessController.test.ts index 33b11cb99..91ae38748 100644 --- a/test/ZNSAccessController.test.ts +++ b/test/ZNSAccessController.test.ts @@ -1,9 +1,10 @@ import * as hre from "hardhat"; import { SignerWithAddress } from "@nomiclabs/hardhat-ethers/signers"; import { ZNSAccessController } from "../typechain"; -import { deployAccessController } from "./helpers"; +import { deployAccessController, INITIALIZED_ERR } from "./helpers"; import { expect } from "chai"; -import { ADMIN_ROLE, getAccessRevertMsg, GOVERNOR_ROLE, OPERATOR_ROLE, REGISTRAR_ROLE } from "./helpers/access"; +import { ADMIN_ROLE, GOVERNOR_ROLE, EXECUTOR_ROLE, REGISTRAR_ROLE } from "./helpers/access"; +import { getAccessRevertMsg } from "./helpers/errors"; describe("ZNSAccessController", () => { @@ -45,6 +46,17 @@ describe("ZNSAccessController", () => { }, Promise.resolve() ); }); + + it("Should not allow to initialize twice", async () => { + await expect( + znsAccessController.initialize( + governorAccs.map(acc => acc.address), + adminAccs.map(acc => acc.address), + ) + ).to.be.revertedWith( + INITIALIZED_ERR + ); + }); }); describe("Role Management from the Initial Setup", () => { @@ -142,29 +154,86 @@ describe("ZNSAccessController", () => { expect(has).to.be.false; }); - it("GOVERNOR_ROLE should be able to assign new OPERATOR_ROLE as admin for REGISTRAR_ROLE", async () => { + it("GOVERNOR_ROLE should be able to assign new EXECUTOR_ROLE as admin for REGISTRAR_ROLE", async () => { const [ governor ] = governorAccs; - await znsAccessController.connect(governor).setRoleAdmin(REGISTRAR_ROLE, OPERATOR_ROLE); + await znsAccessController.connect(governor).setRoleAdmin(REGISTRAR_ROLE, EXECUTOR_ROLE); const registrarAdminRole = await znsAccessController.getRoleAdmin(REGISTRAR_ROLE); - expect(registrarAdminRole).to.be.equal(OPERATOR_ROLE); + expect(registrarAdminRole).to.be.equal(EXECUTOR_ROLE); }); // eslint-disable-next-line max-len - it("GOVERNOR_ROLE should be able to make himself a new OPERATOR_ROLE's admin and assign this role to anyone", async () => { + it("GOVERNOR_ROLE should be able to make himself a new EXECUTOR_ROLE's admin and assign this role to anyone", async () => { const [ governor ] = governorAccs; const [ { address: newOperator } ] = randomAccs; - await znsAccessController.connect(governor).setRoleAdmin(OPERATOR_ROLE, GOVERNOR_ROLE); - const roleAdminFrom = await znsAccessController.getRoleAdmin(OPERATOR_ROLE); + await znsAccessController.connect(governor).setRoleAdmin(EXECUTOR_ROLE, GOVERNOR_ROLE); + const roleAdminFrom = await znsAccessController.getRoleAdmin(EXECUTOR_ROLE); expect(roleAdminFrom).to.be.equal(GOVERNOR_ROLE); - await znsAccessController.connect(governor).grantRole(OPERATOR_ROLE, newOperator); - const has = await znsAccessController.hasRole(OPERATOR_ROLE, newOperator); + await znsAccessController.connect(governor).grantRole(EXECUTOR_ROLE, newOperator); + const has = await znsAccessController.hasRole(EXECUTOR_ROLE, newOperator); expect(has).to.be.true; }); + + it("Should revert when setting role admin without GOVERNOR_ROLE", async () => { + const [ { address: random } ] = randomAccs; + await expect( + znsAccessController.connect(random).setRoleAdmin(REGISTRAR_ROLE, EXECUTOR_ROLE) + ).to.be.revertedWith( + getAccessRevertMsg(random, GOVERNOR_ROLE) + ); + }); }); - // TODO AC: test setRoleAdmin for someone other than governor - // test initializer + describe("Role Validator Functions", () => { + it("Should return true for ADMIN_ROLE", async () => { + const [ admin ] = adminAccs; + const isAdmin = await znsAccessController.isAdmin(admin.address); + expect(isAdmin).to.be.true; + }); + + it("Should return true for REGISTRAR_ROLE", async () => { + const [ registrar ] = randomAccs; + await znsAccessController.connect(adminAccs[0]).grantRole(REGISTRAR_ROLE, registrar.address); + const isRegistrar = await znsAccessController.isRegistrar(registrar.address); + expect(isRegistrar).to.be.true; + }); + + it("Should revert if account does not have GOVERNOR_ROLE", async () => { + const [ { address: random } ] = randomAccs; + await expect( + znsAccessController.connect(random).checkGovernor(random) + ).to.be.revertedWith( + getAccessRevertMsg(random, GOVERNOR_ROLE) + ); + }); + + it("Should revert if account does not have ADMIN_ROLE", async () => { + const [ { address: random } ] = randomAccs; + await expect( + znsAccessController.connect(random).checkAdmin(random) + ).to.be.revertedWith( + getAccessRevertMsg(random, ADMIN_ROLE) + ); + }); + + it("Should revert if account does not have REGISTRAR_ROLE", async () => { + const [ { address: random } ] = randomAccs; + await expect( + znsAccessController.connect(random).checkRegistrar(random) + ).to.be.revertedWith( + getAccessRevertMsg(random, REGISTRAR_ROLE) + ); + }); + + it("Should revert if account does not have EXECUTOR_ROLE", async () => { + const [ { address: random } ] = randomAccs; + await expect( + znsAccessController.connect(random).checkExecutor(random) + ).to.be.revertedWith( + getAccessRevertMsg(random, EXECUTOR_ROLE) + ); + }); + }); }); diff --git a/test/ZNSAddressResolver.test.ts b/test/ZNSAddressResolver.test.ts index 9d9a17976..6a4c09f37 100644 --- a/test/ZNSAddressResolver.test.ts +++ b/test/ZNSAddressResolver.test.ts @@ -1,27 +1,29 @@ import * as hre from "hardhat"; import { ZNSRegistry, - ZNSRegistry__factory, ZNSAddressResolver, - ZNSAddressResolver__factory, - ERC165__factory, + ERC165__factory, ZNSAccessController, } from "../typechain"; import { SignerWithAddress } from "@nomiclabs/hardhat-ethers/signers"; import { hashDomainLabel, hashDomainName } from "./helpers/hashing"; +import { + ADMIN_ROLE, + deployAccessController, + deployAddressResolver, + deployRegistry, + getAccessRevertMsg, + REGISTRAR_ROLE, +} from "./helpers"; // eslint-disable-next-line @typescript-eslint/no-var-requires const { expect } = require("chai"); -/** - * TODO the registry should have a function for checking isOwnerOrOperator, - * so that the AddressResolver can implement a single call in its modifier. - * - * Consider moving these tests to ZNSRegistry since they deploy a registry. - */ + describe("ZNSAddressResolver", () => { let deployer : SignerWithAddress; let mockRegistrar : SignerWithAddress; let znsAddressResolver : ZNSAddressResolver; + let accessController : ZNSAccessController; let znsRegistry : ZNSRegistry; let owner : SignerWithAddress; let addr1 : SignerWithAddress; @@ -32,14 +34,15 @@ describe("ZNSAddressResolver", () => { [owner, addr1] = await hre.ethers.getSigners(); [deployer, operator, mockRegistrar] = await hre.ethers.getSigners(); - const znsAddressResolverFactory = new ZNSAddressResolver__factory(deployer); - const znsRegistryFactory = new ZNSRegistry__factory(deployer); - - znsRegistry = await znsRegistryFactory.deploy(); - znsAddressResolver = await znsAddressResolverFactory.deploy(znsRegistry.address); + accessController = await deployAccessController({ + deployer, + governorAddresses: [deployer.address], + adminAddresses: [deployer.address], + }); + await accessController.connect(deployer).grantRole(REGISTRAR_ROLE, mockRegistrar.address); - // Initialize registry and domain - await znsRegistry.connect(deployer).initialize(mockRegistrar.address); + znsRegistry = await deployRegistry(deployer, accessController.address); + znsAddressResolver = await deployAddressResolver(deployer, accessController.address, znsRegistry.address); // Have to get this value for every test, but can be fixed wilderDomainNameHash = hashDomainName("wilder"); @@ -63,14 +66,52 @@ describe("ZNSAddressResolver", () => { expect(notExistResolver).to.eq(hre.ethers.constants.AddressZero); }); - it("Should have registry address correctly set", async () => { + it("Should have registry address correctly set when initializing", async () => { expect(await znsAddressResolver.registry()).to.equal(znsRegistry.address); }); + it("Should setRegistry() correctly with ADMIN_ROLE", async () => { + await expect( + znsAddressResolver.connect(deployer).setRegistry(operator.address) + ) + .to.emit(znsAddressResolver, "RegistrySet") + .withArgs(operator.address); + + expect(await znsAddressResolver.registry()).to.equal(operator.address); + }); + + it("Should revert when setRegistry() without ADMIN_ROLE", async () => { + await expect( + znsAddressResolver.connect(operator).setRegistry(operator.address) + ).to.be.revertedWith( + getAccessRevertMsg(operator.address, ADMIN_ROLE) + ); + }); + + it("Should setAccessController() correctly with ADMIN_ROLE", async () => { + await expect( + znsAddressResolver.connect(deployer).setAccessController(operator.address) + ) + .to.emit(znsAddressResolver, "AccessControllerSet") + .withArgs(operator.address); + + expect(await znsAddressResolver.getAccessController()).to.equal(operator.address); + }); + + it("Should revert when setAccessController() without ADMIN_ROLE", async () => { + await expect( + znsAddressResolver.connect(operator).setAccessController(operator.address) + ).to.be.revertedWith( + getAccessRevertMsg(operator.address, ADMIN_ROLE) + ); + }); + it("Should not allow non-owner address to setAddress", async () => { await expect( znsAddressResolver.connect(addr1).setAddress(wilderDomainNameHash, addr1.address) - ).to.be.revertedWith("ZNSAddressResolver: Not allowed"); + ).to.be.revertedWith( + "ZNSAddressResolver: Not authorized for this domain" + ); }); it("Should allow owner to setAddress and emit event", async () => { @@ -80,6 +121,9 @@ describe("ZNSAddressResolver", () => { ) .to.emit(znsAddressResolver, "AddressSet") .withArgs(wilderDomainNameHash, addr1.address); + + const resolvedAddress = await znsAddressResolver.getAddress(wilderDomainNameHash); + expect(resolvedAddress).to.equal(addr1.address); }); it("Should allow operator to setAddress and emit event", async () => { @@ -93,10 +137,24 @@ describe("ZNSAddressResolver", () => { .withArgs(wilderDomainNameHash, addr1.address); }); - it("Should not allow owner to setAddress(0)", async () => { + it("Should allow REGISTRAR_ROLE to setAddress and emit event", async () => { + await accessController.connect(deployer).grantRole(REGISTRAR_ROLE, mockRegistrar.address); + await expect( - znsAddressResolver.connect(owner).setAddress(wilderDomainNameHash, hre.ethers.constants.AddressZero) - ).to.be.revertedWith("ZNS: Cant set address to 0"); + znsAddressResolver.connect(mockRegistrar) + .setAddress(wilderDomainNameHash, addr1.address) + ) + .to.emit(znsAddressResolver, "AddressSet") + .withArgs(wilderDomainNameHash, addr1.address); + }); + + it("Should allow owner to set address to 0", async () => { + await znsAddressResolver + .connect(owner) + .setAddress(wilderDomainNameHash, hre.ethers.constants.AddressZero); + + const resolvedAddress = await znsAddressResolver.getAddress(wilderDomainNameHash); + expect(resolvedAddress).to.equal(hre.ethers.constants.AddressZero); }); it("Should resolve address correctly", async () => { @@ -123,4 +181,15 @@ describe("ZNSAddressResolver", () => { const notSupported = await znsAddressResolver.supportsInterface("0xffffffff"); expect(notSupported).to.be.false; }); -}); \ No newline at end of file + + it("Should support full discovery flow from ZNSRegistry", async () => { + await znsAddressResolver.connect(owner) + .setAddress(wilderDomainNameHash, addr1.address); + + const resolverAddress = await znsRegistry.getDomainResolver(wilderDomainNameHash); + expect(resolverAddress).to.eq(znsAddressResolver.address); + + const resolvedAddress = await znsAddressResolver.getAddress(wilderDomainNameHash); + expect(resolvedAddress).to.eq(addr1.address); + }); +}); diff --git a/test/ZNSDomainToken.test.ts b/test/ZNSDomainToken.test.ts index 675c1e559..d57d882da 100644 --- a/test/ZNSDomainToken.test.ts +++ b/test/ZNSDomainToken.test.ts @@ -1,11 +1,18 @@ import * as hre from "hardhat"; import { + ZNSAccessController, ZNSDomainToken, } from "../typechain"; import { expect } from "chai"; import { SignerWithAddress } from "@nomiclabs/hardhat-ethers/signers"; import { ethers } from "ethers"; -import { deployDomainToken } from "./helpers"; +import { + ADMIN_ROLE, + deployAccessController, + deployDomainToken, + getAccessRevertMsg, INVALID_TOKENID_ERC_ERR, + REGISTRAR_ROLE, +} from "./helpers"; describe("ZNSDomainToken:", () => { @@ -13,16 +20,25 @@ describe("ZNSDomainToken:", () => { const TokenSymbol = "ZDT"; let deployer : SignerWithAddress; + let accessController : ZNSAccessController; let caller : SignerWithAddress; let domainToken : ZNSDomainToken; beforeEach(async () => { [deployer, caller] = await hre.ethers.getSigners(); - domainToken = await deployDomainToken(deployer); + + accessController = await deployAccessController({ + deployer, + governorAddresses: [deployer.address], + adminAddresses: [deployer.address], + }); + + domainToken = await deployDomainToken(deployer, accessController.address); + await accessController.connect(deployer).grantRole(REGISTRAR_ROLE, deployer.address); }); describe("External functions", () => { - it("Registers a token", async () => { + it("Should register (mint) the token if caller has REGISTRAR_ROLE", async () => { const tokenId = ethers.BigNumber.from("1"); const tx = await domainToken .connect(deployer) @@ -42,7 +58,18 @@ describe("ZNSDomainToken:", () => { expect(await domainToken.ownerOf(tokenId)).to.equal(caller.address); }); - it("Revokes a token", async () => { + it("Should revert when registering (minting) if caller does not have REGISTRAR_ROLE", async () => { + const tokenId = ethers.BigNumber.from("1"); + await expect( + domainToken + .connect(caller) + .register(caller.address, tokenId) + ).to.be.revertedWith( + getAccessRevertMsg(caller.address, REGISTRAR_ROLE) + ); + }); + + it("Should revoke (burn) the token if caller has REGISTRAR_ROLE", async () => { const tokenId = ethers.BigNumber.from("1"); // Mint domain await domainToken @@ -69,35 +96,42 @@ describe("ZNSDomainToken:", () => { // Verify token has been burned await expect( domainToken.ownerOf(tokenId) - ).to.be.revertedWith("ERC721: invalid token ID"); + ).to.be.revertedWith(INVALID_TOKENID_ERC_ERR); }); - }); - describe("Require Statement Validation", () => { - it("Only authorized can revoke a token", async () => { + it("Should revert when revoking (burning) if caller does not have REGISTRAR_ROLE", async () => { const tokenId = ethers.BigNumber.from("1"); // Mint domain await domainToken .connect(deployer) .register(caller.address, tokenId); - // Verify caller owns tokenId expect(await domainToken.ownerOf(tokenId)).to.equal( caller.address ); - // Verify caller owns tokenId - expect(await domainToken.ownerOf(tokenId)).to.equal(caller.address); - // Revoke domain const tx = domainToken.connect(caller).revoke(tokenId); await expect(tx).to.be.revertedWith( - "ZNS: Not authorized" + getAccessRevertMsg(caller.address, REGISTRAR_ROLE) ); // Verify token has not been burned expect(await domainToken.ownerOf(tokenId)).to.equal(caller.address); }); + + it("Should set access controller if caller has ADMIN_ROLE", async () => { + await domainToken.connect(deployer).setAccessController(caller.address); + expect(await domainToken.getAccessController()).to.equal(caller.address); + }); + + it("Should revert when setting access controller if caller does not have ADMIN_ROLE", async () => { + await expect( + domainToken.connect(caller).setAccessController(caller.address) + ).to.be.revertedWith( + getAccessRevertMsg(caller.address, ADMIN_ROLE) + ); + }); }); describe("Contract Configuration", () => { diff --git a/test/ZNSEthRegistrar.test.ts b/test/ZNSEthRegistrar.test.ts index 292706093..825871c38 100644 --- a/test/ZNSEthRegistrar.test.ts +++ b/test/ZNSEthRegistrar.test.ts @@ -1,7 +1,13 @@ import * as hre from "hardhat"; import { expect } from "chai"; import { SignerWithAddress } from "@nomiclabs/hardhat-ethers/signers"; -import { deployZNS } from "./helpers"; +import { + deployZNS, INVALID_TOKENID_ERC_ERR, + NOT_AUTHORIZED_REG_ERR, + NOT_NAME_OWNER_RAR_ERR, NOT_TOKEN_OWNER_RAR_ERR, + ONLY_NAME_OWNER_REG_ERR, + ONLY_OWNER_REGISTRAR_REG_ERR, +} from "./helpers"; import { ZNSContracts } from "./helpers/types"; import * as ethers from "ethers"; import { defaultRegistration } from "./helpers/registerDomain"; @@ -10,8 +16,9 @@ import { priceConfigDefault } from "./helpers/constants"; import { getPrice, getPriceObject } from "./helpers/pricing"; import { getDomainHashFromEvent, getTokenIdFromEvent } from "./helpers/events"; import { BigNumber } from "ethers"; -import { ADMIN_ROLE, getAccessRevertMsg } from "./helpers/access"; +import { ADMIN_ROLE } from "./helpers/access"; import { ZNSEthRegistrar__factory } from "../typechain"; +import { getAccessRevertMsg } from "./helpers/errors"; require("@nomicfoundation/hardhat-chai-matchers"); @@ -39,14 +46,6 @@ describe("ZNSEthRegistrar", () => { zeroVaultAddress: zeroVault.address, }); - // TODO AC: change this when access control implemented - // Give the user permission on behalf of the parent domain owner - await zns.registry.connect(deployer).setOwnerOperator(user.address, true); - - // TODO AC: change this when access control implemented - // Give the registrar permission on behalf of the user - await zns.registry.connect(user).setOwnerOperator(zns.registrar.address, true); - // Give funds to user await zns.zeroToken.connect(user).approve(zns.treasury.address, ethers.constants.MaxUint256); await zns.zeroToken.transfer(user.address, ethers.utils.parseEther("15")); @@ -60,8 +59,7 @@ describe("ZNSEthRegistrar", () => { expect(allowance).to.eq(ethers.constants.MaxUint256); }); - // TODO AC: is this a problem it is set up this way? - it("Should initialize correctly from an ADMIN account only", async () => { + it("Should revert when initialize() without ADMIN_ROLE", async () => { const userHasAdmin = await zns.accessController.hasRole(ADMIN_ROLE, user.address); expect(userHasAdmin).to.be.false; @@ -100,7 +98,7 @@ describe("ZNSEthRegistrar", () => { const balanceBeforeVault = await zns.zeroToken.balanceOf(zeroVault.address); // Deploy "wilder" with default configuration - const tx = await defaultRegistration(user, zns, defaultDomain); + const tx = await defaultRegistration(user, zns, defaultDomain, user.address); const domainHash = await getDomainHashFromEvent(tx); const { totalPrice, @@ -268,7 +266,7 @@ describe("ZNSEthRegistrar", () => { const tx = zns.registrar.connect(user).reclaimDomain(domainHash); // Verify Domain is not reclaimed - await expect(tx).to.be.revertedWith("ZNSEthRegistrar: Not the owner of the Token"); + await expect(tx).to.be.revertedWith(NOT_TOKEN_OWNER_RAR_ERR); // Verify domain is not owned in registrar const registryOwner = await zns.registry.connect(user).getDomainOwner(domainHash); @@ -281,7 +279,7 @@ describe("ZNSEthRegistrar", () => { const tx = zns.registrar.connect(user).reclaimDomain(domainHash); // Verify Domain is not reclaimed - await expect(tx).to.be.revertedWith("ERC721: invalid token ID"); + await expect(tx).to.be.revertedWith(INVALID_TOKENID_ERC_ERR); }); it("Domain Token can be reclaimed, transferred, and then reclaimed again", async () => { @@ -370,7 +368,7 @@ describe("ZNSEthRegistrar", () => { // Verify token has been burned const ownerOfTx = zns.domainToken.connect(user).ownerOf(tokenId); await expect(ownerOfTx).to.be.revertedWith( - "ERC721: invalid token ID" + INVALID_TOKENID_ERC_ERR ); // Verify Domain Record Deleted @@ -386,7 +384,7 @@ describe("ZNSEthRegistrar", () => { // Verify transaction is reverted const tx = zns.registrar.connect(user).revokeDomain(fakeHash); - await expect(tx).to.be.revertedWith("ZNSEthRegistrar: Not the Owner of the Name"); + await expect(tx).to.be.revertedWith(NOT_NAME_OWNER_RAR_ERR); }); it("Revoking domain unstakes", async () => { @@ -433,7 +431,7 @@ describe("ZNSEthRegistrar", () => { // Try to revoke domain const tx = zns.registrar.connect(user).revokeDomain(parentDomainHash); - await expect(tx).to.be.revertedWith("ZNSEthRegistrar: Not the Owner of the Name"); + await expect(tx).to.be.revertedWith(NOT_NAME_OWNER_RAR_ERR); }); it("No one can revoke if Token and Name have different owners", async () => { @@ -449,10 +447,10 @@ describe("ZNSEthRegistrar", () => { // Try to revoke domain as a new owner of the token const tx = zns.registrar.connect(user).revokeDomain(parentDomainHash); - await expect(tx).to.be.revertedWith("ZNSEthRegistrar: Not the Owner of the Name"); + await expect(tx).to.be.revertedWith(NOT_NAME_OWNER_RAR_ERR); const tx2 = zns.registrar.connect(deployer).revokeDomain(parentDomainHash); - await expect(tx2).to.be.revertedWith("ZNSEthRegistrar: Not the owner of the Token"); + await expect(tx2).to.be.revertedWith(NOT_TOKEN_OWNER_RAR_ERR); }); it("After domain has been revoked, an old operator can NOT access Registry", async () => { @@ -473,7 +471,9 @@ describe("ZNSEthRegistrar", () => { domainHash, operator.address ); - await expect(tx2).to.be.revertedWith("ZNSRegistry: Not authorized"); + await expect(tx2).to.be.revertedWith( + ONLY_OWNER_REGISTRAR_REG_ERR + ); const tx3 = zns.registry .connect(operator) @@ -482,7 +482,9 @@ describe("ZNSEthRegistrar", () => { user.address, operator.address ); - await expect(tx3).to.be.revertedWith("ZNSRegistry: Not authorized"); + await expect(tx3).to.be.revertedWith( + ONLY_NAME_OWNER_REG_ERR + ); const tx4 = zns.registry .connect(operator) @@ -490,7 +492,9 @@ describe("ZNSEthRegistrar", () => { domainHash, zeroVault.address ); - await expect(tx4).to.be.revertedWith("ZNSRegistry: Not authorized"); + await expect(tx4).to.be.revertedWith( + NOT_AUTHORIZED_REG_ERR + ); }); }); @@ -521,101 +525,101 @@ describe("ZNSEthRegistrar", () => { }); describe("#setZnsRegistry", () => { - it("Should set ZNSRegistry and fire ZnsRegistrySet event", async () => { - const currentRegistry = await zns.registrar.znsRegistry(); - const tx = await zns.registrar.connect(deployer).setZnsRegistry(randomAcc.address); - const newRegistry = await zns.registrar.znsRegistry(); + it("Should set ZNSRegistry and fire RegistrySet event", async () => { + const currentRegistry = await zns.registrar.registry(); + const tx = await zns.registrar.connect(deployer).setRegistry(randomAcc.address); + const newRegistry = await zns.registrar.registry(); - await expect(tx).to.emit(zns.registrar, "ZnsRegistrySet").withArgs(randomAcc.address); + await expect(tx).to.emit(zns.registrar, "RegistrySet").withArgs(randomAcc.address); expect(newRegistry).to.equal(randomAcc.address); expect(currentRegistry).to.not.equal(newRegistry); }); it("Should revert if not called by ADMIN", async () => { - const tx = zns.registrar.connect(user).setZnsRegistry(randomAcc.address); + const tx = zns.registrar.connect(user).setRegistry(randomAcc.address); await expect(tx).to.be.revertedWith( getAccessRevertMsg(user.address, ADMIN_ROLE) ); }); it("Should revert if ZNSRegistry is address zero", async () => { - const tx = zns.registrar.connect(deployer).setZnsRegistry(ethers.constants.AddressZero); + const tx = zns.registrar.connect(deployer).setRegistry(ethers.constants.AddressZero); await expect(tx).to.be.revertedWith("ZNSEthRegistrar: znsRegistry_ is 0x0 address"); }); }); - describe("#setZnsTreasury", () => { + describe("#setTreasury", () => { it("Should set ZNSTreasury and fire ZnsTreasurySet event", async () => { - const currentTreasury = await zns.registrar.znsTreasury(); - const tx = await zns.registrar.connect(deployer).setZnsTreasury(randomAcc.address); - const newTreasury = await zns.registrar.znsTreasury(); + const currentTreasury = await zns.registrar.treasury(); + const tx = await zns.registrar.connect(deployer).setTreasury(randomAcc.address); + const newTreasury = await zns.registrar.treasury(); - await expect(tx).to.emit(zns.registrar, "ZnsTreasurySet").withArgs(randomAcc.address); + await expect(tx).to.emit(zns.registrar, "TreasurySet").withArgs(randomAcc.address); expect(newTreasury).to.equal(randomAcc.address); expect(currentTreasury).to.not.equal(newTreasury); }); it("Should revert if not called by ADMIN", async () => { - const tx = zns.registrar.connect(user).setZnsTreasury(randomAcc.address); + const tx = zns.registrar.connect(user).setTreasury(randomAcc.address); await expect(tx).to.be.revertedWith( getAccessRevertMsg(user.address, ADMIN_ROLE) ); }); it("Should revert if ZNSTreasury is address zero", async () => { - const tx = zns.registrar.connect(deployer).setZnsTreasury(ethers.constants.AddressZero); + const tx = zns.registrar.connect(deployer).setTreasury(ethers.constants.AddressZero); await expect(tx).to.be.revertedWith("ZNSEthRegistrar: znsTreasury_ is 0x0 address"); }); }); describe("#setZnsDomainToken", () => { it("Should set ZNSDomainToken and fire ZnsDomainTokenSet event", async () => { - const currentToken = await zns.registrar.znsDomainToken(); - const tx = await zns.registrar.connect(deployer).setZnsDomainToken(randomAcc.address); - const newToken = await zns.registrar.znsDomainToken(); + const currentToken = await zns.registrar.domainToken(); + const tx = await zns.registrar.connect(deployer).setDomainToken(randomAcc.address); + const newToken = await zns.registrar.domainToken(); - await expect(tx).to.emit(zns.registrar, "ZnsDomainTokenSet").withArgs(randomAcc.address); + await expect(tx).to.emit(zns.registrar, "DomainTokenSet").withArgs(randomAcc.address); expect(newToken).to.equal(randomAcc.address); expect(currentToken).to.not.equal(newToken); }); it("Should revert if not called by ADMIN", async () => { - const tx = zns.registrar.connect(user).setZnsDomainToken(randomAcc.address); + const tx = zns.registrar.connect(user).setDomainToken(randomAcc.address); await expect(tx).to.be.revertedWith( getAccessRevertMsg(user.address, ADMIN_ROLE) ); }); it("Should revert if ZNSDomainToken is address zero", async () => { - const tx = zns.registrar.connect(deployer).setZnsDomainToken(ethers.constants.AddressZero); + const tx = zns.registrar.connect(deployer).setDomainToken(ethers.constants.AddressZero); await expect(tx).to.be.revertedWith("ZNSEthRegistrar: znsDomainToken_ is 0x0 address"); }); }); - describe("#setZnsAddressResolver", () => { - it("Should set ZNSAddressResolver and fire ZnsAddressResolverSet event", async () => { - const currentResolver = await zns.registrar.znsAddressResolver(); - const tx = await zns.registrar.connect(deployer).setZnsAddressResolver(randomAcc.address); - const newResolver = await zns.registrar.znsAddressResolver(); + describe("#setAddressResolver", () => { + it("Should set ZNSAddressResolver and fire AddressResolverSet event", async () => { + const currentResolver = await zns.registrar.addressResolver(); + const tx = await zns.registrar.connect(deployer).setAddressResolver(randomAcc.address); + const newResolver = await zns.registrar.addressResolver(); - await expect(tx).to.emit(zns.registrar, "ZnsAddressResolverSet").withArgs(randomAcc.address); + await expect(tx).to.emit(zns.registrar, "AddressResolverSet").withArgs(randomAcc.address); expect(newResolver).to.equal(randomAcc.address); expect(currentResolver).to.not.equal(newResolver); }); it("Should revert if not called by ADMIN", async () => { - const tx = zns.registrar.connect(user).setZnsAddressResolver(randomAcc.address); + const tx = zns.registrar.connect(user).setAddressResolver(randomAcc.address); await expect(tx).to.be.revertedWith( getAccessRevertMsg(user.address, ADMIN_ROLE) ); }); it("Should revert if ZNSAddressResolver is address zero", async () => { - const tx = zns.registrar.connect(deployer).setZnsAddressResolver(ethers.constants.AddressZero); + const tx = zns.registrar.connect(deployer).setAddressResolver(ethers.constants.AddressZero); await expect(tx).to.be.revertedWith("ZNSEthRegistrar: znsAddressResolver_ is 0x0 address"); }); }); diff --git a/test/ZNSPriceOracle.test.ts b/test/ZNSPriceOracle.test.ts index 53e99edae..38c685a53 100644 --- a/test/ZNSPriceOracle.test.ts +++ b/test/ZNSPriceOracle.test.ts @@ -4,9 +4,10 @@ import { SignerWithAddress } from "@nomiclabs/hardhat-ethers/signers"; import { BigNumber, ethers } from "ethers"; import { parseEther } from "ethers/lib/utils"; import { ZNSContracts } from "./helpers/types"; -import { deployZNS, getPrice } from "./helpers"; +import { deployZNS, getPrice, MULTIPLIER_OUT_OF_RANGE_ORA_ERR } from "./helpers"; import { priceConfigDefault, registrationFeePercDefault } from "./helpers/constants"; -import { ADMIN_ROLE, getAccessRevertMsg } from "./helpers/access"; +import { ADMIN_ROLE } from "./helpers/access"; +import { getAccessRevertMsg } from "./helpers/errors"; require("@nomicfoundation/hardhat-chai-matchers"); @@ -345,7 +346,7 @@ describe("ZNSPriceOracle", () => { const newMultiplier = BigNumber.from("299"); const tx = zns.priceOracle.connect(deployer).setPriceMultiplier(newMultiplier); - await expect(tx).to.be.revertedWith("ZNS: Multiplier out of range"); + await expect(tx).to.be.revertedWith(MULTIPLIER_OUT_OF_RANGE_ORA_ERR); }); it("Fails when setting to a value above the specified range", async () => { @@ -353,7 +354,7 @@ describe("ZNSPriceOracle", () => { const newMultiplier = BigNumber.from("401"); const tx = zns.priceOracle.connect(deployer).setPriceMultiplier(newMultiplier); - await expect(tx).to.be.revertedWith("ZNS: Multiplier out of range"); + await expect(tx).to.be.revertedWith(MULTIPLIER_OUT_OF_RANGE_ORA_ERR); }); it("Succeeds when setting a value within the allowed range", async () => { diff --git a/test/ZNSRegistry.test.ts b/test/ZNSRegistry.test.ts index 6e3783beb..3b561d5e8 100644 --- a/test/ZNSRegistry.test.ts +++ b/test/ZNSRegistry.test.ts @@ -1,10 +1,22 @@ import * as hre from "hardhat"; import { expect } from "chai"; import { SignerWithAddress } from "@nomiclabs/hardhat-ethers/signers"; -import { ZNSRegistry } from "../typechain"; +import { ZNSAccessController, ZNSRegistry } from "../typechain"; import { deployRegistry } from "./helpers/deployZNS"; import { ethers } from "ethers"; import { hashDomainLabel, hashDomainName } from "./helpers/hashing"; +import { + ADMIN_ROLE, + deployAccessController, + getAccessRevertMsg, INITIALIZED_ERR, + REGISTRAR_ROLE, +} from "./helpers"; +import { + ONLY_NAME_OWNER_REG_ERR, + NOT_AUTHORIZED_REG_ERR, + ONLY_OWNER_REGISTRAR_REG_ERR, + OWNER_NOT_ZERO_REG_ERR, +} from "./helpers/errors"; // eslint-disable-next-line @typescript-eslint/no-var-requires require("@nomicfoundation/hardhat-chai-matchers"); @@ -20,12 +32,20 @@ describe("ZNSRegistry Tests", () => { let mockRegistrar : SignerWithAddress; let registry : ZNSRegistry; + let accessController : ZNSAccessController; let wilderDomainHash : string; beforeEach(async () => { [deployer, operator, randomUser, mockResolver, mockRegistrar] = await hre.ethers.getSigners(); - registry = await deployRegistry(deployer, mockRegistrar); + accessController = await deployAccessController({ + deployer, + governorAddresses: [deployer.address], + adminAddresses: [deployer.address], + }); + await accessController.connect(deployer).grantRole(REGISTRAR_ROLE, mockRegistrar.address); + + registry = await deployRegistry(deployer, accessController.address); wilderDomainHash = hashDomainName("wilder"); @@ -36,7 +56,34 @@ describe("ZNSRegistry Tests", () => { ); }); - // a valid operator can change the owner of a domain, is this wanted? + it("Cannot be initialized twice", async () => { + await expect( + registry.initialize( + accessController.address + ) + ).to.be.revertedWith( + INITIALIZED_ERR + ); + }); + + it("Should set access controller correctly with ADMIN_ROLE", async () => { + const currentAC = await registry.getAccessController(); + + await registry.connect(deployer).setAccessController(randomUser.address); + const newAC = await registry.getAccessController(); + + expect(currentAC).to.not.equal(newAC); + expect(newAC).to.equal(randomUser.address); + }); + + it("Should revert when setting access controller without ADMIN_ROLE", async () => { + await expect( + registry.connect(randomUser).setAccessController(deployer.address) + ).to.be.revertedWith( + getAccessRevertMsg(randomUser.address, ADMIN_ROLE) + ); + }); + describe("Operator functionality", () => { it("Returns false when an operator is not allowed by an owner", async () => { await registry.connect(deployer).setOwnerOperator(operator.address, false); @@ -64,7 +111,7 @@ describe("ZNSRegistry Tests", () => { expect(allowed).to.be.false; }); - it("Permits an allowed operator to modify a domain record", async () => { + it("Permits an allowed operator to update a domain resolver", async () => { await registry.connect(deployer).setOwnerOperator(operator.address, true); const tx = registry @@ -73,16 +120,34 @@ describe("ZNSRegistry Tests", () => { await expect(tx).to.not.be.reverted; }); - it("Does not permit a disallowed operator to modify a domain record", async () => { + it("Does not permit an allowed operator to update a domain owner", async () => { + await registry.connect(deployer).setOwnerOperator(operator.address, true); + + const tx = registry + .connect(operator) + .updateDomainOwner(wilderDomainHash, operator.address); + await expect(tx).to.be.revertedWith(ONLY_OWNER_REGISTRAR_REG_ERR); + }); + + it("Does not permit an allowed operator to update a domain record", async () => { + await registry.connect(deployer).setOwnerOperator(operator.address, true); + + const tx = registry + .connect(operator) + .updateDomainRecord(wilderDomainHash, operator.address, operator.address); + await expect(tx).to.be.revertedWith(ONLY_NAME_OWNER_REG_ERR); + }); + + it("Does not permit a disallowed operator to update a domain record", async () => { await registry.connect(deployer).setOwnerOperator(operator.address, false); const tx = registry.connect(operator).updateDomainResolver(wilderDomainHash, operator.address); - await expect(tx).to.be.revertedWith("ZNSRegistry: Not authorized"); + await expect(tx).to.be.revertedWith(NOT_AUTHORIZED_REG_ERR); }); it("Does not permit an operator that's never been allowed to modify a record", async () => { const tx = registry.connect(operator).updateDomainResolver(wilderDomainHash, operator.address); - await expect(tx).to.be.revertedWith("ZNSRegistry: Not authorized"); + await expect(tx).to.be.revertedWith(NOT_AUTHORIZED_REG_ERR); }); }); @@ -140,7 +205,7 @@ describe("ZNSRegistry Tests", () => { ); }); - it("Fails to create a new domain record if the caller is not the registrar", async () => { + it("Fails to create a new domain record if the caller does not have REGISTRAR_ROLE", async () => { const domainHash = hashDomainLabel("world"); const tx = registry.connect(deployer).createDomainRecord( @@ -149,28 +214,23 @@ describe("ZNSRegistry Tests", () => { mockResolver.address ); - await expect(tx).to.be.revertedWith("ZNSRegistry: Caller is not the Registrar"); + await expect(tx).to.be.revertedWith( + getAccessRevertMsg(deployer.address, REGISTRAR_ROLE) + ); }); }); describe("Setter functions for a domain's record, owner, or resolver", () => { - // setters pass if domain exists - // updates a domain record - // fails to update a domain record if that domain does not exist - // deleteRecord works - - // can you get around this by setting operators for zero address? - - it("Cannot set a domain record if the domain doesn't exist", async () => { + it("Cannot update a domain record if the domain doesn't exist", async () => { const domainHash = hashDomainLabel("world"); const tx = registry.updateDomainRecord(domainHash, deployer.address, mockResolver.address); // Because nobody owns a non-existing record, the error is caught by the `onlyOwnerOrOperator` first - await expect(tx).to.be.revertedWith("ZNSRegistry: Not authorized"); + await expect(tx).to.be.revertedWith(ONLY_NAME_OWNER_REG_ERR); }); - it("Can set a domain record if the domain exists", async () => { + it("Can update a domain record if the domain exists", async () => { const domainHash = hashDomainLabel("world"); await registry.connect(mockRegistrar).createDomainRecord(domainHash, deployer.address, mockResolver.address); @@ -182,16 +242,18 @@ describe("ZNSRegistry Tests", () => { expect(record.resolver).to.eq(deployer.address); }); - it("Cannot set a domain owner if the domain doesn't exist", async () => { + it("Cannot update a domain owner if the domain doesn't exist", async () => { const domainHash = hashDomainLabel("world"); const tx = registry.updateDomainOwner(domainHash, deployer.address); // Because nobody owns a non-existing record, the error is caught by the `onlyOwnerOrOperator` first - await expect(tx).to.be.revertedWith("ZNSRegistry: Not authorized"); + await expect(tx).to.be.revertedWith( + ONLY_OWNER_REGISTRAR_REG_ERR + ); }); - it("Can set a domain owner if the domain exists", async () => { + it("Can update a domain owner if the domain exists", async () => { const domainHash = hashDomainLabel("world"); await registry.connect(mockRegistrar).createDomainRecord(domainHash, deployer.address, mockResolver.address); @@ -202,15 +264,15 @@ describe("ZNSRegistry Tests", () => { expect(record.owner).to.eq(mockRegistrar.address); }); - it("Cannot set a domain resolver if the domain doesn't exist", async () => { + it("Cannot update a domain resolver if the domain doesn't exist", async () => { const domainHash = hashDomainLabel("world"); const tx = registry.updateDomainResolver(domainHash, mockResolver.address); // Because nobody owns a non-existing record, the error is caught by the `onlyOwnerOrOperator` first - await expect(tx).to.be.revertedWith("ZNSRegistry: Not authorized"); + await expect(tx).to.be.revertedWith(NOT_AUTHORIZED_REG_ERR); }); - it("Can set a domain resolver if the domain exists", async () => { + it("Can update a domain resolver if the domain exists", async () => { const domainHash = hashDomainLabel("world"); await registry.connect(mockRegistrar).createDomainRecord(domainHash, deployer.address, mockResolver.address); @@ -221,16 +283,16 @@ describe("ZNSRegistry Tests", () => { expect(record.resolver).to.eq(deployer.address); }); - it("Cannot set a domain record if the owner is zero address", async () => { + it("Cannot update a domain record if the owner is zero address", async () => { const domainHash = hashDomainLabel("world"); await registry.connect(mockRegistrar).createDomainRecord(domainHash, deployer.address, mockResolver.address); const tx = registry.updateDomainRecord(domainHash, ethers.constants.AddressZero, mockResolver.address); - await expect(tx).to.be.revertedWith("ZNSRegistry: Owner cannot be zero address"); + await expect(tx).to.be.revertedWith(OWNER_NOT_ZERO_REG_ERR); }); - it("Can set a domain record if the resolver is zero address", async () => { + it("Can update a domain record if the resolver is zero address", async () => { const domainHash = hashDomainLabel("world"); await registry.connect(mockRegistrar).createDomainRecord(domainHash, deployer.address, mockResolver.address); @@ -239,7 +301,7 @@ describe("ZNSRegistry Tests", () => { await expect(tx).to.be.fulfilled; }); - it("Cannot set a domain owner if owner is zero address", async () => { + it("Cannot update a domain owner if owner is zero address", async () => { const tx = registry .connect(deployer) .updateDomainOwner( @@ -247,10 +309,10 @@ describe("ZNSRegistry Tests", () => { ethers.constants.AddressZero ); - await expect(tx).to.be.revertedWith("ZNSRegistry: Owner cannot be zero address"); + await expect(tx).to.be.revertedWith(OWNER_NOT_ZERO_REG_ERR); }); - it("Can set a domain resolver if resolver is zero address", async () => { + it("Can update a domain resolver if resolver is zero address", async () => { await registry .connect(deployer) .updateDomainResolver( @@ -263,7 +325,7 @@ describe("ZNSRegistry Tests", () => { expect(zeroResolver).to.be.eq(ethers.constants.AddressZero); }); - it("Fails to set a record when caller is not owner or operator", async () => { + it("Fails to set a record when caller is not owner", async () => { const tx = registry .connect(operator) .updateDomainRecord( @@ -271,7 +333,7 @@ describe("ZNSRegistry Tests", () => { operator.address, mockResolver.address ); - await expect(tx).to.be.revertedWith("ZNSRegistry: Not authorized"); + await expect(tx).to.be.revertedWith(ONLY_NAME_OWNER_REG_ERR); }); it("Cannot set a domain's record if not an owner or operator", async () => { @@ -280,7 +342,7 @@ describe("ZNSRegistry Tests", () => { await registry.connect(mockRegistrar).createDomainRecord(domainHash, deployer.address, mockResolver.address); const tx = registry.connect(randomUser).updateDomainRecord(domainHash, mockResolver.address, deployer.address); - await expect(tx).to.be.revertedWith("ZNSRegistry: Not authorized"); + await expect(tx).to.be.revertedWith(ONLY_NAME_OWNER_REG_ERR); }); it("Cannot set an domain's owner if not an owner or operator", async () => { @@ -289,7 +351,9 @@ describe("ZNSRegistry Tests", () => { await registry.connect(mockRegistrar).createDomainRecord(domainHash, deployer.address, mockResolver.address); const tx = registry.connect(randomUser).updateDomainOwner(domainHash, mockResolver.address); - await expect(tx).to.be.revertedWith("ZNSRegistry: Not authorized"); + await expect(tx).to.be.revertedWith( + ONLY_OWNER_REGISTRAR_REG_ERR + ); }); it("Cannot set a domain's resolver if not an owner or operator", async () => { @@ -298,17 +362,33 @@ describe("ZNSRegistry Tests", () => { await registry.connect(mockRegistrar).createDomainRecord(domainHash, deployer.address, mockResolver.address); const tx = registry.connect(randomUser).updateDomainResolver(domainHash, deployer.address); - await expect(tx).to.be.revertedWith("ZNSRegistry: Not authorized"); + await expect(tx).to.be.revertedWith(NOT_AUTHORIZED_REG_ERR); }); - }); - describe("Event emitters", () => { - it("Emits an event when the registrar is set", async () => { - // TODO currently no AC on this function, make sure it's added - const tx = registry.connect(deployer).setZNSRegistrar(mockResolver.address); - await expect(tx).to.emit(registry, "ZNSRegistrarSet").withArgs(mockResolver.address); + it("Can delete record with REGISTRAR_ROLE", async () => { + const domainHash = hashDomainLabel("world"); + + await registry.connect(mockRegistrar).createDomainRecord(domainHash, deployer.address, mockResolver.address); + await registry.connect(mockRegistrar).deleteRecord(domainHash); + + const record = await registry.getDomainRecord(domainHash); + + expect(record.owner).to.eq(ethers.constants.AddressZero); + }); + + it("Cannot delete record without REGISTRAR_ROLE", async () => { + const domainHash = hashDomainLabel("world"); + + await registry.connect(mockRegistrar).createDomainRecord(domainHash, deployer.address, mockResolver.address); + const tx = registry.connect(randomUser).deleteRecord(domainHash); + + await expect(tx).to.be.revertedWith( + getAccessRevertMsg(randomUser.address, REGISTRAR_ROLE) + ); }); + }); + describe("Event emitters", () => { it("Emits an event when an operator is set", async () => { // TODO currently no AC on this function, make sure it's added const tx = registry.connect(deployer).setOwnerOperator(randomUser.address, true); @@ -317,7 +397,6 @@ describe("ZNSRegistry Tests", () => { deployer.address, randomUser.address, true, - ); }); @@ -396,6 +475,4 @@ describe("ZNSRegistry Tests", () => { await expect(tx).to.emit(registry, "DomainRecordDeleted").withArgs(wilderDomainHash); }); }); - // TODO test delete works - // TODO event emitter tests }); diff --git a/test/ZNSTreasury.test.ts b/test/ZNSTreasury.test.ts index 1e8d9187f..e711e94f1 100644 --- a/test/ZNSTreasury.test.ts +++ b/test/ZNSTreasury.test.ts @@ -5,7 +5,8 @@ import { checkBalance, deployZNS } from "./helpers"; import { ZNSContracts } from "./helpers/types"; import * as ethers from "ethers"; import { hashDomainLabel } from "./helpers/hashing"; -import { ADMIN_ROLE, getAccessRevertMsg, REGISTRAR_ROLE } from "./helpers/access"; +import { ADMIN_ROLE, REGISTRAR_ROLE } from "./helpers/access"; +import { getAccessRevertMsg } from "./helpers/errors"; require("@nomicfoundation/hardhat-chai-matchers"); @@ -46,7 +47,7 @@ describe("ZNSTreasury", () => { }); it("Confirms deployment", async () => { - const priceOracle = await zns.treasury.znsPriceOracle(); + const priceOracle = await zns.treasury.priceOracle(); const token = await zns.treasury.stakingToken(); const accessController = await zns.treasury.getAccessController(); @@ -166,17 +167,17 @@ describe("ZNSTreasury", () => { }); }); - describe("#setPriceOracle() and ZnsPriceOracleSet event", () => { + describe("#setPriceOracle() and PriceOracleSet event", () => { it("Should set the correct address of ZNS Price Oracle", async () => { - const currentPriceOracle = await zns.treasury.znsPriceOracle(); + const currentPriceOracle = await zns.treasury.priceOracle(); expect(currentPriceOracle).to.not.eq(randomAcc.address); const tx = await zns.treasury.setPriceOracle(randomAcc.address); - const newPriceOracle = await zns.treasury.znsPriceOracle(); + const newPriceOracle = await zns.treasury.priceOracle(); expect(newPriceOracle).to.eq(randomAcc.address); - await expect(tx).to.emit(zns.treasury, "ZnsPriceOracleSet").withArgs(randomAcc.address); + await expect(tx).to.emit(zns.treasury, "PriceOracleSet").withArgs(randomAcc.address); }); it("Should revert when called from any address without ADMIN_ROLE", async () => { @@ -202,7 +203,7 @@ describe("ZNSTreasury", () => { const newStakingToken = await zns.treasury.stakingToken(); expect(newStakingToken).to.eq(randomAcc.address); - await expect(tx).to.emit(zns.treasury, "ZnsStakingTokenSet").withArgs(randomAcc.address); + await expect(tx).to.emit(zns.treasury, "StakingTokenSet").withArgs(randomAcc.address); }); it("Should revert when called from any address without ADMIN_ROLE", async () => { diff --git a/test/helpers/access.ts b/test/helpers/access.ts index ed0a50a23..4b66f486b 100644 --- a/test/helpers/access.ts +++ b/test/helpers/access.ts @@ -17,9 +17,9 @@ export const REGISTRAR_ROLE = ethers.utils.solidityKeccak256( ["REGISTRAR_ROLE"] ); -export const OPERATOR_ROLE = ethers.utils.solidityKeccak256( +export const EXECUTOR_ROLE = ethers.utils.solidityKeccak256( ["string"], - ["OPERATOR_ROLE"] + ["EXECUTOR_ROLE"] ); export const deployAccessController = async ({ @@ -37,6 +37,3 @@ export const deployAccessController = async ({ await controller.initialize(governorAddresses, adminAddresses); return controller; }; - -export const getAccessRevertMsg = (addr : string, role : string) : string => - `AccessControl: account ${addr.toLowerCase()} is missing role ${role}`; diff --git a/test/helpers/deployZNS.ts b/test/helpers/deployZNS.ts index e081d0c7a..89d47c089 100644 --- a/test/helpers/deployZNS.ts +++ b/test/helpers/deployZNS.ts @@ -24,23 +24,27 @@ import { BigNumber } from "ethers"; export const deployRegistry = async ( deployer : SignerWithAddress, - registrar : SignerWithAddress + accessControllerAddress : string ) : Promise => { const registryFactory = new ZNSRegistry__factory(deployer); const registry = await registryFactory.deploy(); // To set the owner of the zero domain to the deployer - await registry.connect(deployer).initialize(registrar.address); + await registry.connect(deployer).initialize(accessControllerAddress); return registry; }; export const deployAddressResolver = async ( deployer : SignerWithAddress, + accessControllerAddress : string, registryAddress : string ) : Promise => { const addressResolverFactory = new ZNSAddressResolver__factory(deployer); - const addressResolver = await addressResolverFactory.deploy(registryAddress); + const addressResolver = await addressResolverFactory.deploy( + accessControllerAddress, + registryAddress + ); return addressResolver; }; @@ -69,10 +73,15 @@ export const deployPriceOracle = async ({ }; export const deployDomainToken = async ( - deployer : SignerWithAddress + deployer : SignerWithAddress, + accessControllerAddress : string ) : Promise => { const domainTokenFactory = new ZNSDomainToken__factory(deployer); - return domainTokenFactory.deploy("ZNSDomainToken", "ZDT"); + return domainTokenFactory.deploy( + "ZNSDomainToken", + "ZDT", + accessControllerAddress + ); }; export const deployZeroTokenMock = async ( @@ -139,16 +148,17 @@ export const deployZNS = async ({ adminAddresses: [deployer.address, ...adminAddresses], }); - // TODO AC: fix this! - // Can't set to zero, but registrar address must be given. - // Due to order of deployment, add deployer as registrar address for now and change after - const registry = await deployRegistry(deployer, deployer); + const registry = await deployRegistry(deployer, accessController.address); - const domainToken = await deployDomainToken(deployer); + const domainToken = await deployDomainToken(deployer, accessController.address); const zeroTokenMock = await deployZeroTokenMock(deployer); - const addressResolver = await deployAddressResolver(deployer, registry.address); + const addressResolver = await deployAddressResolver( + deployer, + accessController.address, + registry.address + ); const priceOracle = await deployPriceOracle({ deployer, @@ -185,13 +195,6 @@ export const deployZNS = async ({ registrar, }; - // Final configuration steps - // TODO AC: remove all redundant calls here! and delete hashing of the root and the need - // for Registrar to be owner/operator of the root - await domainToken.connect(deployer).authorize(registrar.address); - await registry.connect(deployer).setOwnerOperator(registrar.address, true); - await registry.connect(deployer).setZNSRegistrar(registrar.address); - // Give 15 ZERO to the deployer and allowance to the treasury await zeroTokenMock.connect(deployer).approve(treasury.address, ethers.constants.MaxUint256); await zeroTokenMock.transfer(deployer.address, ethers.utils.parseEther("15")); diff --git a/test/helpers/errors.ts b/test/helpers/errors.ts new file mode 100644 index 000000000..5ceb84db1 --- /dev/null +++ b/test/helpers/errors.ts @@ -0,0 +1,15 @@ +export const getAccessRevertMsg = (addr : string, role : string) : string => + `AccessControl: account ${addr.toLowerCase()} is missing role ${role}`; + +// revert messages +// When adding a revert test, check if this message is already present in other tests +// if it is, add a new constant here and use it in all tests +export const ONLY_NAME_OWNER_REG_ERR = "ZNSRegistry: Not the Name Owner"; +export const ONLY_OWNER_REGISTRAR_REG_ERR = "ZNSRegistry: Only Name Owner or Registrar allowed to call"; +export const NOT_AUTHORIZED_REG_ERR = "ZNSRegistry: Not authorized"; +export const OWNER_NOT_ZERO_REG_ERR = "ZNSRegistry: Owner cannot be zero address"; +export const NOT_NAME_OWNER_RAR_ERR = "ZNSEthRegistrar: Not the Owner of the Name"; +export const NOT_TOKEN_OWNER_RAR_ERR = "ZNSEthRegistrar: Not the Owner of the Token"; +export const MULTIPLIER_OUT_OF_RANGE_ORA_ERR = "ZNSPriceOracle: Multiplier out of range"; +export const INVALID_TOKENID_ERC_ERR = "ERC721: invalid token ID"; +export const INITIALIZED_ERR = "Initializable: contract is already initialized"; diff --git a/test/helpers/index.ts b/test/helpers/index.ts index 8f00f4b6d..fe2c8fcea 100644 --- a/test/helpers/index.ts +++ b/test/helpers/index.ts @@ -5,3 +5,4 @@ export * from "./hashing"; export * from "./constants"; export * from "./balances"; export * from "./access"; +export * from "./errors";