Refactors of the smart contracts - using error instead of reverts (#167)

* refactors of the smart contracts - using error instead of reverts and minor modifications

* linting

contracts/ChainIdManager.sol

@@ -1,9 +1,9 @@
 // SPDX-License-Identifier: GPL-3.0-only
 pragma solidity ^0.8.20;
 
-import {Owned} from "./mixin/Owned.sol";
+import {Ownable} from "@openzeppelin/contracts/access/Ownable.sol";
 
-contract ChainIdManager is Owned {
+contract ChainIdManager is Ownable {
     // Counter for the number of Chain IDs
     uint64 public chainIdCounter = 0;
     // contains a list of denied Chain IDs that should not be used as chainIds
@@ -12,7 +12,7 @@ contract ChainIdManager is Owned {
     // Fee to use up a Chain ID
     uint256 public immutable gasBurnAmount = 1000000;
 
-    constructor(uint64 _chainIdCounter) Owned() {
+    constructor(uint64 _chainIdCounter) Ownable() {
         chainIdCounter = _chainIdCounter;
     }
 

contracts/ForkableBridge.sol

@@ -124,7 +124,9 @@ contract ForkableBridge is
         bytes calldata metadata,
         address destinationAddress
     ) external onlyParent {
-        require(originNetwork != networkID, "wrong Token");
+        if (originNetwork == networkID) {
+            revert InvalidOriginNetwork();
+        }
         _issueBridgedTokens(
             originNetwork,
             token,

contracts/ForkingManager.sol

@@ -85,7 +85,9 @@ contract ForkingManager is IForkingManager, ForkableStructure {
     function initiateFork(
         DisputeData memory _disputeData
     ) external onlyBeforeForking {
-        require(executionTimeForProposal == 0, "ForkingManager: fork pending");
+        if (executionTimeForProposal != 0) {
+            revert ForkingAlreadyInitiated();
+        }
         // Charge the forking fee
         IERC20(forkonomicToken).safeTransferFrom(
             msg.sender,
@@ -106,12 +108,13 @@ contract ForkingManager is IForkingManager, ForkableStructure {
      * @dev function that executes a fork proposal
      */
     function executeFork() external onlyBeforeForking {
-        require(
-            executionTimeForProposal != 0 &&
-                // solhint-disable-next-line not-rely-on-time
-                executionTimeForProposal <= block.timestamp,
-            "ForkingManager: fork not ready"
-        );
+        if (
+            executionTimeForProposal == 0 ||
+            // solhint-disable-next-line not-rely-on-time
+            executionTimeForProposal > block.timestamp
+        ) {
+            revert NotYetReadyToFork();
+        }
 
         // Create the children of each contract
         NewInstances memory newInstances;

contracts/ForkonomicToken.sol

@@ -38,10 +38,9 @@ contract ForkonomicToken is
 
     /// @inheritdoc IForkonomicToken
     function mint(address to, uint256 amount) external {
-        require(
-            hasRole(MINTER_ROLE, msg.sender) || msg.sender == parentContract,
-            "Caller is not a minter"
-        );
+        if (!hasRole(MINTER_ROLE, msg.sender) && msg.sender != parentContract) {
+            revert NotMinterRole();
+        }
         _mint(to, amount);
     }
 
@@ -59,12 +58,10 @@ contract ForkonomicToken is
         bool firstChild,
         bool useChildTokenAllowance
     ) public onlyAfterForking {
-        require(children[0] != address(0), "Children not created yet");
         if (useChildTokenAllowance) {
-            require(
-                childTokenAllowances[msg.sender][firstChild] >= amount,
-                "Not enough allowance"
-            );
+            if (childTokenAllowances[msg.sender][firstChild] < amount) {
+                revert NotSufficientAllowance();
+            }
             childTokenAllowances[msg.sender][firstChild] -= amount;
         } else {
             _burn(msg.sender, amount);
@@ -79,8 +76,7 @@ contract ForkonomicToken is
     /// @dev Allows anyone to prepare the splitting of tokens
     /// by burning them
     /// @param amount The amount of tokens to burn
-    function prepareSplittingTokens(uint256 amount) public {
-        require(children[0] != address(0), "Children not created yet");
+    function prepareSplittingTokens(uint256 amount) public onlyAfterForking {
         _burn(msg.sender, amount);
         childTokenAllowances[msg.sender][false] += amount;
         childTokenAllowances[msg.sender][true] += amount;

contracts/interfaces/IForkableBridge.sol

@@ -13,6 +13,8 @@ interface IForkableBridge is IForkableStructure, IPolygonZkEVMBridge {
     error InvalidDestinationForHardAsset();
     /// @dev Error thrown when hardasset manager tries to send gas token to a child contract
     error GasTokenIsNotHardAsset();
+    /// @dev Error thrown when trying to send bridged tokens to a child contract
+    error InvalidOriginNetwork();
 
     /**
      * @dev Function to initialize the contract

contracts/interfaces/IForkingManager.sol

@@ -4,6 +4,11 @@ pragma solidity ^0.8.20;
 import {IForkableStructure} from "./IForkableStructure.sol";
 
 interface IForkingManager is IForkableStructure {
+    /// @dev Error thrown when the forking manager is not ready to fork
+    error NotYetReadyToFork();
+    /// @dev Error thrown when the forking manager is already initiated
+    error ForkingAlreadyInitiated();
+
     // Dispute contract and call to identify the dispute
     // that will be used to initiate/justify the fork
     struct DisputeData {

contracts/interfaces/IForkonomicToken.sol

@@ -5,6 +5,11 @@ import {IForkableStructure} from "./IForkableStructure.sol";
 import {IERC20Upgradeable} from "@openzeppelin/contracts-upgradeable/token/ERC20/IERC20Upgradeable.sol";
 
 interface IForkonomicToken is IForkableStructure, IERC20Upgradeable {
+    /// @dev Error thrown when activity is started by a non-authorized minter
+    error NotMinterRole();
+    /// @dev Error thrown when not sufficient balance has been burn to mint new tokens at the child contracts
+    error NotSufficientAllowance();
+
     /**
      * @notice Allows the forkmanager to initialize the contract
      * @param _forkmanager The address of the forkmanager

contracts/lib/BridgeAssetOperations.sol

@@ -8,6 +8,11 @@ import {IERC20} from "@openzeppelin/contracts/token/ERC20/IERC20.sol";
 import {IERC20Metadata} from "@openzeppelin/contracts/token/ERC20/extensions/IERC20Metadata.sol";
 
 library BridgeAssetOperations {
+    // @dev Error thrown when non-forkable token is intended to be used, but it is not forkable
+    error TokenNotForkable();
+    // @dev Error thrown when token is not issued before
+    error TokenNotIssuedBefore();
+
     /**
      * @notice Function to merge tokens from children-bridge contracts
      * @param token Address of the token
@@ -23,11 +28,12 @@ library BridgeAssetOperations {
         address child0,
         address child1
     ) public {
-        require(tokenInfo.originNetwork != 0, "Token not forkable");
-        require(
-            tokenInfo.originTokenAddress != address(0),
-            "Token not issued before"
-        );
+        if (tokenInfo.originNetwork == 0) {
+            revert TokenNotForkable();
+        }
+        if (tokenInfo.originTokenAddress == address(0)) {
+            revert TokenNotIssuedBefore();
+        }
         ForkableBridge(child0).burnForkableTokens(
             msg.sender,
             tokenInfo.originTokenAddress,
@@ -57,7 +63,9 @@ library BridgeAssetOperations {
         PolygonZkEVMBridge.TokenInformation memory tokenInfo,
         address child
     ) public {
-        require(tokenInfo.originNetwork != 0, "Token not forkable");
+        if (tokenInfo.originNetwork == 0) {
+            revert TokenNotForkable();
+        }
         bytes memory metadata = abi.encode(
             IERC20Metadata(token).name(),
             IERC20Metadata(token).symbol(),

contracts/lib/reality-eth/Arbitrator.sol

@@ -5,7 +5,7 @@ pragma solidity ^0.8.20;
 import "./../../interfaces/IArbitrator.sol";
 import "./../../interfaces/IRealityETH.sol";
 import "./../../interfaces/IERC20.sol";
-import "./../../mixin/Owned.sol";
+import "./Owned.sol";
 
 contract Arbitrator is Owned, IArbitrator {
     IRealityETH public realitio;

contracts/mixin/InitializeChain.sol

@@ -1,6 +1,9 @@
 // SPDX-License-Identifier: GPL-3.0-only
 pragma solidity ^0.8.20;
 
+// Currently, this contract is not used. Ed came up with its own isChainUpToDate() modifier.
+//  Maybe we have to delete it.
+
 /** This contract can be inherited form any other contrac that needs to be aware of Forks.
 This contract uses the fact that after a fork the chainId changes and thereby detects forks*/
 

test/ChainIdManager.t.sol

@@ -29,7 +29,7 @@ contract ChainIdManagerTest is Test {
 
         // Attempt to add a ChainId by a non-owner, expect a revert
         vm.prank(nonOwner);
-        vm.expectRevert(bytes("Caller is not the owner")); // Expect a revert with a specific revert message
+        vm.expectRevert(bytes("Ownable: caller is not the owner")); // Expect a revert with a specific revert message
         chainIdManager.denyListChainId(2);
     }
 

test/ForkableBridge.t.sol

@@ -108,7 +108,7 @@ contract ForkableBridgeTest is Test {
         );
 
         vm.prank(forkableBridge.parentContract());
-        vm.expectRevert(bytes("wrong Token"));
+        vm.expectRevert(IForkableBridge.InvalidOriginNetwork.selector);
         forkableBridge.mintForkableToken(
             address(token),
             networkID, // <-- this line is changed

test/ForkingManager.t.sol

@@ -608,7 +608,7 @@ contract ForkingManagerTest is Test {
 
     function testExecuteForkRespectsTime() public {
         // reverts on empty proposal list
-        vm.expectRevert("ForkingManager: fork not ready");
+        vm.expectRevert(IForkingManager.NotYetReadyToFork.selector);
         forkmanager.executeFork();
 
         // Mint and approve the arbitration fee for the test contract
@@ -621,7 +621,7 @@ contract ForkingManagerTest is Test {
         vm.warp(testTimestamp);
         forkmanager.initiateFork(disputeData);
 
-        vm.expectRevert("ForkingManager: fork not ready");
+        vm.expectRevert(IForkingManager.NotYetReadyToFork.selector);
         forkmanager.executeFork();
         vm.warp(testTimestamp + forkmanager.forkPreparationTime() + 1);
         forkmanager.executeFork();
@@ -653,7 +653,7 @@ contract ForkingManagerTest is Test {
         disputeData.disputeContent = "0x1";
         forkmanager.initiateFork(disputeData);
         disputeData.disputeContent = "0x2";
-        vm.expectRevert(bytes("ForkingManager: fork pending"));
+        vm.expectRevert(IForkingManager.ForkingAlreadyInitiated.selector);
         forkmanager.initiateFork(disputeData);
     }
 

test/ForkonomicToken.t.sol

@@ -57,7 +57,7 @@ contract ForkonomicTokenTest is Test {
 
         assertEq(forkonomicToken.balanceOf(address(this)), mintAmount);
 
-        vm.expectRevert(bytes("Caller is not a minter"));
+        vm.expectRevert(IForkonomicToken.NotMinterRole.selector);
         forkonomicToken.mint(address(this), mintAmount);
     }