Skip to content

Conversation

jaybuidl
Copy link
Member

@jaybuidl jaybuidl commented Aug 27, 2025

Changes:

  • More explicit boundary between the sortition trees and the sortition module.
  • The tree key encoding doesn't leak anymore into the Core through the createTree() function, into the Dispute Kits through the draw() function.
  • More readable handling of the tree key and courtID.
  • Test coverage of the sortition trees

PR-Codex overview

This PR focuses on refactoring the SortitionModule to use uint96 for court identifiers instead of bytes32, improving type safety and clarity. It also extracts the sortition sum trees logic into a dedicated library.

Detailed summary

  • Changed function parameters in ISortitionModule from bytes32 _key to uint96 _courtID.
  • Updated createTree and draw functions to use uint96 for court IDs.
  • Refactored SortitionSumTree structure into a library SortitionTrees.
  • Adjusted stake management functions to accommodate new types.
  • Added tests for the new functionality and utilities in SortitionTreesMock.

✨ Ask PR-Codex anything about this PR by commenting with /codex {your question}

Summary by CodeRabbit

  • New Features

    • Draw now returns the drawn juror and their originating subcourt.
    • New per‑court APIs to apply penalties/unstake and to view juror balances by court.
  • Refactor

    • System-wide switch to numeric court IDs.
    • Sortition tree logic extracted to a shared library for consistent multi‑court handling.
  • Tests

    • Comprehensive test suite and mocks validating tree, stake, draw, and multi‑court scenarios.

Copy link

netlify bot commented Aug 27, 2025

Deploy Preview for kleros-v2-testnet ready!

Name Link
🔨 Latest commit 1e0c87c
🔍 Latest deploy log https://app.netlify.com/projects/kleros-v2-testnet/deploys/68ae991119604f00087c3bc2
😎 Deploy Preview https://deploy-preview-2113--kleros-v2-testnet.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify project configuration.

Copy link

netlify bot commented Aug 27, 2025

Deploy Preview for kleros-v2-testnet-devtools ready!

Name Link
🔨 Latest commit 1e0c87c
🔍 Latest deploy log https://app.netlify.com/projects/kleros-v2-testnet-devtools/deploys/68ae9911b4412300083dea3d
😎 Deploy Preview https://deploy-preview-2113--kleros-v2-testnet-devtools.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify project configuration.

Copy link
Contributor

coderabbitai bot commented Aug 27, 2025

Walkthrough

Refactors sortition into a new SortitionTrees library, replaces bytes32 tree keys with uint96 courtIDs across createTree/draw/setStake APIs, changes draw to return (drawnAddress, fromSubcourtID), and migrates per-court stake/penalty operations to the library-backed implementation.

Changes

Cohort / File(s) Summary
Core init & usage
contracts/src/arbitration/KlerosCoreBase.sol
FORKING_COURT and GENERAL_COURT now call sortitionModule.createTree with uint96 courtIDs; initialization and internal calls use numeric courtIDs.
Sortition module refactor
contracts/src/arbitration/SortitionModuleBase.sol
Replaces bespoke SortitionSumTree storage with SortitionTrees library (mapping(TreeKey => SortitionTrees.Tree)); createTree/draw signatures now use uint96 _courtID; adds setStakePenalty and getJurorBalance; delegates stake updates/draws to the library and removes old helpers/errors.
Dispute kit draw propagation
contracts/src/arbitration/dispute-kits/DisputeKitClassicBase.sol
Calls sortitionModule.draw(courtID, _coreDisputeID, _nonce) with uint96 (removed bytes32 conversion); continues handling (drawnAddress, fromSubcourtID).
Interface alignment
contracts/src/arbitration/interfaces/ISortitionModule.sol
createTree now accepts uint96 _courtID; draw accepts uint96 _courtID and returns (address drawnAddress, uint96 fromSubcourtID).
University module
contracts/src/arbitration/university/SortitionModuleUniversity.sol
Adapts createTree and draw signatures to uint96 _courtID; draw returns (address drawnAddress, uint96 fromSubcourtID) (fromSubcourtID defaults to 0).
New library
contracts/src/libraries/SortitionTrees.sol
Adds SortitionTrees library with TreeKey/CourtID types, Tree struct, createTree, draw, set, stakeOf, toStakePathID/toAccountAndCourtID, updateParents, and errors (TreeAlreadyExists, KMustBeGreaterThanOne).
Tests / mocks
contracts/src/test/SortitionModuleMock.sol, contracts/src/test/SortitionTreesMock.sol, contracts/test/sortition/index.ts
Mocks/tests updated/added to expose and validate SortitionTrees: storage uses SortitionTrees.Tree/TreeKey, test helpers for stakePathID, tree creation, set/draw/stakeOf, hierarchy helpers, and comprehensive tests.
Changelog
contracts/CHANGELOG.md
Documents extraction of SortitionTrees library and API key/type changes (bytes32 key → uint96 courtID).

Sequence Diagram(s)

sequenceDiagram
  autonumber
  actor Core as KlerosCoreBase
  participant DK as DisputeKitClassicBase
  participant SM as SortitionModule
  participant Lib as SortitionTrees (library)

  Note over Core,DK: Juror selection flow (new numeric courtID)
  Core->>DK: request draw(coreDisputeID, nonce)
  DK->>SM: draw(uint96 courtID, coreDisputeID, nonce)
  SM->>Lib: trees[courtID].draw(coreDisputeID, nonce, rand)
  Lib-->>SM: (drawnAddress, fromSubcourtID)
  SM-->>DK: (drawnAddress, fromSubcourtID)
  DK-->>Core: (drawnAddress, fromSubcourtID)
Loading
sequenceDiagram
  autonumber
  actor Core as KlerosCoreBase
  participant SM as SortitionModule
  participant Lib as SortitionTrees (library)

  Note over Core,SM: Apply per-court stake penalty (library-backed)
  Core->>SM: setStakePenalty(account, courtID, penalty)
  SM->>Lib: trees[courtID].set(...) / stake update
  Lib-->>SM: updated stake info
  SM-->>Core: (pnkBalance, newCourtStake, availablePenalty)
Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~60 minutes

Possibly related PRs

Suggested reviewers

  • unknownunknown1

Poem

I hop through nodes and pack each trail,
From court to leaf on a carrot-tail.
Sums climb up, the stack reclaims,
A juror drawn from numbered plains.
🥕🐇 New trees take root — I celebrate the gains!


📜 Recent 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.

📥 Commits

Reviewing files that changed from the base of the PR and between a12e966 and 323ba18.

📒 Files selected for processing (10)
  • contracts/CHANGELOG.md (1 hunks)
  • contracts/src/arbitration/KlerosCoreBase.sol (4 hunks)
  • contracts/src/arbitration/SortitionModuleBase.sol (5 hunks)
  • contracts/src/arbitration/dispute-kits/DisputeKitClassicBase.sol (1 hunks)
  • contracts/src/arbitration/interfaces/ISortitionModule.sol (2 hunks)
  • contracts/src/arbitration/university/SortitionModuleUniversity.sol (2 hunks)
  • contracts/src/libraries/SortitionTrees.sol (1 hunks)
  • contracts/src/test/SortitionModuleMock.sol (1 hunks)
  • contracts/src/test/SortitionTreesMock.sol (1 hunks)
  • contracts/test/sortition/index.ts (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (10)
  • contracts/src/arbitration/dispute-kits/DisputeKitClassicBase.sol
  • contracts/src/test/SortitionModuleMock.sol
  • contracts/src/arbitration/interfaces/ISortitionModule.sol
  • contracts/CHANGELOG.md
  • contracts/src/arbitration/university/SortitionModuleUniversity.sol
  • contracts/src/test/SortitionTreesMock.sol
  • contracts/test/sortition/index.ts
  • contracts/src/arbitration/KlerosCoreBase.sol
  • contracts/src/libraries/SortitionTrees.sol
  • 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). (2)
  • GitHub Check: SonarCloud
  • GitHub Check: Mend Security Check
✨ Finishing Touches
  • 📝 Generate Docstrings
🧪 Generate unit tests
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch refactor/sortition-trees-library

🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.

Support

Need help? Create a ticket on our support page for assistance with any issues or questions.

CodeRabbit Commands (Invoked using PR/Issue comments)

Type @coderabbitai help to get the list of available commands.

Other keywords and placeholders

  • Add @coderabbitai ignore or @coderabbit ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai or @coderabbitai title anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Status, Documentation and Community

  • Visit our Status Page to check the current availability of CodeRabbit.
  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

@jaybuidl jaybuidl changed the base branch from dev to fix/penalties-reduce-drawing-odds August 27, 2025 05:35
Copy link

netlify bot commented Aug 27, 2025

Deploy Preview for kleros-v2-university failed. Why did it fail? →

Name Link
🔨 Latest commit 1e0c87c
🔍 Latest deploy log https://app.netlify.com/projects/kleros-v2-university/deploys/68ae99117d33a7000856d39d

Copy link

netlify bot commented Aug 27, 2025

Deploy Preview for kleros-v2-neo ready!

Name Link
🔨 Latest commit 1e0c87c
🔍 Latest deploy log https://app.netlify.com/projects/kleros-v2-neo/deploys/68ae9911ef1f81000816baf8
😎 Deploy Preview https://deploy-preview-2113--kleros-v2-neo.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify project configuration.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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/SortitionModuleBase.sol (1)

390-403: Potential issue with parent court iteration.

In the _setStake function, Line 395 uses _courtID to create the tree key, but the loop iterates through parent courts via currentCourtID. This appears to be incorrect - the key should be created from currentCourtID for each iteration.

 while (!finished) {
     // Tokens are also implicitly staked in parent courts through sortition module to increase the chance of being drawn.
-    TreeKey key = CourtID.wrap(_courtID).toTreeKey();
+    TreeKey key = CourtID.wrap(currentCourtID).toTreeKey();
     sortitionSumTrees[key].set(_newStake, stakePathID);
     if (currentCourtID == GENERAL_COURT) {
         finished = true;
     } else {
         (currentCourtID, , , , , , ) = core.courts(currentCourtID); // Get the parent court.
     }
 }
🧹 Nitpick comments (3)
contracts/src/libraries/SortitionTrees.sol (3)

51-53: Consider validating the tree state more explicitly.

While returning (address(0), 0) when the tree is empty is reasonable, consider whether this should revert with a descriptive error to make the failure mode more explicit to callers.

 if (_tree.nodes[0] == 0) {
-    return (address(0), 0); // No jurors staked.
+    revert NoJurorsStaked(); // More explicit error
 }

Add the error definition:

error NoJurorsStaked();

219-248: Assembly implementation for unpacking looks correct but could be simplified.

The toAccountAndCourtID function correctly unpacks the stake path ID, but like the packing function, it could benefit from a more straightforward implementation.

Consider this simpler implementation:

 function toAccountAndCourtID(bytes32 _stakePathID) internal pure returns (address account, uint96 courtID) {
-    assembly {
-        // solium-disable-line security/no-inline-assembly
-        let ptr := mload(0x40)
-
-        // Read account address (first 20 bytes)
-        for {
-            let i := 0x00
-        } lt(i, 0x14) {
-            i := add(i, 0x01)
-        } {
-            mstore8(add(add(ptr, 0x0c), i), byte(i, _stakePathID))
-        }
-        account := mload(ptr)
-
-        // Read court ID (last 12 bytes)
-        for {
-            let i := 0x00
-        } lt(i, 0x0c) {
-            i := add(i, 0x01)
-        } {
-            mstore8(add(add(ptr, 0x14), i), byte(add(i, 0x14), _stakePathID))
-        }
-        courtID := mload(ptr)
-    }
+    account = address(uint160(uint256(_stakePathID) >> 96));
+    courtID = uint96(uint256(_stakePathID));
 }

189-217: Byte layout verified; optional bitwise refactor recommended

  • Verified that the current assembly implementation correctly packs the 20-byte address into bytes 0–19 and the 12-byte courtID into bytes 20–31 as intended.
  • The proposed one-liner
    stakePathID = bytes32(bytes20(_account)) | bytes32(uint256(_courtID));
    yields an identical byte layout and is more concise and gas-efficient.

Feel free to replace the inline assembly with the above bitwise OR implementation if you’d like to simplify the code.

📜 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.

📥 Commits

Reviewing files that changed from the base of the PR and between 1e0c87c and 1d86159.

📒 Files selected for processing (2)
  • contracts/src/arbitration/SortitionModuleBase.sol (5 hunks)
  • contracts/src/libraries/SortitionTrees.sol (1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
  • GitHub Check: SonarCloud
🔇 Additional comments (7)
contracts/src/libraries/SortitionTrees.sol (2)

1-13: LGTM! Clean type definitions and global extension.

The use of custom types TreeKey and CourtID with a global extension function provides type safety and clear abstractions for the sortition tree implementation.


88-157: Complex tree update logic handles edge cases well.

The set function correctly handles all cases: new nodes, removals, and updates. The logic for maintaining vacant spots via the stack and handling parent sum nodes is sound.

contracts/src/arbitration/SortitionModuleBase.sol (5)

10-18: Good integration with the SortitionTrees library.

The import and usage declarations properly set up the library integration, enabling clean delegation of tree operations.


465-474: Draw function correctly returns the subcourt information.

The updated draw function properly returns both the drawn address and the originating subcourt ID, which aligns with the penalty mechanism requirements.


493-507: New getJurorBalance function provides comprehensive juror information.

The function efficiently returns all relevant juror balance information in a single call, which will help reduce external calls when querying juror state.


312-334: Verify penalty edge cases and inter-module assumptions

I updated the search to cover all .sol files (removing the unrecognized --type solidity filter) and did not find any other checks or calls that assume _penalty is always ≤ juror.stakedPnk. However, absence of automated matches doesn’t guarantee there aren’t inter-module dependencies or off-chain tooling that rely on the old behavior. Please manually verify the following before approving:

  • Review any contracts or libraries that call setStakePenalty (or otherwise handle slashing) to ensure they correctly handle availablePenalty being capped at juror.stakedPnk even when the input _penalty is larger.
  • Add or update unit tests for scenarios where _penalty > juror.stakedPnk to confirm no underflow occurs and event/state updates remain correct.
  • Check off-chain integrations (scripts, indexers, UI) that read the penalty values—confirm they don’t break if they encounter a capped penalty.

Once these points have been confirmed, the new penalty mechanism can be approved.


183-189: No additional guards needed for createTree
The sortitionSumTrees.createTree library call already reverts on duplicate trees (TreeAlreadyExists), and all invocations from KlerosCoreBase happen exactly once per court ID:

  • Initial FORKING_COURT creation in the constructor
  • GENERAL_COURT setup immediately afterward
  • Each new subcourt uses a fresh courtID (derived from courts.length) and calls createTree only once.

Since there are no code paths that attempt to recreate a tree for the same key, the existing revert behavior sufficiently prevents duplicates.

@jaybuidl jaybuidl force-pushed the refactor/sortition-trees-library branch from 1d86159 to 9d15604 Compare August 28, 2025 22:53
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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/SortitionModuleBase.sol (1)

313-334: Penalty can push stakedPnk below lockedPnk (invariant breach).

setStakePenalty deducts from juror.stakedPnk without respecting juror.lockedPnk, allowing stakedPnk < lockedPnk (e.g., staked=100, locked=90, penalty=50 → staked=50 < 90). This breaks the locked≤staked invariant enforced elsewhere and can cause underflows or inconsistent accounting.

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;
-  }
+  // Do not deduct below the locked amount.
+  uint256 maxPenalty = juror.stakedPnk > juror.lockedPnk ? juror.stakedPnk - juror.lockedPnk : 0;
+  availablePenalty = _penalty > maxPenalty ? maxPenalty : _penalty;
+  newCourtStake = stakeOf(_account, _courtID);
 
   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;
-  }
+  uint256 currentStake = newCourtStake;
+  uint256 newStake = currentStake > availablePenalty ? currentStake - availablePenalty : 0;
   _setStake(_account, _courtID, 0, availablePenalty, newStake);
   pnkBalance = juror.stakedPnk; // updated by _setStake()
   newCourtStake = stakeOf(_account, _courtID); // updated by _setStake()
 }
🧹 Nitpick comments (5)
contracts/src/arbitration/SortitionModuleBase.sol (5)

183-189: Guard K locally or confirm library-level validation.

If SortitionTrees.createTree reverts for K < 2, fine; otherwise add a local check to fail fast with a clear reason.

Example:

 function createTree(uint96 _courtID, bytes memory _extraData) external override onlyByCore {
   TreeKey key = CourtID.wrap(_courtID).toTreeKey();
   uint256 K = _extraDataToTreeK(_extraData);
+  require(K > 1, "Sortition: K must be > 1");
   sortitionSumTrees.createTree(key, K);
 }

336-345: Fix Natspec: function describes reward but performs penalty.

The header says “reward deposit” for setStakePenalty; it should say “penalty deduction.”

-/// @dev Update the state of the stakes with a PNK reward deposit, called by KC during rewards execution.
+/// @dev Update the state of the stakes with a PNK penalty deduction, called by KC during penalties execution.

389-397: OK; consider small gas nits in tree updates.

Loop is correct. Minor nits:

  • Cache toTreeKey for GENERAL_COURT if hot.
  • A do/while eliminates the finished flag.
-bool finished = false;
-uint96 currentCourtID = _courtID;
-while (!finished) {
+uint96 currentCourtID = _courtID;
+for (;;) {
   TreeKey key = CourtID.wrap(currentCourtID).toTreeKey();
   sortitionSumTrees[key].set(_newStake, stakePathID);
-  if (currentCourtID == GENERAL_COURT) {
-      finished = true;
-  } else {
-      (currentCourtID, , , , , , ) = core.courts(currentCourtID); // Get the parent court.
-  }
+  if (currentCourtID == GENERAL_COURT) break;
+  (currentCourtID, , , , , , ) = core.courts(currentCourtID); // parent
}

63-75: Unify courtID types in events.

StakeSet uses uint256 for _courtID while the rest of the API moved to uint96 (StakeDelayed already uses uint96). Standardize to uint96 to avoid decoding friction and ABI surprises.

-event StakeSet(address indexed _address, uint256 _courtID, uint256 _amount, uint256 _amountAllCourts);
+event StakeSet(address indexed _address, uint96 _courtID, uint256 _amount, uint256 _amountAllCourts);

Confirm downstream indexers/off-chain consumers are updated.

Also applies to: 403-404


379-385: Micro gas: unchecked i-- in swap-and-pop loop.

Safe in 0.8.x and saves gas.

for (uint256 i = juror.courtIDs.length; i > 0; i--) {
-    if (juror.courtIDs[i - 1] == _courtID) {
+    unchecked { // i > 0
+        if (juror.courtIDs[i - 1] == _courtID) {
+    }
         juror.courtIDs[i - 1] = juror.courtIDs[juror.courtIDs.length - 1];
         juror.courtIDs.pop();
         break;
-    }
+    }
}
📜 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.

📥 Commits

Reviewing files that changed from the base of the PR and between 1d86159 and 9d15604.

📒 Files selected for processing (7)
  • contracts/src/arbitration/KlerosCoreBase.sol (4 hunks)
  • contracts/src/arbitration/SortitionModuleBase.sol (5 hunks)
  • contracts/src/arbitration/dispute-kits/DisputeKitClassicBase.sol (1 hunks)
  • contracts/src/arbitration/interfaces/ISortitionModule.sol (2 hunks)
  • contracts/src/arbitration/university/SortitionModuleUniversity.sol (2 hunks)
  • contracts/src/libraries/SortitionTrees.sol (1 hunks)
  • contracts/src/test/SortitionModuleMock.sol (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (6)
  • contracts/src/arbitration/university/SortitionModuleUniversity.sol
  • contracts/src/arbitration/interfaces/ISortitionModule.sol
  • contracts/src/arbitration/dispute-kits/DisputeKitClassicBase.sol
  • contracts/src/arbitration/KlerosCoreBase.sol
  • contracts/src/test/SortitionModuleMock.sol
  • contracts/src/libraries/SortitionTrees.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). (1)
  • GitHub Check: SonarCloud
🔇 Additional comments (5)
contracts/src/arbitration/SortitionModuleBase.sol (5)

10-10: Library extraction and typed keys look good.

The switch to SortitionTrees with typed TreeKey/CourtID improves safety; mapping extension usage is idiomatic. No issues spotted here.

Also applies to: 17-19, 54-54


466-474: Confirm randomness domain separation in SortitionTrees.draw.

Ensure the library mixes randomNumber, _coreDisputeID, and _nonce (e.g., keccak(randomNumber, _coreDisputeID, _nonce)) to prevent cross-dispute/court correlation and replay across nonces.

Would you like a quick property-based test that asserts different (_coreDisputeID, _nonce) pairs produce independent selections for a fixed tree?


481-484: stakeOf port looks correct.

Path ID derivation + per-court lookup align with the library model.


494-507: getJurorBalance surface is coherent.

The values reflect juror struct + library-based stake; no issues.


183-189: No issues found in SortitionModuleBase.createTree and related flows
Usage of createTree, draw, setStakePenalty, and the StakeSet event is consistent across contracts, libraries, interfaces, and tests; _extraDataToTreeK correctly defaults to DEFAULT_K = 6; no unintended stakedPnk vs lockedPnk assumptions detected.

@jaybuidl jaybuidl marked this pull request as ready for review August 28, 2025 23:07
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 6

🧹 Nitpick comments (5)
contracts/CHANGELOG.md (1)

19-20: Changelog wording mismatches the implementation

The library functions still take a TreeKey (bytes32) for createTree(mapping(TreeKey => Tree), TreeKey _key, ...) and draw(Tree storage, ...) returns (address,uint96); the “Replace bytes32 _key by uint96 courtID” line makes it sound like the library’s own signatures changed. Clarify that the external module APIs switched to uint96 courtID, while the library keeps TreeKey and provides CourtID.toTreeKey() for conversion.

Apply:

-- Replace the `bytes32 _key` parameter in `SortitionTrees.createTree()` and `SortitionTrees.draw()` by `uint96 courtID` ([#2113](...))
-– Extract the sortition sum trees logic into a library `SortitionTrees` ([#2113](...))
+- Replace `bytes32` court keys with `uint96 courtID` in module-facing APIs; the library keeps `TreeKey (bytes32)` and adds `CourtID.toTreeKey()` to convert. ([#2113](...))
+- Extract the sortition-sum-tree logic into the `SortitionTrees` library. ([#2113](...))
contracts/src/libraries/SortitionTrees.sol (4)

44-53: Document _randomNumber param

draw() takes _randomNumber but it’s undocumented in the NatSpec.

-/// @param _nonce Nonce to hash with random number.
+/// @param _nonce Nonce to hash with random number.
+/// @param _randomNumber External randomness seed (e.g., from RNG/VRF).

167-183: Tiny typo in comments and minor clarity

“substract” -> “subtract”. Also consider early-returning when _value == 0 to skip the loop, but optional.

-    /// @param _plusOrMinus Whether to add (true) or substract (false).
-    /// @param _value The value to add or substract.
+    /// @param _plusOrMinus Whether to add (true) or subtract (false).
+    /// @param _value The value to add or subtract.

36-42: Consider returning the created Tree reference or emitting an event (optional)

Returning Tree storage (or emitting an event) can simplify call sites and aid debugging, but optional.


266-268: Add specific error for uninitialized tree (optional)

If you adopt the guard in set(), consider error TreeNotInitialized(); to distinguish from KMustBeGreaterThanOne().

📜 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.

📥 Commits

Reviewing files that changed from the base of the PR and between 9d15604 and e43b335.

📒 Files selected for processing (2)
  • contracts/CHANGELOG.md (1 hunks)
  • contracts/src/libraries/SortitionTrees.sol (1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
  • GitHub Check: SonarCloud
🔇 Additional comments (2)
contracts/CHANGELOG.md (1)

21-22: Align pragma with toolchain bump

You note Hardhat/Foundry set to 0.8.30, but SortitionTrees.sol uses pragma ^0.8.24. Bump the library to ^0.8.30 to avoid mismatched compiler behaviors (e.g., IR pipeline) and ensure consistent bytecode.

Do you want me to raise a tiny follow-up PR to bump the pragma across the new library and any dependents?

contracts/src/libraries/SortitionTrees.sol (1)

54-61: Prefer reverting when no stake exists rather than returning zero (behavioral choice)

Current behavior returns (address(0), 0) if root sum is zero. If consumers assume a revert on “no jurors,” define a dedicated error (e.g., NothingToDraw()). If returning zero is intentional, document it in NatSpec.

Do downstream modules (e.g., DisputeKitClassicBase) expect a revert or a sentinel value here?

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 5

🧹 Nitpick comments (5)
contracts/src/test/SortitionTreesMock.sol (2)

9-13: Consistent library invocation style.

You’re mixing method-style (trees.createTree(...)) and free-function calls (SortitionTrees.set(...), SortitionTrees.stakeOf(...)). For readability, pick one style (e.g., add using SortitionTrees for SortitionTrees.Tree; and call trees[key].set(...) / trees[key].stakeOf(...)).

Example:

+    using SortitionTrees for SortitionTrees.Tree;
@@
-        bytes32 stakePathID = SortitionTrees.toStakePathID(_account, _courtID);
-        SortitionTrees.set(trees[key], _value, stakePathID);
+        bytes32 stakePathID = SortitionTrees.toStakePathID(_account, _courtID);
+        trees[key].set(_value, stakePathID);
@@
-        bytes32 stakePathID = SortitionTrees.toStakePathID(_account, _courtID);
-        return SortitionTrees.stakeOf(trees[key], stakePathID);
+        bytes32 stakePathID = SortitionTrees.toStakePathID(_account, _courtID);
+        return trees[key].stakeOf(stakePathID);

Also applies to: 22-33, 46-51


98-114: Direct struct field access is fine for a mock; keep it strictly test-only.

Accessing nodes, stack and IDsToNodeIndexes couples tests to internals that may change. If these structs evolve, tests will break. Consider adding thin getters in the library or clearly marking these helpers as test-only and keeping them out of prod code.

Also applies to: 116-134

contracts/test/sortition/index.ts (3)

7-7: Strongly type signers.

Avoid any[]. Use HardhatEthersSigner[] for better IDE/type safety.

Apply:

-  let accounts: any[];
+  import type { HardhatEthersSigner } from "@nomicfoundation/hardhat-ethers/signers";
+  let accounts: HardhatEthersSigner[];

279-283: Use bigint for courtID in empty-tree draw assertion.

courtID is decoded as bigint.

-      expect(courtID).to.equal(0);
+      expectBnEq(courtID, 0n);

316-334: Reduce flakiness in weighted draw test.

With only 100 draws, deterministic seeds could still yield borderline counts. Increase draws and set a tolerance band (e.g., >90%) to reduce false negatives.

-      const numDraws = 100;
+      const numDraws = 512;
@@
-      expect(draws[getTestAddress(1).toLowerCase()]).to.be.greaterThan(numDraws * 0.8); // At least 80% for high stake
+      expect(draws[getTestAddress(1).toLowerCase()]).to.be.greaterThan(numDraws * 0.9); // Heavier weight dominates
📜 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.

📥 Commits

Reviewing files that changed from the base of the PR and between e43b335 and a12e966.

📒 Files selected for processing (3)
  • contracts/src/libraries/SortitionTrees.sol (1 hunks)
  • contracts/src/test/SortitionTreesMock.sol (1 hunks)
  • contracts/test/sortition/index.ts (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
  • contracts/src/libraries/SortitionTrees.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). (2)
  • GitHub Check: SonarCloud
  • GitHub Check: Mend Security Check
🔇 Additional comments (3)
contracts/src/test/SortitionTreesMock.sol (1)

41-44: Drop the type-mismatch suggestion
The draw function in contracts/src/libraries/SortitionTrees.sol is defined to return (address drawnAddress, uint96 fromSubcourtID), so the external wrapper’s signature already matches and will compile as-is.

Likely an incorrect or invalid review comment.

contracts/test/sortition/index.ts (2)

510-519: Clarify expectations for fromSubcourtID.

The checks include([0,1]) suggest ambiguity. If draw should return the exact subcourt used, assert deterministically per setup. Otherwise, document the intended behavior for hierarchical draws.

Would you confirm whether fromSubcourtID should always equal the requested _courtID in this mock (no parent aggregation)? If yes, replace these with explicit equals.


3-3: Fix TypeChain import path and use a type-only import

– import { SortitionTreesMock } from "../../typechain-types";
+ import type { SortitionTreesMock } from "../../../typechain-types";

Ensure the typechain-types directory lives at your project root (i.e. three levels up from contracts/test/sortition/index.ts).

unknownunknown1
unknownunknown1 previously approved these changes Sep 1, 2025
coderabbitai[bot]
coderabbitai bot previously approved these changes Sep 3, 2025
@jaybuidl jaybuidl force-pushed the refactor/sortition-trees-library branch from a12e966 to 323ba18 Compare September 3, 2025 22:15
Base automatically changed from fix/penalties-reduce-drawing-odds to dev September 3, 2025 22:16
@jaybuidl jaybuidl dismissed stale reviews from unknownunknown1 and coderabbitai[bot] September 3, 2025 22:16

The base branch was changed.

Copy link

sonarqubecloud bot commented Sep 3, 2025

@jaybuidl jaybuidl merged commit a7cfba0 into dev Sep 3, 2025
5 checks passed
@jaybuidl jaybuidl deleted the refactor/sortition-trees-library branch September 3, 2025 22:34
@coderabbitai coderabbitai bot mentioned this pull request Sep 4, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Compatibility: ABI change 🗯 Smart contract ABI is changing. Package: Contracts Court smart contracts Type: Enhancement ✨
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants