Skip to content

fix(SortitionModule): fix staking logic and remove instant staking #2004

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 2 commits into
base: dev
Choose a base branch
from

Conversation

unknownunknown1
Copy link
Contributor

@unknownunknown1 unknownunknown1 commented May 20, 2025

PR-Codex overview

This PR focuses on enhancing the SortitionModule functionality by introducing new methods for managing stakes and penalties, improving the handling of delayed stakes, and adding a mechanism for withdrawing leftover tokens. It also includes modifications to existing functions for better clarity and efficiency.

Detailed summary

  • Added Delayed state to StakingResult enum.
  • Updated penalizeStake function to return pnkBalance and availablePenalty.
  • Introduced withdrawLeftoverPNK function for jurors to withdraw unused tokens.
  • Added updateState function to manage stake updates.
  • Removed unused _alreadyTransferred parameter from several functions.
  • Changed event emissions to reflect new states and removed deprecated events.
  • Adjusted tests to validate new functionality and ensure correct behavior with delayed stakes and penalties.

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

Summary by CodeRabbit

  • New Features

    • Added support for jurors to withdraw leftover tokens after fully unstaking.
    • Introduced a new function allowing direct token transfers to jurors by the sortition module.
    • Improved event notifications for delayed staking and leftover token withdrawals.
  • Improvements

    • Simplified and streamlined delayed staking and penalty handling, ensuring more accurate penalty accounting and clearer staking state updates.
    • Enhanced eligibility checks for jurors by removing redundant stake sufficiency conditions.
  • Bug Fixes

    • Corrected staking and token balance updates during delayed staking and unstaking scenarios.
  • Tests

    • Updated and expanded test coverage for staking, penalty execution, and leftover token withdrawal behaviors.

Copy link
Contributor

coderabbitai bot commented May 20, 2025

Walkthrough

This update refactors staking and penalty logic across arbitration contracts by removing the deprecated _alreadyTransferred parameter, introducing new functions for direct PNK transfers and state updates, simplifying delayed stake management, and adjusting penalty and reward execution. Interfaces and tests are updated to reflect these changes, ensuring streamlined stake and token handling.

Changes

File(s) Change Summary
contracts/src/arbitration/KlerosCoreBase.sol Removed _alreadyTransferred usage, added transferBySortitionModule, updated penalty/reward logic, improved stake state.
contracts/src/arbitration/SortitionModuleBase.sol Removed deprecated delayed stake logic, unified events, added updateState and withdrawLeftoverPNK, updated penalties.
contracts/src/arbitration/SortitionModuleNeo.sol Marked _alreadyTransferred as unused, simplified stake increase logic.
contracts/src/arbitration/dispute-kits/DisputeKitClassicBase.sol Simplified _postDrawCheck, removed stake sufficiency check for eligibility.
contracts/src/arbitration/interfaces/ISortitionModule.sol Updated penalizeStake to return values, added withdrawLeftoverPNK and updateState functions.
contracts/src/arbitration/university/SortitionModuleUniversity.sol Updated penalizeStake return, added empty withdrawLeftoverPNK and updateState functions.
contracts/test/foundry/KlerosCore.t.sol Updated tests for new delayed stake, penalty, and reward logic; added/removed relevant tests and assertions.
contracts/src/libraries/Constants.sol Added Delayed variant to StakingResult enum.

Sequence Diagram(s)

sequenceDiagram
    participant User
    participant KlerosCore
    participant SortitionModule

    User->>KlerosCore: setStakeBySortitionModule(account, courtID, newStake, _)
    KlerosCore->>SortitionModule: setStake(account, courtID, newStake, false)
    SortitionModule-->>KlerosCore: (StakingResult)
    KlerosCore->>SortitionModule: updateState(account, courtID, deposit, withdrawal, newStake)
Loading
sequenceDiagram
    participant KlerosCore
    participant SortitionModule

    KlerosCore->>SortitionModule: penalizeStake(account, relativeAmount)
    SortitionModule-->>KlerosCore: (pnkBalance, availablePenalty)
    KlerosCore->>KlerosCore: update penalty accounting, set juror inactive if needed
Loading
sequenceDiagram
    participant User
    participant SortitionModule

    User->>SortitionModule: withdrawLeftoverPNK(account)
    SortitionModule-->>User: Transfer leftover PNK, emit event
Loading

Possibly related PRs

Suggested labels

Type: Enhancement :sparkles:

Suggested reviewers

  • jaybuidl

Poem

In the fields where PNK bunnies stake,
Old flags are gone, new contracts awake.
Delayed no more, the tokens hop—
Penalties counted, rewards never stop.
With every update, the system grows bright,
Blockchain justice—rabbits get it right!
🐇✨


📜 Recent review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between d128f10 and f7591d2.

📒 Files selected for processing (8)
  • contracts/src/arbitration/KlerosCoreBase.sol (5 hunks)
  • contracts/src/arbitration/SortitionModuleBase.sol (8 hunks)
  • contracts/src/arbitration/SortitionModuleNeo.sol (2 hunks)
  • contracts/src/arbitration/dispute-kits/DisputeKitClassicBase.sol (2 hunks)
  • contracts/src/arbitration/interfaces/ISortitionModule.sol (2 hunks)
  • contracts/src/arbitration/university/SortitionModuleUniversity.sol (3 hunks)
  • contracts/src/libraries/Constants.sol (1 hunks)
  • contracts/test/foundry/KlerosCore.t.sol (10 hunks)
✅ Files skipped from review due to trivial changes (1)
  • contracts/src/libraries/Constants.sol
🚧 Files skipped from review as they are similar to previous changes (5)
  • contracts/src/arbitration/interfaces/ISortitionModule.sol
  • contracts/src/arbitration/SortitionModuleNeo.sol
  • contracts/src/arbitration/dispute-kits/DisputeKitClassicBase.sol
  • contracts/src/arbitration/university/SortitionModuleUniversity.sol
  • contracts/src/arbitration/KlerosCoreBase.sol
⏰ Context from checks skipped due to timeout of 90000ms (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-university
  • GitHub Check: Header rules - kleros-v2-university
  • GitHub Check: contracts-testing
  • GitHub Check: Analyze (javascript)
  • GitHub Check: Pages changed - kleros-v2-university
  • GitHub Check: Redirect rules - kleros-v2-testnet-devtools
  • GitHub Check: Header rules - kleros-v2-testnet-devtools
  • GitHub Check: Pages changed - kleros-v2-testnet-devtools
🔇 Additional comments (15)
contracts/test/foundry/KlerosCore.t.sol (6)

1013-1013: Event name change looks correct.

The event emission has been updated to StakeDelayed which aligns with the PR objective to rename from StakeDelayedAlreadyTransferred.


1019-1025: Inconsistency with deprecated parameter usage.

The test is still checking the alreadyTransferred field, but according to the PR summary, the deprecated _alreadyTransferred parameter was removed. This suggests either the test wasn't fully updated or the field still exists with different behavior.

Please verify if the alreadyTransferred field should still be present in the delayedStakes struct and clarify the expected behavior.

#!/bin/bash
# Search for alreadyTransferred field usage in the sortition module contracts
rg -A 5 -B 5 "alreadyTransferred" --type sol

Likely an incorrect or invalid review comment.


1029-1029: Token balance assertions correctly reflect delayed staking behavior.

The assertions properly verify that during the drawing phase, token balances and staked amounts don't change until delayed stakes are executed in the staking phase. This aligns with the updated delayed stake handling described in the AI summary.

Also applies to: 1039-1041


1094-1119: Locked token handling logic is correctly implemented.

The test properly verifies that:

  • Locked tokens persist after unstaking and cannot be withdrawn prematurely
  • Subsequent staking correctly accounts for existing locked tokens
  • Total staked amount reflects the sum of new stake and locked tokens

This aligns with the updated stake state management described in the AI summary.


2387-2452: Excellent addition of comprehensive unstaking scenario tests.

The new test functions test_execute_UnstakeInactive and test_execute_UnstakeInsolvent provide critical coverage for edge cases in the updated penalty and reward execution logic:

  • UnstakeInactive: Properly tests automatic unstaking across multiple courts when penalties are applied
  • UnstakeInsolvent: Correctly handles scenarios where locked tokens exceed staked tokens, verifying insolvency handling

Both tests include thorough setup, execution, and verification of token balances and state changes.

Also applies to: 2454-2510


2512-2587: Comprehensive test coverage for new withdrawLeftoverPNK functionality.

The test_execute_withdrawLeftoverPNK function excellently tests the new leftover PNK withdrawal mechanism introduced in the PR:

Key test scenarios covered:

  • Eligibility checks (withdrawal only after execution)
  • Access control verification (SortitionModuleOnly modifier)
  • Proper token transfer and balance updates
  • Complete state cleanup after withdrawal

The test properly verifies that leftover locked tokens cannot be withdrawn prematurely and that only the sortition module can transfer tokens on behalf of the core contract, aligning perfectly with the PR objectives.

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

42-42: LGTM: Proper deprecation marking

The field is correctly marked as deprecated with a clear comment explaining its previous purpose.


71-71: LGTM: Consistent deprecation marking

The mapping is properly marked as deprecated, consistent with the removal of the alreadyTransferred functionality.


84-106: LGTM: Event consolidation improves clarity

The consolidation of multiple delayed stake events into a single StakeDelayed event simplifies the interface while maintaining necessary information.


114-117: LGTM: Well-defined event for new functionality

The new event properly documents the leftover PNK withdrawal functionality with appropriate indexed parameters.


244-250: LGTM: Simplified delayed stake execution

The removal of conditional logic around alreadyTransferred simplifies the execution flow while maintaining correctness.


283-285: LGTM: Proper parameter deprecation handling

The function correctly ignores the deprecated _alreadyTransferred parameter and passes a fixed value to the internal function.


388-405: LGTM: Improved penalty handling with return values

The modified function correctly returns the updated balance and actual penalty applied, which improves transparency and enables better accounting in the calling contract.


332-376:

✅ Verification successful

Optimize array cleanup and verify state consistency

The updateState function correctly separates state management from stake calculation, but the array cleanup logic could be gas-intensive for jurors staked in many courts.

Consider optimizing the court ID cleanup:

-            if (_newStake == 0) {
-                // Cleanup
-                for (uint256 i = juror.courtIDs.length; i > 0; i--) {
-                    if (juror.courtIDs[i - 1] == _courtID) {
-                        juror.courtIDs[i - 1] = juror.courtIDs[juror.courtIDs.length - 1];
-                        juror.courtIDs.pop();
-                        break;
-                    }
-                }
-            }
+            if (_newStake == 0) {
+                // Cleanup - consider using mapping for O(1) lookups if gas becomes an issue
+                _removeCourtID(juror, _courtID);
+            }

🏁 Script executed:

#!/bin/bash
# Description: Check if there are any constraints on the maximum number of courts a juror can stake in
# Expected: Find MAX_STAKE_PATHS definition and usage

echo "Searching for maximum stake paths constraints..."
rg "MAX_STAKE_PATHS" -A 3 -B 3

echo "Searching for court ID array size related code..."
rg "courtIDs\.length" -A 5 -B 5

Length of output: 65730


Court ID cleanup is constant time – MAX_STAKE_PATHS caps loops
The juror.courtIDs array is limited by MAX_STAKE_PATHS (4), so the clean-up loop in updateState iterates at most four times, incurring negligible gas overhead. You can still extract the removal logic into an internal helper (e.g., _removeCourtID) for clarity, but a mapping for O(1) lookups isn’t necessary here.


421-439:

❓ Verification inconclusive

Verify security of leftover PNK withdrawal

The withdrawal function has proper checks but needs verification for potential edge cases and reentrancy protection.

Potential edge case concern: Consider what happens if the core.transferBySortitionModule call fails or reverts. The function should either handle this gracefully or ensure the failure doesn't leave the contract in an inconsistent state.

Consider adding error handling:

 function withdrawLeftoverPNK(address _account) external override {
     Juror storage juror = jurors[_account];
     if (juror.stakedPnk > 0 && juror.courtIDs.length == 0 && juror.lockedPnk == 0) {
         uint256 amount = juror.stakedPnk;
         juror.stakedPnk = 0;
-        core.transferBySortitionModule(_account, amount);
-        emit LeftoverPNKWithdrawn(_account, amount);
+        try core.transferBySortitionModule(_account, amount) {
+            emit LeftoverPNKWithdrawn(_account, amount);
+        } catch {
+            // Revert state change if transfer fails
+            juror.stakedPnk = amount;
+            revert("Transfer failed");
+        }
     } else {
         revert("Not eligible for withdrawal.");
     }
 }

🏁 Script executed:

#!/bin/bash
# Description: Verify the security aspects of the withdrawLeftoverPNK function
# Expected: Find related security checks and reentrancy protection measures

echo "Checking for reentrancy guards in the contract..."
rg "nonReentrant|ReentrancyGuard" -A 2 -B 2

echo "Searching for core.transferBySortitionModule implementation..."
rg "transferBySortitionModule" -A 10 -B 5

echo "Looking for related security tests..."
rg -A 15 "test.*[Ll]eftover|test.*[Ww]ithdraw.*PNK" --type solidity

Length of output: 65762


Review withdrawLeftoverPNK for reentrancy and error handling

The function correctly uses the checks-effects-interactions pattern (it zeroes out juror.stakedPnk before calling the external core.transferBySortitionModule), so any revert in the transfer will roll back the state change. No explicit nonReentrant guard or try/catch was found in this module, and I didn’t locate any existing tests covering this path.

Recommendations:

  • Confirm that core.transferBySortitionModule (in the KlerosCore contract) is audited, does not invoke callbacks into this module, and bubbles up errors as expected.
  • Add unit tests for withdrawLeftoverPNK covering:
    • Successful withdrawal when all eligibility conditions are met
    • Reverts when ineligible (e.g., stakedPnk == 0, non-empty courtIDs, or lockedPnk > 0)
    • A simulated failure of transferBySortitionModule to verify the rollback of state
  • If extra safety is desired, consider applying OpenZeppelin’s ReentrancyGuard and the nonReentrant modifier.

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share
🪧 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.
    • Explain this complex logic.
    • 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. Examples:
    • @coderabbitai explain this code block.
    • @coderabbitai modularize this function.
  • 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 src/utils.ts and explain its main purpose.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

Support

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

Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments.

CodeRabbit Commands (Invoked using PR comments)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai generate docstrings to generate docstrings for this PR.
  • @coderabbitai generate sequence diagram to generate a sequence diagram of the changes in this PR.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai 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

Documentation and Community

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

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: 1

🔭 Outside diff range comments (1)
contracts/src/arbitration/university/SortitionModuleUniversity.sol (1)

238-248: ⚠️ Potential issue

Function signature updated but missing return values

While the function signature has been updated to match the interface, the function doesn't actually set or return the declared values pnkBalance and availablePenalty. This needs to be fixed to properly implement the interface.

Update the function to return the required values:

function penalizeStake(
    address _account,
    uint256 _relativeAmount
) external override onlyByCore returns (uint256 pnkBalance, uint256 availablePenalty) {
    Juror storage juror = jurors[_account];
    if (juror.stakedPnk >= _relativeAmount) {
        juror.stakedPnk -= _relativeAmount;
+       availablePenalty = _relativeAmount;
    } else {
+       availablePenalty = juror.stakedPnk;
        juror.stakedPnk = 0; // stakedPnk might become lower after manual unstaking, but lockedPnk will always cover the difference.
    }
+   pnkBalance = juror.stakedPnk;
+   return (pnkBalance, availablePenalty);
}
🧹 Nitpick comments (12)
contracts/src/arbitration/university/SortitionModuleUniversity.sol (1)

264-265: Empty implementation of withdrawLeftoverPNK

This is an empty implementation of the required interface function. While technically compliant with the interface, this implementation doesn't actually withdraw any leftover PNK. If this is intentional for the University module, consider adding a comment explaining why.

Consider adding a clarifying comment:

-function withdrawLeftoverPNK(address _account) external override {}
+function withdrawLeftoverPNK(address _account) external override {
+    // No implementation needed for University module as it doesn't track leftover PNK
+}
contracts/src/arbitration/KlerosCoreBase.sol (2)

474-482: Deprecate the unused _alreadyTransferred parameter to save deployment & call gas

setStakeBySortitionModule keeps the parameter in the ABI but never uses it (/* _alreadyTransferred */).
If no externally-deployed SortitionModule still relies on the 4-argument signature, consider removing the parameter entirely (and adjusting the interface) or adding a dedicated deprecated overload to keep backwards compatibility. Eliminating the dead parameter shaves 3-5 gas per call and simplifies the API.


1083-1099: Follow-up: remove deprecated boolean in call to setStake

sortitionModule.setStake(..., false) passes a hard-coded value that is now unused.
After cleaning up the interface (see comment above), this fourth argument and the trailing comment can be dropped to avoid confusion:

-        (uint256 pnkDeposit, uint256 pnkWithdrawal, StakingResult stakingResult) = sortitionModule.setStake(
-            _account,
-            _courtID,
-            _newStake,
-            false // Unused parameter.
-        );
+        (uint256 pnkDeposit, uint256 pnkWithdrawal, StakingResult stakingResult) =
+            sortitionModule.setStake(_account, _courtID, _newStake);
contracts/test/foundry/KlerosCore.t.sol (5)

1019-1026: Clarify assertion description to match the expected flag value

alreadyTransferred is asserted to be false, yet the accompanying message says “Should be flagged as transferred”.
This wording is confusing because a value of false actually means “not transferred”. Renaming the
message to something like “Should be not flagged as transferred” (or the opposite, depending on the intended
semantics) will prevent mis-interpretation when the test fails.


1094-1119: Replace magic numbers with expressions for better readability & resilience

The balance assertions use large hard-coded literals such as
999999999999998200.
Although they are numerically correct (1 ether - 1800), the intent is obscured and any future change
to the staked amount will require hunting down and editing multiple constants.

Consider computing the expected value on-the-fly, e.g.

uint256 expected = 1 ether - 1_800;
assertEq(pinakion.balanceOf(staker1), expected, "Wrong token balance of staker1");

or even

assertEq(
    pinakion.balanceOf(staker1),
    1 ether - (1500 /* first stake */ + 300 /* diff */),
    "Wrong token balance of staker1"
);

This keeps the arithmetic obvious, avoids copy-paste mistakes, and makes the test less brittle.


1203-1210: Impersonating the Core to self-transfer can hide real-world constraints

vm.prank(address(core)); pinakion.transfer(governor, 10000); works in the
unit test, but in production KlerosCore has no such method and cannot trigger ERC-20 transfer
directly. If the goal is only to zero the Core’s balance, consider minting a fresh token or using
deal(address(pinakion), address(core), 0) (forge cheat-code) instead.

This keeps the test closer to real behaviour and avoids accidentally relying on “the contract calls
transfer on itself”
paths that will never exist on-chain.


2454-2510: Hard-coded iteration counts & stake figures make new tests brittle

core.execute(disputeID, 0, 6) assumes that exactly six repartition iterations are always required.
The actual number depends on internal constants such as DEFAULT_NB_OF_JURORS, future changes to
penalty logic, or even gas-cap tweaks. The same applies to the literal 3000 penalty math and the
final balance check for a full 1 ether.

Suggestion:

uint256 iterations = DEFAULT_NB_OF_JURORS * 2; // worst-case upper bound
core.execute(disputeID, 0, iterations);

and compute expected balances from recorded pre-/post-state instead of fixed literals.
Doing so will prevent spurious failures when business rules evolve while preserving
the intent of proving “juror becomes insolvent and is automatically unstaked”.


2532-2574: Leverage helper functions to minimise duplicated balance assertions

withdrawLeftoverPNK is tested with a long sequence of assertEq calls that
repeat the “balance before / after” pattern already present in several other stake-related tests.
Extracting a small internal helper:

function _assertPnkBalances(
    uint256 coreBal,
    uint256 jurorBal,
    string memory msgSuffix
) internal {
    assertEq(pinakion.balanceOf(address(core)), coreBal, string.concat("Core ", msgSuffix));
    assertEq(pinakion.balanceOf(staker1), jurorBal, string.concat("Juror ", msgSuffix));
}

and re-using it across the suite will reduce noise, ease future refactors, and make the intent of
each check clearer.

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

69-72: Consider explicitly annotating deprecated storage to reduce cognitive overhead

latestDelayedStakeIndex is kept for storage-layout compatibility, but it now serves no functional purpose.
Adding an /// @deprecated NatSpec tag (or // DEPRECATED: kept for storage-layout) immediately above the declaration would make the intent clearer to future maintainers and external auditors.


84-100: Mark legacy events with @deprecated to avoid accidental reuse

The three StakeDelayed* events are retained but unused. Emitting them elsewhere by mistake would silently revive dead semantics.
Consider:

-/// @notice DEPRECATED Emitted when a juror's stake is delayed and tokens are not transferred yet.
+/// @deprecated Replaced by `StakeDelayed`. Kept for ABI compatibility with historic logs.

Doing so on all three legacy events keeps the ABI unchanged while signalling that integrators should subscribe only to StakeDelayed.


242-253: Guard against zero-iteration calls in executeDelayedStakes

Passing _iterations == 0 wastes gas and returns silently. A tiny require improves UX and prevents accidental no-op transactions:

 function executeDelayedStakes(uint256 _iterations) external {
     require(phase == Phase.staking, "Should be in Staking phase.");
     require(delayedStakeWriteIndex >= delayedStakeReadIndex, "No delayed stake to execute.");
+    require(_iterations > 0, "Iterations must be > 0");

411-423: Minor polish for withdrawLeftoverPNK

  1. The function is callable by anyone for any _account, which is harmless but unusual.
    If the goal is to let third parties “dust-sweep” for jurors, document this explicitly; otherwise consider require(msg.sender == _account, "Only juror");.

  2. Replace the if / else { revert(...) } pattern with a single require to save gas and keep style consistent:

-function withdrawLeftoverPNK(address _account) external override {
+function withdrawLeftoverPNK(address _account) external override {
     Juror storage juror = jurors[_account];
-    if (juror.stakedPnk > 0 && juror.courtIDs.length == 0 && juror.lockedPnk == 0) {
-        ...
-    } else {
-        revert("Not eligible for withdrawal.");
-    }
+    require(
+        juror.stakedPnk > 0 && juror.courtIDs.length == 0 && juror.lockedPnk == 0,
+        "Not eligible for withdrawal"
+    );
+    ...
 }
📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 1ca9066 and d876e24.

📒 Files selected for processing (7)
  • contracts/src/arbitration/KlerosCoreBase.sol (4 hunks)
  • contracts/src/arbitration/SortitionModuleBase.sol (8 hunks)
  • contracts/src/arbitration/SortitionModuleNeo.sol (2 hunks)
  • contracts/src/arbitration/dispute-kits/DisputeKitClassicBase.sol (2 hunks)
  • contracts/src/arbitration/interfaces/ISortitionModule.sol (2 hunks)
  • contracts/src/arbitration/university/SortitionModuleUniversity.sol (2 hunks)
  • contracts/test/foundry/KlerosCore.t.sol (10 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (3)
  • GitHub Check: Analyze (javascript)
  • GitHub Check: SonarCloud
  • GitHub Check: contracts-testing
🔇 Additional comments (9)
contracts/src/arbitration/dispute-kits/DisputeKitClassicBase.sol (1)

609-630: Simplified juror eligibility check improves code clarity

The _postDrawCheck function has been simplified to only check if the juror has already been drawn when singleDrawPerJuror is enabled, removing previous stake sufficiency checks. This aligns with the broader refactoring to simplify staking logic across the arbitration system.

contracts/src/arbitration/interfaces/ISortitionModule.sol (2)

30-34: Enhanced penalty accounting with detailed return values

The updated return signature for penalizeStake now provides more detailed information by returning both the juror's updated PNK balance and the actual penalty applied. This change enables more precise accounting of penalties in the core arbitration contract.


52-53: New function for withdrawing leftover PNK tokens

This new function enables jurors to withdraw any remaining PNK tokens after fully unstaking and clearing their locked tokens, improving the user experience by ensuring no funds are trapped in the contract.

contracts/src/arbitration/SortitionModuleNeo.sol (3)

87-92: Properly marking deprecated parameter

The _alreadyTransferred parameter has been marked as unused with a comment, clearly indicating its deprecation as part of removing instant staking functionality.


97-104: Simplified conditional logic for stake increases

The conditional logic has been streamlined to only check maximum stake limits when the stake is being increased, making the code more focused and easier to understand.


112-117: Explicit handling of deprecated parameter

The code now explicitly passes false for the deprecated _alreadyTransferred parameter with a clarifying comment, ensuring proper handling while maintaining compatibility with the base contract.

contracts/src/arbitration/KlerosCoreBase.sol (1)

789-803: Double-unlock edge case: verify that unlocked but un-penalised PNK cannot be withdrawn

The flow is:

  1. unlockStake(account, penalty);
  2. penalizeStake(account, penalty) → returns availablePenalty (may be < penalty).

If the juror’s locked stake is smaller than penalty, the surplus tokens get unlocked in step 1 but are not slashed in step 2, letting the juror immediately withdraw them-–potentially rewarding under-collateralised behaviour.

Please confirm that sortitionModule.unlockStake prevents unlocking more than currently locked or that penalizeStake internally adjusts the unlocked amount. If not, swap the order (penalise first, then unlock the actual availablePenalty) or unlock only availablePenalty.

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

327-334: Double-check tree consistency when withdrawals are limited by locked PNK

If lockedPnk prevents a full withdrawal, pnkWithdrawal is trimmed but _newStake remains unchanged.
This deliberately produces “left-over” PNK that can later be withdrawn via withdrawLeftoverPNK, so it may be correct by design.
However, it temporarily breaks the invariant juror.stakedPnk == Σ(tree stakes) until the leftover is withdrawn.

Please verify that all downstream logic (drawing, penalties, etc.) tolerates this mismatch; otherwise consider updating _newStake after trimming:

-    // _newStake is still the user-requested target (might be < effective stake)
+    // Align stake recorded in the tree with the actual post-withdrawal amount.
+    _newStake = currentStake - pnkWithdrawal;

372-389: Potential invariant break: lockedPnk can exceed stakedPnk after penalties

penalizeStake reduces stakedPnk but never touches lockedPnk.
If lockedPnk > stakedPnk afterwards, subsequent calls to unlockStake could underflow or leave stakedPnk negative in intent (though uint256 prevents it).

Please confirm that lockedPnk ≤ stakedPnk is not relied upon elsewhere (e.g., when drawing, displaying balances, or executing further penalties).
An inexpensive safeguard could be:

assert(juror.stakedPnk >= juror.lockedPnk);

or clamping the penalty to stakedPnk - lockedPnk.

Comment on lines +483 to 490
/// @dev Transfers PNK to the juror by SortitionModule.
/// @param _account The account of the juror whose PNK to transfer.
/// @param _amount The amount to transfer.
function transferBySortitionModule(address _account, uint256 _amount) external {
if (msg.sender != address(sortitionModule)) revert SortitionModuleOnly();
// Note eligibility is checked in SortitionModule.
pinakion.safeTransfer(_account, _amount);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Emit an event for PNK moved via transferBySortitionModule

transferBySortitionModule performs an external token transfer but leaves no on-chain trace inside Core.
For transparency, accounting and easier off-chain indexing, emit a lightweight event such as:

+ event SortitionPNKTransfer(address indexed account, uint256 amount);
...
 function transferBySortitionModule(address _account, uint256 _amount) external {
     if (msg.sender != address(sortitionModule)) revert SortitionModuleOnly();
     pinakion.safeTransfer(_account, _amount);
+    emit SortitionPNKTransfer(_account, _amount);
 }

This incurs negligible gas yet gives jurors and auditors full visibility.

🤖 Prompt for AI Agents
In contracts/src/arbitration/KlerosCoreBase.sol around lines 483 to 490, the
function transferBySortitionModule transfers PNK tokens without emitting any
event, which reduces transparency and traceability. Add an event declaration for
PNK transfers by the SortitionModule and emit this event inside the function
after the safeTransfer call, including the recipient address and the amount
transferred to provide on-chain visibility and facilitate off-chain indexing.

Copy link

netlify bot commented May 20, 2025

Deploy Preview for kleros-v2-testnet ready!

Name Link
🔨 Latest commit f7591d2
🔍 Latest deploy log https://app.netlify.com/projects/kleros-v2-testnet/deploys/6839e324239e760008e6b652
😎 Deploy Preview https://deploy-preview-2004--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 May 20, 2025

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

Name Link
🔨 Latest commit f7591d2
🔍 Latest deploy log https://app.netlify.com/projects/kleros-v2-university/deploys/6839e324c53f1c000831f56c

Copy link

netlify bot commented May 20, 2025

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

Name Link
🔨 Latest commit f7591d2
🔍 Latest deploy log https://app.netlify.com/projects/kleros-v2-testnet-devtools/deploys/6839e324a9577100084d90c7
😎 Deploy Preview https://deploy-preview-2004--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

netlify bot commented May 20, 2025

Deploy Preview for kleros-v2-neo ready!

Name Link
🔨 Latest commit f7591d2
🔍 Latest deploy log https://app.netlify.com/projects/kleros-v2-neo/deploys/6839e324392bff000846805c
😎 Deploy Preview https://deploy-preview-2004--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

🔭 Outside diff range comments (1)
subgraph/core/abi-migrations/SortitionModule.json (1)

823-843: 💡 Verification agent

🧩 Analysis chain

Invalid unnamed boolean input in setStake
The input list for setStake still contains an unnamed bool parameter (formerly _alreadyTransferred), which is invalid in the ABI and will break code generation. You should either remove this entry entirely or assign it a proper name to match the contract signature.

Apply this diff to remove it:

--- before
         {
           "internalType": "bool",
-          "name": "",
+          "name": "",         // <–– remove this block if the parameter is deprecated
           "type": "bool"
         }
--- after
 // block removed

🏁 Script executed:

#!/usr/bin/env bash
# Extract the full ABI entry for the `setStake` function to verify its inputs
jq '.[] | select(.name == "setStake")' subgraph/core/abi-migrations/SortitionModule.json

Length of output: 198


🏁 Script executed:

#!/usr/bin/env bash
# Attempt to extract the `setStake` entry from the ABI
jq '.abi[] | select(.name=="setStake")' subgraph/core/abi-migrations/SortitionModule.json \
  || rg -n '"name": "setStake"' -A 10 -B 10 subgraph/core/abi-migrations/SortitionModule.json

Length of output: 984


🏁 Script executed:

#!/usr/bin/env bash
# Locate the Solidity definition of setStake to confirm the parameter name
rg -n "function setStake" -g "*.sol" -C 3

Length of output: 6473


Remove Invalid Unnamed Boolean from setStake ABI
The ABI for setStake currently includes a fourth, unnamed bool parameter that isn’t part of the contract’s external signature (address, uint96, uint256 only). This extra input breaks codegen and must be removed.

• File: subgraph/core/abi-migrations/SortitionModule.json (inputs around lines 823–843)
• Function: setStake(address _account, uint96 _courtID, uint256 _newStake)

Apply this diff to drop the stray boolean block:

--- a/subgraph/core/abi-migrations/SortitionModule.json
+++ b/subgraph/core/abi-migrations/SortitionModule.json
@@ -823,10 +823,6 @@
         {
           "internalType": "uint256",
           "name": "_newStake",
           "type": "uint256"
         },
-        {
-          "internalType": "bool",
-          "name": "",
-          "type": "bool"
-        },
         {
           "internalType": "uint256",
           "name": "pnkDeposit",
           "type": "uint256"
         },
🧹 Nitpick comments (2)
subgraph/core/abi-migrations/SortitionModule.json (2)

67-81: Added LeftoverPNKWithdrawn event
Introduces the LeftoverPNKWithdrawn(address _account, uint256 _amount) event for leftover PNK withdrawals. Make sure your subgraph’s GraphQL schema and mapping handlers are updated to index this new event.


954-961: New withdrawLeftoverPNK function
Adds withdrawLeftoverPNK(address _account) for direct PNK withdrawals. Update any subgraph custom handlers or scripts that invoke this function if you intend to index these calls.

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between d876e24 and d128f10.

📒 Files selected for processing (6)
  • subgraph/core-neo/abi-migrations/SortitionModuleNeo.json (6 hunks)
  • subgraph/core-neo/subgraph.yaml (1 hunks)
  • subgraph/core/abi-migrations/SortitionModule.json (7 hunks)
  • subgraph/core/src/SortitionModule.ts (1 hunks)
  • subgraph/core/subgraph.yaml (1 hunks)
  • subgraph/temp-older-events-addition.txt (1 hunks)
✅ Files skipped from review due to trivial changes (1)
  • subgraph/temp-older-events-addition.txt
⏰ Context from checks skipped due to timeout of 90000ms (7)
  • 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-testnet
  • GitHub Check: Header rules - kleros-v2-testnet
  • GitHub Check: Pages changed - kleros-v2-testnet
  • GitHub Check: contracts-testing
🔇 Additional comments (14)
subgraph/core/subgraph.yaml (1)

162-163: LGTM! Event consolidation simplifies the handling logic.

The consolidation of multiple StakeDelayed event variants into a single unified event handler aligns well with the contract changes that removed the deprecated _alreadyTransferred parameter. This simplification reduces complexity and maintains consistency across the system.

subgraph/core-neo/subgraph.yaml (1)

161-162: LGTM! Consistent event handling across networks.

The changes mirror those in the core network subgraph, ensuring consistent event handling across both arbitrum-sepolia and arbitrum-one networks. The unified StakeDelayed event signature maintains compatibility while simplifying the codebase.

subgraph/core/src/SortitionModule.ts (2)

1-1: LGTM! Import statement correctly reflects the consolidated events.

The import statement now includes only the necessary events after the consolidation, removing the deprecated variants. This keeps the imports clean and aligned with the contract changes.


7-9: LGTM! Unified handler simplifies event processing.

The single handleStakeDelayed function correctly extracts all necessary parameters (_address, _courtID, _amount) from the consolidated event. This approach eliminates code duplication while maintaining full functionality.

subgraph/core-neo/abi-migrations/SortitionModuleNeo.json (6)

69-80: LGTM! Event rename clarifies its purpose.

The rename from StakeDelayedAlreadyTransferredDeposited to LeftoverPNKWithdrawn makes the event name more descriptive and aligns with its actual functionality of withdrawing leftover PNK tokens.


87-93: LGTM! Simplified NewPhase event reduces complexity.

The NewPhase event now only includes the essential _phase parameter as a uint8 enum, removing unnecessary court ID and amount parameters that were previously included. This simplification makes the event more focused and easier to handle.


117-118: LGTM! Consolidated StakeDelayed event improves consistency.

The unified StakeDelayed event with indexed address and uint96 court ID parameters, along with the uint256 amount, provides a clean interface that eliminates the need for multiple event variants.


742-753: LGTM! Enhanced penalizeStake function provides better accountability.

The updated penalizeStake function now returns both pnkBalance and availablePenalty values, which improves transparency by providing information about the juror's remaining balance and the actual penalty applied. This is valuable for accurate penalty accounting.


885-905: LGTM! setStake function improvements enhance functionality.

The changes to setStake include:

  • Unnamed boolean parameter (formerly _alreadyTransferred) indicating deprecation
  • Additional oldStake return value for better state tracking

These changes improve the function's utility while maintaining backward compatibility through the unnamed parameter.


1024-1037: LGTM! New withdrawLeftoverPNK function adds important functionality.

The addition of withdrawLeftoverPNK function provides jurors with a mechanism to withdraw any remaining PNK tokens after full unstaking. This is an important feature for complete token recovery and aligns with the overall staking logic improvements.

subgraph/core/abi-migrations/SortitionModule.json (4)

4-7: Explicit no-args constructor declared
The ABI now includes a parameterless constructor (type: "constructor") and has removed any fallback/receive entries to align with the updated contract deployment.


87-93: Consolidated NewPhase event
Replaces legacy phase‐related events with a single NewPhase(uint8 _phase) event. Verify that your subgraph mappings now listen to NewPhase instead of the old events.


114-118: Updated StakeDelayed event
All delayed‐stake events have been merged into StakeDelayed(address _address, uint96 _courtID, uint256 _amount). Ensure your subgraph only handles this consolidated event going forward.


679-691: penalizeStake now returns actual balances
The penalizeStake function signature was changed to return (uint256 pnkBalance, uint256 availablePenalty). If your subgraph makes contract calls to this function, update the ABICall definitions accordingly.

Copy link

codeclimate bot commented May 30, 2025

Code Climate has analyzed commit f7591d2 and detected 0 issues on this pull request.

View more on Code Climate.

Copy link

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants