Support for recovery hash in Shutter DK #2100
Conversation
❌ 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-testnet-devtools ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
WalkthroughImplements juror recovery for Shutter dispute kit (store recovery commits, allow juror reveal without justification, conditional hashing), refactors Classic DK vote-hash lookup and hashVote mutability, and updates build configs to target EVM "cancun" with higher optimizer runs. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant C as Caller
participant DK as DisputeKitShutter
participant S as Storage
rect rgb(245,248,255)
note over C,DK: Commit phase (Shutter)
C->>DK: castCommitShutter(dispute,round,voteID,commit,recoveryCommit)
DK->>S: store votes[...].commit
DK->>S: store recoveryCommitments[...]
DK-->>C: CommitCastShutter(..., recoveryCommit)
end
rect rgb(245,255,245)
note over C,DK: Reveal phase
C->>DK: castVoteShutter(dispute,round,voteID,choice,salt,justif)
alt Caller is juror
DK->>DK: callerIsJuror = true
DK->>DK: actual = hashVote(choice,salt,justif) %% returns hash(choice+salt)
DK->>S: expected = recoveryCommitments[...]
else Auto-reveal (non-juror)
DK->>DK: callerIsJuror = false
DK->>DK: actual = hashVote(choice,salt,justif) %% returns hash(choice+salt+justifHash)
DK->>S: expected = votes[...].commit
end
DK->>DK: compare actual vs expected -> accept or revert
DK->>DK: callerIsJuror = false
DK-->>C: result
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Assessment against linked issues
Assessment against linked issues: Out-of-scope changes
Possibly related PRs
Suggested reviewers
Poem
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. 📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 💡 Knowledge Base configuration:
You can enable these sources in your CodeRabbit configuration. 📒 Files selected for processing (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). (12)
🔇 Additional comments (2)
✨ Finishing Touches🧪 Generate unit tests
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
jaybuidl
added
Type: Feature🗿
Package: Contracts
jaybuidl
changed the title
✅ 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
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/DisputeKitShutter.sol (1)
117-133: Guard against empty _voteIDs to avoid OOB revert and preserve intended error semanticscastVoteShutter reads _voteIDs[0] before delegating to _castVote. If _voteIDs is empty, this will revert with an OOB panic instead of EmptyVoteIDs. Add an explicit length check first.
Apply this diff:
- Dispute storage dispute = disputes[coreDisputeIDToLocal[_coreDisputeID]]; + if (_voteIDs.length == 0) revert EmptyVoteIDs(); + Dispute storage dispute = disputes[coreDisputeIDToLocal[_coreDisputeID]];Optionally, restore callerIsJuror after the call using a save/restore pattern to be future-proof against nested flows:
- callerIsJuror = juror == msg.sender; - // `_castVote()` ensures that all the `_voteIDs` do belong to `juror` - _castVote(_coreDisputeID, _voteIDs, _choice, _salt, _justification, juror); - callerIsJuror = false; + bool prev = callerIsJuror; + callerIsJuror = (juror == msg.sender); + // `_castVote()` ensures that all the `_voteIDs` do belong to `juror` + _castVote(_coreDisputeID, _voteIDs, _choice, _salt, _justification, juror); + callerIsJuror = prev;
🧹 Nitpick comments (3)
contracts/src/arbitration/dispute-kits/DisputeKitShutter.sol (3)
28-29: Using transient storage for caller role — correct for CancunTransient bool callerIsJuror is appropriate for request-scoped switching of hashing behavior and avoids persistent state. With both toolchains set to Cancun, this should work as intended.
Consider a brief NatSpec note near the declaration clarifying that it influences hashVote() behavior during castVoteShutter only.
103-115: Write recoveryCommitments only after ownership/period checks to avoid wasted gas on revertYou populate recoveryCommitments before calling _castCommit. Although a revert in _castCommit will roll back these writes, the SSTORE gas is still consumed in the failing path. Move the writes after _castCommit to avoid unnecessary gas burn on invalid calls.
Apply this diff:
- if (_recoveryCommit == bytes32(0)) revert EmptyRecoveryCommit(); - - uint256 localDisputeID = coreDisputeIDToLocal[_coreDisputeID]; - Dispute storage dispute = disputes[localDisputeID]; - uint256 localRoundID = dispute.rounds.length - 1; - for (uint256 i = 0; i < _voteIDs.length; i++) { - recoveryCommitments[localDisputeID][localRoundID][_voteIDs[i]] = _recoveryCommit; - } - - // `_castCommit()` ensures that the caller owns the vote - _castCommit(_coreDisputeID, _voteIDs, _commit); + if (_recoveryCommit == bytes32(0)) revert EmptyRecoveryCommit(); + + // `_castCommit()` ensures that the caller owns the vote and we are in commit period. + _castCommit(_coreDisputeID, _voteIDs, _commit); + + uint256 localDisputeID = coreDisputeIDToLocal[_coreDisputeID]; + Dispute storage dispute = disputes[localDisputeID]; + uint256 localRoundID = dispute.rounds.length - 1; + for (uint256 i = 0; i < _voteIDs.length; i++) { + recoveryCommitments[localDisputeID][localRoundID][_voteIDs[i]] = _recoveryCommit; + }
150-159: Context-sensitive hashVote: confirm off-chain commit derivation matches client librarieshashVote now depends on callerIsJuror:
- juror path: keccak(abi.encodePacked(choice, salt))
- non-juror path: keccak(abi.encode(choice, salt, keccak256(bytes(justification))))
This is fine for on-chain verification, but it changes expectations for off-chain commitment generation. Please confirm that the Shutter client/wallet libraries compute:
- commit (with justification) as keccak(abi.encode(choice, salt, keccak256(bytes(justification))))
- recoveryCommit (without justification) as keccak(abi.encodePacked(choice, salt))
If prior versions used abi.encodePacked for the justification-including commit, this is an ABI-breaking change for commit preimages.
I can scan your SDKs/examples to ensure these formulas match. Want me to run a targeted repo search?
Consider adding a NatSpec note here stating the context-dependent behavior to prevent integrator confusion.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (4)
contracts/foundry.toml(1 hunks)contracts/hardhat.config.ts(1 hunks)contracts/src/arbitration/dispute-kits/DisputeKitClassicBase.sol(3 hunks)contracts/src/arbitration/dispute-kits/DisputeKitShutter.sol(6 hunks)
⏰ 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). (15)
- GitHub Check: Redirect rules - kleros-v2-testnet
- GitHub Check: Redirect rules - kleros-v2-testnet-devtools
- GitHub Check: Redirect rules - kleros-v2-testnet
- GitHub Check: Header rules - kleros-v2-testnet-devtools
- GitHub Check: Header rules - kleros-v2-testnet
- GitHub Check: Header rules - kleros-v2-testnet
- GitHub Check: Redirect rules - kleros-v2-university
- GitHub Check: Pages changed - kleros-v2-testnet
- GitHub Check: Pages changed - kleros-v2-testnet-devtools
- GitHub Check: Pages changed - kleros-v2-testnet
- GitHub Check: Header rules - kleros-v2-university
- GitHub Check: SonarCloud
- GitHub Check: Analyze (javascript)
- GitHub Check: contracts-testing
- GitHub Check: Pages changed - kleros-v2-university
🔇 Additional comments (10)
contracts/foundry.toml (1)
4-7: Aligning Cancun EVM target and raising optimizer runs — LGTMSetting evm_version = "cancun" and bumping optimizer_runs to 10000 aligns Foundry with the Solidity 0.8.30 toolchain and the Hardhat config in this PR. This is required for transient storage (EIP-1153) used in Shutter. No action needed.
If you want an extra safety check for deterministic bytecode across toolchains, run a quick cross-build compare in CI (Foundry vs Hardhat) on a small sample contract to catch accidental setting drift.
contracts/hardhat.config.ts (1)
31-36: Hardhat now targeting Cancun — consistent with FoundryAdding evmVersion: "cancun" keeps Hardhat in sync with Foundry and enables compilation of transient storage (used by DisputeKitShutter). Optimizer runs = 10000 also match Foundry. Looks good.
contracts/src/arbitration/dispute-kits/DisputeKitClassicBase.sol (4)
317-332: Hidden-vote validation now routed through an overrideable expected-hash — solid hook pointSwitching to localDisputeID/localRoundID and comparing hashVote(...) against getExpectedVoteHash(...) cleanly decouples “how to compute/where to fetch expected commits” from the base flow. This enables Shutter’s dual-hash scheme without touching core logic. Nice separation of concerns.
491-494: hashVote mutability change (pure → view) to allow overrides — OKThe base implementation remains keccak(choice, salt). Documenting the unused justification parameter in the signature is helpful for overrides. No issues.
715-727: New internal hook getExpectedVoteHash — good extensibility pointProviding a virtual getter for the expected hash is the right way to support alternative commitment schemes (e.g., recovery commitments in Shutter). No concerns.
737-737: Minor arg name anonymization to reduce stack footprint — fineRenaming the Round parameter to an unused placeholder is harmless and helps with “stack too deep” pressure in downstream overrides.
contracts/src/arbitration/dispute-kits/DisputeKitShutter.sol (4)
21-23: New recoveryCommitments storage — acknowledge storage layout breakAdding recoveryCommitments is a storage-layout change versus previously deployed versions. Given the PR explicitly plans a full redeploy (not upgrade), this is acceptable. Ensure all deployment scripts and on-chain addresses reflect the redeploy plan to avoid proxy upgrades on old storage layouts.
Do you want a quick checklist PR comment for “full redeploy” steps (addresses, permit lists, court params, governor handoff, etc.)?
38-46: CommitCastShutter ABI change: _recoveryCommit addedEvent shape change is ABI-breaking as noted. Make sure indexation choices match consumer needs: you kept _commit indexed and left _recoveryCommit non-indexed (reasonable since topics are limited). Communicate this to indexers/SDKs.
If downstream infra filters on the old event signature, flag them to update to the new one.
167-176: getExpectedVoteHash override correctly switches between commit and recoveryCommitThis matches the intended flows:
- juror caller: validate against recoveryCommitments
- non-juror caller: validate against stored commit
Looks correct.
182-183: EmptyRecoveryCommit error — good explicit revertClear error improves debuggability at commit time.
…cted, causing stack too deep
|
Closes #2016
_recoveryCommitin the eventCommitCastShutterrecoveryCommitmentsinDisputeKitShutterwhich collides withDisputeKitBase.wNativeadded recently.These changes require a full redeploy, not an upgrade.
PR-Codex overview
This PR focuses on enhancing the
DisputeKitClassicBaseandDisputeKitShuttercontracts by improving vote hashing, adding recovery commitments, and optimizing the Solidity compiler settings. It also updates error handling and introduces new functions for better vote management.Detailed summary
evmVersionconfiguration tohardhat.config.ts.foundry.tomlwith new optimizer settings.DisputeKitClassicBaseto use local IDs.getExpectedVoteHashfunction for retrieving vote hashes.hashVotefunction to handle justification differently.recoveryCommitmentsmapping inDisputeKitShutter.castCommitShutterto handle recovery commitments.CommitCastShutterevent to include recovery commitment.hashVoteinDisputeKitShutterto differentiate juror calls.EmptyRecoveryCommiterror for better error handling.Summary by CodeRabbit
New Features
Refactor
Chores