fix(SortitionModule): fix staking logic and remove instant staking #2004
Conversation
WalkthroughThe changes remove the deprecated Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant KlerosCoreBase
participant SortitionModule
participant PNKToken
User->>KlerosCoreBase: setStakeBySortitionModule(account, courtID, newStake, _)
KlerosCoreBase->>SortitionModule: setStake(account, courtID, newStake, false)
SortitionModule->>SortitionModule: Update juror's stakedPnk
Note right of SortitionModule: Delayed stake logic simplified
User->>KlerosCoreBase: transferBySortitionModule(account, amount)
KlerosCoreBase->>PNKToken: Transfer PNK to account
KlerosCoreBase->>SortitionModule: penalizeStake(account, relativeAmount)
SortitionModule->>SortitionModule: Reduce stakedPnk, return (pnkBalance, availablePenalty)
KlerosCoreBase->>KlerosCoreBase: Update penalty accounting
User->>SortitionModule: withdrawLeftoverPNK(account)
SortitionModule->>PNKToken: Transfer leftover PNK to account
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Note ⚡️ AI Code Reviews for VS Code, Cursor, WindsurfCodeRabbit now has a plugin for VS Code, Cursor and Windsurf. This brings AI code reviews directly in the code editor. Each commit is reviewed immediately, finding bugs before the PR is raised. Seamless context handoff to your AI code agent ensures that you can easily incorporate review feedback. Note ⚡️ Faster reviews with cachingCodeRabbit now supports caching for code and dependencies, helping speed up reviews. This means quicker feedback, reduced wait times, and a smoother review experience overall. Cached data is encrypted and stored securely. This feature will be automatically enabled for all accounts on May 16th. To opt out, configure 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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed 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)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
Code Climate has analyzed commit d876e24 and detected 0 issues on this pull request. View more on Code Climate. |
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
contracts/src/arbitration/university/SortitionModuleUniversity.sol (1)
238-248:⚠️ Potential issueFunction 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
pnkBalanceandavailablePenalty. 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 withdrawLeftoverPNKThis 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_alreadyTransferredparameter to save deployment & call gas
setStakeBySortitionModulekeeps 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 tosetStake
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
alreadyTransferredis asserted to befalse, yet the accompanying message says “Should be flagged as transferred”.
This wording is confusing because a value offalseactually 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 & resilienceThe 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 productionKlerosCorehas no such method and cannot trigger ERC-20transfer
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 asDEFAULT_NB_OF_JURORS, future changes to
penalty logic, or even gas-cap tweaks. The same applies to the literal3000penalty math and the
final balance check for a full1 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
withdrawLeftoverPNKis tested with a long sequence ofassertEqcalls 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
latestDelayedStakeIndexis kept for storage-layout compatibility, but it now serves no functional purpose.
Adding an/// @deprecatedNatSpec 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 reuseThe 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 inexecuteDelayedStakesPassing
_iterations == 0wastes 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 forwithdrawLeftoverPNK
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 considerrequire(msg.sender == _account, "Only juror");.Replace the
if / else { revert(...) }pattern with a singlerequireto 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
📒 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 clarityThe
_postDrawCheckfunction has been simplified to only check if the juror has already been drawn whensingleDrawPerJuroris 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 valuesThe updated return signature for
penalizeStakenow 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 tokensThis 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 parameterThe
_alreadyTransferredparameter 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 increasesThe 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 parameterThe code now explicitly passes
falsefor the deprecated_alreadyTransferredparameter 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 withdrawnThe flow is:
unlockStake(account, penalty);penalizeStake(account, penalty)→ returnsavailablePenalty(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.unlockStakeprevents unlocking more than currently locked or thatpenalizeStakeinternally adjusts the unlocked amount. If not, swap the order (penalise first, then unlock the actualavailablePenalty) or unlock onlyavailablePenalty.contracts/src/arbitration/SortitionModuleBase.sol (2)
327-334: Double-check tree consistency when withdrawals are limited by locked PNKIf
lockedPnkprevents a full withdrawal,pnkWithdrawalis trimmed but_newStakeremains unchanged.
This deliberately produces “left-over” PNK that can later be withdrawn viawithdrawLeftoverPNK, so it may be correct by design.
However, it temporarily breaks the invariantjuror.stakedPnk == Σ(tree stakes)until the leftover is withdrawn.Please verify that all downstream logic (drawing, penalties, etc.) tolerates this mismatch; otherwise consider updating
_newStakeafter 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:lockedPnkcan exceedstakedPnkafter penalties
penalizeStakereducesstakedPnkbut never toucheslockedPnk.
IflockedPnk > stakedPnkafterwards, subsequent calls tounlockStakecould underflow or leavestakedPnknegative in intent (though uint256 prevents it).Please confirm that
lockedPnk ≤ stakedPnkis 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.
| /// @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); | ||
| } |
There was a problem hiding this comment.
🛠️ 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.
✅ 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-university failed. Why did it fail? →
|
✅ 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. |
jaybuidl
added
Type: Bug 🐛
Compatibility: ABI change 🗯
PR-Codex overview
This PR focuses on enhancing the
ISortitionModuleinterface and its implementations by adding new functionalities and improving existing ones, particularly around stake management and penalty processing.Detailed summary
withdrawLeftoverPNK(address _account)function toISortitionModuleand its implementations.penalizeStakefunction to returnpnkBalanceandavailablePenalty.Summary by CodeRabbit
New Features
Improvements
Bug Fixes
Tests