Merge pull request #1765 from kleros/feat/foundry-tests

jaybuidl · web-flow · commit f9cb5a695b89 · 2024-12-18T13:51:39.000Z
feat(KC): add foundry test file
diff --git a/.github/workflows/contracts-testing.yml b/.github/workflows/contracts-testing.yml
@@ -40,14 +40,16 @@ jobs:
           54.185.253.63:443
 
     - name: Setup Node.js environment
-      uses: actions/setup-node@v4
+      uses: actions/setup-node@39370e3970a6d050c480ffad4ff0ed4d3fdee5af # v4.1.0
       with:
         node-version: 18.x   
         
-    - uses: actions/checkout@v4
+    - uses: actions/checkout@11bd71901bbe5b1630ceea73d27597364c9af683 # v4.2.2
+      with:
+        submodules: recursive
     
     - name: Cache node modules
-      uses: actions/cache@v4
+      uses: actions/cache@1bd1e32a3bdc45362d1e726936510720a7c30a57 # v4.2.0
       env:
         cache-name: cache-node-modules
       with:
@@ -57,28 +59,22 @@ jobs:
         key: ${{ runner.os }}-build-${{ secrets.CACHE_VERSION }}-${{ env.cache-name }}-${{ hashFiles('**/package-lock.json', '**/yarn.lock') }}
         restore-keys: |
           ${{ runner.os }}-build-${{ secrets.CACHE_VERSION }}-${{ env.cache-name }}-
-          
-    #- name: Install parent dependencies
-    #  run: |
-    #    echo "current dir: $PWD"
-    #    yarn install
-             
+
     - name: Install contracts dependencies
-      run: |
-        yarn workspace @kleros/kleros-v2-contracts install
-      
-    - name: Compile
-      run: |
-        yarn hardhat compile
-      working-directory: contracts
-      
-    - name: Test with coverage
-      run: |         
-        yarn hardhat coverage --solcoverjs ./.solcover.js --temp artifacts --testfiles './test/**/*.ts' --show-stack-traces
-      working-directory: contracts
+      run: yarn workspace @kleros/kleros-v2-contracts install
+
+    - name: Install Foundry
+      uses: foundry-rs/foundry-toolchain@8f1998e9878d786675189ef566a2e4bf24869773 # v1.2.0
 
+    - name: Install lcov
+      run: sudo apt-get install -y lcov
+
+    - name: Run Hardhat and Foundry tests with coverage
+      run: yarn coverage
+      working-directory: contracts
+                   
     - name: Upload a build artifact
-      uses: actions/upload-artifact@v4
+      uses: actions/upload-artifact@b4b15b8c7c6ac21ea08fcf65892d2ee8f75cf882 # v4.4.3
       with:    
         name: code-coverage-report    
         path: contracts/coverage
diff --git a/contracts/.solcover.js b/contracts/.solcover.js
@@ -5,7 +5,7 @@ const shell = require("shelljs");
 // The environment variables are loaded in hardhat.config.ts
 
 module.exports = {
-  istanbulReporter: ["html"],
+  istanbulReporter: ["lcov"],
   onCompileComplete: async function (_config) {
     await run("typechain");
   },
@@ -14,7 +14,7 @@ module.exports = {
     shell.rm("-rf", "./artifacts");
     shell.rm("-rf", "./typechain");
   },
-  skipFiles: ["mocks", "test"],
+  skipFiles: ["test", "token", "kleros-v1", "proxy/mock", "gateway/mock", "rng/mock"],
   mocha: {
     timeout: 20000,
     grep: "@skip-on-coverage", // Find everything with this tag
diff --git a/contracts/lib/forge-std b/contracts/lib/forge-std
@@ -1 +1 @@
-Subproject commit 066ff16c5c03e6f931cd041fd366bc4be1fae82a
+Subproject commit 8f24d6b04c92975e0795b5868aa0d783251cdeaa
diff --git a/contracts/package.json b/contracts/package.json
@@ -22,6 +22,7 @@
     "clean": "hardhat clean",
     "check": "hardhat check",
     "test": "TS_NODE_TRANSPILE_ONLY=1 hardhat test",
+    "coverage": "scripts/coverage.sh",
     "start": "hardhat node --tags nop",
     "start-local": "hardhat node --tags Arbitration,HomeArbitrable --hostname 0.0.0.0",
     "deploy": "hardhat deploy",
diff --git a/contracts/scripts/coverage.sh b/contracts/scripts/coverage.sh
@@ -0,0 +1,62 @@
+#!/usr/bin/env bash
+
+set -e # exit on error
+
+rm -rf coverage
+mkdir -p coverage
+
+# Generate the Forge coverage report
+forge clean
+if [ "$CI" != "true" ]; then
+  forge coverage --report summary --report lcov --report-file coverage/lcov-forge.info
+else 
+  # FIXME: Temporarily workaround a CI issue
+  touch coverage/lcov-forge.info
+fi
+
+# Generate the Hardhat coverage report
+yarn clean
+yarn hardhat coverage --solcoverjs ./.solcover.js --temp artifacts --show-stack-traces --testfiles "test/**/*.ts"
+mv coverage/lcov.info coverage/lcov-hardhat.info
+
+# Make the Hardhat report paths relative for consistency with Forge coverage report
+sed -i -e 's/\/.*\/kleros-v2\/contracts\///g' coverage/lcov-hardhat.info
+
+# Merge the two reports
+lcov \
+  --ignore-errors format \
+  --ignore-errors inconsistent \
+  --ignore-errors empty \
+  --rc max_message_count=3 \
+  --rc derive_function_end_line=0 \
+  --rc branch_coverage=1 \
+  --add-tracefile coverage/lcov-hardhat.info \
+  --add-tracefile coverage/lcov-forge.info \
+  --output-file coverage/merged-lcov.info
+
+# Filter out unnecessary contracts from the report
+lcov \
+  --ignore-errors format \
+  --ignore-errors inconsistent \
+  --ignore-errors empty \
+  --ignore-errors unused \
+  --rc max_message_count=3 \
+  --rc branch_coverage=1 \
+  --rc derive_function_end_line=0 \
+  --remove coverage/merged-lcov.info \
+  --output-file coverage/filtered-lcov.info \
+  "../node_modules" "src/test" "src/token" "src/kleros-v1" "src/proxy/mock" "src/gateway/mock" "src/rng/mock"
+
+# Open more granular breakdown in browser
+if [ "$CI" != "true" ]; then
+  # Generate the HTML report
+  genhtml coverage/filtered-lcov.info \
+    --ignore-errors format \
+    --ignore-errors inconsistent \
+    --ignore-errors empty \
+    --ignore-errors category \
+    --rc branch_coverage=1 \
+    --rc max_message_count=3 \
+    -o coverage
+  open coverage/index.html
+fi
diff --git a/contracts/src/arbitration/KlerosCoreBase.sol b/contracts/src/arbitration/KlerosCoreBase.sol
@@ -113,7 +113,7 @@ abstract contract KlerosCoreBase is IArbitratorV2 {
     event AppealDecision(uint256 indexed _disputeID, IArbitrableV2 indexed _arbitrable);
     event Draw(address indexed _address, uint256 indexed _disputeID, uint256 _roundID, uint256 _voteID);
     event CourtCreated(
-        uint256 indexed _courtID,
+        uint96 indexed _courtID,
         uint96 indexed _parent,
         bool _hiddenVotes,
         uint256 _minStake,
@@ -237,16 +237,18 @@ abstract contract KlerosCoreBase is IArbitratorV2 {
 
         sortitionModule.createTree(bytes32(uint256(GENERAL_COURT)), _sortitionExtraData);
 
+        uint256[] memory supportedDisputeKits = new uint256[](1);
+        supportedDisputeKits[0] = DISPUTE_KIT_CLASSIC;
         emit CourtCreated(
-            1,
+            GENERAL_COURT,
             court.parent,
             _hiddenVotes,
             _courtParameters[0],
             _courtParameters[1],
             _courtParameters[2],
             _courtParameters[3],
             _timesPerPeriod,
-            new uint256[](0)
+            supportedDisputeKits
         );
         _enableDisputeKit(GENERAL_COURT, DISPUTE_KIT_CLASSIC, true);
     }
@@ -351,7 +353,7 @@ abstract contract KlerosCoreBase is IArbitratorV2 {
             if (_supportedDisputeKits[i] == 0 || _supportedDisputeKits[i] >= disputeKits.length) {
                 revert WrongDisputeKitIndex();
             }
-            court.supportedDisputeKits[_supportedDisputeKits[i]] = true;
+            _enableDisputeKit(uint96(courtID), _supportedDisputeKits[i], true);
         }
         // Check that Classic DK support was added.
         if (!court.supportedDisputeKits[DISPUTE_KIT_CLASSIC]) revert MustSupportDisputeKitClassic();
@@ -370,7 +372,7 @@ abstract contract KlerosCoreBase is IArbitratorV2 {
         // Update the parent.
         courts[_parent].children.push(courtID);
         emit CourtCreated(
-            courtID,
+            uint96(courtID),
             _parent,
             _hiddenVotes,
             _minStake,
@@ -1061,7 +1063,7 @@ abstract contract KlerosCoreBase is IArbitratorV2 {
         bool _alreadyTransferred,
         OnError _onError
     ) internal returns (bool) {
-        if (_courtID == FORKING_COURT || _courtID > courts.length) {
+        if (_courtID == FORKING_COURT || _courtID >= courts.length) {
             _stakingFailed(_onError, StakingResult.CannotStakeInThisCourt); // Staking directly into the forking court is not allowed.
             return false;
         }
@@ -1102,6 +1104,7 @@ abstract contract KlerosCoreBase is IArbitratorV2 {
         if (_result == StakingResult.CannotStakeInMoreCourts) revert StakingInTooManyCourts();
         if (_result == StakingResult.CannotStakeInThisCourt) revert StakingNotPossibeInThisCourt();
         if (_result == StakingResult.CannotStakeLessThanMinStake) revert StakingLessThanCourtMinStake();
+        if (_result == StakingResult.CannotStakeZeroWhenNoStake) revert StakingZeroWhenNoStake();
     }
 
     /// @dev Gets a court ID, the minimum number of jurors and an ID of a dispute kit from a specified extra data bytes array.
@@ -1147,13 +1150,11 @@ abstract contract KlerosCoreBase is IArbitratorV2 {
     error SortitionModuleOnly();
     error UnsuccessfulCall();
     error InvalidDisputKitParent();
-    error DepthLevelMax();
     error MinStakeLowerThanParentCourt();
     error UnsupportedDisputeKit();
     error InvalidForkingCourtAsParent();
     error WrongDisputeKitIndex();
     error CannotDisableClassicDK();
-    error ArraysLengthMismatch();
     error StakingInTooManyCourts();
     error StakingNotPossibeInThisCourt();
     error StakingLessThanCourtMinStake();
@@ -1177,4 +1178,5 @@ abstract contract KlerosCoreBase is IArbitratorV2 {
     error TransferFailed();
     error WhenNotPausedOnly();
     error WhenPausedOnly();
+    error StakingZeroWhenNoStake();
 }
diff --git a/contracts/src/arbitration/SortitionModuleBase.sol b/contracts/src/arbitration/SortitionModuleBase.sol
@@ -206,13 +206,14 @@ abstract contract SortitionModuleBase is ISortitionModule {
             DelayedStake storage delayedStake = delayedStakes[i];
             // Delayed stake could've been manually removed already. In this case simply move on to the next item.
             if (delayedStake.account != address(0)) {
+                // Nullify the index so the delayed stake won't get deleted before its own execution.
+                delete latestDelayedStakeIndex[delayedStake.account][delayedStake.courtID];
                 core.setStakeBySortitionModule(
                     delayedStake.account,
                     delayedStake.courtID,
                     delayedStake.stake,
                     delayedStake.alreadyTransferred
                 );
-                delete latestDelayedStakeIndex[delayedStake.account][delayedStake.courtID];
                 delete delayedStakes[i];
             }
         }
@@ -265,13 +266,16 @@ abstract contract SortitionModuleBase is ISortitionModule {
         uint256 currentStake = stakeOf(_account, _courtID);
 
         uint256 nbCourts = juror.courtIDs.length;
-        if (_newStake == 0 && (nbCourts >= MAX_STAKE_PATHS || currentStake == 0)) {
+        if (currentStake == 0 && nbCourts >= MAX_STAKE_PATHS) {
             return (0, 0, StakingResult.CannotStakeInMoreCourts); // Prevent staking beyond MAX_STAKE_PATHS but unstaking is always allowed.
         }
 
-        if (phase != Phase.staking) {
-            pnkWithdrawal = _deleteDelayedStake(_courtID, _account);
+        if (currentStake == 0 && _newStake == 0) {
+            return (0, 0, StakingResult.CannotStakeZeroWhenNoStake); // Forbid staking 0 amount when current stake is 0 to avoid flaky behaviour.
+        }
 
+        pnkWithdrawal = _deleteDelayedStake(_courtID, _account);
+        if (phase != Phase.staking) {
             // Store the stake change as delayed, to be applied when the phase switches back to Staking.
             DelayedStake storage delayedStake = delayedStakes[++delayedStakeWriteIndex];
             delayedStake.account = _account;
diff --git a/contracts/src/arbitration/dispute-kits/DisputeKitClassic.sol b/contracts/src/arbitration/dispute-kits/DisputeKitClassic.sol
@@ -238,7 +238,6 @@ contract DisputeKitClassic is IDisputeKit, Initializable, UUPSProxiable {
         (uint96 courtID, , , , ) = core.disputes(_coreDisputeID);
         bytes32 key = bytes32(uint256(courtID)); // Get the ID of the tree.
 
-        // TODO: Handle the situation when no one has staked yet.
         drawnAddress = sortitionModule.draw(key, _coreDisputeID, _nonce);
 
         if (_postDrawCheck(_coreDisputeID, drawnAddress)) {
@@ -603,6 +602,10 @@ contract DisputeKitClassic is IDisputeKit, Initializable, UUPSProxiable {
     /// @param _coreDisputeID ID of the dispute in the core contract.
     /// @param _juror Chosen address.
     /// @return Whether the address can be drawn or not.
+    /// Note that we don't check the minStake requirement here because of the implicit staking in parent courts.
+    /// minStake is checked directly during staking process however it's possible for the juror to get drawn
+    /// while having < minStake if it is later increased by governance.
+    /// This issue is expected and harmless since we check for insolvency anyway.
     function _postDrawCheck(uint256 _coreDisputeID, address _juror) internal view returns (bool) {
         (uint96 courtID, , , , ) = core.disputes(_coreDisputeID);
         uint256 lockedAmountPerJuror = core
diff --git a/contracts/src/arbitration/dispute-kits/DisputeKitSybilResistant.sol b/contracts/src/arbitration/dispute-kits/DisputeKitSybilResistant.sol
@@ -255,7 +255,6 @@ contract DisputeKitSybilResistant is IDisputeKit, Initializable, UUPSProxiable {
         (uint96 courtID, , , , ) = core.disputes(_coreDisputeID);
         bytes32 key = bytes32(uint256(courtID)); // Get the ID of the tree.
 
-        // TODO: Handle the situation when no one has staked yet.
         drawnAddress = sortitionModule.draw(key, _coreDisputeID, _nonce);
 
         if (_postDrawCheck(_coreDisputeID, drawnAddress) && !round.alreadyDrawn[drawnAddress]) {
@@ -621,6 +620,10 @@ contract DisputeKitSybilResistant is IDisputeKit, Initializable, UUPSProxiable {
     /// @param _coreDisputeID ID of the dispute in the core contract.
     /// @param _juror Chosen address.
     /// @return Whether the address can be drawn or not.
+    /// Note that we don't check the minStake requirement here because of the implicit staking in parent courts.
+    /// minStake is checked directly during staking process however it's possible for the juror to get drawn
+    /// while having < minStake if it is later increased by governance.
+    /// This issue is expected and harmless since we check for insolvency anyway.
     function _postDrawCheck(uint256 _coreDisputeID, address _juror) internal view returns (bool) {
         (uint96 courtID, , , , ) = core.disputes(_coreDisputeID);
         uint256 lockedAmountPerJuror = core
diff --git a/contracts/src/arbitration/interfaces/IDisputeKit.sol b/contracts/src/arbitration/interfaces/IDisputeKit.sol
@@ -41,6 +41,7 @@ interface IDisputeKit {
     /// @param _coreDisputeID The ID of the dispute in Kleros Core, not in the Dispute Kit.
     /// @param _numberOfChoices Number of choices of the dispute
     /// @param _extraData Additional info about the dispute, for possible use in future dispute kits.
+    /// @param _nbVotes Maximal number of votes this dispute can get. DEPRECATED as we don't need to pass it now. KC handles the count.
     function createDispute(
         uint256 _coreDisputeID,
         uint256 _numberOfChoices,
diff --git a/contracts/src/libraries/Constants.sol b/contracts/src/libraries/Constants.sol
@@ -9,7 +9,7 @@ uint96 constant FORKING_COURT = 0; // Index of the forking court.
 uint96 constant GENERAL_COURT = 1; // Index of the default (general) court.
 
 // Dispute Kits
-uint256 constant NULL_DISPUTE_KIT = 0; // Null pattern to indicate a top-level DK which has no parent.
+uint256 constant NULL_DISPUTE_KIT = 0; // Null pattern to indicate a top-level DK which has no parent. DEPRECATED, as its main purpose was to accommodate forest structure which is not used now.
 uint256 constant DISPUTE_KIT_CLASSIC = 1; // Index of the default DK. 0 index is skipped.
 
 // Sortition Module
@@ -33,5 +33,6 @@ enum StakingResult {
     CannotStakeInThisCourt,
     CannotStakeLessThanMinStake,
     CannotStakeMoreThanMaxStakePerJuror,
-    CannotStakeMoreThanMaxTotalStaked
+    CannotStakeMoreThanMaxTotalStaked,
+    CannotStakeZeroWhenNoStake
 }
diff --git a/contracts/src/test/KlerosCoreMock.sol b/contracts/src/test/KlerosCoreMock.sol
@@ -0,0 +1,25 @@
+// SPDX-License-Identifier: MIT
+
+/// @custom:authors: [@unknownunknown1]
+/// @custom:reviewers: []
+/// @custom:auditors: []
+/// @custom:bounties: []
+/// @custom:deployments: []
+
+pragma solidity 0.8.24;
+
+import "../arbitration/KlerosCore.sol";
+
+/// @title KlerosCoreMock
+/// KlerosCore with view functions to use in Foundry tests.
+contract KlerosCoreMock is KlerosCore {
+    function getCourtChildren(uint256 _courtId) external view returns (uint256[] memory children) {
+        children = courts[_courtId].children;
+    }
+
+    function extraDataToCourtIDMinJurorsDisputeKit(
+        bytes memory _extraData
+    ) external view returns (uint96 courtID, uint256 minJurors, uint256 disputeKitID) {
+        (courtID, minJurors, disputeKitID) = _extraDataToCourtIDMinJurorsDisputeKit(_extraData);
+    }
+}
diff --git a/contracts/src/test/SortitionModuleMock.sol b/contracts/src/test/SortitionModuleMock.sol
@@ -0,0 +1,23 @@
+// SPDX-License-Identifier: MIT
+
+/**
+ *  @custom:authors: [unknownunknown1]
+ *  @custom:reviewers: []
+ *  @custom:auditors: []
+ *  @custom:bounties: []
+ *  @custom:deployments: []
+ */
+
+pragma solidity 0.8.24;
+
+import "../arbitration/SortitionModule.sol";
+
+/// @title SortitionModuleMock
+/// @dev Adds getter functions to sortition module for Foundry tests.
+contract SortitionModuleMock is SortitionModule {
+    function getSortitionProperties(bytes32 _key) external view returns (uint256 K, uint256 nodeLength) {
+        SortitionSumTree storage tree = sortitionSumTrees[_key];
+        K = tree.K;
+        nodeLength = tree.nodes.length;
+    }
+}
diff --git a/contracts/test/arbitration/index.ts b/contracts/test/arbitration/index.ts
@@ -30,7 +30,7 @@ describe("DisputeKitClassic", async () => {
     expect(events2[0].args._feeForJuror).to.equal(ethers.parseUnits("0.1", 18));
     expect(events2[0].args._jurorsForCourtJump).to.equal(256);
     expect(events2[0].args._timesPerPeriod).to.deep.equal([0, 0, 0, 10]);
-    expect(events2[0].args._supportedDisputeKits).to.deep.equal([]);
+    expect(events2[0].args._supportedDisputeKits).to.deep.equal([1]);
 
     const events3 = await core.queryFilter(core.filters.DisputeKitEnabled());
     expect(events3.length).to.equal(1);
diff --git a/contracts/test/foundry/Contract.t.sol b/contracts/test/foundry/Contract.t.sol
diff --git a/contracts/test/foundry/KlerosCore.t.sol b/contracts/test/foundry/KlerosCore.t.sol
diff --git a/contracts/test/rng/index.ts b/contracts/test/rng/index.ts
diff --git a/cspell.json b/cspell.json
