fix: add missing custom errors (#43)

* Fix missing custom errors * Update tests
sprintertech · Mar 25, 2024 · 34e2440 · 34e2440
1 parent 30cb6c4
commit 34e2440
Show file tree

Hide file tree

Showing 3 changed files with 43 additions and 24 deletions.
diff --git a/src/contracts/handlers/ERCHandlerHelpers.sol b/src/contracts/handlers/ERCHandlerHelpers.sol
@@ -28,9 +28,11 @@ abstract contract ERCHandlerHelpers is IERCHandler {
         Decimals decimals;
     }
 
+    error SenderNotBridgeContract();
+    error SenderNotExecutorContract();
+    error SenderNotRouterContract();
     error ContractAddressNotWhitelisted(address contractAddress);
     error DepositAmountTooSmall(uint256 depositAmount);
-    error SenderNotBridgeRouterOrExecutor();
 
     // resourceID => token contract address
     mapping(bytes32 => address) public _resourceIDToTokenContractAddress;
@@ -53,6 +55,19 @@ abstract contract ERCHandlerHelpers is IERCHandler {
         _;
     }
 
+    function _onlyBridge() private view {
+        if (msg.sender != _bridgeAddress) revert SenderNotBridgeContract();
+    }
+
+    function _onlyExecutor() private view {
+        if (msg.sender != _executorAddress) revert SenderNotExecutorContract();
+    }
+
+    function _onlyRouter() private view {
+        if (msg.sender != _routerAddress) revert SenderNotRouterContract();
+    }
+
+
     /**
         @param bridgeAddress Contract address of previously deployed Bridge.
         @param routerAddress Contract address of previously deployed Router.
@@ -64,18 +79,6 @@ abstract contract ERCHandlerHelpers is IERCHandler {
         _executorAddress = executorAddress;
     }
 
-    function _onlyBridge() private view {
-        require(msg.sender == _bridgeAddress, "sender must be bridge contract");
-    }
-
-    function _onlyRouter() private view {
-        require(msg.sender == _routerAddress, "sender must be router contract");
-    }
-
-    function _onlyExecutor() private view {
-        require(msg.sender == _executorAddress, "sender must be executor contract");
-    }
-
     /**
         @notice First verifies {contractAddress} is whitelisted, then sets
         {_tokenContractAddressToTokenProperties[contractAddress].isBurnable} to true.

diff --git a/src/contracts/libraries/MerkleTrie.sol b/src/contracts/libraries/MerkleTrie.sol
@@ -33,6 +33,7 @@ library MerkleTrie {
     error MerkleTrieValueLengthIsZero();
     error MerkleTrieNodeWithUnknownPrefix();
     error MerkleTrieUnparsableNode();
+    error MerkleTrieRanOutOfProofElements();
 
     /// @notice Determines the number of elements per branch node.
     uint256 internal constant TREE_RADIX = 16;
@@ -185,7 +186,7 @@ library MerkleTrie {
             }
         }
 
-        revert("MerkleTrie: ran out of proof elements");
+        revert MerkleTrieRanOutOfProofElements();
     }
 
     /// @notice Parses an array of proof elements into a new array that contains both the original

diff --git a/test/handlers/erc20/admin.test.ts b/test/handlers/erc20/admin.test.ts
@@ -66,34 +66,49 @@ describe("ERC20Handler - [constructor]", function () {
   it("[sanity] should revert if deposit is not called by Router", async () => {
     await expect(
       ERC20HandlerInstance.deposit(resourceID, depositorAccount, depositData),
-    ).to.be.revertedWith("sender must be router contract");
+    ).to.be.revertedWithCustomError(
+      ERC20HandlerInstance,
+      "SenderNotRouterContract()",
+    );
   });
 
-  it("[sanity] should revert if deposit is not called by Router", async () => {
+  it("[sanity] should revert if execution is not called by Executor", async () => {
     await expect(
       ERC20HandlerInstance.executeProposal(resourceID, depositData),
-    ).to.be.revertedWith("sender must be executor contract");
+    ).to.be.revertedWithCustomError(
+      ERC20HandlerInstance,
+      "SenderNotExecutorContract()",
+    );
   });
 
-  it("[sanity] should revert if deposit is not called by Router", async () => {
+  it("[sanity] should revert if setResource is not called by Bridge", async () => {
     await expect(
       ERC20HandlerInstance.setResource(
         resourceID,
         await ERC20MintableInstance.getAddress(),
         "0x",
       ),
-    ).to.be.revertedWith("sender must be bridge contract");
+    ).to.be.revertedWithCustomError(
+      ERC20HandlerInstance,
+      "SenderNotBridgeContract()",
+    );
   });
 
-  it("[sanity] should revert if deposit is not called by Router", async () => {
+  it("[sanity] should revert if setBurnable is not called by Bridge", async () => {
     await expect(
       ERC20HandlerInstance.setBurnable(await ERC20HandlerInstance.getAddress()),
-    ).to.be.revertedWith("sender must be bridge contract");
+    ).to.be.revertedWithCustomError(
+      ERC20HandlerInstance,
+      "SenderNotBridgeContract()",
+    );
   });
 
-  it("[sanity] should revert if deposit is not called by Router", async () => {
-    await expect(ERC20HandlerInstance.withdraw("0x")).to.be.revertedWith(
-      "sender must be bridge contract",
+  it("[sanity] should revert if withdraw is not called by Bridge", async () => {
+    await expect(
+      ERC20HandlerInstance.withdraw("0x"),
+    ).to.be.revertedWithCustomError(
+      ERC20HandlerInstance,
+      "SenderNotBridgeContract()",
     );
   });
 });