-
Notifications
You must be signed in to change notification settings - Fork 48
Release #2012
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
…cting an internal function
…ata sources not deployed e.g. DisputeKitShutter on testnet
…or-multiple-dispute-kit-above-id-1
…ple-dispute-kit-above-id-1 feat: subgraph support for shutter disputekit in devnet
fix(web): blst error shutter v0.0.2
fix(subgraph): total-leaderboard-jurors-count
…bgraph chore: dispute-kit-gated-subgraph-support
Support for Gated Dispute Kits and other improvements
chore: testnet upgrade and new Dispute Kits deployment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 2
🧹 Nitpick comments (1)
contracts/deployments/arbitrumSepolia/DisputeKitClassic.json (1)
757-785
: Minor naming / UX nit – consider pluralising the function name
getLocalDisputeRoundID()
returns two IDs (localDisputeID
,localRoundID
) yet its name is singular.
Renaming togetLocalDisputeAndRoundIDs
(plural) would better communicate the tuple return and avoid future mis-reads.Purely cosmetic, feel free to ignore if you want to preserve backwards compatibility.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
yarn.lock
is excluded by!**/yarn.lock
,!**/*.lock
📒 Files selected for processing (10)
contracts/README.md
(4 hunks)contracts/deployments/arbitrumSepolia/DisputeKitClassic.json
(5 hunks)contracts/deployments/arbitrumSepolia/DisputeKitGated.json
(1 hunks)contracts/deployments/arbitrumSepolia/DisputeKitGatedShutter.json
(1 hunks)contracts/deployments/arbitrumSepolia/DisputeKitGatedShutter_Proxy.json
(1 hunks)contracts/deployments/arbitrumSepolia/DisputeKitGated_Proxy.json
(1 hunks)contracts/deployments/arbitrumSepolia/DisputeKitShutter.json
(1 hunks)contracts/deployments/arbitrumSepolia/DisputeKitShutter_Proxy.json
(1 hunks)contracts/deployments/arbitrumSepolia/KlerosCore.json
(4 hunks)contracts/deployments/arbitrumSepolia/SortitionModule.json
(11 hunks)
✅ Files skipped from review due to trivial changes (6)
- contracts/deployments/arbitrumSepolia/DisputeKitShutter_Proxy.json
- contracts/deployments/arbitrumSepolia/DisputeKitGated_Proxy.json
- contracts/deployments/arbitrumSepolia/DisputeKitGatedShutter.json
- contracts/deployments/arbitrumSepolia/DisputeKitGated.json
- contracts/deployments/arbitrumSepolia/DisputeKitGatedShutter_Proxy.json
- contracts/deployments/arbitrumSepolia/DisputeKitShutter.json
🚧 Files skipped from review as they are similar to previous changes (1)
- contracts/README.md
🧰 Additional context used
🧠 Learnings (4)
📓 Common learnings
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: kemuru
PR: kleros/kleros-v2#1702
File: web/src/pages/Home/TopJurors/JurorCard/index.tsx:10-11
Timestamp: 2024-10-07T06:18:23.427Z
Learning: In the `kleros-v2` codebase, the property `totalResolvedDisputes` should remain and should not be renamed to `totalResolvedVotes`.
Learnt from: kemuru
PR: kleros/kleros-v2#1702
File: web/src/pages/Home/TopJurors/JurorCard/index.tsx:10-11
Timestamp: 2024-10-08T16:23:56.291Z
Learning: In the `kleros-v2` codebase, the property `totalResolvedDisputes` should remain and should not be renamed to `totalResolvedVotes`.
contracts/deployments/arbitrumSepolia/DisputeKitClassic.json (4)
Learnt from: Harman-singh-waraich
PR: #1703
File: kleros-sdk/src/utils/getDispute.ts:38-40
Timestamp: 2024-10-21T10:32:16.970Z
Learning: The variables 'arbitrableChainID' and 'externalDisputeID' are required by the context to have uppercase 'ID', so they should remain unchanged even if the corresponding source properties use 'Id'.
Learnt from: kemuru
PR: #1702
File: web/src/pages/Home/TopJurors/JurorCard/index.tsx:10-11
Timestamp: 2024-10-07T06:18:23.427Z
Learning: In the kleros-v2
codebase, the property totalResolvedDisputes
should remain and should not be renamed to totalResolvedVotes
.
Learnt from: kemuru
PR: #1702
File: web/src/pages/Home/TopJurors/JurorCard/index.tsx:10-11
Timestamp: 2024-10-08T16:23:56.291Z
Learning: In the kleros-v2
codebase, the property totalResolvedDisputes
should remain and should not be renamed to totalResolvedVotes
.
Learnt from: Harman-singh-waraich
PR: #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.
contracts/deployments/arbitrumSepolia/KlerosCore.json (1)
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.
contracts/deployments/arbitrumSepolia/SortitionModule.json (3)
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.
Learnt from: jaybuidl
PR: #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: #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). (18)
- GitHub Check: Redirect rules - kleros-v2-university
- GitHub Check: Header rules - kleros-v2-university
- GitHub Check: Pages changed - kleros-v2-university
- GitHub Check: contracts-testing
- GitHub Check: SonarCloud
- GitHub Check: Analyze (javascript)
- GitHub Check: Redirect rules - kleros-v2-neo
- GitHub Check: Header rules - kleros-v2-neo
- GitHub Check: Redirect rules - kleros-v2-testnet
- GitHub Check: Pages changed - kleros-v2-neo
- 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: SonarCloud
- GitHub Check: Analyze (javascript)
- GitHub Check: contracts-testing
🔇 Additional comments (13)
contracts/deployments/arbitrumSepolia/DisputeKitClassic.json (4)
486-504
: Confirm active-flag lifecycle is handled correctly
coreDisputeIDToActive()
surfaces the liveness of a dispute at the DK level.
Because this is now part of the public interface, it is critical that the flag is:
- Set to
true
exactly once at dispute creation, and- Reset to
false
immediately after the final ruling (including jumps / forks).Otherwise the mapping becomes an ever-growing list of
true
values that can never be GC’d, inflating storage costs indefinitely.Please double-check the implementation in
DisputeKitClassic.sol
and the core hooks that flip this bit.
787-804
: LGTM – cheap accessor
getNumberOfRounds(uint256)
is a view that will be heavily used by indexers; exposing it directly is welcome.
Assuming the implementation is an O(1) array length read, no concerns.
916-944
: Ensure off-chain commit hash helpers include justification too
hashVote(uint256 _choice, uint256 _salt, string _justification)
now factors the justification into the commitment.
This prevents jurors from editing justifications post-reveal, but it is a breaking change for any client code that previously usedkeccak256(abi.encodePacked(choice, salt))
.Please verify that:
• Front-end helpers (e.g.utils/hashVote.ts
) have been updated,
• Keeper / bot scripts use the same formula, and
• Historical disputes created with the old hash are not affected (i.e. the DK version check gates the new logic).If any external integrations rely on the old 2-field hash, consider keeping both helpers or bumping a major version.
965-969
: Initializer v7 – double-check that storage gaps remain forward-compatibleThe proxy now executes
initialize7()
with no arguments.
Given previous initializers took governor & core addresses, the new version presumably only migrates storage/layout.Action items:
- Confirm
initialize7()
isreinitializer(7)
(or equivalent) to guard against replay.- Make sure the upgrade script does not accidentally re-run earlier initializers on fresh deployments.
- Verify that storage gaps (if any) were not repurposed, to avoid clashing with future upgrades.
Everything else looks consistent with the UUPS flow.
Also applies to: 1165-1167
contracts/deployments/arbitrumSepolia/KlerosCore.json (3)
1682-1686
:initialize5()
– verify upgrade flow & single-execution guaranteeThe proxy is executed with
initialize5()
(no params).
Make sure:
- Older deployment scripts are updated to call only this new version to avoid the
AlreadyInitialized
revert.- Any new storage introduced in re-initializer 5 is actually set via the call (empty args here suggest purely internal changes).
1329-1336
: Audit alldraw()
invocations to consume its new return valueThe
draw
method now returnsnbDrawnJurors
. Any script, test or front-end code that simply callsawait core.draw(disputeID, iterations)will only get a TransactionResponse and silently discard the juror count. Please review and update all callers, for example:
- contracts/test/integration/index.ts
- contracts/test/arbitration/staking.ts
- contracts/test/arbitration/draw.ts
- contracts/scripts/console-init.ts
- specifications/dispute-kit-classic.md
Ensure consumers needing the juror count use a static call or unpack the return, e.g.:
const nbDrawnJurors = await core.callStatic.draw(disputeID, 10);Or, if you intentionally ignore it, make that explicit in code or comments.
1857-1874
: transferBySortitionModule is already access‐controlledThe
transferBySortitionModule
implementation in KlerosCoreBase checks that only the sortition module can call it:function transferBySortitionModule(address _account, uint256 _amount) external { if (msg.sender != address(sortitionModule)) revert SortitionModuleOnly(); pinakion.safeTransfer(_account, _amount); }No further changes needed.
contracts/deployments/arbitrumSepolia/SortitionModule.json (6)
68-84
: Event rename looks good – confirm downstream updates.
LeftoverPNK
cleanly replaces the old delayed–stake leftover-PNK events and indexes the_account
address, which is what sub-graph consumers usually filter on.
Please double-check that:
- Subgraph mappings (and any off-chain listeners) have been migrated to this new event name.
- No code still relies on the legacy
StakeDelayedAlreadyTransferredDeposited
/…Withdrawn
events.
491-509
: Handy view helper – LGTM.
getJurorLeftoverPNK(address)
is a welcome addition and isview
, avoiding gas for simple queries.
No concerns here.
562-566
:initialize4()
– ensure proper re-initializer guards.Having both
initialize
and a parameter-lessinitialize4
is fine under the OZreinitializer(4)
pattern, but please verify that:
• The implementation contract uses@openzeppelin/contracts-upgradeable
’sreinitializer(4)
modifier, not a custom boolean, and
• Front-end/deployment scripts call the correct initializer only once.
1019-1031
: Confirm Access Control on withdrawLeftoverPNK(address)I wasn’t able to locate the Solidity source for the SortitionModule implementation, only its ABI in
contracts/deployments/arbitrumSepolia/SortitionModule.json
. Please verify in your SortitionModule.sol that:•
withdrawLeftoverPNK(address _account)
enforces that only_account
can withdraw its own leftovers (for example,require(msg.sender == _account)
).
• If there is no such check, either add an appropriate access modifier (onlyAccount
,onlyOwner
, etc.) or refactor the function towithdrawLeftoverPNK()
(no args) and usemsg.sender
directly.Without confirming these protections, callers could drain or grief arbitrary accounts.
858-880
: Incorrect:setStake
ABI matches contract signatureThe deployed ABI’s
setStake
inputs—(_account, _courtID, _pnkDeposit, _pnkWithdrawal, _newStake)
—exactly mirror the Solidity declaration in SortitionModuleBase.sol. There is no_alreadyTransferred
parameter in the function signature to drop, nor any reordering beyond the intended parameter order.You can safely ignore this review comment.
Likely an incorrect or invalid review comment.
128-141
: Indexeduint96
is supported by The Graph via BigInt — no change neededThe Graph’s AssemblyScript API only offers
i32
(for 32-bit values) andBigInt
(for anything larger). Auint96
event parameter will automatically be decoded as aBigInt
in your mappings, so you don’t need to switch it touint256
or remove theindexed
flag. Just be sure your handler uses:let courtID = event.params._courtID.toBigInt();Likely an incorrect or invalid review comment.
"name": "penalizeStake", | ||
"outputs": [], | ||
"outputs": [ | ||
{ | ||
"internalType": "uint256", | ||
"name": "pnkBalance", | ||
"type": "uint256" | ||
}, | ||
{ | ||
"internalType": "uint256", | ||
"name": "availablePenalty", | ||
"type": "uint256" | ||
} | ||
], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
penalizeStake
return tuple changes are breaking.
The function now returns (pnkBalance, availablePenalty)
.
All callers (Core, DKs, bots, tests, subgraph handlers) must adjust to:
(uint256 balanceAfter, uint256 penaltyApplied) = sortition.penalizeStake(...);
Failing to update will silently ignore the new return data and may break business logic.
🤖 Prompt for AI Agents
In contracts/deployments/arbitrumSepolia/SortitionModule.json around lines 725
to 737, the penalizeStake function's return type has changed to a tuple
(pnkBalance, availablePenalty). Update all callers of penalizeStake in the Core,
DKs, bots, tests, and subgraph handlers to destructure the returned tuple into
two variables, for example (uint256 balanceAfter, uint256 penaltyApplied), to
properly handle both returned values and avoid ignoring important data that
could break business logic.
{ | ||
"inputs": [ | ||
{ | ||
"internalType": "address", | ||
"name": "_account", | ||
"type": "address" | ||
}, | ||
{ | ||
"internalType": "uint96", | ||
"name": "_courtID", | ||
"type": "uint96" | ||
}, | ||
{ | ||
"internalType": "uint256", | ||
"name": "_newStake", | ||
"type": "uint256" | ||
} | ||
], | ||
"name": "validateStake", | ||
"outputs": [ | ||
{ | ||
"internalType": "uint256", | ||
"name": "pnkDeposit", | ||
"type": "uint256" | ||
}, | ||
{ | ||
"internalType": "uint256", | ||
"name": "pnkWithdrawal", | ||
"type": "uint256" | ||
}, | ||
{ | ||
"internalType": "enum StakingResult", | ||
"name": "stakingResult", | ||
"type": "uint8" | ||
} | ||
], | ||
"stateMutability": "nonpayable", | ||
"type": "function" | ||
}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Refactor suggestion
validateStake
should be view
– no state mutation observed.
validateStake
only returns computed values and an enum – it shouldn’t modify storage. Marking it nonpayable
instead of view
prevents Solidity from enforcing read-only guarantees and incurs higher gas if mis-used.
-function validateStake(address _account, uint96 _courtID, uint256 _newStake) external nonpayable
+function validateStake(address _account, uint96 _courtID, uint256 _newStake) external view
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In contracts/deployments/arbitrumSepolia/SortitionModule.json between lines 967
and 1005, the function validateStake is marked as nonpayable but does not modify
any state and only returns computed values. Change its stateMutability from
"nonpayable" to "view" to correctly indicate it is a read-only function,
enabling Solidity to enforce this and optimize gas usage.
|
Contract changes
PR-Codex overview
This PR focuses on enhancing the dispute resolution system by introducing new components and updating existing ones, particularly around the
Shutter
dispute kit, along with various improvements in the codebase for better functionality and maintainability.Detailed summary
AllCasesButton
component.SeeAllCasesButton
andSeeAllJurorsButton
components.JustificationArea
import paths.disputeKit
to theStakingResult
enum.REACT_APP_SHUTTER_API
.disputeKit
structure.package.json
files.DisputeKitGatedShutterProxy
andDisputeKitShutterProxy
.Summary by CodeRabbit
New Features
Improvements
Bug Fixes
Chores