chore: sync-changes-from-governor-in-kleros-v2#16
Conversation
✅ Deploy Preview for kleros-governor-v2 ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
WalkthroughAdds SafeSend (WETH fallback) and wNative support, refactors KlerosGovernor access/errors and dispute mapping, updates deployment artifacts/scripts (WETH, chainId, factory args), consolidates Etherscan API usage, and adjusts frontend list modal flow and governor addresses. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant User as User (EOA)
participant Governor as KlerosGovernor
participant Recipient as Recipient (payable)
participant WETH as WETH (wNative)
participant Arbitrator as Arbitrator
Note over User,Governor: submitList / payout / dispute flow (high-level)
User->>Governor: submitList(...){payable ETH}
Governor->>Recipient: try native ETH transfer
alt native transfer succeeds
Recipient-->>Governor: receive ETH
else native transfer fails
Governor->>WETH: deposit() payable (wrap ETH)
Governor->>WETH: transfer(Recipient, amount)
end
opt dispute requested
Governor->>Arbitrator: createDispute(...)
Arbitrator-->>Governor: disputeID
Governor->>Governor: arbitratorDisputeIDToSessionIndex[disputeID] = sessionIndex
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 |
![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
📜 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 (3)
contracts/package.json(1 hunks)contracts/src/KlerosGovernor.sol(16 hunks)contracts/src/libraries/SafeSend.sol(1 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). (3)
- GitHub Check: Redirect rules - kleros-governor-v2
- GitHub Check: Header rules - kleros-governor-v2
- GitHub Check: Pages changed - kleros-governor-v2
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
web/src/consts/governors.ts (1)
21-43: Three governors sharing the same address breaks chain-specific resolutionVerification confirms the issue. All three governor entries (lines 22, 30, 38) use the identical address
0x52e6766e6C4fB05Caec92e5318668b3E366D649a:export const getGovernor = (address: string) => governors.find((governor) => governor.address === address);Since
getGovernorsearches by address alone and.find()returns the first match, any lookup for that address will always return the Arbitrum Sepolia entry. Additionally,generateStaticParams()will only create a single route for that shared address, making the Gnosis and mainnet entries unreachable.Smart contracts typically deploy to different addresses on different networks. Either:
- Update each governor entry with its correct network-specific address, or
- Modify
getGovernorto also accept and filter by chain if there is legitimate address reuse.Without this fix, the Gnosis and mainnet governors cannot be accessed, and any route to that address will serve incorrect chain metadata.
♻️ Duplicate comments (2)
contracts/src/KlerosGovernor.sol (2)
55-60: Add a zero‑address guard for_wNativeto avoid bricking SafeSend refunds
wNativeis stored directly from the constructor argument with no validation:address public wNative; // line 58 ... constructor(..., uint256 _withdrawTimeout, address _wNative) { ... wNative = _wNative; }If
_wNativeis ever passed asaddress(0), every SafeSend fallback path will revert when trying to interact with the WETH‑like contract, breaking refunds to contracts (e.g. Safes) whenever a plain ETH send fails.Add a dedicated error and guard in the constructor:
@@ - address public wNative; // The wrapped native token for safeSend(). + address public wNative; // The wrapped native token for safeSend(). @@ - uint256 _withdrawTimeout, - address _wNative + uint256 _withdrawTimeout, + address _wNative ) { @@ - arbitratorExtraData = _arbitratorExtraData; - wNative = _wNative; + arbitratorExtraData = _arbitratorExtraData; + if (_wNative == address(0)) revert InvalidWNativeAddress(); + wNative = _wNative; @@ - error SubmissionTimeHasEnded(); + error SubmissionTimeHasEnded(); @@ - error AlreadyExecuted(); + error AlreadyExecuted(); + error InvalidWNativeAddress();This is a one‑time check but prevents subtle failures in all paths relying on SafeSend.
Also applies to: 127-142, 460-478
90-93:onlyByOwnercurrently blocks all external governance callsThe
onlyByOwnermodifier still checks againstaddress(this):modifier onlyByOwner() { if (address(this) != msg.sender) revert OwnerOnly(); _; }All governance setters (e.g.,
changeSubmissionDeposit,changeSubmissionTimeout,changeExecutionTimeout,changeWithdrawTimeout,changeArbitrator,changeDisputeTemplate) are now effectively callable only by the contract itself, making them unusable after deployment.Introduce an explicit
ownervariable and set it in the constructor, then checkmsg.senderagainst that:contract KlerosGovernor is IArbitrableV2 { + address public owner; @@ - modifier onlyByOwner() { - if (address(this) != msg.sender) revert OwnerOnly(); - _; - } + modifier onlyByOwner() { + if (msg.sender != owner) revert OwnerOnly(); + _; + } @@ constructor - ) { - arbitrator = _arbitrator; + ) { + owner = msg.sender; + arbitrator = _arbitrator;Without this fix, all the governance configuration endpoints are effectively dead.
Also applies to: 163-208
🧹 Nitpick comments (8)
contracts/scripts/verifyProxies.sh (1)
5-5: Etherscan v2 wiring looks good; consider tightening the proxy file loopThe switch to the unified
https://api.etherscan.io/v2/apiendpoint with an explicitchainidandETHERSCAN_API_KEYplus the added key check all look sane.The only thing I’d tweak is the
for f in $(ls …)pattern, which ShellCheck flagged (SC2045) and can misbehave with spaces / no matches. You can avoidlsand rely on globs directly:function verify() { #deploymentDir #explorerApiUrl #apiKey #chainId @@ - echo "verifying proxies on $(basename $deploymentDir)" - for f in $(ls -1 $deploymentDir/*_Proxy.json 2>/dev/null); do + echo "verifying proxies on $(basename "$deploymentDir")" + shopt -s nullglob + for f in "$deploymentDir"/*_Proxy.json; do contractName=$(basename $f .json) @@ - echo - done + echo + done + shopt -u nullglob }This keeps behavior the same while being more robust.
Also applies to: 7-22, 24-28, 30-34
contracts/.env.example (1)
1-5: Align.envformat withdotenvexpectations and README, avoidexportsyntaxUsing
export VAR = ""is unconventional for.envfiles and may not be parsed as expected bydotenv, and it also conflicts with the linter output. Sincecontracts/scripts/dotenv.shloads this viadotenv.config, I’d strongly suggest switching to plainKEY=lines and adding the optional keys mentioned in the README:-export PRIVATE_KEY = "" -export MAINNET_PRIVATE_KEY = "" -export INFURA_API_KEY = "" -export ETHERSCAN_API_KEY_FIX="" -export ETHERSCAN_API_KEY="" +PRIVATE_KEY= +MAINNET_PRIVATE_KEY= +INFURA_API_KEY= +ETHERSCAN_API_KEY_FIX= +ETHERSCAN_API_KEY= +ARBISCAN_API_KEY= +GNOSISSCAN_API_KEY=This matches typical
.envconventions, fixes the linter warnings, and keeps the example in sync with the docs.contracts/scripts/dotenv.sh (1)
3-15: Simplify quoting around the Node dotenv call to avoid SC2027-style pitfallsFunctionally this works, but the current nested quoting (
"$SCRIPT_DIR"/../.envinside a double-quotednode -estring) is fragile and triggered ShellCheck (SC2027). You can make it more robust by passing the path and key as arguments and keeping the Node snippet single‑quoted:-node -e " - require('dotenv').config({ path: '"$SCRIPT_DIR"/../.env' }) - console.log(process.env.$varKey) -" +node -e ' + const [path, key] = process.argv.slice(2); + require("dotenv").config({ path }); + console.log(process.env[key] ?? ""); +' "$SCRIPT_DIR/../.env" "$varKey"This removes shell interpolation inside the JS, makes the script easier to maintain, and keeps behavior (load
.env, print the requested key).contracts/README.md (1)
69-80: Keep env var docs and.env.examplein sync (ARBISCAN / GNOSISSCAN keys)The README lists
ARBISCAN_API_KEYandGNOSISSCAN_API_KEYas optional verification keys, butcontracts/.env.examplecurrently only definesPRIVATE_KEY,MAINNET_PRIVATE_KEY,INFURA_API_KEY,ETHERSCAN_API_KEY_FIX, andETHERSCAN_API_KEY.To avoid confusion for integrators, consider adding placeholders for
ARBISCAN_API_KEYandGNOSISSCAN_API_KEYin.env.example(and/or updating this section if those keys are no longer used in the current Hardhat/verify setup).web/src/app/(main)/governor/[governorAddress]/ActiveLists/index.tsx (1)
35-42: Modal open/close wiring looks good; minor optional cleanupsThe
handleOpencallback andclosePopupwithrouter.replace(window.location.pathname, { scroll: false })correctly centralize modal state and clear thelistIdparam on close.Two optional polish points (no need to block on them):
ExamineModalis only rendered whenopenListis truthy, soisOpen={!isUndefined(openList)}is alwaystruein that branch; you can hardcodeisOpenor derive it once to reduce redundancy.- If you want list opens from
ListCardto be shareable via URL (like those coming from the dispute template), you could also push?listId=…inhandleOpenfor consistency.web/src/app/(main)/governor/[governorAddress]/ActiveLists/ListCard.tsx (1)
19-25: Externalizing open behavior viasetIsOpenlooks solid (with one minor prop nit)The switch to a parent‑supplied
setIsOpen(list)on hover click cleanly removes modal state fromListCardand keeps the component focused on display concerns.Minor optional tweak:
IListCardstill requiresgovernorAddress, butListCardno longer uses it. If nothing else depends on that prop, you can drop it from the interface and call sites to avoid unused‑parameter noise:-interface IListCard { - list: Submission; - governorAddress: Address; - setIsOpen: (list: Submission) => void; -} -const ListCard: React.FC<IListCard> = ({ list, setIsOpen }) => { +interface IListCard { + list: Submission; + setIsOpen: (list: Submission) => void; +} +const ListCard: React.FC<IListCard> = ({ list, setIsOpen }) => {Also applies to: 60-69
contracts/deploy/00-governor-v2.ts (1)
21-45: wNative deployment wiring is consistent;gfArgscan be simplifiedThe script correctly:
- Pulls
WETHviaethers.getContract("WETH").- Passes
wNative.targetinto bothGovernorFactory.deploy(...)and theKlerosGovernorconstructor (kgArgs).- Uses
kgArgsagain for constructor args during Etherscan verification, which should keep deploy/verify in sync as you evolve constructor parameters.One small cleanup opportunity:
gfArgsis only used to initializekgArgsand never forGovernorFactoryitself. You can inline or rename it to avoid confusion about its purpose, e.g.:- const gfArgs = [ + const kgArgs = [ klerosCore.target, extraData, disputeTemplateRegistry.target, disputeTemplate, dataMappings, 0, 600, 600, - 600, // feeTimeout: 10 minutes + 600, // feeTimeout: 10 minutes wNative.target, ]; - - await governorFactory.deploy( + await governorFactory.deploy( klerosCore.target, extraData, disputeTemplateRegistry.target, disputeTemplate, dataMappings, 0, 600, 600, 600, // feeTimeout: 10 minutes, wNative.target ); - - const kgArgs = gfArgs;Purely cosmetic, but it makes the intent of
kgArgsclearer.Also applies to: 59-67, 68-92
contracts/src/KlerosGovernor.sol (1)
273-291: Consider usingsafeSendfor withdrawal refunds for consistency
withdrawTransactionListstill refunds the deposit with a raw.transfer:reservedETH -= submission.deposit; payable(msg.sender).transfer(submission.deposit);Every other ETH outbound path in this contract now goes through
SafeSendto avoid bricking calls when the recipient is a contract without a payable receive/fallback. For consistency and resilience, you could align this path as well:- reservedETH -= submission.deposit; - payable(msg.sender).transfer(submission.deposit); + reservedETH -= submission.deposit; + payable(msg.sender).safeSend(submission.deposit, wNative);Not strictly a regression, but it makes refunds behave the same way as the rest of the payout logic.
📜 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 (14)
contracts/.env.example(1 hunks)contracts/.gitignore(1 hunks)contracts/README.md(1 hunks)contracts/deploy/00-governor-v2.ts(2 hunks)contracts/deploy/utils/disputeTemplate.ts(2 hunks)contracts/deploy/utils/getContracts.ts(1 hunks)contracts/hardhat.config.ts(6 hunks)contracts/package.json(2 hunks)contracts/scripts/dotenv.sh(1 hunks)contracts/scripts/verifyProxies.sh(1 hunks)contracts/src/KlerosGovernor.sol(16 hunks)web/src/app/(main)/governor/[governorAddress]/ActiveLists/ListCard.tsx(3 hunks)web/src/app/(main)/governor/[governorAddress]/ActiveLists/index.tsx(3 hunks)web/src/consts/governors.ts(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- contracts/.gitignore
- contracts/hardhat.config.ts
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-06-15T18:23:26.791Z
Learnt from: tractorss
Repo: kleros/governor-v2 PR: 10
File: contracts/deploy/utils/disputeTemplate.ts:42-44
Timestamp: 2025-06-15T18:23:26.791Z
Learning: In Kleros Governor dispute template data mappings, the "seek" array values should be strings (e.g., "2") rather than numbers because they represent path strings that the SDK splits on, not direct array indices for indexing.
Applied to files:
contracts/deploy/utils/disputeTemplate.ts
📚 Learning: 2025-06-15T17:21:38.772Z
Learnt from: tractorss
Repo: kleros/governor-v2 PR: 10
File: web/src/utils/txnBuilder/constructSubmissionData.ts:5-11
Timestamp: 2025-06-15T17:21:38.772Z
Learning: In the Kleros Governor v2 web application, input validation is handled at the form level rather than in utility functions like constructSubmissionData. The team prefers to centralize validation at entry points and fix any validation leaks at the form level when discovered.
Applied to files:
contracts/src/KlerosGovernor.sol
🧬 Code graph analysis (3)
web/src/app/(main)/governor/[governorAddress]/ActiveLists/ListCard.tsx (1)
web/src/hooks/useFetchSubmittedLists.ts (1)
Submission(23-33)
web/src/app/(main)/governor/[governorAddress]/ActiveLists/index.tsx (2)
web/src/hooks/useFetchSubmittedLists.ts (2)
Submission(23-33)useFetchSubmittedLists(36-88)web/src/utils/index.ts (1)
isUndefined(39-40)
contracts/deploy/00-governor-v2.ts (2)
contracts/deploy/utils/getContracts.ts (1)
getArbitratorContracts(11-20)contracts/deploy/utils/disputeTemplate.ts (2)
templateFn(2-30)dataMappings(32-65)
🪛 dotenv-linter (4.0.0)
contracts/.env.example
[warning] 1-1: [SpaceCharacter] The line has spaces around equal sign
(SpaceCharacter)
[warning] 2-2: [SpaceCharacter] The line has spaces around equal sign
(SpaceCharacter)
[warning] 2-2: [UnorderedKey] The MAINNET_PRIVATE_KEY key should go before the PRIVATE_KEY key
(UnorderedKey)
[warning] 3-3: [SpaceCharacter] The line has spaces around equal sign
(SpaceCharacter)
[warning] 3-3: [UnorderedKey] The INFURA_API_KEY key should go before the MAINNET_PRIVATE_KEY key
(UnorderedKey)
[warning] 4-4: [QuoteCharacter] The value has quote characters (', ")
(QuoteCharacter)
[warning] 4-4: [UnorderedKey] The ETHERSCAN_API_KEY_FIX key should go before the INFURA_API_KEY key
(UnorderedKey)
[warning] 5-5: [EndingBlankLine] No blank line at the end of the file
(EndingBlankLine)
[warning] 5-5: [QuoteCharacter] The value has quote characters (', ")
(QuoteCharacter)
[warning] 5-5: [UnorderedKey] The ETHERSCAN_API_KEY key should go before the ETHERSCAN_API_KEY_FIX key
(UnorderedKey)
🪛 markdownlint-cli2 (0.18.1)
contracts/README.md
85-85: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
91-91: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
99-99: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
107-107: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
129-129: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
135-135: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
🪛 Shellcheck (0.11.0)
contracts/scripts/verifyProxies.sh
[warning] 13-13: Iterating over ls output is fragile. Use globs.
(SC2045)
contracts/scripts/dotenv.sh
[warning] 13-13: The surrounding quotes actually unquote this. Remove or escape them.
(SC2027)
🔇 Additional comments (9)
contracts/package.json (1)
60-60: Version bumps look consistent with the rest of the PRUpdating
@nomicfoundation/hardhat-verifyand@kleros/kleros-v2-contractsaligns with the new deployment/verification and arbitration flows; I don’t see issues in this file itself. Please just confirm that the new deployment names / APIs in@kleros/kleros-v2-contracts@^2.0.0-rc.1(e.g.mainnetvsmainnetNeo) match how you’re using them ingetContracts.tsand deploy scripts.Also applies to: 93-93
contracts/deploy/utils/getContracts.ts (1)
5-9:arbitrum→mainnetdeployment mapping: confirm it matches kleros-v2 contract artifactsSwitching the
NETWORK_TO_DEPLOYMENTentry forarbitrumto"mainnet"makes sense with the new@kleros/kleros-v2-contractsversion, but it does tightly couple this file to the deployment naming in that package. IfmainnetNeoartifacts still exist or naming changes again,getArbitratorContractswill start throwing.Please double‑check that:
DeploymentNamefor Arbitrum mainnet is indeed"mainnet"in@kleros/kleros-v2-contracts@^2.0.0-rc.1, and- The
deployments/arbitrumfolder here lines up with that expectation.Otherwise this file looks good.
contracts/deploy/utils/disputeTemplate.ts (1)
5-6: NewsessionIndexmapping is consistent with governor changes; confirm"value"seek pathThe template update to reference
sessionIndexin the description and the newdataMappingsentry that:
- calls
arbitratorDisputeIDToSessionIndex(arbitratorDisputeID),populates"sessionIndex", and- then feeds
{{{sessionIndex}}}into the existinggetSessioncallis conceptually sound and lines up with the new
arbitratorDisputeIDToSessionIndexmapping on the contract.Also, using string values in
seek(["value"]for the first call and["2"]forgetSession) follows the earlier guidance that these are path strings, not numeric indices. Just make sure"value"matches the actual response shape expected by the dispute-template SDK forabi/callresults.Overall, this looks correct.
Based on learnings
Also applies to: 34-56
web/src/app/(main)/governor/[governorAddress]/ActiveLists/index.tsx (2)
59-60: ListCard integration aligns with externalized open handlerPassing
setIsOpen={handleOpen}intoListCardfor both last‑session and active lists matches the new pattern of delegating modal control to the parent. The props spread{ governorAddress, list }remains type‑safe and backward compatible.Also applies to: 75-76
77-84: ExamineModal props wiring is coherent with new stateConditionally rendering
ExamineModalwhenopenListis set and passing the currentlistplustoggleIsOpen={closePopup}cleanly reflects the view state. This should work well with the deep‑link auto‑open behavior from the effect above oncelistIdparsing is hardened.contracts/src/KlerosGovernor.sol (4)
215-271:submitListchecks and duplicate detection look correct nowThe revamped
submitListlogic reads clean and defensive:
- Length mismatches
_target.length != _value.length/_target.length != _dataSize.lengthcorrectly revert with specific errors.- The deposit is computed as
submissionBaseDeposit + arbitrator.arbitrationCost(arbitratorExtraData)and enforced viaInsufficientDeposit.- The hash chain
listHashand the duplicate guard:if (alreadySubmitted[sessions.length - 1][listHash]) revert ListAlreadySubmitted(); alreadySubmitted[sessions.length - 1][listHash] = true;correctly prevent resubmitting the same list in a session.
- Remainder refunds use
safeSend(remainder, wNative)to avoid.transferpitfalls for contracts.This section now matches the intended behavior and integrates wNative/SafeSend safely.
293-327: Dispute flow updates (mapping + SafeSend) are coherentThe dispute/approval flow changes appear consistent:
executeSubmissionsnow:
- Uses
AlreadyDisputedwhensession.status != Status.NoDispute.- Pays the single‑list submitter with
submission.submitter.safeSend(sumDeposit, wNative);.- Records
arbitratorDisputeIDToSessionIndex[session.disputeID] = sessions.length - 1;when creating a dispute, matching the new view mapping consumed bydataMappings.
rule:
- Restricts entry to
msg.sender == address(arbitrator)withOnlyArbitratorAllowed.- Enforces
_ruling <= session.submittedLists.lengthandsession.status == Status.DisputeCreated.- Uses
safeSend(session.sumDeposit, wNative)to pay the winning submitter, then correctly zeroessession.sumDepositand decreasesreservedETH.The flow remains single‑dispute‑at‑a‑time (via session status), and the introduced mapping is a useful read‑path hook without affecting core logic.
Also applies to: 329-356
362-378: Execution and expendable‑funds helpers look correct under stated invariants
executeTransactionListnow:
- Checks
SubmissionNotApprovedandTimeToExecuteHasPassedvia custom errors.- Uses
getExpendableFunds()per iteration, and guards against re‑entrancy by reverting withAlreadyExecutediftransaction.executedis set during the external call.
getExpendableFundsreturnsaddress(this).balance - reservedETH, which is correct assumingreservedETHis always kept ≤ balance as you already do when adjusting it in submission/withdraw/dispute flows.Given those invariants, the new logic is sound and doesn’t introduce obvious re‑entrancy or accounting issues.
Also applies to: 380-387
389-455: View helpers and custom errors are a solid API improvementThe added view functions (
getTransactionInfo,getSubmittedLists,getSubmission,getNumberOfTransactions,getNumberOfCreatedLists,getCurrentSessionNumber,getSession,getTotalSubmissionDeposit) give off‑chain code a clean, gas‑efficient way to inspect state without reconstructing it manually.Grouping all the new custom errors at the bottom also improves readability and gas behavior versus string reverts.
No issues from a correctness perspective here.
Also applies to: 456-478
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (10)
web/src/app/(main)/governor/[governorAddress]/ActiveLists/index.tsx (4)
19-23: URL-driven auto-open logic is now safe; consider making URL the single source of truthThe guarded
BigInt(listId)withtry/catchand!isUndefinedchecks looks solid and avoids crashes from malformed or missinglistId. However,openListis only ever set (never cleared) in this effect, so the modal can stay open even if the user navigates to a URL withoutlistIdor with an invalid one (e.g., via browser back or manual URL edits).If you want the URL to fully control which list is open, consider extending the effect to also clear
openListwheneverlistIdisnull/invalid or no matching list is found, and, conversely, havehandleOpen/closePopupupdatelistIdin the URL so state and URL can’t drift apart.Also applies to: 27-36
38-41: Optionally synchandleOpenwith thelistIdquery param
handleOpencorrectly centralizes modal control viaopenList, but it doesn’t update the URL, while the effect reads fromlistIdto auto-open. This asymmetry means deep-links work, but clicking a card doesn’t produce a shareable URL.If that’s desirable UX, consider having
handleOpenalso setlistIdin the query string (e.g., viarouter.replace/push), usinglist.listId.toString(). That would keep the URL and UI in sync and make list openings reproducible via navigation.
42-45: Avoid blowing away unrelated query params when closing the modal
closePopupcurrently callsrouter.replace(window.location.pathname, { scroll: false }), which clears all query parameters, not justlistId. That can be surprising if other filters/search params are added later.Consider deriving the current pathname/search from Next helpers and only removing
listId, e.g. usingusePathname+useSearchParamsto rebuild a URL withoutlistId, thenrouter.replace(newUrl, { scroll: false }). This keeps modal state decoupled from any future query parameters.
80-87: SimplifyisOpenhandling forExamineModalGiven the surrounding conditional
{openList ? ( ... ) : null},openListis guaranteed to be truthy whenExamineModalrenders, soisOpen={!isUndefined(openList)}will always betruein practice.You can either:
- Derive a boolean once (e.g.
const isModalOpen = !isUndefined(openList);) and use it both for the conditional andisOpen, or- Rely solely on the conditional render and hardcode
isOpen={true}(ifExamineModaldoesn’t need a separate closed-but-mounted state).Either approach reduces duplicated state and makes the intent clearer.
contracts/.env.example (1)
1-5: Env example matches config; linter warnings are purely stylisticThe variable names line up with
hardhat.config.ts, so this is functionally fine. Ifdotenv-linterwarnings are enforced in CI, consider reordering keys alphabetically and adding a trailing newline to silence theUnorderedKey/EndingBlankLinenotices; otherwise you can safely ignore them.contracts/README.md (1)
69-79: Clarify the roles ofETHERSCAN_API_KEYvsETHERSCAN_API_KEY_FIXBoth keys are documented with the same description, which makes it unclear when each is actually used (e.g.,
verify-allvs proxy verification or other flows). Consider briefly explaining which tooling reads each variable, or consolidating if only one is needed, so users don’t have to guess which key to set.contracts/README.md.template (1)
63-65: Keep template aligned with README and clarify ETHERSCAN env varsThe updated template correctly mirrors the KlerosGovernor tag and new ETHERSCAN variables, but it carries the same ambiguity as the main README about how
ETHERSCAN_API_KEYvsETHERSCAN_API_KEY_FIXare used. Since this file is the source for generated docs, it’s a good place to add a short note explaining which tooling reads each variable so downstream READMEs stay clear.Also applies to: 80-81, 104-105, 118-119
contracts/package.json (1)
41-41: Verify-all wiring and RC dependency bumpThe
etherscan-verifyscript now cleanly targets the newverify-alltask, which matches the README examples. The bump to@nomicfoundation/hardhat-verifyand@kleros/kleros-v2-contracts@^2.0.0-rc.1looks intentional, but please double-check compatibility (especially with Hardhat 2.22.x) and that you’re comfortable depending on an RC before tagging a stable release.Also applies to: 60-60, 93-93
contracts/tasks/verify-all.ts (1)
1-42: verify-all task looks solid; consider skipping imported deployments and hardening error matchThe task correctly walks Hardhat-Deploy deployments, supports an optional
--contractfilter, and skips proxies by naming convention, which is a reasonable default. Two optional refinements you might consider:
- If Hardhat-Deploy exposes a way to distinguish imported/external deployments (e.g. coming from
@kleros/kleros-v2-contractsviaexternal.deployments), you may want to skip those to avoid noisy failures or redundant verification attempts.- The
"Already Verified"check is brittle against small message changes; a case-insensitive match or checking for a shorter, stable substring (like"already verified") would make the script more resilient across plugin versions.contracts/hardhat.config.ts (1)
5-5: Etherscan config and verify-all wiring look correct; watch the dual API key envsImporting
@nomicfoundation/hardhat-verifyand./tasks/verify-allcorrectly wires up the new verification flow, and the top-leveletherscan.apiKeyusingETHERSCAN_API_KEY_FIXmatches the new env/example setup. Networkverify.etherscan.apiKeyblocks still useETHERSCAN_API_KEY, which is fine but means you now rely on two different env vars for verification depending on the path (verify-all vs other verify usages/proxy script).It’s worth confirming that:
- Both env vars are set appropriately in local and CI environments, and
- This split is intentional (e.g., one key via a new gateway vs legacy per-network keys).
If the distinction is purely historical, you might eventually simplify to a single env var to reduce configuration drift.
Also applies to: 16-16, 69-71, 83-85, 95-97, 133-135, 148-155
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
contracts/.env.example(1 hunks)contracts/README.md(1 hunks)contracts/README.md.template(4 hunks)contracts/deploy/00-governor-v2.ts(3 hunks)contracts/hardhat.config.ts(7 hunks)contracts/package.json(3 hunks)contracts/tasks/verify-all.ts(1 hunks)web/src/app/(main)/governor/[governorAddress]/ActiveLists/index.tsx(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- contracts/deploy/00-governor-v2.ts
🧰 Additional context used
🧬 Code graph analysis (1)
web/src/app/(main)/governor/[governorAddress]/ActiveLists/index.tsx (2)
web/src/hooks/useFetchSubmittedLists.ts (2)
Submission(23-33)useFetchSubmittedLists(36-88)web/src/utils/index.ts (1)
isUndefined(39-40)
🪛 dotenv-linter (4.0.0)
contracts/.env.example
[warning] 1-1: [QuoteCharacter] The value has quote characters (', ")
(QuoteCharacter)
[warning] 2-2: [QuoteCharacter] The value has quote characters (', ")
(QuoteCharacter)
[warning] 2-2: [UnorderedKey] The MAINNET_PRIVATE_KEY key should go before the PRIVATE_KEY key
(UnorderedKey)
[warning] 3-3: [QuoteCharacter] The value has quote characters (', ")
(QuoteCharacter)
[warning] 3-3: [UnorderedKey] The INFURA_API_KEY key should go before the MAINNET_PRIVATE_KEY key
(UnorderedKey)
[warning] 4-4: [QuoteCharacter] The value has quote characters (', ")
(QuoteCharacter)
[warning] 4-4: [UnorderedKey] The ETHERSCAN_API_KEY_FIX key should go before the INFURA_API_KEY key
(UnorderedKey)
[warning] 5-5: [EndingBlankLine] No blank line at the end of the file
(EndingBlankLine)
[warning] 5-5: [QuoteCharacter] The value has quote characters (', ")
(QuoteCharacter)
[warning] 5-5: [UnorderedKey] The ETHERSCAN_API_KEY key should go before the ETHERSCAN_API_KEY_FIX key
(UnorderedKey)
🪛 markdownlint-cli2 (0.18.1)
contracts/README.md
84-84: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
90-90: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
98-98: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
106-106: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
128-128: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
134-134: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
🔇 Additional comments (1)
web/src/app/(main)/governor/[governorAddress]/ActiveLists/index.tsx (1)
61-63: Centralizing modal open viasetIsOpen={handleOpen}looks goodPassing
setIsOpen={handleOpen}toListCardfor both last session and active lists is a nice consolidation of modal behavior. It keeps list cards dumb and moves the open/close logic into a single place (ActiveLists), which will make future changes around opening behavior much easier.Also applies to: 75-79
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/deploy/00-governor-v2.ts (1)
33-57: Eliminate code duplication by usinggfArgsin the factory deploy call.The
gfArgsarray is defined (lines 33-44) but then the identical arguments are duplicated in thegovernorFactory.deploycall (lines 46-57). This violates DRY and creates maintenance risk—if parameters change, both locations must be updated.Apply this diff to use the spread operator:
- await governorFactory.deploy( - klerosCore.target, - extraData, - disputeTemplateRegistry.target, - disputeTemplate, - dataMappings, - 0, - 600, - 600, - 600, // feeTimeout: 10 minutes, - wNative.target - ); + await governorFactory.deploy(...gfArgs);
♻️ Duplicate comments (3)
contracts/hardhat.config.ts (3)
72-84: Arbitrum Sepolia Devnet config is consistent with the new key strategyThis mirrors the
arbitrumSepoliachange and keeps the devnet using the sameETHERSCAN_API_KEY, which is good for consistency. Just ensure the devnet/deployer environment also hasETHERSCAN_API_KEYset and no longer expects a dedicated Arbiscan key.
85-97: Main Arbitrum network now also usingETHERSCAN_API_KEYSame pattern here: using
ETHERSCAN_API_KEYfor Arbitrum mainnet simplifies configuration and matches the Sepolia networks. As above, this relies on the API key being valid on Arbiscan via Etherscan’s multi-chain API system; worth confirming this with your currently issued key before merge.
123-135: GnosisChain verification also unified underETHERSCAN_API_KEYGnosisChain now uses the shared
ETHERSCAN_API_KEYas well. This keeps all Etherscan-family explorers under one env var, which is nice, but increases coupling: rotating or changing keys now impacts multiple networks at once.Make sure:
- The new key is recognized by Gnosisscan.
- Any previous
GNOSISSCAN_API_KEYusage has been removed from scripts/docs so there’s no confusion.
🧹 Nitpick comments (1)
contracts/deploy/00-governor-v2.ts (1)
59-62: Consider usinggfArgsdirectly instead of aliasing.The alias
kgArgs = gfArgsadds an extra variable without clear benefit. Unless there's a future need to differentiate these arguments, you could usegfArgsdirectly in the deployment.- const kgArgs = gfArgs; await deploy("KlerosGovernor", { from: deployer, - args: kgArgs, + args: gfArgs, log: true, });
📜 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 (3)
contracts/deploy/00-governor-v2.ts(3 hunks)contracts/hardhat.config.ts(4 hunks)contracts/package.json(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- contracts/package.json
🧰 Additional context used
🧬 Code graph analysis (1)
contracts/deploy/00-governor-v2.ts (1)
contracts/deploy/utils/disputeTemplate.ts (1)
dataMappings(32-65)
🔇 Additional comments (3)
contracts/deploy/00-governor-v2.ts (2)
31-31: Good improvement: typed contract reference.Adding the generic type parameter improves type safety and developer experience.
21-21: > Likely an incorrect or invalid review comment.contracts/hardhat.config.ts (1)
58-71: Consolidating Arbitrum Sepolia ontoETHERSCAN_API_KEYlooks correct; confirm multi‑chain key + env wiringUsing
process.env.ETHERSCAN_API_KEYhere aligns this network with the new single Etherscan-style API key strategy and should work fine with Etherscan’s multi-chain keys, assuming that key is enabled for Arbitrum’s explorer.Please double-check:
- The configured
ETHERSCAN_API_KEYis a multi-chain key that covers Arbitrum Sepolia.- All relevant
.envtemplates, CI secrets, and deployment environments now provideETHERSCAN_API_KEY(and no longer rely on legacyARBISCAN_API_KEY).
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
contracts/deploy/00-governor-v2.ts (1)
33-57: Remove duplicated arguments by usinggfArgsforgovernorFactory.deploy().The
gfArgsarray is constructed at lines 33-44 but then the same arguments are passed inline togovernorFactory.deploy()at lines 46-57. This duplication creates a maintenance risk where updates to one location may not be reflected in the other.🔎 Apply this diff to use gfArgs consistently:
const gfArgs = [ klerosCore.target, extraData, disputeTemplateRegistry.target, disputeTemplate, dataMappings, 0, 600, 600, 600, // feeTimeout: 10 minutes wNative.target, - ]; + ] as const; - await governorFactory.deploy( - klerosCore.target, - extraData, - disputeTemplateRegistry.target, - disputeTemplate, - dataMappings, - 0, - 600, - 600, - 600, // feeTimeout: 10 minutes, - wNative.target - ); + await governorFactory.deploy(...gfArgs);
🧹 Nitpick comments (2)
contracts/README.md (1)
9-24: Empty deployment sections should be populated or removed.The Official Testnet sections (Arbitrum Sepolia at line 11, Sepolia at line 13) and Devnet Sepolia (line 23) are empty placeholders. Consider either populating them with actual deployment addresses or removing them until deployments are available to avoid confusion.
contracts/deploy/00-governor-v2.ts (1)
59-64: Consider adding inline comments for timeout values.The
gfArgsarray contains multiple timeout values (600) with only the last one commented. For better maintainability, document all timeout parameters.🔎 Suggested improvement:
const gfArgs = [ klerosCore.target, extraData, disputeTemplateRegistry.target, disputeTemplate, dataMappings, - 0, - 600, - 600, - 600, // feeTimeout: 10 minutes + 0, // submissionDeposit + 600, // submissionTimeout: 10 minutes + 600, // executionTimeout: 10 minutes + 600, // feeTimeout: 10 minutes wNative.target, ] as const;
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (3)
contracts/README.md(1 hunks)contracts/deploy/00-governor-v2.ts(4 hunks)web/src/consts/governors.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- web/src/consts/governors.ts
🧰 Additional context used
🧬 Code graph analysis (1)
contracts/deploy/00-governor-v2.ts (1)
contracts/deploy/utils/disputeTemplate.ts (1)
dataMappings(32-65)
🪛 markdownlint-cli2 (0.18.1)
contracts/README.md
84-84: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
90-90: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
98-98: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
106-106: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
128-128: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
134-134: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
⏰ 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-governor-v2
- GitHub Check: Header rules - kleros-governor-v2
- GitHub Check: Pages changed - kleros-governor-v2
🔇 Additional comments (1)
contracts/deploy/00-governor-v2.ts (1)
21-22: LGTM!The WETH contract retrieval pattern is correct and aligns with the SafeSend integration for native token handling.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
contracts/README.md.template (1)
64-65: Clarify the purpose ofETHERSCAN_API_KEY_FIX.Both
ETHERSCAN_API_KEYandETHERSCAN_API_KEY_FIXare listed with identical descriptions. This creates confusion about which variable to use and when.contracts/README.md (1)
77-78: Clarify the purpose ofETHERSCAN_API_KEY_FIX.Both environment variables have identical descriptions, creating confusion about their distinct purposes.
🧹 Nitpick comments (2)
contracts/README.md (2)
84-84: Consider using proper headings for Shell labels.Static analysis flags the bold text format for "Shell 1:" and "Shell 2:" labels. While this doesn't affect readability, using proper markdown headings (e.g.,
##### Shell 1: the node) would align with markdown best practices and silence linter warnings.Also applies to: 90-90, 98-98, 106-106, 128-128, 134-134
126-126: Consider numbering the "Running Test Fixtures" section.The section numbering jumps from 4 to 5 with "Running Test Fixtures" unnumbered in between. While this may be intentional, sequential numbering (making this section 5 and renumbering verification to 6) would improve navigation consistency.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
contracts/README.md(1 hunks)contracts/README.md.template(5 hunks)
🧰 Additional context used
🪛 markdownlint-cli2 (0.18.1)
contracts/README.md
84-84: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
90-90: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
98-98: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
106-106: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
128-128: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
134-134: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
⏰ 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-governor-v2
- GitHub Check: Header rules - kleros-governor-v2
- GitHub Check: Pages changed - kleros-governor-v2
🔇 Additional comments (5)
contracts/README.md.template (4)
80-80: LGTM: GovernorV2 → KlerosGovernor update.The deployment tag reference has been correctly updated to
KlerosGovernor, consistent with the contract refactoring described in this PR.
104-104: LGTM: Deploy command updated.The deployment command correctly references
KlerosGovernortag.
118-118: LGTM: Test fixture tag updated.The node command correctly references
KlerosGovernortag for test fixtures.
127-127: Section numbering corrected.The verification section has been correctly renumbered to 5, addressing the sequential numbering of deployment instructions.
contracts/README.md (1)
19-21: Deployment addresses documented.The devnet deployment addresses for GovernorFactory, KlerosGovernor, and WETH on Arbitrum Sepolia are properly formatted with block explorer links.
2 participants
WIPSummary by CodeRabbit
New Features
Improvements
Updates
✏️ Tip: You can customize this high-level summary in your review settings.