From 81df8e2287e29aff15a20405ac66a620faf05e8f Mon Sep 17 00:00:00 2001 From: Ermyas Abebe Date: Wed, 15 May 2024 06:02:08 +1000 Subject: [PATCH] Add custom error types --- .../deployer/AccessControlledDeployer.sol | 93 ++++++++++++------- test/deployer/AccessControlledDeployer.t.sol | 24 +++-- 2 files changed, 73 insertions(+), 44 deletions(-) diff --git a/contracts/deployer/AccessControlledDeployer.sol b/contracts/deployer/AccessControlledDeployer.sol index d9e193bb..68116b12 100644 --- a/contracts/deployer/AccessControlledDeployer.sol +++ b/contracts/deployer/AccessControlledDeployer.sol @@ -17,6 +17,15 @@ contract AccessControlledDeployer is AccessControlEnumerable, Pausable { /// @notice Role identifier for those who can deploy contracts bytes32 public constant DEPLOYER_ROLE = keccak256("DEPLOYER"); + /// @notice Emitted when the zero address is provided when it is not expected + error ZeroAddress(); + + /// @notice Emitted when a provided list of deployer addresses is empty + error EmptyDeployerList(); + + /// @notice Emitted if the caller, this contract, is not the owner of the targeted deployer + error NotOwnerOfDeployer(); + /** * @notice Construct a new RBACDeployer contract * @param admin The address to grant the DEFAULT_ADMIN_ROLE @@ -24,13 +33,13 @@ contract AccessControlledDeployer is AccessControlEnumerable, Pausable { * @param unpauser The address to grant the UNPAUSER_ROLE */ constructor(address admin, address pauser, address unpauser) { - require(admin != address(0), "admin is the zero address"); - require(pauser != address(0), "pauser is the zero address"); - require(unpauser != address(0), "unpauser is the zero address"); + if (admin == address(0) || pauser == address(0) || unpauser == address(0)) { + revert ZeroAddress(); + } - _setupRole(DEFAULT_ADMIN_ROLE, admin); - _setupRole(PAUSER_ROLE, pauser); - _setupRole(UNPAUSER_ROLE, unpauser); + _grantRole(DEFAULT_ADMIN_ROLE, admin); + _grantRole(PAUSER_ROLE, pauser); + _grantRole(UNPAUSER_ROLE, unpauser); } /** @@ -39,18 +48,20 @@ contract AccessControlledDeployer is AccessControlEnumerable, Pausable { * @param bytecode The bytecode of the contract to be deployed * @param salt A salt to influence the contract address * @dev Only address with DEPLOYER_ROLE can call this function + * @dev This function requires that the current owner of `deployer` is this contract * @dev The function can only be called if the contract is not in a paused state + * @dev The function emits `Deployed` event after the contract is deployed * @return The address of the deployed contract */ - function deploy(IDeployer deployer, bytes memory bytecode, bytes32 salt) - external - payable - whenNotPaused - onlyRole(DEPLOYER_ROLE) - returns (address) - { - require(address(deployer) != address(0), "deployer contract is the zero address"); - return deployer.deploy(bytecode, salt); + function deploy( + IDeployer deployer, + bytes memory bytecode, + bytes32 salt + ) external payable whenNotPaused onlyRole(DEPLOYER_ROLE) returns (address) { + if (address(deployer) == address(0)) { + revert ZeroAddress(); + } + return deployer.deploy{value: msg.value}(bytecode, salt); } /** @@ -60,17 +71,20 @@ contract AccessControlledDeployer is AccessControlEnumerable, Pausable { * @param salt A salt to influence the contract address * @param init Init data used to initialize the deployed contract * @dev Only address with DEPLOYER_ROLE can call this function + * @dev This function requires that the current owner of `deployer` is this contract * @dev The function can only be called if the contract is not in a paused state + * @dev The function emits `Deployed` event after the contract is deployed * @return The address of the deployed contract */ - function deployAndInit(IDeployer deployer, bytes memory bytecode, bytes32 salt, bytes calldata init) - external - payable - whenNotPaused - onlyRole(DEPLOYER_ROLE) - returns (address) - { - require(address(deployer) != address(0), "deployer contract is the zero address"); + function deployAndInit( + IDeployer deployer, + bytes memory bytecode, + bytes32 salt, + bytes calldata init + ) external payable whenNotPaused onlyRole(DEPLOYER_ROLE) returns (address) { + if (address(deployer) == address(0)) { + revert ZeroAddress(); + } return deployer.deployAndInit{value: msg.value}(bytecode, salt, init); } @@ -82,9 +96,13 @@ contract AccessControlledDeployer is AccessControlEnumerable, Pausable { * This is not emitted if an address is already a deployer */ function grantDeployerRole(address[] memory deployers) public { - require(deployers.length > 0, "deployers list is empty"); + if (deployers.length == 0) { + revert EmptyDeployerList(); + } for (uint256 i = 0; i < deployers.length; i++) { - require(deployers[i] != address(0), "deployer is the zero address"); + if (deployers[i] == address(0)) { + revert ZeroAddress(); + } grantRole(DEPLOYER_ROLE, deployers[i]); } } @@ -97,9 +115,13 @@ contract AccessControlledDeployer is AccessControlEnumerable, Pausable { * This is not emitted if an address was not a deployer */ function revokeDeployerRole(address[] memory deployers) public { - require(deployers.length > 0, "deployers list is empty"); + if (deployers.length == 0) { + revert EmptyDeployerList(); + } for (uint256 i = 0; i < deployers.length; i++) { - require(deployers[i] != address(0), "deployer is the zero address"); + if (deployers[i] == address(0)) { + revert ZeroAddress(); + } revokeRole(DEPLOYER_ROLE, deployers[i]); } } @@ -111,13 +133,16 @@ contract AccessControlledDeployer is AccessControlEnumerable, Pausable { * @dev Only address with DEFAULT_ADMIN_ROLE can call this function * @dev This function requires that the current owner of `ownableDeployer` is this contract */ - function transferOwnershipOfDeployer(Ownable ownableDeployer, address newOwner) - external - onlyRole(DEFAULT_ADMIN_ROLE) - { - require(address(ownableDeployer) != address(0), "deployer contract is the zero address"); - require(newOwner != address(0), "new owner is the zero address"); - require(ownableDeployer.owner() == address(this), "deployer contract is not owned by this contract"); + function transferOwnershipOfDeployer( + Ownable ownableDeployer, + address newOwner + ) external onlyRole(DEFAULT_ADMIN_ROLE) { + if (address(ownableDeployer) == address(0) || newOwner == address(0)) { + revert ZeroAddress(); + } + if (ownableDeployer.owner() != address(this)) { + revert NotOwnerOfDeployer(); + } ownableDeployer.transferOwnership(newOwner); } diff --git a/test/deployer/AccessControlledDeployer.t.sol b/test/deployer/AccessControlledDeployer.t.sol index 19fbb26c..a0db506d 100644 --- a/test/deployer/AccessControlledDeployer.t.sol +++ b/test/deployer/AccessControlledDeployer.t.sol @@ -26,6 +26,10 @@ contract AccessControlledDeployerTest is Test, Create2Utils, Create3Utils { event Deployed(address indexed deployedAddress, address indexed sender, bytes32 indexed salt, bytes32 bytecodeHash); + error ZeroAddress(); + error EmptyDeployerList(); + error NotOwnerOfDeployer(); + function setUp() public { rbacDeployer = new AccessControlledDeployer(admin, pauser, unpauser); @@ -38,17 +42,17 @@ contract AccessControlledDeployerTest is Test, Create2Utils, Create3Utils { * Constructor */ function test_Constructor_RevertIf_AdminIsZeroAddress() public { - vm.expectRevert("admin is the zero address"); + vm.expectRevert(ZeroAddress.selector); new AccessControlledDeployer(address(0), pauser, unpauser); } function test_Constructor_RevertIf_PauserIsZeroAddress() public { - vm.expectRevert("pauser is the zero address"); + vm.expectRevert(ZeroAddress.selector); new AccessControlledDeployer(admin, address(0), unpauser); } function test_Constructor_RevertIf_UnpauserIsZeroAddress() public { - vm.expectRevert("unpauser is the zero address"); + vm.expectRevert(ZeroAddress.selector); new AccessControlledDeployer(admin, pauser, address(0)); } @@ -91,20 +95,20 @@ contract AccessControlledDeployerTest is Test, Create2Utils, Create3Utils { function test_RevertIf_TransferDeployerOwnership_WithZeroOwnerAddress() public { OwnableCreate2Deployer create2Deployer = new OwnableCreate2Deployer(address(rbacDeployer)); vm.startPrank(admin); - vm.expectRevert("new owner is the zero address"); + vm.expectRevert(ZeroAddress.selector); rbacDeployer.transferOwnershipOfDeployer(create2Deployer, address(0)); } function test_RevertIf_TransferDeployerOwnership_WhenNotCurrentOwner() public { OwnableCreate2Deployer create2Deployer = new OwnableCreate2Deployer(makeAddr("currentOwner")); vm.startPrank(admin); - vm.expectRevert("deployer contract is not owned by this contract"); + vm.expectRevert(NotOwnerOfDeployer.selector); rbacDeployer.transferOwnershipOfDeployer(create2Deployer, makeAddr("newOwner2")); } function test_RevertIf_TransferDeployerOwnership_WithZeroDeployerAddress() public { vm.startPrank(admin); - vm.expectRevert("deployer contract is the zero address"); + vm.expectRevert(ZeroAddress.selector); rbacDeployer.transferOwnershipOfDeployer(Ownable(address(0)), makeAddr("newOwner2")); } @@ -169,7 +173,7 @@ contract AccessControlledDeployerTest is Test, Create2Utils, Create3Utils { */ function test_RevertIf_GrantDeployerRole_WithEmptyArray() public { address[] memory emptyDeployers = new address[](0); - vm.expectRevert("deployers list is empty"); + vm.expectRevert(EmptyDeployerList.selector); vm.prank(admin); rbacDeployer.grantDeployerRole(emptyDeployers); } @@ -180,7 +184,7 @@ contract AccessControlledDeployerTest is Test, Create2Utils, Create3Utils { // note that second deployer in the array is the zero address vm.prank(admin); - vm.expectRevert("deployer is the zero address"); + vm.expectRevert(ZeroAddress.selector); rbacDeployer.grantDeployerRole(newDeployers); } @@ -213,7 +217,7 @@ contract AccessControlledDeployerTest is Test, Create2Utils, Create3Utils { function test_RevertIf_RevokeDeployerRole_WithEmptyArray() public { address[] memory emptyDeployers = new address[](0); - vm.expectRevert("deployers list is empty"); + vm.expectRevert(EmptyDeployerList.selector); vm.prank(admin); rbacDeployer.revokeDeployerRole(emptyDeployers); } @@ -224,7 +228,7 @@ contract AccessControlledDeployerTest is Test, Create2Utils, Create3Utils { // note that second deployer in the array is the zero address vm.prank(admin); - vm.expectRevert("deployer is the zero address"); + vm.expectRevert(ZeroAddress.selector); rbacDeployer.grantDeployerRole(existingDeployers); }