feat: fuzzing tests #2144
Open
feat: fuzzing tests #2144
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
✅ Deploy Preview for kleros-v2-neo ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
✅ Deploy Preview for kleros-v2-testnet ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
❌ Deploy Preview for kleros-v2-testnet-devtools failed. Why did it fail? →
|
WalkthroughIntroduces a configurable numberOfRulingOptions state in ArbitrableExample with a governance setter, replacing hard-coded values in dispute creation. Adds multiple fuzz tests across appeals, disputes, drawing, execution, staking, and voting to exercise variable ruling options, amounts, iterations, and balances. Minor assertion text tweak. Possible duplicate tests in execution. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Owner
actor User
participant Arbitrable as ArbitrableExample
participant Core as KlerosCore (Arbitrator)
participant Jurors
Owner->>Arbitrable: changeNumberOfRulingOptions(n)
note right of Arbitrable: Stores n in numberOfRulingOptions
User->>Arbitrable: createDispute(...)
Arbitrable->>Core: createDispute(numberOfRulingOptions)
Core-->>Arbitrable: disputeID
Core->>Jurors: Draw and notify
Jurors->>Core: Cast votes (1..n)
User->>Core: (optional) Fund appeal(s)
Core->>Jurors: Appeal rounds (as needed)
Core-->>Arbitrable: Ruling (1..n)
Arbitrable-->>User: Enforce ruling
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches🧪 Generate unit tests
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Please see the documentation for more information. Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (10)
contracts/test/foundry/KlerosCore_Disputes.t.sol (1)
150-193: Bound fuzzed value instead of assuming; reduces discarded runs and flakiness.Use bound to keep runs in-range deterministically.
Apply this diff:
- // Cap it to 10 eth, so the number of jurors is not astronomical. - vm.assume(disputeValue >= arbitrationCost && disputeValue <= 10 ether); - vm.deal(disputer, 10 ether); + // Cap it to 10 eth, so the number of jurors is not astronomical. + disputeValue = bound(disputeValue, arbitrationCost, 10 ether); + vm.deal(disputer, 10 ether);contracts/test/foundry/KlerosCore_Appeals.t.sol (1)
569-628: Prefer bound over assume for appealValue.Keeps runs within limits and reduces discarded cases.
Apply this diff:
- vm.assume(appealValue <= 10 ether); - vm.deal(crowdfunder1, 10 ether); + appealValue = bound(appealValue, 0, 10 ether); + vm.deal(crowdfunder1, 10 ether);contracts/test/foundry/KlerosCore_Staking.t.sol (4)
10-10: Remove unused console import.Apply this diff:
-import {console} from "forge-std/console.sol";
446-501: Use bound to constrain stake amounts; avoids discarded fuzz runs.Apply this diff:
- vm.assume(firstStake >= minStake && firstStake <= stakerSupply); - vm.assume(secondStake >= minStake && secondStake <= stakerSupply); + firstStake = bound(firstStake, minStake, stakerSupply); + secondStake = bound(secondStake, minStake, stakerSupply);
502-579: Bound stake amounts for the second court scenario as well.Apply this diff:
- vm.assume(firstStake >= minStake && firstStake <= stakerSupply / 2); // Split the supply into two because courts are different now - vm.assume(secondStake >= minStake && secondStake <= stakerSupply / 2); + firstStake = bound(firstStake, minStake, stakerSupply / 2); // Split the supply into two because courts are different now + secondStake = bound(secondStake, minStake, stakerSupply / 2);
580-620: Constrain iterations to realistic bounds to speed up fuzzing.Only up to 4 delayed stakes exist here.
Apply this diff:
- // Test with large numbers but do not trigger possible overflow - vm.assume(iterations < 2 ** 128); + // Keep within a practical bound; there are only 4 delayed stakes in this test. + iterations = bound(iterations, 0, 8);contracts/test/foundry/KlerosCore_Drawing.t.sol (2)
125-164: Bound iterations to avoid pathological values and OOG risk.Apply this diff:
- core.draw(disputeID, iterations); + iterations = bound(iterations, 0, DEFAULT_NB_OF_JURORS * 8); + core.draw(disputeID, iterations);
165-209: Prefer bound over assume; also constrain iterations relative to jurors.Apply this diff:
- // Cap it to 10 eth, so the number of jurors is not astronomical. - vm.assume(disputeValue >= arbitrationCost && disputeValue <= 10 ether); - vm.deal(disputer, 10 ether); + // Cap it to 10 eth, so the number of jurors is not astronomical. + disputeValue = bound(disputeValue, arbitrationCost, 10 ether); + vm.deal(disputer, 10 ether); @@ - core.draw(disputeID, iterations); + iterations = bound(iterations, 0, (disputeValue / feeForJuror) * 8); + core.draw(disputeID, iterations);contracts/test/foundry/KlerosCore_Execution.t.sol (2)
752-789: Constrain iterations to a sane range for execute().Prevents excessive loop requests and speeds up fuzzing.
Apply this diff:
- core.execute(disputeID, roundID, iterations); + iterations = bound(iterations, 0, DEFAULT_NB_OF_JURORS * 16); + core.execute(disputeID, roundID, iterations);
790-850: Bound disputeValue and iterations; keep iterations relative to juror count.Apply this diff:
- // Cap it to 10 eth, so the number of jurors is not astronomical. - vm.assume(disputeValue >= arbitrationCost && disputeValue <= 10 ether); - vm.deal(disputer, 10 ether); + // Cap it to 10 eth, so the number of jurors is not astronomical. + disputeValue = bound(disputeValue, arbitrationCost, 10 ether); + vm.deal(disputer, 10 ether); @@ - core.execute(disputeID, roundID, iterations); + iterations = bound(iterations, 0, nbJurors * 16); + core.execute(disputeID, roundID, iterations);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
contracts/src/arbitration/arbitrables/ArbitrableExample.sol(2 hunks)contracts/test/foundry/KlerosCore_Appeals.t.sol(1 hunks)contracts/test/foundry/KlerosCore_Disputes.t.sol(2 hunks)contracts/test/foundry/KlerosCore_Drawing.t.sol(1 hunks)contracts/test/foundry/KlerosCore_Execution.t.sol(1 hunks)contracts/test/foundry/KlerosCore_Staking.t.sol(3 hunks)contracts/test/foundry/KlerosCore_Voting.t.sol(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (11)
- GitHub Check: Redirect rules - kleros-v2-testnet
- GitHub Check: Redirect rules - kleros-v2-testnet
- GitHub Check: Redirect rules - kleros-v2-neo
- GitHub Check: Header rules - kleros-v2-testnet
- GitHub Check: Header rules - kleros-v2-testnet
- GitHub Check: Header rules - kleros-v2-neo
- GitHub Check: Pages changed - kleros-v2-testnet
- GitHub Check: Pages changed - kleros-v2-testnet
- GitHub Check: Pages changed - kleros-v2-neo
- GitHub Check: Analyze (javascript)
- GitHub Check: hardhat-tests
🔇 Additional comments (3)
contracts/src/arbitration/arbitrables/ArbitrableExample.sol (1)
43-44: Good move: centralize ruling options in storage.This reduces duplication and aligns dispute creation with runtime configuration.
contracts/test/foundry/KlerosCore_Disputes.t.sol (1)
97-97: Assertion message tweak is fine.contracts/test/foundry/KlerosCore_Voting.t.sol (1)
5-5: Import consolidation LGTM.
Comment on lines
+105
to
+108
| function changeNumberOfRulingOptions(uint256 _numberOfRulingOptions) external onlyByOwner { | ||
| numberOfRulingOptions = _numberOfRulingOptions; | ||
| } | ||
|
|
There was a problem hiding this comment.
Guard against zero ruling options and emit an event on change.
Zero would make future dispute creations likely revert (and voting choices invalid). Add validation and an event for observability.
Apply this diff:
function changeNumberOfRulingOptions(uint256 _numberOfRulingOptions) external onlyByOwner {
- numberOfRulingOptions = _numberOfRulingOptions;
+ if (_numberOfRulingOptions == 0) revert InvalidNumberOfRulingOptions();
+ uint256 previous = numberOfRulingOptions;
+ numberOfRulingOptions = _numberOfRulingOptions;
+ emit NumberOfRulingOptionsChanged(previous, _numberOfRulingOptions);
}And add these declarations in their respective sections:
// Events section
event NumberOfRulingOptionsChanged(uint256 previous, uint256 current);
// Errors section
error InvalidNumberOfRulingOptions();🤖 Prompt for AI Agents
In contracts/src/arbitration/arbitrables/ArbitrableExample.sol around lines 105
to 108, the setter changeNumberOfRulingOptions currently allows zero and
provides no observable signal; update it to revert when _numberOfRulingOptions
is zero (use a custom error InvalidNumberOfRulingOptions()) and emit a
NumberOfRulingOptionsChanged(previous, current) event after updating
numberOfRulingOptions; also add the event declaration in the Events section and
the error declaration in the Errors section.
Comment on lines
+504
to
+567
| function testFuzz_appeal(uint256 numberOfOptions, uint256 choice1, uint256 choice2, uint256 choice3) public { | ||
| uint256 disputeID = 0; | ||
|
|
||
| arbitrable.changeNumberOfRulingOptions(numberOfOptions); | ||
|
|
||
| // Have only 2 options for 3 jurors to create a majority | ||
| vm.assume(choice1 <= numberOfOptions); | ||
| vm.assume(choice2 <= numberOfOptions); | ||
| vm.assume(choice3 <= numberOfOptions); // Will be used for appeal | ||
|
|
||
| vm.prank(staker1); | ||
| core.setStake(GENERAL_COURT, 2000); | ||
| vm.prank(disputer); | ||
| arbitrable.createDispute{value: feeForJuror * DEFAULT_NB_OF_JURORS}("Action"); | ||
|
|
||
| (uint256 numberOfChoices, , ) = disputeKit.disputes(disputeID); | ||
|
|
||
| assertEq(numberOfChoices, numberOfOptions, "Wrong numberOfChoices"); | ||
|
|
||
| vm.warp(block.timestamp + minStakingTime); | ||
| sortitionModule.passPhase(); // Generating | ||
| vm.warp(block.timestamp + rngLookahead); | ||
| sortitionModule.passPhase(); // Drawing phase | ||
|
|
||
| // Split the stakers' votes. The first staker will get VoteID 0 and the second will take the rest. | ||
| core.draw(disputeID, 1); | ||
|
|
||
| vm.warp(block.timestamp + maxDrawingTime); | ||
| sortitionModule.passPhase(); // Staking phase to stake the 2nd voter | ||
| vm.prank(staker2); | ||
| core.setStake(GENERAL_COURT, 20000); | ||
| vm.warp(block.timestamp + minStakingTime); | ||
| sortitionModule.passPhase(); // Generating | ||
| vm.warp(block.timestamp + rngLookahead); | ||
| sortitionModule.passPhase(); // Drawing phase | ||
|
|
||
| core.draw(disputeID, 2); // Assign leftover votes to staker2 | ||
|
|
||
| vm.warp(block.timestamp + timesPerPeriod[0]); | ||
| core.passPeriod(disputeID); // Vote | ||
|
|
||
| uint256[] memory voteIDs = new uint256[](1); | ||
| voteIDs[0] = 0; | ||
| vm.prank(staker1); | ||
| disputeKit.castVote(disputeID, voteIDs, choice1, 0, "XYZ"); // Staker1 only got 1 vote because of low stake | ||
|
|
||
| voteIDs = new uint256[](2); | ||
| voteIDs[0] = 1; | ||
| voteIDs[1] = 2; | ||
| vm.prank(staker2); | ||
| disputeKit.castVote(disputeID, voteIDs, choice2, 0, "XYZ"); | ||
| core.passPeriod(disputeID); // Appeal | ||
|
|
||
| vm.assume(choice3 != choice2); | ||
| vm.prank(crowdfunder1); | ||
| disputeKit.fundAppeal{value: 0.63 ether}(disputeID, choice3); // Fund the losing choice. Total cost will be 0.63 (0.21 + 0.21 * (20000/10000)) | ||
|
|
||
| assertEq((disputeKit.getFundedChoices(disputeID)).length, 1, "1 choice should be funded"); | ||
|
|
||
| vm.prank(crowdfunder1); | ||
| disputeKit.fundAppeal{value: 0.42 ether}(disputeID, choice2); // Fund the winning choice. Total cost will be 0.42 (0.21 + 0.21 * (10000/10000)) | ||
|
|
||
| assertEq((disputeKit.getFundedChoices(disputeID)).length, 0, "No funded choices in a fresh round"); | ||
| } |
There was a problem hiding this comment.
Fuzz safety: enforce nonzero options and choices in [1..N].
Prevents castVote/fundAppeal from reverting on choice 0 or N=0.
Apply this diff:
- // Have only 2 options for 3 jurors to create a majority
- vm.assume(choice1 <= numberOfOptions);
- vm.assume(choice2 <= numberOfOptions);
- vm.assume(choice3 <= numberOfOptions); // Will be used for appeal
+ // Have only 2 options for 3 jurors to create a majority
+ vm.assume(numberOfOptions > 0);
+ choice1 = bound(choice1, 1, numberOfOptions);
+ choice2 = bound(choice2, 1, numberOfOptions);
+ choice3 = bound(choice3, 1, numberOfOptions); // Will be used for appeal📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| function testFuzz_appeal(uint256 numberOfOptions, uint256 choice1, uint256 choice2, uint256 choice3) public { | |
| uint256 disputeID = 0; | |
| arbitrable.changeNumberOfRulingOptions(numberOfOptions); | |
| // Have only 2 options for 3 jurors to create a majority | |
| vm.assume(choice1 <= numberOfOptions); | |
| vm.assume(choice2 <= numberOfOptions); | |
| vm.assume(choice3 <= numberOfOptions); // Will be used for appeal | |
| vm.prank(staker1); | |
| core.setStake(GENERAL_COURT, 2000); | |
| vm.prank(disputer); | |
| arbitrable.createDispute{value: feeForJuror * DEFAULT_NB_OF_JURORS}("Action"); | |
| (uint256 numberOfChoices, , ) = disputeKit.disputes(disputeID); | |
| assertEq(numberOfChoices, numberOfOptions, "Wrong numberOfChoices"); | |
| vm.warp(block.timestamp + minStakingTime); | |
| sortitionModule.passPhase(); // Generating | |
| vm.warp(block.timestamp + rngLookahead); | |
| sortitionModule.passPhase(); // Drawing phase | |
| // Split the stakers' votes. The first staker will get VoteID 0 and the second will take the rest. | |
| core.draw(disputeID, 1); | |
| vm.warp(block.timestamp + maxDrawingTime); | |
| sortitionModule.passPhase(); // Staking phase to stake the 2nd voter | |
| vm.prank(staker2); | |
| core.setStake(GENERAL_COURT, 20000); | |
| vm.warp(block.timestamp + minStakingTime); | |
| sortitionModule.passPhase(); // Generating | |
| vm.warp(block.timestamp + rngLookahead); | |
| sortitionModule.passPhase(); // Drawing phase | |
| core.draw(disputeID, 2); // Assign leftover votes to staker2 | |
| vm.warp(block.timestamp + timesPerPeriod[0]); | |
| core.passPeriod(disputeID); // Vote | |
| uint256[] memory voteIDs = new uint256[](1); | |
| voteIDs[0] = 0; | |
| vm.prank(staker1); | |
| disputeKit.castVote(disputeID, voteIDs, choice1, 0, "XYZ"); // Staker1 only got 1 vote because of low stake | |
| voteIDs = new uint256[](2); | |
| voteIDs[0] = 1; | |
| voteIDs[1] = 2; | |
| vm.prank(staker2); | |
| disputeKit.castVote(disputeID, voteIDs, choice2, 0, "XYZ"); | |
| core.passPeriod(disputeID); // Appeal | |
| vm.assume(choice3 != choice2); | |
| vm.prank(crowdfunder1); | |
| disputeKit.fundAppeal{value: 0.63 ether}(disputeID, choice3); // Fund the losing choice. Total cost will be 0.63 (0.21 + 0.21 * (20000/10000)) | |
| assertEq((disputeKit.getFundedChoices(disputeID)).length, 1, "1 choice should be funded"); | |
| vm.prank(crowdfunder1); | |
| disputeKit.fundAppeal{value: 0.42 ether}(disputeID, choice2); // Fund the winning choice. Total cost will be 0.42 (0.21 + 0.21 * (10000/10000)) | |
| assertEq((disputeKit.getFundedChoices(disputeID)).length, 0, "No funded choices in a fresh round"); | |
| } | |
| function testFuzz_appeal(uint256 numberOfOptions, uint256 choice1, uint256 choice2, uint256 choice3) public { | |
| uint256 disputeID = 0; | |
| arbitrable.changeNumberOfRulingOptions(numberOfOptions); | |
| // Have only 2 options for 3 jurors to create a majority | |
| vm.assume(numberOfOptions > 0); | |
| choice1 = bound(choice1, 1, numberOfOptions); | |
| choice2 = bound(choice2, 1, numberOfOptions); | |
| choice3 = bound(choice3, 1, numberOfOptions); // Will be used for appeal | |
| vm.prank(staker1); | |
| core.setStake(GENERAL_COURT, 2000); | |
| vm.prank(disputer); | |
| arbitrable.createDispute{value: feeForJuror * DEFAULT_NB_OF_JURORS}("Action"); | |
| (uint256 numberOfChoices, , ) = disputeKit.disputes(disputeID); | |
| assertEq(numberOfChoices, numberOfOptions, "Wrong numberOfChoices"); | |
| vm.warp(block.timestamp + minStakingTime); | |
| sortitionModule.passPhase(); // Generating | |
| vm.warp(block.timestamp + rngLookahead); | |
| sortitionModule.passPhase(); // Drawing phase | |
| // Split the stakers' votes. The first staker will get VoteID 0 and the second will take the rest. | |
| core.draw(disputeID, 1); | |
| vm.warp(block.timestamp + maxDrawingTime); | |
| sortitionModule.passPhase(); // Staking phase to stake the 2nd voter | |
| vm.prank(staker2); | |
| core.setStake(GENERAL_COURT, 20000); | |
| vm.warp(block.timestamp + minStakingTime); | |
| sortitionModule.passPhase(); // Generating | |
| vm.warp(block.timestamp + rngLookahead); | |
| sortitionModule.passPhase(); // Drawing phase | |
| core.draw(disputeID, 2); // Assign leftover votes to staker2 | |
| vm.warp(block.timestamp + timesPerPeriod[0]); | |
| core.passPeriod(disputeID); // Vote | |
| uint256[] memory voteIDs = new uint256[](1); | |
| voteIDs[0] = 0; | |
| vm.prank(staker1); | |
| disputeKit.castVote(disputeID, voteIDs, choice1, 0, "XYZ"); // Staker1 only got 1 vote because of low stake | |
| voteIDs = new uint256[](2); | |
| voteIDs[0] = 1; | |
| voteIDs[1] = 2; | |
| vm.prank(staker2); | |
| disputeKit.castVote(disputeID, voteIDs, choice2, 0, "XYZ"); | |
| core.passPeriod(disputeID); // Appeal | |
| vm.assume(choice3 != choice2); | |
| vm.prank(crowdfunder1); | |
| disputeKit.fundAppeal{value: 0.63 ether}(disputeID, choice3); // Fund the losing choice. Total cost will be 0.63 (0.21 + 0.21 * (20000/10000)) | |
| assertEq((disputeKit.getFundedChoices(disputeID)).length, 1, "1 choice should be funded"); | |
| vm.prank(crowdfunder1); | |
| disputeKit.fundAppeal{value: 0.42 ether}(disputeID, choice2); // Fund the winning choice. Total cost will be 0.42 (0.21 + 0.21 * (10000/10000)) | |
| assertEq((disputeKit.getFundedChoices(disputeID)).length, 0, "No funded choices in a fresh round"); | |
| } |
🤖 Prompt for AI Agents
In contracts/test/foundry/KlerosCore_Appeals.t.sol around lines 504 to 567, the
fuzz test does not guard against numberOfOptions being zero or choices being
zero/outside 1..numberOfOptions which can cause castVote/fundAppeal to revert;
add vm.assume(numberOfOptions > 0) and replace the existing vm.assume(choiceX <=
numberOfOptions) checks with vm.assume(choiceX >= 1 && choiceX <=
numberOfOptions) for choice1, choice2 and choice3 so all choices are in the
valid 1..N range and N is nonzero.
Comment on lines
+487
to
+540
| function testFuzz_castVote(uint256 numberOfOptions, uint256 choice1, uint256 choice2) public { | ||
| uint256 disputeID = 0; | ||
|
|
||
| arbitrable.changeNumberOfRulingOptions(numberOfOptions); | ||
|
|
||
| // Have only 2 options for 3 jurors to create a majority | ||
| vm.assume(choice1 <= numberOfOptions); | ||
| vm.assume(choice2 <= numberOfOptions); | ||
|
|
||
| vm.prank(staker1); | ||
| core.setStake(GENERAL_COURT, 2000); | ||
| vm.prank(disputer); | ||
| arbitrable.createDispute{value: feeForJuror * DEFAULT_NB_OF_JURORS}("Action"); | ||
| vm.warp(block.timestamp + minStakingTime); | ||
| sortitionModule.passPhase(); // Generating | ||
| vm.warp(block.timestamp + rngLookahead); | ||
| sortitionModule.passPhase(); // Drawing phase | ||
|
|
||
| // Split the stakers' votes. The first staker will get VoteID 0 and the second will take the rest. | ||
| core.draw(disputeID, 1); | ||
|
|
||
| vm.warp(block.timestamp + maxDrawingTime); | ||
| sortitionModule.passPhase(); // Staking phase to stake the 2nd voter | ||
| vm.prank(staker2); | ||
| core.setStake(GENERAL_COURT, 20000); | ||
| vm.warp(block.timestamp + minStakingTime); | ||
| sortitionModule.passPhase(); // Generating | ||
| vm.warp(block.timestamp + rngLookahead); | ||
| sortitionModule.passPhase(); // Drawing phase | ||
|
|
||
| core.draw(disputeID, 2); // Assign leftover votes to staker2 | ||
|
|
||
| vm.warp(block.timestamp + timesPerPeriod[0]); | ||
| core.passPeriod(disputeID); // Vote | ||
|
|
||
| uint256[] memory voteIDs = new uint256[](1); | ||
| voteIDs[0] = 0; | ||
| vm.prank(staker1); | ||
| disputeKit.castVote(disputeID, voteIDs, choice1, 0, "XYZ"); // Staker1 only got 1 vote because of low stake | ||
|
|
||
| voteIDs = new uint256[](2); | ||
| voteIDs[0] = 1; | ||
| voteIDs[1] = 2; | ||
| vm.prank(staker2); | ||
| disputeKit.castVote(disputeID, voteIDs, choice2, 0, "XYZ"); | ||
| core.passPeriod(disputeID); // Appeal | ||
|
|
||
| vm.warp(block.timestamp + timesPerPeriod[3]); | ||
| core.passPeriod(disputeID); // Execution | ||
|
|
||
| vm.expectEmit(true, true, true, true); | ||
| emit IArbitrableV2.Ruling(IArbitratorV2(address(core)), disputeID, choice2); | ||
| core.executeRuling(disputeID); | ||
| } |
There was a problem hiding this comment.
Fuzz safety: enforce choice lower bound and nonzero options.
choice == 0 will revert in castVote; also guard against numberOfOptions == 0.
Apply this diff:
- // Have only 2 options for 3 jurors to create a majority
- vm.assume(choice1 <= numberOfOptions);
- vm.assume(choice2 <= numberOfOptions);
+ // Have only 2 options for 3 jurors to create a majority
+ vm.assume(numberOfOptions > 0);
+ choice1 = bound(choice1, 1, numberOfOptions);
+ choice2 = bound(choice2, 1, numberOfOptions);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| function testFuzz_castVote(uint256 numberOfOptions, uint256 choice1, uint256 choice2) public { | |
| uint256 disputeID = 0; | |
| arbitrable.changeNumberOfRulingOptions(numberOfOptions); | |
| // Have only 2 options for 3 jurors to create a majority | |
| vm.assume(choice1 <= numberOfOptions); | |
| vm.assume(choice2 <= numberOfOptions); | |
| vm.prank(staker1); | |
| core.setStake(GENERAL_COURT, 2000); | |
| vm.prank(disputer); | |
| arbitrable.createDispute{value: feeForJuror * DEFAULT_NB_OF_JURORS}("Action"); | |
| vm.warp(block.timestamp + minStakingTime); | |
| sortitionModule.passPhase(); // Generating | |
| vm.warp(block.timestamp + rngLookahead); | |
| sortitionModule.passPhase(); // Drawing phase | |
| // Split the stakers' votes. The first staker will get VoteID 0 and the second will take the rest. | |
| core.draw(disputeID, 1); | |
| vm.warp(block.timestamp + maxDrawingTime); | |
| sortitionModule.passPhase(); // Staking phase to stake the 2nd voter | |
| vm.prank(staker2); | |
| core.setStake(GENERAL_COURT, 20000); | |
| vm.warp(block.timestamp + minStakingTime); | |
| sortitionModule.passPhase(); // Generating | |
| vm.warp(block.timestamp + rngLookahead); | |
| sortitionModule.passPhase(); // Drawing phase | |
| core.draw(disputeID, 2); // Assign leftover votes to staker2 | |
| vm.warp(block.timestamp + timesPerPeriod[0]); | |
| core.passPeriod(disputeID); // Vote | |
| uint256[] memory voteIDs = new uint256[](1); | |
| voteIDs[0] = 0; | |
| vm.prank(staker1); | |
| disputeKit.castVote(disputeID, voteIDs, choice1, 0, "XYZ"); // Staker1 only got 1 vote because of low stake | |
| voteIDs = new uint256[](2); | |
| voteIDs[0] = 1; | |
| voteIDs[1] = 2; | |
| vm.prank(staker2); | |
| disputeKit.castVote(disputeID, voteIDs, choice2, 0, "XYZ"); | |
| core.passPeriod(disputeID); // Appeal | |
| vm.warp(block.timestamp + timesPerPeriod[3]); | |
| core.passPeriod(disputeID); // Execution | |
| vm.expectEmit(true, true, true, true); | |
| emit IArbitrableV2.Ruling(IArbitratorV2(address(core)), disputeID, choice2); | |
| core.executeRuling(disputeID); | |
| } | |
| function testFuzz_castVote(uint256 numberOfOptions, uint256 choice1, uint256 choice2) public { | |
| uint256 disputeID = 0; | |
| arbitrable.changeNumberOfRulingOptions(numberOfOptions); | |
| // Have only 2 options for 3 jurors to create a majority | |
| vm.assume(numberOfOptions > 0); | |
| choice1 = bound(choice1, 1, numberOfOptions); | |
| choice2 = bound(choice2, 1, numberOfOptions); | |
| vm.prank(staker1); | |
| core.setStake(GENERAL_COURT, 2000); | |
| vm.prank(disputer); | |
| arbitrable.createDispute{value: feeForJuror * DEFAULT_NB_OF_JURORS}("Action"); | |
| vm.warp(block.timestamp + minStakingTime); | |
| sortitionModule.passPhase(); // Generating | |
| vm.warp(block.timestamp + rngLookahead); | |
| sortitionModule.passPhase(); // Drawing phase | |
| // Split the stakers' votes. The first staker will get VoteID 0 and the second will take the rest. | |
| core.draw(disputeID, 1); | |
| vm.warp(block.timestamp + maxDrawingTime); | |
| sortitionModule.passPhase(); // Staking phase to stake the 2nd voter | |
| vm.prank(staker2); | |
| core.setStake(GENERAL_COURT, 20000); | |
| vm.warp(block.timestamp + minStakingTime); | |
| sortitionModule.passPhase(); // Generating | |
| vm.warp(block.timestamp + rngLookahead); | |
| sortitionModule.passPhase(); // Drawing phase | |
| core.draw(disputeID, 2); // Assign leftover votes to staker2 | |
| vm.warp(block.timestamp + timesPerPeriod[0]); | |
| core.passPeriod(disputeID); // Vote | |
| uint256[] memory voteIDs = new uint256[](1); | |
| voteIDs[0] = 0; | |
| vm.prank(staker1); | |
| disputeKit.castVote(disputeID, voteIDs, choice1, 0, "XYZ"); // Staker1 only got 1 vote because of low stake | |
| voteIDs = new uint256[](2); | |
| voteIDs[0] = 1; | |
| voteIDs[1] = 2; | |
| vm.prank(staker2); | |
| disputeKit.castVote(disputeID, voteIDs, choice2, 0, "XYZ"); | |
| core.passPeriod(disputeID); // Appeal | |
| vm.warp(block.timestamp + timesPerPeriod[3]); | |
| core.passPeriod(disputeID); // Execution | |
| vm.expectEmit(true, true, true, true); | |
| emit IArbitrableV2.Ruling(IArbitratorV2(address(core)), disputeID, choice2); | |
| core.executeRuling(disputeID); | |
| } |
🤖 Prompt for AI Agents
In contracts/test/foundry/KlerosCore_Voting.t.sol around lines 487 to 540, the
fuzzed inputs allow numberOfOptions == 0 and choice1/choice2 == 0 which cause
castVote to revert; add preconditions to the fuzz assumptions: require
numberOfOptions > 0, and constrain choice1 and choice2 to be between 1 and
numberOfOptions inclusive (e.g., vm.assume(numberOfOptions > 0);
vm.assume(choice1 >= 1 && choice1 <= numberOfOptions); vm.assume(choice2 >= 1 &&
choice2 <= numberOfOptions)), leaving the rest of the test flow unchanged.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
PR-Codex overview
This PR introduces enhancements to the
ArbitrableExamplecontract by adding functionality to manage the number of ruling options in disputes and includes tests for various scenarios related to disputes, voting, and stakes in the Kleros arbitration system.Detailed summary
numberOfRulingOptionsvariable toArbitrableExample.changeNumberOfRulingOptionsfunction to update ruling options.numberOfRulingOptionsdeclarations increateDisputefunctions.Summary by CodeRabbit
New Features
Tests