fix(SortitionModule): fix staking logic and remove instant staking #2004
Conversation
WalkthroughThis update removes the deprecated Changes
Sequence Diagram(s)sequenceDiagram participant User participant Core as KlerosCoreBase participant Sortition as SortitionModuleBase participant Token as PNKToken User->>Core: setStakeBySortitionModule(account, courtID, newStake) Core->>Sortition: validateStake(account, courtID, newStake) alt Staking phase Sortition->>Token: Transfer PNK (if deposit) Sortition->>Token: Withdraw PNK (if withdrawal) Sortition-->>Core: Return deposit, withdrawal, Success Core->>Sortition: setStake(account, courtID, deposit, withdrawal, newStake) else Not staking phase Sortition->>Sortition: Record delayed stake Sortition-->>Core: Return Delayed status end sequenceDiagram participant Core as KlerosCoreBase participant Sortition as SortitionModuleBase participant Token as PNKToken participant Juror Core->>Sortition: penalizeStake(account, penaltyAmount) Sortition->>Token: Reduce staked balance Sortition-->>Core: Return (newBalance, actualPenalty) Core->>Juror: Emit Penalty event with actualPenalty alt Juror fully unstaked, no locked tokens Juror->>Sortition: withdrawLeftoverPNK(account) Sortition->>Token: Transfer leftover PNK to Juror Sortition->>Juror: Emit LeftoverPNKWithdrawn event end Possibly related PRs
Suggested labels
Suggested reviewers
Poem
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 ( |
![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.
✅ 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 failed. Why did it fail? →
|
jaybuidl added Type: Bug 🐛 Compatibility: ABI change 🗯 There was a problem hiding this comment.
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 forsetStakestill contains an unnamedboolparameter (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.jsonLength 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.jsonLength 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 3Length of output: 6473
Remove Invalid Unnamed Boolean from
setStakeABI
The ABI forsetStakecurrently includes a fourth, unnamedboolparameter that isn’t part of the contract’s external signature (address,uint96,uint256only). 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 theLeftoverPNKWithdrawn(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
AddswithdrawLeftoverPNK(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
📒 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
StakeDelayedevent variants into a single unified event handler aligns well with the contract changes that removed the deprecated_alreadyTransferredparameter. 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
StakeDelayedevent 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
handleStakeDelayedfunction 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
StakeDelayedAlreadyTransferredDepositedtoLeftoverPNKWithdrawnmakes 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
NewPhaseevent now only includes the essential_phaseparameter as auint8enum, 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
StakeDelayedevent with indexedaddressanduint96court ID parameters, along with theuint256amount, provides a clean interface that eliminates the need for multiple event variants.
742-753: LGTM! Enhanced penalizeStake function provides better accountability.The updated
penalizeStakefunction now returns bothpnkBalanceandavailablePenaltyvalues, 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
setStakeinclude:
- Unnamed boolean parameter (formerly
_alreadyTransferred) indicating deprecation- Additional
oldStakereturn value for better state trackingThese 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
withdrawLeftoverPNKfunction 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 singleNewPhase(uint8 _phase)event. Verify that your subgraph mappings now listen toNewPhaseinstead of the old events.
114-118: Updated StakeDelayed event
All delayed‐stake events have been merged intoStakeDelayed(address _address, uint96 _courtID, uint256 _amount). Ensure your subgraph only handles this consolidated event going forward.
679-691: penalizeStake now returns actual balances
ThepenalizeStakefunction signature was changed to return(uint256 pnkBalance, uint256 availablePenalty). If your subgraph makes contract calls to this function, update the ABICall definitions accordingly.
unknownunknown1 force-pushed the fix/staking-fix branch from d128f10 to f7591d2 Compare There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 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 (12)
- 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-university
- GitHub Check: Redirect rules - kleros-v2-neo
- GitHub Check: Header rules - kleros-v2-university
- GitHub Check: Header rules - kleros-v2-neo
- GitHub Check: Pages changed - kleros-v2-neo
- GitHub Check: Pages changed - kleros-v2-university
- GitHub Check: SonarCloud
- GitHub Check: contracts-testing
- GitHub Check: Analyze (javascript)
🔇 Additional comments (18)
subgraph/core-neo/subgraph.yaml (1)
161-162: Event handler consolidation looks good.The consolidation of multiple delayed stake event handlers into a single
StakeDelayedevent handler simplifies the subgraph configuration and aligns with the contract changes. This improves maintainability and reduces complexity.subgraph/core/subgraph.yaml (1)
162-163: Consistent event handler consolidation across networks.The changes maintain consistency with the core-neo configuration, properly consolidating the delayed stake event handlers. This ensures uniform behavior across different network deployments.
subgraph/core/src/SortitionModule.ts (2)
1-1: Import consolidation is correctly implemented.The import statement has been properly updated to reflect the consolidated event structure, removing the multiple specific delayed stake events in favor of the unified
StakeDelayedevent.
7-9: Event handler consolidation maintains correct logic.The unified
handleStakeDelayedfunction properly callsupdateJurorDelayedStakewith the correct parameters (address, courtID, amount), maintaining the same functionality as the previous separate handlers.subgraph/core/abi-migrations/SortitionModule.json (8)
4-6: Constructor definition updated correctly.The constructor has been properly updated to a standard nonpayable constructor with no inputs, removing the previous fallback/receive function structure.
69-69: Event parameter naming standardized.The parameter naming has been standardized from
_addressto_accountin theLeftoverPNKWithdrawnevent, improving consistency across the ABI.Also applies to: 79-79
87-93: NewPhase event structure improved.The
NewPhaseevent has been updated with proper enum typing (enum ISortitionModule.Phase) and parameter naming (_phase), which provides better type safety and clarity.
117-117: StakeDelayed event consolidation aligns with contract changes.The unified
StakeDelayedevent with parameters (address, uint96, uint256) correctly replaces the multiple variant delayed stake events, simplifying the event structure while maintaining necessary information.
680-691: penalizeStake function returns enhanced penalty information.The function now returns two values (
pnkBalanceandavailablePenalty) instead of none, providing better transparency into penalty execution and available penalty amounts for reward distribution.
823-823: setStake parameter handling updated.The deprecated
_alreadyTransferredparameter has been removed (parameter name is now empty), aligning with the PR objective to remove instant staking and simplify the staking logic.
839-843: setStake returns additional stake information.The function now returns an additional
oldStakevalue, providing better visibility into stake changes for external integrations and event processing.
954-961: withdrawLeftoverPNK function properly defined.The function has been correctly updated from a constructor-type to a regular function with simplified parameters, enabling jurors to withdraw leftover PNK tokens as described in the PR objectives.
subgraph/core-neo/abi-migrations/SortitionModuleNeo.json (6)
67-80: New LeftoverPNKWithdrawn event looks correct
Introducing theLeftoverPNKWithdrawnevent with an indexed_accountparameter allows efficient filtering. Ensure your subgraph mappings handle this new event.Please verify that the subgraph’s event handlers have been updated to consume
LeftoverPNKWithdrawn.
85-93: UpdatedNewPhaseevent signature is in place
The previous variant with an indexed address is removed, leaving only the_phasepayload. Confirm that downstream consumers (subgraph, frontends) are updated for this change.Run a search in your subgraph mappings for
NewPhaseto ensure the filtering logic aligns with this new signature.
109-118: ConsolidatedStakeDelayedevent is correct
Combining the three prior delayed-stake events into this single, indexed version simplifies processing. Confirm that your subgraph and any listeners now reference only thisStakeDelayedevent.Ensure all old event handlers are removed and that
StakeDelayedis fully integrated.
739-753:penalizeStakenow returns two values
The signature update to emit bothpnkBalanceandavailablePenaltyaligns with the new core logic. Verify that any ABI consumers (e.g., subgraph or UI) handle both return values in the correct order.You may grep for
.penalizeStakecalls in your mappings to confirm compatibility.
901-905:oldStakeoutput correctly added tosetStake
CapturingoldStakeenables downstream logic to reconcile previous state. The new ordering aligns with the updatedStakingResultenum.
1024-1037: AddedwithdrawLeftoverPNKfunction
This new method enables jurors to reclaim unused tokens. Ensure that your subgraph ABI and mappings include this function, and that theLeftoverPNKWithdrawnevent is emitted accordingly in the contract.Double-check your graph schema to include
withdrawLeftoverPNKand that calls to it are indexed.
| Code Climate has analyzed commit 3faccb6 and detected 0 issues on this pull request. View more on Code Climate. |
|
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (2)
contracts/src/arbitration/university/KlerosCoreUniversity.sol (1)
740-776:⚠️ Potential issuePenalty accounting bug – returned values ignored.
penalizeStakenow yields(pnkBalance, availablePenalty), but the code:_params.pnkPenaltiesInRound += penalty; ... penalizeStake(account, dispute.courtID, penalty);never uses
availablePenalty. If the juror’s stake is insufficient, you will over-credit penalties, breaking reward maths.- uint256 penalty = ...; - _params.pnkPenaltiesInRound += penalty; - ... - sortitionModule.penalizeStake(account, dispute.courtID, penalty); + uint256 penalty = ...; + (uint256 pnkBalance, uint256 applied) = + sortitionModule.penalizeStake(account, dispute.courtID, penalty); + _params.pnkPenaltiesInRound += applied; + ... + if (pnkBalance == 0) { + sortitionModule.setJurorInactive(account); + }This aligns behaviour with the Neo/Core version.
contracts/src/arbitration/university/SortitionModuleUniversity.sol (1)
247-258:⚠️ Potential issueMissing return statement – contract will not compile.
penalizeStakepromises(uint256 pnkBalance, uint256 availablePenalty)but neverreturns.- ) external override onlyByCore returns (uint256 pnkBalance, uint256 availablePenalty) { + ) external override onlyByCore returns (uint256 pnkBalance, uint256 availablePenalty) { Juror storage juror = jurors[_account]; if (juror.stakedPnk >= _relativeAmount) { juror.stakedPnk -= _relativeAmount; - } else { + availablePenalty = _relativeAmount; + } else { juror.stakedPnk = 0; + availablePenalty = juror.stakedPnk; // whatever could actually be taken } - } + pnkBalance = juror.stakedPnk; + return (pnkBalance, availablePenalty); + }Without this, compilation fails and upstream contracts depending on the returned values break.
🧹 Nitpick comments (11)
contracts/src/arbitration/SortitionModuleNeo.sol (1)
87-120: Minor gas-save opportunity in_validateStake.The arithmetic inside
if (phase == Phase.staking)is overflow-checked twice by default.
Wrapping thetotalStakedupdates in anunchecked { ... }block (after confirming bounds with the earlierifchecks) will shave a few hundred gas per stake adjustment without changing behaviour.contracts/src/arbitration/university/SortitionModuleUniversity.sol (2)
142-181:validateStakealways reports Success – consider max-stake caps.Unlike the Neo module, no upper-bound checks are applied. If instructional simplicity is intended that’s fine; otherwise replicate
maxStakePerJuror/maxTotalStakedvalidation to keep examples consistent.
183-185:setStakeis still TODO.Core now calls this method after deposits succeed. Implement the function (even a thin wrapper around tree updates) to avoid silent no-ops once the classroom moves past validation.
contracts/src/arbitration/SortitionModuleBase.sol (8)
38-43:alreadyTransferredfield is dead-code – drop it to slim the structThe flag is now documented as DEPRECATED and is never read anywhere in the contract after this PR.
Keeping it around:
- Bloats storage layout and therefore deployment/runtime gas.
- Confuses auditors who must still reason about its invariants.
If backward-compatibility is not strictly required at the storage level, delete the field altogether; otherwise at least add an inline comment that the slot is intentionally preserved for storage-layout compatibility.
48-50: Nice addition, but document the per-court locking invariantIntroducing
lockedInCourtclarifies where tokens are frozen.
Please extend NatSpec onJuror.lockedPnk/lockedInCourtto state the intended invariant (e.g.sum(lockedInCourt) == lockedPnk) so future maintainers cannot break it inadvertently.
72-73: Unused mapping – consider pruning
latestDelayedStakeIndexis marked DEPRECATED but is still instantiated.
Unless an external migration script still relies on it, removing it (and its writes in legacy code, if any) will save ~20 000 gas at deploy and 5 000 gas per cold SLOAD.
85-101: Legacy delayed-stake events still emitted nowhereAll three
StakeDelayed*legacy events remain declared but are never emitted after this PR.
Leaving them around:
- Creates an ABI surface that dApp/indexer devs might listen to by mistake.
- Adds 96 bytes to the runtime bytecode.
Recommend deleting the declarations once subgraphs have been migrated.
156-163: ModifieronlyByCoreOrThisenables external self-calls – be deliberateUsing
this.setStake(...)(seepenalizeStake) incurs an external call, extra gas, and re-entrancy surface (albeit to self).
An internal helper (_applyStake) would avoid this.-function setStake(address _account, uint96 _courtID, uint256 _newStake) public ... +function _applyStake(address _account, uint96 _courtID, uint256 _newStake) internal ...Then expose a thin external wrapper guarded by the modifier if needed.
348-388: Micro-optimisations & typo insidesetStake
currenCourtIDis misspelled – minor, but typo leaks into public bytecode.- The for-loop used for array cleanup is O(n). Keeping a mapping
courtID => indexwould make deletions O(1) and save gas for jurors with many courts.Not blocking, but worth tracking.
403-414:penalizeStakeperforms an external self-callCalling
this.setStakecosts an extra 700–1 000 gas and bypasses Solidity’s internal call optimisations.
Extract the core logic into an internal function and reuse it here.Also, since penalty burns stake while
lockedPnkstays unchanged untilunlockStake, make sure downstream code never assumesstakedPnk ≥ lockedPnkbetween these two calls (see earlier comment).
377-377: Nit: variable name typo
uint96 currenCourtID→uint96 currentCourtID
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
contracts/src/arbitration/KlerosCoreBase.sol(7 hunks)contracts/src/arbitration/KlerosCoreNeo.sol(1 hunks)contracts/src/arbitration/SortitionModuleBase.sol(8 hunks)contracts/src/arbitration/SortitionModuleNeo.sol(2 hunks)contracts/src/arbitration/interfaces/ISortitionModule.sol(1 hunks)contracts/src/arbitration/university/KlerosCoreUniversity.sol(7 hunks)contracts/src/arbitration/university/SortitionModuleUniversity.sol(5 hunks)contracts/test/foundry/KlerosCore.t.sol(11 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- contracts/src/arbitration/interfaces/ISortitionModule.sol
- contracts/test/foundry/KlerosCore.t.sol
⏰ Context from checks skipped due to timeout of 90000ms (15)
- GitHub Check: Redirect rules - kleros-v2-neo
- GitHub Check: Redirect rules - kleros-v2-university
- GitHub Check: Header rules - kleros-v2-neo
- GitHub Check: Header rules - kleros-v2-university
- GitHub Check: Pages changed - kleros-v2-neo
- GitHub Check: Pages changed - kleros-v2-university
- GitHub Check: Redirect rules - kleros-v2-testnet
- GitHub Check: Redirect rules - kleros-v2-testnet
- GitHub Check: Analyze (javascript)
- GitHub Check: contracts-testing
- GitHub Check: SonarCloud
- 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
🔇 Additional comments (14)
contracts/src/arbitration/KlerosCoreNeo.sol (1)
108-108: _setStake call matches the new interface – looks good.contracts/src/arbitration/KlerosCoreBase.sol (5)
466-476: Interface alignment looks correct.The updated invocations of
_setStakenow supplyOnError.*instead of the removed_alreadyTransferredflag – consistent with the refactor. No further action required.
606-607: Court-scoped locking call adopted correctly.
lockStake(drawnAddress, dispute.courtID, …)propagates the court context expected by the new SortitionModule ABI.
775-798: Good use of real penalty returned bypenalizeStake, but double-check ordering.Unlocking the stake before calling
penalizeStakeis necessary, yet it briefly dropslockedInCourtbelow the eventual penalty size. Confirm thatpenalizeStakeitself never relies onlockedInCourtfor invariants to avoid under-penalisation.
845-846: Court-scoped unlock reflected here as well – looks fine.
1084-1108: Delayed-stake branch returns before token transfer – verify follow-up call.When
stakingResult == Delayed, the function exits without moving tokens or updating the SortitionModule. Ensure the follow-upsetStakeBySortitionModulecall executes in all cases so the deposit is eventually pulled exactly once.contracts/src/arbitration/university/KlerosCoreUniversity.sol (4)
459-460: Updated_setStakeinvocation aligns with new signature.
467-469:setStakeBySortitionModulecall updated – OK.
595-596: Court-aware locking adopted correctly.
824-828: Court-aware unlock reflected in reward path – looks OK.contracts/src/arbitration/university/SortitionModuleUniversity.sol (2)
34-35: Per-court locking map added – nice touch.
235-245: Court-scoped lock/unlock bookkeeping looks correct.contracts/src/arbitration/SortitionModuleBase.sol (2)
103-108: 👍 UnifiedStakeDelayedeventClear, single source of truth with both indexed topics – good choice.
115-119: Event defined but never fired in this contract
LeftoverPNKWithdrawnis declared yet noemitis present.
Confirm the emission happens in a derived contract; otherwise add it wherewithdrawLeftoverPNKis implemented.
jaybuidl force-pushed the fix/staking-fix branch from 3faccb6 to 994e8f8 Compare 
jaybuidl force-pushed the fix/staking-fix branch from 994e8f8 to f30d327 Compare There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (2)
subgraph/core/abi-migrations/SortitionModule.json (2)
817-825: Remove the orphan boolean input fromsetStake.
setStakeused to take_alreadyTransferred (bool)which has now been deprecated.
The ABI still contains a boolean input with an emptynamefield.
That produces an invalid ABI (empty identifier) and breaks all generated type bindings.- { - "internalType": "bool", - "name": "", - "type": "bool" - }Delete the entire object above (including the trailing comma of the previous argument) to make the signature compliant.
173-200: DuplicateStakeSetevent declarations – Solidity will not allow this.There are two
StakeSetevents back-to-back with different payloads (one includes_amountAllCourts, the other does not).
Event names cannot be overloaded, so one of these must be renamed or removed; otherwise contract compilation & subgraph code-gen will fail.Action: keep the new/desired shape and delete the outdated one, or give them distinct names (
StakeSetvsStakeSetInCourt, etc.).
🧹 Nitpick comments (1)
subgraph/core/abi-migrations/SortitionModule.json (1)
64-80: Event rename looks good – remember to update subgraph handlers.
LeftoverPNKWithdrawn(_account, _amount)replaces the old _AlreadyTransferred variants.
No issues with the ABI itself; just ensure the new handler (handleLeftoverPNKWithdrawn) is wired inschema.graphqlandmapping.ts.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
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)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 (2)
- contracts/src/arbitration/SortitionModuleNeo.sol
- contracts/src/arbitration/university/SortitionModuleUniversity.sol
🚧 Files skipped from review as they are similar to previous changes (11)
- subgraph/temp-older-events-addition.txt
- subgraph/core-neo/subgraph.yaml
- subgraph/core/subgraph.yaml
- contracts/src/libraries/Constants.sol
- subgraph/core/src/SortitionModule.ts
- contracts/src/arbitration/dispute-kits/DisputeKitClassicBase.sol
- contracts/src/arbitration/interfaces/ISortitionModule.sol
- contracts/src/arbitration/KlerosCoreBase.sol
- contracts/test/foundry/KlerosCore.t.sol
- subgraph/core-neo/abi-migrations/SortitionModuleNeo.json
- contracts/src/arbitration/SortitionModuleBase.sol
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: jaybuidl PR: kleros/kleros-v2#1746 File: contracts/config/courts.v2.mainnet-neo.json:167-170 Timestamp: 2024-11-19T16:31:08.965Z Learning: In `contracts/config/courts.v2.mainnet-neo.json`, the `minStake` parameter is denominated in PNK, not ETH. Learnt from: Harman-singh-waraich PR: kleros/kleros-v2#1775 File: web/src/pages/Courts/CourtDetails/StakePanel/StakeWithdrawButton.tsx:0-0 Timestamp: 2024-12-09T12:36:59.441Z Learning: In the `StakeWithdrawButton` component, the transaction flow logic is tightly linked to component updates, so extracting it into a custom hook does not provide significant benefits. subgraph/core/abi-migrations/SortitionModule.json (4)
Learnt from: Harman-singh-waraich PR: kleros/kleros-v2#1703 File: kleros-sdk/src/dataMappings/utils/actionTypes.ts:22-22 Timestamp: 2024-10-22T09:34:54.761Z Learning: In `kleros-sdk/src/dataMappings/utils/actionTypes.ts`, the `abi` field in `AbiCallMapping` is intentionally typed as `string` because it is parsed later in the action functions. Learnt from: Harman-singh-waraich PR: kleros/kleros-v2#1703 File: kleros-sdk/src/dataMappings/utils/actionTypes.ts:29-29 Timestamp: 2024-10-22T09:36:10.478Z Learning: In `kleros-sdk/src/dataMappings/utils/actionTypes.ts`, the `abi` field in `AbiEventMapping` is intentionally typed as `string` because it is parsed later in the action functions. Learnt from: Harman-singh-waraich PR: kleros/kleros-v2#1703 File: kleros-sdk/src/dataMappings/utils/actionTypes.ts:25-25 Timestamp: 2024-10-22T09:35:14.098Z Learning: In `kleros-sdk/src/dataMappings/utils/actionTypes.ts`, for the `AbiCallMapping` type, it's acceptable to use `args: any[]` since `args` can be anything. Learnt from: jaybuidl PR: kleros/kleros-v2#1746 File: contracts/config/courts.v2.mainnet-neo.json:167-170 Timestamp: 2024-11-19T16:31:08.965Z Learning: In `contracts/config/courts.v2.mainnet-neo.json`, the `minStake` parameter is denominated in PNK, not ETH. ⏰ 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: contracts-testing
- GitHub Check: Analyze (javascript)
🔇 Additional comments (1)
subgraph/core/abi-migrations/SortitionModule.json (1)
679-691: Confirm downstream handling of newpenalizeStakereturn tuple.
penalizeStakenow returns(pnkBalance, availablePenalty).
Subgraph mappings, SDK wrappers and unit tests that decode the old single return value will revert or silently mis-decode.Request: verify all callers and update generated typings.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
contracts/src/arbitration/SortitionModuleBase.sol (1)
71-71: DEPRECATED mapping creates storage bloatThe
latestDelayedStakeIndexmapping is deprecated but still occupies storage. Consider adding a cleanup mechanism or documenting the migration strategy.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
contracts/src/arbitration/KlerosCoreBase.sol(5 hunks)contracts/src/arbitration/KlerosCoreNeo.sol(1 hunks)contracts/src/arbitration/SortitionModuleBase.sol(8 hunks)contracts/src/arbitration/SortitionModuleNeo.sol(2 hunks)contracts/src/arbitration/interfaces/ISortitionModule.sol(3 hunks)contracts/src/arbitration/university/KlerosCoreUniversity.sol(3 hunks)contracts/src/arbitration/university/SortitionModuleUniversity.sol(5 hunks)contracts/test/foundry/KlerosCore.t.sol(11 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- contracts/src/arbitration/SortitionModuleNeo.sol
- contracts/src/arbitration/university/SortitionModuleUniversity.sol
- contracts/src/arbitration/interfaces/ISortitionModule.sol
- contracts/src/arbitration/KlerosCoreBase.sol
🧰 Additional context used
🧠 Learnings (5)
📓 Common learnings
Learnt from: jaybuidl PR: kleros/kleros-v2#1746 File: contracts/config/courts.v2.mainnet-neo.json:167-170 Timestamp: 2024-11-19T16:31:08.965Z Learning: In `contracts/config/courts.v2.mainnet-neo.json`, the `minStake` parameter is denominated in PNK, not ETH. Learnt from: Harman-singh-waraich PR: kleros/kleros-v2#1775 File: web/src/pages/Courts/CourtDetails/StakePanel/StakeWithdrawButton.tsx:0-0 Timestamp: 2024-12-09T12:36:59.441Z Learning: In the `StakeWithdrawButton` component, the transaction flow logic is tightly linked to component updates, so extracting it into a custom hook does not provide significant benefits. contracts/src/arbitration/university/KlerosCoreUniversity.sol (2)
Learnt from: jaybuidl PR: kleros/kleros-v2#1746 File: contracts/config/courts.v2.mainnet-neo.json:167-170 Timestamp: 2024-11-19T16:31:08.965Z Learning: In `contracts/config/courts.v2.mainnet-neo.json`, the `minStake` parameter is denominated in PNK, not ETH. Learnt from: jaybuidl PR: kleros/kleros-v2#1746 File: contracts/config/courts.v2.mainnet-neo.json:167-170 Timestamp: 2024-11-19T16:31:08.965Z Learning: In the court hierarchy, child courts' `minStake` must be greater than or equal to their parent court's `minStake`. contracts/src/arbitration/KlerosCoreNeo.sol (2)
Learnt from: jaybuidl PR: kleros/kleros-v2#1746 File: contracts/config/courts.v2.mainnet-neo.json:167-170 Timestamp: 2024-11-19T16:31:08.965Z Learning: In `contracts/config/courts.v2.mainnet-neo.json`, the `minStake` parameter is denominated in PNK, not ETH. Learnt from: jaybuidl PR: kleros/kleros-v2#1746 File: contracts/config/courts.v2.mainnet-neo.json:167-170 Timestamp: 2024-11-19T16:31:08.965Z Learning: In the court hierarchy, child courts' `minStake` must be greater than or equal to their parent court's `minStake`. contracts/test/foundry/KlerosCore.t.sol (3)
undefined
<retrieved_learning>
Learnt from: jaybuidl
PR: #1746
File: contracts/config/courts.v2.mainnet-neo.json:167-170
Timestamp: 2024-11-19T16:31:08.965Z
Learning: In the court hierarchy, child courts' minStake must be greater than or equal to their parent court's minStake.
</retrieved_learning>
<retrieved_learning>
Learnt from: jaybuidl
PR: #1746
File: contracts/config/courts.v2.mainnet-neo.json:167-170
Timestamp: 2024-11-19T16:31:08.965Z
Learning: In contracts/config/courts.v2.mainnet-neo.json, the minStake parameter is denominated in PNK, not ETH.
</retrieved_learning>
<retrieved_learning>
Learnt from: jaybuidl
PR: #1746
File: contracts/config/courts.v2.mainnet-neo.json:3-5
Timestamp: 2024-11-19T17:18:39.007Z
Learning: In contracts/config/courts.v2.mainnet-neo.json, the General Court (id: 1) intentionally references itself as its parent ("parent": 1). This self-reference is acceptable and should not be flagged as an issue in future reviews.
</retrieved_learning>
contracts/src/arbitration/SortitionModuleBase.sol (6)
Learnt from: jaybuidl PR: kleros/kleros-v2#1746 File: contracts/config/courts.v2.mainnet-neo.json:3-5 Timestamp: 2024-11-19T17:18:39.007Z Learning: In `contracts/config/courts.v2.mainnet-neo.json`, the General Court (id: 1) intentionally references itself as its parent (`"parent": 1`). This self-reference is acceptable and should not be flagged as an issue in future reviews. Learnt from: Harman-singh-waraich PR: kleros/kleros-v2#1739 File: web/src/pages/Home/TopJurors/JurorCard/Coherency.tsx:22-26 Timestamp: 2024-11-07T10:48:16.774Z Learning: In the `Coherency` component (`web/src/pages/Home/TopJurors/JurorCard/Coherency.tsx`), `totalResolvedVotes` is always greater than or equal to `totalCoherentVotes`. When both are zero, `0/0` results in `NaN`, which is acceptable in this context. Learnt from: jaybuidl PR: kleros/kleros-v2#1746 File: contracts/config/courts.v2.mainnet-neo.json:167-170 Timestamp: 2024-11-19T16:31:08.965Z Learning: In the court hierarchy, child courts' `minStake` must be greater than or equal to their parent court's `minStake`. Learnt from: Harman-singh-waraich PR: kleros/kleros-v2#1775 File: web/src/pages/Courts/CourtDetails/StakePanel/StakeWithdrawButton.tsx:0-0 Timestamp: 2024-12-09T12:36:59.441Z Learning: In the `StakeWithdrawButton` component, the transaction flow logic is tightly linked to component updates, so extracting it into a custom hook does not provide significant benefits. Learnt from: jaybuidl PR: kleros/kleros-v2#1746 File: contracts/config/courts.v2.mainnet-neo.json:167-170 Timestamp: 2024-11-19T16:31:08.965Z Learning: In `contracts/config/courts.v2.mainnet-neo.json`, the `minStake` parameter is denominated in PNK, not ETH. Learnt from: jaybuidl PR: kleros/kleros-v2#1746 File: contracts/config/courts.v2.mainnet-neo.json:3-17 Timestamp: 2024-11-19T16:09:41.467Z Learning: In `contracts/config/courts.v2.mainnet-neo.json`, the General Court (ID: 1) can have its `parent` ID set to itself (`"parent": 1`), as there is no parent court with ID 0 currently. ⏰ 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: contracts-testing
- GitHub Check: Analyze (javascript)
🔇 Additional comments (30)
contracts/src/arbitration/KlerosCoreNeo.sol (1)
108-108: Confirmed: Base_setStakesignature updated to four parametersI searched all
_setStakedefinitions and found the internal signature inKlerosCoreBase.solhas been refactored to:•
function _setStake(address _account, uint96 _courtID, uint256 _newStake, OnError _onError) internal returns (bool)This matches the call in
KlerosCoreNeo.sol:- super._setStake(msg.sender, _courtID, _newStake, _alreadyTransferred, OnError.Revert); + super._setStake(msg.sender, _courtID, _newStake, OnError.Revert);No discrepancies remain. The removal of
_alreadyTransferredis consistent across all implementations.contracts/src/arbitration/university/KlerosCoreUniversity.sol (3)
459-459: LGTM! Consistent parameter removal in setStake.The removal of the
_alreadyTransferredparameter from the_setStakecall is consistent with the broader staking refactor.
1041-1041: LGTM! Internal _setStake function signature updated.The internal
_setStakefunction signature has been properly updated to remove the_alreadyTransferredparameter.
466-468: Breaking change:setStakeBySortitionModulesignature updated
The_alreadyTransferredparameter has been removed to simplify staking logic. I’ve verified that all callers have been updated to use the new three-argument signature:
- contracts/src/arbitration/SortitionModuleBase.sol (lines 244, 410)
- contracts/src/arbitration/university/SortitionModuleUniversity.sol (line 256)
- contracts/test/foundry/KlerosCore.t.sol (line 1243)
No remaining four-argument calls were found.
contracts/test/foundry/KlerosCore.t.sol (15)
1013-1013: LGTM: Updated event emission aligns with consolidated delayed stake handling.The event emission has been correctly updated to use the new unified
StakeDelayedevent instead of the previous multiple delayed stake events.
1025-1025: LGTM: Correct assertion update for removedalreadyTransferredparameter.The assertion correctly expects
falsefor thealreadyTransferredflag, which aligns with the removal of the deprecated parameter mentioned in the PR objectives.
1029-1029: LGTM: Token balance assertions correctly reflect delayed stake behavior.The assertions correctly verify that during the drawing phase, token balances remain unchanged as tokens are only transferred upon execution in the staking phase, not immediately during delayed stake creation.
Also applies to: 1039-1040
1059-1059: LGTM: Consistent event emission for decreased stake in drawing phase.The event emission correctly uses the unified
StakeDelayedevent for stake decreases during the drawing phase.
1094-1119: LGTM: Well-documented test logic for locked tokens behavior.The comments and assertions correctly demonstrate that locked tokens remain after unstaking and that subsequent staking properly accounts for previously locked tokens. The balance calculations are accurate.
1142-1189: LGTM: Comprehensive test coverage for delayed stake execution.The test correctly verifies:
- Multiple delayed stakes for different stakers
- Token balances remain unchanged during the drawing phase
- Proper event emissions for each delayed stake
- Correct assertion that
alreadyTransferredisfalsefor all delayed stakes
1193-1198: LGTM: Accurate balance verification before delayed stake execution.The assertions correctly verify that balances remain unchanged until delayed stakes are executed, with appropriate comments explaining the expected behavior.
1205-1209: LGTM: Correct setup for delayed stake execution testing.The test properly sets up the scenario where tokens are disposed to test execution failure, and correctly expects the consolidated
StakeSetevent.
1243-1243: LGTM: Consistent function call with new parameter signature.The function call correctly uses the updated signature for
setStakeBySortitionModulewithout the deprecatedalreadyTransferredparameter.
1430-1430: LGTM: Updated comment reflects removal of stake sufficiency checks.The comment correctly explains that the juror can be drawn multiple times despite low stake, which aligns with the removal of stake sufficiency checks mentioned in the AI summary.
1437-1437: LGTM: Accurate locked token calculation.The assertion correctly verifies that 3000 tokens are locked (1000 per draw for 3 draws), with an appropriate comment explaining the calculation.
2415-2416: LGTM: Correct token balance assertions for multi-court staking.The assertions accurately reflect the token balances when staking in multiple courts (40,000 total staked in two courts).
2440-2448: LGTM: Well-documented penalty execution logic.The comments and event expectations correctly explain the penalty execution process, including the step-by-step calculation of remaining stakes after penalties.
2455-2511: LGTM: Comprehensive test for insolvent juror handling.This new test function properly verifies:
- Insolvent juror behavior when locked tokens exceed staked amount
- Correct penalty application and reward distribution
- Automatic unstaking from all courts when juror becomes insolvent
- Accurate token balance calculations throughout the process
The test thoroughly covers the edge case of juror insolvency.
2513-2588: LGTM: Excellent test coverage for leftover PNK withdrawal functionality.This new test function comprehensively verifies:
- The
withdrawLeftoverPNKfunction behavior- Proper access control (SortitionModuleOnly)
- Correct timing requirements (withdrawal only after execution)
- Accurate token balance management
- Integration with the core's
transferBySortitionModulefunctionThe test properly validates the new functionality mentioned in the PR objectives.
contracts/src/arbitration/SortitionModuleBase.sol (11)
42-42: DEPRECATED field still present in structThe
alreadyTransferredfield is marked as deprecated but remains in the struct. Consider the storage layout implications and migration path for existing deployments.
48-48: New field added to track locked tokens per jurorThe addition of
lockedPnkfield is essential for the improved withdrawal logic. This field will track locked tokens globally across all courts for each juror.
84-100: Deprecated events maintained for backward compatibilityThe deprecated events are properly marked and retained for compatibility. The consolidated approach with the new
StakeDelayedevent is cleaner.
102-106: Consolidated delayed stake event simplifies trackingThe new
StakeDelayedevent consolidates the previous multiple events into a single, cleaner interface. This improves maintainability and reduces complexity.
114-117: New event for leftover PNK withdrawalsThe
LeftoverPNKWithdrawnevent properly tracks when jurors withdraw leftover tokens after full unstaking.
277-279: Function signature updated to remove deprecated parameterThe removal of
_alreadyTransferredparameter is a breaking change but aligns with the PR objectives to simplify the staking logic.
283-323: Improved _setStake function with better separation of concernsThe refactored
_setStakefunction properly separates delayed stake handling from immediate stake changes. The withdrawal capping logic correctly ensures locked tokens remain in the contract.Key improvements:
- Delayed stakes now emit a single event and return
StakingResult.Delayed- Withdrawal capping uses global locked tokens (
juror.lockedPnk) to prevent under-collateralization- No state changes during delayed stake creation, preventing invariant violations
325-369: New updateState function handles state synchronizationThe
updateStatefunction properly handles juror balance updates and courtID management. The cleanup logic for removing courtIDs when stakes reach zero is correct.
381-398: Enhanced penalizeStake function with improved return valuesThe function now returns both the updated balance and actual penalty applied, providing better information for the calling contract. The logic correctly handles cases where the penalty exceeds the staked amount.
407-412: Updated setJurorInactive to use new signatureThe function correctly calls
core.setStakeBySortitionModulewithout the deprecated parameter.
414-432: New withdrawLeftoverPNK function addresses edge caseThe function properly handles the scenario where jurors have leftover PNK after fully unstaking. The conditions are well-defined:
- Must be fully unstaked (no courtIDs)
- No locked tokens
- Positive staked balance
The function safely transfers tokens via the core contract and emits the appropriate event.
…te() into SM.setStake() fixed natspec
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
contracts/src/arbitration/university/SortitionModuleUniversity.sol (1)
276-294: Consider more specific error messages for withdrawal eligibility.The function correctly implements the withdrawal logic, but the generic error message doesn't indicate which specific condition failed.
- 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); - } else { - revert("Not eligible for withdrawal."); - } + if (juror.stakedPnk == 0) { + revert("No PNK balance to withdraw."); + } + if (juror.courtIDs.length != 0) { + revert("Must unstake from all courts first."); + } + if (juror.lockedPnk != 0) { + revert("Cannot withdraw while tokens are locked."); + } + + uint256 amount = juror.stakedPnk; + juror.stakedPnk = 0; + core.transferBySortitionModule(_account, amount); + emit LeftoverPNKWithdrawn(_account, amount);contracts/src/arbitration/SortitionModuleBase.sol (2)
42-42: Consider future cleanup of deprecated fields.While maintaining deprecated fields ensures backward compatibility, consider planning their removal in a future major version to reduce storage costs and complexity. The consolidation into a single
StakeDelayedevent is a good simplification.Also applies to: 71-71, 84-106
428-446: Consider more specific error messages for withdrawal eligibility.The function logic is correct, but more specific error messages would help users understand why their withdrawal failed.
- 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); - } else { - revert("Not eligible for withdrawal."); - } + if (juror.stakedPnk == 0) { + revert("No PNK balance to withdraw."); + } + if (juror.courtIDs.length != 0) { + revert("Must unstake from all courts first."); + } + if (juror.lockedPnk != 0) { + revert("Cannot withdraw while tokens are locked."); + } + + uint256 amount = juror.stakedPnk; + juror.stakedPnk = 0; + core.transferBySortitionModule(_account, amount); + emit LeftoverPNKWithdrawn(_account, amount);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
contracts/src/arbitration/SortitionModuleBase.sol(8 hunks)contracts/src/arbitration/university/KlerosCoreUniversity.sol(11 hunks)contracts/src/arbitration/university/SortitionModuleUniversity.sol(4 hunks)
🧰 Additional context used
🧠 Learnings (4)
📓 Common learnings
Learnt from: jaybuidl PR: kleros/kleros-v2#1746 File: contracts/config/courts.v2.mainnet-neo.json:167-170 Timestamp: 2024-11-19T16:31:08.965Z Learning: In `contracts/config/courts.v2.mainnet-neo.json`, the `minStake` parameter is denominated in PNK, not ETH. Learnt from: Harman-singh-waraich PR: kleros/kleros-v2#1775 File: web/src/pages/Courts/CourtDetails/StakePanel/StakeWithdrawButton.tsx:0-0 Timestamp: 2024-12-09T12:36:59.441Z Learning: In the `StakeWithdrawButton` component, the transaction flow logic is tightly linked to component updates, so extracting it into a custom hook does not provide significant benefits. contracts/src/arbitration/university/SortitionModuleUniversity.sol (2)
Learnt from: jaybuidl PR: kleros/kleros-v2#1746 File: contracts/config/courts.v2.mainnet-neo.json:167-170 Timestamp: 2024-11-19T16:31:08.965Z Learning: In `contracts/config/courts.v2.mainnet-neo.json`, the `minStake` parameter is denominated in PNK, not ETH. Learnt from: jaybuidl PR: kleros/kleros-v2#1746 File: contracts/config/courts.v2.mainnet-neo.json:167-170 Timestamp: 2024-11-19T16:31:08.965Z Learning: In the court hierarchy, child courts' `minStake` must be greater than or equal to their parent court's `minStake`. contracts/src/arbitration/SortitionModuleBase.sol (6)
Learnt from: jaybuidl PR: kleros/kleros-v2#1746 File: contracts/config/courts.v2.mainnet-neo.json:3-5 Timestamp: 2024-11-19T17:18:39.007Z Learning: In `contracts/config/courts.v2.mainnet-neo.json`, the General Court (id: 1) intentionally references itself as its parent (`"parent": 1`). This self-reference is acceptable and should not be flagged as an issue in future reviews. Learnt from: Harman-singh-waraich PR: kleros/kleros-v2#1739 File: web/src/pages/Home/TopJurors/JurorCard/Coherency.tsx:22-26 Timestamp: 2024-11-07T10:48:16.774Z Learning: In the `Coherency` component (`web/src/pages/Home/TopJurors/JurorCard/Coherency.tsx`), `totalResolvedVotes` is always greater than or equal to `totalCoherentVotes`. When both are zero, `0/0` results in `NaN`, which is acceptable in this context. Learnt from: jaybuidl PR: kleros/kleros-v2#1746 File: contracts/config/courts.v2.mainnet-neo.json:167-170 Timestamp: 2024-11-19T16:31:08.965Z Learning: In the court hierarchy, child courts' `minStake` must be greater than or equal to their parent court's `minStake`. Learnt from: Harman-singh-waraich PR: kleros/kleros-v2#1775 File: web/src/pages/Courts/CourtDetails/StakePanel/StakeWithdrawButton.tsx:0-0 Timestamp: 2024-12-09T12:36:59.441Z Learning: In the `StakeWithdrawButton` component, the transaction flow logic is tightly linked to component updates, so extracting it into a custom hook does not provide significant benefits. Learnt from: jaybuidl PR: kleros/kleros-v2#1746 File: contracts/config/courts.v2.mainnet-neo.json:167-170 Timestamp: 2024-11-19T16:31:08.965Z Learning: In `contracts/config/courts.v2.mainnet-neo.json`, the `minStake` parameter is denominated in PNK, not ETH. Learnt from: jaybuidl PR: kleros/kleros-v2#1746 File: contracts/config/courts.v2.mainnet-neo.json:3-17 Timestamp: 2024-11-19T16:09:41.467Z Learning: In `contracts/config/courts.v2.mainnet-neo.json`, the General Court (ID: 1) can have its `parent` ID set to itself (`"parent": 1`), as there is no parent court with ID 0 currently. contracts/src/arbitration/university/KlerosCoreUniversity.sol (7)
Learnt from: jaybuidl PR: kleros/kleros-v2#1746 File: contracts/config/courts.v2.mainnet-neo.json:167-170 Timestamp: 2024-11-19T16:31:08.965Z Learning: In `contracts/config/courts.v2.mainnet-neo.json`, the `minStake` parameter is denominated in PNK, not ETH. Learnt from: jaybuidl PR: kleros/kleros-v2#1746 File: contracts/config/courts.v2.mainnet-neo.json:3-17 Timestamp: 2024-11-19T16:09:41.467Z Learning: In `contracts/config/courts.v2.mainnet-neo.json`, the General Court (ID: 1) can have its `parent` ID set to itself (`"parent": 1`), as there is no parent court with ID 0 currently. Learnt from: jaybuidl PR: kleros/kleros-v2#1746 File: contracts/config/courts.v2.mainnet-neo.json:3-5 Timestamp: 2024-11-19T17:18:39.007Z Learning: In `contracts/config/courts.v2.mainnet-neo.json`, the General Court (id: 1) intentionally references itself as its parent (`"parent": 1`). This self-reference is acceptable and should not be flagged as an issue in future reviews. Learnt from: jaybuidl PR: kleros/kleros-v2#1746 File: contracts/config/courts.v2.mainnet-neo.json:167-170 Timestamp: 2024-11-19T16:31:08.965Z Learning: In the court hierarchy, child courts' `minStake` must be greater than or equal to their parent court's `minStake`. Learnt from: jaybuidl PR: kleros/kleros-v2#1746 File: contracts/config/courts.v2.mainnet-neo.json:3-17 Timestamp: 2024-11-19T16:09:52.022Z Learning: In the Kleros court configuration, it's acceptable for the General Court (id: 1) to have itself as its parent (parent: 1) when there is no court at index 0. Learnt from: Harman-singh-waraich PR: kleros/kleros-v2#1703 File: web/src/hooks/queries/usePopulatedDisputeData.ts:58-61 Timestamp: 2024-10-14T13:58:25.708Z Learning: In `web/src/hooks/queries/usePopulatedDisputeData.ts`, the query and subsequent logic only execute when `disputeData.dispute?.arbitrableChainId` and `disputeData.dispute?.externalDisputeId` are defined, so `initialContext` properties based on these values are safe to use without additional null checks. Learnt from: Harman-singh-waraich PR: kleros/kleros-v2#1739 File: web/src/pages/Home/TopJurors/JurorCard/Coherency.tsx:22-26 Timestamp: 2024-11-07T10:48:16.774Z Learning: In the `Coherency` component (`web/src/pages/Home/TopJurors/JurorCard/Coherency.tsx`), `totalResolvedVotes` is always greater than or equal to `totalCoherentVotes`. When both are zero, `0/0` results in `NaN`, which is acceptable in this context. ⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (14)
- GitHub Check: Redirect rules - kleros-v2-testnet-devtools
- GitHub Check: Header rules - kleros-v2-testnet-devtools
- GitHub Check: Pages changed - kleros-v2-testnet-devtools
- GitHub Check: Redirect rules - kleros-v2-testnet
- GitHub Check: Redirect rules - kleros-v2-testnet
- GitHub Check: Header rules - kleros-v2-testnet
- GitHub Check: Header rules - kleros-v2-testnet
- GitHub Check: Pages changed - kleros-v2-testnet
- GitHub Check: Pages changed - kleros-v2-testnet
- GitHub Check: Redirect rules - kleros-v2-neo
- GitHub Check: Header rules - kleros-v2-neo
- GitHub Check: Pages changed - kleros-v2-neo
- GitHub Check: contracts-testing
- GitHub Check: Analyze (javascript)
🔇 Additional comments (17)
contracts/src/arbitration/university/SortitionModuleUniversity.sol (5)
63-67: Event properly structured for leftover PNK withdrawals.The event is well-designed with appropriate indexing and follows the established naming conventions.
134-176: Validation logic correctly implements stake constraints.The function properly validates stake changes as a view function, including:
- Prevention of staking in more than MAX_STAKE_PATHS courts
- Forbidding zero stakes when no current stake exists
- Ensuring locked tokens remain in the contract during withdrawals
The withdrawal cap calculation correctly ensures that
juror.stakedPnknever falls belowjuror.lockedPnk.
178-231: State update logic correctly manages juror stakes and court relationships.The function properly:
- Updates juror PNK balances based on deposits/withdrawals
- Manages the court IDs array (adding on first stake, removing on full unstake)
- Propagates stake changes up the court hierarchy
- Emits the appropriate event
243-260: Penalty logic correctly handles partial penalties.The function properly returns both the updated balance and the actual penalty applied, which is essential for accurate accounting when a juror's stake is insufficient to cover the full penalty.
272-272: Verified:setStakeBySortitionModuleSignature UpdatedThe
setStakeBySortitionModulefunction in the core contracts now has the signaturefunction setStakeBySortitionModule(address _account, uint96 _courtID, uint256 _newStake) external;which matches the call in
contracts/src/arbitration/university/SortitionModuleUniversity.solat line 272:- core.setStakeBySortitionModule(_account, courtIDs[j - 1], 0, /* bool removed */); + core.setStakeBySortitionModule(_account, courtIDs[j - 1], 0);No further changes are required.
contracts/src/arbitration/university/KlerosCoreUniversity.sol (8)
112-112: Type consistency improvement for court ID.Using
uint96for the court ID parameter improves type consistency with the storage variables and function parameters throughout the contract.
466-469: Access control and signature simplification properly implemented.The function correctly:
- Restricts access to the sortition module only
- Removes the deprecated
_alreadyTransferredparameter- Uses appropriate error handling mode for external calls
471-478: Token transfer function properly secured.The function correctly implements controlled token transfers with:
- Appropriate access control limiting calls to sortition module
- Safe transfer mechanism
- Clear documentation about eligibility checks
574-577: Appeal progression logic improved.The addition of the
isAppealFundedcheck allows the dispute to progress to execution as soon as the appeal is funded, improving efficiency without waiting for the full appeal period to expire.
777-791: Penalty accounting correctly uses actual penalties applied.The changes properly:
- Accumulate only the actual penalty applied (which may be less than requested)
- Emit events with the correct penalty amounts
- Set jurors inactive when their balance reaches zero
1076-1097: Stake setting correctly implements validation-execution pattern.The function properly:
- Validates stake changes before any state modifications
- Handles token transfers based on validation results
- Passes all required parameters to the sortition module's setStake function
979-1004: View functions provide comprehensive query capabilities.The new functions offer valuable insights into dispute and court states. The warning about potential gas limits for
getRoundInfois appropriately documented.
1110-1110: Error handling properly extended for new validation.The new error
StakingZeroWhenNoStakecorrectly corresponds to the validation logic preventing zero stakes when no current stake exists.Also applies to: 1185-1185
contracts/src/arbitration/SortitionModuleBase.sol (4)
242-246: Delayed stake execution properly simplified.The function correctly processes delayed stakes with the updated signature and properly cleans up processed entries.
305-317: Withdrawal cap correctly enforces global constraint.The calculation
juror.stakedPnk > juror.lockedPnk ? juror.stakedPnk - juror.lockedPnk : 0ensures that the total staked PNK never falls below the total locked PNK across all courts, maintaining the critical invariant.
330-383: State update logic properly implements stake management.The function correctly:
- Updates juror PNK balances based on validated amounts
- Manages court ID arrays (adding/removing as needed)
- Updates sortition sum trees for the court hierarchy
- Emits appropriate events
395-412: Penalty handling correctly accounts for partial penalties.The function properly returns both the updated balance and actual penalty applied, ensuring accurate accounting when stakes are insufficient to cover full penalties.
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (5)
subgraph/core/abi-migrations/KlerosCore.json (1)
226-242: DuplicateCourtCreatedevent definition – prune to avoid ABI code-gen clashesHaving the same event twice in the ABI confuses generators such as TypeChain and Ethers v6, producing duplicate symbol errors.
- { - "anonymous": false, - "inputs": [ … ], - "name": "CourtCreated", - "type": "event" - }, + /* duplicate removed */Delete one occurrence to keep the ABI canonical.
Also applies to: 344-403
subgraph/core/abi-migrations/SortitionModule.json (1)
164-219: DuplicateStakeSetevent – this will break every code-gen / log-decoder tool
TwoStakeSetevents with different parameter lists are declared back-to-back.
Most ABI consumers (ethers.js, web3.js, TypeChain, The Graph, etc.) collapse events by name; the second declaration will either overwrite the first or throw at generation time.Suggested fix – keep one canonical definition (the longer variant seems correct) and drop the duplicate:
@@ - { - "anonymous": false, - "inputs": [ - { - "indexed": true, - "internalType": "address", - "name": "_address", - "type": "address" - }, - { - "indexed": false, - "internalType": "uint256", - "name": "_courtID", - "type": "uint256" - }, - { - "indexed": false, - "internalType": "uint256", - "name": "_amount", - "type": "uint256" - } - ], - "name": "StakeSet", - "type": "event" - },subgraph/core-neo/abi-migrations/KlerosCoreNeo.json (1)
302-423: DuplicateCourtCreatedevents – pick one and drop the other
Two ABI entries with the exact same event name appear back-to-back. Duplicated entries break code-generation tools (TypeChain, ethers‐v6 codegen, graph codegen) and may confuse off-chain indexers.- { - "anonymous": false, - ... - "name": "CourtCreated", - "type": "event" - }, - { - "anonymous": false, - ... - "name": "CourtCreated", - "type": "event" - }, + // keep only one definition of CourtCreatedsubgraph/core-neo/abi-migrations/SortitionModuleNeo.json (2)
165-219: TwoStakeSetdefinitions with different payloads – causes ABI ambiguity
The ABI listsStakeSettwice: one variant includes_amountAllCourts, the other omits it. Tools will keep the last definition only, silently discarding the first and breaking any listener expecting the 4-parameter version.Choose a single canonical signature or rename the second event.
- "name": "StakeSet", - ... - "name": "StakeSet", + // keep a single, correctly-typed StakeSet event
417-420:alreadyTransferredfield wasn’t removed fromdelayedStakesoutput
PR summary states this boolean is deprecated. Its presence here will keep subgraph & contract wrappers expecting a value that core contracts no longer populate, leading to runtime decoding failures.- { - "internalType": "bool", - "name": "alreadyTransferred", - "type": "bool" - } + // field removed – struct now has 3 components
🧹 Nitpick comments (2)
subgraph/core/abi-migrations/KlerosCore.json (1)
1530-1553: New viewgetPnkAtStakePerJuror– remember to index / expose itNice addition. If the subgraph or SDK needs this value, add it to the entity schema and mapping; otherwise the new call will remain unused.
subgraph/core/abi-migrations/SortitionModule.json (1)
512-530: ExposegetJurorLeftoverPNKacross the stack
Nice addition, but remember to:
• add it to the SDK typings,
• surface it in the subgraph schema & resolvers,
• update front-end components that show juror balances.Minor now, painful later if missed.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
subgraph/core-neo/abi-migrations/KlerosCoreNeo.json(3 hunks)subgraph/core-neo/abi-migrations/SortitionModuleNeo.json(9 hunks)subgraph/core/abi-migrations/KlerosCore.json(5 hunks)subgraph/core/abi-migrations/SortitionModule.json(10 hunks)
🧰 Additional context used
🧠 Learnings (5)
📓 Common learnings
Learnt from: jaybuidl PR: kleros/kleros-v2#1746 File: contracts/config/courts.v2.mainnet-neo.json:167-170 Timestamp: 2024-11-19T16:31:08.965Z Learning: In `contracts/config/courts.v2.mainnet-neo.json`, the `minStake` parameter is denominated in PNK, not ETH. Learnt from: Harman-singh-waraich PR: kleros/kleros-v2#1775 File: web/src/pages/Courts/CourtDetails/StakePanel/StakeWithdrawButton.tsx:0-0 Timestamp: 2024-12-09T12:36:59.441Z Learning: In the `StakeWithdrawButton` component, the transaction flow logic is tightly linked to component updates, so extracting it into a custom hook does not provide significant benefits. subgraph/core-neo/abi-migrations/KlerosCoreNeo.json (1)
Learnt from: jaybuidl PR: kleros/kleros-v2#1746 File: contracts/config/courts.v2.mainnet-neo.json:167-170 Timestamp: 2024-11-19T16:31:08.965Z Learning: In `contracts/config/courts.v2.mainnet-neo.json`, the `minStake` parameter is denominated in PNK, not ETH. subgraph/core/abi-migrations/KlerosCore.json (4)
Learnt from: Harman-singh-waraich PR: kleros/kleros-v2#1703 File: kleros-sdk/src/dataMappings/utils/actionTypes.ts:22-22 Timestamp: 2024-10-22T09:34:54.761Z Learning: In `kleros-sdk/src/dataMappings/utils/actionTypes.ts`, the `abi` field in `AbiCallMapping` is intentionally typed as `string` because it is parsed later in the action functions. Learnt from: Harman-singh-waraich PR: kleros/kleros-v2#1703 File: kleros-sdk/src/dataMappings/utils/actionTypes.ts:29-29 Timestamp: 2024-10-22T09:36:10.478Z Learning: In `kleros-sdk/src/dataMappings/utils/actionTypes.ts`, the `abi` field in `AbiEventMapping` is intentionally typed as `string` because it is parsed later in the action functions. Learnt from: Harman-singh-waraich PR: kleros/kleros-v2#1703 File: kleros-sdk/src/dataMappings/utils/actionTypes.ts:25-25 Timestamp: 2024-10-22T09:35:14.098Z Learning: In `kleros-sdk/src/dataMappings/utils/actionTypes.ts`, for the `AbiCallMapping` type, it's acceptable to use `args: any[]` since `args` can be anything. Learnt from: jaybuidl PR: kleros/kleros-v2#1746 File: contracts/config/courts.v2.mainnet-neo.json:167-170 Timestamp: 2024-11-19T16:31:08.965Z Learning: In `contracts/config/courts.v2.mainnet-neo.json`, the `minStake` parameter is denominated in PNK, not ETH. subgraph/core/abi-migrations/SortitionModule.json (6)
Learnt from: Harman-singh-waraich PR: kleros/kleros-v2#1703 File: kleros-sdk/src/dataMappings/utils/actionTypes.ts:22-22 Timestamp: 2024-10-22T09:34:54.761Z Learning: In `kleros-sdk/src/dataMappings/utils/actionTypes.ts`, the `abi` field in `AbiCallMapping` is intentionally typed as `string` because it is parsed later in the action functions. Learnt from: Harman-singh-waraich PR: kleros/kleros-v2#1703 File: kleros-sdk/src/dataMappings/utils/actionTypes.ts:29-29 Timestamp: 2024-10-22T09:36:10.478Z Learning: In `kleros-sdk/src/dataMappings/utils/actionTypes.ts`, the `abi` field in `AbiEventMapping` is intentionally typed as `string` because it is parsed later in the action functions. Learnt from: Harman-singh-waraich PR: kleros/kleros-v2#1703 File: kleros-sdk/src/dataMappings/utils/actionTypes.ts:25-25 Timestamp: 2024-10-22T09:35:14.098Z Learning: In `kleros-sdk/src/dataMappings/utils/actionTypes.ts`, for the `AbiCallMapping` type, it's acceptable to use `args: any[]` since `args` can be anything. Learnt from: jaybuidl PR: kleros/kleros-v2#1746 File: contracts/config/courts.v2.mainnet-neo.json:167-170 Timestamp: 2024-11-19T16:31:08.965Z Learning: In `contracts/config/courts.v2.mainnet-neo.json`, the `minStake` parameter is denominated in PNK, not ETH. Learnt from: jaybuidl PR: kleros/kleros-v2#1746 File: contracts/config/courts.v2.mainnet-neo.json:3-17 Timestamp: 2024-11-19T16:09:41.467Z Learning: In `contracts/config/courts.v2.mainnet-neo.json`, the General Court (ID: 1) can have its `parent` ID set to itself (`"parent": 1`), as there is no parent court with ID 0 currently. Learnt from: jaybuidl PR: kleros/kleros-v2#1746 File: contracts/config/courts.v2.mainnet-neo.json:3-5 Timestamp: 2024-11-19T17:18:39.007Z Learning: In `contracts/config/courts.v2.mainnet-neo.json`, the General Court (id: 1) intentionally references itself as its parent (`"parent": 1`). This self-reference is acceptable and should not be flagged as an issue in future reviews. subgraph/core-neo/abi-migrations/SortitionModuleNeo.json (4)
Learnt from: jaybuidl PR: kleros/kleros-v2#1746 File: contracts/config/courts.v2.mainnet-neo.json:167-170 Timestamp: 2024-11-19T16:31:08.965Z Learning: In `contracts/config/courts.v2.mainnet-neo.json`, the `minStake` parameter is denominated in PNK, not ETH. Learnt from: Harman-singh-waraich PR: kleros/kleros-v2#1716 File: web-devtools/src/app/(main)/dispute-template/CustomContextInputs.tsx:29-42 Timestamp: 2024-10-28T05:55:12.728Z Learning: In the `CustomContextInputs` component located at `web-devtools/src/app/(main)/dispute-template/CustomContextInputs.tsx`, the `DisputeRequestParams` array is used to exclude certain variables from the custom input since they are already provided in a preceding component. Therefore, converting it to a type is unnecessary. Learnt from: jaybuidl PR: kleros/kleros-v2#1746 File: contracts/config/courts.v2.mainnet-neo.json:3-17 Timestamp: 2024-11-19T16:09:41.467Z Learning: In `contracts/config/courts.v2.mainnet-neo.json`, the General Court (ID: 1) can have its `parent` ID set to itself (`"parent": 1`), as there is no parent court with ID 0 currently. Learnt from: jaybuidl PR: kleros/kleros-v2#1746 File: contracts/config/courts.v2.mainnet-neo.json:3-5 Timestamp: 2024-11-19T17:18:39.007Z Learning: In `contracts/config/courts.v2.mainnet-neo.json`, the General Court (id: 1) intentionally references itself as its parent (`"parent": 1`). This self-reference is acceptable and should not be flagged as an issue in future reviews. ⏰ 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: contracts-testing
- GitHub Check: Analyze (javascript)
🔇 Additional comments (11)
subgraph/core/abi-migrations/KlerosCore.json (4)
4-6: Constructor switched to non-payable – double-check deployment scripts
stateMutabilityis nownonpayable. Any script that still sendsmsg.valueduring deployment will revert.
Make sure all CI/forge/foundry deployment helpers and hard-hat tasks have been updated accordingly.
1387-1393:drawnow returnsnbDrawnJurors– update subgraph handlers & type-chainsThe added output changes the function signature and the generated TypeScript typings.
Down-stream consumers (subgraph mappings, SDK, front-end) must be regenerated; otherwise run-time decoding will break.
1740-1743:initialize3→initialize5rename – guard against dangling upgrade pathsProjects/tools that still call the old initializer will revert. Ensure:
- Upgrade scripts point to
initialize5.- No proxy instances were already initialized via
initialize3; otherwise the new check will revert withAlreadyInitialized.
1915-1931:transferBySortitionModuleexposes direct PNK transfers – verify access controlAssuming the Solidity side uses the
SortitionModuleOnlymodifier, this is fine.
However, if the modifier was accidentally dropped during the refactor, the core could be drained. Please audit the implementation before deploying.subgraph/core/abi-migrations/SortitionModule.json (2)
391-394:alreadyTransferredstill present indelayedStakesoutput
The migration spec and implementation claim this boolean was removed, yet the ABI still exposes it. Leaving it in place yields a hard mismatch between on-chain storage layout ⇄ subgraph/SDK decoding.Either:
- Remove the field from Solidity & regenerate the ABI, or
- Revert the spec/PR description.
[current ABI ⟂ PR description]
Likely an incorrect or invalid review comment.
747-758:penalizeStakereturn signature changed – audit downstream callers
The function now returns bothpnkBalanceandavailablePenalty.
Make sure every contract, test, subgraph mapping and SDK wrapper that invokespenalizeStakereads the new tuple; silent ignored returns can mask logic errors.subgraph/core-neo/abi-migrations/KlerosCoreNeo.json (1)
1457-1463: Return type ofdrawchanged – verify all consumers
draw()now returnsuint256 nbDrawnJurors. Ensure:
- Subgraph mappings update their call handlers (was
voidbefore).- All TS/JS wrappers regenerate to reflect the new return value.
- Down-stream Solidity callers (if any) ignore/consume the return value explicitly.
No change required in the ABI, but verification across repositories is essential.
subgraph/core-neo/abi-migrations/SortitionModuleNeo.json (4)
682-703:latestDelayedStakeIndexfunction still exposed – remove or mark deprecated
The summary says this helper was removed. Keeping it in the ABI while the implementation is gone (or returns dummy data) will throw on calls. Confirm implementation and either:
- Delete the ABI entry, or
- Re-add the function in Solidity and mark it
viewwith correct semantics.
64-81: Event names:LeftoverPNKvsLeftoverPNKWithdrawn– good consolidation
New events look coherent and replace the verbose legacy names. 👍
808-820: Extended return values forpenalizeStake– verify callers consume both
Returning(pnkBalance, availablePenalty)is a breaking change. Ensure Foundry/Hardhat tests and any off-chain services decode the tuple, otherwise only the first value will be captured.
1063-1101:validateStake↔setStakesplit captured correctly
The new pattern (dry-run validation → state mutation) is reflected accurately with clear I/O. Nice separation of concerns.
|
PR-Codex overview
This PR introduces significant updates to the staking mechanism in the Kleros protocol, including renaming events, modifying function signatures, and enhancing how stakes are validated and managed.
Detailed summary
DelayedtoStakingResultenum.StakeDelayedAlreadyTransferredtoStakeDelayed).setStakeandvalidateStakefunctions to streamline stake management.withdrawLeftoverPNKfunction for jurors.Summary by CodeRabbit
New Features
Improvements
Bug Fixes
Tests
Documentation and ABI