Shutterized Dispute Kit #1965
Shutterized Dispute Kit #1965
Conversation
WalkthroughThis update adds the Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant ShutterScript (CLI)
participant Shutter API
participant Shutter SDK
User->>ShutterScript: encrypt <message>
ShutterScript->>Shutter API: fetchShutterData(decryptionTimestamp)
Shutter API-->>ShutterScript: {pubkey, params, identity}
ShutterScript->>Shutter SDK: encryptData(message, pubkey, params, sigma)
Shutter SDK-->>ShutterScript: encryptedCommitment
ShutterScript-->>User: {encryptedCommitment, identity}
User->>ShutterScript: decrypt <encryptedMessage> <identity>
ShutterScript->>Shutter API: fetchDecryptionKey(identity)
Shutter API-->>ShutterScript: decryptionKey
ShutterScript->>Shutter SDK: decrypt(encryptedMessage, decryptionKey)
Shutter SDK-->>ShutterScript: decryptedMessage
ShutterScript-->>User: decryptedMessage
sequenceDiagram
participant Core
participant DisputeKitClassic
participant DisputeKitShutter
participant Subgraph
Core->>DisputeKitClassic: createDispute(...)
Core->>DisputeKitShutter: createDispute(...)
DisputeKitClassic-->>Core: DisputeKitCreated(id=1)
DisputeKitShutter-->>Core: DisputeKitCreated(id=2)
Core-->>Subgraph: DisputeKitCreated events
Subgraph->>DisputeKitClassic: handle events (dynamic kit ID)
Subgraph->>DisputeKitShutter: handle events (dynamic kit ID)
Possibly related PRs
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ 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)
🪧 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 (
|
jaybuidl
added
Type: Feature🗿
Package: Web
❌ Deploy Preview for kleros-v2-university failed. Why did it fail? →
|
✅ 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-neo failed. Why did it fail? →
|
✅ Deploy Preview for kleros-v2-testnet-devtools ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (5)
contracts/src/arbitration/dispute-kits/DisputeKitClassicBase.sol (1)
439-453: Added virtual hashVote function for extensible vote hashingThis new
hashVotefunction is a key architectural change that enables the Shutter integration:
- It's declared as
virtualso derived contracts can override it- The base implementation hashes only
_choiceand_salt(backward compatible)- It accepts but doesn't use the
_justificationparameter in the base implementation- The comment explicitly indicates that derived contracts may use the unused parameters
This design enables the specialized Shutter implementation to include the justification in the hash calculation.
This is a well-designed extension point that maintains backward compatibility while enabling new functionality in derived contracts. The virtual function pattern allows for specialized implementations like DisputeKitShutter to include the justification in the hash while keeping the base functionality unchanged.
contracts/src/arbitration/dispute-kits/DisputeKitShutter.sol (2)
30-33: Unnecessary external self-call incurs gas & re-entrancy risk
this.castCommit(...)performs an external call to the same contract, paying
extra gas for ABI encoding/decoding and exposing the function to re-entrancy
checks (albeit low-risk here).Since
castCommitis declaredexternal, you can call it internally without
thisby using the fully-qualified function name:- this.castCommit(_coreDisputeID, _voteIDs, _commit); + castCommit(_coreDisputeID, _voteIDs, _commit);This is cheaper and avoids the external-call context switch.
85-92: Natspec parameter order does not match function signatureThe docstring lists the parameters as
(_choice, _justification, _salt)
while the signature is(_choice, _salt, _justification).
This can confuse automated doc generators.- * @param _justification The justification for the vote - * @param _salt A random salt for commitment + * @param _salt A random salt for commitment + * @param _justification The justification for the votecontracts/scripts/shutter.ts (2)
33-82: Missing retry / back-off on Shutter API call
fetchShutterDatafails the whole encryption flow on the first transient HTTP
error. Consider adding a simple exponential back-off or at least one retry to
improve UX when the Shutter API momentarily flakes.Not blocking, but worth considering.
160-186: Leaking raw Shutter keys in logs
encrypt()logs the eon key, identity and sigma to stdout. These are
cryptographically sensitive materials and should not be printed in production
mode.Recommend gating the
console.logstatements behind an environment variable
(e.g.DEBUG) or removing them entirely.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (7)
contracts/package.json(1 hunks)contracts/scripts/shutter.ts(1 hunks)contracts/src/arbitration/dispute-kits/DisputeKitClassic.sol(1 hunks)contracts/src/arbitration/dispute-kits/DisputeKitClassicBase.sol(3 hunks)contracts/src/arbitration/dispute-kits/DisputeKitGated.sol(1 hunks)contracts/src/arbitration/dispute-kits/DisputeKitShutter.sol(1 hunks)contracts/test/foundry/KlerosCore.t.sol(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (12)
- 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-university
- GitHub Check: Header rules - kleros-v2-university
- GitHub Check: Pages changed - kleros-v2-university
- GitHub Check: SonarCloud
- GitHub Check: Analyze (javascript)
- GitHub Check: contracts-testing
🔇 Additional comments (8)
contracts/src/arbitration/dispute-kits/DisputeKitGated.sol (1)
30-30: Version update from 0.8.0 to 0.9.0The version constant has been updated as part of the broader Shutter integration changes across dispute kit contracts.
contracts/package.json (1)
159-160:❓ Verification inconclusive
Added Shutter SDK and related dependencies
These new dependencies are required to support the Shutter Network integration for encrypted commitments:
@shutter-network/shutter-sdk- Core SDK for Shutter Network functionalityisomorphic-fetch- Cross-environment fetch implementationNote that the Shutter SDK is at version
0.0.1, which suggests it's in early development.
🏁 Script executed:
#!/bin/bash # Check for newer versions of the Shutter SDK and potential issues echo "Checking for latest version of @shutter-network/shutter-sdk" npm view @shutter-network/shutter-sdk versions echo "Checking for any reported issues or vulnerabilities" npm audit --json | grep -i shutterLength of output: 682
🏁 Script executed:
#!/bin/bash # Generate a lockfile to enable auditing npm install --package-lock-only echo "Re-checking for any reported issues or vulnerabilities after lockfile creation" npm audit --json | grep -i shutterLength of output: 796
Verify Shutter SDK and Fetch Dependency
The PR adds:
@shutter-network/shutter-sdk@ 0.0.1isomorphic-fetch@ ^3.0.0Because this project uses workspace-protocols, running
npm auditin CI failed—please manually:
• Confirm there are no known vulnerabilities in these packages (e.g. via a localnpm auditoryarn audit).
• Verify that 0.0.1 of the Shutter SDK is the intended stable release (bump when a newer stable version is available).contracts/src/arbitration/dispute-kits/DisputeKitClassicBase.sol (3)
241-241: Improved parameter documentationThe
_commitparameter documentation is updated to more clearly indicate it's a "commitment hash" rather than a general commit.
286-286: Added vote hash calculation for verificationThe code now uses the new
hashVotefunction to generate a vote hash, which enables customized hashing strategies in derived contracts.
292-293: Updated vote hash verification logic and error messageThe verification logic now compares against the computed
voteHashinstead of directly hashing the choice and salt. The error message has been updated accordingly.contracts/src/arbitration/dispute-kits/DisputeKitClassic.sol (1)
14-14: Version update from 0.8.0 to 0.9.0The version constant has been updated as part of the broader Shutter integration changes across dispute kit contracts.
contracts/src/arbitration/dispute-kits/DisputeKitShutter.sol (1)
56-72: Event is emitted even whencastCommitrevertsBecause
castCommitShutteremitsCommitCastShutterafter the external call,
a failure insidecastCommitwill revert the whole transaction and the event
will not be emitted.
If your intention was to guarantee the event only when the commit succeeds,
this is fine. If you need to record the attempt, move theemitin a
try/catchblock.contracts/scripts/shutter.ts (1)
88-127: Error parsing assumes specific JSON structure
fetchDecryptionKeythrows ifjsonResponse.messageis undefined, but the
Shutter API returns the payload atjsonResponseroot according to the spec
used earlier. Align the two helpers or add a type-guard to avoid false
positives.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
contracts/test/arbitration/index.ts (2)
65-65: Test function updated but consider expanding test coverageThe deployContracts function is properly updated to include DisputeKitShutter. However, while the deployment and registration of DisputeKitShutter are verified, there are no specific tests for its unique functionality related to Shutter integration.
Consider adding specific test cases for the DisputeKitShutter's encryption/decryption capabilities and its integration with the Shutter API to ensure complete coverage.
Also applies to: 71-71, 73-73
6-6: Update test description to reflect expanded scopeThe describe block is still labeled "DisputeKitClassic" even though the tests now cover both classic and shutter dispute kits. Consider renaming this to "DisputeKits" or "Arbitration System" to more accurately reflect the expanded test scope.
-describe("DisputeKitClassic", async () => { +describe("DisputeKits", async () => {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro (Legacy)
📒 Files selected for processing (6)
contracts/README.md(1 hunks)contracts/deploy/00-home-chain-arbitration.ts(1 hunks)contracts/deployments/arbitrumSepoliaDevnet/DisputeKitShutter.json(1 hunks)contracts/deployments/arbitrumSepoliaDevnet/DisputeKitShutter_Proxy.json(1 hunks)contracts/src/proxy/KlerosProxies.sol(1 hunks)contracts/test/arbitration/index.ts(2 hunks)
✅ Files skipped from review due to trivial changes (4)
- contracts/README.md
- contracts/src/proxy/KlerosProxies.sol
- contracts/deployments/arbitrumSepoliaDevnet/DisputeKitShutter_Proxy.json
- contracts/deployments/arbitrumSepoliaDevnet/DisputeKitShutter.json
⏰ Context from checks skipped due to timeout of 90000ms (15)
- 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
- GitHub Check: Redirect rules - kleros-v2-testnet
- GitHub Check: Redirect rules - kleros-v2-testnet-devtools
- GitHub Check: Header rules - kleros-v2-testnet-devtools
- GitHub Check: Header rules - kleros-v2-testnet
- GitHub Check: Header rules - kleros-v2-testnet
- GitHub Check: Pages changed - kleros-v2-testnet-devtools
- GitHub Check: Pages changed - kleros-v2-testnet
- GitHub Check: Pages changed - kleros-v2-testnet
- GitHub Check: SonarCloud
- GitHub Check: contracts-testing
- GitHub Check: Analyze (javascript)
🔇 Additional comments (5)
contracts/deploy/00-home-chain-arbitration.ts (1)
99-106: Good integration of DisputeKitShutterThe implementation properly deploys the new DisputeKitShutter contract as upgradeable and registers it with the core contract. This aligns with the PR objective of introducing Shutter API integration for message encryption in the dispute kit system.
Consider adding a brief comment explaining what DisputeKitShutter is for and how it differs from DisputeKitClassic, which would improve code documentation.
contracts/test/arbitration/index.ts (4)
3-3: LGTM!Correctly imports the new DisputeKitShutter type.
9-9: LGTM!Properly declares the disputeKitShutter variable for use in tests.
13-13: LGTM!Updated to destructure the disputeKitShutter from the return value of deployContracts().
18-18: LGTM!Test correctly verifies both dispute kits are created with the expected IDs and addresses.
Also applies to: 21-22
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 (4)
web/src/components/Popup/Description/VoteWithCommit.tsx(1 hunks)web/src/components/Popup/ExtraInfo/VoteWithCommitExtraInfo.tsx(1 hunks)web/src/components/Popup/index.tsx(4 hunks)web/src/pages/Cases/CaseDetails/Voting/index.tsx(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- web/src/pages/Cases/CaseDetails/Voting/index.tsx
🧰 Additional context used
🧠 Learnings (3)
web/src/components/Popup/ExtraInfo/VoteWithCommitExtraInfo.tsx (6)
Learnt from: Harman-singh-waraich
PR: kleros/kleros-v2#1729
File: web/src/layout/Header/navbar/Menu/Settings/Notifications/FormContactDetails/index.tsx:125-127
Timestamp: 2024-10-29T10:14:52.512Z
Learning: In `web/src/layout/Header/navbar/Menu/Settings/Notifications/FormContactDetails/index.tsx`, when `isEmailUpdateable` is false, `user?.emailUpdateableAt` is always defined. Therefore, using the non-null assertion `!` with `user?.emailUpdateableAt!` is acceptable because TypeScript may not infer its definiteness.
Learnt from: Harman-singh-waraich
PR: kleros/kleros-v2#1739
File: web/src/pages/Home/TopJurors/JurorCard/Coherency.tsx:22-26
Timestamp: 2024-11-07T10:48:16.774Z
Learning: In the `Coherency` component (`web/src/pages/Home/TopJurors/JurorCard/Coherency.tsx`), `totalResolvedVotes` is always greater than or equal to `totalCoherentVotes`. When both are zero, `0/0` results in `NaN`, which is acceptable in this context.
Learnt from: 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: Harman-singh-waraich
PR: kleros/kleros-v2#1729
File: web/src/layout/Header/navbar/Menu/Settings/Notifications/FormContactDetails/index.tsx:69-69
Timestamp: 2024-10-29T10:13:04.524Z
Learning: In `web/src/layout/Header/navbar/Menu/Settings/Notifications/FormContactDetails/index.tsx`, when the button is disabled, the associated logic won't be reached, and certain code paths may exist for TypeScript purposes.
Learnt from: Harman-singh-waraich
PR: kleros/kleros-v2#1775
File: web/src/pages/Courts/CourtDetails/StakePanel/StakeWithdrawButton.tsx:0-0
Timestamp: 2024-12-09T12:36:59.441Z
Learning: In the `StakeWithdrawButton` component, the transaction flow logic is tightly linked to component updates, so extracting it into a custom hook does not provide significant benefits.
Learnt from: jaybuidl
PR: kleros/kleros-v2#1582
File: web-devtools/src/app/(main)/ruler/SelectArbitrable.tsx:88-90
Timestamp: 2024-10-09T10:22:41.474Z
Learning: Next.js recommends using the `useEffect` hook to set `isClient` and using `suppressHydrationWarning` as a workaround for handling hydration inconsistencies, especially when dealing with data like `knownArbitrables` that may differ between server-side and client-side rendering. This approach is acceptable in TypeScript/React applications, such as in `web-devtools/src/app/(main)/ruler/SelectArbitrable.tsx`.
web/src/components/Popup/Description/VoteWithCommit.tsx (3)
Learnt from: Harman-singh-waraich
PR: kleros/kleros-v2#1729
File: web/src/layout/Header/navbar/Menu/Settings/Notifications/FormContactDetails/index.tsx:125-127
Timestamp: 2024-10-29T10:14:52.512Z
Learning: In `web/src/layout/Header/navbar/Menu/Settings/Notifications/FormContactDetails/index.tsx`, when `isEmailUpdateable` is false, `user?.emailUpdateableAt` is always defined. Therefore, using the non-null assertion `!` with `user?.emailUpdateableAt!` is acceptable because TypeScript may not infer its definiteness.
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: Harman-singh-waraich
PR: kleros/kleros-v2#1739
File: web/src/pages/Home/TopJurors/JurorCard/Coherency.tsx:22-26
Timestamp: 2024-11-07T10:48:16.774Z
Learning: In the `Coherency` component (`web/src/pages/Home/TopJurors/JurorCard/Coherency.tsx`), `totalResolvedVotes` is always greater than or equal to `totalCoherentVotes`. When both are zero, `0/0` results in `NaN`, which is acceptable in this context.
web/src/components/Popup/index.tsx (1)
Learnt from: Harman-singh-waraich
PR: kleros/kleros-v2#1775
File: web/src/pages/Courts/CourtDetails/StakePanel/StakeWithdrawButton.tsx:0-0
Timestamp: 2024-12-09T12:36:59.441Z
Learning: In the `StakeWithdrawButton` component, the transaction flow logic is tightly linked to component updates, so extracting it into a custom hook does not provide significant benefits.
🧬 Code Graph Analysis (1)
web/src/components/Popup/Description/VoteWithCommit.tsx (1)
web/src/components/Popup/index.tsx (1)
VoteDescriptionEmphasizedDate(111-116)
⏰ 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). (12)
- GitHub Check: Redirect rules - kleros-v2-testnet
- GitHub Check: Redirect rules - kleros-v2-testnet
- GitHub Check: Redirect rules - kleros-v2-neo
- GitHub Check: Redirect rules - kleros-v2-testnet-devtools
- GitHub Check: Header rules - kleros-v2-testnet
- GitHub Check: Header rules - kleros-v2-testnet
- GitHub Check: Header rules - kleros-v2-neo
- GitHub Check: Header rules - kleros-v2-testnet-devtools
- GitHub Check: Pages changed - kleros-v2-testnet
- GitHub Check: Pages changed - kleros-v2-testnet
- GitHub Check: Pages changed - kleros-v2-testnet-devtools
- GitHub Check: Pages changed - kleros-v2-neo
🔇 Additional comments (4)
web/src/components/Popup/ExtraInfo/VoteWithCommitExtraInfo.tsx (1)
15-24: LGTM! Clean implementation of conditional messaging.The addition of the
automaticVoteRevealprop with proper TypeScript interface and default handling is well-implemented. The conditional messaging appropriately differentiates between automatic and manual vote reveal scenarios for the Shutter dispute kit integration.web/src/components/Popup/index.tsx (2)
171-171: LGTM! Proper interface extension for Shutter integration.The addition of the optional
automaticVoteRevealproperty to theIPopupinterface is correctly implemented and follows TypeScript best practices.
183-183: LGTM! Proper prop threading to child components.The
automaticVoteRevealprop is correctly passed through the component hierarchy to bothVoteWithCommitandVoteWithCommitExtraInfocomponents, enabling consistent behavior for the Shutter dispute kit's automatic vote reveal functionality.Also applies to: 218-218, 265-265
web/src/components/Popup/Description/VoteWithCommit.tsx (1)
6-7: LGTM! Proper interface definition for enhanced functionality.The interface correctly defines both props as optional, supporting the new automatic vote reveal functionality while maintaining backward compatibility.
|
@coderabbitai resolve |
✅ Actions performedComments resolved and changes approved. |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| } catch (e) { | ||
| logger.error( | ||
| `CastVoteShutter: will fail for disputeID: ${coreDispute.id} and voteIDs [${voteIDs.join(", ")}], skipping` | ||
| ); | ||
| continue; | ||
| } |
Check notice
Code scanning / SonarCloud
Exceptions should not be ignored Low
| } catch (error) { | ||
| throw new Error(`Failed to parse API response as JSON: ${responseText}`); | ||
| } |
Check notice
Code scanning / SonarCloud
Exceptions should not be ignored Low
| } catch (error) { | ||
| throw new Error(`Failed to parse API response as JSON: ${responseText}`); | ||
| } |
Check notice
Code scanning / SonarCloud
Exceptions should not be ignored Low
| } catch (error) { | ||
| throw new Error(`Failed to parse API response as JSON: ${responseText}`); | ||
| } |
Check notice
Code scanning / SonarCloud
Exceptions should not be ignored Low
| } catch (error) { | ||
| throw new Error(`Failed to parse API response as JSON: ${responseText}`); | ||
| } |
Check notice
Code scanning / SonarCloud
Exceptions should not be ignored Low
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (5)
subgraph/core-neo/subgraph.template.yaml (1)
157-157: Incorrect mapping file forDisputeKitShutterin Neo template.The Neo Shutter data source reuses the Classic mapping file, which should point to the Shutter handler instead.
- file: ./src/DisputeKitClassic.ts + file: ./src/DisputeKitShutter.tscontracts/scripts/keeperBotShutter.ts (3)
59-62: Guard against malformed Graph vote IDs
Number()can silently yieldNaN, propagating bad data downstream. Add validation and a clear error message.- const [disputeKitID, localDisputeID, localRoundID, voteID] = graphVoteId.split("-").map(Number); - return { disputeKitID, localDisputeID, localRoundID, voteID }; + const parts = graphVoteId.split("-"); + if (parts.length !== 4) { + throw new Error(`Invalid graphVoteId format: "${graphVoteId}"`); + } + const [dk, ld, lr, v] = parts.map((n) => { + const parsed = Number(n); + if (Number.isNaN(parsed)) throw new Error(`Invalid graphVoteId segment "${n}" in "${graphVoteId}"`); + return parsed; + }); + return { disputeKitID: dk, localDisputeID: ld, localRoundID: lr, voteID: v };
173-181: Possibleundefinedaccess when no current local round exists
filteredLocalRounds[0]will throw whenlocalRoundsis empty, causing the whole script to abort.Add an existence check:
- const filteredLocalRounds = dk.localRounds.filter((_, i) => i === idx); - return { - coreDispute: dk.coreDispute, - votes: filteredLocalRounds[0].votes, - }; + const currentLocalRound = dk.localRounds[idx]; + if (!currentLocalRound) return null; // skip gracefully + return { + coreDispute: dk.coreDispute, + votes: currentLocalRound.votes, + };Then filter out null values from the resulting array:
- }); + }).filter(Boolean);
219-225:getBytes(vote.commit)may break indexed event lookup
_commitis likely an indexedbytes32. Passing a byte array returned bygetBytesmay not match the topic encoding, leading to empty event sets and missed reveals.- const filter = disputeKitShutter.filters.CommitCastShutter(coreDispute.id, vote.juror.id, getBytes(vote.commit)); + const filter = disputeKitShutter.filters.CommitCastShutter(coreDispute.id, vote.juror.id, vote.commit);contracts/scripts/shutter.ts (1)
172-177:crypto.getRandomValuesis not available in Node.js
getRandomValuesis a Web Crypto API. In a Node environment usecrypto.randomBytes.- return ("0x" + - crypto - .getRandomValues(new Uint8Array(32)) - .reduce((acc, byte) => acc + byte.toString(16).padStart(2, "0"), "")) as Hex; + return ("0x" + crypto.randomBytes(32).toString("hex")) as Hex;This prevents runtime crashes when the script is executed with
ts-node.
🧹 Nitpick comments (3)
contracts/.env.example (1)
21-22: Consider reordering environment variables alphabetically.The static analysis tool suggests that
SHUTTER_APIandSHUTTER_API_KEYshould be placed beforeSUBGRAPH_URLto maintain alphabetical ordering of environment variables.LOGTAIL_TOKEN_DISPUTOR_BOT=ABC123ABC123ABC123ABC HEARTBEAT_URL_KEEPER_BOT=https://uptime.betterstack.com/api/v1/heartbeat/ABC123ABC123ABC123ABC -SHUTTER_API=testnet -SHUTTER_API_KEY= DISPUTES_TO_SKIP= +SHUTTER_API=testnet +SHUTTER_API_KEY= +SUBGRAPH_URL=https://api.studio.thegraph.com/query/61738/kleros-v2-core-devnet/version/latestcontracts/scripts/keeperBotShutter.ts (2)
267-280: Improve error handling for failed vote simulations.The catch block logs the error but swallows the exception. Consider adding a comment to clarify this is intentional to continue processing other votes.
} catch (e) { logger.error( `CastVoteShutter: will fail for disputeID: ${coreDispute.id} and voteIDs [${voteIDs.join(", ")}], skipping` ); + // Continue processing other votes continue; }
302-305: Remove unnecessary continue statement.The
continuestatement at the end of the loop is redundant.} catch (e) { logger.error(e, "Failure"); - continue; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
contracts/.env.example(1 hunks)contracts/package.json(1 hunks)contracts/scripts/keeperBotShutter.ts(1 hunks)contracts/scripts/shutter.ts(1 hunks)contracts/src/arbitration/dispute-kits/DisputeKitClassicBase.sol(7 hunks)contracts/test/foundry/KlerosCore.t.sol(1 hunks)subgraph/core-neo/subgraph.template.yaml(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- contracts/package.json
- contracts/test/foundry/KlerosCore.t.sol
🧰 Additional context used
🧠 Learnings (6)
📓 Common learnings
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`.
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`.
subgraph/core-neo/subgraph.template.yaml (7)
Learnt from: kemuru
PR: kleros/kleros-v2#1774
File: web/src/components/CasesDisplay/index.tsx:61-61
Timestamp: 2024-12-06T13:04:50.495Z
Learning: In `web/src/components/CasesDisplay/index.tsx`, the variables `numberDisputes` and `numberClosedDisputes` can sometimes be `NaN`, and should default to `0` using logical OR (`||`) to prevent display issues in the `StatsAndFilters` component.
Learnt from: jaybuidl
PR: kleros/kleros-v2#1746
File: contracts/config/courts.v2.mainnet-neo.json:3-5
Timestamp: 2024-11-19T17:18:39.007Z
Learning: In `contracts/config/courts.v2.mainnet-neo.json`, the General Court (id: 1) intentionally references itself as its parent (`"parent": 1`). This self-reference is acceptable and should not be flagged as an issue in future reviews.
Learnt from: Harman-singh-waraich
PR: kleros/kleros-v2#1716
File: web-devtools/src/app/(main)/dispute-template/CustomContextInputs.tsx:29-42
Timestamp: 2024-10-28T05:55:12.728Z
Learning: In the `CustomContextInputs` component located at `web-devtools/src/app/(main)/dispute-template/CustomContextInputs.tsx`, the `DisputeRequestParams` array is used to exclude certain variables from the custom input since they are already provided in a preceding component. Therefore, converting it to a type is unnecessary.
Learnt from: jaybuidl
PR: kleros/kleros-v2#1746
File: contracts/config/courts.v2.mainnet-neo.json:3-17
Timestamp: 2024-11-19T16:09:41.467Z
Learning: In `contracts/config/courts.v2.mainnet-neo.json`, the General Court (ID: 1) can have its `parent` ID set to itself (`"parent": 1`), as there is no parent court with ID 0 currently.
Learnt from: Harman-singh-waraich
PR: kleros/kleros-v2#1703
File: kleros-sdk/src/requests/gqlClient.ts:18-18
Timestamp: 2024-10-24T08:16:02.749Z
Learning: In this TypeScript project, when a file (such as `kleros-sdk/src/requests/gqlClient.ts`) exports only a single entity, it's acceptable to use default exports instead of named exports.
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#1846
File: prettier-config/index.js:26-26
Timestamp: 2025-01-23T08:14:47.397Z
Learning: The prettier-plugin-solidity plugin is installed in the kleros-v2 repository, even though it's not visible in the sandbox environment's node_modules (which is expected as node_modules is not committed to the repository).
contracts/src/arbitration/dispute-kits/DisputeKitClassicBase.sol (5)
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`.
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: Harman-singh-waraich
PR: kleros/kleros-v2#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: Harman-singh-waraich
PR: kleros/kleros-v2#1839
File: web/src/pages/Cases/CaseDetails/Voting/Classic/Reveal.tsx:141-149
Timestamp: 2025-01-17T11:11:32.535Z
Learning: In the Kleros v2 codebase, using -1 as an initial value for choice tracking is preferred over undefined on the client-side to explicitly indicate that no option has been selected. This value is only used internally and never reaches the contract.
Learnt from: Harman-singh-waraich
PR: kleros/kleros-v2#1739
File: web/src/pages/Home/TopJurors/JurorCard/Coherency.tsx:22-26
Timestamp: 2024-11-07T10:48:16.774Z
Learning: In the `Coherency` component (`web/src/pages/Home/TopJurors/JurorCard/Coherency.tsx`), `totalResolvedVotes` is always greater than or equal to `totalCoherentVotes`. When both are zero, `0/0` results in `NaN`, which is acceptable in this context.
contracts/.env.example (1)
Learnt from: Harman-singh-waraich
PR: kleros/kleros-v2#1687
File: web/src/context/AtlasProvider.tsx:225-244
Timestamp: 2024-10-15T16:18:32.543Z
Learning: In `web/src/context/AtlasProvider.tsx`, the `atlasUri` variable comes from environment variables and does not change, so it does not need to be included in dependency arrays.
contracts/scripts/keeperBotShutter.ts (18)
Learnt from: kemuru
PR: kleros/kleros-v2#1774
File: web/src/components/CasesDisplay/index.tsx:61-61
Timestamp: 2024-12-06T13:04:50.495Z
Learning: In `web/src/components/CasesDisplay/index.tsx`, the variables `numberDisputes` and `numberClosedDisputes` can sometimes be `NaN`, and should default to `0` using logical OR (`||`) to prevent display issues in the `StatsAndFilters` component.
Learnt from: Harman-singh-waraich
PR: kleros/kleros-v2#1739
File: web/src/pages/Home/TopJurors/JurorCard/Coherency.tsx:22-26
Timestamp: 2024-11-07T10:48:16.774Z
Learning: In the `Coherency` component (`web/src/pages/Home/TopJurors/JurorCard/Coherency.tsx`), `totalResolvedVotes` is always greater than or equal to `totalCoherentVotes`. When both are zero, `0/0` results in `NaN`, which is acceptable in this context.
Learnt from: 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#1762
File: web/src/utils/parseWagmiError.ts:10-17
Timestamp: 2024-11-29T06:23:15.955Z
Learning: In the `web/src/utils/parseWagmiError.ts` file and throughout the codebase, prefer using optional chaining to handle `undefined` or `null` values, including optional arrays, without adding explicit existence or length checks.
Learnt from: Harman-singh-waraich
PR: kleros/kleros-v2#1727
File: web/src/utils/atlas/updateEmail.ts:34-37
Timestamp: 2024-10-28T12:20:19.884Z
Learning: In `web/src/utils/atlas/updateEmail.ts`, the error coming from the `GraphQLError` array already has the necessary structure, so additional specificity in error handling is unnecessary.
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: kemuru
PR: kleros/kleros-v2#1707
File: web/src/pages/Home/CourtOverview/BarChart.tsx:40-42
Timestamp: 2024-10-11T10:37:40.896Z
Learning: In `BarChart.tsx`, the function `getPercentValue` may return `NaN` if `chartData.total` is zero, but this is acceptable since there should always be data, and `chartData.total` should not be zero.
Learnt from: jaybuidl
PR: kleros/kleros-v2#1582
File: web-devtools/src/app/(main)/ruler/RulingModes.tsx:179-199
Timestamp: 2024-10-09T10:18:51.089Z
Learning: In `web-devtools/src/app/(main)/ruler/RulingModes.tsx`, the `handleUpdate` function already handles errors via `wrapWithToast`, so additional error handling is unnecessary.
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: Harman-singh-waraich
PR: kleros/kleros-v2#1703
File: kleros-sdk/src/utils/getDispute.ts:43-43
Timestamp: 2024-10-14T15:29:32.954Z
Learning: In the Kleros SDK, when using the `populateTemplate` function in `kleros-sdk/src/utils/getDispute.ts`, missing variables in Mustache templates are acceptable. Mustache fills in blanks, and it's preferable to return the partially populated template without throwing errors, as it's still helpful for the consumer.
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: Harman-singh-waraich
PR: kleros/kleros-v2#1794
File: web/src/hooks/useStarredCases.tsx:13-18
Timestamp: 2024-12-16T17:17:32.359Z
Learning: In `useStarredCases.tsx`, when handling the `starredCases` Map from local storage, direct mutation is acceptable to prevent the overhead of copying, provided it doesn't adversely affect React's render cycle.
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#1703
File: kleros-sdk/src/utils/getDispute.ts:48-55
Timestamp: 2024-10-22T10:26:23.167Z
Learning: In the codebase, try-catch blocks are used to handle potential JSON parsing errors, even if the catch block rethrows the error.
Learnt from: Harman-singh-waraich
PR: kleros/kleros-v2#1703
File: kleros-sdk/src/requests/fetchDisputeDetails.ts:0-0
Timestamp: 2024-10-24T08:09:39.086Z
Learning: When handling urql `CombinedError`, prefer to throw it directly without additional handling to avoid losing information.
Learnt from: Harman-singh-waraich
PR: kleros/kleros-v2#1727
File: web/src/utils/atlas/updateEmail.ts:32-33
Timestamp: 2024-10-28T12:20:36.536Z
Learning: In the 'kleros-v2' project, it's acceptable to log raw error objects in production code, including in the `updateEmail` function within `web/src/utils/atlas/updateEmail.ts`.
Learnt from: Harman-singh-waraich
PR: kleros/kleros-v2#1759
File: web/src/pages/Cases/CaseDetails/Appeal/Classic/Fund.tsx:140-143
Timestamp: 2024-11-26T10:50:23.399Z
Learning: In the Kleros-v2 React project, errors are handled by the `wrapWithToast` function, which displays error messages to users via toast notifications.
contracts/scripts/shutter.ts (8)
Learnt from: jaybuidl
PR: kleros/kleros-v2#1964
File: contracts/scripts/shutter.ts:3-3
Timestamp: 2025-04-30T22:12:25.476Z
Learning: In Node.js, importing the crypto module as `import crypto from "crypto"` is a valid approach when using methods like `crypto.randomBytes()`. This import style works fine and doesn't need to be changed to a destructured import when these methods are used.
Learnt from: jaybuidl
PR: kleros/kleros-v2#1620
File: contracts/test/arbitration/draw.ts:98-98
Timestamp: 2024-11-05T11:32:29.452Z
Learning: In Ethers v6, `ethers.toBeHex` is the correct method to convert numbers to hex strings.
Learnt from: salgozino
PR: kleros/kleros-v2#1831
File: kleros-sdk/src/utils/getDispute.ts:59-70
Timestamp: 2025-01-13T19:30:33.915Z
Learning: When checking if a hex string represents zero in JavaScript, using `Number()` is safe as it correctly converts "0x0", "0x00" to 0. While `Number()` can have precision issues with large hex values, it's reliable for checking zero.
Learnt from: Harman-singh-waraich
PR: kleros/kleros-v2#1703
File: kleros-sdk/src/utils/getDispute.ts:48-55
Timestamp: 2024-10-22T10:26:23.167Z
Learning: In the codebase, try-catch blocks are used to handle potential JSON parsing errors, even if the catch block rethrows the error.
Learnt from: Harman-singh-waraich
PR: kleros/kleros-v2#1703
File: kleros-sdk/src/requests/fetchDisputeDetails.ts:0-0
Timestamp: 2024-10-24T08:09:39.086Z
Learning: When handling urql `CombinedError`, prefer to throw it directly without additional handling to avoid losing information.
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: Harman-singh-waraich
PR: kleros/kleros-v2#1727
File: web/src/utils/atlas/updateEmail.ts:32-33
Timestamp: 2024-10-28T12:20:36.536Z
Learning: In the 'kleros-v2' project, it's acceptable to log raw error objects in production code, including in the `updateEmail` function within `web/src/utils/atlas/updateEmail.ts`.
Learnt from: Harman-singh-waraich
PR: kleros/kleros-v2#1759
File: web/src/pages/Cases/CaseDetails/Appeal/Classic/Fund.tsx:140-143
Timestamp: 2024-11-26T10:50:23.399Z
Learning: In the Kleros-v2 React project, errors are handled by the `wrapWithToast` function, which displays error messages to users via toast notifications.
🧬 Code Graph Analysis (1)
contracts/scripts/shutter.ts (1)
web/src/utils/shutter.ts (2)
encrypt(151-175)decrypt(183-195)
🪛 dotenv-linter (3.3.0)
contracts/.env.example
[warning] 21-21: [UnorderedKey] The SHUTTER_API key should go before the SUBGRAPH_URL key
[warning] 22-22: [UnorderedKey] The SHUTTER_API_KEY key should go before the SUBGRAPH_URL key
🪛 Biome (1.9.4)
contracts/scripts/keeperBotShutter.ts
[error] 304-304: Unnecessary continue statement
Unsafe fix: Delete the unnecessary continue statement
(lint/correctness/noUnnecessaryContinue)
🪛 GitHub Check: SonarCloud
contracts/scripts/keeperBotShutter.ts
[notice] 275-280: Exceptions should not be ignored
Handle this exception or don't catch it at all.See more on SonarQube Cloud
contracts/scripts/shutter.ts
[notice] 93-95: Exceptions should not be ignored
Handle this exception or don't catch it at all.See more on SonarQube Cloud
[notice] 129-131: Exceptions should not be ignored
Handle this exception or don't catch it at all.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). (10)
- 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-devtools
- GitHub Check: Header rules - kleros-v2-testnet-devtools
- GitHub Check: Pages changed - kleros-v2-testnet-devtools
- GitHub Check: Redirect rules - kleros-v2-testnet
- GitHub Check: Header rules - kleros-v2-testnet
- GitHub Check: Pages changed - kleros-v2-testnet
- GitHub Check: Mend Security Check
🔇 Additional comments (5)
contracts/scripts/keeperBotShutter.ts (1)
40-52: LGTM! Proper validation added.The decode function now includes proper validation for the message format, preventing runtime errors from malformed messages.
contracts/src/arbitration/dispute-kits/DisputeKitClassicBase.sol (3)
249-271: Well-structured refactoring of commit casting.The refactoring to use an internal
_castCommitfunction provides good extensibility for derived contracts while maintaining the same external interface.
467-481: Good extensibility design with the virtual hashVote function.The virtual
hashVotefunction provides a clean extension point for derived contracts to customize vote hashing logic. The documentation clearly explains that unused parameters may be used by overriding contracts.
637-671: Useful view functions added for querying dispute state.The new view functions
getNumberOfRounds,getLocalDisputeRoundID, and the enhancedgetVoteInfoprovide valuable query capabilities for both on-chain and off-chain consumers.contracts/scripts/shutter.ts (1)
41-52: Secure API key handling implementation.The function properly handles API authentication by:
- Only adding the bearer token for mainnet
- Checking if the API key exists and is not empty after trimming
- Using appropriate header structure
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
contracts/src/arbitration/dispute-kits/DisputeKitClassicBase.sol (1)
41-41: Remove deprecated code marked as "DO NOT COMMIT".This line contains a deprecated mapping that is explicitly marked as "DO NOT COMMIT". It should be removed before merging.
🧹 Nitpick comments (1)
contracts/src/arbitration/dispute-kits/DisputeKitClassicBase.sol (1)
308-324: Consider extracting vote validation logic to reduce stack pressure.While the scope workaround for stack too deep is acceptable, consider extracting the vote validation logic into a separate internal function for better readability and maintainability.
Example refactor:
- { - (uint96 courtID, , , , ) = core.disputes(_coreDisputeID); - (, bool hiddenVotes, , , , , ) = core.courts(courtID); - bytes32 voteHash = hashVote(_choice, _salt, _justification); - - // Save the votes. - for (uint256 i = 0; i < _voteIDs.length; i++) { - require(round.votes[_voteIDs[i]].account == _juror, "The juror has to own the vote."); - require( - !hiddenVotes || round.votes[_voteIDs[i]].commit == voteHash, - "The vote hash must match the commitment in courts with hidden votes." - ); - require(!round.votes[_voteIDs[i]].voted, "Vote already cast."); - round.votes[_voteIDs[i]].choice = _choice; - round.votes[_voteIDs[i]].voted = true; - } - } // Workaround stack too deep + _validateAndSaveVotes(_coreDisputeID, round, _voteIDs, _choice, _salt, _justification, _juror);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
contracts/src/arbitration/dispute-kits/DisputeKitClassicBase.sol(7 hunks)contracts/test/foundry/KlerosCore.t.sol(1 hunks)subgraph/core-neo/subgraph.yaml(1 hunks)subgraph/core/subgraph.yaml(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- subgraph/core-neo/subgraph.yaml
- subgraph/core/subgraph.yaml
- contracts/test/foundry/KlerosCore.t.sol
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
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`.
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`.
contracts/src/arbitration/dispute-kits/DisputeKitClassicBase.sol (5)
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`.
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: Harman-singh-waraich
PR: kleros/kleros-v2#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: Harman-singh-waraich
PR: kleros/kleros-v2#1839
File: web/src/pages/Cases/CaseDetails/Voting/Classic/Reveal.tsx:141-149
Timestamp: 2025-01-17T11:11:32.535Z
Learning: In the Kleros v2 codebase, using -1 as an initial value for choice tracking is preferred over undefined on the client-side to explicitly indicate that no option has been selected. This value is only used internally and never reaches the contract.
Learnt from: Harman-singh-waraich
PR: kleros/kleros-v2#1739
File: web/src/pages/Home/TopJurors/JurorCard/Coherency.tsx:22-26
Timestamp: 2024-11-07T10:48:16.774Z
Learning: In the `Coherency` component (`web/src/pages/Home/TopJurors/JurorCard/Coherency.tsx`), `totalResolvedVotes` is always greater than or equal to `totalCoherentVotes`. When both are zero, `0/0` results in `NaN`, which is acceptable in this context.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (14)
- GitHub Check: Redirect rules - kleros-v2-university
- GitHub Check: Header rules - kleros-v2-university
- GitHub Check: Pages changed - kleros-v2-university
- GitHub Check: Redirect rules - kleros-v2-neo
- GitHub Check: Redirect rules - kleros-v2-testnet
- GitHub Check: Redirect rules - kleros-v2-testnet
- GitHub Check: Header rules - kleros-v2-neo
- GitHub Check: Header rules - kleros-v2-testnet
- GitHub Check: Header rules - kleros-v2-testnet
- GitHub Check: Pages changed - kleros-v2-neo
- GitHub Check: Pages changed - kleros-v2-testnet
- GitHub Check: Pages changed - kleros-v2-testnet
- GitHub Check: contracts-testing
- GitHub Check: Analyze (javascript)
🔇 Additional comments (4)
contracts/src/arbitration/dispute-kits/DisputeKitClassicBase.sol (4)
248-271: Good refactoring of commit casting logic.The separation of
castCommitinto an external wrapper and internal_castCommitfunction follows best practices for extensibility. This allows derived contracts to customize the commit behavior while maintaining the core logic.
467-481: Well-designed virtual function for extensible vote hashing.The
hashVotefunction provides a clean extension point for derived contracts to customize vote hash computation. The default implementation maintains backward compatibility while allowing contracts likeDisputeKitShutterto include additional parameters like justification in the hash.
637-671: Useful view functions for querying dispute and vote information.The new view functions provide clean interfaces for:
- Querying the number of rounds in a dispute
- Mapping between core and local dispute/round IDs
- Retrieving detailed vote information
These additions improve the contract's observability without compromising security.
291-298: No external exposure of internal_castVotewith arbitrary_jurorBoth public voting functions only invoke
_castVotewith a validated juror address:
castVotepassesmsg.sendercastVoteShutterderives the juror from on‐chain vote ownershipThere’s no other external entry point calling
_castVotethat would allow spoofing the_jurorparameter. As a result, the originally flagged security concern is not applicable.
|
Devnet contract deployed
DisputeKitShutter
DisputeKitBase changes
hashVote()which is overridden byDisputeKitShutterto include the justification in the commitment hash_castVote()with an arbitrary_jurorparameter to allow anyone to reveal a committed vote (which reverts if hash verification fails)getNumberOfRounds(_localDisputeID)getLocalDisputeRoundID(_coreDisputeID, _coreRoundID) returns (localDisputeID, localRoundID)PR-Codex overview
This PR introduces the
DisputeKitShutteralongside updates to the existing voting and appeal mechanisms, enhancing the Kleros system with new functionalities and improved handling of disputes.Detailed summary
DisputeKitShuttercontract with specific voting and commitment features.Shutterdispute kit.SHUTTER_API.Summary by CodeRabbit
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Chores
Tests