feat: replace requires with custom errors

contracts/CHANGELOG.md

@@ -8,6 +8,7 @@ The format is based on [Common Changelog](https://common-changelog.org/).
 
 ### Changed
 
+- **Breaking:** Replace `require()` with `revert()` and custom errors outside KlerosCore for consistency and smaller bytecode ([#2084](https://github.com/kleros/kleros-v2/issues/2084))
 - Set the Hardhat Solidity version to v0.8.30 and enable the IR pipeline ([#2069](https://github.com/kleros/kleros-v2/issues/2069))
 - Set the Foundry Solidity version to v0.8.30 and enable the IR pipeline ([#2073](https://github.com/kleros/kleros-v2/issues/2073))
 - Widen the allowed solc version to any v0.8.x for the interfaces only ([#2083](https://github.com/kleros/kleros-v2/issues/2083))

contracts/src/arbitration/DisputeTemplateRegistry.sol

@@ -25,7 +25,7 @@ contract DisputeTemplateRegistry is IDisputeTemplateRegistry, UUPSProxiable, Ini
     // ************************************* //
 
     modifier onlyByGovernor() {
-        require(governor == msg.sender, "Governor only");
+        if (governor != msg.sender) revert GovernorOnly();
         _;
     }
 
@@ -80,4 +80,10 @@ contract DisputeTemplateRegistry is IDisputeTemplateRegistry, UUPSProxiable, Ini
         templateId = templates++;
         emit DisputeTemplate(templateId, _templateTag, _templateData, _templateDataMappings);
     }
+
+    // ************************************* //
+    // *              Errors               * //
+    // ************************************* //
+
+    error GovernorOnly();
 }

contracts/src/arbitration/KlerosGovernor.sol

@@ -70,18 +70,18 @@ contract KlerosGovernor is IArbitrableV2 {
 
     modifier duringSubmissionPeriod() {
         uint256 offset = sessions[sessions.length - 1].durationOffset;
-        require(block.timestamp - lastApprovalTime <= submissionTimeout + offset, "Submission time has ended.");
+        if (block.timestamp - lastApprovalTime > submissionTimeout + offset) revert SubmissionTimeHasEnded();
         _;
     }
 
     modifier duringApprovalPeriod() {
         uint256 offset = sessions[sessions.length - 1].durationOffset;
-        require(block.timestamp - lastApprovalTime > submissionTimeout + offset, "Approval time not started yet.");
+        if (block.timestamp - lastApprovalTime <= submissionTimeout + offset) revert ApprovalTimeNotStarted();
         _;
     }
 
     modifier onlyByGovernor() {
-        require(address(this) == msg.sender, "Only the governor allowed.");
+        if (address(this) != msg.sender) revert GovernorOnly();
         _;
     }
 
@@ -208,14 +208,14 @@ contract KlerosGovernor is IArbitrableV2 {
         uint256[] memory _dataSize,
         string memory _description
     ) external payable duringSubmissionPeriod {
-        require(_target.length == _value.length, "Wrong input: target and value");
-        require(_target.length == _dataSize.length, "Wrong input: target and datasize");
+        if (_target.length != _value.length) revert WrongInputTargetAndValue();
+        if (_target.length != _dataSize.length) revert WrongInputTargetAndDatasize();
         Session storage session = sessions[sessions.length - 1];
         Submission storage submission = submissions.push();
         submission.submitter = payable(msg.sender);
         // Do the assignment first to avoid creating a new variable and bypass a 'stack too deep' error.
         submission.deposit = submissionBaseDeposit + arbitrator.arbitrationCost(arbitratorExtraData);
-        require(msg.value >= submission.deposit, "Not enough ETH to cover deposit");
+        if (msg.value < submission.deposit) revert InsufficientDeposit();
 
         bytes32 listHash;
         bytes32 currentTxHash;
@@ -233,7 +233,7 @@ contract KlerosGovernor is IArbitrableV2 {
             currentTxHash = keccak256(abi.encodePacked(transaction.target, transaction.value, transaction.data));
             listHash = keccak256(abi.encodePacked(currentTxHash, listHash));
         }
-        require(!session.alreadySubmitted[listHash], "List already submitted");
+        if (session.alreadySubmitted[listHash]) revert ListAlreadySubmitted();
         session.alreadySubmitted[listHash] = true;
         submission.listHash = listHash;
         submission.submissionTime = block.timestamp;
@@ -256,11 +256,11 @@ contract KlerosGovernor is IArbitrableV2 {
     function withdrawTransactionList(uint256 _submissionID, bytes32 _listHash) external {
         Session storage session = sessions[sessions.length - 1];
         Submission storage submission = submissions[session.submittedLists[_submissionID]];
-        require(block.timestamp - lastApprovalTime <= submissionTimeout / 2, "Should be in first half");
-        // This require statement is an extra check to prevent _submissionID linking to the wrong list because of index swap during withdrawal.
-        require(submission.listHash == _listHash, "Wrong list hash");
-        require(submission.submitter == msg.sender, "Only submitter can withdraw");
-        require(block.timestamp - submission.submissionTime <= withdrawTimeout, "Withdrawing time has passed.");
+        if (block.timestamp - lastApprovalTime > submissionTimeout / 2) revert ShouldOnlyWithdrawInFirstHalf();
+        // This is an extra check to prevent _submissionID linking to the wrong list because of index swap during withdrawal.
+        if (submission.listHash != _listHash) revert WrongListHash();
+        if (submission.submitter != msg.sender) revert OnlySubmitterCanWithdraw();
+        if (block.timestamp - submission.submissionTime > withdrawTimeout) revert WithdrawingTimeHasPassed();
         session.submittedLists[_submissionID] = session.submittedLists[session.submittedLists.length - 1];
         session.alreadySubmitted[_listHash] = false;
         session.submittedLists.pop();
@@ -273,7 +273,7 @@ contract KlerosGovernor is IArbitrableV2 {
     /// If nothing was submitted changes session.
     function executeSubmissions() external duringApprovalPeriod {
         Session storage session = sessions[sessions.length - 1];
-        require(session.status == Status.NoDispute, "Already disputed");
+        if (session.status != Status.NoDispute) revert AlreadyDisputed();
         if (session.submittedLists.length == 0) {
             lastApprovalTime = block.timestamp;
             session.status = Status.Resolved;
@@ -310,9 +310,9 @@ contract KlerosGovernor is IArbitrableV2 {
     /// Note If the final ruling is "0" nothing is approved and deposits will stay locked in the contract.
     function rule(uint256 _disputeID, uint256 _ruling) external override {
         Session storage session = sessions[sessions.length - 1];
-        require(msg.sender == address(arbitrator), "Only arbitrator allowed");
-        require(session.status == Status.DisputeCreated, "Wrong status");
-        require(_ruling <= session.submittedLists.length, "Ruling is out of bounds.");
+        if (msg.sender != address(arbitrator)) revert OnlyArbitratorAllowed();
+        if (session.status != Status.DisputeCreated) revert NotDisputed();
+        if (_ruling > session.submittedLists.length) revert RulingOutOfBounds();
 
         if (_ruling != 0) {
             Submission storage submission = submissions[session.submittedLists[_ruling - 1]];
@@ -338,16 +338,16 @@ contract KlerosGovernor is IArbitrableV2 {
     /// @param _count Number of transactions to execute. Executes until the end if set to "0" or number higher than number of transactions in the list.
     function executeTransactionList(uint256 _listID, uint256 _cursor, uint256 _count) external {
         Submission storage submission = submissions[_listID];
-        require(submission.approved, "Should be approved");
-        require(block.timestamp - submission.approvalTime <= executionTimeout, "Time to execute has passed");
+        if (!submission.approved) revert SubmissionNotApproved();
+        if (block.timestamp - submission.approvalTime > executionTimeout) revert TimeToExecuteHasPassed();
         for (uint256 i = _cursor; i < submission.txs.length && (_count == 0 || i < _cursor + _count); i++) {
             Transaction storage transaction = submission.txs[i];
             uint256 expendableFunds = getExpendableFunds();
             if (!transaction.executed && transaction.value <= expendableFunds) {
                 (bool callResult, ) = transaction.target.call{value: transaction.value}(transaction.data);
                 // An extra check to prevent re-entrancy through target call.
                 if (callResult == true) {
-                    require(!transaction.executed, "Already executed");
+                    if (transaction.executed) revert AlreadyExecuted();
                     transaction.executed = true;
                 }
             }
@@ -407,4 +407,27 @@ contract KlerosGovernor is IArbitrableV2 {
     function getCurrentSessionNumber() external view returns (uint256) {
         return sessions.length - 1;
     }
+
+    // ************************************* //
+    // *              Errors               * //
+    // ************************************* //
+
+    error SubmissionTimeHasEnded();
+    error ApprovalTimeNotStarted();
+    error GovernorOnly();
+    error WrongInputTargetAndValue();
+    error WrongInputTargetAndDatasize();
+    error InsufficientDeposit();
+    error ListAlreadySubmitted();
+    error ShouldOnlyWithdrawInFirstHalf();
+    error WrongListHash();
+    error OnlySubmitterCanWithdraw();
+    error WithdrawingTimeHasPassed();
+    error AlreadyDisputed();
+    error OnlyArbitratorAllowed();
+    error NotDisputed();
+    error RulingOutOfBounds();
+    error SubmissionNotApproved();
+    error TimeToExecuteHasPassed();
+    error AlreadyExecuted();
 }

contracts/src/arbitration/PolicyRegistry.sol

@@ -32,7 +32,7 @@ contract PolicyRegistry is UUPSProxiable, Initializable {
 
     /// @dev Requires that the sender is the governor.
     modifier onlyByGovernor() {
-        require(governor == msg.sender, "No allowed: governor only");
+        if (governor != msg.sender) revert GovernorOnly();
         _;
     }
 
@@ -85,4 +85,10 @@ contract PolicyRegistry is UUPSProxiable, Initializable {
         policies[_courtID] = _policy;
         emit PolicyUpdate(_courtID, _courtName, policies[_courtID]);
     }
+
+    // ************************************* //
+    // *              Errors               * //
+    // ************************************* //
+
+    error GovernorOnly();
 }

contracts/src/arbitration/SortitionModuleBase.sol

@@ -122,12 +122,12 @@ abstract contract SortitionModuleBase is ISortitionModule, Initializable, UUPSPr
     // ************************************* //
 
     modifier onlyByGovernor() {
-        require(address(governor) == msg.sender, "Access not allowed: Governor only.");
+        if (governor != msg.sender) revert GovernorOnly();
         _;
     }
 
     modifier onlyByCore() {
-        require(address(core) == msg.sender, "Access not allowed: KlerosCore only.");
+        if (address(core) != msg.sender) revert KlerosCoreOnly();
         _;
     }
 
@@ -171,23 +171,19 @@ abstract contract SortitionModuleBase is ISortitionModule, Initializable, UUPSPr
 
     function passPhase() external {
         if (phase == Phase.staking) {
-            require(
-                block.timestamp - lastPhaseChange >= minStakingTime,
-                "The minimum staking time has not passed yet."
-            );
-            require(disputesWithoutJurors > 0, "There are no disputes that need jurors.");
+            if (block.timestamp - lastPhaseChange < minStakingTime) revert MinStakingTimeNotPassed();
+            if (disputesWithoutJurors == 0) revert NoDisputesThatNeedJurors();
             rng.requestRandomness(block.number + rngLookahead);
             randomNumberRequestBlock = block.number;
             phase = Phase.generating;
         } else if (phase == Phase.generating) {
             randomNumber = rng.receiveRandomness(randomNumberRequestBlock + rngLookahead);
-            require(randomNumber != 0, "Random number is not ready yet");
+            if (randomNumber == 0) revert RandomNumberNotReady();
             phase = Phase.drawing;
         } else if (phase == Phase.drawing) {
-            require(
-                disputesWithoutJurors == 0 || block.timestamp - lastPhaseChange >= maxDrawingTime,
-                "There are still disputes without jurors and the maximum drawing time has not passed yet."
-            );
+            if (disputesWithoutJurors > 0 && block.timestamp - lastPhaseChange < maxDrawingTime) {
+                revert DisputesWithoutJurorsAndMaxDrawingTimeNotPassed();
+            }
             phase = Phase.staking;
         }
 
@@ -201,17 +197,17 @@ abstract contract SortitionModuleBase is ISortitionModule, Initializable, UUPSPr
     function createTree(bytes32 _key, bytes memory _extraData) external override onlyByCore {
         SortitionSumTree storage tree = sortitionSumTrees[_key];
         uint256 K = _extraDataToTreeK(_extraData);
-        require(tree.K == 0, "Tree already exists.");
-        require(K > 1, "K must be greater than one.");
+        if (tree.K != 0) revert TreeAlreadyExists();
+        if (K <= 1) revert KMustBeGreaterThanOne();
         tree.K = K;
         tree.nodes.push(0);
     }
 
     /// @dev Executes the next delayed stakes.
     /// @param _iterations The number of delayed stakes to execute.
     function executeDelayedStakes(uint256 _iterations) external {
-        require(phase == Phase.staking, "Should be in Staking phase.");
-        require(delayedStakeWriteIndex >= delayedStakeReadIndex, "No delayed stake to execute.");
+        if (phase != Phase.staking) revert NotStakingPhase();
+        if (delayedStakeWriteIndex < delayedStakeReadIndex) revert NoDelayedStakeToExecute();
 
         uint256 actualIterations = (delayedStakeReadIndex + _iterations) - 1 > delayedStakeWriteIndex
             ? (delayedStakeWriteIndex - delayedStakeReadIndex) + 1
@@ -420,7 +416,7 @@ abstract contract SortitionModuleBase is ISortitionModule, Initializable, UUPSPr
         // Can withdraw the leftover PNK if fully unstaked, has no tokens locked and has positive balance.
         // This withdrawal can't be triggered by calling setStake() in KlerosCore because current stake is technically 0, thus it is done via separate function.
         uint256 amount = getJurorLeftoverPNK(_account);
-        require(amount > 0, "Not eligible for withdrawal.");
+        if (amount == 0) revert NotEligibleForWithdrawal();
         jurors[_account].stakedPnk = 0;
         core.transferBySortitionModule(_account, amount);
         emit LeftoverPNKWithdrawn(_account, amount);
@@ -444,7 +440,7 @@ abstract contract SortitionModuleBase is ISortitionModule, Initializable, UUPSPr
         uint256 _coreDisputeID,
         uint256 _nonce
     ) public view override returns (address drawnAddress) {
-        require(phase == Phase.drawing, "Wrong phase.");
+        if (phase != Phase.drawing) revert NotDrawingPhase();
         SortitionSumTree storage tree = sortitionSumTrees[_key];
 
         if (tree.nodes[0] == 0) {
@@ -692,4 +688,21 @@ abstract contract SortitionModuleBase is ISortitionModule, Initializable, UUPSPr
             stakePathID := mload(ptr)
         }
     }
+
+    // ************************************* //
+    // *              Errors               * //
+    // ************************************* //
+
+    error GovernorOnly();
+    error KlerosCoreOnly();
+    error MinStakingTimeNotPassed();
+    error NoDisputesThatNeedJurors();
+    error RandomNumberNotReady();
+    error DisputesWithoutJurorsAndMaxDrawingTimeNotPassed();
+    error TreeAlreadyExists();
+    error KMustBeGreaterThanOne();
+    error NotStakingPhase();
+    error NoDelayedStakeToExecute();
+    error NotEligibleForWithdrawal();
+    error NotDrawingPhase();
 }

contracts/src/arbitration/arbitrables/ArbitrableExample.sol

@@ -37,7 +37,7 @@ contract ArbitrableExample is IArbitrableV2 {
     // ************************************* //
 
     modifier onlyByGovernor() {
-        require(msg.sender == governor, "Only the governor allowed.");
+        if (governor != msg.sender) revert GovernorOnly();
         _;
     }
 
@@ -126,8 +126,8 @@ contract ArbitrableExample is IArbitrableV2 {
         uint256 localDisputeID = disputes.length;
         disputes.push(DisputeStruct({isRuled: false, ruling: 0, numberOfRulingOptions: numberOfRulingOptions}));
 
-        require(weth.safeTransferFrom(msg.sender, address(this), _feeInWeth), "Transfer failed");
-        require(weth.increaseAllowance(address(arbitrator), _feeInWeth), "Allowance increase failed");
+        if (!weth.safeTransferFrom(msg.sender, address(this), _feeInWeth)) revert TransferFailed();
+        if (!weth.increaseAllowance(address(arbitrator), _feeInWeth)) revert AllowanceIncreaseFailed();
 
         disputeID = arbitrator.createDispute(numberOfRulingOptions, arbitratorExtraData, weth, _feeInWeth);
         externalIDtoLocalID[disputeID] = localDisputeID;
@@ -142,13 +142,24 @@ contract ArbitrableExample is IArbitrableV2 {
     function rule(uint256 _arbitratorDisputeID, uint256 _ruling) external override {
         uint256 localDisputeID = externalIDtoLocalID[_arbitratorDisputeID];
         DisputeStruct storage dispute = disputes[localDisputeID];
-        require(msg.sender == address(arbitrator), "Only the arbitrator can execute this.");
-        require(_ruling <= dispute.numberOfRulingOptions, "Invalid ruling.");
-        require(dispute.isRuled == false, "This dispute has been ruled already.");
+        if (msg.sender != address(arbitrator)) revert ArbitratorOnly();
+        if (_ruling > dispute.numberOfRulingOptions) revert RulingOutOfBounds();
+        if (dispute.isRuled) revert DisputeAlreadyRuled();
 
         dispute.isRuled = true;
         dispute.ruling = _ruling;
 
         emit Ruling(IArbitratorV2(msg.sender), _arbitratorDisputeID, dispute.ruling);
     }
+
+    // ************************************* //
+    // *              Errors               * //
+    // ************************************* //
+
+    error GovernorOnly();
+    error TransferFailed();
+    error AllowanceIncreaseFailed();
+    error ArbitratorOnly();
+    error RulingOutOfBounds();
+    error DisputeAlreadyRuled();
 }