fix(SortitionModule): total stake update #2199
Conversation
❌ Deploy Preview for kleros-v2-testnet failed. Why did it fail? →
|
✅ Deploy Preview for kleros-v2-neo ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
WalkthroughKlerosCore now checks ERC20 transfer results and reverts on failure; SortitionModule centralizes totalStaked updates into stake-mutation paths; DisputeKitClassicBase switches ruled checks to Period.execution; new tests and a MaliciousArbitrableMock exercise staking, execution, and revert scenarios. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
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 |
❌ Deploy Preview for kleros-v2-testnet-devtools failed. Why did it fail? →
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
contracts/src/arbitration/SortitionModule.sol (1)
325-333: Consider a more defensive implementation for clarity.The current if/else logic relies on the invariant that
_pnkDepositand_pnkWithdrawalare mutually exclusive (only one is non-zero at a time). While this is enforced byvalidateStake, the function itself doesn't verify this assumption.A more explicit and defensive approach:
function updateTotalStake(uint256 _pnkDeposit, uint256 _pnkWithdrawal) external override onlyByCore { // Note that we don't update totalStake in setStake() function because it doesn't always change total (e.g. during rewards/penalties). if (_pnkDeposit > 0) { totalStaked += _pnkDeposit; - } else { + } + if (_pnkWithdrawal > 0) { totalStaked -= _pnkWithdrawal; } }This handles all cases correctly without relying on the caller's invariant, and also avoids the unnecessary
totalStaked -= 0when both parameters are zero.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
contracts/src/arbitration/KlerosCore.sol(1 hunks)contracts/src/arbitration/SortitionModule.sol(1 hunks)contracts/src/arbitration/interfaces/ISortitionModule.sol(1 hunks)contracts/src/arbitration/university/SortitionModuleUniversity.sol(1 hunks)contracts/test/foundry/KlerosCore_Staking.t.sol(1 hunks)
🧰 Additional context used
🧠 Learnings (6)
📓 Common learnings
Learnt from: jaybuidl
Repo: kleros/kleros-v2 PR: 1746
File: contracts/config/courts.v2.mainnet-neo.json:167-170
Timestamp: 2024-11-19T16:31:08.965Z
Learning: In `contracts/config/courts.v2.mainnet-neo.json`, the `minStake` parameter is denominated in PNK, not ETH.
📚 Learning: 2025-09-03T22:48:32.972Z
Learnt from: jaybuidl
Repo: kleros/kleros-v2 PR: 0
File: :0-0
Timestamp: 2025-09-03T22:48:32.972Z
Learning: In the Kleros v2 codebase, the team prioritizes gas optimization over strict CEI pattern compliance when dealing with trusted contracts. For penalty execution logic, they prefer batching storage writes (`round.pnkPenalties`) rather than updating incrementally after each penalty calculation to save gas costs, as the risk is extremely low between trusted contracts.
Applied to files:
contracts/src/arbitration/KlerosCore.sol
📚 Learning: 2025-09-30T17:18:12.895Z
Learnt from: jaybuidl
Repo: kleros/kleros-v2 PR: 2145
File: contracts/src/arbitration/dispute-kits/DisputeKitClassicBase.sol:277-286
Timestamp: 2025-09-30T17:18:12.895Z
Learning: In DisputeKitClassicBase.sol's castCommit function, jurors are allowed to re-submit commits during the commit period. The implementation uses a commitCount variable to track only first-time commits (where commit == bytes32(0)) so that totalCommitted is not incremented when a juror updates their existing commit.
Applied to files:
contracts/src/arbitration/KlerosCore.sol
📚 Learning: 2024-11-19T16:31:08.965Z
Learnt from: jaybuidl
Repo: kleros/kleros-v2 PR: 1746
File: contracts/config/courts.v2.mainnet-neo.json:167-170
Timestamp: 2024-11-19T16:31:08.965Z
Learning: In the court hierarchy, child courts' `minStake` must be greater than or equal to their parent court's `minStake`.
Applied to files:
contracts/src/arbitration/KlerosCore.sol
📚 Learning: 2024-11-19T16:31:08.965Z
Learnt from: jaybuidl
Repo: kleros/kleros-v2 PR: 1746
File: contracts/config/courts.v2.mainnet-neo.json:167-170
Timestamp: 2024-11-19T16:31:08.965Z
Learning: In `contracts/config/courts.v2.mainnet-neo.json`, the `minStake` parameter is denominated in PNK, not ETH.
Applied to files:
contracts/src/arbitration/interfaces/ISortitionModule.sol
📚 Learning: 2025-09-03T19:34:58.056Z
Learnt from: jaybuidl
Repo: kleros/kleros-v2 PR: 2107
File: contracts/src/arbitration/university/KlerosCoreUniversity.sol:1083-1092
Timestamp: 2025-09-03T19:34:58.056Z
Learning: KlerosCoreUniversity and SortitionModuleUniversity do not have phases, unlike KlerosCoreBase and SortitionModuleBase. Therefore, validateStake in the University contracts will never return StakingResult.Delayed, only Successful or other failure states.
Applied to files:
contracts/src/arbitration/university/SortitionModuleUniversity.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). (14)
- GitHub Check: Redirect rules - kleros-v2-testnet-devtools
- GitHub Check: Header rules - kleros-v2-testnet-devtools
- GitHub Check: Pages changed - kleros-v2-testnet-devtools
- GitHub Check: Redirect rules - kleros-v2-neo
- GitHub Check: Redirect rules - kleros-v2-testnet
- GitHub Check: Header rules - kleros-v2-neo
- GitHub Check: Redirect rules - kleros-v2-testnet
- GitHub Check: Header rules - kleros-v2-testnet
- GitHub Check: Pages changed - kleros-v2-neo
- GitHub Check: Header rules - kleros-v2-testnet
- GitHub Check: Pages changed - kleros-v2-testnet
- GitHub Check: Pages changed - kleros-v2-testnet
- GitHub Check: Analyze (javascript)
- GitHub Check: hardhat-tests
🔇 Additional comments (3)
contracts/src/arbitration/KlerosCore.sol (1)
1365-1367: LGTM! Correctly integrates total stake update into the staking flow.The call is properly placed after
setStakecompletes and only executes for successful, non-delayed stakes. The flow ensurestotalStakedbookkeeping is synchronized with actual PNK transfers.contracts/src/arbitration/university/SortitionModuleUniversity.sol (1)
133-136: LGTM! Appropriate no-op implementation for the University variant.The stub correctly satisfies the interface contract while maintaining access control. Based on learnings, SortitionModuleUniversity doesn't track phases or total stake, so a no-op is the correct implementation here.
contracts/src/arbitration/interfaces/ISortitionModule.sol (1)
60-63: LGTM! Clean interface declaration with appropriate documentation.The function is well-placed adjacent to
validateStakeand has clear NatSpec describing its purpose.
| function test_setStake_totalStaked() public { | ||
| // Increase | ||
| vm.prank(staker1); | ||
| core.setStake(GENERAL_COURT, 4000); | ||
| vm.prank(staker1); | ||
| core.setStake(GENERAL_COURT, 5001); | ||
| vm.prank(staker2); | ||
| core.setStake(GENERAL_COURT, 1000); | ||
| vm.prank(staker2); | ||
| core.setStake(GENERAL_COURT, 1500); | ||
|
|
||
| assertEq(sortitionModule.totalStaked(), 6501, "Wrong totalStaked"); | ||
|
|
||
| // Decrease | ||
| vm.prank(staker1); | ||
| core.setStake(GENERAL_COURT, 3000); | ||
| vm.prank(staker1); | ||
| core.setStake(GENERAL_COURT, 2500); | ||
| vm.prank(staker2); | ||
| core.setStake(GENERAL_COURT, 1400); | ||
| vm.prank(staker2); | ||
| core.setStake(GENERAL_COURT, 1200); | ||
|
|
||
| assertEq(sortitionModule.totalStaked(), 3700, "Wrong totalStaked"); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# First, search for totalStaked references in the test file
rg -n "totalStaked" contracts/test/foundry/KlerosCore_Staking.t.sol -B2 -A2Repository: kleros/kleros-v2
Length of output: 7231
🏁 Script executed:
#!/bin/bash
# Search for tests that combine delayed stakes with totalStaked checks
rg -n "delayedStake.*totalStaked|totalStaked.*delayedStake" contracts/test/foundry/ -iRepository: kleros/kleros-v2
Length of output: 42
🏁 Script executed:
#!/bin/bash
# Search for all test functions that mention delayed stake
rg -n "function test.*[Dd]elayed" contracts/test/foundry/KlerosCore_Staking.t.solRepository: kleros/kleros-v2
Length of output: 182
🏁 Script executed:
#!/bin/bash
# Check the entire context around delayed stake tests to see if totalStaked is tested
rg -n "test_delayedStake" contracts/test/foundry/KlerosCore_Staking.t.sol -A 50 | head -100Repository: kleros/kleros-v2
Length of output: 42
🏁 Script executed:
#!/bin/bash
# Read the test_executeDelayedStakes function
sed -n '324,450p' contracts/test/foundry/KlerosCore_Staking.t.solRepository: kleros/kleros-v2
Length of output: 6960
🏁 Script executed:
#!/bin/bash
# Find the SortitionModule contract file
fd -t f "SortitionModule\.sol" contracts/Repository: kleros/kleros-v2
Length of output: 163
🏁 Script executed:
#!/bin/bash
# Search for the totalStaked function/variable definition
rg -n "totalStaked" contracts/src/ -B2 -A5 | head -100Repository: kleros/kleros-v2
Length of output: 7695
🏁 Script executed:
#!/bin/bash
# Search for executeDelayedStakes implementation
rg -n "function executeDelayedStakes" contracts/src/ -A 30Repository: kleros/kleros-v2
Length of output: 9796
🏁 Script executed:
#!/bin/bash
# Search for setStakeBySortitionModule in KlerosCore
rg -n "function setStakeBySortitionModule" contracts/src/ -A 15Repository: kleros/kleros-v2
Length of output: 3466
🏁 Script executed:
#!/bin/bash
# Confirm _setStake calls the totalStaked update
rg -n "function _setStake" contracts/src/arbitration/KlerosCore.sol -A 40 | head -60Repository: kleros/kleros-v2
Length of output: 2059
Test coverage for delayed stakes affecting totalStaked should be added.
The verification confirms the review comment's concern is valid. While the current test_setStake_totalStaked() correctly validates immediate stakes, there is a test coverage gap:
- Delayed stakes do NOT immediately update
sortitionModule.totalStaked()(they return early in_setStake()at line 1351) - When
executeDelayedStakes()is called during the Staking phase, it invokessetStakeBySortitionModule()which calls_setStake()with_noDelay=true, which then callsupdateTotalStake()to update the global total - The existing
test_executeDelayedStakes()validates delayed stake execution but never checks thatsortitionModule.totalStaked()is correctly updated after execution
A test case should verify that executing delayed stakes properly increments the global totalStaked metric.
🤖 Prompt for AI Agents
In contracts/test/foundry/KlerosCore_Staking.t.sol around lines 144 to 168, add
a test that verifies delayed stakes do not immediately affect
sortitionModule.totalStaked and that calling executeDelayedStakes during the
Staking phase updates totalStaked appropriately: create delayed stakes (use
callers that will trigger the delayed path), assert
sortitionModule.totalStaked() remains unchanged, advance the contract to the
Staking phase (or ensure executeDelayedStakes is callable), call
core.executeDelayedStakes(), then assert sortitionModule.totalStaked() equals
the expected new total after delayed stakes are applied.
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
contracts/test/foundry/KlerosCore_Execution.t.sol(3 hunks)
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2025-09-04T23:36:16.415Z
Learnt from: jaybuidl
Repo: kleros/kleros-v2 PR: 2126
File: contracts/src/arbitration/KlerosCore.sol:472-489
Timestamp: 2025-09-04T23:36:16.415Z
Learning: In this repo, KlerosCore emits AcceptedFeeToken and NewCurrencyRate events that are declared in contracts/src/arbitration/interfaces/IArbitratorV2.sol; implementations don’t need to redeclare these events.
Applied to files:
contracts/test/foundry/KlerosCore_Execution.t.sol
📚 Learning: 2024-11-19T05:31:48.701Z
Learnt from: Harman-singh-waraich
Repo: kleros/kleros-v2 PR: 1744
File: web/src/hooks/useGenesisBlock.ts:9-31
Timestamp: 2024-11-19T05:31:48.701Z
Learning: In `useGenesisBlock.ts`, within the `useEffect` hook, the conditions (`isKlerosUniversity`, `isKlerosNeo`, `isTestnetDeployment`) are mutually exclusive, so multiple imports won't execute simultaneously, and race conditions are not a concern.
Applied to files:
contracts/test/foundry/KlerosCore_Execution.t.sol
📚 Learning: 2025-09-30T17:18:12.895Z
Learnt from: jaybuidl
Repo: kleros/kleros-v2 PR: 2145
File: contracts/src/arbitration/dispute-kits/DisputeKitClassicBase.sol:277-286
Timestamp: 2025-09-30T17:18:12.895Z
Learning: In DisputeKitClassicBase.sol's castCommit function, jurors are allowed to re-submit commits during the commit period. The implementation uses a commitCount variable to track only first-time commits (where commit == bytes32(0)) so that totalCommitted is not incremented when a juror updates their existing commit.
Applied to files:
contracts/test/foundry/KlerosCore_Execution.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). (14)
- GitHub Check: Redirect rules - kleros-v2-testnet
- GitHub Check: Redirect rules - kleros-v2-testnet
- GitHub Check: Header rules - kleros-v2-testnet
- GitHub Check: Header rules - kleros-v2-testnet
- GitHub Check: Pages changed - kleros-v2-testnet
- GitHub Check: Pages changed - kleros-v2-testnet
- GitHub Check: Redirect rules - kleros-v2-testnet-devtools
- GitHub Check: Header rules - kleros-v2-testnet-devtools
- GitHub Check: Pages changed - kleros-v2-testnet-devtools
- GitHub Check: Redirect rules - kleros-v2-neo
- GitHub Check: Header rules - kleros-v2-neo
- GitHub Check: Pages changed - kleros-v2-neo
- GitHub Check: hardhat-tests
- GitHub Check: Analyze (javascript)
🔇 Additional comments (3)
contracts/test/foundry/KlerosCore_Execution.t.sol (3)
11-11: LGTM! Console import for test debugging.The forge-std console import is appropriate for test files and is used effectively to log
totalStakedvalues during test execution.
916-953: LGTM! Helper functions are well-structured.The helper functions
_stakeBalanceForJuror,_stakePnk_createDispute_moveToDrawingPhase,_drawJurors_advancePeriodToVoting, and_vote_executeare well-designed test utilities that properly encapsulate common test setup patterns. They improve test readability and reduce code duplication.
753-797: This test needs assertions to validate the fix or error expectations if documenting known buggy behavior.The test ends with
_stakeBalanceForJuror(staker2, 20000)(line 796) but provides no assertions afterward. The comment indicates staker2's staking should revert with overflow, yet there is novm.expectRevert()wrapper. Either add assertions confirmingtotalStakeddoes not overflow after the fix (if this validates the fix), or wrap the staker2 call withvm.expectRevert()if intentionally documenting the pre-fix bug. Additionally, remove or clarify the "post condition: inflated totalStaked" comment to reflect whether this test expects the bug to exist or to be fixed.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
contracts/src/arbitration/arbitrables/ArbitrableExample.sol (1)
151-163: Missingoverridekeyword—this function will not compile.When implementing an interface function that should also be overridable by derived contracts, the function must be declared as
external virtual override. Theoverridekeyword is mandatory in Solidity when implementing interface functions. The current code is missing it.- function rule(uint256 _arbitratorDisputeID, uint256 _ruling) external virtual { + function rule(uint256 _arbitratorDisputeID, uint256 _ruling) external virtual override {
♻️ Duplicate comments (2)
contracts/test/foundry/KlerosCore_Execution.t.sol (2)
922-936: Critical: Variable shadowing makes all assertions useless.This issue was flagged in a previous review. The tuple destructuring on lines 930-931 redeclares variables with the same names as the function parameters, causing all assertions to compare values against themselves (always passing).
Apply this diff to fix the variable shadowing:
function _assertJurorBalance( uint256 disputeID, address juror, - uint256 totalStakedPnk, - uint256 totalLocked, - uint256 stakedInCourt, - uint256 nbCourts + uint256 expectedTotalStakedPnk, + uint256 expectedTotalLocked, + uint256 expectedStakedInCourt, + uint256 expectedNbCourts ) internal { (uint256 totalStakedPnk, uint256 totalLocked, uint256 stakedInCourt, uint256 nbCourts) = sortitionModule .getJurorBalance(juror, GENERAL_COURT); - assertEq(totalStakedPnk, totalStakedPnk, "Wrong totalStakedPnk"); - assertEq(totalLocked, totalLocked, "Wrong totalLocked"); - assertEq(stakedInCourt, stakedInCourt, "Wrong stakedInCourt"); - assertEq(nbCourts, nbCourts, "Wrong nbCourts"); + assertEq(totalStakedPnk, expectedTotalStakedPnk, "Wrong totalStakedPnk"); + assertEq(totalLocked, expectedTotalLocked, "Wrong totalLocked"); + assertEq(stakedInCourt, expectedStakedInCourt, "Wrong stakedInCourt"); + assertEq(nbCourts, expectedNbCourts, "Wrong nbCourts"); }
815-819: Incomplete test: Missing revert expectation or assertions.This issue was flagged in a previous review. The comment on line 816-817 states that staker2's stake "reverts with arithmetic underflow or overflow," but line 818 calls
_stakeBalanceForJuror(staker2, 20000)withoutvm.expectRevert()and without subsequent assertions.If the stake should revert:
// new juror tries to stake but totalStaked already reached type(uint256).max - // it reverts with "arithmetic underflow or overflow (0x11)" + vm.expectRevert(); // Expect arithmetic overflow _stakeBalanceForJuror(staker2, 20000);If the stake should succeed, add appropriate assertions instead.
🧹 Nitpick comments (1)
contracts/src/test/MaliciousArbitrableMock.sol (1)
36-49: Consider callingsuper.rule()to avoid code duplication.The
rule()function duplicates the parent's implementation. While acceptable for a test mock, you could simplify:function rule(uint256 _arbitratorDisputeID, uint256 _ruling) external override { if (doRevert) revert RuleReverted(); - - uint256 localDisputeID = externalIDtoLocalID[_arbitratorDisputeID]; - DisputeStruct storage dispute = disputes[localDisputeID]; - if (msg.sender != address(arbitrator)) revert ArbitratorOnly(); - if (_ruling > dispute.numberOfRulingOptions) revert RulingOutOfBounds(); - if (dispute.isRuled) revert DisputeAlreadyRuled(); - - dispute.isRuled = true; - dispute.ruling = _ruling; - - emit Ruling(IArbitratorV2(msg.sender), _arbitratorDisputeID, dispute.ruling); + ArbitrableExample.rule(_arbitratorDisputeID, _ruling); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
contracts/src/arbitration/KlerosCore.sol(1 hunks)contracts/src/arbitration/SortitionModule.sol(2 hunks)contracts/src/arbitration/arbitrables/ArbitrableExample.sol(1 hunks)contracts/src/arbitration/dispute-kits/DisputeKitClassicBase.sol(1 hunks)contracts/src/test/MaliciousArbitrableMock.sol(1 hunks)contracts/test/foundry/KlerosCore_Execution.t.sol(7 hunks)
🧰 Additional context used
🧠 Learnings (6)
📚 Learning: 2025-09-04T23:36:16.415Z
Learnt from: jaybuidl
Repo: kleros/kleros-v2 PR: 2126
File: contracts/src/arbitration/KlerosCore.sol:472-489
Timestamp: 2025-09-04T23:36:16.415Z
Learning: In this repo, KlerosCore emits AcceptedFeeToken and NewCurrencyRate events that are declared in contracts/src/arbitration/interfaces/IArbitratorV2.sol; implementations don’t need to redeclare these events.
Applied to files:
contracts/test/foundry/KlerosCore_Execution.t.solcontracts/src/arbitration/dispute-kits/DisputeKitClassicBase.sol
📚 Learning: 2024-11-19T05:31:48.701Z
Learnt from: Harman-singh-waraich
Repo: kleros/kleros-v2 PR: 1744
File: web/src/hooks/useGenesisBlock.ts:9-31
Timestamp: 2024-11-19T05:31:48.701Z
Learning: In `useGenesisBlock.ts`, within the `useEffect` hook, the conditions (`isKlerosUniversity`, `isKlerosNeo`, `isTestnetDeployment`) are mutually exclusive, so multiple imports won't execute simultaneously, and race conditions are not a concern.
Applied to files:
contracts/test/foundry/KlerosCore_Execution.t.sol
📚 Learning: 2025-09-30T17:18:12.895Z
Learnt from: jaybuidl
Repo: kleros/kleros-v2 PR: 2145
File: contracts/src/arbitration/dispute-kits/DisputeKitClassicBase.sol:277-286
Timestamp: 2025-09-30T17:18:12.895Z
Learning: In DisputeKitClassicBase.sol's castCommit function, jurors are allowed to re-submit commits during the commit period. The implementation uses a commitCount variable to track only first-time commits (where commit == bytes32(0)) so that totalCommitted is not incremented when a juror updates their existing commit.
Applied to files:
contracts/test/foundry/KlerosCore_Execution.t.solcontracts/src/arbitration/dispute-kits/DisputeKitClassicBase.sol
📚 Learning: 2024-10-21T10:32:16.970Z
Learnt from: Harman-singh-waraich
Repo: kleros/kleros-v2 PR: 1703
File: kleros-sdk/src/utils/getDispute.ts:38-40
Timestamp: 2024-10-21T10:32:16.970Z
Learning: The variables 'arbitrableChainID' and 'externalDisputeID' are required by the context to have uppercase 'ID', so they should remain unchanged even if the corresponding source properties use 'Id'.
Applied to files:
contracts/src/arbitration/arbitrables/ArbitrableExample.sol
📚 Learning: 2024-11-19T16:31:08.965Z
Learnt from: jaybuidl
Repo: kleros/kleros-v2 PR: 1746
File: contracts/config/courts.v2.mainnet-neo.json:167-170
Timestamp: 2024-11-19T16:31:08.965Z
Learning: In `contracts/config/courts.v2.mainnet-neo.json`, the `minStake` parameter is denominated in PNK, not ETH.
Applied to files:
contracts/src/arbitration/SortitionModule.sol
📚 Learning: 2025-09-03T22:48:32.972Z
Learnt from: jaybuidl
Repo: kleros/kleros-v2 PR: 0
File: :0-0
Timestamp: 2025-09-03T22:48:32.972Z
Learning: In the Kleros v2 codebase, the team prioritizes gas optimization over strict CEI pattern compliance when dealing with trusted contracts. For penalty execution logic, they prefer batching storage writes (`round.pnkPenalties`) rather than updating incrementally after each penalty calculation to save gas costs, as the risk is extremely low between trusted contracts.
Applied to files:
contracts/src/arbitration/dispute-kits/DisputeKitClassicBase.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). (14)
- 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: Redirect rules - kleros-v2-testnet-devtools
- GitHub Check: Header rules - kleros-v2-testnet
- GitHub Check: Header rules - kleros-v2-neo
- GitHub Check: Pages changed - kleros-v2-testnet
- GitHub Check: hardhat-tests
- GitHub Check: Pages changed - kleros-v2-testnet
- GitHub Check: Header rules - kleros-v2-testnet-devtools
- GitHub Check: Analyze (javascript)
- GitHub Check: Pages changed - kleros-v2-neo
- GitHub Check: Pages changed - kleros-v2-testnet-devtools
🔇 Additional comments (7)
contracts/src/arbitration/SortitionModule.sol (2)
385-406: LGTM: totalStaked bookkeeping correctly moved to _setStake.The refactoring properly updates
totalStakedduring actual state mutations rather than during validation. The increment/decrement logic correctly mirrors thejuror.stakedPnkchanges.
457-466: LGTM: totalStaked properly decremented in withdrawLeftoverPNK.The
totalStakeddecrement correctly accounts for leftover PNK withdrawals, maintaining consistency with the juror'sstakedPnkbeing zeroed.contracts/src/arbitration/dispute-kits/DisputeKitClassicBase.sol (1)
473-481: LGTM: Period-based check improves robustness.Replacing the
isRuledcheck with aPeriod.executioncheck allows contributors to withdraw fees even if a malicious arbitrable contract reverts inrule(). This prevents griefing attacks where contributors could be permanently blocked from withdrawing.contracts/test/foundry/KlerosCore_Execution.t.sol (2)
101-103: LGTM: Assertions validate totalStaked tracking.Good addition of assertions to verify
totalStakedequals the core's PNK balance after staking and execution phases.
487-512: LGTM: Transfer failure test path added.The test properly validates that
withdrawLeftoverPNKreverts withTransferFailedwhen the core contract lacks sufficient balance, then succeeds after balance is restored.contracts/src/test/MaliciousArbitrableMock.sol (1)
1-52: LGTM: Test mock for simulating malicious arbitrable behavior.The mock correctly extends
ArbitrableExampleand provides a controllable revert mechanism for testing scenarios whererule()fails.contracts/src/arbitration/KlerosCore.sol (1)
627-631: LGTM: Explicit transfer failure handling added.The
SafeERC20.safeTransferin this codebase returns a boolean (rather than reverting on failure), making the return value check appropriate. Adding the explicit revert withTransferFailed()ensures consistent error handling when the sortition module transfers PNK to jurors.
|
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
contracts/test/foundry/KlerosCore_Execution.t.sol (1)
870-900: Test is still incomplete / contradictory vs PR goal (stake call has no assertion).
The final step_stakeBalanceForJuror(staker2, 20000);has neithervm.expectRevert(...)nor post-assertions. Also, the comment says staking should revert becausetotalStakedhittype(uint256).max, but the PR objective suggests the opposite: ensuretotalStakeddoes not get inflated by a failing delayed stake.If the intended post-condition is “not inflated, so staker2 can stake”, tighten it like:
- // new juror tries to stake but totalStaked already reached type(uint256).max - // it reverts with "arithmetic underflow or overflow (0x11)" _stakeBalanceForJuror(staker2, 20000); + assertEq(sortitionModule.totalStaked(), amountToStake + 20000, "totalStaked should not be inflated");If the intended post-condition is actually “still reverts”, then add an explicit revert expectation instead.
🧹 Nitpick comments (5)
contracts/src/libraries/SafeERC20.sol (1)
18-23: Consider indexing event fields for better onchain filtering.
SafeTransferFailedis helpful, but withoutindexedparams it’s harder to query efficiently (especially_token/_to).- event SafeTransferFailed(IERC20 _token, address _to, uint256 _value); + event SafeTransferFailed(IERC20 indexed _token, address indexed _to, uint256 _value);contracts/test/foundry/KlerosCore_Execution.t.sol (4)
5-12: Prefer importingSafeERC20from the library file, not viaKlerosCore.sol.
ImportingSafeERC20through../../src/arbitration/KlerosCore.solcreates unnecessary coupling between tests and the core source unit composition.-import {KlerosCore, SafeERC20} from "../../src/arbitration/KlerosCore.sol"; +import {KlerosCore} from "../../src/arbitration/KlerosCore.sol"; +import {SafeERC20} from "../../src/libraries/SafeERC20.sol";
566-617: Strengthentest_execute_feeToken_failedTransfer: assert accounting doesn’t “pay” on failed transfers.
Right now it only assertsrepartitionsand the receiver’s balance. If fee transfers fail butsumFeeRewardPaidstill increments, funds can become unclaimable (or accounting becomes misleading).Suggested additions (exact expected values depend on intended semantics):
core.execute(disputeID, 0, 6); KlerosCore.Round memory round = core.getRoundInfo(disputeID, 0); assertEq(round.repartitions, 6, "Wrong repartitions"); + assertEq(round.sumFeeRewardPaid, 0, "sumFeeRewardPaid should not increase if transfers failed"); + assertEq(feeToken.balanceOf(address(core)), 0, "Core still has no fee token balance"); assertEq(feeToken.balanceOf(staker1), 0, "Staker1 still has no balance");
842-845: Remove commented-out code or replace with an assertion of irrelevance.
IfexecuteRuling()is intentionally irrelevant for withdrawals now, prefer asserting the condition that makes it irrelevant (e.g., DK considers the dispute resolved inPeriod.execution) rather than leaving a commented call.
1001-1055: Helpers are useful; consider removingconsole.lognoise and clarifying expected-vs-actual naming.
The helpers improve readability, but the logs add test noise, and_assertJurorBalancereads a bit “reversed” (expected params vs actual locals). Consider renaming locals toactual*(or expected params toexpected*) and dropping logs once stabilized.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
contracts/src/libraries/SafeERC20.sol(2 hunks)contracts/test/foundry/KlerosCore_Execution.t.sol(10 hunks)
🧰 Additional context used
🧠 Learnings (4)
📓 Common learnings
Learnt from: jaybuidl
Repo: kleros/kleros-v2 PR: 2107
File: contracts/src/arbitration/university/KlerosCoreUniversity.sol:1083-1092
Timestamp: 2025-09-03T19:34:58.056Z
Learning: KlerosCoreUniversity and SortitionModuleUniversity do not have phases, unlike KlerosCoreBase and SortitionModuleBase. Therefore, validateStake in the University contracts will never return StakingResult.Delayed, only Successful or other failure states.
Learnt from: jaybuidl
Repo: kleros/kleros-v2 PR: 0
File: :0-0
Timestamp: 2025-09-03T22:48:32.972Z
Learning: In the Kleros v2 codebase, the team prioritizes gas optimization over strict CEI pattern compliance when dealing with trusted contracts. For penalty execution logic, they prefer batching storage writes (`round.pnkPenalties`) rather than updating incrementally after each penalty calculation to save gas costs, as the risk is extremely low between trusted contracts.
Learnt from: jaybuidl
Repo: kleros/kleros-v2 PR: 2127
File: contracts/src/arbitration/university/KlerosCoreUniversity.sol:856-856
Timestamp: 2025-09-11T01:00:13.355Z
Learning: In the Kleros v2 codebase, precision loss from divide-before-multiply patterns is acceptable in reward calculation contexts (specifically in KlerosCoreUniversity reward distribution logic). The team has made a conscious decision to prioritize gas optimization or code simplicity over perfect precision in these calculations.
Learnt from: jaybuidl
Repo: kleros/kleros-v2 PR: 2145
File: contracts/src/arbitration/dispute-kits/DisputeKitClassicBase.sol:277-286
Timestamp: 2025-09-30T17:18:12.895Z
Learning: In DisputeKitClassicBase.sol's castCommit function, jurors are allowed to re-submit commits during the commit period. The implementation uses a commitCount variable to track only first-time commits (where commit == bytes32(0)) so that totalCommitted is not incremented when a juror updates their existing commit.
Learnt from: jaybuidl
Repo: kleros/kleros-v2 PR: 2126
File: contracts/src/arbitration/KlerosCore.sol:472-489
Timestamp: 2025-09-04T23:36:16.415Z
Learning: In this repo, KlerosCore emits AcceptedFeeToken and NewCurrencyRate events that are declared in contracts/src/arbitration/interfaces/IArbitratorV2.sol; implementations don’t need to redeclare these events.
📚 Learning: 2025-09-04T23:36:16.415Z
Learnt from: jaybuidl
Repo: kleros/kleros-v2 PR: 2126
File: contracts/src/arbitration/KlerosCore.sol:472-489
Timestamp: 2025-09-04T23:36:16.415Z
Learning: In this repo, KlerosCore emits AcceptedFeeToken and NewCurrencyRate events that are declared in contracts/src/arbitration/interfaces/IArbitratorV2.sol; implementations don’t need to redeclare these events.
Applied to files:
contracts/test/foundry/KlerosCore_Execution.t.sol
📚 Learning: 2025-09-30T17:18:12.895Z
Learnt from: jaybuidl
Repo: kleros/kleros-v2 PR: 2145
File: contracts/src/arbitration/dispute-kits/DisputeKitClassicBase.sol:277-286
Timestamp: 2025-09-30T17:18:12.895Z
Learning: In DisputeKitClassicBase.sol's castCommit function, jurors are allowed to re-submit commits during the commit period. The implementation uses a commitCount variable to track only first-time commits (where commit == bytes32(0)) so that totalCommitted is not incremented when a juror updates their existing commit.
Applied to files:
contracts/test/foundry/KlerosCore_Execution.t.sol
📚 Learning: 2024-11-19T05:31:48.701Z
Learnt from: Harman-singh-waraich
Repo: kleros/kleros-v2 PR: 1744
File: web/src/hooks/useGenesisBlock.ts:9-31
Timestamp: 2024-11-19T05:31:48.701Z
Learning: In `useGenesisBlock.ts`, within the `useEffect` hook, the conditions (`isKlerosUniversity`, `isKlerosNeo`, `isTestnetDeployment`) are mutually exclusive, so multiple imports won't execute simultaneously, and race conditions are not a concern.
Applied to files:
contracts/test/foundry/KlerosCore_Execution.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). (8)
- GitHub Check: Redirect rules - kleros-v2-testnet
- GitHub Check: Redirect rules - kleros-v2-testnet
- GitHub Check: Header rules - kleros-v2-testnet
- GitHub Check: Header rules - kleros-v2-testnet
- GitHub Check: Pages changed - kleros-v2-testnet
- GitHub Check: Pages changed - kleros-v2-testnet
- GitHub Check: hardhat-tests
- GitHub Check: Analyze (javascript)
🔇 Additional comments (6)
contracts/src/libraries/SafeERC20.sol (1)
38-43:safeTransferchange looks correct; note added log-gas on failures.
Computingok, emitting on failure, and returningokis a clean improvement and keeps the non-reverting semantics.contracts/test/foundry/KlerosCore_Execution.t.sol (5)
101-103: Nice invariant assertion for this scenario (core balance == totalStaked).
Good additional signal thattotalStakedbookkeeping stays aligned in this test’s flow.
149-154: Good: post-execution invariants help catchtotalStakedregressions.
The “shouldn’t change after rewards” checks are a solid guardrail for the refactor.
487-508: Good negative test: withdrawal should revert when core can’t transfer PNK, and state is checked before/after.
The manual balance-drain +TransferFailedexpectation is a clear regression test for the new transfer handling.
546-547: Double-check the vote choice change is intentional for this fee-token path.
This changes the coherence/ruling dynamics; if it’s only meant to be “any valid vote”, consider a short comment why1(not2) is correct here.
721-761: Good coverage: arbitrable revert should not brick withdrawals.
This is a valuable regression test for “rule() can revert” scenarios.
PR-Codex overview
This PR primarily focuses on enhancing the
KlerosCoreandSortitionModulecontracts with improved error handling, new functionality for staking, and the addition of a mock contract for testing purposes. It also refines theSafeERC20library for better transfer failure management.Detailed summary
KlerosCoreto revert on failed token transfers.ArbitrableExamplewith virtualrulefunction.DisputeKitClassicBaseto check dispute periods before resolving.KlerosCore_Staking.SafeERC20with an event for transfer failures.SortitionModuleto manage total stakes accurately.MaliciousArbitrableMockfor testing reverts in rules.KlerosCore_ExecutionTestfor fee handling and dispute execution.Summary by CodeRabbit
Bug Fixes
Tests
✏️ Tip: You can customize this high-level summary in your review settings.