From 641300e7fa192e9fc8ddc26323a82217ae294ee8 Mon Sep 17 00:00:00 2001 From: Kirill Date: Wed, 24 May 2023 15:57:41 -0700 Subject: [PATCH] remove redundant comments + small fixes --- contracts/access/ZNSAccessController.sol | 8 +++----- contracts/access/ZNSRoles.sol | 8 +++----- contracts/distribution/ZNSEthRegistrar.sol | 2 -- contracts/distribution/ZNSTreasury.sol | 1 - test/ZNSEthRegistrar.test.ts | 3 +-- test/helpers/deployZNS.ts | 5 ----- 6 files changed, 7 insertions(+), 20 deletions(-) diff --git a/contracts/access/ZNSAccessController.sol b/contracts/access/ZNSAccessController.sol index c30f3e5e1..0975ccd09 100644 --- a/contracts/access/ZNSAccessController.sol +++ b/contracts/access/ZNSAccessController.sol @@ -16,11 +16,11 @@ 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); } @@ -50,14 +50,12 @@ contract ZNSAccessController is AccessControlUpgradeable, ZNSRoles, IZNSAccessCo 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 722a8bf5d..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: decide what to do with this 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/ZNSEthRegistrar.sol b/contracts/distribution/ZNSEthRegistrar.sol index 2d58ed2e8..3080b0d5e 100644 --- a/contracts/distribution/ZNSEthRegistrar.sol +++ b/contracts/distribution/ZNSEthRegistrar.sol @@ -45,8 +45,6 @@ contract ZNSEthRegistrar is AccessControlled, IZNSEthRegistrar { address znsAddressResolver_ ) { _setAccessController(accessController_); - // TODO AC: should we call protected functions in the constructor/initialize? - // test this! setRegistry(znsRegistry_); setTreasury(znsTreasury_); setDomainToken(znsDomainToken_); diff --git a/contracts/distribution/ZNSTreasury.sol b/contracts/distribution/ZNSTreasury.sol index 94046eb49..7962e0c77 100644 --- a/contracts/distribution/ZNSTreasury.sol +++ b/contracts/distribution/ZNSTreasury.sol @@ -95,7 +95,6 @@ contract ZNSTreasury is AccessControlled, IZNSTreasury { emit ZeroVaultAddressSet(zeroVaultAddress); } - // TODO AC: should we call a protected function in the constructor/initialize? function setPriceOracle(address znsPriceOracle_) public override onlyAdmin { require( znsPriceOracle_ != address(0), diff --git a/test/ZNSEthRegistrar.test.ts b/test/ZNSEthRegistrar.test.ts index 7f1fa934f..825871c38 100644 --- a/test/ZNSEthRegistrar.test.ts +++ b/test/ZNSEthRegistrar.test.ts @@ -59,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; diff --git a/test/helpers/deployZNS.ts b/test/helpers/deployZNS.ts index efede23dc..89d47c089 100644 --- a/test/helpers/deployZNS.ts +++ b/test/helpers/deployZNS.ts @@ -195,11 +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 registry.connect(deployer).setOwnerOperator(registrar.address, true); - // 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"));