fix: Pre-release polish (#33)

contracts/access/ZNSAccessController.sol

@@ -25,7 +25,6 @@ contract ZNSAccessController is AccessControlUpgradeable, ZNSRoles, IZNSAccessCo
     }
 
     // ** Access Validators **
-
     function checkGovernor(address account) external view override {
         _checkRole(GOVERNOR_ROLE, account);
     }

contracts/access/ZNSRoles.sol

@@ -2,15 +2,13 @@
 pragma solidity ^0.8.18;
 
 
-// TODO: can we declare these outside of contract, just in the ZNSAccessController file?
 abstract contract ZNSRoles {
     // 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
     bytes32 public constant ADMIN_ROLE = keccak256("ADMIN_ROLE");
-    // operator can be here to future proof, if we need a new role
+    // executor can be here to future proof, if we need a new role
     // so we don't have to upgrade all contracts
-    // TODO AC: possibly remove executor role?
     bytes32 public constant EXECUTOR_ROLE = keccak256("EXECUTOR_ROLE");
     // this role is here specifically for the ZNSRegistrar contract
     bytes32 public constant REGISTRAR_ROLE = keccak256("REGISTRAR_ROLE");

contracts/distribution/IZNSRegistrar.sol

@@ -8,7 +8,8 @@ interface IZNSRegistrar {
         uint256 indexed tokenId,
         string name,
         address indexed registrant,
-        address resolver
+        address resolver,
+        address domainAddress
     );
 
     event DomainRevoked(bytes32 indexed domainHash, address indexed registrant);

contracts/distribution/IZNSTreasury.sol

@@ -8,25 +8,26 @@ interface IZNSTreasury {
      * @param domainHash The hash of the domain name
      * @param domainName The domain name
      * @param depositor The address of the depositing user
-     * @param amount The amount they are depositing
+     * @param stakeAmount The amount they are depositing
      */
     event StakeDeposited(
         bytes32 indexed domainHash,
         string domainName,
         address indexed depositor,
-        uint256 indexed amount
+        uint256 indexed stakeAmount,
+        uint256 registrationFee
     );
 
     /**
      * @notice Emitted when a stake is withdrawn
      * @param domainHash The hash of the domain name
      * @param owner The owner of the domain
-     * @param amount The amount withdrawn
+     * @param stakeAmount The staked amount withdrawn
      */
     event StakeWithdrawn(
         bytes32 indexed domainHash,
         address indexed owner,
-        uint256 indexed amount
+        uint256 indexed stakeAmount
     );
 
     event PriceOracleSet(address priceOracle);

contracts/distribution/ZNSPriceOracle.sol

@@ -1,8 +1,6 @@
 // SPDX-License-Identifier: MIT
 pragma solidity ^0.8.18;
 
-import { IERC20 } from "@openzeppelin/contracts/token/ERC20/IERC20.sol";
-import { Initializable } from "@openzeppelin/contracts-upgradeable/proxy/utils/Initializable.sol";
 import { UUPSUpgradeable } from "@openzeppelin/contracts-upgradeable/proxy/utils/UUPSUpgradeable.sol";
 import { IZNSPriceOracle } from "./IZNSPriceOracle.sol";
 import { StringUtils } from "../utils/StringUtils.sol";
@@ -123,7 +121,7 @@ contract ZNSPriceOracle is AccessControlled, UUPSUpgradeable, IZNSPriceOracle {
         if (length >= rootDomainPriceConfig.priceMultiplier) {
             rootDomainPriceConfig.priceMultiplier = length + 1;
         }
-        
+
         rootDomainPriceConfig.baseLength = length;
 
         emit BaseLengthSet(length);
@@ -177,11 +175,11 @@ contract ZNSPriceOracle is AccessControlled, UUPSUpgradeable, IZNSPriceOracle {
         emit FeePercentageSet(regFeePercentage);
     }
 
-    function setAccessController(address accessController)
+    function setAccessController(address accessController_)
     external
     override(AccessControlled, IZNSPriceOracle)
     onlyAdmin {
-        _setAccessController(accessController);
+        _setAccessController(accessController_);
     }
 
     function getAccessController() external view override(AccessControlled, IZNSPriceOracle) returns (address) {
@@ -199,7 +197,7 @@ contract ZNSPriceOracle is AccessControlled, UUPSUpgradeable, IZNSPriceOracle {
     ) internal view returns (uint256) {
         DomainPriceConfig memory config = rootDomainPriceConfig;
 
-        // Setting baseLength to 0 indicates to the system that we are 
+        // Setting baseLength to 0 indicates to the system that we are
         // currently in a special phase where we define an exact price for all domains
         // e.g. promotions or sales
         if (config.baseLength == 0) return config.maxPrice;
@@ -211,22 +209,19 @@ contract ZNSPriceOracle is AccessControlled, UUPSUpgradeable, IZNSPriceOracle {
         // Then multiply by the same precision multiplier to get the actual value
         // with truncated values past precision. So having a value of 15.235234324234512365 * 10^18
         // with precision 2 would give us 15.230000000000000000 * 10^18
-        return 
-        (config.baseLength * config.maxPrice / length 
+        return
+        (config.baseLength * config.maxPrice / length
         + (config.maxPrice / config.priceMultiplier))
         / config.precisionMultiplier * config.precisionMultiplier;
     }
 
     function _setPriceMultiplier(uint256 multiplier) internal {
-        // Avoid unnecessary jumps to external storage
-        DomainPriceConfig memory config = rootDomainPriceConfig;
-
         // The multiplier being 0 will cause a division error in the pricing function
         require(multiplier > 0, "ZNSPriceOracle: Multiplier cannot be 0");
 
         // The multiplier being larger than the base length will cause spikes in the pricing function
         require(
-            multiplier >= config.baseLength + 1,
+            multiplier >= rootDomainPriceConfig.baseLength + 1,
             "ZNSPriceOracle: Multiplier must be >= baseLength + 1"
         );
         rootDomainPriceConfig.priceMultiplier = multiplier;

contracts/distribution/ZNSRegistrar.sol

@@ -54,11 +54,11 @@ contract ZNSRegistrar is AccessControlled, UUPSUpgradeable, IZNSRegistrar {
      * @notice Register a new domain such as `0://wilder`
      *
      * @param name Name of the domain to register
-     * @param resolverContent Address for the resolver to return when requested (optional, send 0x0 if not needed)
+     * @param domainAddress Address for the resolver to return when requested (optional, send 0x0 if not needed)
      */
     function registerDomain(
         string calldata name,
-        address resolverContent
+        address domainAddress
     ) external override returns (bytes32) {
         require(
             bytes(name).length != 0,
@@ -80,22 +80,20 @@ contract ZNSRegistrar is AccessControlled, UUPSUpgradeable, IZNSRegistrar {
         uint256 tokenId = uint256(domainHash);
         domainToken.register(msg.sender, tokenId);
 
-        _setDomainData(domainHash, msg.sender, resolverContent);
+        _setDomainData(domainHash, msg.sender, domainAddress);
 
         emit DomainRegistered(
             domainHash,
             tokenId,
             name,
             msg.sender,
-            address(addressResolver)
+            address(addressResolver),
+            domainAddress
         );
 
         return domainHash;
     }
 
-    // TODO: figure out how to guard this so people can stake tokens
-    //  without the risk of staking contract or wallet to call reclaim+revoke
-    //  from underneath them
     function revokeDomain(bytes32 domainHash)
     external
     override
@@ -104,8 +102,8 @@ contract ZNSRegistrar is AccessControlled, UUPSUpgradeable, IZNSRegistrar {
     {
         uint256 tokenId = uint256(domainHash);
         domainToken.revoke(tokenId);
-        treasury.unstakeForDomain(domainHash, msg.sender);
         registry.deleteRecord(domainHash);
+        treasury.unstakeForDomain(domainHash, msg.sender);
 
         emit DomainRevoked(domainHash, msg.sender);
     }
@@ -177,17 +175,17 @@ contract ZNSRegistrar is AccessControlled, UUPSUpgradeable, IZNSRegistrar {
      *
      * @param domainHash The domain name hash
      * @param owner The owner of that domain
-     * @param resolverContent The content it resolves to
+     * @param domainAddress The content it resolves to
      */
     function _setDomainData(
         bytes32 domainHash,
         address owner,
-        address resolverContent
+        address domainAddress
     ) internal {
         // Set only the domain owner if no resolver content is given
-        if (resolverContent != address(0)) {
+        if (domainAddress != address(0)) {
             registry.createDomainRecord(domainHash, owner, address(addressResolver));
-            addressResolver.setAddress(domainHash, resolverContent);
+            addressResolver.setAddress(domainHash, domainAddress);
         } else {
             registry.createDomainRecord(domainHash, owner, address(0));
         }

contracts/distribution/ZNSTreasury.sol

@@ -6,9 +6,12 @@ import { IZNSPriceOracle } from "./IZNSPriceOracle.sol";
 import { AccessControlled } from "../access/AccessControlled.sol";
 import { IERC20 } from "@openzeppelin/contracts/token/ERC20/IERC20.sol";
 import { UUPSUpgradeable } from "@openzeppelin/contracts-upgradeable/proxy/utils/UUPSUpgradeable.sol";
+import { SafeERC20 } from "@openzeppelin/contracts/token/ERC20/utils/SafeERC20.sol";
 
 
 contract ZNSTreasury is AccessControlled, UUPSUpgradeable, IZNSTreasury {
+    using SafeERC20 for IERC20;
+
     /**
      * @notice The price oracle
      */
@@ -17,12 +20,6 @@ contract ZNSTreasury is AccessControlled, UUPSUpgradeable, IZNSTreasury {
     /**
      * @notice The payment/staking token
      */
-    // TODO: this should be changed to be more general
-    //  we might not use ZERO, but any other token here
-    //  so change the naming and change the interface for IERC20,
-    //  instead of a specific ZERO token interface!
-    //  Make sure it is general on all contracts where it's present!
-    // TODO: change all transfer calls to safeTransfer!
     IERC20 public stakingToken;
 
     /**
@@ -55,21 +52,28 @@ contract ZNSTreasury is AccessControlled, UUPSUpgradeable, IZNSTreasury {
         address depositor
     ) external override onlyRegistrar {
         // Get price and fee for the domain
-        ( ,
+        (
+            uint256 totalPrice,
             uint256 stakeAmount,
             uint256 registrationFee
-        ) = priceOracle.getPrice(domainName);
+        ) = priceOracle.getPrice(
+            domainName
+        );
 
         // Transfer stake amount and fee
-        stakingToken.transferFrom(depositor, address(this), stakeAmount);
-        // TODO make sure we show the approval process to the user here to avoid failed transfer
-        // TODO can we make it so it needs a single approval only?!
-        stakingToken.transferFrom(depositor, zeroVault, registrationFee);
+        stakingToken.safeTransferFrom(depositor, address(this), totalPrice);
+        stakingToken.safeTransfer(zeroVault, registrationFee);
 
         // Record staked amount for this domain
         stakedForDomain[domainHash] = stakeAmount;
 
-        emit StakeDeposited(domainHash, domainName, depositor, stakeAmount);
+        emit StakeDeposited(
+            domainHash,
+            domainName,
+            depositor,
+            stakeAmount,
+            registrationFee
+        );
     }
 
     function unstakeForDomain(
@@ -80,16 +84,16 @@ contract ZNSTreasury is AccessControlled, UUPSUpgradeable, IZNSTreasury {
         require(stakeAmount > 0, "ZNSTreasury: No stake for domain");
         delete stakedForDomain[domainHash];
 
-        stakingToken.transfer(owner, stakeAmount);
+        stakingToken.safeTransfer(owner, stakeAmount);
 
         emit StakeWithdrawn(domainHash, owner, stakeAmount);
     }
 
-    function setZeroVaultAddress(address zeroVaultAddress) public override onlyAdmin {
-        require(zeroVaultAddress != address(0), "ZNSTreasury: zeroVault passed as 0x0 address");
+    function setZeroVaultAddress(address zeroVault_) public override onlyAdmin {
+        require(zeroVault_ != address(0), "ZNSTreasury: zeroVault passed as 0x0 address");
 
-        zeroVault = zeroVaultAddress;
-        emit ZeroVaultAddressSet(zeroVaultAddress);
+        zeroVault = zeroVault_;
+        emit ZeroVaultAddressSet(zeroVault_);
     }
 
     function setPriceOracle(address priceOracle_) public override onlyAdmin {

contracts/registry/ZNSRegistry.sol

@@ -26,19 +26,13 @@ contract ZNSRegistry is AccessControlled, UUPSUpgradeable, IZNSRegistry {
      * @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"
         );
         _;
     }
 
-    modifier onlyRegistrar {
-        accessController.checkRegistrar(msg.sender);
-        _;
-    }
-
     modifier onlyOwner(bytes32 domainHash) {
         require(
             records[domainHash].owner == msg.sender,
@@ -47,12 +41,17 @@ contract ZNSRegistry is AccessControlled, UUPSUpgradeable, IZNSRegistry {
         _;
     }
 
+    modifier onlyRegistrar {
+        accessController.checkRegistrar(msg.sender);
+        _;
+    }
+
     /**
      * @notice Initialize the ZNSRegistry contract
-     * @param accessController The address of the AccessController contract
+     * @param accessController_ The address of the AccessController contract
      */
-    function initialize(address accessController) public override initializer {
-        _setAccessController(accessController);
+    function initialize(address accessController_) public override initializer {
+        _setAccessController(accessController_);
     }
 
     /**
@@ -138,7 +137,7 @@ contract ZNSRegistry is AccessControlled, UUPSUpgradeable, IZNSRegistry {
     }
 
     /**
-     * @notice Update an existing domain record's owner or resolver
+     * @notice Update an existing domain record's owner and 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

contracts/resolver/ZNSAddressResolver.sol

@@ -20,16 +20,16 @@ contract ZNSAddressResolver is AccessControlled, UUPSUpgradeable, ERC165, IZNSAd
      *         to Ethereum wallets or contracts registered in ZNS
      */
     mapping(bytes32 domainHash => address resolvedAddress)
-        private addressOf;
+        private domainAddresses;
 
     /**
      * @notice Initialize an instance of the ZNSAddressResolver
-     * @param _accessController The access controller
-     * @param _registry The registry address
+     * @param accessController_ The access controller
+     * @param registry_ The registry address
      */
-    function initialize(address _accessController, address _registry) public override initializer {
-        _setAccessController(_accessController);
-        setRegistry(_registry);
+    function initialize(address accessController_, address registry_) public override initializer {
+        _setAccessController(accessController_);
+        setRegistry(registry_);
     }
 
     /**
@@ -39,7 +39,7 @@ contract ZNSAddressResolver is AccessControlled, UUPSUpgradeable, ERC165, IZNSAd
     function getAddress(
         bytes32 domainHash
     ) external view override returns (address) {
-        return addressOf[domainHash];
+        return domainAddresses[domainHash];
     }
 
     /**
@@ -59,7 +59,7 @@ contract ZNSAddressResolver is AccessControlled, UUPSUpgradeable, ERC165, IZNSAd
             "ZNSAddressResolver: Not authorized for this domain"
         );
 
-        addressOf[domainHash] = newAddress;
+        domainAddresses[domainHash] = newAddress;
 
         emit AddressSet(domainHash, newAddress);
     }
@@ -108,7 +108,7 @@ contract ZNSAddressResolver is AccessControlled, UUPSUpgradeable, ERC165, IZNSAd
      * @notice To use UUPS proxy we override this function and revert if `msg.sender` isn't authorized
      * @param newImplementation The implementation contract to upgrade to
      */
-    // solhint-disable-next-line
+    // solhint-disable-next-line unused-vars
     function _authorizeUpgrade(address newImplementation) internal view override {
         accessController.checkGovernor(msg.sender);
     }