PNK penalties applied to the sortition tree stakes #2107
Conversation
✅ 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 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. |
WalkthroughAdds per-drawn-juror origin tracking, changes draw() to return a subcourt ID, refactors sortition internals to use 32-byte stakePathIDs, introduces setStakePenalty/setStakeReward and targeted/forced-unstake APIs, and updates stake/penalty/unstake flows and tests to apply penalties against the juror’s originating court. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Core as KlerosCore
participant DK as DisputeKit
participant SM as SortitionModule
Note over Core,SM: Draw flow with origin tracking
Core->>DK: draw(disputeID, nonce)
DK->>SM: draw(courtKey, disputeID, nonce)
SM-->>DK: (drawnAddress, fromSubcourtID)
DK-->>Core: (drawnAddress, fromSubcourtID)
Core->>Core: round.drawnJurorFromCourtIDs.push(originID)
rect rgba(200,230,255,0.18)
Note over Core,SM: Penalty applied to origin subcourt
Core->>SM: setStakePenalty(account, originCourtID, penalty)
SM-->>Core: (pnkBalance, newCourtStake, appliedPenalty)
Core->>Core: pnkPenaltiesInRound += appliedPenalty
alt pnkBalance == 0 or vote inactive
Core->>SM: forcedUnstakeAllCourts(account)
else newCourtStake < minStake(originCourtID)
Core->>SM: forcedUnstake(account, originCourtID)
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
✨ 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 (
|
❌ Deploy Preview for kleros-v2-university 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: 2
🧹 Nitpick comments (5)
contracts/src/arbitration/SortitionModuleBase.sol (2)
20-24: Bounded per-leaf subcourt accounting looks good—watch storage growthThe packing with fixed MAX_STAKE_PATHS keeps per-stakePath storage bounded. After the fixes above, this should remain gas-predictable. Consider adding a brief natspec on how index slots are reused to aid auditors.
526-533: Docstring alignment: clarify “stakedInCourt” is the effective stake in that courtThe code returns stakeOf(_juror, _courtID), i.e., the leaf value in that court’s tree (direct stake plus propagated subcourt amounts, minus penalties). Add one sentence: “stakedInCourt is the juror’s effective stake for the specified court.”
contracts/test/arbitration/dispute-kit-gated.ts (1)
110-114: Consider removing hardcoded gasLimit unless needed for flaky networksThe explicit gasLimit on setStake has helped previously, but Hardhat auto-estimation is usually sufficient and keeps tests resilient across optimizer changes. If this isn’t masking an issue, drop it; otherwise, leave a short comment stating why 500000 is required.
contracts/src/arbitration/dispute-kits/DisputeKitClassicBase.sol (1)
229-253: draw passthrough looks correct; consider storing fromSubcourtID when neededThe function now returns (address, uint96) and short-circuits on zero address. If any downstream logic will depend on juror origin in DK, consider storing fromSubcourtID alongside Vote for richer analytics. Not required for this PR.
contracts/src/arbitration/university/SortitionModuleUniversity.sol (1)
201-211: Consider returning the actual PNK balance after penalty application.The function currently returns
currentStakeas the PNK balance, but this doesn't reflect the balance after the penalty is applied. Consider returningnewStakeinstead to provide accurate post-penalty balance information.function setStakePenalty( address _account, uint96 _courtID, uint256 _penalty ) external override onlyByCore returns (uint256 pnkBalance, uint256 availablePenalty) { uint256 currentStake = _stakeOf(_account, _courtID); availablePenalty = _penalty; if (currentStake < _penalty) { availablePenalty = currentStake; } - if (availablePenalty == 0) return (currentStake, availablePenalty); // No penalty to apply. + if (availablePenalty == 0) return (currentStake, availablePenalty); // No penalty to apply. uint256 newStake = currentStake - availablePenalty; _setStake(_account, _courtID, 0, availablePenalty, newStake); + return (newStake, availablePenalty); }
📜 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/hardhat.config.ts(1 hunks)contracts/src/arbitration/KlerosCoreBase.sol(3 hunks)contracts/src/arbitration/SortitionModuleBase.sol(10 hunks)contracts/src/arbitration/dispute-kits/DisputeKitClassicBase.sol(2 hunks)contracts/src/arbitration/interfaces/IDisputeKit.sol(1 hunks)contracts/src/arbitration/interfaces/ISortitionModule.sol(1 hunks)contracts/src/arbitration/university/KlerosCoreUniversity.sol(3 hunks)contracts/src/arbitration/university/SortitionModuleUniversity.sol(2 hunks)contracts/test/arbitration/dispute-kit-gated.ts(1 hunks)contracts/test/foundry/KlerosCore.t.sol(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2024-11-19T16:31:08.965Z
Learnt from: jaybuidl
PR: kleros/kleros-v2#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/university/SortitionModuleUniversity.solcontracts/src/arbitration/SortitionModuleBase.sol
📚 Learning: 2024-11-19T16:31:08.965Z
Learnt from: jaybuidl
PR: kleros/kleros-v2#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/SortitionModuleBase.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: SonarCloud
- GitHub Check: contracts-testing
- GitHub Check: Analyze (javascript)
- GitHub Check: Redirect rules - kleros-v2-testnet-devtools
- GitHub Check: Redirect rules - kleros-v2-neo
- GitHub Check: Redirect rules - kleros-v2-testnet
- GitHub Check: Redirect rules - kleros-v2-testnet
- GitHub Check: Header rules - kleros-v2-testnet-devtools
- GitHub Check: Header rules - kleros-v2-neo
- GitHub Check: Header rules - kleros-v2-testnet
- GitHub Check: Header rules - kleros-v2-testnet
- GitHub Check: Pages changed - kleros-v2-testnet-devtools
- GitHub Check: Pages changed - kleros-v2-neo
- GitHub Check: Pages changed - kleros-v2-testnet
- GitHub Check: Pages changed - kleros-v2-testnet
🔇 Additional comments (17)
contracts/hardhat.config.ts (1)
33-35: Action Required: Parameterize Solidity optimizer runs and verify gas/size trade-offsPlease update your Hardhat config to make
runsenv-configurable, and then perform a local comparison of bytecode size and gas usage for both 2000 and 10000 runs to confirm this change is intentional.• File:
contracts/hardhat.config.ts, lines 33–35- runs: 2000, + runs: Number(process.env.SOLC_OPT_RUNS ?? 2000),• Near the top of the same file (with other env-driven settings):
const SOLC_OPT_RUNS = Number(process.env.SOLC_OPT_RUNS ?? 2000);Rationale:
Lowering optimizer runs from 10000 → 2000 reduces bytecode size and speeds up compilation but can increase runtime gas on hot functions likedrawandsetStakePenalty. Making this value configurable preserves flexibility across networks/environments.Manual verification steps (run locally):
- Compile & generate size reports with
runs=2000vs.runs=10000.- Benchmark gas for
drawandsetStakePenaltyunder both settings.- Review any regressions; confirm whether the trade-off is acceptable.
Once verified, please share the results before merging.
contracts/src/arbitration/SortitionModuleBase.sol (1)
476-502: Draw range attribution can be skewed if subcourt totals are staleThe draw logic assumes subcourtStakes.totalStakedInCourts stays consistent with the sum of stakedInCourts. With the current _updateSubCourtStakes deletion bug (see above), totals can be overstated, forcing currentCourtStake to zero and always entering the subcourt path. Fixing _updateSubCourtStakes as proposed restores the expected split between “direct stake in current court” and “from subcourts.”
Run your Foundry tests that cover draw attribution across stake increases/decreases in child courts after applying the patch to confirm round.drawnJurorFromCourtIDs distribution remains unchanged except for corrected edge cases.
contracts/test/arbitration/dispute-kit-gated.ts (1)
115-121: Assertion targets match the new getJurorBalance semanticsGood to see the expectations reflect totalStaked and stakedInCourt including penalties. No action needed.
contracts/src/arbitration/interfaces/IDisputeKit.sol (1)
51-55: Alldraw()definitions updated—please verify call sitesI ran a repository-wide search for any
function draw(uint256 _coreDisputeID, uint256 _nonce) public|externaldeclarations missing the newuint96return and found none.• No stale
drawimplementations withoutuint96remain.
• Next, confirm every invocation in KlerosCoreBase (and university variants) as well as all Dispute Kit contracts unpacks and uses both returned values:
- e.g. change
toaddress drawn = disputeKit.draw(id, nonce);(address drawn, uint96 fromSubcourtID) = disputeKit.draw(id, nonce);This ensures the extra
uint96subcourt ID isn’t silently dropped.contracts/test/foundry/KlerosCore.t.sol (1)
2523-2529: Event sequence comment is helpful; verify penalties flow via setStakePenalty (not penalizeStake)The two StakeSet events for newCourtID and subsequent GENERAL_COURT update reflect the new per-subcourt penalty path. After applying the setStakePenalty fix, these expectations should still hold. If they shift, adjust comments to mirror applied vs leftover penalty semantics.
contracts/src/arbitration/university/KlerosCoreUniversity.sol (3)
62-62: LGTM! Good addition for subcourt tracking.The new field
drawnJurorFromCourtIDsproperly tracks the origin subcourt of each drawn juror, which enables penalties to be applied at the correct court level.
603-610: Correctly handles the two-value return from dispute kit draw.The code properly destructures the new return value
(address drawnAddress, uint96 fromSubcourtID)from the dispute kit's draw function and appropriately stores the subcourt ID for penalty tracking.
779-784: Good implementation of subcourt-specific penalty application.The penalty is now correctly applied to the specific subcourt where the juror was drawn from, using the tracked
drawnJurorFromCourtIDsdata. This maintains the intended penalty behavior while enabling proper subcourt attribution.contracts/src/arbitration/interfaces/ISortitionModule.sol (2)
32-36: LGTM! Well-designed penalty API.The new
setStakePenaltyfunction signature properly replaces the oldpenalizeStakewith subcourt-specific penalty application. The return values provide useful feedback about the actual penalty applied.
46-50: Good enhancement to the draw function return type.Adding
fromSubcourtIDto the return values enables proper tracking of where stakes originate, which is essential for the penalty system to work correctly with subcourt stakes.contracts/src/arbitration/university/SortitionModuleUniversity.sol (4)
193-194: Clean refactoring to centralize stake logic.Good design decision to delegate to the internal
_setStakefunction, promoting code reuse and maintainability.
196-211: Well-implemented penalty application with proper bounds checking.The function correctly:
- Caps the penalty to the current stake to prevent underflow
- Returns early if no penalty can be applied
- Delegates to
_setStakewith appropriate parameters
213-255: Excellent implementation of the centralized stake update logic.The function properly handles:
- Court ID management when staking/unstaking
- Stake propagation through parent courts
- Proper cleanup when stake becomes zero
- Event emission with complete information
304-310: Simple but correct implementation for educational purposes.The function appropriately returns the transient juror address while leaving the subcourt ID uninitialized (defaults to 0), which is sufficient for the educational context.
contracts/src/arbitration/KlerosCoreBase.sol (3)
63-63: LGTM! Consistent with University contract.The addition of
drawnJurorFromCourtIDsfield maintains consistency across both the base and university implementations.
614-621: Proper handling of draw return values in the production contract.The implementation correctly destructures the new return values and stores the subcourt ID with appropriate fallback to the dispute's court ID when
fromSubcourtIDis 0.
791-796: Consistent penalty application using subcourt tracking.The penalty system correctly uses the tracked subcourt ID to apply penalties at the appropriate court level, maintaining consistency with the University implementation.
jaybuidl
added
Type: Bug 🐛
Package: Contracts
jaybuidl
force-pushed
the
fix/penalties-reduce-drawing-odds
branch
from
58eb32c to
a48f402
Compare
jaybuidl
force-pushed
the
fix/penalties-reduce-drawing-odds
branch
from
a48f402 to
f2b2183
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
contracts/src/arbitration/SortitionModuleBase.sol (3)
370-381: Do not record “subcourt 0” contributions; they pollute accounting and skew draw ranges.
fromSubCourtIDstarts at 0 for the directly staked court and is then set tocurrentCourtIDwhen moving upward. Feeding0into_updateSubCourtStakescauses the direct stake of the current court to be mis-accounted as a “subcourt”, makingcurrentCourtStakeartificially zero indraw.Guard
_fromSubCourtID == 0in_updateSubCourtStakes(see diff below).
315-335: setStakePenalty incorrectly applies and reports penalties; can double-subtract and break cascading.Echoing the earlier review: the function (a) never sets
pnkBalanceon all paths, (b) returns the clamped input instead of the true leftover, and (c) passes the fullavailablePenaltyas_pnkWithdrawaleven when the current court stake is smaller, which over-reducesjuror.stakedPnkcompared to what was actually removed from this court.Apply only the amount actually taken from this court, return the leftover for cascading, and set
pnkBalanceto the updated total.Apply this diff:
function setStakePenalty( address _account, uint96 _courtID, uint256 _penalty ) external override onlyByCore returns (uint256 pnkBalance, uint256 availablePenalty) { - Juror storage juror = jurors[_account]; - availablePenalty = _penalty; - if (juror.stakedPnk < _penalty) { - availablePenalty = juror.stakedPnk; - } - - if (availablePenalty == 0) return (juror.stakedPnk, 0); // No penalty to apply. - - uint256 currentStake = stakeOf(_account, _courtID); - uint256 newStake = 0; - if (currentStake >= availablePenalty) { - newStake = currentStake - availablePenalty; - } - _setStake(_account, _courtID, 0, availablePenalty, newStake); - pnkBalance = juror.stakedPnk; // updated by _setStake() + Juror storage juror = jurors[_account]; + // Clamp by total staked balance + uint256 requested = _penalty; + uint256 maxApplicable = juror.stakedPnk < requested ? juror.stakedPnk : requested; + if (maxApplicable == 0) return (juror.stakedPnk, 0); // Nothing to penalize. + + // Apply against this court first + uint256 currentStake = stakeOf(_account, _courtID); + uint256 applied = maxApplicable > currentStake ? currentStake : maxApplicable; + uint256 newStake = currentStake - applied; // applied <= currentStake + _setStake(_account, _courtID, 0, applied, newStake); + + // Report leftover to allow caller to cascade to other courts + availablePenalty = requested - applied; + pnkBalance = juror.stakedPnk; // updated inside _setStake }
593-627: _updateSubCourtStakes undercounts on deletion and should ignore “ID 0”.Two problems:
- When
_value == 0you delete the slot but don’t subtract the previous value fromtotalStakedInCourts, leaving stale totals._fromSubCourtID == 0represents “no subcourt” and should be ignored; otherwise, direct stakes are consumed as if they came from a subcourt and distort draw odds.Apply this diff:
function _updateSubCourtStakes( SubCourtStakes storage _subcourtStakes, uint256 _value, uint96 _fromSubCourtID ) internal { + // Do not treat direct stake as a subcourt contribution. + if (_fromSubCourtID == 0) return; // Update existing slot for (uint256 i = 0; i < MAX_STAKE_PATHS; i++) { if (_subcourtStakes.courtIDs[i] == _fromSubCourtID) { if (_value == 0) { - delete _subcourtStakes.courtIDs[i]; - delete _subcourtStakes.stakedInCourts[i]; + uint256 prev = _subcourtStakes.stakedInCourts[i]; + if (prev > 0) { + _subcourtStakes.totalStakedInCourts -= prev; + } + delete _subcourtStakes.stakedInCourts[i]; + delete _subcourtStakes.courtIDs[i]; } else { - _subcourtStakes.totalStakedInCourts += _value; - _subcourtStakes.totalStakedInCourts -= _subcourtStakes.stakedInCourts[i]; - _subcourtStakes.stakedInCourts[i] = _value; + uint256 prev = _subcourtStakes.stakedInCourts[i]; + if (_value >= prev) { + _subcourtStakes.totalStakedInCourts += (_value - prev); + } else { + _subcourtStakes.totalStakedInCourts -= (prev - _value); + } + _subcourtStakes.stakedInCourts[i] = _value; } return; } } // Add a new slot for (uint256 i = 0; i < MAX_STAKE_PATHS; i++) { if (_subcourtStakes.courtIDs[i] == 0) { _subcourtStakes.courtIDs[i] = _fromSubCourtID; - _subcourtStakes.totalStakedInCourts += _value; - _subcourtStakes.totalStakedInCourts -= _subcourtStakes.stakedInCourts[i]; - _subcourtStakes.stakedInCourts[i] = _value; + _subcourtStakes.stakedInCourts[i] = _value; + _subcourtStakes.totalStakedInCourts += _value; return; } } }
🧹 Nitpick comments (4)
contracts/src/arbitration/interfaces/ISortitionModule.sol (2)
32-37: Define and standardize setStakePenalty return semantics (and document them).The tuple
(uint256 pnkBalance, uint256 availablePenalty)is ambiguous. Please specify in NatSpec whetheravailablePenaltyis the leftover (unapplied) portion or the portion applied in this call. Current downstream code (Core/University) expects a leftover to cascade across courts. Also, consider renaming toleftoverPenaltyfor clarity across implementations. At minimum, add NatSpec that makes this explicit and consistent with the base/university modules.Would you like me to add NatSpec comments to this interface and align both SortitionModuleBase/University implementations?
46-50: Keep parameter naming consistent with other tree APIs.
draw(bytes32 _court, ...)uses_courtwhere the rest of the API uses_keyfor the tree key (e.g.,createTree(bytes32 _key, ...)). Recommend renaming to_keyfor consistency (or update comments to clarify that_courtis the sortition tree key).If you prefer to keep
_court, please confirm all call sites treat it as the tree key and not a numeric court ID casted ad-hoc.contracts/test/foundry/KlerosCore.t.sol (1)
2524-2526: Tests assert transient StakeSet sequences; consider asserting final state or decoding logs to reduce brittleness.The double StakeSet expectation for
newCourtID(19000→39000 then 0→20000) tightly couples the test to internal emission order and intermediate balances. Minor refactors in penalty flow could make this flaky while preserving correct end-state.
- Option: assert only the final balances via
getJurorBalanceandpinakion.balanceOf.- Option: use Foundry's
recordLogs()/getRecordedLogs()to decode and assert presence of at least the final StakeSet, not the exact sequence.contracts/src/arbitration/university/KlerosCoreUniversity.sol (1)
603-615: Draw path correctly captures origin; minor guard suggestion.Flow looks good: you fetch
(drawnAddress, fromSubcourtID), lock stake, push juror, and recordfromSubcourtID != 0 ? fromSubcourtID : dispute.courtID. Consider adding a lightweight assert to ensuredrawnJurorFromCourtIDs.length == drawnJurors.lengthafter push to prevent accidental desync in future edits (debug-only or via an invariant test).
📜 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/hardhat.config.ts(1 hunks)contracts/src/arbitration/KlerosCoreBase.sol(3 hunks)contracts/src/arbitration/SortitionModuleBase.sol(10 hunks)contracts/src/arbitration/dispute-kits/DisputeKitClassicBase.sol(2 hunks)contracts/src/arbitration/interfaces/IDisputeKit.sol(1 hunks)contracts/src/arbitration/interfaces/ISortitionModule.sol(1 hunks)contracts/src/arbitration/university/KlerosCoreUniversity.sol(3 hunks)contracts/src/arbitration/university/SortitionModuleUniversity.sol(2 hunks)contracts/test/arbitration/dispute-kit-gated.ts(1 hunks)contracts/test/foundry/KlerosCore.t.sol(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (6)
- contracts/src/arbitration/dispute-kits/DisputeKitClassicBase.sol
- contracts/hardhat.config.ts
- contracts/test/arbitration/dispute-kit-gated.ts
- contracts/src/arbitration/KlerosCoreBase.sol
- contracts/src/arbitration/university/SortitionModuleUniversity.sol
- contracts/src/arbitration/interfaces/IDisputeKit.sol
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2024-11-19T16:31:08.965Z
Learnt from: jaybuidl
PR: kleros/kleros-v2#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/SortitionModuleBase.sol
📚 Learning: 2024-11-19T16:31:08.965Z
Learnt from: jaybuidl
PR: kleros/kleros-v2#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/SortitionModuleBase.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-devtools
- GitHub Check: Redirect rules - kleros-v2-neo
- GitHub Check: Redirect rules - kleros-v2-testnet
- GitHub Check: Header rules - kleros-v2-testnet
- GitHub Check: Header rules - kleros-v2-testnet-devtools
- GitHub Check: Header rules - kleros-v2-neo
- GitHub Check: Header rules - kleros-v2-testnet
- GitHub Check: Pages changed - kleros-v2-testnet-devtools
- GitHub Check: Pages changed - kleros-v2-testnet
- GitHub Check: Pages changed - kleros-v2-neo
- GitHub Check: Pages changed - kleros-v2-testnet
- GitHub Check: contracts-testing
- GitHub Check: Analyze (javascript)
🔇 Additional comments (6)
contracts/src/arbitration/university/KlerosCoreUniversity.sol (2)
62-63: LGTM: track origin court for each drawn juror.Storing
drawnJurorFromCourtIDsaligns the penalty path to the juror’s origin, enabling subcourt-aware penalties in execution.
779-785: Penalty routed to origin court: add bounds safety.Indexing
round.drawnJurorFromCourtIDs[_params.repartition]assumes the array is in-sync withdrawnJurors. Given prior pushes occur in tandem, this is fine, but a defensive check (or a revert in dispute kit if lengths diverge) would make this future-proof.Would you like a quick invariant test that asserts
drawnJurorFromCourtIDs.length == drawnJurors.lengthacross all draws?contracts/src/arbitration/SortitionModuleBase.sol (4)
20-24: Subcourt accounting structure is sound; ensure invariants are maintained.The
SubCourtStakesdesign and per-treeIDsToSubCourtStakesmapping enable origin attribution. Please ensure the invariant holds:totalStakedInCourts == sum(stakedInCourts[i])at all times. See comments below on_updateSubCourtStakeswhich currently breaks this invariant on deletion and for “ID 0”.I can add a fuzz/invariant test to validate the equality across stake updates and removals.
Also applies to: 29-34
447-503: Draw: origin resolution logic is OK; relies on accurate subcourt totals.The residual
currentDrawnNumberis correctly used within the selected leaf. However, correctness depends ontotalStakedInCourtsbeing maintained precisely. See fix below for_updateSubCourtStakesto avoid under/over-counting during updates and deletions.I recommend adding a property test that repeatedly stakes/unstakes across nested courts and checks that:
currentCourtStake + sum(subcourtStakes) == leafValuefromSubcourtIDis stable across repeated draws with a fixed RNG seed.
666-738: Feed only meaningful subcourt markers into subcourt accounting.After applying the fix above,
_set(..., _fromSubCourtID)will correctly ignore0and only account for actual subcourt contributions as you walk up the tree. This keepscurrentCourtStakeaccurate indraw.
527-548: getJurorBalance doc aligns with implementation.The doc now states totals include locked tokens and penalty deductions. The return uses
juror.stakedPnkandstakeOfwhich matches the intended semantics. Good.
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
contracts/src/arbitration/SortitionModuleBase.sol (1)
599-626: Critical: subcourt totals drift and “ID 0” should not be tracked as a subcourt.Two correctness bugs:
- When
_value == 0, you delete slots but don’t subtract the previous amount fromtotalStakedInCourts(totals drift upward → biased draws).fromSubCourtID == 0means “direct stake in current court” and must be ignored in subcourt tracking. Recording it erroneously zeroes current-court range.Apply:
function _updateSubCourtStakes( SubCourtStakes storage _subcourtStakes, uint256 _value, uint96 _fromSubCourtID ) internal { + // Do not account direct stake as a "subcourt" contribution. + if (_fromSubCourtID == 0) return; // Update existing stake item if found for (uint256 i = 0; i < MAX_STAKE_PATHS; i++) { if (_subcourtStakes.courtIDs[i] == _fromSubCourtID) { if (_value == 0) { - delete _subcourtStakes.courtIDs[i]; - delete _subcourtStakes.stakedInCourts[i]; + uint256 prev = _subcourtStakes.stakedInCourts[i]; + if (prev > 0) { + _subcourtStakes.totalStakedInCourts -= prev; + } + delete _subcourtStakes.stakedInCourts[i]; + delete _subcourtStakes.courtIDs[i]; } else { - _subcourtStakes.totalStakedInCourts += _value; - _subcourtStakes.totalStakedInCourts -= _subcourtStakes.stakedInCourts[i]; - _subcourtStakes.stakedInCourts[i] = _value; + uint256 prev = _subcourtStakes.stakedInCourts[i]; + if (_value >= prev) { + _subcourtStakes.totalStakedInCourts += (_value - prev); + } else { + _subcourtStakes.totalStakedInCourts -= (prev - _value); + } + _subcourtStakes.stakedInCourts[i] = _value; } return; } } // Not found so add a new stake item for (uint256 i = 0; i < MAX_STAKE_PATHS; i++) { if (_subcourtStakes.courtIDs[i] == 0) { _subcourtStakes.courtIDs[i] = _fromSubCourtID; - _subcourtStakes.totalStakedInCourts += _value; - _subcourtStakes.stakedInCourts[i] = _value; + _subcourtStakes.stakedInCourts[i] = _value; + _subcourtStakes.totalStakedInCourts += _value; return; } } }
🧹 Nitpick comments (4)
contracts/src/arbitration/university/KlerosCoreUniversity.sol (1)
603-615: Propagate origin subcourt from DK.draw and persist fallback to dispute court.Capturing
(drawnAddress, fromSubcourtID)and storingfromSubcourtID != 0 ? fromSubcourtID : dispute.courtIDis correct given University SM may return 0 when it can’t resolve subcourt. Please document in DK/SM interfaces thatfromSubcourtID == 0denotes a direct stake in the current dispute court.Also applies to: 611-611
contracts/src/arbitration/interfaces/ISortitionModule.sol (2)
32-36: Clarify return semantics for penalties.Rename or document that the second return value from
setStakePenaltyis the “applied penalty” (the portion actually deducted in_courtID), not the leftover. If renaming is too invasive, add NatSpec explicitly stating this.- ) external returns (uint256 pnkBalance, uint256 availablePenalty); + ) external returns (uint256 pnkBalance, uint256 appliedPenalty); + /// @return appliedPenalty The portion of `_penalty` that was actually deducted in `_courtID`.
46-51: DefinefromSubcourtID == 0semantics in the interface.Callers rely on 0 meaning “drawn from the current court’s direct stake.” Add a one-liner to avoid ambiguity across implementations.
- ) external view returns (address drawnAddress, uint96 fromSubcourtID); + ) external view returns (address drawnAddress, uint96 fromSubcourtID); + /// @return fromSubcourtID 0 if selected from the current court's direct stake; otherwise the originating subcourt ID.contracts/src/arbitration/university/SortitionModuleUniversity.sol (1)
309-315: University draw doesn’t setfromSubcourtID(always 0) — confirm intent.Returning 0 is acceptable if the educational module can’t resolve subcourts. Core falls back to
dispute.courtID. Please add a comment or explicitreturn (drawnAddress, 0)to make this intentional.- ) public view override returns (address drawnAddress, uint96 fromSubcourtID) { - drawnAddress = transientJuror; - } + ) public view override returns (address drawnAddress, uint96 fromSubcourtID) { + drawnAddress = transientJuror; + fromSubcourtID = 0; // University module: origin subcourt not tracked + }
📜 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/hardhat.config.ts(1 hunks)contracts/src/arbitration/KlerosCoreBase.sol(3 hunks)contracts/src/arbitration/SortitionModuleBase.sol(10 hunks)contracts/src/arbitration/dispute-kits/DisputeKitClassicBase.sol(2 hunks)contracts/src/arbitration/interfaces/IDisputeKit.sol(1 hunks)contracts/src/arbitration/interfaces/ISortitionModule.sol(1 hunks)contracts/src/arbitration/university/KlerosCoreUniversity.sol(3 hunks)contracts/src/arbitration/university/SortitionModuleUniversity.sol(2 hunks)contracts/test/arbitration/dispute-kit-gated.ts(1 hunks)contracts/test/foundry/KlerosCore.t.sol(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- contracts/hardhat.config.ts
- contracts/test/arbitration/dispute-kit-gated.ts
- contracts/src/arbitration/dispute-kits/DisputeKitClassicBase.sol
- contracts/src/arbitration/interfaces/IDisputeKit.sol
- contracts/test/foundry/KlerosCore.t.sol
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2024-11-19T16:31:08.965Z
Learnt from: jaybuidl
PR: kleros/kleros-v2#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/SortitionModuleBase.sol
📚 Learning: 2024-11-19T16:31:08.965Z
Learnt from: jaybuidl
PR: kleros/kleros-v2#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/SortitionModuleBase.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). (13)
- 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: 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: contracts-testing
🔇 Additional comments (10)
contracts/src/arbitration/university/KlerosCoreUniversity.sol (2)
62-62: Track origin court per drawn juror — aligns with penalty routing.Adding
drawnJurorFromCourtIDskeeps round-local provenance for penalties. Ensure tests assertdrawnJurorFromCourtIDs.length == drawnJurors.lengthafter drawing completes to guard against skew.
771-785: Penalty routing depends on SM.setStakePenalty returning “applied” amount, not “leftover.”The accounting here assumes the second return value from
setStakePenaltyis the amount actually deducted (and thus distributable). Verify the SM implementation returns the applied penalty limited to stake inpenalizedInCourtID. See my comments in SortitionModuleBase/University.contracts/src/arbitration/KlerosCoreBase.sol (2)
63-64: Round now tracks per-juror origin court IDs — good foundation for per-court penalties.Change is minimal and compatible with existing getters (returned via
getRoundInfo).
614-622: Draw: origin propagation is correct and bounded.Using the DK’s
(address,uint96)and pushing the origin ID with a 0→current court fallback matches the new penalty semantics. Loop indexing (startIndex + i++) is consistent withdrawIterations.contracts/src/arbitration/interfaces/ISortitionModule.sol (1)
52-56: getJurorBalance signature shift noted.Good to pass
_courtIDexplicitly. Ensure downstream callers in tooling/tests are updated.contracts/src/arbitration/university/SortitionModuleUniversity.sol (1)
247-260: Stake propagation up the court tree looks correct for this simplified module.Using the same
(newStake - currentStake)delta per ancestor keeps parents consistent with the leaf stake. Event emission includes updated total PNK.contracts/src/arbitration/SortitionModuleBase.sol (4)
20-24: Per-subcourt stake tracking is the right abstraction.
SubCourtStakes{total, courtIDs[], stakedInCourts[]}provides the data needed to attribute draws to subcourts without blowing up storage thanks toMAX_STAKE_PATHS.
31-34: Typed mappings improve clarity and reduce collision risk.Replacing generic node/ID maps with typed stakePathID mappings is a nice readability win.
447-503: Draw origin calculation is sound — contingent on accurate subcourt totals.Using the residual
currentDrawnNumberwithin the leaf to pick between current court and subcourt ranges is correct. This relies on_updateSubCourtStakesproperly maintainingtotalStakedInCourts; please fix that as above to avoid skewed odds.
527-548: Updated juror balance view matches new semantics.Returning “effective” stake via the tree plus locked amount separately is consistent with the new API.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
contracts/src/arbitration/SortitionModuleBase.sol (2)
326-346: Critical: setStakePenalty over-deducts global stakedPnk and returns the wrong amount.
- If currentStake < _penalty, you still pass the full availablePenalty as _pnkWithdrawal, reducing juror.stakedPnk by more than the stake you actually remove from this court.
- You return availablePenalty as the clamped request, not the applied amount, contradicting the docstring.
This breaks invariants between per-court stakes and juror.stakedPnk and confuses callers about what was actually deducted.
Apply this fix (compute applied, update only that, return applied):
function setStakePenalty( address _account, uint96 _courtID, uint256 _penalty ) external override onlyByCore returns (uint256 pnkBalance, uint256 availablePenalty) { - Juror storage juror = jurors[_account]; - availablePenalty = _penalty; - if (juror.stakedPnk < _penalty) { - availablePenalty = juror.stakedPnk; - } - - if (availablePenalty == 0) return (juror.stakedPnk, 0); // No penalty to apply. - - uint256 currentStake = stakeOf(_account, _courtID); - uint256 newStake = 0; - if (currentStake >= availablePenalty) { - newStake = currentStake - availablePenalty; - } - _setStake(_account, _courtID, 0, availablePenalty, newStake); - pnkBalance = juror.stakedPnk; // Updated by _setStake(). + Juror storage juror = jurors[_account]; + if (_penalty == 0 || juror.stakedPnk == 0) return (juror.stakedPnk, 0); + uint256 currentStake = stakeOf(_account, _courtID); + if (currentStake == 0) return (juror.stakedPnk, 0); // Nothing to penalize in this court. + // Clamp by both court stake and overall staked balance + uint256 maxByBalance = juror.stakedPnk < _penalty ? juror.stakedPnk : _penalty; + uint256 applied = currentStake < maxByBalance ? currentStake : maxByBalance; + uint256 newStake = currentStake - applied; // applied <= currentStake + _setStake(_account, _courtID, 0, applied, newStake); + pnkBalance = juror.stakedPnk; // updated inside _setStake + availablePenalty = applied; // return the actually deducted amount }Run to confirm call-sites expect “applied” (not “leftover”):
#!/bin/bash # Inspect how setStakePenalty's second return is used. rg -n -C3 --type=sol '\bsetStakePenalty\s*\('
604-637: Fix subcourt accounting: deletion doesn’t decrement totals, and ID 0 should be ignored.
- When _value == 0, you delete the slot without subtracting its previous amount from totalStakedInSubCourts — totals become stale and can exceed the leaf value.
- You also record _fromSubCourtID == 0 as a subcourt, which pushes “direct stake” into the subcourt pool, setting currentCourtStake to zero and corrupting draw attribution.
Apply this correction:
function _updateSubCourtStakes( SubCourtStakes storage _subcourtStakes, uint256 _value, uint96 _fromSubCourtID ) internal { - // Update existing stake item if found + // Do not track direct stake of the current court as a "subcourt". + if (_fromSubCourtID == 0) return; + // Update existing stake item if found for (uint256 i = 0; i < MAX_STAKE_PATHS; i++) { if (_subcourtStakes.subCourtIDs[i] == _fromSubCourtID) { if (_value == 0) { - delete _subcourtStakes.subCourtIDs[i]; - delete _subcourtStakes.stakedInSubCourts[i]; + uint256 prev = _subcourtStakes.stakedInSubCourts[i]; + if (prev > 0) { + _subcourtStakes.totalStakedInSubCourts -= prev; + } + delete _subcourtStakes.stakedInSubCourts[i]; + delete _subcourtStakes.subCourtIDs[i]; } else { - _subcourtStakes.totalStakedInSubCourts += _value; - _subcourtStakes.totalStakedInSubCourts -= _subcourtStakes.stakedInSubCourts[i]; - _subcourtStakes.stakedInSubCourts[i] = _value; + uint256 prev = _subcourtStakes.stakedInSubCourts[i]; + if (_value >= prev) { + _subcourtStakes.totalStakedInSubCourts += (_value - prev); + } else { + _subcourtStakes.totalStakedInSubCourts -= (prev - _value); + } + _subcourtStakes.stakedInSubCourts[i] = _value; } return; } } // Not found so add a new stake item for (uint256 i = 0; i < MAX_STAKE_PATHS; i++) { if (_subcourtStakes.subCourtIDs[i] == 0) { _subcourtStakes.subCourtIDs[i] = _fromSubCourtID; - _subcourtStakes.totalStakedInSubCourts += _value; - _subcourtStakes.stakedInSubCourts[i] = _value; + _subcourtStakes.stakedInSubCourts[i] = _value; + _subcourtStakes.totalStakedInSubCourts += _value; return; } } }
🧹 Nitpick comments (5)
contracts/src/arbitration/SortitionModuleBase.sol (5)
20-24: Struct shape LGTM; keep naming consistent across codebase.Solid layout with bounded arrays by MAX_STAKE_PATHS. Minor: you mix “SubCourt”/“Subcourt” casing in different places; consider standardizing to one form for readability.
315-325: Docs mismatch: “reward deposit” vs penalty; clarify return meaning.
- The devdoc says “with a PNK reward deposit” but the function performs a penalty.
- The name availablePenalty suggests “remaining/leftover”, while the doc text says “actually deducted”.
Please align wording so integrators don’t misinterpret the return value.
Apply this doc-only diff:
-/// @dev Update the state of the stakes with a PNK reward deposit, called by KC during rewards execution. +/// @dev Apply a PNK penalty to a juror's stake in a specific court, called by KC during rewards execution. ... -/// @return availablePenalty The amount of PNK that was actually deducted. +/// @return availablePenalty The amount of PNK that was actually deducted (applied) in this call.
381-389: Don’t track “0” as a subcourt; it pollutes accounting.fromSubCourtID = 0 is a sentinel (“not from a subcourt”). Ensure downstream accounting ignores it to avoid zero-ID entries crowding the fixed-size arrays and skewing totals. You can either skip calling _updateSubCourtStakes when 0, or early-return in that function. See dedicated fix below.
458-514: Draw origin logic is sound, but relies on a strict invariant — protect against drift.
- Logic assumes totalStakedInSubCourts never exceeds the leaf value. With the current _updateSubCourtStakes implementation, deletions can underflow the “current court stake” and force every draw into a subcourt path.
- Once you ignore ID 0 and fix deletions (see below), consider also skipping any entries where subCourtIDs[i] == 0 for extra safety.
Optional robustness tweak (safe skip of zero IDs):
- for (uint256 i = 0; i < MAX_STAKE_PATHS; i++) { - if (subcourtStakes.stakedInSubCourts[i] > 0) { + for (uint256 i = 0; i < MAX_STAKE_PATHS; i++) { + if (subcourtStakes.subCourtIDs[i] != 0 && subcourtStakes.stakedInSubCourts[i] > 0) { if (currentDrawnNumber < accumulatedStake + subcourtStakes.stakedInSubCourts[i]) { fromSubcourtID = subcourtStakes.subCourtIDs[i]; break; } accumulatedStake += subcourtStakes.stakedInSubCourts[i]; } }You can add a Foundry test that:
- stakes directly in a child and parent,
- applies penalties that zero-out the child,
- draws repeatedly to ensure fromSubcourtID toggles between 0 and child as expected and never returns 0 due to a “fake” subcourt entry.
672-748: Only update subcourt metadata when applicable; small gas save.Once _updateSubCourtStakes ignores zero IDs, this is mostly handled. You can optionally skip the call altogether when _fromSubCourtID == 0 to save a few SLOADs.
- _updateSubCourtStakes(tree.IDsToSubCourtStakes[_ID], _value, _fromSubCourtID); + if (_fromSubCourtID != 0) { + _updateSubCourtStakes(tree.IDsToSubCourtStakes[_ID], _value, _fromSubCourtID); + }
📜 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 (1)
contracts/src/arbitration/SortitionModuleBase.sol(10 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2024-11-19T16:31:08.965Z
Learnt from: jaybuidl
PR: kleros/kleros-v2#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/SortitionModuleBase.sol
📚 Learning: 2024-11-19T16:31:08.965Z
Learnt from: jaybuidl
PR: kleros/kleros-v2#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/SortitionModuleBase.sol
🔇 Additional comments (3)
contracts/src/arbitration/SortitionModuleBase.sol (3)
31-34: Typed mappings refactor: good improvement.The explicit key names (stakePathID/nodeIndex) improve readability and reduce class of bugs from generic mappings.
45-45: Comment update matches new semantics.stakedPnk now representing net balance (including locked and penalties) is aligned with penalty flow.
538-559: getJurorBalance doc + behavior alignment looks good.This now reports stakedInCourt after penalties, which downstream UIs will expect when visualizing effective odds.
jaybuidl
force-pushed
the
fix/penalties-reduce-drawing-odds
branch
from
139fe76 to
59781de
Compare
…ourt origination logic
jaybuidl
force-pushed
the
fix/penalties-reduce-drawing-odds
branch
from
59781de to
1aae950
Compare
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/university/KlerosCoreUniversity.sol (1)
806-812: Do not use .send without checking the return value; use a checked call or reuse the Base helper.
sendreturns false on failure and silently drops funds; it also has a 2300-gas stipend constraint. Switch to a checked call (or mirror KlerosCoreBase’s_transferFeeTokenhelper using SafeSend).- if (round.feeToken == NATIVE_CURRENCY) { - // The dispute fees were paid in ETH - payable(governor).send(round.totalFeesForJurors); - } else { - // The dispute fees were paid in ERC20 - round.feeToken.safeTransfer(governor, round.totalFeesForJurors); - } + if (round.feeToken == NATIVE_CURRENCY) { + (bool ok, ) = payable(governor).call{value: round.totalFeesForJurors}(""); + if (!ok) revert TransferFailed(); + } else { + round.feeToken.safeTransfer(governor, round.totalFeesForJurors); + }
♻️ Duplicate comments (4)
contracts/src/arbitration/university/KlerosCoreUniversity.sol (1)
779-785: Core relies on setStakePenalty returning the applied amount; verify module implementations.
_params.pnkPenaltiesInRoundis incremented byavailablePenalty. This must be the penalty actually deducted frompenalizedInCourtID. If SortitionModule returns the requested amount (or clamps by total staked PNK) instead of the applied per-court amount, the round will over-credit penalties and corrupt balances.Confirm both SortitionModuleBase and SortitionModuleUniversity return the applied-per-court amount and do not over-withdraw beyond that court’s stake. If not, fix modules accordingly.
#!/bin/bash # Inspect setStakePenalty implementations for correct "applied" semantics. rg -n -C3 --type=sol "function setStakePenalty\(" rg -n -C5 --type=sol "setStakePenalty\(" contracts/src/arbitration/SortitionModuleBase.sol contracts/src/arbitration/university/SortitionModuleUniversity.solAlso applies to: 796-802
contracts/src/arbitration/KlerosCoreBase.sol (1)
791-797: Ensure setStakePenalty returns applied-per-court penalty; otherwise penalties are over-credited.Here
_params.pnkPenaltiesInRoundincreases byavailablePenalty. This must equal the amount actually deducted frompenalizedInCourtID. If the SortitionModule returns the requested amount or clamps by total PNK rather than court stake, accounting breaks.Action: verify/fix SortitionModuleBase/University to (a) cap by the stake in
penalizedInCourtIDand (b) return the applied amount. Optional: add an assertion in tests thatavailablePenalty <= previous stake in penalizedInCourtID.#!/bin/bash # Quick sanity: look for availablePenalty semantics in SM modules. rg -n -C3 --type=sol "availablePenalty" contracts/src/arbitration/SortitionModuleBase.sol contracts/src/arbitration/university/SortitionModuleUniversity.solAlso applies to: 808-814
contracts/src/arbitration/university/SortitionModuleUniversity.sol (1)
196-218: Critical: setStakePenalty can over-withdraw and returns the wrong amount.Issues:
- It subtracts
_penaltyfrom the juror’s total PNK even when the stake in_courtIDis smaller.- It returns
availablePenaltyas a clamp by total staked PNK, not the amount actually applied to_courtID.- Core assumes the returned amount is what was applied to the specified court for redistribution and follow-up logic.
Fix: apply only
applied = min(_penalty, juror.stakedPnk, currentStakeInCourt), reduce only that court byapplied, and return(updatedPNKBalance, updatedCourtStake, applied).function setStakePenalty( address _account, uint96 _courtID, uint256 _penalty ) external override onlyByCore returns (uint256 pnkBalance, uint256 newCourtStake, uint256 availablePenalty) { - Juror storage juror = jurors[_account]; - availablePenalty = _penalty; - newCourtStake = _stakeOf(_account, _courtID); - if (juror.stakedPnk < _penalty) { - availablePenalty = juror.stakedPnk; - } - - if (availablePenalty == 0) return (juror.stakedPnk, newCourtStake, 0); // No penalty to apply. - - uint256 currentStake = _stakeOf(_account, _courtID); - uint256 newStake = 0; - if (currentStake >= availablePenalty) { - newStake = currentStake - availablePenalty; - } - _setStake(_account, _courtID, 0, availablePenalty, newStake); - pnkBalance = juror.stakedPnk; // updated by _setStake() - newCourtStake = _stakeOf(_account, _courtID); // updated by _setStake() + Juror storage juror = jurors[_account]; + uint256 currentStake = _stakeOf(_account, _courtID); + if (_penalty == 0 || juror.stakedPnk == 0 || currentStake == 0) { + return (juror.stakedPnk, currentStake, 0); + } + uint256 maxByBalance = _penalty <= juror.stakedPnk ? _penalty : juror.stakedPnk; + uint256 applied = currentStake < maxByBalance ? currentStake : maxByBalance; + uint256 newStake = currentStake - applied; // applied <= currentStake + _setStake(_account, _courtID, 0, applied, newStake); + pnkBalance = juror.stakedPnk; // updated by _setStake() + newCourtStake = newStake; // updated court stake + availablePenalty = applied; // amount actually deducted (applied) }contracts/src/arbitration/SortitionModuleBase.sol (1)
321-343: Critical: setStakePenalty over-deducts and reports the wrong amount.Same issues as the university module:
- Over-deducts from
juror.stakedPnkwhen the stake in_courtIDis smaller than_penalty.- Returns a clamped request, not the amount applied to
_courtID.Fix to apply only
applied = min(_penalty, juror.stakedPnk, stakeOf(_account, _courtID)), update that court’s stake, and return(updatedPNKBalance, updatedCourtStake, applied).function setStakePenalty( address _account, uint96 _courtID, uint256 _penalty ) external override onlyByCore returns (uint256 pnkBalance, uint256 newCourtStake, uint256 availablePenalty) { - Juror storage juror = jurors[_account]; - availablePenalty = _penalty; - newCourtStake = stakeOf(_account, _courtID); - if (juror.stakedPnk < _penalty) { - availablePenalty = juror.stakedPnk; - } - - if (availablePenalty == 0) return (juror.stakedPnk, newCourtStake, 0); // No penalty to apply. - - uint256 currentStake = stakeOf(_account, _courtID); - uint256 newStake = 0; - if (currentStake >= availablePenalty) { - newStake = currentStake - availablePenalty; - } - _setStake(_account, _courtID, 0, availablePenalty, newStake); - pnkBalance = juror.stakedPnk; // updated by _setStake() - newCourtStake = stakeOf(_account, _courtID); // updated by _setStake() + Juror storage juror = jurors[_account]; + uint256 currentStake = stakeOf(_account, _courtID); + if (_penalty == 0 || juror.stakedPnk == 0 || currentStake == 0) { + return (juror.stakedPnk, currentStake, 0); + } + uint256 maxByBalance = _penalty <= juror.stakedPnk ? _penalty : juror.stakedPnk; + uint256 applied = currentStake < maxByBalance ? currentStake : maxByBalance; + uint256 newStake = currentStake - applied; + _setStake(_account, _courtID, 0, applied, newStake); + pnkBalance = juror.stakedPnk; // updated inside _setStake + newCourtStake = newStake; // updated court stake + availablePenalty = applied; // amount actually deducted (applied) }
🧹 Nitpick comments (9)
contracts/src/arbitration/interfaces/ISortitionModule.sol (2)
32-37: Clarify return semantics of setStakePenalty to prevent mis-implementations.“availablePenalty” must be the amount actually deducted from the specified court (applied penalty), not the requested amount nor a clamp by total staked balance. Several module implementations tend to get this wrong, which corrupts accounting in Core’s execute path.
Please tighten the NatSpec to make this explicit and avoid future regressions.
Apply this diff to the signature comment to improve clarity (name change optional; ABI unaffected):
- function setStakePenalty( + /// @notice Apply a PNK penalty against the juror's stake in _courtID. + /// @dev Returns the updated total PNK balance (pnkBalance), the updated stake in _courtID (newCourtStake), + /// and the amount actually deducted from _courtID (availablePenalty == applied penalty). + function setStakePenalty( address _account, uint96 _courtID, uint256 _penalty - ) external returns (uint256 pnkBalance, uint256 newCourtStake, uint256 availablePenalty); + ) external returns (uint256 pnkBalance, uint256 newCourtStake, uint256 availablePenalty /* applied */);
46-51: Parameter naming: _court is a bytes32 tree key, not a court ID.For readability and to reduce confusion with uint96 court IDs elsewhere, consider renaming
_courtto_treeKey(or similar).- function draw( - bytes32 _court, + function draw( + bytes32 _treeKey, uint256 _coreDisputeID, uint256 _nonce ) external view returns (address drawnAddress, uint96 fromSubcourtID);contracts/src/arbitration/university/KlerosCoreUniversity.sol (2)
62-62: Keep array invariants in sync: drawnJurors and drawnJurorFromCourtIDs must match lengths 1:1.You already push both in draw(). Consider adding lightweight invariants in tests (or internal sanity checks) to ensure
round.drawnJurorFromCourtIDs.length == round.drawnJurors.lengthacross state transitions.
744-747: Minor CEI improvement opportunity.The comment flags CEI, but this store happens after external calls inside the loop. If feasible, consider moving the persistent write of
round.pnkPenaltiesearlier or isolating state updates per iteration to reduce reentrancy surface. Not a blocker given the overall flow, just a note.contracts/src/arbitration/KlerosCoreBase.sol (1)
756-759: Minor CEI improvement opportunity.As in the university contract, consider updating
round.pnkPenaltiesbefore external transfers and module calls, or otherwise contain updates per-iteration. Not critical; flow appears safe with SafeSend/ERC20 guards.contracts/src/arbitration/university/SortitionModuleUniversity.sol (1)
311-317: OK for educational scope: fromSubcourtID intentionally omitted.Returning only the transient juror and zero
fromSubcourtIDis acceptable here because the caller falls back todispute.courtID. Consider addingfromSubcourtID = 0;explicitly for readability.contracts/src/arbitration/SortitionModuleBase.sol (3)
308-320: NatSpec mismatch: this is a penalty, not a “PNK reward deposit”.The docstring says “reward deposit” but the function deducts PNK. Update wording and clarify that
availablePenaltyis the amount actually deducted from the specified court.- /// @dev Update the state of the stakes with a PNK reward deposit, called by KC during rewards execution. + /// @dev Apply a PNK penalty against the juror's stake in a specific court, called by KC during penalties execution. @@ - /// @param _penalty The amount of PNK to be deducted. - /// @return pnkBalance The updated total PNK balance of the juror, including the penalty. - /// @return newCourtStake The updated stake of the juror in the court. - /// @return availablePenalty The amount of PNK that was actually deducted. + /// @param _penalty The requested penalty amount to deduct (will be capped). + /// @return pnkBalance The updated total PNK balance of the juror after deduction. + /// @return newCourtStake The updated stake of the juror in the specified court after deduction. + /// @return availablePenalty The amount actually deducted from the specified court (applied penalty).
498-506: Minor: prefer uint256 for consistency.Use
uint256fortreeIndexto stay consistent with the rest of the codebase.- uint treeIndex = tree.IDsToNodeIndexes[_stakePathID]; + uint256 treeIndex = tree.IDsToNodeIndexes[_stakePathID];
669-695: Assembly: document endianness and guarantee zeroing for readability.The pack/unpack are correct but a brief comment about big-endian byte indexing and “low 20/12 bytes” would help future maintainers. Optionally, zero the unused high bytes before mload for clarity (gas impact negligible here).
Also applies to: 701-728
📜 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 (5)
contracts/src/arbitration/KlerosCoreBase.sol(4 hunks)contracts/src/arbitration/SortitionModuleBase.sol(12 hunks)contracts/src/arbitration/interfaces/ISortitionModule.sol(1 hunks)contracts/src/arbitration/university/KlerosCoreUniversity.sol(4 hunks)contracts/src/arbitration/university/SortitionModuleUniversity.sol(2 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2024-11-19T16:31:08.965Z
Learnt from: jaybuidl
PR: kleros/kleros-v2#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/SortitionModuleBase.sol
🔇 Additional comments (4)
contracts/src/arbitration/university/KlerosCoreUniversity.sol (1)
603-611: LGTM: origin tracking wired correctly into draw flow.Pulling
(drawnAddress, fromSubcourtID)from DK and falling back todispute.courtIDwhen zero matches the intended semantics.contracts/src/arbitration/KlerosCoreBase.sol (2)
63-63: LGTM: per-juror origin tracking added to Round.This unlocks per-origin penalty routing. Ensure tests assert length parity with
drawnJurors.
614-622: LGTM: DK draw tuple integration and origin fallback.The draw loop and fallback to
dispute.courtIDwhenfromSubcourtID == 0are correct.contracts/src/arbitration/SortitionModuleBase.sol (1)
452-458: LGTM: two-way mapping to stakePathID and draw tuple return.Resolving
(address, courtID)fromstakePathIDat the leaf is clean and keeps ABI changes localized.Also applies to: 482-484
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
contracts/test/foundry/KlerosCore_Execution.t.sol (2)
275-278: Event sequence matches per-origin penalties; consider de-brittling and asserting origin.
- The two StakeSet expectations for
newCourtIDcorrectly reflect: (1) applying the first 1k PNK penalty tonewCourtID(20000→19000; total 40000→39000) and (2) then zeroing that court’s stake due to inactivity (19000→0; total 39000→20000). This aligns with origin-based penalties.- To reduce brittleness from batching, run the first iteration separately so these three StakeSet events are deterministically the next emitted events, then finish the remaining iterations. This prevents future refactors from interleaving additional events ahead of these expectations.
Diff (minimal change here; add a follow-up execute right after):
- core.execute(disputeID, 0, 3); + core.execute(disputeID, 0, 1);Then, immediately after these expectations (before the final asserts), finish the batch:
core.execute(disputeID, 0, 2);
- Optional: explicitly assert the draw origin for voteID 0 to harden the linkage between penalty and subcourt (if exposed on Round):
KlerosCoreBase.Round memory r = core.getRoundInfo(disputeID, 0); assertEq(r.drawnJurorFromCourtIDs[0], newCourtID, "voteID 0 should originate from newCourtID");Also applies to: 280-280
273-273: Fix small comment typo.s/her/here/.
- // Note that these events are emitted only after the first iteration of execute() therefore the juror has been penalized only for 1000 PNK her. + // Note that these events are emitted only after the first iteration of execute(), therefore the juror has been penalized only for 1000 PNK here.
📜 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 (1)
contracts/test/foundry/KlerosCore_Execution.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). (14)
- 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: 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: contracts-testing
- GitHub Check: Analyze (javascript)
… PNK to the juror
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
contracts/src/arbitration/university/KlerosCoreUniversity.sol (2)
800-817: Unsafe ETH transfer with .send — funds may be stuck or lost.send returns bool and is ignored; failures silently drop rewards. Use a robust transfer helper (like base’s _transferFeeToken) with call-and-check.
Apply this diff:
- if (round.feeToken == NATIVE_CURRENCY) { - // The dispute fees were paid in ETH - payable(owner).send(round.totalFeesForJurors); - } else { - // The dispute fees were paid in ERC20 - round.feeToken.safeTransfer(owner, round.totalFeesForJurors); - } + _transferFeeToken(round.feeToken, payable(owner), round.totalFeesForJurors);Add this helper (outside the shown range) to the contract:
function _transferFeeToken(IERC20 _feeToken, address payable _recipient, uint256 _amount) internal { if (_feeToken == NATIVE_CURRENCY) { (bool ok, ) = _recipient.call{value: _amount}(""); if (!ok) revert TransferFailed(); } else { _feeToken.safeTransfer(_recipient, _amount); } }
886-905: Same unsafe .send in leftover reward transferMirror the fix above here to avoid silent failures.
Apply this diff:
- if (leftoverFeeReward != 0) { - if (round.feeToken == NATIVE_CURRENCY) { - // The dispute fees were paid in ETH - payable(owner).send(leftoverFeeReward); - } else { - // The dispute fees were paid in ERC20 - round.feeToken.safeTransfer(owner, leftoverFeeReward); - } - } + if (leftoverFeeReward != 0) { + _transferFeeToken(round.feeToken, payable(owner), leftoverFeeReward); + }
♻️ Duplicate comments (4)
contracts/src/arbitration/university/KlerosCoreUniversity.sol (1)
775-781: Ensure setStakePenalty caps by per-court stake and returns applied amountCore adds availablePenalty to pnkPenalties; SortitionModule must clamp by stake in penalizedInCourtID, not total stake, and return actual applied amount.
contracts/src/arbitration/KlerosCoreBase.sol (1)
780-786: Penalty application relies on module’s returned availablePenaltyEnsure module caps by per-court stake and returns actual applied amount; otherwise penalty pool inflates.
contracts/src/arbitration/university/SortitionModuleUniversity.sol (1)
197-219: Fix penalty application: potential over-withdrawal from juror balance.The current implementation has a critical flaw where it may withdraw more PNK than exists in the specific court while deducting the full penalty from the juror's total balance.
Issues:
- Line 216 calls
_setStakewithavailablePenaltyas withdrawal even whencurrentStake < availablePenalty- This causes
juror.stakedPnkto be reduced by more than what was actually removed from the court- The function returns
availablePenaltyinstead of the actually applied amountApply this fix to ensure only the court's available stake is penalized:
function setStakePenalty( address _account, uint96 _courtID, uint256 _penalty ) external override onlyByCore returns (uint256 pnkBalance, uint256 newCourtStake, uint256 availablePenalty) { - Juror storage juror = jurors[_account]; - availablePenalty = _penalty; - newCourtStake = _stakeOf(_account, _courtID); - if (juror.stakedPnk < _penalty) { - availablePenalty = juror.stakedPnk; - } - - if (availablePenalty == 0) return (juror.stakedPnk, newCourtStake, 0); // No penalty to apply. - - uint256 currentStake = _stakeOf(_account, _courtID); - uint256 newStake = 0; - if (currentStake >= availablePenalty) { - newStake = currentStake - availablePenalty; - } - _setStake(_account, _courtID, 0, availablePenalty, newStake); - pnkBalance = juror.stakedPnk; // updated by _setStake() - newCourtStake = _stakeOf(_account, _courtID); // updated by _setStake() + Juror storage juror = jurors[_account]; + if (juror.stakedPnk == 0 || _penalty == 0) return (juror.stakedPnk, _stakeOf(_account, _courtID), 0); + + uint256 currentStake = _stakeOf(_account, _courtID); + if (currentStake == 0) return (juror.stakedPnk, 0, 0); + + // Calculate the actual penalty that can be applied + uint256 maxByBalance = juror.stakedPnk < _penalty ? juror.stakedPnk : _penalty; + uint256 applied = currentStake < maxByBalance ? currentStake : maxByBalance; + + uint256 newStake = currentStake - applied; + _setStake(_account, _courtID, 0, applied, newStake); + + pnkBalance = juror.stakedPnk; // updated by _setStake() + newCourtStake = _stakeOf(_account, _courtID); // updated by _setStake() + availablePenalty = applied; // return the actually applied amount }contracts/src/arbitration/SortitionModuleBase.sol (1)
323-345: Critical: Fix penalty application logic to prevent balance corruption.The current implementation has the same critical issues as the university module:
- Line 342 passes
availablePenaltyas withdrawal even whencurrentStake < availablePenalty- This reduces
juror.stakedPnkby more than the stake removed from the court- Returns the clamped request instead of the actually applied amount
Apply this fix:
function setStakePenalty( address _account, uint96 _courtID, uint256 _penalty ) external override onlyByCore returns (uint256 pnkBalance, uint256 newCourtStake, uint256 availablePenalty) { - Juror storage juror = jurors[_account]; - availablePenalty = _penalty; - newCourtStake = stakeOf(_account, _courtID); - if (juror.stakedPnk < _penalty) { - availablePenalty = juror.stakedPnk; - } - - if (availablePenalty == 0) return (juror.stakedPnk, newCourtStake, 0); // No penalty to apply. - - uint256 currentStake = stakeOf(_account, _courtID); - uint256 newStake = 0; - if (currentStake >= availablePenalty) { - newStake = currentStake - availablePenalty; - } - _setStake(_account, _courtID, 0, availablePenalty, newStake); - pnkBalance = juror.stakedPnk; // updated by _setStake() - newCourtStake = stakeOf(_account, _courtID); // updated by _setStake() + Juror storage juror = jurors[_account]; + if (juror.stakedPnk == 0 || _penalty == 0) return (juror.stakedPnk, stakeOf(_account, _courtID), 0); + + uint256 currentStake = stakeOf(_account, _courtID); + if (currentStake == 0) return (juror.stakedPnk, currentStake, 0); + + // Calculate the actual penalty that can be applied + uint256 maxByBalance = juror.stakedPnk < _penalty ? juror.stakedPnk : _penalty; + uint256 applied = currentStake < maxByBalance ? currentStake : maxByBalance; + + uint256 newStake = currentStake - applied; + _setStake(_account, _courtID, 0, applied, newStake); + + pnkBalance = juror.stakedPnk; // updated by _setStake() + newCourtStake = stakeOf(_account, _courtID); // updated by _setStake() + availablePenalty = applied; // return the actually applied amount }
🧹 Nitpick comments (3)
contracts/test/foundry/KlerosCore_Execution.t.sol (1)
375-376: Fix misleading comment (staker1 staked 1200, not 1000)Update the inline note to avoid confusion.
Apply this diff:
- assertEq(totalLocked, 1000, "Wrong totalLocked"); // Juror only staked 1000 but will fall below minStake with a bad vote + assertEq(totalLocked, 1000, "Wrong totalLocked"); // Juror staked 1200 and will fall below minStake with a bad votecontracts/src/arbitration/university/KlerosCoreUniversity.sol (1)
792-799: Prefer explicit “unstakeByCore” over setStake(...,0) for minStake breachAlign with base to avoid relying on internal stake-set semantics and to make intent clear.
Apply this diff:
- } else if (newCourtStake < courts[penalizedInCourtID].minStake) { - // The juror's balance fell below the court minStake, unstake them from the court. - sortitionModule.setStake(account, penalizedInCourtID, 0, 0, 0); + } else if (newCourtStake < courts[penalizedInCourtID].minStake) { + // The juror's balance fell below the court minStake, unstake them from the court. + sortitionModule.unstakeByCore(account, penalizedInCourtID); }contracts/src/arbitration/KlerosCoreBase.sol (1)
745-747: Minor CEI tightening (optional)Update round.pnkPenalties before external interactions to fully follow Check-Effects-Interactions.
Apply this diff:
- if (round.pnkPenalties != pnkPenaltiesInRound) { - round.pnkPenalties = pnkPenaltiesInRound; // Reentrancy risk: breaks Check-Effect-Interact - } + if (round.pnkPenalties != pnkPenaltiesInRound) { + round.pnkPenalties = pnkPenaltiesInRound; + }And (optional) move this block just before entering the for-loop to minimize state drift during external calls.
📜 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 (8)
contracts/src/arbitration/KlerosCoreBase.sol(7 hunks)contracts/src/arbitration/KlerosCoreNeo.sol(1 hunks)contracts/src/arbitration/SortitionModuleBase.sol(15 hunks)contracts/src/arbitration/SortitionModuleNeo.sol(2 hunks)contracts/src/arbitration/interfaces/ISortitionModule.sol(2 hunks)contracts/src/arbitration/university/KlerosCoreUniversity.sol(8 hunks)contracts/src/arbitration/university/SortitionModuleUniversity.sol(4 hunks)contracts/test/foundry/KlerosCore_Execution.t.sol(6 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2024-11-19T16:31:08.965Z
Learnt from: jaybuidl
PR: kleros/kleros-v2#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/SortitionModuleBase.solcontracts/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-neo
- GitHub Check: Header rules - kleros-v2-neo
- GitHub Check: Pages changed - kleros-v2-neo
- GitHub Check: Redirect rules - kleros-v2-university
- GitHub Check: Header rules - kleros-v2-university
- GitHub Check: Pages changed - kleros-v2-university
- GitHub Check: Analyze (javascript)
- GitHub Check: contracts-testing
🔇 Additional comments (24)
contracts/src/arbitration/KlerosCoreNeo.sol (1)
111-112: Propagate noDelay=false — LGTMPassing false to _setStake aligns with delayed-stake semantics and base changes.
contracts/src/arbitration/SortitionModuleNeo.sol (1)
77-83: _validateStake signature passthrough — LGTM_newStake + _noDelay forwarded to super; semantics preserved; onlyByCore enforced.
Also applies to: 102-103
contracts/test/foundry/KlerosCore_Execution.t.sol (5)
19-20: Stake increased to 2000 — tests reflect new oddsChange mirrors penalties affecting effective stake.
114-115: Post-penalty balance assertion is correct2000 − 1000 penalty => 1000 remaining staked.
151-153: Updated core/staker balances — verify totalsNumbers add up with 1k PNK redistributed; keep an eye if token mint/burn side effects change.
275-278: Event ordering commentEvent comments imply 2-step StakeSet pattern; ensure module emits deterministically across forks (Neo/University).
403-406: Double-check expected balances in new min-stake testCore balance 11000 and unchanged staker2 balance look right with one incoherent juror; confirm against module’s leftover withdrawal rules.
contracts/src/arbitration/university/KlerosCoreUniversity.sol (2)
63-63: Origin tracking added — LGTMdrawnJurorFromCourtIDs aligns arrays 1:1 with drawnJurors.
599-607: draw() returns (address, fromSubcourtID) — LGTMFallback to dispute.courtID when DK returns 0 is sensible.
contracts/src/arbitration/KlerosCoreBase.sol (5)
63-64: Origin tracking array — LGTMKeeps per-vote provenance needed for targeted penalties.
467-477: noDelay plumbing for setStake paths — LGTMPublic setStake delays; SortitionModule path bypasses delay; consistent with module API.
610-618: draw(): record fromSubcourtID with fallback — LGTMMatches interface change and ensures penalties route to origin.
797-803: Unstake fallout — LGTMGlobal-unstake for insolvent/inactive; per-court unstake on minStake breach matches tests.
1145-1164: Accept Delayed staking — LGTMBehavior matches delayed-stake semantics; avoids reverting on valid delays.
Also applies to: 1165-1171
contracts/src/arbitration/interfaces/ISortitionModule.sol (3)
33-37: LGTM! Well-structured penalty interface.The new
setStakePenaltyfunction signature provides good visibility into the penalty application results with its three return values, enabling the caller to track the penalty distribution and resulting balances.
41-43: LGTM! Proper separation of unstaking methods.The addition of
unstakeByCoreFromAllCourtsandunstakeByCoreprovides fine-grained control over juror unstaking, allowing the core to handle both complete unstaking and court-specific unstaking scenarios.
51-55: LGTM! Enhanced draw return value for origin tracking.The addition of
fromSubcourtIDto the draw return values is essential for the new per-origin penalty system, allowing penalties to be applied to the correct originating court.contracts/src/arbitration/university/SortitionModuleUniversity.sol (2)
306-311: LGTM! Clean implementation of complete unstaking.The function correctly iterates through all court IDs and triggers unstaking via the core module.
348-354: Enhanced return signature improves transparency.The addition of
fromSubcourtIDto the draw return values, while not used in this educational implementation, maintains consistency with the main contract interfaces.contracts/src/arbitration/SortitionModuleBase.sol (5)
487-523: LGTM! Proper implementation of origin tracking in draw.The draw function correctly:
- Returns both the drawn address and the originating subcourt ID
- Properly decodes the stakePathID to extract both values
- Returns (address(0), 0) when no jurors are staked
703-734: Well-structured stakePathID encoding.The 32-byte encoding scheme efficiently packs the 20-byte address and 12-byte court ID (uint96). The assembly implementation is clean and the memory layout is clearly documented.
736-767: LGTM! Correct decoding of stakePathID.The decoding function properly extracts both the address and court ID from the packed 32-byte stakePathID, maintaining symmetry with the encoding function.
266-274: Good addition of bypass mechanism for penalty application.The
_noDelayparameter allows immediate stake updates during penalty application, preventing penalties from being queued as delayed stakes.
625-631: Approve stakePathID usage. All calls to _set use a stakePathID derived via _accountAndCourtIDToStakePathID and correctly update both mappings.
There was a problem hiding this comment.
Actionable comments posted: 1
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/university/KlerosCoreUniversity.sol (1)
802-809: Avoid silent ETH transfer failures; use call and revert on failure.
sendreturns false on failure and is ignored. Switch to low-level call with revert, like Base’s approach via helper.Apply:
- // The dispute fees were paid in ETH - payable(owner).send(round.totalFeesForJurors); + (bool ok, ) = payable(owner).call{value: round.totalFeesForJurors}(""); + if (!ok) revert TransferFailed();And similarly for the second
sendat Lines 891-896.Also applies to: 891-896
♻️ Duplicate comments (4)
contracts/src/arbitration/KlerosCoreBase.sol (1)
780-786: Critical: rely only on “applied” penalties from SortitionModule.
_params.pnkPenaltiesInRound += availablePenaltyassumes the module returns the amount actually deducted frompenalizedInCourtID. If the module instead clamps against total stake or returns the requested amount, you’ll over-credit penalties and corrupt balances. Ensure SM implementations cap by stake inpenalizedInCourtIDand return the applied value.To verify across implementations:
#!/bin/bash # Inspect base & university modules for per-court cap and applied returns rg -nP --type=sol -C4 'setStakePenalty\(' contracts/src/arbitration | sed -n '1,200p' rg -nP --type=sol -C3 'stakeOf\(|_stakeOf\(' contracts/src/arbitrationAlso applies to: 797-803
contracts/src/arbitration/university/KlerosCoreUniversity.sol (1)
775-781: Critical: depend on “applied” penalties from SortitionModuleUniversity.Same concern as Base:
availablePenaltymust be the amount actually deducted frompenalizedInCourtID. Otherwise, penalties and balances will diverge.Also applies to: 792-799
contracts/src/arbitration/university/SortitionModuleUniversity.sol (1)
197-219: Fix penalty application to the targeted court and return the applied amount.Current code may withdraw more than the stake in
_courtIDand returns the clamped request, not the applied amount. Computeapplied = min(_penalty, juror.stakedPnk, stakeInCourt); reduce only that court’s stake; returnavailablePenalty = applied.Apply:
function setStakePenalty( address _account, uint96 _courtID, uint256 _penalty ) external override onlyByCore returns (uint256 pnkBalance, uint256 newCourtStake, uint256 availablePenalty) { - Juror storage juror = jurors[_account]; - availablePenalty = _penalty; - newCourtStake = _stakeOf(_account, _courtID); - if (juror.stakedPnk < _penalty) { - availablePenalty = juror.stakedPnk; - } - - if (availablePenalty == 0) return (juror.stakedPnk, newCourtStake, 0); // No penalty to apply. - - uint256 currentStake = _stakeOf(_account, _courtID); - uint256 newStake = 0; - if (currentStake >= availablePenalty) { - newStake = currentStake - availablePenalty; - } - _setStake(_account, _courtID, 0, availablePenalty, newStake); - pnkBalance = juror.stakedPnk; // updated by _setStake() - newCourtStake = _stakeOf(_account, _courtID); // updated by _setStake() + Juror storage juror = jurors[_account]; + uint256 currentStake = _stakeOf(_account, _courtID); + if (_penalty == 0 || juror.stakedPnk == 0 || currentStake == 0) { + return (juror.stakedPnk, currentStake, 0); + } + uint256 maxByBalance = _penalty <= juror.stakedPnk ? _penalty : juror.stakedPnk; + uint256 applied = currentStake <= maxByBalance ? currentStake : maxByBalance; + uint256 newStake = currentStake - applied; + _setStake(_account, _courtID, 0, applied, newStake); + pnkBalance = juror.stakedPnk; // updated inside _setStake + newCourtStake = _stakeOf(_account, _courtID); + availablePenalty = applied; // amount actually deducted }contracts/src/arbitration/SortitionModuleBase.sol (1)
323-345: Fix per-court penalty application and return the applied amount.As in University, this must apply only up to the stake in
_courtIDand return the amount actually deducted. Current code can over-withdraw and misreportavailablePenalty.Apply:
function setStakePenalty( address _account, uint96 _courtID, uint256 _penalty ) external override onlyByCore returns (uint256 pnkBalance, uint256 newCourtStake, uint256 availablePenalty) { - Juror storage juror = jurors[_account]; - availablePenalty = _penalty; - newCourtStake = stakeOf(_account, _courtID); - if (juror.stakedPnk < _penalty) { - availablePenalty = juror.stakedPnk; - } - - if (availablePenalty == 0) return (juror.stakedPnk, newCourtStake, 0); // No penalty to apply. - - uint256 currentStake = stakeOf(_account, _courtID); - uint256 newStake = 0; - if (currentStake >= availablePenalty) { - newStake = currentStake - availablePenalty; - } - _setStake(_account, _courtID, 0, availablePenalty, newStake); - pnkBalance = juror.stakedPnk; // updated by _setStake() - newCourtStake = stakeOf(_account, _courtID); // updated by _setStake() + Juror storage juror = jurors[_account]; + uint256 currentStake = stakeOf(_account, _courtID); + if (_penalty == 0 || juror.stakedPnk == 0 || currentStake == 0) { + return (juror.stakedPnk, currentStake, 0); + } + uint256 maxByBalance = _penalty <= juror.stakedPnk ? _penalty : juror.stakedPnk; + uint256 applied = currentStake <= maxByBalance ? currentStake : maxByBalance; + uint256 newStake = currentStake - applied; + _setStake(_account, _courtID, 0, applied, newStake); + pnkBalance = juror.stakedPnk; // updated inside _setStake + newCourtStake = stakeOf(_account, _courtID); + availablePenalty = applied; // amount actually deducted from this court }
🧹 Nitpick comments (5)
contracts/src/arbitration/interfaces/ISortitionModule.sol (1)
51-56: Spell out fromSubcourtID=0 semantics.Please add a brief note that
fromSubcourtID == 0means “drawn directly from the current court,” and consumers should fallback todispute.courtID. This matches Core usage.contracts/src/arbitration/KlerosCoreBase.sol (1)
745-747: Optional: remove confusing CEI comment or update write timing.The assignment to
round.pnkPenaltieshappens after external calls; this is by design as you accumulate in memory. Either remove the “Reentrancy risk” comment or update to set storage per-iteration if you want strict CEI at the cost of gas.contracts/src/arbitration/SortitionModuleBase.sol (3)
540-545: Nit: prefer explicit uint256.Use
uint256 treeIndexfor consistency with the rest of the codebase.Apply:
- uint treeIndex = tree.IDsToNodeIndexes[_stakePathID]; + uint256 treeIndex = tree.IDsToNodeIndexes[_stakePathID];
708-734: Minor gas/readability: pack without memory loops.Use shifts/ors to build the stakePathID in-place.
Apply:
- ) internal pure returns (bytes32 stakePathID) { - assembly { - let ptr := mload(0x40) - // write 20B addr then 12B courtID... - ... - stakePathID := mload(ptr) - } - } + ) internal pure returns (bytes32 stakePathID) { + // [addr (20 bytes) | courtID (12 bytes)] + stakePathID = bytes32((uint256(uint160(_account)) << 96) | uint256(uint96(_courtID))); + }
741-767: Minor gas/readability: unpack without memory loops.Bit-slice instead of memory copies.
Apply:
- ) internal pure returns (address account, uint96 courtID) { - assembly { ... } - } + ) internal pure returns (address account, uint96 courtID) { + account = address(uint160(uint256(_stakePathID >> 96))); + courtID = uint96(uint256(_stakePathID)); + }
📜 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 (5)
contracts/src/arbitration/KlerosCoreBase.sol(7 hunks)contracts/src/arbitration/SortitionModuleBase.sol(15 hunks)contracts/src/arbitration/interfaces/ISortitionModule.sol(2 hunks)contracts/src/arbitration/university/KlerosCoreUniversity.sol(8 hunks)contracts/src/arbitration/university/SortitionModuleUniversity.sol(4 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-09-03T19:34:58.018Z
Learnt from: jaybuidl
PR: kleros/kleros-v2#2107
File: contracts/src/arbitration/university/KlerosCoreUniversity.sol:1083-1092
Timestamp: 2025-09-03T19:34:58.018Z
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/KlerosCoreUniversity.sol
📚 Learning: 2024-11-19T16:31:08.965Z
Learnt from: jaybuidl
PR: kleros/kleros-v2#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/SortitionModuleBase.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-university
- GitHub Check: Header rules - kleros-v2-university
- GitHub Check: Pages changed - kleros-v2-university
- GitHub Check: Redirect rules - kleros-v2-testnet
- GitHub Check: Redirect rules - kleros-v2-neo
- GitHub Check: Redirect rules - kleros-v2-testnet
- GitHub Check: Header rules - kleros-v2-testnet
- GitHub Check: Header rules - kleros-v2-neo
- GitHub Check: Header rules - kleros-v2-testnet
- GitHub Check: Pages changed - kleros-v2-testnet
- GitHub Check: Pages changed - kleros-v2-neo
- GitHub Check: Pages changed - kleros-v2-testnet
- GitHub Check: Analyze (javascript)
- GitHub Check: contracts-testing
🔇 Additional comments (7)
contracts/src/arbitration/interfaces/ISortitionModule.sol (1)
21-23: LGTM: validateStake gains _noDelay flag.Signature aligns with Core’s
_setStake(..., _noDelay, ...)and delayed-stake flow.contracts/src/arbitration/KlerosCoreBase.sol (2)
610-618: LGTM: store origin subcourt per draw.Capturing
fromSubcourtIDand falling back todispute.courtIDis correct and matches ABI changes.
466-477: LGTM: unified staking path with _noDelay.Public and module-driven staking both route through
_setStake(..., _noDelay, ...), aligning with interface changes.Also applies to: 1159-1165
contracts/src/arbitration/university/KlerosCoreUniversity.sol (2)
599-607: LGTM: origin-aware draw flow integrated.Correctly records origin court and falls back to
dispute.courtID.
455-465: LGTM: staking API matches university module semantics.No delayed stakes in University; passing
_noDelayis harmless and consistent.Also applies to: 1101-1106
contracts/src/arbitration/university/SortitionModuleUniversity.sol (1)
349-354: LGTM: draw returns transient juror; fromSubcourtID defaults to 0.Sufficient for the educational module; callers already fallback to
dispute.courtID.contracts/src/arbitration/SortitionModuleBase.sol (1)
491-497: LGTM: draw now returns (address, fromSubcourtID).Graceful empty-tree handling and correct unpacking from
stakePathID.Also applies to: 521-523
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
contracts/CHANGELOG.md (4)
17-17: Clarify “include penalties” semantics.Specify whether
stakedInCourtis net-of-penalties and whether it factors in locked stake, to avoid downstream misinterpretation in integrators.
18-18: Document type/shape of drawnJurorFromCourtIDs and migration impact.If this is an array aligned with
drawnJurors(e.g.,uint96[] drawnJurorFromCourtIDs), state it explicitly and reiterate the storage layout incompatibility and redeploy requirement.
19-19: Use “subcourt ID” if the return value isfromSubcourtID.The PR and identifiers suggest subcourt-level origin. If correct, update the wording; if not, rename the identifier for consistency.
21-21: Grammar: “bypass” instead of “by-pass”.Minor copy edit for consistency.
- - Allow stake changes to by-pass delayed stakes when initiated by the SortitionModule by setting the `_noDelay` parameter to `true` in `SortitionModule.validateStake()` ... + - Allow stake changes to bypass delayed stakes when initiated by the SortitionModule by setting the `_noDelay` parameter to `true` in `SortitionModule.validateStake()` ...
📜 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 (1)
contracts/CHANGELOG.md(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). (14)
- GitHub Check: Redirect rules - kleros-v2-testnet-devtools
- 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-devtools
- 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-devtools
- GitHub Check: Pages changed - kleros-v2-university
- GitHub Check: Pages changed - kleros-v2-testnet
- GitHub Check: Pages changed - kleros-v2-testnet
- GitHub Check: contracts-testing
- GitHub Check: Analyze (javascript)
1 participant
Motivation
The drawing odds are currently not impacted by the penalties. In addition there are UX issues associated with not being able to identify the court where the penalties are applied.
Analysis
Currently a sortition trees is only aware of the "effective stake" which is the amount directly staked in a court + the amounts directly staked in its subcourts. Given only a stakePathID, it is not possible to know whether the court or one of its subcourts contributed to the draw.
We need to be able to attribute a draw to the court where the stake originates from. Once we make this information available to the SortitionModule, it becomes possible to apply the penalties to the appropriate court's sortition tree.
Solution
To achieve this we map a
stakePathIDto a new struct which keeps track of the subcourt stakes contributing to the effective stake. The size of the struct remains small because it is limited byMAX_STAKE_PATHSwhich is currently set to 4.Impact
SortitionModule.getJurorBalance().stakedInCourtnow includes the penalties, it did not previously.drawnJurorFromCourtIDsin the structKlerosCore.RoundThese changes require a full redeploy, not an upgrade.
PR-Codex overview
This PR focuses on enhancing the staking mechanism in the Kleros arbitration system. It introduces penalties for jurors' stakes, modifies stake handling to bypass delays, and updates functions to return additional information, such as the court ID from which a juror was drawn.
Detailed summary
gasLimitforsetStakeindispute-kit-gated.ts.setStakefunction inKlerosCoreNeo.solto include a_noDelayparameter.drawfunction inIDisputeKit.solto returnfromSubcourtID.setStakePenaltyfunction inISortitionModule.sol.setJurorInactivetoforcedUnstakeAllCourtsinSortitionModuleUniversity.sol.drawfunctions to return additional data.Summary by CodeRabbit
New Features
Tests