From 374b4f28884d3e5a269bbdda62ab26d043987894 Mon Sep 17 00:00:00 2001 From: jaybuidl Date: Mon, 13 Oct 2025 23:42:54 +0100 Subject: [PATCH 1/2] refactor: active/jump checks, no more _round param in withdrawFeesAndRewards(), DK errors renamed --- .../dispute-kits/DisputeKitClassicBase.sol | 100 ++++++++-------- .../dispute-kits/DisputeKitGatedShutter.sol | 2 +- .../dispute-kits/DisputeKitShutter.sol | 2 +- .../arbitration/interfaces/IDisputeKit.sol | 1 + .../test/foundry/KlerosCore_Appeals.t.sol | 112 ++++++++---------- .../test/foundry/KlerosCore_Disputes.t.sol | 10 +- .../test/foundry/KlerosCore_Execution.t.sol | 12 +- .../test/foundry/KlerosCore_Voting.t.sol | 11 +- 8 files changed, 117 insertions(+), 133 deletions(-) diff --git a/contracts/src/arbitration/dispute-kits/DisputeKitClassicBase.sol b/contracts/src/arbitration/dispute-kits/DisputeKitClassicBase.sol index fddf384e4..859b3b0f5 100644 --- a/contracts/src/arbitration/dispute-kits/DisputeKitClassicBase.sol +++ b/contracts/src/arbitration/dispute-kits/DisputeKitClassicBase.sol @@ -24,7 +24,6 @@ abstract contract DisputeKitClassicBase is IDisputeKit, Initializable, UUPSProxi struct Dispute { Round[] rounds; // Rounds of the dispute. 0 is the default round, and [1, ..n] are the appeal rounds. uint256 numberOfChoices; // The number of choices jurors have when voting. This does not include choice `0` which is reserved for "refuse to arbitrate". - bool jumped; // True if dispute jumped to a parent dispute kit and won't be handled by this DK anymore. mapping(uint256 => uint256) coreRoundIDToLocal; // Maps id of the round in the core contract to the index of the round of related local dispute. bytes extraData; // Extradata for the dispute. uint256[10] __gap; // Reserved slots for future upgrades. @@ -54,6 +53,11 @@ abstract contract DisputeKitClassicBase is IDisputeKit, Initializable, UUPSProxi uint256[10] __gap; // Reserved slots for future upgrades. } + struct Active { + bool dispute; // True if at least one round in the dispute has been active on this Dispute Kit. False if the dispute is unknown to this Dispute Kit. + bool currentRound; // True if the dispute's current round is active on this Dispute Kit. False if the dispute has jumped to another Dispute Kit. + } + // ************************************* // // * Storage * // // ************************************* // @@ -67,7 +71,7 @@ abstract contract DisputeKitClassicBase is IDisputeKit, Initializable, UUPSProxi Dispute[] public disputes; // Array of the locally created disputes. mapping(uint256 => uint256) public coreDisputeIDToLocal; // Maps the dispute ID in Kleros Core to the local dispute ID. bool public singleDrawPerJuror; // Whether each juror can only draw once per dispute, false by default. - mapping(uint256 coreDisputeID => bool) public coreDisputeIDToActive; // True if this dispute kit is active for this core dispute ID. + mapping(uint256 coreDisputeID => Active) public coreDisputeIDToActive; // Active status of the dispute and the current round. address public wNative; // The wrapped native token for safeSend(). uint256 public jumpDisputeKitID; // The ID of the dispute kit in Kleros Core disputeKits array that the dispute should switch to after the court jump, in case the new court doesn't support this dispute kit. @@ -106,17 +110,10 @@ abstract contract DisputeKitClassicBase is IDisputeKit, Initializable, UUPSProxi /// @notice To be emitted when the contributed funds are withdrawn. /// @param _coreDisputeID The identifier of the dispute in the Arbitrator contract. - /// @param _coreRoundID The identifier of the round in the Arbitrator contract. /// @param _choice The choice that is being funded. /// @param _contributor The address of the contributor. /// @param _amount The amount withdrawn. - event Withdrawal( - uint256 indexed _coreDisputeID, - uint256 indexed _coreRoundID, - uint256 _choice, - address indexed _contributor, - uint256 _amount - ); + event Withdrawal(uint256 indexed _coreDisputeID, uint256 _choice, address indexed _contributor, uint256 _amount); /// @notice To be emitted when a choice is fully funded for an appeal. /// @param _coreDisputeID The identifier of the dispute in the Arbitrator contract. @@ -138,8 +135,9 @@ abstract contract DisputeKitClassicBase is IDisputeKit, Initializable, UUPSProxi _; } - modifier notJumped(uint256 _coreDisputeID) { - if (disputes[coreDisputeIDToLocal[_coreDisputeID]].jumped) revert DisputeJumpedToParentDK(); + modifier isActive(uint256 _coreDisputeID) { + if (!coreDisputeIDToActive[_coreDisputeID].dispute) revert DisputeUnknownInThisDisputeKit(); + if (!coreDisputeIDToActive[_coreDisputeID].currentRound) revert DisputeJumpedToAnotherDisputeKit(); _; } @@ -208,25 +206,26 @@ abstract contract DisputeKitClassicBase is IDisputeKit, Initializable, UUPSProxi ) external override onlyByCore { uint256 localDisputeID; Dispute storage dispute; - // Check if this dk wasn't already active before. Can happen if DK1 jumps to DK2 and then back to DK1. - if (coreDisputeIDToActive[_coreDisputeID]) { + Active storage active = coreDisputeIDToActive[_coreDisputeID]; + if (active.dispute) { + // The dispute has already been created in this DK in a previous round. E.g. if DK1 jumps to DK2 and then back to DK1. localDisputeID = coreDisputeIDToLocal[_coreDisputeID]; dispute = disputes[localDisputeID]; - dispute.jumped = false; } else { + // The dispute has not been created in this DK yet. localDisputeID = disputes.length; dispute = disputes.push(); coreDisputeIDToLocal[_coreDisputeID] = localDisputeID; - coreDisputeIDToActive[_coreDisputeID] = true; } + active.dispute = true; + active.currentRound = true; dispute.numberOfChoices = _numberOfChoices; dispute.extraData = _extraData; - // New round in the Core should be created before the dispute creation in DK. - dispute.coreRoundIDToLocal[core.getNumberOfRounds(_coreDisputeID) - 1] = dispute.rounds.length; - Round storage round = dispute.rounds.push(); - round.tied = true; + // KlerosCore.Round must have been already created. + dispute.coreRoundIDToLocal[core.getNumberOfRounds(_coreDisputeID) - 1] = dispute.rounds.length; + dispute.rounds.push().tied = true; emit DisputeCreation(_coreDisputeID, _numberOfChoices, _extraData); } @@ -235,7 +234,7 @@ abstract contract DisputeKitClassicBase is IDisputeKit, Initializable, UUPSProxi function draw( uint256 _coreDisputeID, uint256 _nonce - ) external override onlyByCore notJumped(_coreDisputeID) returns (address drawnAddress, uint96 fromSubcourtID) { + ) external override onlyByCore isActive(_coreDisputeID) returns (address drawnAddress, uint96 fromSubcourtID) { uint256 localDisputeID = coreDisputeIDToLocal[_coreDisputeID]; Dispute storage dispute = disputes[localDisputeID]; uint256 localRoundID = dispute.rounds.length - 1; @@ -274,11 +273,10 @@ abstract contract DisputeKitClassicBase is IDisputeKit, Initializable, UUPSProxi uint256 _coreDisputeID, uint256[] calldata _voteIDs, bytes32 _commit - ) internal notJumped(_coreDisputeID) { + ) internal isActive(_coreDisputeID) { (, , KlerosCore.Period period, , ) = core.disputes(_coreDisputeID); if (period != KlerosCore.Period.commit) revert NotCommitPeriod(); if (_commit == bytes32(0)) revert EmptyCommit(); - if (!coreDisputeIDToActive[_coreDisputeID]) revert NotActiveForCoreDisputeID(); Dispute storage dispute = disputes[coreDisputeIDToLocal[_coreDisputeID]]; Round storage round = dispute.rounds[dispute.rounds.length - 1]; @@ -321,11 +319,10 @@ abstract contract DisputeKitClassicBase is IDisputeKit, Initializable, UUPSProxi uint256 _salt, string memory _justification, address _juror - ) internal notJumped(_coreDisputeID) { + ) internal isActive(_coreDisputeID) { (, , KlerosCore.Period period, , ) = core.disputes(_coreDisputeID); if (period != KlerosCore.Period.vote) revert NotVotePeriod(); if (_voteIDs.length == 0) revert EmptyVoteIDs(); - if (!coreDisputeIDToActive[_coreDisputeID]) revert NotActiveForCoreDisputeID(); uint256 localDisputeID = coreDisputeIDToLocal[_coreDisputeID]; Dispute storage dispute = disputes[localDisputeID]; @@ -372,10 +369,9 @@ abstract contract DisputeKitClassicBase is IDisputeKit, Initializable, UUPSProxi /// Note that the surplus deposit will be reimbursed. /// @param _coreDisputeID Index of the dispute in Kleros Core. /// @param _choice A choice that receives funding. - function fundAppeal(uint256 _coreDisputeID, uint256 _choice) external payable notJumped(_coreDisputeID) { + function fundAppeal(uint256 _coreDisputeID, uint256 _choice) external payable isActive(_coreDisputeID) { Dispute storage dispute = disputes[coreDisputeIDToLocal[_coreDisputeID]]; if (_choice > dispute.numberOfChoices) revert ChoiceOutOfBounds(); - if (!coreDisputeIDToActive[_coreDisputeID]) revert NotActiveForCoreDisputeID(); (uint256 appealPeriodStart, uint256 appealPeriodEnd) = core.appealPeriod(_coreDisputeID); if (block.timestamp < appealPeriodStart || block.timestamp >= appealPeriodEnd) revert NotAppealPeriod(); @@ -425,7 +421,7 @@ abstract contract DisputeKitClassicBase is IDisputeKit, Initializable, UUPSProxi if (core.isDisputeKitJumping(_coreDisputeID)) { // Don't create a new round in case of a jump, and remove local dispute from the flow. - dispute.jumped = true; + coreDisputeIDToActive[_coreDisputeID].currentRound = false; } else { // Don't subtract 1 from length since both round arrays haven't been updated yet. dispute.coreRoundIDToLocal[coreRoundID + 1] = dispute.rounds.length; @@ -441,48 +437,50 @@ abstract contract DisputeKitClassicBase is IDisputeKit, Initializable, UUPSProxi /// @notice Allows those contributors who attempted to fund an appeal round to withdraw any reimbursable fees or rewards after the dispute gets resolved. /// @dev Withdrawals are not possible if the core contract is paused. + /// @dev It can be called after the dispute has jumped to another dispute kit. /// @param _coreDisputeID Index of the dispute in Kleros Core contract. /// @param _beneficiary The address whose rewards to withdraw. - /// @param _coreRoundID The round in the Kleros Core contract the caller wants to withdraw from. /// @param _choice The ruling option that the caller wants to withdraw from. /// @return amount The withdrawn amount. function withdrawFeesAndRewards( uint256 _coreDisputeID, address payable _beneficiary, - uint256 _coreRoundID, uint256 _choice ) external returns (uint256 amount) { (, , , bool isRuled, ) = core.disputes(_coreDisputeID); if (!isRuled) revert DisputeNotResolved(); if (core.paused()) revert CoreIsPaused(); - if (!coreDisputeIDToActive[_coreDisputeID]) revert NotActiveForCoreDisputeID(); + if (!coreDisputeIDToActive[_coreDisputeID].dispute) revert DisputeUnknownInThisDisputeKit(); Dispute storage dispute = disputes[coreDisputeIDToLocal[_coreDisputeID]]; - Round storage round = dispute.rounds[dispute.coreRoundIDToLocal[_coreRoundID]]; (uint256 finalRuling, , ) = core.currentRuling(_coreDisputeID); - if (!round.hasPaid[_choice]) { - // Allow to reimburse if funding was unsuccessful for this ruling option. - amount = round.contributions[_beneficiary][_choice]; - } else { - // Funding was successful for this ruling option. - if (_choice == finalRuling) { - // This ruling option is the ultimate winner. - amount = round.paidFees[_choice] > 0 - ? (round.contributions[_beneficiary][_choice] * round.feeRewards) / round.paidFees[_choice] - : 0; - } else if (!round.hasPaid[finalRuling]) { - // The ultimate winner was not funded in this round. In this case funded ruling option(s) are reimbursed. - amount = - (round.contributions[_beneficiary][_choice] * round.feeRewards) / - (round.paidFees[round.fundedChoices[0]] + round.paidFees[round.fundedChoices[1]]); + for (uint256 i = 0; i < dispute.rounds.length; i++) { + Round storage round = dispute.rounds[i]; + + if (!round.hasPaid[_choice]) { + // Allow to reimburse if funding was unsuccessful for this ruling option. + amount += round.contributions[_beneficiary][_choice]; + } else { + // Funding was successful for this ruling option. + if (_choice == finalRuling) { + // This ruling option is the ultimate winner. + amount += round.paidFees[_choice] > 0 + ? (round.contributions[_beneficiary][_choice] * round.feeRewards) / round.paidFees[_choice] + : 0; + } else if (!round.hasPaid[finalRuling]) { + // The ultimate winner was not funded in this round. In this case funded ruling option(s) are reimbursed. + amount += + (round.contributions[_beneficiary][_choice] * round.feeRewards) / + (round.paidFees[round.fundedChoices[0]] + round.paidFees[round.fundedChoices[1]]); + } } + round.contributions[_beneficiary][_choice] = 0; } - round.contributions[_beneficiary][_choice] = 0; if (amount != 0) { _beneficiary.safeSend(amount, wNative); - emit Withdrawal(_coreDisputeID, _coreRoundID, _choice, _beneficiary, amount); + emit Withdrawal(_coreDisputeID, _choice, _beneficiary, amount); } } @@ -756,11 +754,11 @@ abstract contract DisputeKitClassicBase is IDisputeKit, Initializable, UUPSProxi error OwnerOnly(); error KlerosCoreOnly(); - error DisputeJumpedToParentDK(); + error DisputeJumpedToAnotherDisputeKit(); + error DisputeUnknownInThisDisputeKit(); error UnsuccessfulCall(); error NotCommitPeriod(); error EmptyCommit(); - error NotActiveForCoreDisputeID(); error JurorHasToOwnTheVote(); error NotVotePeriod(); error EmptyVoteIDs(); diff --git a/contracts/src/arbitration/dispute-kits/DisputeKitGatedShutter.sol b/contracts/src/arbitration/dispute-kits/DisputeKitGatedShutter.sol index dbcde618c..fe53dff92 100644 --- a/contracts/src/arbitration/dispute-kits/DisputeKitGatedShutter.sol +++ b/contracts/src/arbitration/dispute-kits/DisputeKitGatedShutter.sol @@ -118,7 +118,7 @@ contract DisputeKitGatedShutter is DisputeKitClassicBase { bytes32 _recoveryCommit, bytes32 _identity, bytes calldata _encryptedVote - ) external notJumped(_coreDisputeID) { + ) external isActive(_coreDisputeID) { if (_recoveryCommit == bytes32(0)) revert EmptyRecoveryCommit(); uint256 localDisputeID = coreDisputeIDToLocal[_coreDisputeID]; diff --git a/contracts/src/arbitration/dispute-kits/DisputeKitShutter.sol b/contracts/src/arbitration/dispute-kits/DisputeKitShutter.sol index 0ea8bdb41..47ff78166 100644 --- a/contracts/src/arbitration/dispute-kits/DisputeKitShutter.sol +++ b/contracts/src/arbitration/dispute-kits/DisputeKitShutter.sol @@ -102,7 +102,7 @@ contract DisputeKitShutter is DisputeKitClassicBase { bytes32 _recoveryCommit, bytes32 _identity, bytes calldata _encryptedVote - ) external notJumped(_coreDisputeID) { + ) external isActive(_coreDisputeID) { if (_recoveryCommit == bytes32(0)) revert EmptyRecoveryCommit(); uint256 localDisputeID = coreDisputeIDToLocal[_coreDisputeID]; diff --git a/contracts/src/arbitration/interfaces/IDisputeKit.sol b/contracts/src/arbitration/interfaces/IDisputeKit.sol index 20e42a0d2..88467d83f 100644 --- a/contracts/src/arbitration/interfaces/IDisputeKit.sol +++ b/contracts/src/arbitration/interfaces/IDisputeKit.sol @@ -32,6 +32,7 @@ interface IDisputeKit { /// @notice Creates a local dispute and maps it to the dispute ID in the Core contract. /// @dev Access restricted to Kleros Core only. + /// @dev The new `KlerosCore.Round` must be created before calling this function. /// @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. diff --git a/contracts/test/foundry/KlerosCore_Appeals.t.sol b/contracts/test/foundry/KlerosCore_Appeals.t.sol index 295128d37..c14ccfb43 100644 --- a/contracts/test/foundry/KlerosCore_Appeals.t.sol +++ b/contracts/test/foundry/KlerosCore_Appeals.t.sol @@ -298,8 +298,8 @@ contract KlerosCore_AppealsTest is KlerosCore_TestBase { vm.prank(crowdfunder2); newDisputeKit.fundAppeal{value: 0.42 ether}(disputeID, 2); - (, bool jumped, ) = newDisputeKit.disputes(disputeID); - assertEq(jumped, true, "jumped should be true"); + (, bool currentRound) = newDisputeKit.coreDisputeIDToActive(disputeID); + assertEq(currentRound, false, "round should be jumped"); assertEq( (newDisputeKit.getFundedChoices(disputeID)).length, 2, @@ -312,12 +312,12 @@ contract KlerosCore_AppealsTest is KlerosCore_TestBase { (uint96 courtID, , , , ) = core.disputes(disputeID); assertEq(courtID, GENERAL_COURT, "Wrong court ID"); - (, jumped, ) = disputeKit.disputes(disputeID); - assertEq(jumped, false, "jumped should be false in the DK that dispute jumped to"); + (, currentRound) = disputeKit.coreDisputeIDToActive(disputeID); + assertEq(currentRound, true, "round should be active in the DK that dispute jumped to"); // Check jump modifier vm.prank(address(core)); - vm.expectRevert(DisputeKitClassicBase.DisputeJumpedToParentDK.selector); + vm.expectRevert(DisputeKitClassicBase.DisputeJumpedToAnotherDisputeKit.selector); newDisputeKit.draw(disputeID, 1); // And check that draw in the new round works @@ -436,8 +436,8 @@ contract KlerosCore_AppealsTest is KlerosCore_TestBase { vm.prank(crowdfunder2); disputeKit3.fundAppeal{value: 0.42 ether}(disputeID, 2); - (, bool jumped, ) = disputeKit3.disputes(disputeID); - assertEq(jumped, true, "jumped should be true"); + (, bool currentRound) = disputeKit3.coreDisputeIDToActive(disputeID); + assertEq(currentRound, false, "round should be jumped"); assertEq( (disputeKit3.getFundedChoices(disputeID)).length, 2, @@ -450,12 +450,12 @@ contract KlerosCore_AppealsTest is KlerosCore_TestBase { (uint96 courtID, , , , ) = core.disputes(disputeID); assertEq(courtID, GENERAL_COURT, "Wrong court ID"); - (, jumped, ) = disputeKit2.disputes(disputeID); - assertEq(jumped, false, "jumped should be false in the DK that dispute jumped to"); + (, currentRound) = disputeKit2.coreDisputeIDToActive(disputeID); + assertEq(currentRound, true, "round should be active in the DK that dispute jumped to"); // Check jump modifier vm.prank(address(core)); - vm.expectRevert(DisputeKitClassicBase.DisputeJumpedToParentDK.selector); + vm.expectRevert(DisputeKitClassicBase.DisputeJumpedToAnotherDisputeKit.selector); disputeKit3.draw(disputeID, 1); // And check that draw in the new round works @@ -578,14 +578,14 @@ contract KlerosCore_AppealsTest is KlerosCore_TestBase { KlerosCore.Round memory round = core.getRoundInfo(disputeID, 0); assertEq(round.disputeKitID, dkID3, "Wrong DK ID"); - assertEq(disputeKit3.coreDisputeIDToActive(disputeID), true, "Should be true for dk3"); assertEq(disputeKit3.coreDisputeIDToLocal(disputeID), 0, "Wrong local dispute ID to core dispute ID"); assertEq(disputeKit3.getNumberOfRounds(0), 1, "Wrong number of rounds dk3"); // local dispute id (, uint256 localRoundID) = disputeKit3.getLocalDisputeRoundID(disputeID, 0); assertEq(localRoundID, 0, "Wrong local round ID dk3"); - (, bool jumped, ) = disputeKit3.disputes(0); - assertEq(jumped, false, "jumped should be false in dk3"); + (bool disputeActive, bool currentRound) = disputeKit3.coreDisputeIDToActive(0); + assertEq(disputeActive, true, "dispute should be active for dk3"); + assertEq(currentRound, true, "round should be active in dk3"); core.draw(disputeID, DEFAULT_NB_OF_JURORS); vm.warp(block.timestamp + timesPerPeriod[0]); @@ -621,15 +621,14 @@ contract KlerosCore_AppealsTest is KlerosCore_TestBase { // Round2 // - (, jumped, ) = disputeKit3.disputes(0); - assertEq(jumped, true, "jumped should be true in dk3"); + (disputeActive, currentRound) = disputeKit3.coreDisputeIDToActive(0); + assertEq(disputeActive, true, "dispute should still be active for dk3"); + assertEq(currentRound, false, "round should be jumped in dk3"); assertEq( (disputeKit3.getFundedChoices(disputeID)).length, 2, "No fresh round created so the number of funded choices should be 2" ); - - assertEq(disputeKit3.coreDisputeIDToActive(disputeID), true, "Should still be true for dk3"); assertEq(disputeKit3.coreDisputeIDToLocal(disputeID), 0, "core to local ID should not change for dk3"); assertEq(disputeKit3.getNumberOfRounds(0), 1, "Wrong number of rounds dk3"); // local dispute id (, localRoundID) = disputeKit3.getLocalDisputeRoundID(disputeID, 0); @@ -641,17 +640,16 @@ contract KlerosCore_AppealsTest is KlerosCore_TestBase { (uint96 courtID, , , , ) = core.disputes(disputeID); assertEq(courtID, courtID2, "Wrong court ID after jump"); - (, jumped, ) = disputeKit2.disputes(0); - assertEq(jumped, false, "jumped should be false in the DK that dispute jumped to"); - - assertEq(disputeKit2.coreDisputeIDToActive(disputeID), true, "Should be true for dk2"); + (disputeActive, currentRound) = disputeKit2.coreDisputeIDToActive(0); + assertEq(disputeActive, true, "dispute should be active for dk2"); + assertEq(currentRound, true, "round should be active in the DK that dispute jumped to"); assertEq(disputeKit2.coreDisputeIDToLocal(disputeID), 0, "Wrong local dispute ID to core dispute ID dk2"); assertEq(disputeKit2.getNumberOfRounds(0), 1, "Wrong number of rounds dk2"); // local dispute id (, localRoundID) = disputeKit2.getLocalDisputeRoundID(disputeID, 1); assertEq(localRoundID, 0, "Wrong local round ID for dk2"); vm.prank(address(core)); - vm.expectRevert(DisputeKitClassicBase.DisputeJumpedToParentDK.selector); + vm.expectRevert(DisputeKitClassicBase.DisputeJumpedToAnotherDisputeKit.selector); disputeKit3.draw(disputeID, 1); core.draw(disputeID, 7); // New round requires 7 jurors @@ -664,7 +662,7 @@ contract KlerosCore_AppealsTest is KlerosCore_TestBase { } vm.prank(staker1); - vm.expectRevert(DisputeKitClassicBase.DisputeJumpedToParentDK.selector); + vm.expectRevert(DisputeKitClassicBase.DisputeJumpedToAnotherDisputeKit.selector); disputeKit3.castVote(disputeID, voteIDs, 2, 0, "XYZ"); vm.prank(staker1); @@ -673,7 +671,7 @@ contract KlerosCore_AppealsTest is KlerosCore_TestBase { core.passPeriod(disputeID); // Appeal vm.prank(crowdfunder1); - vm.expectRevert(DisputeKitClassicBase.DisputeJumpedToParentDK.selector); + vm.expectRevert(DisputeKitClassicBase.DisputeJumpedToAnotherDisputeKit.selector); disputeKit3.fundAppeal{value: 1.35 ether}(disputeID, 1); assertEq(core.isDisputeKitJumping(disputeID), true, "Should be jumping"); @@ -691,8 +689,9 @@ contract KlerosCore_AppealsTest is KlerosCore_TestBase { // Round3 // - (, jumped, ) = disputeKit2.disputes(0); - assertEq(jumped, true, "jumped should be true in dk2"); + (disputeActive, currentRound) = disputeKit2.coreDisputeIDToActive(0); + assertEq(disputeActive, true, "dispute should still be active for dk2"); + assertEq(currentRound, false, "round should be jumped in dk2"); assertEq( (disputeKit2.getFundedChoices(disputeID)).length, 2, @@ -703,8 +702,6 @@ contract KlerosCore_AppealsTest is KlerosCore_TestBase { 0, "Should be 0 funded choices in dk3 because fresh round" ); - - assertEq(disputeKit3.coreDisputeIDToActive(disputeID), true, "Should be true for dk3 round3"); assertEq(disputeKit3.coreDisputeIDToLocal(disputeID), 0, "core to local ID should stay the same for dk3"); assertEq(disputeKit3.getNumberOfRounds(0), 2, "Wrong number of rounds dk3 round3"); // local dispute id (, localRoundID) = disputeKit3.getLocalDisputeRoundID(disputeID, 2); @@ -716,10 +713,10 @@ contract KlerosCore_AppealsTest is KlerosCore_TestBase { (courtID, , , , ) = core.disputes(disputeID); assertEq(courtID, GENERAL_COURT, "Wrong court ID after jump"); - (, jumped, ) = disputeKit3.disputes(0); // local dispute id - assertEq(jumped, false, "jumped should be false in the DK that dispute jumped to"); + (disputeActive, currentRound) = disputeKit3.coreDisputeIDToActive(0); // local dispute id + assertEq(disputeActive, true, "dispute should still be active for dk3"); + assertEq(currentRound, true, "round should be active in the DK that dispute jumped to"); - assertEq(disputeKit2.coreDisputeIDToActive(disputeID), true, "Should be true for dk2 round3"); assertEq( disputeKit2.coreDisputeIDToLocal(disputeID), 0, @@ -730,7 +727,7 @@ contract KlerosCore_AppealsTest is KlerosCore_TestBase { assertEq(localRoundID, 0, "Wrong local round ID for dk2 round3"); vm.prank(address(core)); - vm.expectRevert(DisputeKitClassicBase.DisputeJumpedToParentDK.selector); + vm.expectRevert(DisputeKitClassicBase.DisputeJumpedToAnotherDisputeKit.selector); disputeKit2.draw(disputeID, 1); core.draw(disputeID, 15); // New round requires 15 jurors @@ -743,7 +740,7 @@ contract KlerosCore_AppealsTest is KlerosCore_TestBase { } vm.prank(staker1); - vm.expectRevert(DisputeKitClassicBase.DisputeJumpedToParentDK.selector); + vm.expectRevert(DisputeKitClassicBase.DisputeJumpedToAnotherDisputeKit.selector); disputeKit2.castVote(disputeID, voteIDs, 2, 0, "XYZ"); vm.prank(staker1); @@ -757,39 +754,24 @@ contract KlerosCore_AppealsTest is KlerosCore_TestBase { core.executeRuling(disputeID); // winning choice is 2 - // Appeal Rewards for Round 1 // - disputeKit3.withdrawFeesAndRewards(disputeID, payable(crowdfunder1), 0, 1); // wrong side, no reward + // Appeal Rewards // + disputeKit3.withdrawFeesAndRewards(disputeID, payable(crowdfunder1), 1); // wrong side, no reward + vm.expectEmit(); - emit DisputeKitClassicBase.Withdrawal(disputeID, 0, 2, payable(crowdfunder2), 0.84 ether); - disputeKit3.withdrawFeesAndRewards(disputeID, payable(crowdfunder2), 0, 2); // REWARDS - disputeKit2.withdrawFeesAndRewards(disputeID, payable(crowdfunder1), 0, 1); // wrong DK, no reward - disputeKit2.withdrawFeesAndRewards(disputeID, payable(crowdfunder2), 0, 2); // wrong DK, no reward - vm.expectRevert(DisputeKitClassicBase.NotActiveForCoreDisputeID.selector); - disputeKit.withdrawFeesAndRewards(disputeID, payable(crowdfunder1), 0, 1); // wrong DK, no reward - vm.expectRevert(DisputeKitClassicBase.NotActiveForCoreDisputeID.selector); - disputeKit.withdrawFeesAndRewards(disputeID, payable(crowdfunder2), 0, 2); // wrong DK, no reward - - // Appeal Rewards for Round 2 // - disputeKit3.withdrawFeesAndRewards(disputeID, payable(crowdfunder1), 1, 1); // wrong DK, no reward - disputeKit3.withdrawFeesAndRewards(disputeID, payable(crowdfunder2), 1, 2); // wrong DK, no reward - disputeKit2.withdrawFeesAndRewards(disputeID, payable(crowdfunder1), 1, 1); // wrong side, no reward + emit DisputeKitClassicBase.Withdrawal(disputeID, 2, payable(crowdfunder2), 0.84 ether); + disputeKit3.withdrawFeesAndRewards(disputeID, payable(crowdfunder2), 2); // REWARDS + + disputeKit2.withdrawFeesAndRewards(disputeID, payable(crowdfunder1), 1); // wrong DK, no reward + vm.expectEmit(); - emit DisputeKitClassicBase.Withdrawal(disputeID, 1, 2, payable(crowdfunder2), 1.8 ether); - disputeKit2.withdrawFeesAndRewards(disputeID, payable(crowdfunder2), 1, 2); // REWARDS - vm.expectRevert(DisputeKitClassicBase.NotActiveForCoreDisputeID.selector); - disputeKit.withdrawFeesAndRewards(disputeID, payable(crowdfunder1), 1, 1); // wrong DK, no reward - vm.expectRevert(DisputeKitClassicBase.NotActiveForCoreDisputeID.selector); - disputeKit.withdrawFeesAndRewards(disputeID, payable(crowdfunder2), 1, 2); // wrong DK, no reward - - // Appeal Rewards for Round 3 // - disputeKit3.withdrawFeesAndRewards(disputeID, payable(crowdfunder1), 2, 1); // no appeal, no reward - disputeKit3.withdrawFeesAndRewards(disputeID, payable(crowdfunder2), 2, 2); // no appeal, no reward - disputeKit2.withdrawFeesAndRewards(disputeID, payable(crowdfunder1), 2, 1); // no appeal, no reward - disputeKit2.withdrawFeesAndRewards(disputeID, payable(crowdfunder2), 2, 2); // no appeal, no reward - vm.expectRevert(DisputeKitClassicBase.NotActiveForCoreDisputeID.selector); - disputeKit.withdrawFeesAndRewards(disputeID, payable(crowdfunder1), 2, 1); // wrong DK, no reward - vm.expectRevert(DisputeKitClassicBase.NotActiveForCoreDisputeID.selector); - disputeKit.withdrawFeesAndRewards(disputeID, payable(crowdfunder2), 2, 2); // wrong DK, no reward + emit DisputeKitClassicBase.Withdrawal(disputeID, 2, payable(crowdfunder2), 1.8 ether); + disputeKit2.withdrawFeesAndRewards(disputeID, payable(crowdfunder2), 2); // REWARDS + + vm.expectRevert(DisputeKitClassicBase.DisputeUnknownInThisDisputeKit.selector); + disputeKit.withdrawFeesAndRewards(disputeID, payable(crowdfunder1), 1); // wrong DK, no reward + + vm.expectRevert(DisputeKitClassicBase.DisputeUnknownInThisDisputeKit.selector); + disputeKit.withdrawFeesAndRewards(disputeID, payable(crowdfunder2), 2); // wrong DK, no reward } function test_appeal_quickPassPeriod() public { @@ -841,7 +823,7 @@ contract KlerosCore_AppealsTest is KlerosCore_TestBase { vm.prank(disputer); arbitrable.createDispute{value: feeForJuror * DEFAULT_NB_OF_JURORS}("Action"); - (uint256 numberOfChoices, , ) = disputeKit.disputes(disputeID); + (uint256 numberOfChoices, ) = disputeKit.disputes(disputeID); assertEq(numberOfChoices, numberOfOptions, "Wrong numberOfChoices"); diff --git a/contracts/test/foundry/KlerosCore_Disputes.t.sol b/contracts/test/foundry/KlerosCore_Disputes.t.sol index 2d029ddcf..93c4c0757 100644 --- a/contracts/test/foundry/KlerosCore_Disputes.t.sol +++ b/contracts/test/foundry/KlerosCore_Disputes.t.sol @@ -88,13 +88,15 @@ contract KlerosCore_DisputesTest is KlerosCore_TestBase { assertEq(address(round.feeToken), address(0), "feeToken should be 0"); assertEq(round.drawIterations, 0, "drawIterations should be 0"); - (uint256 numberOfChoices, bool jumped, bytes memory extraData) = disputeKit.disputes(disputeID); - + (uint256 numberOfChoices, bytes memory extraData) = disputeKit.disputes(disputeID); assertEq(numberOfChoices, 2, "Wrong numberOfChoices"); - assertEq(jumped, false, "jumped should be false"); assertEq(extraData, newExtraData, "Wrong extra data"); + + (bool dispute, bool currentRound) = disputeKit.coreDisputeIDToActive(0); + assertEq(dispute, true, "Dispute should be active in this DK"); + assertEq(currentRound, true, "Current round should be active in this DK"); + assertEq(disputeKit.coreDisputeIDToLocal(0), disputeID, "Wrong local disputeID"); - assertEq(disputeKit.coreDisputeIDToActive(0), true, "Dispute should be active in this DK"); ( uint256 winningChoice, diff --git a/contracts/test/foundry/KlerosCore_Execution.t.sol b/contracts/test/foundry/KlerosCore_Execution.t.sol index bbce3234a..81dd723a0 100644 --- a/contracts/test/foundry/KlerosCore_Execution.t.sol +++ b/contracts/test/foundry/KlerosCore_Execution.t.sol @@ -721,14 +721,14 @@ contract KlerosCore_ExecutionTest is KlerosCore_TestBase { core.passPeriod(disputeID); // Execution vm.expectRevert(DisputeKitClassicBase.DisputeNotResolved.selector); - disputeKit.withdrawFeesAndRewards(disputeID, payable(staker1), 0, 1); + disputeKit.withdrawFeesAndRewards(disputeID, payable(staker1), 1); core.executeRuling(disputeID); vm.prank(owner); core.pause(); vm.expectRevert(DisputeKitClassicBase.CoreIsPaused.selector); - disputeKit.withdrawFeesAndRewards(disputeID, payable(staker1), 0, 1); + disputeKit.withdrawFeesAndRewards(disputeID, payable(staker1), 1); vm.prank(owner); core.unpause(); @@ -737,12 +737,12 @@ contract KlerosCore_ExecutionTest is KlerosCore_TestBase { assertEq(address(disputeKit).balance, 1.04 ether, "Wrong balance of the DK"); vm.expectEmit(true, true, true, true); - emit DisputeKitClassicBase.Withdrawal(disputeID, 0, 1, crowdfunder1, 0.63 ether); - disputeKit.withdrawFeesAndRewards(disputeID, payable(crowdfunder1), 0, 1); + emit DisputeKitClassicBase.Withdrawal(disputeID, 1, crowdfunder1, 0.63 ether); + disputeKit.withdrawFeesAndRewards(disputeID, payable(crowdfunder1), 1); vm.expectEmit(true, true, true, true); - emit DisputeKitClassicBase.Withdrawal(disputeID, 0, 2, crowdfunder2, 0.41 ether); - disputeKit.withdrawFeesAndRewards(disputeID, payable(crowdfunder2), 0, 2); + emit DisputeKitClassicBase.Withdrawal(disputeID, 2, crowdfunder2, 0.41 ether); + disputeKit.withdrawFeesAndRewards(disputeID, payable(crowdfunder2), 2); assertEq(crowdfunder1.balance, 10 ether, "Wrong balance of the crowdfunder1"); assertEq(crowdfunder2.balance, 10 ether, "Wrong balance of the crowdfunder2"); diff --git a/contracts/test/foundry/KlerosCore_Voting.t.sol b/contracts/test/foundry/KlerosCore_Voting.t.sol index 961d0741d..689df3a7d 100644 --- a/contracts/test/foundry/KlerosCore_Voting.t.sol +++ b/contracts/test/foundry/KlerosCore_Voting.t.sol @@ -456,13 +456,14 @@ contract KlerosCore_VotingTest is KlerosCore_TestBase { core.passPeriod(disputeID); // Vote // Check that the new DK has the info but not the old one. - - assertEq(disputeKit.coreDisputeIDToActive(disputeID), false, "Should be false for old DK"); + (bool disputeActive, ) = disputeKit.coreDisputeIDToActive(disputeID); + assertEq(disputeActive, false, "Should be false for old DK"); // This is the DK where dispute was created. Core dispute points to index 1 because new DK has two disputes. + (disputeActive, ) = newDisputeKit.coreDisputeIDToActive(disputeID); + assertEq(disputeActive, true, "Should be active for new DK"); assertEq(newDisputeKit.coreDisputeIDToLocal(disputeID), 1, "Wrong local dispute ID for new DK"); - assertEq(newDisputeKit.coreDisputeIDToActive(disputeID), true, "Should be active for new DK"); - (uint256 numberOfChoices, , bytes memory extraData) = newDisputeKit.disputes(1); + (uint256 numberOfChoices, bytes memory extraData) = newDisputeKit.disputes(1); assertEq(numberOfChoices, 2, "Wrong numberOfChoices in new DK"); assertEq(extraData, newExtraData, "Wrong extra data"); @@ -473,7 +474,7 @@ contract KlerosCore_VotingTest is KlerosCore_TestBase { // Deliberately cast votes using the old DK to see if the exception will be caught. vm.prank(staker1); - vm.expectRevert(DisputeKitClassicBase.NotActiveForCoreDisputeID.selector); + vm.expectRevert(DisputeKitClassicBase.DisputeUnknownInThisDisputeKit.selector); disputeKit.castVote(disputeID, voteIDs, 2, 0, "XYZ"); // And check the new DK. From f9fd9445b8cf2ff7b5f547d8751f2a6bfb6b5f0c Mon Sep 17 00:00:00 2001 From: jaybuidl Date: Tue, 14 Oct 2025 15:38:50 +0100 Subject: [PATCH 2/2] fix: removed unnecessary isActive modifier on the shutter DKs --- .../src/arbitration/dispute-kits/DisputeKitGatedShutter.sol | 4 ++-- contracts/src/arbitration/dispute-kits/DisputeKitShutter.sol | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/contracts/src/arbitration/dispute-kits/DisputeKitGatedShutter.sol b/contracts/src/arbitration/dispute-kits/DisputeKitGatedShutter.sol index fe53dff92..a4ec2f8dc 100644 --- a/contracts/src/arbitration/dispute-kits/DisputeKitGatedShutter.sol +++ b/contracts/src/arbitration/dispute-kits/DisputeKitGatedShutter.sol @@ -118,7 +118,7 @@ contract DisputeKitGatedShutter is DisputeKitClassicBase { bytes32 _recoveryCommit, bytes32 _identity, bytes calldata _encryptedVote - ) external isActive(_coreDisputeID) { + ) external { if (_recoveryCommit == bytes32(0)) revert EmptyRecoveryCommit(); uint256 localDisputeID = coreDisputeIDToLocal[_coreDisputeID]; @@ -128,7 +128,7 @@ contract DisputeKitGatedShutter is DisputeKitClassicBase { recoveryCommitments[localDisputeID][localRoundID][_voteIDs[i]] = _recoveryCommit; } - // `_castCommit()` ensures that the caller owns the vote + // `_castCommit()` ensures that the caller owns the vote and that dispute is active _castCommit(_coreDisputeID, _voteIDs, _commit); emit CommitCastShutter(_coreDisputeID, msg.sender, _commit, _recoveryCommit, _identity, _encryptedVote); } diff --git a/contracts/src/arbitration/dispute-kits/DisputeKitShutter.sol b/contracts/src/arbitration/dispute-kits/DisputeKitShutter.sol index 47ff78166..fe2c0eb7b 100644 --- a/contracts/src/arbitration/dispute-kits/DisputeKitShutter.sol +++ b/contracts/src/arbitration/dispute-kits/DisputeKitShutter.sol @@ -102,7 +102,7 @@ contract DisputeKitShutter is DisputeKitClassicBase { bytes32 _recoveryCommit, bytes32 _identity, bytes calldata _encryptedVote - ) external isActive(_coreDisputeID) { + ) external { if (_recoveryCommit == bytes32(0)) revert EmptyRecoveryCommit(); uint256 localDisputeID = coreDisputeIDToLocal[_coreDisputeID]; @@ -112,7 +112,7 @@ contract DisputeKitShutter is DisputeKitClassicBase { recoveryCommitments[localDisputeID][localRoundID][_voteIDs[i]] = _recoveryCommit; } - // `_castCommit()` ensures that the caller owns the vote + // `_castCommit()` ensures that the caller owns the vote and that dispute is active _castCommit(_coreDisputeID, _voteIDs, _commit); emit CommitCastShutter(_coreDisputeID, msg.sender, _commit, _recoveryCommit, _identity, _encryptedVote); }