Token whitelist for DisputeKitGated and DisputeKitGatedShutter #2176
Conversation
✅ 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-testnet-devtools failed. Why did it fail? →
|
jaybuidl
added
Type: Bug 🐛
Package: Contracts
WalkthroughcreateDispute visibility made overridable in DisputeKitClassicBase. DisputeKitGated and DisputeKitGatedShutter add token-gating state and checks (supportedTokens, changeSupportedTokens, TokenNotSupported) and override createDispute to validate tokenGate extracted from extraData. Tests refactored to shared helpers and add mocks exposing token parsing. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Core
participant Gated as DisputeKitGated
participant Base as DisputeKitClassicBase
Core->>Gated: createDispute(coreDisputeID, coreRoundID, choices, extraData, nbVotes)
note right of Gated: extract tokenGate via _extraDataToTokenInfo(extraData)
alt tokenGate not supported
Gated-->>Core: revert TokenNotSupported(tokenGate)
else supported
Gated->>Base: super.createDispute(...)
Base-->>Core: dispute created
end
sequenceDiagram
autonumber
actor Core
participant GatedShutter as DisputeKitGatedShutter
participant Base as DisputeKitClassicBase
Core->>GatedShutter: createDispute(..., extraData, ...)
GatedShutter->>GatedShutter: _extraDataToTokenInfo(extraData)
alt unsupported
GatedShutter-->>Core: revert TokenNotSupported
else supported
GatedShutter->>Base: super.createDispute(...)
Base-->>Core: success
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
✅ Deploy Preview for kleros-v2-neo ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
![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
🧹 Nitpick comments (5)
contracts/src/arbitration/dispute-kits/DisputeKitGatedShutter.sol (2)
100-107: Emit events for governance transparency.The
changeSupportedTokensfunction modifies critical state but doesn't emit events. Consider emitting an event to track token whitelist changes for transparency and off-chain monitoring.Apply this diff:
+ event SupportedTokensChanged(address indexed token, bool supported); + /// @notice Changes the supported tokens. /// @param _tokens The tokens to support. /// @param _supported Whether the tokens are supported or not. function changeSupportedTokens(address[] memory _tokens, bool _supported) external onlyByOwner { for (uint256 i = 0; i < _tokens.length; i++) { supportedTokens[_tokens[i]] = _supported; + emit SupportedTokensChanged(_tokens[i], _supported); } }
100-107: Validate token addresses.The function doesn't validate that token addresses are non-zero. While
address(0)might be intentionally supported for "no gating," explicit validation or documentation would prevent accidental misconfigurations.Consider adding validation:
function changeSupportedTokens(address[] memory _tokens, bool _supported) external onlyByOwner { for (uint256 i = 0; i < _tokens.length; i++) { + if (_tokens[i] == address(0) && _supported) revert InvalidTokenAddress(); supportedTokens[_tokens[i]] = _supported; } }contracts/test/arbitration/dispute-kit-gated.ts (1)
162-301: Test coverage gap: whitelist functionality not tested.The tests verify token-gating behavior (jurors with/without tokens) but don't cover the new
changeSupportedTokensgovernance function or the scenario where a token is not whitelisted (should revert withTokenNotSupported).Consider adding tests for:
- Calling
changeSupportedTokensto whitelist tokens- Attempting to create a dispute with an unsupported token (should revert)
- Removing tokens from the whitelist
Would you like me to generate these additional test cases?
contracts/src/arbitration/dispute-kits/DisputeKitGated.sol (2)
71-78: Emit events for governance transparency.The
changeSupportedTokensfunction modifies critical state but doesn't emit events. Consider emitting an event to track token whitelist changes for transparency and off-chain monitoring.Apply this diff:
+ event SupportedTokensChanged(address indexed token, bool supported); + /// @notice Changes the supported tokens. /// @param _tokens The tokens to support. /// @param _supported Whether the tokens are supported or not. function changeSupportedTokens(address[] memory _tokens, bool _supported) external onlyByOwner { for (uint256 i = 0; i < _tokens.length; i++) { supportedTokens[_tokens[i]] = _supported; + emit SupportedTokensChanged(_tokens[i], _supported); } }
71-78: Validate token addresses.The function doesn't validate that token addresses are non-zero. While
address(0)might be intentionally supported for "no gating," explicit validation or documentation would prevent accidental misconfigurations.Consider adding validation:
function changeSupportedTokens(address[] memory _tokens, bool _supported) external onlyByOwner { for (uint256 i = 0; i < _tokens.length; i++) { + if (_tokens[i] == address(0) && _supported) revert InvalidTokenAddress(); supportedTokens[_tokens[i]] = _supported; } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
contracts/src/arbitration/dispute-kits/DisputeKitClassicBase.sol(1 hunks)contracts/src/arbitration/dispute-kits/DisputeKitGated.sol(4 hunks)contracts/src/arbitration/dispute-kits/DisputeKitGatedShutter.sol(5 hunks)contracts/test/arbitration/dispute-kit-gated.ts(5 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
contracts/test/arbitration/dispute-kit-gated.ts (1)
contracts/deploy/utils/deployUpgradable.ts (1)
deployUpgradable(37-89)
⏰ 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-neo
- GitHub Check: Header rules - kleros-v2-neo
- GitHub Check: Pages changed - kleros-v2-neo
- 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: Analyze (javascript)
- GitHub Check: hardhat-tests
🔇 Additional comments (8)
contracts/src/arbitration/dispute-kits/DisputeKitClassicBase.sol (1)
201-207: LGTM! Visibility change enables token-gating pattern.Changing from
externaltopublic virtualcorrectly allows derived dispute kits (DisputeKitGated, DisputeKitGatedShutter) to override this function, perform token validation, then delegate to the base implementation viasuper.createDispute(). TheonlyByCoremodifier is preserved, ensuring access control remains intact.contracts/src/arbitration/dispute-kits/DisputeKitGatedShutter.sol (3)
113-126: Token gating logic is correct.The override properly extracts the token address from
extraData, validates it against the whitelist, and delegates to the base implementation. The comment correctly notes thatsuper.createDispute()enforcesonlyByCoreaccess control.
237-252: Visibility change tointernalis appropriate.Changing
_extraDataToTokenInfofrompublictointernalis correct since it's an implementation detail only needed by internal logic and overriding contracts. Tests now use a mock that exposes this publicly.
37-37: Consider initializing supported tokens.The
supportedTokensmapping is not initialized in theinitializefunction. This means all tokens start as unsupported, requiring explicit whitelisting through governance after deployment. Consider whether any tokens should be supported by default.Do you want to initialize any tokens as supported by default, or is the empty whitelist intentional?
contracts/test/arbitration/dispute-kit-gated.ts (1)
58-68: Deployment pattern is correct.The switch to using
deployUpgradableandDisputeKitGatedMockaligns with the upgradeable proxy pattern and enables testing of internal functions through the mock contract.contracts/src/arbitration/dispute-kits/DisputeKitGated.sol (3)
84-97: Token gating logic is correct.The override properly extracts the token address from
extraData, validates it against the whitelist, and delegates to the base implementation. The comment correctly notes thatsuper.createDispute()enforcesonlyByCoreaccess control.
113-128: Visibility change tointernalis appropriate.Changing
_extraDataToTokenInfofrompublictointernalis correct since it's an implementation detail only needed by internal logic and overriding contracts. Tests now use a mock that exposes this publicly.
36-36: Consider initializing supported tokens.The
supportedTokensmapping is not initialized in theinitializefunction. This means all tokens start as unsupported, requiring explicit whitelisting through governance after deployment. Consider whether any tokens should be supported by default.Do you want to initialize any tokens as supported by default, or is the empty whitelist intentional?
jaybuidl
force-pushed
the
fix/gated-dk-token-whitelist
branch
from
17c684f to
bcbe228
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 (2)
contracts/src/test/DisputeKitGatedMock.sol(1 hunks)contracts/test/arbitration/dispute-kit-gated.ts(13 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
contracts/test/arbitration/dispute-kit-gated.ts (2)
contracts/deploy/utils/index.ts (1)
PNK(40-40)contracts/deploy/utils/deployUpgradable.ts (1)
deployUpgradable(37-89)
⏰ 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: Analyze (javascript)
- GitHub Check: hardhat-tests
🔇 Additional comments (8)
contracts/test/arbitration/dispute-kit-gated.ts (8)
22-31: LGTM!The documentation comment provides a clear overview of the test coverage, which is helpful for understanding the test suite structure.
69-80: LGTM!The deployment using
deployUpgradablecorrectly initializes the mock contract with proper proxy setup, adds it to the core, and enables it for the GENERAL court.
99-100: LGTM!Whitelisting all tokens by default is a pragmatic approach that allows existing tests to continue working without modification while new tests can explicitly test whitelist behavior.
106-145: LGTM!The helper functions are well-designed, reduce code duplication, and provide a clean API for test operations. The type handling for
string | Addressableis appropriate and flexible.
202-273: LGTM!The whitelist management tests provide comprehensive coverage including:
- Single and batch operations
- Add and remove operations
- Mixed operations
- Idempotency (duplicate operations)
- Access control
The test structure is well-organized with clear descriptions.
275-312: LGTM!The error handling tests thoroughly verify the
TokenNotSupportederror for all token types (ERC20, ERC721, ERC1155) and include a state transition test that validates the full lifecycle of removing and re-adding tokens.
455-510: LGTM!The integration tests validate the complete workflow:
- Token whitelisting → dispute creation → juror drawing
- Token removal → dispute rejection
- State consistency across multiple operations
These tests ensure the whitelist feature integrates correctly with the existing dispute system.
180-180: LGTM!The call is correctly updated to use the public
extraDataToTokenInfowrapper fromDisputeKitGatedMockinstead of the now-internal_extraDataToTokenInfo.
| /// @title KlerosCoreMock | ||
| /// KlerosCore with view functions to use in Foundry tests. |
There was a problem hiding this comment.
Fix the outdated comment.
The comment references "KlerosCoreMock" but the contract is "DisputeKitGatedMock". This appears to be a copy-paste error from another mock file.
Apply this diff to correct the comment:
-/// @title KlerosCoreMock
-/// KlerosCore with view functions to use in Foundry tests.
+/// @title DisputeKitGatedMock
+/// @dev Test mock that exposes internal functions for testing DisputeKitGated.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| /// @title KlerosCoreMock | |
| /// KlerosCore with view functions to use in Foundry tests. | |
| /// @title DisputeKitGatedMock | |
| /// @dev Test mock that exposes internal functions for testing DisputeKitGated. |
🤖 Prompt for AI Agents
In contracts/src/test/DisputeKitGatedMock.sol around lines 7 to 8, the file
header comment incorrectly says "KlerosCoreMock" while the contract is named
"DisputeKitGatedMock"; update the comment to reference the correct contract name
and purpose (e.g., "DisputeKitGatedMock" and a brief description matching this
mock) so the header accurately reflects the file's contract.
|
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
contracts/src/arbitration/dispute-kits/DisputeKitGatedShutter.sol (1)
258-279: Validate token is a contract to prevent draw-time revertsIf a non-contract or wrong interface address is whitelisted, _postDrawCheck may revert on balanceOf. Add a code-size check when gating is active to fail early during dispute creation.
function createDispute( @@ - (address tokenGate, , ) = _extraDataToTokenInfo(_extraData); + (address tokenGate, , ) = _extraDataToTokenInfo(_extraData); if (!supportedTokens[tokenGate]) revert TokenNotSupported(tokenGate); + if (tokenGate != NO_TOKEN_GATE) { + // Prevent footguns: require a contract address + if (tokenGate.code.length == 0) revert TokenNotSupported(tokenGate); + }
🧹 Nitpick comments (11)
contracts/src/arbitration/dispute-kits/DisputeKitGatedShutter.sol (3)
103-111: Emit whitelist change events and consider zero-address policy
- No event is emitted on whitelist updates; governance changes are silent on-chain.
- Clarify whether toggling NO_TOKEN_GATE (address(0)) is allowed; currently possible via changeSupportedTokens.
Add an event and emit per token; optionally guard address(0) if you want to keep “no-gate” always enabled.
+ // * Events * // + event SupportedTokenChanged(address indexed token, bool supported); @@ function changeSupportedTokens(address[] memory _tokens, bool _supported) external onlyByOwner { for (uint256 i = 0; i < _tokens.length; i++) { - supportedTokens[_tokens[i]] = _supported; + address token = _tokens[i]; + supportedTokens[token] = _supported; + emit SupportedTokenChanged(token, _supported); } }
116-129: createDispute pre-check OK; consider early “no-gate” fast pathLogic is correct. Minor optimization/readability: early-return when tokenGate == NO_TOKEN_GATE to skip mapping lookup.
240-255: Avoid calldata→memory copy for extraData parsingThis is called from createDispute(bytes calldata _extraData) but accepts bytes memory, incurring a copy. Consider overloading with a calldata variant to save gas at hot paths.
- function _extraDataToTokenInfo( - bytes memory _extraData - ) internal pure returns (address tokenGate, bool isERC1155, uint256 tokenId) { + function _extraDataToTokenInfoCalldata( + bytes calldata _extraData + ) internal pure returns (address tokenGate, bool isERC1155, uint256 tokenId) { // Need at least 160 bytes to safely read the parameters if (_extraData.length < 160) return (address(0), false, 0); assembly { let packedTokenGateIsERC1155 := calldataload(_extraData.offset + 0x60) // 4th param (128) tokenId := calldataload(_extraData.offset + 0x80) // 5th param (160) tokenGate := and(packedTokenGateIsERC1155, 0xffffffffffffffffffffffffffffffffffffffff) isERC1155 := and(shr(160, packedTokenGateIsERC1155), 1) } }Then use the calldata version in createDispute; keep the memory version for storage/memory call sites.
contracts/src/arbitration/dispute-kits/DisputeKitGated.sol (3)
74-82: Emit events for whitelist changesMirror the event in the non-Shutter variant to keep observability consistent.
+ event SupportedTokenChanged(address indexed token, bool supported); @@ function changeSupportedTokens(address[] memory _tokens, bool _supported) external onlyByOwner { for (uint256 i = 0; i < _tokens.length; i++) { - supportedTokens[_tokens[i]] = _supported; + address token = _tokens[i]; + supportedTokens[token] = _supported; + emit SupportedTokenChanged(token, _supported); } }
87-100: Fast-path and contract-code guard (same as Shutter)
- Optional: fast-path when tokenGate == NO_TOKEN_GATE.
- Recommended: require tokenGate.code.length > 0 when gating is active to avoid draw-time failures.
116-131: extraData parsing: avoid calldata→memory copySame suggestion as in Shutter file: provide a calldata overload for _extraDataToTokenInfo and call it from createDispute.
contracts/test/arbitration/helpers/dispute-kit-gated-common.ts (2)
76-85: whitelistTokens signer assumptionFunction relies on disputeKit being connected to the owner. If tests ever change signer context, pass the signer explicitly to avoid flakiness.
108-162: stakeAndDraw: disputeId hardcoded to 0This relies on fresh fixtures per test. If suites evolve to reuse state, derive the disputeId from the CreateDispute event or core state.
contracts/test/arbitration/helpers/dispute-kit-shutter-common.ts (3)
185-201: Remove the arbitrary 10‑vote cap when collecting vote IDsA fixed cap can miss valid votes and cause flakiness. Stop when getVoteInfo reverts, but keep a reasonable safety cap.
Apply this diff:
- const voteIDs: bigint[] = []; - const maxVotes = 10; // Reasonable limit for testing - - for (let i = 0; i < maxVotes; i++) { + const voteIDs: bigint[] = []; + const SAFETY_CAP = 64; // upper bound to avoid infinite loops + for (let i = 0; i < SAFETY_CAP; i++) { try { const voteInfo = await context.disputeKit.getVoteInfo(disputeId, roundIndex, i); if (voteInfo[0] === juror.address) { // account is at index 0 voteIDs.push(BigInt(i)); } } catch { // No more votes break; } }
53-56: Narrow the config contractName to a string unionPrevents typos and improves DX.
Apply this diff:
-export interface ShutterTestConfig { - contractName: string; // "DisputeKitShutter" or "DisputeKitGatedShutter" +export interface ShutterTestConfig { + contractName: "DisputeKitShutter" | "DisputeKitGatedShutter"; isGated?: boolean; // Whether to setup token gating }
139-141: Optional: use Hardhat network-helpers for time/mineSwitch to @nomicfoundation/hardhat-network-helpers (time.increase, mine) for readability and consistency with Hardhat v3 runtime.
Based on learnings
Also applies to: 165-167
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
contracts/src/arbitration/dispute-kits/DisputeKitGated.sol(6 hunks)contracts/src/arbitration/dispute-kits/DisputeKitGatedShutter.sol(6 hunks)contracts/src/test/DisputeKitGatedShutterMock.sol(1 hunks)contracts/test/arbitration/dispute-kit-gated-shutter.ts(1 hunks)contracts/test/arbitration/dispute-kit-gated.ts(1 hunks)contracts/test/arbitration/dispute-kit-shutter.ts(1 hunks)contracts/test/arbitration/helpers/dispute-kit-gated-common.ts(1 hunks)contracts/test/arbitration/helpers/dispute-kit-shutter-common.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (5)
contracts/test/arbitration/dispute-kit-shutter.ts (1)
contracts/test/arbitration/helpers/dispute-kit-shutter-common.ts (7)
ShutterTestContext(25-50)setupShutterTest(206-325)testCommitPhase(329-443)testNormalFlowBotReveals(445-582)testRecoveryFlowJurorReveals(584-730)testHashFunctionBehavior(732-772)testEdgeCasesAndSecurity(774-875)
contracts/test/arbitration/dispute-kit-gated.ts (1)
contracts/test/arbitration/helpers/dispute-kit-gated-common.ts (9)
TokenGatedTestContext(27-46)setupTokenGatedTest(165-236)testTokenWhitelistManagement(240-305)testAccessControl(307-323)testUnsupportedTokenErrors(325-368)testERC20Gating(370-431)testERC721Gating(433-494)testERC1155Gating(496-557)testWhitelistIntegration(559-625)
contracts/test/arbitration/dispute-kit-gated-shutter.ts (2)
contracts/test/arbitration/helpers/dispute-kit-gated-common.ts (9)
TokenGatedTestContext(27-46)setupTokenGatedTest(165-236)testTokenWhitelistManagement(240-305)testAccessControl(307-323)testUnsupportedTokenErrors(325-368)testERC20Gating(370-431)testERC721Gating(433-494)testERC1155Gating(496-557)testWhitelistIntegration(559-625)contracts/test/arbitration/helpers/dispute-kit-shutter-common.ts (7)
ShutterTestContext(25-50)setupShutterTest(206-325)testCommitPhase(329-443)testNormalFlowBotReveals(445-582)testRecoveryFlowJurorReveals(584-730)testHashFunctionBehavior(732-772)testEdgeCasesAndSecurity(774-875)
contracts/test/arbitration/helpers/dispute-kit-shutter-common.ts (2)
contracts/test/arbitration/helpers/dispute-kit-gated-common.ts (1)
encodeExtraData(59-74)contracts/deploy/utils/deployUpgradable.ts (1)
deployUpgradable(37-89)
contracts/test/arbitration/helpers/dispute-kit-gated-common.ts (4)
contracts/deploy/utils/index.ts (1)
PNK(40-40)contracts/test/arbitration/helpers/dispute-kit-shutter-common.ts (1)
encodeExtraData(71-72)contracts/deploy/utils/deployUpgradable.ts (1)
deployUpgradable(37-89)contracts/deploy/utils/deployTokens.ts (2)
deployERC721(55-67)deployERC1155(69-81)
⏰ 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-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: Analyze (javascript)
- GitHub Check: hardhat-tests
🔇 Additional comments (7)
contracts/src/arbitration/dispute-kits/DisputeKitGated.sol (1)
146-155: Post-draw check assumes prior whitelist validationOK to skip supportedTokens check here. If you plan to allow dynamic whitelist changes mid-dispute, consider caching tokenGate per dispute at creation to avoid state drift. Otherwise current approach is fine.
contracts/src/test/DisputeKitGatedShutterMock.sol (1)
10-14: Mock wrapper is cleanPublic wrapper correctly exposes parsing for tests; no concerns.
contracts/test/arbitration/dispute-kit-gated-shutter.ts (1)
36-72: Shared test harness usage LGTMGood split between token-gating and Shutter suites with contexts and mocks. No issues spotted.
contracts/test/arbitration/dispute-kit-shutter.ts (1)
12-38: Refactor to shared helpers LGTMCleaner suite via shared helpers; maintains coverage and readability.
contracts/test/arbitration/dispute-kit-gated.ts (1)
14-41: Adoption of shared token-gated helpers LGTMContext-driven tests cover whitelist, access control, and gating paths comprehensively.
contracts/test/arbitration/helpers/dispute-kit-gated-common.ts (1)
58-74: Packed encoding matches on-chain parsingThe uint88 + bool + address packing aligns with assembly extraction (addr in low 160 bits, bool at bit 160). Good.
contracts/test/arbitration/helpers/dispute-kit-shutter-common.ts (1)
343-349: Verify/align test event args with the dispute‑kit ABI (CommitCastShutter / VoteCast)
- IDisputeKit.VoteCast matches the test shape: (uint256 indexed coreDisputeID, address indexed juror, uint256[] voteIDs, uint256 indexed choice, string justification).
- CommitCastShutter is inconsistent across implementations/ABIs in this repo: some variants include a recoveryCommit bytes32 (6 args), others do not (5 args). The test currently expects the 6‑arg form (commit, recoveryCommit, identity, encryptedVote).
- Action: ensure the test deploys/uses the dispute‑kit implementation whose ABI includes recoveryCommit, or update the test to expect the actual emitted event arguments (remove recoveryCommit from withArgs or assert it separately if present).
| const arbitrationCost = await context.core["arbitrationCost(bytes)"](extraData); | ||
|
|
||
| // Create dispute via core contract | ||
| await context.core["createDispute(uint256,bytes)"](2, extraData, { value: arbitrationCost }).then((tx) => tx.wait()); | ||
| const disputeId = 0; | ||
|
|
||
| await network.provider.send("evm_increaseTime", [2000]); // Wait for minStakingTime |
There was a problem hiding this comment.
Do not hard‑code disputeId=0; parse it from the DisputeCreation event
Hard‑coding to 0 makes tests brittle and wrong once multiple disputes exist. Parse the emitted event to get the actual ID.
Apply this diff:
const arbitrationCost = await context.core["arbitrationCost(bytes)"](extraData);
// Create dispute via core contract
- await context.core["createDispute(uint256,bytes)"](2, extraData, { value: arbitrationCost }).then((tx) => tx.wait());
- const disputeId = 0;
+ const tx = await context.core["createDispute(uint256,bytes)"](2, extraData, { value: arbitrationCost });
+ const receipt = await tx.wait();
+ // Parse DisputeCreation(disputeID, ...) to obtain the ID
+ let disputeId: number | undefined;
+ for (const log of receipt.logs) {
+ try {
+ const parsed = context.core.interface.parseLog(log);
+ if (parsed.name === "DisputeCreation") {
+ disputeId = Number(parsed.args[0]);
+ break;
+ }
+ } catch {}
+ }
+ if (disputeId === undefined) throw new Error("DisputeCreation event not found");📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const arbitrationCost = await context.core["arbitrationCost(bytes)"](extraData); | |
| // Create dispute via core contract | |
| await context.core["createDispute(uint256,bytes)"](2, extraData, { value: arbitrationCost }).then((tx) => tx.wait()); | |
| const disputeId = 0; | |
| await network.provider.send("evm_increaseTime", [2000]); // Wait for minStakingTime | |
| const arbitrationCost = await context.core["arbitrationCost(bytes)"](extraData); | |
| // Create dispute via core contract | |
| const tx = await context.core["createDispute(uint256,bytes)"](2, extraData, { value: arbitrationCost }); | |
| const receipt = await tx.wait(); | |
| // Parse DisputeCreation(disputeID, ...) to obtain the ID | |
| let disputeId: number | undefined; | |
| for (const log of receipt.logs) { | |
| try { | |
| const parsed = context.core.interface.parseLog(log); | |
| if (parsed.name === "DisputeCreation") { | |
| disputeId = Number(parsed.args[0]); | |
| break; | |
| } | |
| } catch {} | |
| } | |
| if (disputeId === undefined) throw new Error("DisputeCreation event not found"); | |
| await network.provider.send("evm_increaseTime", [2000]); // Wait for minStakingTime |
🤖 Prompt for AI Agents
In contracts/test/arbitration/helpers/dispute-kit-shutter-common.ts around lines
133 to 139, the code hard‑codes disputeId = 0 after creating the dispute;
instead capture the transaction receipt from the awaited createDispute call,
find the DisputeCreation event in receipt.events, and extract the emitted
dispute id (typically via event.args.disputeID or the appropriate indexed arg
name/position). Replace the hard‑coded assignment with parsing logic that sets
disputeId from that event so tests use the actual created dispute id.
| export const advanceToVotePeriod = async (context: ShutterTestContext, disputeId: number) => { | ||
| // Advance from commit to vote period | ||
| const timesPerPeriod = [300, 300, 300, 300]; // Default times from deployment | ||
| const commitPeriod = timesPerPeriod[Period.commit]; | ||
|
|
||
| await network.provider.send("evm_increaseTime", [Number(commitPeriod)]); | ||
| await network.provider.send("evm_mine"); | ||
|
|
||
| await context.core.passPeriod(disputeId).then((tx) => tx.wait()); | ||
|
|
There was a problem hiding this comment.
Advance time with a safety buffer; avoid hard‑coding period times
Increasing time by exactly the commit period can be borderline if the contract uses strict > comparisons. Add +1s. Also, consider reading the period durations from the court instead of duplicating [300,300,300,300].
Apply this diff for the buffer:
- const timesPerPeriod = [300, 300, 300, 300]; // Default times from deployment
+ const timesPerPeriod = [300, 300, 300, 300]; // Keep in sync with court config
const commitPeriod = timesPerPeriod[Period.commit];
- await network.provider.send("evm_increaseTime", [Number(commitPeriod)]);
+ await network.provider.send("evm_increaseTime", [Number(commitPeriod) + 1]);
await network.provider.send("evm_mine");And consider fetching the actual configured times from Core (e.g., via courts[shutterCourtID].timesPerPeriod or an accessor) to prevent drift.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| export const advanceToVotePeriod = async (context: ShutterTestContext, disputeId: number) => { | |
| // Advance from commit to vote period | |
| const timesPerPeriod = [300, 300, 300, 300]; // Default times from deployment | |
| const commitPeriod = timesPerPeriod[Period.commit]; | |
| await network.provider.send("evm_increaseTime", [Number(commitPeriod)]); | |
| await network.provider.send("evm_mine"); | |
| await context.core.passPeriod(disputeId).then((tx) => tx.wait()); | |
| export const advanceToVotePeriod = async (context: ShutterTestContext, disputeId: number) => { | |
| // Advance from commit to vote period | |
| const timesPerPeriod = [300, 300, 300, 300]; // Keep in sync with court config | |
| const commitPeriod = timesPerPeriod[Period.commit]; | |
| await network.provider.send("evm_increaseTime", [Number(commitPeriod) + 1]); | |
| await network.provider.send("evm_mine"); | |
| await context.core.passPeriod(disputeId).then((tx) => tx.wait()); |
🤖 Prompt for AI Agents
In contracts/test/arbitration/helpers/dispute-kit-shutter-common.ts around lines
160 to 169, the test advances time by the exact commit period and uses a
hard‑coded timesPerPeriod array; change the evm_increaseTime call to add a 1s
safety buffer (commitPeriod + 1) to avoid strict > boundary issues, and replace
the fixed timesPerPeriod with a fetch from the deployed Core/Court (read the
configured timesPerPeriod via the appropriate Core/court accessor for the test
context) so the test uses the actual contract durations rather than duplicating
[300,300,300,300].
| describe("Hash Function Behavior", () => { | ||
| it("Should return different hashes for juror vs non-juror callers", async () => { | ||
| const ctx = context(); | ||
| const disputeId = await createDisputeAndDraw(ctx, ctx.shutterCourtID, 3, ctx.shutterDKID); | ||
| await advanceToCommitPeriod(ctx, disputeId); | ||
|
|
||
| const voteIDs = await getVoteIDsForJuror(ctx, disputeId, ctx.juror1); | ||
| const { fullCommit, recoveryCommit } = generateCommitments(ctx.choice, ctx.salt, ctx.justification); | ||
|
|
||
| await ctx.disputeKit | ||
| .connect(ctx.juror1) | ||
| .castCommitShutter(disputeId, voteIDs, fullCommit, recoveryCommit, ctx.identity, ctx.encryptedVote); | ||
|
|
||
| await advanceToVotePeriod(ctx, disputeId); | ||
|
|
||
| // During castVoteShutter, the contract should use different hash logic | ||
| // For juror: hash(choice, salt) | ||
| // For non-juror: hash(choice, salt, justificationHash) | ||
|
|
||
| // This is tested implicitly by the recovery flow tests above | ||
| // The juror can reveal with any justification, while non-juror must provide exact justification | ||
| }); | ||
|
|
There was a problem hiding this comment.
Test has no assertions; either add checks or skip
This describes behavior but asserts nothing, so it can pass even if behavior changes.
Apply this diff to mark it as pending until assertions are added:
- it("Should return different hashes for juror vs non-juror callers", async () => {
+ it.skip("Should return different hashes for juror vs non-juror callers (covered implicitly)", async () => {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| describe("Hash Function Behavior", () => { | |
| it("Should return different hashes for juror vs non-juror callers", async () => { | |
| const ctx = context(); | |
| const disputeId = await createDisputeAndDraw(ctx, ctx.shutterCourtID, 3, ctx.shutterDKID); | |
| await advanceToCommitPeriod(ctx, disputeId); | |
| const voteIDs = await getVoteIDsForJuror(ctx, disputeId, ctx.juror1); | |
| const { fullCommit, recoveryCommit } = generateCommitments(ctx.choice, ctx.salt, ctx.justification); | |
| await ctx.disputeKit | |
| .connect(ctx.juror1) | |
| .castCommitShutter(disputeId, voteIDs, fullCommit, recoveryCommit, ctx.identity, ctx.encryptedVote); | |
| await advanceToVotePeriod(ctx, disputeId); | |
| // During castVoteShutter, the contract should use different hash logic | |
| // For juror: hash(choice, salt) | |
| // For non-juror: hash(choice, salt, justificationHash) | |
| // This is tested implicitly by the recovery flow tests above | |
| // The juror can reveal with any justification, while non-juror must provide exact justification | |
| }); | |
| describe("Hash Function Behavior", () => { | |
| it.skip("Should return different hashes for juror vs non-juror callers (covered implicitly)", async () => { | |
| const ctx = context(); | |
| const disputeId = await createDisputeAndDraw(ctx, ctx.shutterCourtID, 3, ctx.shutterDKID); | |
| await advanceToCommitPeriod(ctx, disputeId); | |
| const voteIDs = await getVoteIDsForJuror(ctx, disputeId, ctx.juror1); | |
| const { fullCommit, recoveryCommit } = generateCommitments(ctx.choice, ctx.salt, ctx.justification); | |
| await ctx.disputeKit | |
| .connect(ctx.juror1) | |
| .castCommitShutter(disputeId, voteIDs, fullCommit, recoveryCommit, ctx.identity, ctx.encryptedVote); | |
| await advanceToVotePeriod(ctx, disputeId); | |
| // During castVoteShutter, the contract should use different hash logic | |
| // For juror: hash(choice, salt) | |
| // For non-juror: hash(choice, salt, justificationHash) | |
| // This is tested implicitly by the recovery flow tests above | |
| // The juror can reveal with any justification, while non-juror must provide exact justification | |
| }); |
🤖 Prompt for AI Agents
In contracts/test/arbitration/helpers/dispute-kit-shutter-common.ts around lines
733 to 755, the "Should return different hashes for juror vs non-juror callers"
test contains no assertions so it always passes; either add explicit assertions
to verify juror vs non-juror hash behavior (e.g., call the contract/hash helper
for both roles and assert outputs differ and that recovery permits juror with
any justification but requires exact justification for non-juror), or mark the
test as pending/skipped (use it.skip or add a comment + return) until concrete
assertions are implemented; update the test accordingly so CI fails when
behavior is incorrect.
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
contracts/test/arbitration/helpers/dispute-kit-gated-common.ts (1)
386-389: Improve type safety for log filtering.The log filtering uses
anytype and optional chaining (log.fragment?.name). While functional, this reduces type safety and could hide issues.Consider adding a type guard or helper function:
// At the top of the file, add a helper for type-safe log filtering const isDrawLog = (log: any): log is { fragment: { name: string }, address: string, args: any[] } => { return log.fragment?.name === "Draw"; }; // Then use it in tests: const drawLogs = tx?.logs.filter((log) => isDrawLog(log) && log.address === ctx.core.target) || [];This pattern is repeated throughout the test suites (lines 407, 449, 470, 512, 533, 581, 655, 684), so a shared helper would improve consistency and type safety.
contracts/test/arbitration/dispute-kit-gated.ts (1)
14-16: Combine ESLint disable comments.The two separate ESLint disable comments can be combined into a single comment for consistency.
/* eslint-disable no-unused-vars */ -/* eslint-disable no-unused-expressions */ - +/* eslint-disable no-unused-expressions */ // https://github.com/standard/standard/issues/690#issuecomment-278533482 +
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
contracts/test/arbitration/dispute-kit-gated-shutter.ts(1 hunks)contracts/test/arbitration/dispute-kit-gated.ts(1 hunks)contracts/test/arbitration/helpers/dispute-kit-gated-common.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- contracts/test/arbitration/dispute-kit-gated-shutter.ts
🧰 Additional context used
🧬 Code graph analysis (2)
contracts/test/arbitration/dispute-kit-gated.ts (1)
contracts/test/arbitration/helpers/dispute-kit-gated-common.ts (10)
TokenGatedTestContext(27-46)setupTokenGatedTest(165-236)testTokenWhitelistManagement(240-305)testAccessControl(307-323)testUnsupportedTokenErrors(325-368)testERC20Gating(370-431)testERC721Gating(433-494)testERC1155Gating(496-557)testWhitelistIntegration(559-625)testNoTokenGateAddress(627-704)
contracts/test/arbitration/helpers/dispute-kit-gated-common.ts (4)
contracts/deploy/utils/index.ts (1)
PNK(40-40)contracts/test/arbitration/helpers/dispute-kit-shutter-common.ts (1)
encodeExtraData(71-72)contracts/deploy/utils/deployUpgradable.ts (1)
deployUpgradable(37-89)contracts/deploy/utils/deployTokens.ts (2)
deployERC721(55-67)deployERC1155(69-81)
⏰ 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 (5)
contracts/test/arbitration/helpers/dispute-kit-gated-common.ts (4)
1-51: LGTM: Well-structured type definitions.The type definitions, interfaces, and imports are well-organized and comprehensive. The
TokenGatedTestContextinterface captures all necessary test state, and theDisputeKitGatedTypeunion type correctly handles both gated variants.
108-162: Note the test limitation regarding EOA-created disputes.The comment on line 150 correctly notes that the dispute "cannot be executed, in reality it should be created by an arbitrable contract, not an EOA." This is an acceptable test limitation, but it means these tests don't fully validate the end-to-end dispute execution flow.
Additionally, the hardcoded gas limits (lines 125, 130, 161) might be fragile across different EVM versions or network conditions. Consider using automatic gas estimation or documenting why specific limits are needed.
238-704: Comprehensive test coverage for token gating.The test suites provide excellent coverage of token gating functionality:
- Whitelist management (add/remove single/multiple tokens, duplicates)
- Access control (owner-only operations)
- Error handling for unsupported tokens
- Token gating for ERC20, ERC721, and ERC1155
- Whitelist state management across operations
- Edge case handling for address(0) as "no token gate"
The tests are well-structured, clearly documented, and cover both positive and negative scenarios.
59-74: Token packing format matches Solidity unpacking. Verified thatencodeExtraData’s solidityPacked(["uint88","bool","address"]) aligns with_extraDataToTokenInfo’s assembly logic in both DisputeKitGated.sol and DisputeKitGatedShutter.sol.contracts/test/arbitration/dispute-kit-gated.ts (1)
1-43: Excellent refactoring to shared test helpers.This test file demonstrates good software engineering practices:
DRY principle: Test logic is centralized in the common helper file, eliminating duplication between DisputeKitGated and DisputeKitGatedShutter tests.
Clear structure: The test file is easy to understand—it sets up context once and delegates to focused test suite functions.
Maintainability: Changes to test logic only need to be made in one place (the common helper), while contract-specific behavior can still be tested separately if needed.
Documentation: The JSDoc comment clearly explains what the test suite covers.
The context factory pattern (
() => context) is used correctly to ensure each test gets fresh context via thebeforeEachhook.
| const hre = require("hardhat"); | ||
| await deployERC721(hre, deployer, "TestERC721", "Nft721"); | ||
| const nft721 = await ethers.getContract<TestERC721>("Nft721"); | ||
|
|
||
| await deployERC1155(hre, deployer, "TestERC1155", "Nft1155"); | ||
| const nft1155 = await ethers.getContract<TestERC1155>("Nft1155"); |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Replace dynamic require with top-level import.
Using require("hardhat") at runtime (line 202) can cause module resolution issues and loses type safety. The hre object should be imported at the top of the file or passed as a parameter.
Apply this diff to import at the top:
-import { deployments, ethers, getNamedAccounts, network } from "hardhat";
+import { deployments, ethers, getNamedAccounts, network } from "hardhat";
+import type { HardhatRuntimeEnvironment } from "hardhat/types";Then update the function signature and usage:
-export async function setupTokenGatedTest(config: TokenGatedTestConfig): Promise<TokenGatedTestContext> {
+export async function setupTokenGatedTest(
+ config: TokenGatedTestConfig,
+ hre: HardhatRuntimeEnvironment
+): Promise<TokenGatedTestContext> {
const { deployer } = await getNamedAccounts();
const [, juror1, juror2] = await ethers.getSigners();
// ... existing code ...
- const hre = require("hardhat");
await deployERC721(hre, deployer, "TestERC721", "Nft721");Note: This would require updating the call sites in the test files to pass hre.
🤖 Prompt for AI Agents
In contracts/test/arbitration/helpers/dispute-kit-gated-common.ts around lines
202 to 207, replace the runtime require('hardhat') with a top-level import of
hre or accept hre as a function parameter: update the file to import { hre } (or
import HardhatRuntimeEnvironment as hre) at the top or modify the helper
function signature to take hre, then remove the inline require and use the
injected/imported hre variable when calling deployERC721/deployERC1155; finally
adjust all test call sites to pass the hre argument where the helper is invoked.
| // Whitelist all tokens by default to make existing tests work | ||
| await whitelistTokens(context, [dai.target, nft721.target, nft1155.target], true); | ||
|
|
There was a problem hiding this comment.
Clarify the comment about "existing tests".
The comment on line 232 states "Whitelist all tokens by default to make existing tests work," but this is a new file being introduced in this PR. This comment is confusing.
Either:
- Update the comment to explain why default whitelisting is beneficial for test setup, or
- Remove the default whitelisting if it's not actually needed for these tests
Suggested revision:
- // Whitelist all tokens by default to make existing tests work
+ // Whitelist all tokens by default to simplify test setup (tests can explicitly remove tokens as needed)
await whitelistTokens(context, [dai.target, nft721.target, nft1155.target], true);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // Whitelist all tokens by default to make existing tests work | |
| await whitelistTokens(context, [dai.target, nft721.target, nft1155.target], true); | |
| // Whitelist all tokens by default to simplify test setup (tests can explicitly remove tokens as needed) | |
| await whitelistTokens(context, [dai.target, nft721.target, nft1155.target], true); |
🤖 Prompt for AI Agents
In contracts/test/arbitration/helpers/dispute-kit-gated-common.ts around lines
232-234, the comment "Whitelist all tokens by default to make existing tests
work" is confusing for a new file; either replace the comment with a clear
explanation of why default whitelisting is required for the test setup (e.g.,
which tests rely on tokens being whitelisted or that it prevents permission
errors during fixture setup) or remove the default whitelist call and the
comment if none of the tests here actually require whitelisted tokens; update
code accordingly and run the test suite to confirm behavior.
| it("Should draw only the jurors who have some DAI balance", async () => { | ||
| const ctx = context(); | ||
| ctx.dai.transfer(ctx.juror1.address, 1); | ||
|
|
||
| const nbOfJurors = 15n; | ||
| const tx = await stakeAndDraw( | ||
| ctx, | ||
| Courts.GENERAL, | ||
| nbOfJurors, | ||
| ctx.gatedDisputeKitID, | ||
| ctx.dai.target, | ||
| false, | ||
| 0 | ||
| ).then((tx) => tx.wait()); |
There was a problem hiding this comment.
Critical: Missing await on token transfer.
Line 393 is missing await on the ctx.dai.transfer() call. This means the transfer may not complete before stakeAndDraw is called, causing the test to fail or produce incorrect results.
Apply this diff:
it("Should draw only the jurors who have some DAI balance", async () => {
const ctx = context();
- ctx.dai.transfer(ctx.juror1.address, 1);
+ await ctx.dai.transfer(ctx.juror1.address, 1);
const nbOfJurors = 15n;Note: Check for similar missing await statements throughout the file. I found additional instances at:
- Line 456:
await ctx.nft721.safeMint(ctx.juror2.address);(this one has await, good) - Line 519:
await ctx.nft1155.mint(ctx.juror2.address, ctx.TOKEN_ID, 1, "0x00");(this one has await, good) - Line 567:
await ctx.dai.transfer(ctx.juror1.address, 1);(this one has await, good) - Line 589:
await ctx.dai.transfer(ctx.juror1.address, 1);(this one has await, good)
So only line 393 is missing the await.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| it("Should draw only the jurors who have some DAI balance", async () => { | |
| const ctx = context(); | |
| ctx.dai.transfer(ctx.juror1.address, 1); | |
| const nbOfJurors = 15n; | |
| const tx = await stakeAndDraw( | |
| ctx, | |
| Courts.GENERAL, | |
| nbOfJurors, | |
| ctx.gatedDisputeKitID, | |
| ctx.dai.target, | |
| false, | |
| 0 | |
| ).then((tx) => tx.wait()); | |
| it("Should draw only the jurors who have some DAI balance", async () => { | |
| const ctx = context(); | |
| await ctx.dai.transfer(ctx.juror1.address, 1); | |
| const nbOfJurors = 15n; | |
| const tx = await stakeAndDraw( | |
| ctx, | |
| Courts.GENERAL, | |
| nbOfJurors, | |
| ctx.gatedDisputeKitID, | |
| ctx.dai.target, | |
| false, | |
| 0 | |
| ).then((tx) => tx.wait()); | |
| // …rest of test… |
🤖 Prompt for AI Agents
In contracts/test/arbitration/helpers/dispute-kit-gated-common.ts around lines
391 to 404, the call to ctx.dai.transfer(ctx.juror1.address, 1) (line 393) is
missing an await which can cause the transfer to not complete before
stakeAndDraw runs; add await before ctx.dai.transfer(...) so the test waits for
the transfer to finish, and scan the file for any other token transfer/mint
calls missing await (lines noted in the review are already awaited).
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (7)
contracts/test/arbitration/dispute-kit-gated-shutter.ts (1)
37-39: Nit: avoid async on describe callbacks.Mocha ignores async return on describe; keep async on hooks/tests only.
contracts/src/arbitration/dispute-kits/DisputeKitGated.sol (3)
74-81: Emit events for whitelist changes.Governance actions should be observable. Add an event and emit per updated token.
Apply:
+ /// @notice Emitted when supported token status changes. + event SupportedTokenChanged(address indexed token, bool supported); + function changeSupportedTokens(address[] memory _tokens, bool _supported) external onlyByOwner { for (uint256 i = 0; i < _tokens.length; i++) { supportedTokens[_tokens[i]] = _supported; + emit SupportedTokenChanged(_tokens[i], _supported); } }
87-101: Consider enforcing access control first (minor).Currently the whitelist check runs before super.createDispute’s onlyByCore. Calling super first would fail fast for unauthorized callers. Not critical since the pre-check is cheap and data is public anyway.
149-154: Harden balance checks to avoid revert on bad whitelist entries.If a non-contract or non-compliant token is whitelisted, interface calls may revert and block draws. Prefer safe staticcall; treat failures as zero balance.
Example replacement (helper functions to add elsewhere in the contract):
function _hasERC20or721Balance(address token, address owner) internal view returns (bool) { (bool ok, bytes memory ret) = token.staticcall(abi.encodeWithSignature("balanceOf(address)", owner)); if (!ok || ret.length < 32) return false; return abi.decode(ret, (uint256)) > 0; } function _hasERC1155Balance(address token, address owner, uint256 id) internal view returns (bool) { (bool ok, bytes memory ret) = token.staticcall(abi.encodeWithSignature("balanceOf(address,uint256)", owner, id)); if (!ok || ret.length < 32) return false; return abi.decode(ret, (uint256)) > 0; }Then within this block:
- if (isERC1155) { - return IBalanceHolderERC1155(tokenGate).balanceOf(_juror, tokenId) > 0; - } else { - return IBalanceHolder(tokenGate).balanceOf(_juror) > 0; - } + return isERC1155 + ? _hasERC1155Balance(tokenGate, _juror, tokenId) + : _hasERC20or721Balance(tokenGate, _juror);contracts/src/arbitration/dispute-kits/DisputeKitGatedShutter.sol (2)
39-41: Emit events when whitelist changes for auditabilityMapping flips are silent. Emit an event per token to ease monitoring and off-chain indexing, and to support revert-free idempotent updates.
Example:
+ event SupportedTokenChanged(address indexed token, bool supported); function changeSupportedTokens(address[] memory _tokens, bool _supported) external onlyByOwner { for (uint256 i = 0; i < _tokens.length; i++) { supportedTokens[_tokens[i]] = _supported; + emit SupportedTokenChanged(_tokens[i], _supported); } }Also applies to: 103-111
116-129: Access control check runs after gating logicYou rely on super.createDispute for onlyByCore. Minor: consider moving access-control earlier (e.g., replicate the modifier or a lightweight check) to fail fast on unauthorized callers, then run gating. Not critical.
Confirm base enforces onlyByCore and no state is mutated before the revert in this override.
contracts/test/arbitration/helpers/dispute-kit-gated-common.ts (1)
76-85: Normalize AddressLike instead of toString()Using toString() on Addressable can yield non-address strings. Prefer AddressLike and ethers.getAddress for safety.
-export const whitelistTokens = async ( - context: TokenGatedTestContext, - tokens: (string | Addressable)[], - supported: boolean = true -) => { - const tokenAddresses = tokens.map((token) => (typeof token === "string" ? token : token.toString())); +export const whitelistTokens = async ( + context: TokenGatedTestContext, + tokens: readonly ethers.AddressLike[], + supported: boolean = true +) => { + const tokenAddresses = tokens.map((t) => ethers.getAddress(t as string)); return context.disputeKit.changeSupportedTokens(tokenAddresses, supported); };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
contracts/src/arbitration/dispute-kits/DisputeKitGated.sol(6 hunks)contracts/src/arbitration/dispute-kits/DisputeKitGatedShutter.sol(6 hunks)contracts/src/test/DisputeKitGatedShutterMock.sol(1 hunks)contracts/test/arbitration/dispute-kit-gated-shutter.ts(1 hunks)contracts/test/arbitration/dispute-kit-gated.ts(1 hunks)contracts/test/arbitration/dispute-kit-shutter.ts(1 hunks)contracts/test/arbitration/helpers/dispute-kit-gated-common.ts(1 hunks)contracts/test/arbitration/helpers/dispute-kit-shutter-common.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (5)
contracts/test/arbitration/dispute-kit-gated-shutter.ts (2)
contracts/test/arbitration/helpers/dispute-kit-gated-common.ts (2)
TokenGatedTestContext(27-46)setupTokenGatedTest(165-236)contracts/test/arbitration/helpers/dispute-kit-shutter-common.ts (2)
ShutterTestContext(25-50)setupShutterTest(206-325)
contracts/test/arbitration/dispute-kit-gated.ts (1)
contracts/test/arbitration/helpers/dispute-kit-gated-common.ts (10)
TokenGatedTestContext(27-46)setupTokenGatedTest(165-236)testTokenWhitelistManagement(240-305)testAccessControl(307-323)testUnsupportedTokenErrors(325-368)testERC20Gating(370-431)testERC721Gating(433-494)testERC1155Gating(496-557)testWhitelistIntegration(559-625)testNoTokenGateAddress(627-704)
contracts/test/arbitration/helpers/dispute-kit-shutter-common.ts (2)
contracts/test/arbitration/helpers/dispute-kit-gated-common.ts (1)
encodeExtraData(59-74)contracts/deploy/utils/deployUpgradable.ts (1)
deployUpgradable(37-89)
contracts/test/arbitration/helpers/dispute-kit-gated-common.ts (4)
contracts/deploy/utils/index.ts (1)
PNK(40-40)contracts/test/arbitration/helpers/dispute-kit-shutter-common.ts (1)
encodeExtraData(71-72)contracts/deploy/utils/deployUpgradable.ts (1)
deployUpgradable(37-89)contracts/deploy/utils/deployTokens.ts (2)
deployERC721(55-67)deployERC1155(69-81)
contracts/test/arbitration/dispute-kit-shutter.ts (1)
contracts/test/arbitration/helpers/dispute-kit-shutter-common.ts (7)
ShutterTestContext(25-50)setupShutterTest(206-325)testCommitPhase(329-443)testNormalFlowBotReveals(445-582)testRecoveryFlowJurorReveals(584-730)testHashFunctionBehavior(732-772)testEdgeCasesAndSecurity(774-875)
⏰ 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-neo
- GitHub Check: Header rules - kleros-v2-neo
- GitHub Check: Redirect rules - kleros-v2-testnet-devtools
- GitHub Check: Header rules - kleros-v2-testnet-devtools
- GitHub Check: Pages changed - kleros-v2-neo
- GitHub Check: Pages changed - kleros-v2-testnet-devtools
- GitHub Check: hardhat-tests
- GitHub Check: Analyze (javascript)
- 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
🔇 Additional comments (13)
contracts/src/test/DisputeKitGatedShutterMock.sol (1)
10-14: Good: test-only wrapper for internal decoder.Simple, pure passthrough is appropriate for tests.
contracts/test/arbitration/dispute-kit-shutter.ts (1)
32-37: Refactor to shared suites looks good.Centralizing commit/reveal tests via common helpers reduces duplication.
contracts/test/arbitration/dispute-kit-gated-shutter.ts (2)
45-53: Token-gating shared tests wired correctly.Good coverage across whitelist, access control, and gating variants.
67-73: Shutter shared tests wired correctly under gated context.Nice reuse of the same suites with isGated=true.
contracts/test/arbitration/dispute-kit-gated.ts (1)
34-42: Shared gated tests integration looks good.Concise orchestration; promotes reuse and clarity.
contracts/src/arbitration/dispute-kits/DisputeKitGated.sol (1)
116-131: Docs match decoding layout; no update needed.contracts/src/arbitration/dispute-kits/DisputeKitGatedShutter.sol (3)
240-255: Decoder offsets and bit unpacking look correctLength guard, offsets (0x80/0xA0 with bytes length slot accounted), and bit-160 flag extraction match the packed layout used in tests.
258-279: Gating check in _postDrawCheck is soundNO_TOKEN_GATE shortcut and ERC20/721 vs ERC1155 balance checks are correct. Dependent on createDispute-time whitelist only, which is fine.
If whitelist may change mid‑dispute, confirm the intended behavior is to gate juror eligibility independent of later whitelist flips.
90-91: Allowing address(0) by default is sensibleKeeps non‑gated disputes possible. Intentional and matches tests.
contracts/test/arbitration/helpers/dispute-kit-gated-common.ts (2)
59-75: Encoder matches on-chain decoderPacked [uint88,bool,address] into bytes32 and ABI-encodes at slot 4 with tokenId at slot 5; aligns with contract decoding.
232-236: Global default whitelist in setup is finePre-whitelisting test tokens keeps legacy tests green. Consider asserting ZeroAddress remains supported if you rely on non-gated flows.
Add a quick check here: expectTokenSupported(ctx, ethers.ZeroAddress, true);
contracts/test/arbitration/helpers/dispute-kit-shutter-common.ts (2)
70-73: Ungated encoder OKABI shape [uint256,uint256,uint256] matches on-chain expectations.
224-278: Setup path selection logic is consistentUsing the Mock for gated shutter and enabling hidden votes on General court is reasonable for tests.
Ensure Court IDs and DisputeKit IDs (1=Classic, 2=Shutter) match current Core deployment constants.
| export const stakeAndDraw = async ( | ||
| context: TokenGatedTestContext, | ||
| courtId: number, | ||
| minJurors: BigNumberish, | ||
| disputeKitId: number, | ||
| tokenGate: string | Addressable, | ||
| isERC1155: boolean, | ||
| tokenId: BigNumberish | ||
| ) => { | ||
| // Stake jurors | ||
| for (const juror of [context.juror1, context.juror2]) { | ||
| await context.pnk.transfer(juror.address, context.thousandPNK(10)).then((tx) => tx.wait()); | ||
| expect(await context.pnk.balanceOf(juror.address)).to.equal(context.thousandPNK(10)); | ||
|
|
||
| await context.pnk | ||
| .connect(juror) | ||
| .approve(context.core.target, context.thousandPNK(10), { gasLimit: 300000 }) | ||
| .then((tx) => tx.wait()); | ||
|
|
||
| await context.core | ||
| .connect(juror) | ||
| .setStake(Courts.GENERAL, context.thousandPNK(10), { gasLimit: 500000 }) | ||
| .then((tx) => tx.wait()); | ||
|
|
||
| expect(await context.sortitionModule.getJurorBalance(juror.address, 1)).to.deep.equal([ | ||
| context.thousandPNK(10), // totalStaked | ||
| 0, // totalLocked | ||
| context.thousandPNK(10), // stakedInCourt | ||
| 1, // nbOfCourts | ||
| ]); | ||
| } | ||
|
|
||
| const extraData = encodeExtraData(courtId, minJurors, disputeKitId, tokenGate, isERC1155, tokenId); | ||
|
|
||
| const tokenInfo = await context.disputeKit.extraDataToTokenInfo(extraData); | ||
| expect(tokenInfo[0]).to.equal(tokenGate); | ||
| expect(tokenInfo[1]).to.equal(isERC1155); | ||
| expect(tokenInfo[2]).to.equal(tokenId); | ||
|
|
||
| const arbitrationCost = await context.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 context.core["createDispute(uint256,bytes)"](2, extraData, { value: arbitrationCost }).then((tx) => | ||
| tx.wait() | ||
| ); | ||
| const disputeId = 0; | ||
|
|
||
| await network.provider.send("evm_increaseTime", [2000]); // Wait for minStakingTime | ||
| await network.provider.send("evm_mine"); | ||
| await context.sortitionModule.passPhase().then((tx) => tx.wait()); // Staking -> Generating | ||
|
|
||
| await context.sortitionModule.passPhase().then((tx) => tx.wait()); // Generating -> Drawing | ||
| return context.core.draw(disputeId, 70, { gasLimit: 10000000 }); | ||
| }; |
There was a problem hiding this comment.
🧩 Analysis chain
Bug: disputeId is hardcoded to 0; parse it from the receipt
Subsequent tests creating multiple disputes will draw against the wrong dispute. Parse DisputeCreation from the createDispute receipt.
Apply this diff:
- // Warning: this dispute cannot be executed, in reality it should be created by an arbitrable contract, not an EOA.
- const tx = await context.core["createDispute(uint256,bytes)"](2, extraData, { value: arbitrationCost }).then((tx) =>
- tx.wait()
- );
- const disputeId = 0;
+ // Warning: this dispute cannot be executed, in reality it should be created by an arbitrable contract, not an EOA.
+ const receipt = await context.core["createDispute(uint256,bytes)"](2, extraData, { value: arbitrationCost }).then(
+ (tx) => tx.wait()
+ );
+ const created = receipt?.logs.find(
+ (log: any) => log.fragment?.name === "DisputeCreation" && log.address === context.core.target
+ );
+ const disputeId = Number(created?.args[0]);🏁 Script executed:
#!/bin/bash
# Verify the exact event name emitted by KlerosCore on dispute creation.
rg -nP -C2 --type=sol 'event\s+Dispute.*' contracts | sed -n '1,200p'Length of output: 27
🏁 Script executed:
#!/bin/bash
# Search for DisputeCreation event definitions in Solidity contracts
rg -n 'event\s+DisputeCreation' -g '*.sol' -C3 contracts
rg -n 'DisputeCreation' -g '*.sol' -C3 contractsLength of output: 11814
Bug: Hardcoded disputeId; extract it from the DisputeCreation event
Subsequent tests creating multiple disputes will draw against the wrong dispute. Replace the hardcoded 0 with the actual ID emitted by createDispute:
- const receipt = await context.core["createDispute(uint256,bytes)"](2, extraData, { value: arbitrationCost }).then((tx) =>
- tx.wait()
- );
- const disputeId = 0;
+ const receipt = await context.core["createDispute(uint256,bytes)"](2, extraData, { value: arbitrationCost }).then((tx) =>
+ tx.wait()
+ );
+ const event = receipt.events?.find((e) => e.event === "DisputeCreation" && e.address === context.core.target);
+ const disputeId = Number(event?.args?._disputeID ?? event?.args?.[0]);Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In contracts/test/arbitration/helpers/dispute-kit-gated-common.ts around lines
109-162 the code uses a hardcoded disputeId = 0 after creating a dispute;
extract the actual disputeId from the receipt returned by createDispute instead.
After awaiting tx.wait() (the receipt), find the DisputeCreation event in
receipt.events and pull the dispute id from event.args (or event.args[0]),
convert it to a JS number or BigNumber as needed, and use that variable in the
subsequent draw call instead of the literal 0.
| export const createDisputeAndDraw = async ( | ||
| context: ShutterTestContext, | ||
| courtId: BigNumberish, | ||
| minJurors: BigNumberish, | ||
| disputeKitId: number | ||
| ) => { | ||
| // Stake jurors | ||
| for (const juror of [context.juror1, context.juror2]) { | ||
| await context.pnk.transfer(juror.address, context.thousandPNK(10)).then((tx) => tx.wait()); | ||
| expect(await context.pnk.balanceOf(juror.address)).to.equal(context.thousandPNK(10)); | ||
|
|
||
| await context.pnk | ||
| .connect(juror) | ||
| .approve(context.core.target, context.thousandPNK(10), { gasLimit: 300000 }) | ||
| .then((tx) => tx.wait()); | ||
|
|
||
| await context.core | ||
| .connect(juror) | ||
| .setStake(context.shutterCourtID, context.thousandPNK(10), { gasLimit: 500000 }) | ||
| .then((tx) => tx.wait()); | ||
|
|
||
| expect(await context.sortitionModule.getJurorBalance(juror.address, context.shutterCourtID)).to.deep.equal([ | ||
| context.thousandPNK(10), // totalStaked | ||
| 0, // totalLocked | ||
| context.thousandPNK(10), // stakedInCourt | ||
| 1, // nbOfCourts | ||
| ]); | ||
|
|
||
| // If gated, give tokens to jurors | ||
| if (context.isGated && context.dai) { | ||
| await context.dai.transfer(juror.address, 1); | ||
| } | ||
| } | ||
|
|
||
| // Use gated extra data if this is a gated dispute kit with DAI token | ||
| let extraData: string; | ||
| if (context.isGated && context.dai) { | ||
| extraData = encodeGatedExtraData(courtId, minJurors, disputeKitId, context.dai.target, false, 0); | ||
| } else { | ||
| extraData = encodeExtraData(courtId, minJurors, disputeKitId); | ||
| } | ||
|
|
||
| const arbitrationCost = await context.core["arbitrationCost(bytes)"](extraData); | ||
|
|
||
| // Create dispute via core contract | ||
| await context.core["createDispute(uint256,bytes)"](2, extraData, { value: arbitrationCost }).then((tx) => tx.wait()); | ||
| const disputeId = 0; | ||
|
|
||
| await network.provider.send("evm_increaseTime", [2000]); // Wait for minStakingTime | ||
| await network.provider.send("evm_mine"); | ||
| await context.sortitionModule.passPhase().then((tx) => tx.wait()); // Staking -> Generating | ||
|
|
||
| await context.sortitionModule.passPhase().then((tx) => tx.wait()); // Generating -> Drawing | ||
| await context.core.draw(disputeId, 70, { gasLimit: 10000000 }); | ||
|
|
||
| return disputeId; | ||
| }; |
There was a problem hiding this comment.
🧩 Analysis chain
Bug: disputeId is hardcoded to 0; parse it from the receipt
Same issue as gated helper; breaks when multiple disputes exist in one context.
Apply this diff:
- // Create dispute via core contract
- await context.core["createDispute(uint256,bytes)"](2, extraData, { value: arbitrationCost }).then((tx) => tx.wait());
- const disputeId = 0;
+ // Create dispute via core contract and fetch disputeId from the receipt
+ const receipt = await context.core["createDispute(uint256,bytes)"](2, extraData, { value: arbitrationCost }).then(
+ (tx) => tx.wait()
+ );
+ const created = receipt?.logs.find(
+ (log: any) => log.fragment?.name === "DisputeCreation" && log.address === context.core.target
+ );
+ const disputeId = Number(created?.args[0]);🏁 Script executed:
#!/bin/bash
# Confirm the event name and args order to decode disputeId reliably.
rg -nP -C3 --type=sol 'event\s+DisputeCreation\(|event\s+DisputeCreated\(' contracts/src | sed -n '1,200p'Length of output: 27
🏁 Script executed:
#!/bin/bash
# Search for DisputeCreation or DisputeCreated event in solidity contracts
rg -nP -C3 'event\s+(DisputeCreation|DisputeCreated)\b' -g '*.sol'Length of output: 2536
🏁 Script executed:
#!/bin/bash
# Locate createDisputeAndDraw in helpers and how disputeId is handled elsewhere
rg -nP -C3 'createDisputeAndDraw' -g 'contracts/test/arbitration/helpers/*.ts'
rg -nP -C3 'const\s+receipt' -g 'contracts/test/arbitration/helpers/*.ts'Length of output: 15828
Extract disputeId from event instead of hardcoded 0
The helper currently sets disputeId = 0, which breaks when more than one dispute exists. Parse it from the DisputeCreation event in the transaction receipt.
- // Create dispute via core contract
- await context.core["createDispute(uint256,bytes)"](2, extraData, { value: arbitrationCost }).then((tx) => tx.wait());
- const disputeId = 0;
+ // Create dispute via core contract and fetch disputeId from the receipt
+ const receipt = await context.core["createDispute(uint256,bytes)"](2, extraData, { value: arbitrationCost })
+ .then((tx) => tx.wait());
+ const createdLog = receipt.logs.find(
+ (log: any) =>
+ log.fragment?.name === "DisputeCreation" && log.address === context.core.target
+ );
+ const disputeId = Number(createdLog?.args[0]);Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In contracts/test/arbitration/helpers/dispute-kit-shutter-common.ts around lines
91 to 147, the helper currently hardcodes disputeId = 0; instead capture the
transaction returned by context.core["createDispute(uint256,bytes)"](...) await
its .wait() receipt, find the DisputeCreation event in receipt.events (or filter
by event name), extract the disputeId argument from the event (convert from
BigNumber if needed) and assign that value to disputeId; replace the hardcoded 0
with this extracted value so the helper works when multiple disputes exist.
2 participants
PR-Codex overview
This PR introduces enhancements to the
DisputeKitGatedandDisputeKitGatedShuttercontracts, including new functionality for token gating in dispute creation and improvements in testing frameworks for both kits.Detailed summary
createDisputevisibility fromexternaltopublicinDisputeKitClassicBase.changeSupportedTokensfunction to manage token support inDisputeKitGatedandDisputeKitGatedShutter._extraDataToTokenInfoto beinternalin both kits.TokenNotSupportederror for unsupported tokens.DisputeKitGatedMockandDisputeKitGatedShutterMockfor testing.DisputeKitGatedandDisputeKitGatedShutterto cover token gating and access control scenarios.dispute-kit-gated-common.tsfor setting up tests and managing token whitelisting.address(0)as a token gate, allowing disputes without token gating.Summary by CodeRabbit
New Features
Behavior Changes
Tests