Foundry tests file split into several files #2115
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughAdds an extensive Foundry test suite and test scaffolding for KlerosCore covering initialization, governance, staking, drawing, voting, appeals, execution, RNG fallback, dispute creation, and a reusable proxied test base. Only test files and test infrastructure are added; no production contract logic is modified. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 💡 Knowledge Base configuration:
You can enable these sources in your CodeRabbit configuration. 📒 Files selected for processing (10)
✨ Finishing Touches🧪 Generate unit tests
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (15)
contracts/test/foundry/KlerosCore_TestBase.sol (5)
5-5: Prune unnecessary imports to keep tests lean.console and KlerosCoreSnapshotProxy/IKlerosCore aren’t used here. Dropping them reduces compile noise and speeds up builds.
-import {console} from "forge-std/console.sol"; // Import the console for logging -import "../../src/libraries/Constants.sol"; -import {IKlerosCore, KlerosCoreSnapshotProxy} from "../../src/arbitration/view/KlerosCoreSnapshotProxy.sol"; +import "../../src/libraries/Constants.sol";Also applies to: 21-22
6-7: Avoid importing KlerosCoreBase from the mock file.Import KlerosCoreBase (or IArbitratorV2) from the canonical source only to prevent type duplication if interfaces diverge.
-import {KlerosCoreMock, KlerosCoreBase} from "../../src/test/KlerosCoreMock.sol"; +import {KlerosCoreMock} from "../../src/test/KlerosCoreMock.sol";
112-141: RNG consumer is initialized as address(0).This works because you set it later, but it’s easy to miss and can confuse future readers. Either document it or set the consumer in the constructor, then swap.
- rng = new BlockHashRNG(msg.sender, address(sortitionModule), rngLookahead); + // sortitionModule is not initialized yet; set consumer to owner and update below. + rng = new BlockHashRNG(msg.sender, address(this), rngLookahead); ... - vm.prank(owner); - rng.changeConsumer(address(sortitionModule)); + vm.prank(owner); + rng.changeConsumer(address(sortitionModule));
150-155: Initialize params array literal type-safety.Explicitly cast the mixed-literal array to the expected fixed-size type to avoid silent type inference surprises across compiler versions.
- [minStake, alpha, feeForJuror, jurorsForCourtJump], + [uint256(minStake), uint256(alpha), uint256(feeForJuror), uint256(jurorsForCourtJump)],
161-176: Explicitly encode arbitratorExtraData element widths.encodePacked(uint256, uint256, uint256) is fine, but a short comment helps future readers who may expect abi.encode and fixed widths.
Add comment:
- arbitratorExtraData = abi.encodePacked(uint256(GENERAL_COURT), DEFAULT_NB_OF_JURORS, DISPUTE_KIT_CLASSIC); + // Packed as three uint256: courtID | nbJurors | dkID + arbitratorExtraData = abi.encodePacked(uint256(GENERAL_COURT), DEFAULT_NB_OF_JURORS, DISPUTE_KIT_CLASSIC);contracts/test/foundry/KlerosCore_Execution.t.sol (1)
271-286: Unstake across multiple courts: event math comment could be clearer.The inline comments compress several balance transitions; consider splitting into stepwise notes to avoid misreading the 19k/2k numbers.
contracts/test/foundry/KlerosCore_Appeals.t.sol (2)
291-297: Avoid asserting a hardcoded local dispute ID in DisputeCreation.Local IDs are internal to the DK and can vary with setup. Prefer not checking event data for this emission and assert post-conditions via state reads.
- vm.expectEmit(true, true, true, true, address(disputeKit)); - emit DisputeKitClassicBase.DisputeCreation(disputeID, 2, newExtraData); + vm.expectEmit(true, true, true, false, address(disputeKit)); // don't check data + emit DisputeKitClassicBase.DisputeCreation(disputeID, 0, bytes("")); // topics only + // After the call, assert mapping/state: + uint256 localId = disputeKit.coreDisputeIDToLocal(disputeID); + (uint256 nChoices,, bytes memory xData) = disputeKit.disputes(localId); + assertEq(nChoices, 2, "Wrong numberOfChoices in classic DK"); + assertEq(xData, newExtraData, "Wrong extra data");
39-42: Style: use the disputeID variable for appealPeriod/appealCost calls.Minor consistency/readability improvement.
- (uint256 start, uint256 end) = core.appealPeriod(0); + (uint256 start, uint256 end) = core.appealPeriod(disputeID); ... - (start, end) = core.appealPeriod(0); + (start, end) = core.appealPeriod(disputeID); - assertEq(core.appealCost(0), 0.21 ether, "Wrong appealCost"); + assertEq(core.appealCost(disputeID), 0.21 ether, "Wrong appealCost");Also applies to: 55-61
contracts/test/foundry/KlerosCore_Initialization.t.sol (1)
58-62: Duplicate jump DK assertions.Both getJumpDisputeKitID() and jumpDisputeKitID() are asserted to the same value. One is enough unless you intend to validate both getter and storage explicitly.
contracts/test/foundry/KlerosCore_Voting.t.sol (2)
232-239: Minor: clarify comment about “Vote should be active”.Consider also asserting the inverse for a non-owned vote index to document expectations.
assertEq(disputeKit.isVoteActive(0, 0, 0), true, "Vote should be active"); + assertEq(disputeKit.isVoteActive(0, 0, 1), false, "Non-owned vote should be inactive");
359-376: Optional: DRY the commit/reveal helpers.Commit and reveal patterns repeat; factoring helper methods in the test base improves readability.
contracts/test/foundry/KlerosCore_Staking.t.sol (3)
57-60: Improve readability of big integer assertions.Use arithmetic expressions instead of long literals for balances/allowances.
- assertEq(pinakion.balanceOf(staker1), 999999999999998999, "Wrong token balance of staker1"); // 1 eth - 1001 wei - assertEq(pinakion.allowance(staker1, address(core)), 999999999999998999, "Wrong allowance for staker1"); + assertEq(pinakion.balanceOf(staker1), 1 ether - 1001, "Wrong token balance of staker1"); + assertEq(pinakion.allowance(staker1, address(core)), 1 ether - 1001, "Wrong allowance for staker1"); ... - assertEq(pinakion.balanceOf(staker1), 999999999999998000, "Wrong token balance of staker1"); // 1 eth - 2000 wei - assertEq(pinakion.allowance(staker1, address(core)), 999999999999998000, "Wrong allowance for staker1"); + assertEq(pinakion.balanceOf(staker1), 1 ether - 2000, "Wrong token balance of staker1"); + assertEq(pinakion.allowance(staker1, address(core)), 1 ether - 2000, "Wrong allowance for staker1"); ... - assertEq(pinakion.balanceOf(staker1), 999999999999998500, "Wrong token balance of staker1"); + assertEq(pinakion.balanceOf(staker1), 1 ether - 1500, "Wrong token balance of staker1"); ... - 999999999999998000, + 1 ether - 2000, "Allowance should not change during withdrawal"Also applies to: 77-80, 103-110, 133-139
185-219: Document phase expectations around delayed stakes.Tests assert no balance change during drawing phase; add a brief comment or helper to make the phase gating explicit.
Also applies to: 232-246
417-422: Test completeness: positive path for setStakeBySortitionModule.Consider adding a call from sortitionModule (vm.prank(address(sortitionModule))) to cover the allowed path.
contracts/test/foundry/KlerosCore_Governance.t.sol (1)
220-239: Ensure supportedDK array reflects both DKs explicitly.You re-use supportedDK earlier; add a local array here to avoid accidental coupling between tests when refactoring.
- badSupportedDK = new uint256[](1); + uint256[] memory badSupportedDK = new uint256[](1);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (10)
contracts/test/foundry/KlerosCore_Appeals.t.sol(1 hunks)contracts/test/foundry/KlerosCore_Disputes.t.sol(1 hunks)contracts/test/foundry/KlerosCore_Drawing.t.sol(1 hunks)contracts/test/foundry/KlerosCore_Execution.t.sol(1 hunks)contracts/test/foundry/KlerosCore_Governance.t.sol(1 hunks)contracts/test/foundry/KlerosCore_Initialization.t.sol(1 hunks)contracts/test/foundry/KlerosCore_RNG.t.sol(1 hunks)contracts/test/foundry/KlerosCore_Staking.t.sol(1 hunks)contracts/test/foundry/KlerosCore_TestBase.sol(1 hunks)contracts/test/foundry/KlerosCore_Voting.t.sol(1 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). (1)
- GitHub Check: SonarCloud
🔇 Additional comments (16)
contracts/test/foundry/KlerosCore_Execution.t.sol (4)
18-26: Good phased drawing flow split across stakers.Clear separation of draw waves and phase transitions improves determinism of assignment in tests.
Also applies to: 30-40
62-68: Pause guards validated.Nice negative tests for execute under pause/unpause.
98-111: Event expectations cover both penalties and rewards.Comprehensive expectEmit coverage makes regressions visible if repartition semantics change.
Also applies to: 123-136
411-417: Access control negative test is valuable.SortitionModuleOnly revert check prevents accidental admin misuse.
contracts/test/foundry/KlerosCore_RNG.t.sol (3)
14-27: Ownership/consumer wiring assertions are solid.Good sanity checks on RNGWithFallback constructor properties before integrating.
34-50: Fallback path validated against blockhash with timeout and rolled blocks.Strong signal that the module uses the intended block for entropy when the RNG backend is silent.
79-120: Access control matrix thoroughly covered.OwnerOnly/ConsumerOnly reverts and event checks on timeout changes are on point.
contracts/test/foundry/KlerosCore_Disputes.t.sol (4)
24-38: Court/DK enablement path mirrors production usage.Creating a court then enabling a specific DK ID before dispute creation avoids hidden coupling to defaults.
Also applies to: 49-53
41-48: Underpayment and unsupported-DK reverts checked.Good guardrail tests that would catch fee math or DK filtering regressions.
78-89: Round/state assertions are comprehensive.Validating pnkAtStakePerJuror, totals, and DK-local mappings ensures the core↔DK handshake is correct.
Also applies to: 98-112
119-147: ERC-20 flow: acceptance, rates, and transfer failure covered.Token path parity with ETH is well tested, including TransferFailed via direct core call.
contracts/test/foundry/KlerosCore_Drawing.t.sol (3)
26-33: Deterministic draw with low stake and full 3 slots is asserted well.Lock amounts and per-vote state checks (commit/choice/voted) make the round’s initial state unambiguous.
Also applies to: 42-49
62-69: No stakers path asserts drawIterations without phantom voters.Ensures empty addresses aren’t admitted as jurors.
76-90: Parent-court drawing power verified.Child-only staking leading to draws in the parent court is a subtle but important invariant.
Also applies to: 108-115, 118-123
contracts/test/foundry/KlerosCore_Initialization.t.sol (1)
63-74: Confirm delayed stake indices are intended initial values.delayedStakeReadIndex asserted as 1 (not 0). If that’s by design, consider a short comment in TestBase to document why.
contracts/test/foundry/KlerosCore_Governance.t.sol (1)
103-111: Changing to an uninitialized SortitionModuleMock might break invariants.If changeSortitionModule requires the new module to point back to Core or be initialized, add a positive-path test wiring a proper module via proxy and asserting linkage.
| emit DisputeKitClassicBase.Contribution(disputeID, 0, 1, crowdfunder1, 0.21 ether); | ||
| disputeKit.fundAppeal{value: 0.21 ether}(disputeID, 1); // Fund the losing choice. Total cost will be 0.63 (0.21 + 0.21 * (20000/10000)) | ||
|
|
||
| assertEq(crowdfunder1.balance, 9.79 ether, "Wrong balance of the crowdfunder"); | ||
| assertEq(address(disputeKit).balance, 0.21 ether, "Wrong balance of the DK"); | ||
| assertEq((disputeKit.getFundedChoices(disputeID)).length, 0, "No funded choices"); | ||
|
|
||
| vm.prank(crowdfunder1); | ||
| vm.expectEmit(true, true, true, true); | ||
| emit DisputeKitClassicBase.Contribution(disputeID, 0, 1, crowdfunder1, 0.42 ether); | ||
| vm.expectEmit(true, true, true, true); | ||
| emit DisputeKitClassicBase.ChoiceFunded(disputeID, 0, 1); | ||
| disputeKit.fundAppeal{value: 5 ether}(disputeID, 1); // Deliberately overpay to check reimburse | ||
|
|
||
| assertEq(crowdfunder1.balance, 9.37 ether, "Wrong balance of the crowdfunder"); | ||
| assertEq(address(disputeKit).balance, 0.63 ether, "Wrong balance of the DK"); | ||
| assertEq((disputeKit.getFundedChoices(disputeID)).length, 1, "One choice should be funded"); | ||
| assertEq((disputeKit.getFundedChoices(disputeID))[0], 1, "Incorrect funded choice"); | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Replace magic fee literals with computed amounts to de-brittle tests.
0.21/0.42/0.63/0.30 are derived; compute from core.appealCost and feeForJuror to keep tests stable across param changes.
- emit DisputeKitClassicBase.Contribution(disputeID, 0, 1, crowdfunder1, 0.21 ether);
- disputeKit.fundAppeal{value: 0.21 ether}(disputeID, 1);
+ uint256 appealCost = core.appealCost(disputeID);
+ emit DisputeKitClassicBase.Contribution(disputeID, 0, 1, crowdfunder1, appealCost);
+ disputeKit.fundAppeal{value: appealCost}(disputeID, 1);
...
- emit DisputeKitClassicBase.Contribution(disputeID, 0, 1, crowdfunder1, 0.42 ether);
+ emit DisputeKitClassicBase.Contribution(disputeID, 0, 1, crowdfunder1, 2 * appealCost);
...
- assertEq(address(disputeKit).balance, 0.63 ether, "Wrong balance of the DK");
+ assertEq(address(disputeKit).balance, 3 * appealCost, "Wrong balance of the DK");
...
- disputeKit.fundAppeal{value: 0.63 ether}(disputeID, 1);
+ disputeKit.fundAppeal{value: 3 * appealCost}(disputeID, 1);
...
- disputeKit.fundAppeal{value: 0.42 ether}(disputeID, 2);
+ disputeKit.fundAppeal{value: 2 * appealCost}(disputeID, 2);
...
- assertEq(address(core).balance, 0.3 ether, "Wrong balance of the core"); // 0.09 arbFee + 0.21 appealFee
+ assertEq(address(core).balance, (DEFAULT_NB_OF_JURORS * feeForJuror) + appealCost, "Wrong balance of the core");Also applies to: 175-184, 191-193
🤖 Prompt for AI Agents
In contracts/test/foundry/KlerosCore_Appeals.t.sol around lines 81-99 (and
similarly for 175-184 and 191-193), replace hard-coded ether literals
(0.21/0.42/0.63/0.30 etc.) with values computed from core.appealCost and the
feeForJuror multiplier: compute the single juror share = appealCost *
feeForJuror / 10000 (or the appropriate denominator used in the contract), use
that to calculate required contributions, overpay amounts and expected
reimbursements, and then use those computed values in the fundAppeal{value: ...}
calls and assertEq checks so tests derive amounts dynamically from
core.appealCost and feeForJuror rather than magic literals.
| 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); | ||
| disputeKit.fundAppeal{value: 0.1 ether}(disputeID, 2); |
There was a problem hiding this comment.
Zero-value funding doesn’t exercise the winning-choices window.
Calling fundAppeal without value is a no-op; it can pass spuriously and not validate timing logic. Send a positive contribution to actually test the “winner still fundable” path.
- disputeKit.fundAppeal(disputeID, 2); // Winning choice funding should not revert yet
+ disputeKit.fundAppeal{value: 0.1 ether}(disputeID, 2); // Winning choice funding should not revert yet📝 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.
| 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); | |
| disputeKit.fundAppeal{value: 0.1 ether}(disputeID, 2); | |
| disputeKit.fundAppeal{value: 0.1 ether}(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); | |
| disputeKit.fundAppeal{value: 0.1 ether}(disputeID, 2); |
🤖 Prompt for AI Agents
In contracts/test/foundry/KlerosCore_Appeals.t.sol around lines 141 to 146, the
test calls disputeKit.fundAppeal with zero value which is a no-op and can
falsely pass; replace the zero-value call with a positive contribution (e.g.,
0.1 ether) while keeping the vm.prank and vm.expectRevert so the test actually
attempts to fund the winning choice and triggers the AppealPeriodIsOver revert
as intended.
| vm.expectEmit(true, true, true, true); | ||
| emit KlerosCoreBase.Paused(); | ||
| core.pause(); | ||
| assertEq(core.paused(), true, "Wrong paused value"); | ||
| // Switch between owner and guardian to test both. WhenNotPausedOnly modifier is triggered after owner's check. | ||
| vm.prank(owner); | ||
| vm.expectRevert(KlerosCoreBase.WhenNotPausedOnly.selector); | ||
| core.pause(); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Bind event emitters for governance events.
Strengthen expectations by specifying the Core proxy as the emitter.
- vm.expectEmit(true, true, true, true);
+ vm.expectEmit(true, true, true, true, address(core));
emit KlerosCoreBase.Paused();
...
- vm.expectEmit(true, true, true, true);
+ vm.expectEmit(true, true, true, true, address(core));
emit KlerosCoreBase.Unpaused();
...
- vm.expectEmit(true, true, true, true);
+ vm.expectEmit(true, true, true, true, address(core));
emit KlerosCoreBase.DisputeKitCreated(2, newDK);
...
- vm.expectEmit(true, true, true, true);
+ vm.expectEmit(true, true, true, true, address(core));
emit KlerosCoreBase.DisputeKitEnabled(2, DISPUTE_KIT_CLASSIC, true);
- vm.expectEmit(true, true, true, true);
+ vm.expectEmit(true, true, true, true, address(core));
emit KlerosCoreBase.DisputeKitEnabled(2, 2, true);
- vm.expectEmit(true, true, true, true);
+ vm.expectEmit(true, true, true, true, address(core));
emit KlerosCoreBase.CourtCreated(
2,
GENERAL_COURT,
true,
2000,
20000,
0.04 ether,
50,
[uint256(10), uint256(20), uint256(30), uint256(40)],
supportedDK
);
...
- vm.expectEmit(true, true, true, true);
+ vm.expectEmit(true, true, true, true, address(core));
emit KlerosCoreBase.CourtModified(
GENERAL_COURT,
true,
2000,
20000,
0.04 ether,
50,
[uint256(10), uint256(20), uint256(30), uint256(40)]
);
...
- vm.expectEmit(true, true, true, true);
+ vm.expectEmit(true, true, true, true, address(core));
emit KlerosCoreBase.DisputeKitEnabled(GENERAL_COURT, newDkID, true);
- vm.expectEmit(true, true, true, true);
+ vm.expectEmit(true, true, true, true, address(core));
emit KlerosCoreBase.DisputeKitEnabled(GENERAL_COURT, newDkID, false);Also applies to: 41-47, 119-124, 240-257, 339-357, 391-401
🤖 Prompt for AI Agents
contracts/test/foundry/KlerosCore_Governance.t.sol around lines 21-29 (and
similarly update lines 41-47, 119-124, 240-257, 339-357, 391-401): the test uses
vm.expectEmit(...) without binding the emitter, so it may match events from the
wrong contract; update each vm.expectEmit call for governance-related events to
pass the Core proxy as the emitter (i.e., add the core contract address as the
final argument to vm.expectEmit), and ensure the corresponding emit expectations
remain unchanged so the emitted event is asserted to come from the Core proxy.
| vm.expectEmit(true, true, true, true); | ||
| emit KlerosCoreBase.DisputeKitCreated(DISPUTE_KIT_CLASSIC, newDisputeKit); | ||
| vm.expectEmit(true, true, true, true); | ||
|
|
||
| uint256[] memory supportedDK = new uint256[](1); | ||
| supportedDK[0] = DISPUTE_KIT_CLASSIC; | ||
| emit KlerosCoreBase.CourtCreated( | ||
| GENERAL_COURT, | ||
| FORKING_COURT, | ||
| false, | ||
| 1000, | ||
| 10000, | ||
| 0.03 ether, | ||
| 511, | ||
| [uint256(60), uint256(120), uint256(180), uint256(240)], // Explicitly convert otherwise it throws | ||
| supportedDK | ||
| ); | ||
| vm.expectEmit(true, true, true, true); | ||
| emit KlerosCoreBase.DisputeKitEnabled(GENERAL_COURT, DISPUTE_KIT_CLASSIC, true); | ||
| newCore.initialize( |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Bind expected emitters for initialization events.
Pin the emitter to the proxy Core to ensure we’re validating the correct contract emitted the events.
- vm.expectEmit(true, true, true, true);
+ vm.expectEmit(true, true, true, true, address(proxyCore));
emit KlerosCoreBase.DisputeKitCreated(DISPUTE_KIT_CLASSIC, newDisputeKit);
- vm.expectEmit(true, true, true, true);
+ vm.expectEmit(true, true, true, true, address(proxyCore));
emit KlerosCoreBase.CourtCreated(
GENERAL_COURT,
FORKING_COURT,
false,
1000,
10000,
0.03 ether,
511,
[uint256(60), uint256(120), uint256(180), uint256(240)],
supportedDK
);
- vm.expectEmit(true, true, true, true);
+ vm.expectEmit(true, true, true, true, address(proxyCore));
emit KlerosCoreBase.DisputeKitEnabled(GENERAL_COURT, DISPUTE_KIT_CLASSIC, true);Also applies to: 146-156
🤖 Prompt for AI Agents
In contracts/test/foundry/KlerosCore_Initialization.t.sol around lines 140 to
159, the test uses vm.expectEmit but doesn't pin the expected emitter to the
proxy Core, so emitted events may be validated against the wrong address; update
each vm.expectEmit call to use the overload that accepts an address and pass
address(newCore) (e.g. vm.expectEmit(true, true, true, true, address(newCore)))
before each corresponding emit to ensure the proxy Core is the expected emitter
(apply same change for lines 146-156).
| emit SortitionModuleBase.StakeSet(staker1, GENERAL_COURT, 1500, 1500); | ||
| vm.expectEmit(true, true, true, true); | ||
| emit SortitionModuleBase.StakeSet(staker1, GENERAL_COURT, 1800, 1800); | ||
| sortitionModule.executeDelayedStakes(20); // Deliberately ask for more iterations than needed | ||
|
|
||
| assertEq(sortitionModule.delayedStakeWriteIndex(), 3, "Wrong delayedStakeWriteIndex"); | ||
| assertEq(sortitionModule.delayedStakeReadIndex(), 4, "Wrong delayedStakeReadIndex"); | ||
|
|
||
| // Check that delayed stakes are nullified |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Constrain emitters for StakeSet during execution.
Bind to SortitionModule address.
- vm.expectEmit(true, true, true, true);
+ vm.expectEmit(true, true, true, true, address(sortitionModule));
emit SortitionModuleBase.StakeSet(staker1, GENERAL_COURT, 1500, 1500);
- vm.expectEmit(true, true, true, true);
+ vm.expectEmit(true, true, true, true, address(sortitionModule));
emit SortitionModuleBase.StakeSet(staker1, GENERAL_COURT, 1800, 1800);Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In contracts/test/foundry/KlerosCore_Staking.t.sol around lines 387-395, the
vm.expectEmit calls need to be constrained to the SortitionModule address so the
expected StakeSet events are bound to that emitter; before emitting the expected
event(s) call vm.expectEmit(true, true, true, true, address(sortitionModule))
(or the equivalent used in this test suite) and then emit the
SortitionModuleBase.StakeSet(...) as before so the check verifies the event came
from the SortitionModule.
| // Check that delayed stakes are nullified | ||
| for (uint i = 2; i <= sortitionModule.delayedStakeWriteIndex(); i++) { | ||
| (account, courtID, stake, alreadyTransferred) = sortitionModule.delayedStakes(i); | ||
|
|
||
| assertEq(account, address(0), "Wrong staker account after delayed stake deletion"); | ||
| assertEq(courtID, 0, "Court id should be nullified"); | ||
| assertEq(stake, 0, "No amount to stake"); | ||
| assertEq(alreadyTransferred, false, "Should be false"); | ||
| } | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Also verify the first delayed stake (index 1) was cleared.
Loop currently starts at 2; index 1 should be zeroed after execution as well.
- for (uint i = 2; i <= sortitionModule.delayedStakeWriteIndex(); i++) {
+ for (uint256 i = 1; i <= sortitionModule.delayedStakeWriteIndex(); i++) {
(account, courtID, stake, alreadyTransferred) = sortitionModule.delayedStakes(i);📝 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.
| // Check that delayed stakes are nullified | |
| for (uint i = 2; i <= sortitionModule.delayedStakeWriteIndex(); i++) { | |
| (account, courtID, stake, alreadyTransferred) = sortitionModule.delayedStakes(i); | |
| assertEq(account, address(0), "Wrong staker account after delayed stake deletion"); | |
| assertEq(courtID, 0, "Court id should be nullified"); | |
| assertEq(stake, 0, "No amount to stake"); | |
| assertEq(alreadyTransferred, false, "Should be false"); | |
| } | |
| // Check that delayed stakes are nullified | |
| for (uint256 i = 1; i <= sortitionModule.delayedStakeWriteIndex(); i++) { | |
| (account, courtID, stake, alreadyTransferred) = sortitionModule.delayedStakes(i); | |
| assertEq(account, address(0), "Wrong staker account after delayed stake deletion"); | |
| assertEq(courtID, 0, "Court id should be nullified"); | |
| assertEq(stake, 0, "No amount to stake"); | |
| assertEq(alreadyTransferred, false, "Should be false"); | |
| } |
🤖 Prompt for AI Agents
In contracts/test/foundry/KlerosCore_Staking.t.sol around lines 395 to 404, the
verification loop skips delayed stake index 1 by starting at 2; update the test
so index 1 is also checked — either change the loop start to 1 (for (uint i = 1;
i <= sortitionModule.delayedStakeWriteIndex(); i++)) or add an explicit
assertion block that fetches delayedStakes(1) and asserts account == address(0),
courtID == 0, stake == 0, and alreadyTransferred == false.
| vm.expectEmit(true, true, true, true); | ||
| emit KlerosCoreBase.NewPeriod(disputeID, KlerosCoreBase.Period.commit); | ||
| core.passPeriod(disputeID); | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Constrain event emitters (Core/DK) to reduce test false positives.
Attach the contract address that should emit each event.
- vm.expectEmit(true, true, true, true);
+ vm.expectEmit(true, true, true, true, address(core));
emit KlerosCoreBase.NewPeriod(disputeID, KlerosCoreBase.Period.commit);
...
- vm.expectEmit(true, true, true, true);
+ vm.expectEmit(true, true, true, true, address(disputeKit));
emit DisputeKitClassicBase.CommitCast(disputeID, staker1, voteIDs, commit);
...
- vm.expectEmit(true, true, true, true);
+ vm.expectEmit(true, true, true, true, address(core));
emit KlerosCoreBase.NewPeriod(disputeID, KlerosCoreBase.Period.vote);
...
- vm.expectEmit(true, true, true, true);
+ vm.expectEmit(true, true, true, true, address(disputeKit));
emit IDisputeKit.VoteCast(disputeID, staker1, voteIDs, 2, "XYZ");
...
- vm.expectEmit(true, true, true, true);
+ vm.expectEmit(true, true, true, true, address(disputeKit));
emit IDisputeKit.VoteCast(disputeID, staker1, voteIDs, 1, "XYZZ");
...
- vm.expectEmit(true, true, true, true);
+ vm.expectEmit(true, true, true, true, address(core));
emit KlerosCoreBase.AppealPossible(disputeID, arbitrable);
- vm.expectEmit(true, true, true, true);
+ vm.expectEmit(true, true, true, true, address(core));
emit KlerosCoreBase.NewPeriod(disputeID, KlerosCoreBase.Period.appeal);
...
- vm.expectEmit(true, true, true, true);
+ vm.expectEmit(true, true, true, true, address(core));
emit KlerosCoreBase.NewPeriod(disputeID, KlerosCoreBase.Period.appeal);
...
- vm.expectEmit(true, true, true, true);
+ vm.expectEmit(true, true, true, true, address(core));
emit KlerosCoreBase.DisputeKitEnabled(GENERAL_COURT, newDkID, true);Also applies to: 71-74, 186-189, 210-213, 244-247, 261-263, 293-297, 383-385, 409-413
jaybuidl
force-pushed
the
refactor/foundry-tests-split
branch
from
6b109da to
602120e
Compare
❌ Deploy Preview for kleros-v2-university failed. Why did it fail? →
|
✅ 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-neo ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
… was over 3000 lines
jaybuidl
force-pushed
the
refactor/foundry-tests-split
branch
from
602120e to
ea5b3a7
Compare
✅ Deploy Preview for kleros-v2-testnet-devtools ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
|
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (5)
contracts/test/foundry/KlerosCore_Initialization.t.sol (1)
137-156: Bind expected emitters to the Core proxy for event checks.Without pinning the emitter, assertions may pass on events from the wrong contract. Bind to address(proxyCore) for all three expectations.
- vm.expectEmit(true, true, true, true); + vm.expectEmit(true, true, true, true, address(proxyCore)); emit KlerosCoreBase.DisputeKitCreated(DISPUTE_KIT_CLASSIC, newDisputeKit); - vm.expectEmit(true, true, true, true); + vm.expectEmit(true, true, true, true, address(proxyCore)); // ... - vm.expectEmit(true, true, true, true); + vm.expectEmit(true, true, true, true, address(proxyCore)); emit KlerosCoreBase.DisputeKitEnabled(GENERAL_COURT, DISPUTE_KIT_CLASSIC, true);contracts/test/foundry/KlerosCore_Appeals.t.sol (4)
48-53: Duplicate comment: tighten event assertions by specifying emitters.Previous review already identified this issue - the
vm.expectEmitcalls should specify the emitter address to avoid false positives and improve test reliability.
81-82: Duplicate comment: replace magic fee literals with computed amounts.Previous review identified this issue - the hardcoded
0.21 ethershould be computed fromcore.appealCost(disputeID)to make tests more maintainable and less brittle.
141-141: Duplicate comment: zero-value funding doesn't exercise timing logic.Previous review correctly identified that the zero-value
fundAppealcall is a no-op and should use a positive value to actually test the timing constraints.
286-295: Duplicate comment: specify emitters in expectEmit calls.These event expectations also lack emitter specifications as identified in the previous review covering lines 48-53.
🧹 Nitpick comments (6)
contracts/test/foundry/KlerosCore_Initialization.t.sol (6)
147-153: Replace magic numbers with the variables defined above to avoid drift.- 1000, - 10000, - 0.03 ether, - 511, - [uint256(60), uint256(120), uint256(180), uint256(240)], // Explicitly convert otherwise it throws + newMinStake, + newAlpha, + newFeeForJuror, + newJurorsForCourtJump, + newTimesPerPeriod,
99-101: Decouple from base constant and drop redundant self-transfer.The first transfer is a no-op (to msg.sender itself) and “totalSupply” couples to TestBase. Prefer the token’s own supply; optionally remove the self-transfer.
Option A (decouple only):
- newPinakion.transfer(msg.sender, totalSupply - 1 ether); + newPinakion.transfer(msg.sender, newPinakion.totalSupply() - 1 ether);Option B (remove redundant self-transfer, if PNK mints to deployer/msg.sender):
- newPinakion.transfer(msg.sender, totalSupply - 1 ether); newPinakion.transfer(newStaker1, 1 ether);
133-134: Remove redundant prank.newOwner == msg.sender here, so vm.prank is unnecessary.
- vm.prank(newOwner); newRng.changeConsumer(address(newSortitionModule));
66-66: Avoid brittle equality to block.timestamp.Equality can flake if setup and test run in different tx contexts. Use a small tolerance.
- assertEq(sortitionModule.lastPhaseChange(), block.timestamp, "Wrong lastPhaseChange"); + assertApproxEqAbs(sortitionModule.lastPhaseChange(), block.timestamp, 1, "Wrong lastPhaseChange");If forge-std’s approx assertions aren’t available in this test base, confirm and I’ll propose an alternative.
70-72: Assert the index relation instead of hard-coded values.This ties the expectations to the invariant (read = write + 1) and keeps intent if defaults change.
- assertEq(sortitionModule.delayedStakeWriteIndex(), 0, "delayedStakeWriteIndex should be 0"); - assertEq(sortitionModule.delayedStakeReadIndex(), 1, "Wrong delayedStakeReadIndex"); + uint256 _w = sortitionModule.delayedStakeWriteIndex(); + assertEq(_w, 0, "delayedStakeWriteIndex should be 0"); + assertEq(sortitionModule.delayedStakeReadIndex(), _w + 1, "Wrong delayedStakeReadIndex");
91-91: Remove unused variable newOther.It’s not referenced.
- address newOther = vm.addr(9);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (10)
contracts/test/foundry/KlerosCore_Appeals.t.sol(1 hunks)contracts/test/foundry/KlerosCore_Disputes.t.sol(1 hunks)contracts/test/foundry/KlerosCore_Drawing.t.sol(1 hunks)contracts/test/foundry/KlerosCore_Execution.t.sol(1 hunks)contracts/test/foundry/KlerosCore_Governance.t.sol(1 hunks)contracts/test/foundry/KlerosCore_Initialization.t.sol(1 hunks)contracts/test/foundry/KlerosCore_RNG.t.sol(1 hunks)contracts/test/foundry/KlerosCore_Staking.t.sol(1 hunks)contracts/test/foundry/KlerosCore_TestBase.sol(1 hunks)contracts/test/foundry/KlerosCore_Voting.t.sol(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (8)
- contracts/test/foundry/KlerosCore_RNG.t.sol
- contracts/test/foundry/KlerosCore_Execution.t.sol
- contracts/test/foundry/KlerosCore_Staking.t.sol
- contracts/test/foundry/KlerosCore_Drawing.t.sol
- contracts/test/foundry/KlerosCore_Governance.t.sol
- contracts/test/foundry/KlerosCore_TestBase.sol
- contracts/test/foundry/KlerosCore_Voting.t.sol
- contracts/test/foundry/KlerosCore_Disputes.t.sol
⏰ 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). (15)
- GitHub Check: Redirect rules - kleros-v2-neo
- GitHub Check: Header rules - kleros-v2-neo
- GitHub Check: Pages changed - kleros-v2-neo
- GitHub Check: Redirect rules - kleros-v2-testnet
- GitHub Check: Redirect rules - kleros-v2-testnet
- GitHub Check: Redirect rules - kleros-v2-university
- GitHub Check: Header rules - kleros-v2-testnet
- GitHub Check: Header rules - kleros-v2-testnet
- GitHub Check: Header rules - kleros-v2-university
- GitHub Check: Pages changed - kleros-v2-testnet
- GitHub Check: Pages changed - kleros-v2-testnet
- GitHub Check: Pages changed - kleros-v2-university
- GitHub Check: contracts-testing
- GitHub Check: Analyze (javascript)
- GitHub Check: SonarCloud
🔇 Additional comments (5)
contracts/test/foundry/KlerosCore_Appeals.t.sol (5)
13-103: Test coverage looks comprehensive for basic appeal funding flow.The test properly covers the complete appeal funding workflow including error conditions, event emissions, balance tracking, and state transitions. The logic correctly validates funding restrictions, overpayment handling, and duplicate funding prevention.
211-326: Complex DK/court switching test is well-structured.This test effectively validates the dispute kit and court jumping mechanism, including proper event emission, state transitions, and access control. The test correctly verifies that the dispute jumps to the parent DK and that draw operations work in the new round.
328-361: Quick appeal period pass test is clear and focused.The test properly validates that the appeal period can be passed early when no funding occurs, transitioning directly to execution. The logic and timing manipulation are correct.
229-229: extraData encoding is correct
Matches the 3×32-byte layout parsed by_extraDataToCourtIDMinJurorsDisputeKitin KlerosCoreBase.sol.
149-209: Ignore balance calculation warning. The test’s assertions match the on-chain logic: disputeKit collects 0.63 ETH + 0.42 ETH = 1.05 ETH, pays one appeal fee of 0.21 ETH to core (viacore.appeal), leaving 0.84 ETH; core holds 0.09 ETH arbitration fees + 0.21 ETH appeal fee = 0.30 ETH.Likely an incorrect or invalid review comment.
Reason: the test file was over 3000 lines.
PR-Codex overview
This PR introduces new test contracts for the
KlerosCoresystem, focusing on random number generation, jury drawing, dispute management, and governance functionalities. It enhances testing coverage for various scenarios and interactions within the Kleros arbitration framework.Detailed summary
KlerosCore_RNGTestfor random number generation and fallback testing.KlerosCore_DrawingTestto verify jury drawing and selection mechanisms.KlerosCore_DisputesTestfor dispute creation and management.KlerosCore_InitializationTestfor core initialization checks.KlerosCore_AppealsTestto test appeal processes and funding.KlerosCore_GovernanceTestfor governance operations like pausing and owner changes.KlerosCore_VotingTestto validate voting mechanisms and commit/reveal processes.Summary by CodeRabbit