Contracts review feedback - round 1

contracts/src/arbitration/KlerosCore.sol

@@ -424,7 +424,8 @@ contract KlerosCore is IArbitratorV2, Initializable, UUPSProxiable {
         sortitionModule = _sortitionModule;
     }
 
-    /// @notice Add a new supported dispute kit module to the court.
+    /// @notice Add a new supported dispute kit, without enabling it.
+    /// Use `enableDisputeKits()` to enable the dispute kit for a specific court.
     /// @param _disputeKitAddress The address of the dispute kit contract.
     function addNewDisputeKit(IDisputeKit _disputeKitAddress) external onlyByOwner {
         uint256 disputeKitID = disputeKits.length;
@@ -461,7 +462,7 @@ contract KlerosCore is IArbitratorV2, Initializable, UUPSProxiable {
         Court storage court = courts.push();
 
         for (uint256 i = 0; i < _supportedDisputeKits.length; i++) {
-            if (_supportedDisputeKits[i] == 0 || _supportedDisputeKits[i] >= disputeKits.length) {
+            if (_supportedDisputeKits[i] == NULL_DISPUTE_KIT || _supportedDisputeKits[i] >= disputeKits.length) {
                 revert WrongDisputeKitIndex();
             }
             _enableDisputeKit(uint96(courtID), _supportedDisputeKits[i], true);
@@ -483,7 +484,7 @@ contract KlerosCore is IArbitratorV2, Initializable, UUPSProxiable {
         // Update the parent.
         courts[_parent].children.push(courtID);
         emit CourtCreated(
-            uint96(courtID),
+            courtID,
             _parent,
             _hiddenVotes,
             _minStake,
@@ -518,7 +519,7 @@ contract KlerosCore is IArbitratorV2, Initializable, UUPSProxiable {
         }
         for (uint256 i = 0; i < court.children.length; i++) {
             if (courts[court.children[i]].minStake < _minStake) {
-                revert MinStakeLowerThanParentCourt();
+                revert MinStakeHigherThanChildCourt(court.children[i]);
             }
         }
         court.minStake = _minStake;
@@ -544,10 +545,10 @@ contract KlerosCore is IArbitratorV2, Initializable, UUPSProxiable {
     /// @param _enable Whether add or remove the dispute kits from the court.
     function enableDisputeKits(uint96 _courtID, uint256[] memory _disputeKitIDs, bool _enable) external onlyByOwner {
         for (uint256 i = 0; i < _disputeKitIDs.length; i++) {
+            if (_disputeKitIDs[i] == NULL_DISPUTE_KIT || _disputeKitIDs[i] >= disputeKits.length) {
+                revert WrongDisputeKitIndex();
+            }
             if (_enable) {
-                if (_disputeKitIDs[i] == 0 || _disputeKitIDs[i] >= disputeKits.length) {
-                    revert WrongDisputeKitIndex();
-                }
                 _enableDisputeKit(_courtID, _disputeKitIDs[i], true);
             } else {
                 // Classic dispute kit must be supported by all courts.
@@ -940,7 +941,7 @@ contract KlerosCore is IArbitratorV2, Initializable, UUPSProxiable {
         }
 
         if (pnkBalance == 0 || !disputeKit.isVoteActive(_params.disputeID, _params.round, _params.repartition)) {
-            // The juror is inactive or their balance is can't cover penalties anymore, unstake them from all courts.
+            // The juror is inactive or their balance can't cover penalties anymore, unstake them from all courts.
             sortitionModule.forcedUnstakeAllCourts(account);
         } else if (newCourtStake < courts[penalizedInCourtID].minStake) {
             // The juror's balance fell below the court minStake, unstake them from the court.
@@ -1398,8 +1399,9 @@ contract KlerosCore is IArbitratorV2, Initializable, UUPSProxiable {
     error DisputeKitOnly();
     error SortitionModuleOnly();
     error UnsuccessfulCall();
-    error InvalidDisputKitParent();
+    error InvalidDisputeKitParent();
     error MinStakeLowerThanParentCourt();
+    error MinStakeHigherThanChildCourt(uint256 _childCourtID);
     error UnsupportedDisputeKit();
     error InvalidForkingCourtAsParent();
     error WrongDisputeKitIndex();

contracts/src/arbitration/SortitionModule.sol

@@ -343,14 +343,14 @@ contract SortitionModule is ISortitionModule, Initializable, UUPSProxiable {
 
         if (availablePenalty == 0) return (juror.stakedPnk, newCourtStake, 0); // No penalty to apply.
 
-        uint256 currentStake = _stakeOf(_account, _courtID);
+        uint256 currentStake = newCourtStake;
         uint256 newStake = 0;
         if (currentStake >= availablePenalty) {
             newStake = currentStake - availablePenalty;
         }
         _setStake(_account, _courtID, 0, availablePenalty, newStake);
         pnkBalance = juror.stakedPnk; // updated by _setStake()
-        newCourtStake = _stakeOf(_account, _courtID); // updated by _setStake()
+        newCourtStake = newStake;
     }
 
     /// @inheritdoc ISortitionModule

contracts/src/arbitration/dispute-kits/DisputeKitClassicBase.sol

@@ -370,7 +370,7 @@ abstract contract DisputeKitClassicBase is IDisputeKit, Initializable, UUPSProxi
         if (!coreDisputeIDToActive[_coreDisputeID]) revert NotActiveForCoreDisputeID();
 
         (uint256 appealPeriodStart, uint256 appealPeriodEnd) = core.appealPeriod(_coreDisputeID);
-        if (block.timestamp < appealPeriodStart || block.timestamp >= appealPeriodEnd) revert AppealPeriodIsOver();
+        if (block.timestamp < appealPeriodStart || block.timestamp >= appealPeriodEnd) revert NotAppealPeriod();
 
         uint256 multiplier;
         (uint256 ruling, , ) = this.currentRuling(_coreDisputeID);
@@ -381,7 +381,7 @@ abstract contract DisputeKitClassicBase is IDisputeKit, Initializable, UUPSProxi
                 block.timestamp - appealPeriodStart >=
                 ((appealPeriodEnd - appealPeriodStart) * LOSER_APPEAL_PERIOD_MULTIPLIER) / ONE_BASIS_POINT
             ) {
-                revert AppealPeriodIsOverForLoser();
+                revert NotAppealPeriodForLoser();
             }
             multiplier = LOSER_STAKE_MULTIPLIER;
         }
@@ -759,8 +759,8 @@ abstract contract DisputeKitClassicBase is IDisputeKit, Initializable, UUPSProxi
     error ChoiceOutOfBounds();
     error HashDoesNotMatchHiddenVoteCommitment();
     error VoteAlreadyCast();
-    error AppealPeriodIsOver();
-    error AppealPeriodIsOverForLoser();
+    error NotAppealPeriod();
+    error NotAppealPeriodForLoser();
     error AppealFeeIsAlreadyPaid();
     error DisputeNotResolved();
     error CoreIsPaused();

contracts/src/arbitration/interfaces/IArbitratorV2.sol

@@ -50,7 +50,7 @@ interface IArbitratorV2 {
     ) external payable returns (uint256 disputeID);
 
     /// @notice Create a dispute and pay for the fees in a supported ERC20 token.
-    /// @dev Must be called by the arbitrable contract and pay at least `arbitrationCost(_extraData)` in the supported ERC20 token.
+    /// @dev Must be called by the arbitrable contract and pay at least `arbitrationCost(_extraData, _feeToken)` in the supported ERC20 token.
     /// @param _numberOfChoices The number of choices the arbitrator can choose from in this dispute.
     /// @param _extraData Additional info about the dispute. We use it to pass the ID of the dispute's court (first 32 bytes), the minimum number of jurors required (next 32 bytes) and the ID of the specific dispute kit (last 32 bytes).
     /// @param _feeToken The ERC20 token used to pay fees.

contracts/src/arbitration/interfaces/ISortitionModule.sol

@@ -78,7 +78,7 @@ interface ISortitionModule {
         uint256 _newStake
     ) external;
 
-    /// @notice Update the state of the stakes with a PNK reward deposit, called by KC during rewards execution.
+    /// @notice Update the state of the stakes with a PNK penalty, called by KC during rewards execution.
     ///
     /// @dev `O(n + p * log_k(j))` where
     /// `n` is the number of courts the juror has staked in,

contracts/src/arbitration/university/KlerosCoreUniversity.sol

@@ -1173,7 +1173,7 @@ contract KlerosCoreUniversity is IArbitratorV2, UUPSProxiable, Initializable {
     error DisputeKitOnly();
     error SortitionModuleOnly();
     error UnsuccessfulCall();
-    error InvalidDisputKitParent();
+    error InvalidDisputeKitParent();
     error MinStakeLowerThanParentCourt();
     error UnsupportedDisputeKit();
     error InvalidForkingCourtAsParent();

contracts/test/foundry/KlerosCore_Appeals.t.sol

@@ -127,22 +127,22 @@ contract KlerosCore_AppealsTest is KlerosCore_TestBase {
         disputeKit.castVote(disputeID, voteIDs, 2, 0, "XYZ");
 
         vm.prank(crowdfunder1);
-        vm.expectRevert(DisputeKitClassicBase.AppealPeriodIsOver.selector);
+        vm.expectRevert(DisputeKitClassicBase.NotAppealPeriod.selector);
         disputeKit.fundAppeal{value: 0.1 ether}(disputeID, 1);
         core.passPeriod(disputeID);
 
         (uint256 start, uint256 end) = core.appealPeriod(0);
 
         vm.prank(crowdfunder1);
         vm.warp(block.timestamp + ((end - start) / 2 + 1));
-        vm.expectRevert(DisputeKitClassicBase.AppealPeriodIsOverForLoser.selector);
+        vm.expectRevert(DisputeKitClassicBase.NotAppealPeriodForLoser.selector);
         disputeKit.fundAppeal{value: 0.1 ether}(disputeID, 1); // Losing choice
 
         disputeKit.fundAppeal(disputeID, 2); // Winning choice funding should not revert yet
 
         vm.prank(crowdfunder1);
         vm.warp(block.timestamp + (end - start) / 2); // Warp one more to cover the whole period
-        vm.expectRevert(DisputeKitClassicBase.AppealPeriodIsOver.selector);
+        vm.expectRevert(DisputeKitClassicBase.NotAppealPeriod.selector);
         disputeKit.fundAppeal{value: 0.1 ether}(disputeID, 2);
     }
 

contracts/test/foundry/KlerosCore_Governance.t.sol

@@ -310,7 +310,7 @@ contract KlerosCore_GovernanceTest is KlerosCore_TestBase {
             50, // jurors for jump
             [uint256(10), uint256(20), uint256(30), uint256(40)] // Times per period
         );
-        vm.expectRevert(KlerosCore.MinStakeLowerThanParentCourt.selector);
+        vm.expectRevert(abi.encodeWithSelector(KlerosCore.MinStakeHigherThanChildCourt.selector, newCourtID));
         vm.prank(owner);
         // Min stake of a parent became higher than of a child
         core.changeCourtParameters(