Universal Gated Dispute Kit and Shutter Gated Dispute Kit #2045
Conversation
❌ Deploy Preview for kleros-v2-university failed. Why did it fail? →
|
WalkthroughThis update refactors the Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant KlerosCore
participant DisputeKitGated
User->>KlerosCore: createDispute(extraData)
KlerosCore->>DisputeKitGated: drawJuror(juror, disputeId)
DisputeKitGated->>DisputeKitGated: extraDataToTokenInfo(extraData)
alt tokenGate is set
DisputeKitGated->>Juror: checkTokenBalance(tokenGate, tokenId, isERC1155)
alt juror passes gate
DisputeKitGated-->>KlerosCore: juror eligible
else juror fails gate
DisputeKitGated-->>KlerosCore: juror ineligible
end
else no tokenGate
DisputeKitGated-->>KlerosCore: juror eligible
end
Possibly related PRs
Suggested reviewers
Poem
✨ Finishing Touches
🪧 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 (
|
✅ 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-testnet ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| const arbitrationCost = await core["arbitrationCost(bytes)"](extraData); | ||
|
|
||
| // Warning: this dispute cannot be executed, in reality it should be created by an arbitrable contract, not an EOA. | ||
| const tx = await core["createDispute(uint256,bytes)"](2, extraData, { value: arbitrationCost }).then((tx) => |
Check notice
Code scanning / SonarCloud
Unused assignments should be removed Low test
❌ Deploy Preview for kleros-v2-neo failed. Why did it fail? →
|
jaybuidl
force-pushed
the
feat/universal-gated-dk
branch
from
b1e385d to
9d19c70
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
contracts/test/arbitration/dispute-kit-gated.ts (1)
108-110: Remove unused variable assignment.The variable
txis assigned but never used afterwards, as flagged by static analysis.Apply this diff to fix the unused assignment:
- const tx = await core["createDispute(uint256,bytes)"](2, extraData, { value: arbitrationCost }).then((tx) => - tx.wait() - ); + await core["createDispute(uint256,bytes)"](2, extraData, { value: arbitrationCost }).then((tx) => tx.wait());
🧹 Nitpick comments (1)
contracts/test/arbitration/dispute-kit-gated.ts (1)
127-133: Complete the test assertions.The test case is properly structured but missing the final assertions to validate the expected behavior.
The TODO comment indicates missing expectations. Consider adding assertions to verify:
- Jurors are drawn successfully
- Token gating requirements are met
- Event emissions are correct
Would you like me to help generate the missing assertions for this test?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
contracts/deploy/00-home-chain-arbitration.ts(2 hunks)contracts/hardhat.config.ts(2 hunks)contracts/src/arbitration/dispute-kits/DisputeKitGated.sol(3 hunks)contracts/src/arbitration/dispute-kits/DisputeKitShutter.sol(1 hunks)contracts/src/proxy/KlerosProxies.sol(1 hunks)contracts/test/arbitration/dispute-kit-gated.ts(1 hunks)contracts/test/arbitration/draw.ts(1 hunks)contracts/test/arbitration/index.ts(4 hunks)
✅ Files skipped from review due to trivial changes (1)
- contracts/src/arbitration/dispute-kits/DisputeKitShutter.sol
🚧 Files skipped from review as they are similar to previous changes (6)
- contracts/test/arbitration/draw.ts
- contracts/src/proxy/KlerosProxies.sol
- contracts/hardhat.config.ts
- contracts/deploy/00-home-chain-arbitration.ts
- contracts/test/arbitration/index.ts
- contracts/src/arbitration/dispute-kits/DisputeKitGated.sol
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: tractorss
PR: kleros/kleros-v2#1982
File: web/src/pages/Resolver/Landing/index.tsx:62-62
Timestamp: 2025-05-15T06:50:40.859Z
Learning: In the Landing component, it's safe to pass `dispute?.dispute?.arbitrated.id as 0x${string}` to `usePopulatedDisputeData` without additional null checks because the hook internally handles undefined parameters through its `isEnabled` flag and won't execute the query unless all required data is available.
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.
contracts/test/arbitration/dispute-kit-gated.ts (9)
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#1744
File: web/src/hooks/useGenesisBlock.ts:9-31
Timestamp: 2024-11-19T05:31:48.701Z
Learning: In `useGenesisBlock.ts`, within the `useEffect` hook, the conditions (`isKlerosUniversity`, `isKlerosNeo`, `isTestnetDeployment`) are mutually exclusive, so multiple imports won't execute simultaneously, and race conditions are not a concern.
Learnt from: Harman-singh-waraich
PR: kleros/kleros-v2#1703
File: kleros-sdk/src/sdk.ts:1-3
Timestamp: 2024-10-22T10:23:15.789Z
Learning: In `kleros-sdk/src/sdk.ts`, the `PublicClient` type is used and should not be flagged as unused.
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#1647
File: web/src/context/NewDisputeContext.tsx:0-0
Timestamp: 2024-10-08T16:23:56.290Z
Learning: The `delete` operator is used in the `constructDisputeTemplate` function in `web/src/context/NewDisputeContext.tsx` to remove the `policyURI` field if it is an empty string.
Learnt from: jaybuidl
PR: kleros/kleros-v2#1647
File: web/src/context/NewDisputeContext.tsx:0-0
Timestamp: 2024-07-25T11:58:27.058Z
Learning: The `delete` operator is used in the `constructDisputeTemplate` function in `web/src/context/NewDisputeContext.tsx` to remove the `policyURI` field if it is an empty string.
Learnt from: Harman-singh-waraich
PR: kleros/kleros-v2#1755
File: kleros-app/src/lib/atlas/hooks/useSessionStorage.ts:3-12
Timestamp: 2024-11-21T05:38:11.576Z
Learning: In the `useSessionStorage` hook in `kleros-app/src/lib/atlas/hooks/useSessionStorage.ts`, the error handling in the `catch` block covers cases where `window` is undefined or `sessionStorage` throws an exception, so additional checks are unnecessary.
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: tractorss
PR: kleros/kleros-v2#1982
File: web/src/pages/Resolver/Landing/index.tsx:62-62
Timestamp: 2025-05-15T06:50:40.859Z
Learning: In the Landing component, it's safe to pass `dispute?.dispute?.arbitrated.id as 0x${string}` to `usePopulatedDisputeData` without additional null checks because the hook internally handles undefined parameters through its `isEnabled` flag and won't execute the query unless all required data is available.
🪛 GitHub Check: SonarCloud
contracts/test/arbitration/dispute-kit-gated.ts
[notice] 108-108: Unused assignments should be removed
Remove this useless assignment to variable "tx".See more on SonarQube Cloud
⏰ 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). (11)
- GitHub Check: Redirect rules - kleros-v2-university
- GitHub Check: Header rules - kleros-v2-university
- GitHub Check: Pages changed - kleros-v2-university
- 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: Analyze (javascript)
- GitHub Check: contracts-testing
🔇 Additional comments (5)
contracts/test/arbitration/dispute-kit-gated.ts (5)
1-10: LGTM!The imports are well-structured and appropriate for a Hardhat test suite. The ESLint disable comments are standard practice for test files.
11-47: Well-structured test setup with deterministic randomness.The test setup properly uses deployment fixtures and implements deterministic randomness for reproducible tests. The constant definitions and contract initialization follow best practices.
49-64: Correct implementation of extraData encoding.The function properly packs token gating parameters and encodes them according to the contract's expected format. The comments clearly explain the packing structure.
66-107: Comprehensive test helper with proper validation.The function correctly implements the complete flow from juror staking to dispute creation, with appropriate balance checks and extraData validation.
111-125: Proper sortition phase management.The function correctly advances blockchain time and phases to enable juror drawing. The implementation follows the expected sortition flow.
|
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
contracts/test/arbitration/dispute-kit-gated.ts (1)
134-134: Remove unused assignment to variabletx.The assignment to
txis not used after this line. You can simplify this by removing the assignment.- const tx = await core["createDispute(uint256,bytes)"](2, extraData, { value: arbitrationCost }).then((tx) => - tx.wait() - ); + await core["createDispute(uint256,bytes)"](2, extraData, { value: arbitrationCost }).then((tx) => tx.wait());
🧹 Nitpick comments (3)
contracts/src/token/TestERC1155.sol (2)
19-21: Consider providing a meaningful URI for token metadata.The constructor initializes the ERC1155 with an empty URI string. For testing purposes, consider providing a placeholder URI or documenting that metadata is not required for these tests.
- constructor() ERC1155("") { + constructor() ERC1155("https://example.com/token/{id}.json") { owner = msg.sender; }
13-13: Remove unused variable.The
_nextTokenIdvariable is declared but never used in the contract. Consider removing it to reduce gas costs and code complexity.- uint256 private _nextTokenId;contracts/deploy/utils/deployTokens.ts (1)
69-81: Remove unused parameter.The
nameparameter is accepted but not used in the deployment. TheTestERC1155contract constructor doesn't take any arguments.export const deployERC1155 = async ( hre: HardhatRuntimeEnvironment, deployer: string, - name: string, ticker: string ): Promise<Contract> => { return getContractOrDeploy(hre, ticker, { from: deployer, contract: "TestERC1155", args: [], log: true, }); };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
contracts/deploy/utils/deployTokens.ts(1 hunks)contracts/src/arbitration/dispute-kits/DisputeKitGatedShutter.sol(1 hunks)contracts/src/token/TestERC1155.sol(1 hunks)contracts/test/arbitration/dispute-kit-gated.ts(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: tractorss
PR: kleros/kleros-v2#1982
File: web/src/pages/Resolver/Landing/index.tsx:62-62
Timestamp: 2025-05-15T06:50:40.859Z
Learning: In the Landing component, it's safe to pass `dispute?.dispute?.arbitrated.id as 0x${string}` to `usePopulatedDisputeData` without additional null checks because the hook internally handles undefined parameters through its `isEnabled` flag and won't execute the query unless all required data is available.
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.
contracts/test/arbitration/dispute-kit-gated.ts (5)
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#1744
File: web/src/hooks/useGenesisBlock.ts:9-31
Timestamp: 2024-11-19T05:31:48.701Z
Learning: In `useGenesisBlock.ts`, within the `useEffect` hook, the conditions (`isKlerosUniversity`, `isKlerosNeo`, `isTestnetDeployment`) are mutually exclusive, so multiple imports won't execute simultaneously, and race conditions are not a concern.
Learnt from: Harman-singh-waraich
PR: kleros/kleros-v2#1703
File: kleros-sdk/src/sdk.ts:1-3
Timestamp: 2024-10-22T10:23:15.789Z
Learning: In `kleros-sdk/src/sdk.ts`, the `PublicClient` type is used and should not be flagged as unused.
Learnt from: jaybuidl
PR: kleros/kleros-v2#1647
File: web/src/context/NewDisputeContext.tsx:0-0
Timestamp: 2024-10-08T16:23:56.290Z
Learning: The `delete` operator is used in the `constructDisputeTemplate` function in `web/src/context/NewDisputeContext.tsx` to remove the `policyURI` field if it is an empty string.
Learnt from: jaybuidl
PR: kleros/kleros-v2#1647
File: web/src/context/NewDisputeContext.tsx:0-0
Timestamp: 2024-07-25T11:58:27.058Z
Learning: The `delete` operator is used in the `constructDisputeTemplate` function in `web/src/context/NewDisputeContext.tsx` to remove the `policyURI` field if it is an empty string.
🧬 Code Graph Analysis (1)
contracts/deploy/utils/deployTokens.ts (1)
contracts/deploy/utils/getContractOrDeploy.ts (1)
getContractOrDeploy(6-20)
🪛 GitHub Check: SonarCloud
contracts/test/arbitration/dispute-kit-gated.ts
[notice] 134-134: Unused assignments should be removed
Remove this useless assignment to variable "tx".See more on SonarQube Cloud
⏰ 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). (3)
- GitHub Check: Redirect rules - kleros-v2-testnet
- GitHub Check: Header rules - kleros-v2-testnet
- GitHub Check: Pages changed - kleros-v2-testnet
🔇 Additional comments (15)
contracts/src/token/TestERC1155.sol (2)
27-30: LGTM: Secure access control implementation.The
ownerOnlymodifier correctly restricts access to sensitive functions and provides clear error messaging.
44-63: LGTM: Proper delegation to OpenZeppelin's internal functions.The mint and burn functions correctly delegate to the secure internal implementations from the base ERC1155 contract while maintaining proper access control.
contracts/test/arbitration/dispute-kit-gated.ts (5)
75-90: LGTM: Well-designed extraData encoding function.The
encodeExtraDatafunction correctly packs the token gating parameters according to the expected format. The solidityPacked call properly handles the address and boolean flag packing.
92-151: LGTM: Comprehensive staking and drawing helper function.The
stakeAndDrawfunction properly simulates the complete flow of juror staking, dispute creation, phase transitions, and drawing. The use of deterministic RNG and proper event verification enhances test reliability.
153-198: LGTM: Thorough DAI token gating tests.Both test cases properly verify the token gating logic - ensuring no jurors are drawn without DAI balance and only token holders are selected when they have balance. The verification of locked PNK amounts adds good coverage.
200-245: LGTM: Comprehensive ERC721 token gating tests.The ERC721 tests follow the same pattern as DAI tests and properly verify token ownership requirements for juror selection.
247-292: LGTM: Complete ERC1155 token gating tests.The ERC1155 tests properly validate token ID-specific ownership requirements, which is crucial for ERC1155 token gating functionality.
contracts/src/arbitration/dispute-kits/DisputeKitGatedShutter.sol (8)
7-21: LGTM: Well-defined interfaces for token balance checking.The interfaces correctly define the
balanceOffunctions for both standard tokens (ERC-20/ERC-721) and ERC-1155 tokens, providing type safety for the gating logic.
55-65: LGTM: Proper initialization and upgradeability setup.The constructor correctly disables initializers for the proxy pattern, and the initialize function properly calls the base class initializer with the reinitializer modifier.
90-99: LGTM: Secure shielded voting commit function.The
castCommitShutterfunction properly delegates to the base_castCommitand emits the appropriate event with encrypted vote data for shielded voting.
101-113: LGTM: Proper vote reveal implementation.The
castVoteShutterfunction correctly retrieves the juror address and delegates to the base_castVotefunction, maintaining the security of the reveal phase.
126-133: LGTM: Secure vote hashing implementation.The
hashVotefunction properly hashes the justification first, then combines all parameters usingabi.encode, providing security against hash collision attacks.
152-153: Good defensive programming with length check.The function correctly validates that
_extraDatahas sufficient length before attempting to read from it, preventing potential out-of-bounds reads.
167-188: LGTM: Robust token gating implementation.The
_postDrawCheckfunction properly:
- Calls the parent implementation first
- Extracts token gating parameters from extraData
- Handles the case where no token gate is specified
- Uses appropriate interfaces for different token types
The logic correctly distinguishes between ERC-1155 and ERC-20/ERC-721 tokens.
149-164: Assembly parameter extraction is correctly guarded
The length check (_extraData.length < 160) and the use of offsets0x80(tokenGate / isERC1155) and0xA0(tokenId) match the ABI-encoded bytes layout, preventing out-of-bounds reads. No security issues detected.• File: contracts/src/arbitration/dispute-kits/DisputeKitGatedShutter.sol
Lines: 149–164
PR-Codex overview
This PR enhances the Kleros arbitration system by introducing new dispute kits, including
DisputeKitGatedandDisputeKitGatedShutter, which implement token gating for jurors using ERC-20, ERC-721, and ERC-1155 tokens. It also adds aTestERC1155contract for testing purposes.Detailed summary
DisputeKitGatedandDisputeKitGatedShuttercontracts for token gating.TestERC1155contract with minting, burning, and ownership management.deployTokens.tsto includedeployERC1155function.hardhat.config.tsto streamline account management.dispute-kit-gated.tsfor different token types.DisputeKitClassicto accommodate new dispute kits and events.Summary by CodeRabbit