Modifiers call internal functions (#163)

modifiers onlyRelayers, onlyAdmin, onlyBridge, and onlyAdminOrRelayer now call equivalent internal functions
ChainSafe · May 29, 2020 · 5382f0c · 5382f0c
1 parent 0c59ad3
commit 5382f0c
Show file tree

Hide file tree

Showing 5 changed files with 34 additions and 18 deletions.
diff --git a/contracts/Bridge.sol b/contracts/Bridge.sol
@@ -91,21 +91,33 @@ contract Bridge is Pausable, AccessControl {
     bytes32 public constant RELAYER_ROLE = keccak256("RELAYER_ROLE");
 
     modifier onlyAdmin() {
-        require(hasRole(DEFAULT_ADMIN_ROLE, msg.sender), "sender does not have admin role");
+        _onlyAdmin();
         _;
     }
 
     modifier onlyAdminOrRelayer() {
-        require(hasRole(DEFAULT_ADMIN_ROLE, msg.sender) || hasRole(RELAYER_ROLE, msg.sender),
-                "sender does not have admin role");
+        _onlyAdminOrRelayer();
         _;
     }
 
     modifier onlyRelayers() {
-        require(hasRole(RELAYER_ROLE, msg.sender), "sender does not have relayer role");
+        _onlyRelayers();
         _;
     }
 
+    function _onlyAdminOrRelayer() private {
+        require(hasRole(DEFAULT_ADMIN_ROLE, msg.sender) || hasRole(RELAYER_ROLE, msg.sender),
+                "sender does not have admin role");
+    }
+
+    function _onlyAdmin() private {
+        require(hasRole(DEFAULT_ADMIN_ROLE, msg.sender), "sender does not have admin role");
+    }
+
+    function _onlyRelayers() private {
+        require(hasRole(RELAYER_ROLE, msg.sender), "sender does not have relayer role");
+    }
+
     /**
         @notice Initializes Bridge, creates and grants {msg.sender} the admin role,
         creates and grants {initialRelayers} the relayer role.

diff --git a/contracts/handlers/ERC20Handler.sol b/contracts/handlers/ERC20Handler.sol
@@ -94,7 +94,7 @@ contract ERC20Handler is IDepositExecute, HandlerHelpers, ERC20Safe {
         uint64 depositNonce,
         address depositer,
         bytes calldata data
-    ) external override _onlyBridge {
+    ) external override onlyBridge {
         bytes32        resourceID;
         bytes   memory recipientAddress;
         uint256        amount;
@@ -147,7 +147,7 @@ contract ERC20Handler is IDepositExecute, HandlerHelpers, ERC20Safe {
         destinationRecipientAddress length     uint256     bytes  64 - 96
         destinationRecipientAddress            bytes       bytes  96 - END
      */
-    function executeProposal(bytes calldata data) external override _onlyBridge {
+    function executeProposal(bytes calldata data) external override onlyBridge {
         uint256       amount;
         bytes32       resourceID;
         bytes  memory destinationRecipientAddress;
@@ -190,7 +190,7 @@ contract ERC20Handler is IDepositExecute, HandlerHelpers, ERC20Safe {
         @param recipient Address to release tokens to.
         @param amount The amount of ERC20 tokens to release.
      */
-    function withdraw(address tokenAddress, address recipient, uint amount) external override _onlyBridge {
+    function withdraw(address tokenAddress, address recipient, uint amount) external override onlyBridge {
         releaseERC20(tokenAddress, recipient, amount);
     }
 }
diff --git a/contracts/handlers/ERC721Handler.sol b/contracts/handlers/ERC721Handler.sol
@@ -100,7 +100,7 @@ contract ERC721Handler is IDepositExecute, HandlerHelpers, ERC721Safe {
         @dev Depending if the corresponding {tokenAddress} for the parsed {resourceID} is
         marked true in {_burnList}, deposited tokens will be burned, if not, they will be locked.
      */
-    function deposit(uint8 destinationChainID, uint64 depositNonce, address depositer, bytes calldata data) external override _onlyBridge {
+    function deposit(uint8 destinationChainID, uint64 depositNonce, address depositer, bytes calldata data) external override onlyBridge {
         bytes32      resourceID;
         uint         lenDestinationRecipientAddress;
         uint         tokenID;
@@ -171,7 +171,7 @@ contract ERC721Handler is IDepositExecute, HandlerHelpers, ERC721Safe {
         metadata                        length      uint256    bytes    (96 + len(destinationRecipientAddress)) - (96 + len(destinationRecipientAddress) + 32)
         metadata                                      bytes    bytes    (96 + len(destinationRecipientAddress) + 32) - END
      */
-    function executeProposal(bytes calldata data) external override _onlyBridge {
+    function executeProposal(bytes calldata data) external override onlyBridge {
         uint256         tokenID;
         bytes32         resourceID;
         bytes  memory   destinationRecipientAddress;
@@ -239,7 +239,7 @@ contract ERC721Handler is IDepositExecute, HandlerHelpers, ERC721Safe {
         @param recipient Address to release token to.
         @param tokenID The ERC721 token ID to release.
      */
-    function withdraw(address tokenAddress, address recipient, uint tokenID) external override _onlyBridge {
+    function withdraw(address tokenAddress, address recipient, uint tokenID) external override onlyBridge {
         releaseERC721(tokenAddress, address(this), recipient, tokenID);
     }
 }
diff --git a/contracts/handlers/GenericHandler.sol b/contracts/handlers/GenericHandler.sol
@@ -36,7 +36,7 @@ contract GenericHandler is IGenericHandler {
     // token contract address => is whitelisted
     mapping (address => bool) public _contractWhitelist;
 
-    modifier _onlyBridge() {
+    modifier onlyBridge() {
         require(msg.sender == _bridgeAddress, "sender must be bridge contract");
         _;
     }
@@ -116,7 +116,7 @@ contract GenericHandler is IGenericHandler {
         address contractAddress,
         bytes4 depositFunctionSig,
         bytes4 executeFunctionSig
-    ) external override {
+    ) external onlyBridge override {
         require(_resourceIDToContractAddress[resourceID] == address(0), "resourceID already has a corresponding contract address");
 
         bytes32 currentResourceID = _contractAddressToResourceID[contractAddress];
@@ -142,7 +142,7 @@ contract GenericHandler is IGenericHandler {
         @notice If {_contractAddressToDepositFunctionSignature}[{contractAddress}] is set,
         {metaData} is expected to consist of needed function arguments.
      */
-    function deposit(uint8 destinationChainID, uint64 depositNonce, address depositer, bytes calldata data) external _onlyBridge {
+    function deposit(uint8 destinationChainID, uint64 depositNonce, address depositer, bytes calldata data) external onlyBridge {
         bytes32      resourceID;
         bytes32      lenMetadata;
         bytes memory metadata;
@@ -196,7 +196,7 @@ contract GenericHandler is IGenericHandler {
         @notice If {_contractAddressToExecuteFunctionSignature}[{contractAddress}] is set,
         {metaData} is expected to consist of needed function arguments.
      */
-    function executeProposal(bytes calldata data) external _onlyBridge {
+    function executeProposal(bytes calldata data) external onlyBridge {
         bytes32      resourceID;
         bytes memory metaData;
         assembly {

diff --git a/contracts/handlers/HandlerHelpers.sol b/contracts/handlers/HandlerHelpers.sol
@@ -22,11 +22,15 @@ contract HandlerHelpers is IERCHandler {
     // token contract address => is burnable
     mapping (address => bool) public _burnList;
 
-    modifier _onlyBridge() {
-        require(msg.sender == _bridgeAddress, "sender must be bridge contract");
+    modifier onlyBridge() {
+        _onlyBridge();
         _;
     }
 
+    function _onlyBridge() private {
+        require(msg.sender == _bridgeAddress, "sender must be bridge contract");
+    }
+
     /**
         @notice First verifies {_resourceIDToContractAddress}[{resourceID}] and
         {_contractAddressToResourceID}[{contractAddress}] are not already set,
@@ -36,7 +40,7 @@ contract HandlerHelpers is IERCHandler {
         @param resourceID ResourceID to be used when making deposits.
         @param contractAddress Address of contract to be called when a deposit is made and a deposited is executed.
      */
-    function setResource(bytes32 resourceID, address contractAddress) external override _onlyBridge {
+    function setResource(bytes32 resourceID, address contractAddress) external override onlyBridge {
         require(_resourceIDToTokenContractAddress[resourceID] == address(0), "resourceID already has a corresponding contract address");
 
         bytes32 currentResourceID = _tokenContractAddressToResourceID[contractAddress];
@@ -50,7 +54,7 @@ contract HandlerHelpers is IERCHandler {
         to true.
         @param contractAddress Address of contract to be used when making or executing deposits.
      */
-    function setBurnable(address contractAddress) external override _onlyBridge{
+    function setBurnable(address contractAddress) external override onlyBridge{
         _setBurnable(contractAddress);
     }