refactor(protocol): bring two modifiers to EssentialContract (#17233)

taikoxyz · May 18, 2024 · 0b81d42 · 0b81d42
1 parent b7802b1
commit 0b81d42
Show file tree

Hide file tree

Showing 6 changed files with 32 additions and 34 deletions.
diff --git a/packages/protocol/contracts/bridge/Bridge.sol b/packages/protocol/contracts/bridge/Bridge.sol
@@ -82,7 +82,6 @@ contract Bridge is EssentialContract, IBridge {
     error B_INVALID_FEE();
     error B_INVALID_GAS_LIMIT();
     error B_INVALID_STATUS();
-    error B_INVALID_USER();
     error B_INVALID_VALUE();
     error B_INSUFFICIENT_GAS();
     error B_MESSAGE_NOT_SENT();
@@ -101,11 +100,6 @@ contract Bridge is EssentialContract, IBridge {
         _;
     }
 
-    modifier nonZeroAddr(address _addr) {
-        if (_addr == address(0)) revert B_INVALID_USER();
-        _;
-    }
-
     /// @notice Function to receive Ether.
     receive() external payable { }
 

diff --git a/packages/protocol/contracts/common/EssentialContract.sol b/packages/protocol/contracts/common/EssentialContract.sol
@@ -36,10 +36,11 @@ abstract contract EssentialContract is UUPSUpgradeable, Ownable2StepUpgradeable,
     /// @param account The account that unpaused the contract.
     event Unpaused(address account);
 
-    error REENTRANT_CALL();
     error INVALID_PAUSE_STATUS();
-    error ZERO_ADDR_MANAGER();
     error FUNC_NOT_IMPLEMENTED();
+    error REENTRANT_CALL();
+    error ZERO_ADDRESS();
+    error ZERO_VALUE();
 
     /// @dev Modifier that ensures the caller is the owner or resolved address of a given name.
     /// @param _name The name to check against.
@@ -70,6 +71,16 @@ abstract contract EssentialContract is UUPSUpgradeable, Ownable2StepUpgradeable,
         _;
     }
 
+    modifier nonZeroAddr(address _addr) {
+        if (_addr == address(0)) revert ZERO_ADDRESS();
+        _;
+    }
+
+    modifier nonZeroValue(bytes32 _value) {
+        if (_value == 0) revert ZERO_VALUE();
+        _;
+    }
+
     /// @custom:oz-upgrades-unsafe-allow constructor
     constructor() {
         _disableInitializers();
@@ -108,8 +119,13 @@ abstract contract EssentialContract is UUPSUpgradeable, Ownable2StepUpgradeable,
     /// @notice Initializes the contract.
     /// @param _owner The owner of this contract. msg.sender will be used if this value is zero.
     /// @param _addressManager The address of the {AddressManager} contract.
-    function __Essential_init(address _owner, address _addressManager) internal {
-        if (_addressManager == address(0)) revert ZERO_ADDR_MANAGER();
+    function __Essential_init(
+        address _owner,
+        address _addressManager
+    )
+        internal
+        nonZeroAddr(_addressManager)
+    {
         __Essential_init(_owner);
         __AddressResolver_init(_addressManager);
     }

diff --git a/packages/protocol/contracts/signal/SignalService.sol b/packages/protocol/contracts/signal/SignalService.sol
@@ -33,24 +33,12 @@ contract SignalService is EssentialContract, ISignalService {
 
     error SS_EMPTY_PROOF();
     error SS_INVALID_HOPS_WITH_LOOP();
-    error SS_INVALID_SENDER();
     error SS_INVALID_LAST_HOP_CHAINID();
     error SS_INVALID_MID_HOP_CHAINID();
     error SS_INVALID_STATE();
-    error SS_INVALID_VALUE();
     error SS_SIGNAL_NOT_FOUND();
     error SS_UNAUTHORIZED();
 
-    modifier nonZeroApp(address _app) {
-        if (_app == address(0)) revert SS_INVALID_SENDER();
-        _;
-    }
-
-    modifier nonZeroValue(bytes32 _input) {
-        if (_input == 0) revert SS_INVALID_VALUE();
-        _;
-    }
-
     /// @notice Initializes the contract.
     /// @param _owner The owner of this contract. msg.sender will be used if this value is zero.
     /// @param _addressManager The address of the {AddressManager} contract.
@@ -204,7 +192,7 @@ contract SignalService is EssentialContract, ISignalService {
         internal
         view
         virtual
-        nonZeroApp(_app)
+        nonZeroAddr(_app)
         nonZeroValue(_signal)
         nonZeroValue(_value)
         returns (bytes32)
@@ -245,7 +233,7 @@ contract SignalService is EssentialContract, ISignalService {
         bytes32 _value
     )
         private
-        nonZeroApp(_app)
+        nonZeroAddr(_app)
         nonZeroValue(_signal)
         nonZeroValue(_value)
         returns (bytes32 slot_)
@@ -287,7 +275,7 @@ contract SignalService is EssentialContract, ISignalService {
     )
         private
         view
-        nonZeroApp(_app)
+        nonZeroAddr(_app)
         nonZeroValue(_signal)
         returns (bytes32 value_)
     {
@@ -306,7 +294,7 @@ contract SignalService is EssentialContract, ISignalService {
     )
         private
         view
-        nonZeroApp(_app)
+        nonZeroAddr(_app)
         nonZeroValue(_signal)
         returns (CacheAction[] memory actions)
     {

diff --git a/packages/protocol/test/bridge/Bridge.t.sol b/packages/protocol/test/bridge/Bridge.t.sol
@@ -276,7 +276,7 @@ contract BridgeTest is TaikoTest {
             destChain: destChainId
         });
 
-        vm.expectRevert(Bridge.B_INVALID_USER.selector);
+        vm.expectRevert(EssentialContract.ZERO_ADDRESS.selector);
         bridge.sendMessage{ value: amount }(message);
     }
 

diff --git a/packages/protocol/test/bridge/Bridge2_sendMessage.t.sol b/packages/protocol/test/bridge/Bridge2_sendMessage.t.sol
@@ -12,11 +12,11 @@ contract BridgeTest2_sendMessage is BridgeTest2 {
         // init an all-zero message
         IBridge.Message memory message;
 
-        vm.expectRevert(Bridge.B_INVALID_USER.selector);
+        vm.expectRevert(EssentialContract.ZERO_ADDRESS.selector);
         bridge.sendMessage(message);
 
         message.srcOwner = Alice;
-        vm.expectRevert(Bridge.B_INVALID_USER.selector);
+        vm.expectRevert(EssentialContract.ZERO_ADDRESS.selector);
         bridge.sendMessage(message);
 
         message.destOwner = Bob;

diff --git a/packages/protocol/test/signal/SignalService.t.sol b/packages/protocol/test/signal/SignalService.t.sol
@@ -88,17 +88,17 @@ contract TestSignalService is TaikoTest {
     }
 
     function test_SignalService_sendSignal_revert() public {
-        vm.expectRevert(SignalService.SS_INVALID_VALUE.selector);
+        vm.expectRevert(EssentialContract.ZERO_VALUE.selector);
         signalService.sendSignal(0);
     }
 
     function test_SignalService_isSignalSent_revert() public {
         bytes32 signal = bytes32(uint256(1));
-        vm.expectRevert(SignalService.SS_INVALID_SENDER.selector);
+        vm.expectRevert(EssentialContract.ZERO_ADDRESS.selector);
         signalService.isSignalSent(address(0), signal);
 
         signal = bytes32(uint256(0));
-        vm.expectRevert(SignalService.SS_INVALID_VALUE.selector);
+        vm.expectRevert(EssentialContract.ZERO_VALUE.selector);
         signalService.isSignalSent(Alice, signal);
     }
 
@@ -114,7 +114,7 @@ contract TestSignalService is TaikoTest {
         SignalService.HopProof[] memory proofs = new SignalService.HopProof[](1);
 
         // app being address(0) will revert
-        vm.expectRevert(SignalService.SS_INVALID_SENDER.selector);
+        vm.expectRevert(EssentialContract.ZERO_ADDRESS.selector);
         signalService.proveSignalReceived({
             _chainId: 1,
             _app: address(0),
@@ -123,7 +123,7 @@ contract TestSignalService is TaikoTest {
         });
 
         // signal being 0 will revert
-        vm.expectRevert(SignalService.SS_INVALID_VALUE.selector);
+        vm.expectRevert(EssentialContract.ZERO_VALUE.selector);
         signalService.proveSignalReceived({
             _chainId: uint64(block.chainid),
             _app: randAddress(),