feat: pre-commit type checking and lint - contracts#2278
Open
kyrers wants to merge 10 commits into
Open
Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds/updates linting and TypeScript/tooling (ESLint, lint-staged, pre-commit TypeScript check, wagmi .mts configs), broad code formatting/type-tightening across many contract scripts/tests, and refactors the dispute relayer to parse cross-chain dispute events and call HomeGateway with a new param struct shape. ChangesLinting, TypeScript & Tooling
Dispute Relayer & Cross-chain Event Flow
Sequence Diagram(s)sequenceDiagram
participant PreCommit as Pre-commit Hook
participant Git as Git Staging
participant Contracts as Contracts Workspace
participant TSCheck as TypeChecker (yarn workspace check-types)
participant Lint as lint-staged (ESLint/Prettier)
Git->>PreCommit: commit staged
PreCommit->>Contracts: inspect staged paths
alt contracts/ files present
PreCommit->>TSCheck: run yarn workspace `@kleros/kleros-v2-contracts` check-types
TSCheck-->>PreCommit: type check result
PreCommit->>Lint: run lint-staged
Lint-->>PreCommit: lint/format result
end
PreCommit-->>Git: allow or block commit
sequenceDiagram
participant ForeignGateway as Foreign Gateway
participant Relayer as Dispute Relayer
participant EventSub as Event Listener
participant ReceiptSvc as RPC / Receipt Fetcher
participant Parser as Log Parser
participant HomeGateway as Home Gateway
ForeignGateway->>EventSub: emit CrossChainDisputeOutgoing (tx)
EventSub->>Relayer: deliver eventLog (with txHash)
Relayer->>ReceiptSvc: fetch full receipt by txHash
ReceiptSvc-->>Relayer: full transaction receipt with logs
Relayer->>Parser: filter/parse DisputeRequest log → DisputeRequestEvent.OutputObject
Parser-->>Relayer: parsed dispute params (templateId, choices, extraData, etc.)
Relayer->>HomeGateway: call relayCreateDispute(paramsStruct[, cost])
HomeGateway-->>Relayer: tx response (relayCreateDispute tx hash)
Relayer->>Relayer: log relayCreateDispute txId
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Suggested labels
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@contracts/package.json`:
- Around line 106-107: The package declares Node engine range that allows Node
16 but you run ESLint 9.15.0 via the "check-style" script which requires Node
>=18.18.0; update the package.json "engines" -> "node" entry to ">=18.18.0" so
the declared Node runtime matches ESLint 9.15.0's requirements and avoids
failures when running "check-style".
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 01ebbe38-20a3-47b2-8b1d-9b8d9bf76289
📒 Files selected for processing (6)
.husky/pre-commitcontracts/.eslintignorecontracts/.lintstagedrc.jsoncontracts/eslint.config.mjscontracts/package.jsoncontracts/tsconfig.json
💤 Files with no reviewable changes (1)
- contracts/.eslintignore
Contributor
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
contracts/deploy/05-dispute-kit-acp.ts (1)
43-86:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRestore the court enablement step.
This script now deploys the ACP kit but never registers it against
consumerProtectionCourtID, so the kit will stay inactive after deployment.Suggested fix
- const { courtUrl } = config[hre.network.name as keyof typeof config]; + const { consumerProtectionCourtID, courtUrl } = config[hre.network.name as keyof typeof config];- // const disputeKitID = (await core.getDisputeKitsLength()) - 1n; - // console.log(`core.enableDisputeKits(${consumerProtectionCourtID}, ${[disputeKitID]}, true)`); - // await core.enableDisputeKits(consumerProtectionCourtID, [disputeKitID], true); + const disputeKitID = (await core.getDisputeKitsLength()) - 1n; + await core.enableDisputeKits(consumerProtectionCourtID, [disputeKitID], true);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@contracts/deploy/05-dispute-kit-acp.ts` around lines 43 - 86, The deployment registers the dispute kit via core.addNewDisputeKit but never enables it for consumerProtectionCourtID; restore the enablement by computing the new dispute kit ID after registration (use disputeKitID = (await core.getDisputeKitsLength()) - 1n) and then call await core.enableDisputeKits(consumerProtectionCourtID, [disputeKitID], true) (optionally log the enablement). Ensure you reference the deployed disputeKit variable, use await on getDisputeKitsLength and enableDisputeKits, and call these methods on the core instance.
🧹 Nitpick comments (1)
contracts/scripts/wagmiHelpers.ts (1)
10-12: ⚡ Quick winValidate
artifactbefore reading.abi.The
unknownparameter is a step in the right direction, but the immediate cast still accepts malformed inputs and can returnundefinedwithout a clear error. A small runtime check would make the helper’s contract explicit.♻️ Proposed fix
export const getAbi = (artifact: unknown) => { - return (artifact as ArtifactPartial).abi; + if ( + artifact === null || + typeof artifact !== "object" || + !("abi" in artifact) + ) { + throw new Error("Artifact is missing an abi"); + } + + return (artifact as ArtifactPartial).abi; };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@contracts/scripts/wagmiHelpers.ts` around lines 10 - 12, The getAbi helper currently casts an unknown artifact to ArtifactPartial and returns .abi without validation; update getAbi to first verify the input is an object and that (artifact as ArtifactPartial).abi exists and is an array (or otherwise matches the expected ABI shape), and if not throw a clear Error mentioning getAbi and the invalid artifact; reference the getAbi function and the ArtifactPartial type when making the change so callers receive a descriptive runtime error instead of undefined.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@contracts/scripts/console-init.ts`:
- Around line 261-265: The finally block calls sender.queryFilter(filter,
tx.blockNumber, tx.blockNumber) unconditionally and will throw if
sender.sendBatch reverted and tx is undefined; update the finally logic to first
check that the transaction receipt variable (tx) is truthy before attempting to
read tx.blockNumber and perform the log query (i.e., only call
sender.queryFilter when tx exists), otherwise skip the log lookup so the
original error from sendBatch isn't masked.
In `@contracts/scripts/disputeRelayerBot.ts`:
- Around line 94-107: The current code always emits logger.warn after collecting
DisputeRequest events; change it to only warn when there are actually multiple
events by checking disputeRequests.length > 1 before calling logger.warn, and
ensure the code still processes the first event and skips the rest; reference
the disputeRequests array, arbitrableInterface.getEventTopic("DisputeRequest")
parsing logic, and the logger.warn call so you update the condition around that
warn and any logic that selects which DisputeRequestEventObject to use.
In `@contracts/scripts/keeperBot.ts`:
- Around line 585-606: The function always returns success=false even after a
successful executeDelayedStakes transaction; update the logic so success is set
to true on the happy path—e.g., after the awaited transaction confirmation (the
result of (await sortition.executeDelayedStakes(delayedStakes, { gasLimit: gas
})).wait() assigned to tx) set success = true before logging and returning; keep
the existing staticCall/estimateGas/handleError flow and only return false on
the early staticCall failure or in the catch branch.
In `@contracts/scripts/populateCourts.ts`:
- Around line 162-164: The calculation of end treats taskArgs.maxNumberOfCourts
as truthy so a value of 0 is ignored; change the conditional to explicitly check
for undefined/null (e.g., taskArgs.maxNumberOfCourts != null or typeof
taskArgs.maxNumberOfCourts === "number") when computing end so that end = start
+ taskArgs.maxNumberOfCourts is used when the caller passes 0, falling back to
courtsV2.length only when the argument is absent; adjust any related logic
around start/end to handle a zero limit correctly.
In `@contracts/scripts/populatePolicyRegistry.ts`:
- Around line 120-126: The code currently treats taskArgs.maxNumberOfCourts as
truthy so a value of 0 is ignored; change the guard to check for undefined (or
null) instead of truthiness: compute end using something like
(taskArgs.maxNumberOfCourts !== undefined) ? start + taskArgs.maxNumberOfCourts
: policiesV2.length, then keep the existing console.log and policiesV2 =
policiesV2.slice(start, end) so a 0 limit is correctly respected; update
references to taskArgs.maxNumberOfCourts, start, end, and policiesV2
accordingly.
In `@contracts/test/proxy/index.ts`:
- Around line 147-150: The teardown calls proxy.upgradeToAndCall with a fallback
empty string which will cause a revert if proxyDeployment.implementation is
missing; remove the `?? ""` fallback and pass proxyDeployment.implementation
directly (or assert/throw if it's undefined) so upgradeToAndCall always receives
a valid implementation address—update the call in the teardown to use
proxyDeployment.implementation as-is or add an explicit runtime check before
calling proxy.upgradeToAndCall.
---
Outside diff comments:
In `@contracts/deploy/05-dispute-kit-acp.ts`:
- Around line 43-86: The deployment registers the dispute kit via
core.addNewDisputeKit but never enables it for consumerProtectionCourtID;
restore the enablement by computing the new dispute kit ID after registration
(use disputeKitID = (await core.getDisputeKitsLength()) - 1n) and then call
await core.enableDisputeKits(consumerProtectionCourtID, [disputeKitID], true)
(optionally log the enablement). Ensure you reference the deployed disputeKit
variable, use await on getDisputeKitsLength and enableDisputeKits, and call
these methods on the core instance.
---
Nitpick comments:
In `@contracts/scripts/wagmiHelpers.ts`:
- Around line 10-12: The getAbi helper currently casts an unknown artifact to
ArtifactPartial and returns .abi without validation; update getAbi to first
verify the input is an object and that (artifact as ArtifactPartial).abi exists
and is an array (or otherwise matches the expected ABI shape), and if not throw
a clear Error mentioning getAbi and the invalid artifact; reference the getAbi
function and the ArtifactPartial type when making the change so callers receive
a descriptive runtime error instead of undefined.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: f1b28245-dc65-4362-ba24-3b8598538c3d
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (83)
contracts/.solcover.jscontracts/deploy/00-home-chain-arbitration-mainnet.tscontracts/deploy/00-home-chain-arbitration-ruler.tscontracts/deploy/00-home-chain-arbitration.tscontracts/deploy/00-home-chain-resolver.tscontracts/deploy/00-rng-chainlink.tscontracts/deploy/00-rng-randomizer.tscontracts/deploy/01-foreign-gateway-on-ethereum.tscontracts/deploy/01-foreign-gateway-on-gnosis.tscontracts/deploy/02-home-gateway-to-ethereum.tscontracts/deploy/02-home-gateway-to-gnosis.tscontracts/deploy/03-vea-mock.tscontracts/deploy/04-foreign-arbitrable.tscontracts/deploy/04-klerosliquid-to-v2-gnosis.tscontracts/deploy/05-dispute-kit-acp.tscontracts/deploy/05-dispute-kit-gated-junin.tscontracts/deploy/change-arbitrable-dispute-template.tscontracts/deploy/change-sortition-module-rng.tscontracts/deploy/fix1148.tscontracts/deploy/upgrade-all.tscontracts/deploy/utils/deployTokens.tscontracts/deploy/utils/deployUpgradable.tscontracts/deploy/utils/getContractAddress.tscontracts/deploy/utils/getContractOrDeploy.tscontracts/deploy/utils/index.tscontracts/deploy/utils/klerosCoreHelper.tscontracts/deployments/contractsEthers.tscontracts/deployments/contractsViem.tscontracts/deployments/disputeKitsViem.tscontracts/deployments/index.tscontracts/deployments/utils.tscontracts/eslint.config.mjscontracts/hardhat.config.tscontracts/package.jsoncontracts/scripts/changeOwner.tscontracts/scripts/compareStorageLayout.tscontracts/scripts/console-init-chiado-resolver.tscontracts/scripts/console-init-chiado.tscontracts/scripts/console-init.tscontracts/scripts/disputeCreatorBot.tscontracts/scripts/disputeRelayerBot.tscontracts/scripts/disputeRelayerBotFromChiado.tscontracts/scripts/disputeRelayerBotFromHardhat.tscontracts/scripts/disputeRelayerBotFromSepolia.tscontracts/scripts/getCourtsV1.tscontracts/scripts/getDisputeTemplate.tscontracts/scripts/getPoliciesV1.tscontracts/scripts/keeperBot.tscontracts/scripts/keeperBotShutter.tscontracts/scripts/populateCourts.tscontracts/scripts/populatePolicyRegistry.tscontracts/scripts/shutter.tscontracts/scripts/storage-layout.tscontracts/scripts/utils/contracts.tscontracts/scripts/utils/env.tscontracts/scripts/utils/execution.tscontracts/scripts/utils/logger.tscontracts/scripts/utils/tx-builder.tscontracts/scripts/viemTest.tscontracts/scripts/wagmiHelpers.tscontracts/test/arbitration/dispute-kit-gated-shutter.tscontracts/test/arbitration/dispute-kit-gated.tscontracts/test/arbitration/dispute-kit-shutter.tscontracts/test/arbitration/draw.tscontracts/test/arbitration/helpers/dispute-kit-gated-common.tscontracts/test/arbitration/helpers/dispute-kit-shutter-common.tscontracts/test/arbitration/index.tscontracts/test/arbitration/ruler.tscontracts/test/arbitration/staking-neo.tscontracts/test/arbitration/staking.tscontracts/test/evidence/index.tscontracts/test/integration/getContractsEthers.test.tscontracts/test/integration/getContractsViem.test.tscontracts/test/integration/index.tscontracts/test/proxy/index.tscontracts/test/rng/index.tscontracts/test/sortition/index.tscontracts/test/utils/getActualAddress.test.tscontracts/test/utils/getActualAddress.tscontracts/wagmi.config.devnet.tscontracts/wagmi.config.hardhat.tscontracts/wagmi.config.mainnet.tscontracts/wagmi.config.testnet.ts
💤 Files with no reviewable changes (1)
- contracts/test/arbitration/dispute-kit-shutter.ts
✅ Files skipped from review due to trivial changes (58)
- contracts/test/arbitration/dispute-kit-gated.ts
- contracts/deployments/index.ts
- contracts/scripts/disputeRelayerBotFromChiado.ts
- contracts/scripts/storage-layout.ts
- contracts/deploy/utils/deployUpgradable.ts
- contracts/scripts/disputeRelayerBotFromHardhat.ts
- contracts/wagmi.config.mainnet.ts
- contracts/scripts/disputeRelayerBotFromSepolia.ts
- contracts/deploy/utils/index.ts
- contracts/deploy/change-arbitrable-dispute-template.ts
- contracts/wagmi.config.hardhat.ts
- contracts/test/utils/getActualAddress.test.ts
- contracts/wagmi.config.testnet.ts
- contracts/scripts/utils/tx-builder.ts
- contracts/scripts/console-init-chiado-resolver.ts
- contracts/wagmi.config.devnet.ts
- contracts/deploy/change-sortition-module-rng.ts
- contracts/scripts/getPoliciesV1.ts
- contracts/deploy/00-home-chain-resolver.ts
- contracts/.solcover.js
- contracts/scripts/disputeCreatorBot.ts
- contracts/scripts/getDisputeTemplate.ts
- contracts/deploy/utils/getContractAddress.ts
- contracts/deploy/00-rng-randomizer.ts
- contracts/scripts/utils/execution.ts
- contracts/deploy/utils/klerosCoreHelper.ts
- contracts/scripts/compareStorageLayout.ts
- contracts/deploy/04-klerosliquid-to-v2-gnosis.ts
- contracts/deploy/02-home-gateway-to-gnosis.ts
- contracts/deploy/01-foreign-gateway-on-ethereum.ts
- contracts/scripts/getCourtsV1.ts
- contracts/deploy/02-home-gateway-to-ethereum.ts
- contracts/deploy/01-foreign-gateway-on-gnosis.ts
- contracts/deploy/00-home-chain-arbitration-ruler.ts
- contracts/deploy/04-foreign-arbitrable.ts
- contracts/scripts/changeOwner.ts
- contracts/deploy/utils/getContractOrDeploy.ts
- contracts/deployments/disputeKitsViem.ts
- contracts/test/arbitration/index.ts
- contracts/scripts/console-init-chiado.ts
- contracts/test/utils/getActualAddress.ts
- contracts/deploy/utils/deployTokens.ts
- contracts/deploy/00-rng-chainlink.ts
- contracts/test/integration/index.ts
- contracts/deploy/03-vea-mock.ts
- contracts/deploy/fix1148.ts
- contracts/deployments/utils.ts
- contracts/hardhat.config.ts
- contracts/test/rng/index.ts
- contracts/test/arbitration/draw.ts
- contracts/scripts/shutter.ts
- contracts/test/arbitration/staking.ts
- contracts/test/arbitration/staking-neo.ts
- contracts/scripts/utils/contracts.ts
- contracts/test/arbitration/helpers/dispute-kit-shutter-common.ts
- contracts/deployments/contractsEthers.ts
- contracts/deploy/05-dispute-kit-gated-junin.ts
- contracts/test/integration/getContractsEthers.test.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- contracts/package.json
- contracts/eslint.config.mjs
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@contracts/deploy/04-klerosliquid-to-v2-gnosis.ts`:
- Line 25: Validate chainId before casting to ForeignChains: check membership
(e.g., if (!(chainId in ForeignChains)) throw new Error(...)) and only then
assign const foreignChain = chainId as ForeignChains; do not change existing Map
lookups from .get() to .has()—leave the .get() calls (used to fetch addresses)
intact; after fetching wPnkAddress via the .get() call, add a guard that throws
a clear error if wPnkAddress is undefined before using it so downstream code
cannot operate on an unmapped value (apply same validation pattern at the
locations referencing chainId, the .get() call at line ~28, and the wPnkAddress
usage around line ~48).
In `@contracts/scripts/disputeRelayerBot.ts`:
- Line 86: The JSON.stringify calls on eventLog, disputeRequest, and
relayCreateDisputeParams will throw on bigint fields; replace each
JSON.stringify(obj) with JSON.stringify(obj, bigintReplacer) and add a reusable
bigintReplacer function (e.g. (k,v) => typeof v === 'bigint' ? v.toString() : v)
in disputeRelayerBot.ts so numeric uint256/bigint values are serialized safely;
update the three logger.debug invocations that reference eventLog,
disputeRequest, and relayCreateDisputeParams to use this replacer.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 9067f4d9-4766-4554-a692-4459aba12e66
📒 Files selected for processing (6)
contracts/deploy/00-rng-chainlink.tscontracts/deploy/00-rng-randomizer.tscontracts/deploy/04-klerosliquid-to-v2-gnosis.tscontracts/deploy/utils/getContractAddress.tscontracts/scripts/disputeCreatorBot.tscontracts/scripts/disputeRelayerBot.ts
✅ Files skipped from review due to trivial changes (1)
- contracts/deploy/00-rng-chainlink.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- contracts/deploy/00-rng-randomizer.ts
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@contracts/deploy/04-klerosliquid-to-v2-gnosis.ts`:
- Around line 69-74: The console.log formatting in the deploy script passes
three values (wPnkAddress, foreignGateway.address, weth.address) but the format
string only has two %s placeholders, causing the WETH address to be unlabeled;
update the format string used in the console.log (the call that prints "using:
\nwPNK at %s, \nForeignGateway at %s") to include a third placeholder and label
for WETH (or switch to a template literal) so all three values (wPnkAddress,
foreignGateway.address, weth.address) are printed with clear labels.
In `@contracts/scripts/disputeRelayerBot.ts`:
- Around line 104-117: disputeRequests can be empty which makes
disputeRequests[0] undefined and later property access crash; update the
handling around the disputeRequests extraction (the block that builds
disputeRequests, logs, and sets disputeRequest) to check if
disputeRequests.length === 0 and early-return or skip processing, logging a
clear warning (use logger.warn) that no DisputeRequest event was found; ensure
subsequent code that uses disputeRequest (e.g., disputeRequest._templateId) only
runs when disputeRequest is defined, keeping references to
arbitrableInterface.parseLog and safeJson unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 9ca05adf-a21b-475e-9481-ed533cdc1337
📒 Files selected for processing (2)
contracts/deploy/04-klerosliquid-to-v2-gnosis.tscontracts/scripts/disputeRelayerBot.ts
Contributor
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
contracts/scripts/wagmiHelpers.mts (1)
39-39:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winFix typo in address type literal.
The template literal type is missing the
$character for interpolation. The correct viem address type is`0x${string}`, not`0x{string}`.Proposed fix
- [chain.id]: jsonContent.address as `0x{string}`, + [chain.id]: jsonContent.address as `0x${string}`,🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@contracts/scripts/wagmiHelpers.mts` at line 39, The address type literal is malformed on the assignment of jsonContent.address (the mapping entry using [chain.id]): replace the incorrect template literal type `0x{string}` with the correct TypeScript template literal `0x${string}` so the cast becomes jsonContent.address as `0x${string}` (or use the appropriate Address type) to satisfy viem's address type.
♻️ Duplicate comments (2)
contracts/deploy/04-klerosliquid-to-v2-gnosis.ts (1)
69-74:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winFix mismatched log placeholders.
The format string contains 2
%splaceholders but 3 values are passed (wPnkAddress,foreignGateway.address,weth.address). Theweth.addresswill be silently ignored byconsole.log.Proposed fix
console.log( - "using: \nwPNK at %s, \nForeignGateway at %s", + "using: \nwPNK at %s, \nForeignGateway at %s, \nWETH at %s", wPnkAddress, foreignGateway.address, weth.address, );🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@contracts/deploy/04-klerosliquid-to-v2-gnosis.ts` around lines 69 - 74, The console.log call that prints wPnkAddress, foreignGateway.address and weth.address has only two %s placeholders so the third value (weth.address) is ignored; update the console.log format string used in that console.log invocation to include three placeholders (e.g., add ", \nWETH at %s") or remove the extra argument so the number of %s matches the provided values—refer to the variables wPnkAddress, foreignGateway.address, weth.address and the console.log call to locate and fix it.contracts/scripts/disputeRelayerBot.ts (1)
104-116:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winGuard zero-or-many
DisputeRequestmatches before indexing.Line 116 still indexes
disputeRequests[0]without checking whether a matching log was found, and the warning at Lines 112-114 still fires on the happy path. Return early whenlength === 0, and only warn whenlength > 1.🛡️ Proposed fix
const disputeRequests: DisputeRequestEvent.OutputObject[] = fullTxReceipt.logs .filter((log: Log) => log.topics[0] === disputeRequestTopic) .map( (log: Log) => arbitrableInterface.parseLog(log)! .args as unknown as DisputeRequestEvent.OutputObject, ); - logger.warn( - `More than 1 DisputeRequest event: not supported yet, skipping the others events.`, - ); + if (disputeRequests.length === 0) { + logger.warn( + `No DisputeRequest event found in transaction ${eventLog.transactionHash}, skipping.`, + ); + return; + } + if (disputeRequests.length > 1) { + logger.warn( + `More than 1 DisputeRequest event: not supported yet, skipping the other events.`, + ); + } const disputeRequest = disputeRequests[0];🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@contracts/scripts/disputeRelayerBot.ts` around lines 104 - 116, Check the disputeRequests array before indexing: after building disputeRequests from fullTxReceipt.logs using disputeRequestTopic and arbitrableInterface.parseLog, if disputeRequests.length === 0 return/skip processing (no indexing), and only call logger.warn when disputeRequests.length > 1; then safely use disputeRequests[0] to set disputeRequest. This ensures you never access disputeRequests[0] when empty and only warn on duplicate events.
🧹 Nitpick comments (1)
contracts/test/integration/getContractsEthers.test.ts (1)
126-131: ⚡ Quick winAvoid hard-coding shared public RPC endpoints in the test file.
These URLs are rate-limited and occasionally flaky, so CI can fail for reasons unrelated to the contracts code. Reading them from env keeps the suite reproducible while still allowing local fallbacks.
Suggested change
- const arbitrumSepoliaProvider = new ethers.JsonRpcProvider( - "https://sepolia-rollup.arbitrum.io/rpc", - ); - const arbitrumProvider = new ethers.JsonRpcProvider( - "https://arb1.arbitrum.io/rpc", - ); + const arbitrumSepoliaProvider = new ethers.JsonRpcProvider( + process.env.ARBITRUM_SEPOLIA_RPC_URL ?? + "https://sepolia-rollup.arbitrum.io/rpc", + ); + const arbitrumProvider = new ethers.JsonRpcProvider( + process.env.ARBITRUM_RPC_URL ?? "https://arb1.arbitrum.io/rpc", + );🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@contracts/test/integration/getContractsEthers.test.ts` around lines 126 - 131, The test currently hard-codes public RPC endpoints when constructing ethers.JsonRpcProvider for arbitrumSepoliaProvider and arbitrumProvider; change these to read from environment variables (e.g., process.env.ARBITRUM_SEPOLIA_RPC, process.env.ARBITRUM_MAINNET_RPC) with the current URLs as local fallbacks, and use those values when calling new ethers.JsonRpcProvider(...) so CI can override endpoints while local runs still work.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@contracts/scripts/disputeRelayerBot.ts`:
- Around line 104-110: The current scan of fullTxReceipt.logs filters only by
disputeRequestTopic and may pick up DisputeRequest events from other contracts;
update the filter used when building disputeRequests to also check that
log.address matches the emitting arbitrable (use the foreignArbitrable
address/identifier used in this module, e.g. foreignArbitrable.address or
foreignArbitrable) so only logs from that contract are parsed by
arbitrableInterface.parseLog into DisputeRequestEvent.OutputObject; ensure
address comparison is normalized (e.g. lowercased) to avoid mismatch.
- Around line 78-153: The listener callback passed to foreignGateway.on (the
async function handling CrossChainDisputeOutgoing events) must be wrapped in a
try/catch so any rejection from getTransactionReceipt, parseLog,
feeToken.approve, core.arbitrationCost, or homeGateway relay calls doesn’t
become an unhandled rejection; surround the entire body that references
eventLog, foreignBlockHash, disputeRequests/disputeRequest,
relayCreateDisputeParams, feeToken, core and homeGateway in a single try block
and in catch log the error (including eventLog.transactionHash and
safeJson(eventLog) for context) via logger.error and then return/exit the
handler without rethrowing so the listener completes cleanly.
In `@contracts/scripts/storage-layout.ts`:
- Around line 22-25: The nested lookup for
buildInfo.output.contracts["src/arbitration/KlerosCore.sol"]["KlerosCore"] may
be undefined before accessing contractOutput.storageLayout; add an explicit
runtime guard after resolving contractOutput (the value currently assigned to
the variable contractOutput) to check that buildInfo and the specific contract
entry exist and, if not, throw a clear error (e.g., mentioning "KlerosCore build
info not found") instead of proceeding to access storageLayout; ensure the check
references buildInfo.output.contracts and the "KlerosCore" key so it fails fast
with a targeted message.
In `@contracts/scripts/viemTest.ts`:
- Around line 31-32: Add a fail-fast cardinality check in the createObject
helper: verify keys.length === values.length before calling Object.fromEntries
and throw an informative Error if they differ (include the lengths and a short
preview of keys/values in the message). Apply the same check to the other
similar mapping (the second keys.map(...) usage around the other mapping on the
same file) so that unnamed/filtered ABI outputs can't silently truncate data;
ensure the error clearly names the function (createObject) and the mismatched
lengths.
- Around line 21-29: getFunctionReturnParameterNames currently assumes a
matching function exists and dereferences f.outputs which can throw; replace the
brittle filter(...)[0] with a safe lookup (e.g., Array.prototype.find) and guard
against undefined: if no match, either return an empty array or throw a clear
error that includes the searched name and available function names, and for
overloaded functions handle by requiring a full signature or picking the first
match explicitly; ensure you reference the parameters abi and name and the local
variable f (the matched AbiFunction) when adding the check and error/fallback
logic.
In `@contracts/test/integration/getContractsEthers.test.ts`:
- Around line 401-404: The test currently allows a generic "Cannot destructure
property" error to satisfy the spec; change the assertion in the getContracts
test to only expect the unsupported-deployment error so failures before the
guard don't pass. Specifically, update the expectation for
getContracts(arbitrumSepoliaProvider, "invalid") to use a stricter matcher like
.to.be.rejectedWith(/Unsupported deployment/) (or assert the exact error
message/class thrown by getContracts) and remove the alternation that includes
"Cannot destructure property", ensuring the unsupported-deployment guard in
getContracts is actually exercised.
---
Outside diff comments:
In `@contracts/scripts/wagmiHelpers.mts`:
- Line 39: The address type literal is malformed on the assignment of
jsonContent.address (the mapping entry using [chain.id]): replace the incorrect
template literal type `0x{string}` with the correct TypeScript template literal
`0x${string}` so the cast becomes jsonContent.address as `0x${string}` (or use
the appropriate Address type) to satisfy viem's address type.
---
Duplicate comments:
In `@contracts/deploy/04-klerosliquid-to-v2-gnosis.ts`:
- Around line 69-74: The console.log call that prints wPnkAddress,
foreignGateway.address and weth.address has only two %s placeholders so the
third value (weth.address) is ignored; update the console.log format string used
in that console.log invocation to include three placeholders (e.g., add ",
\nWETH at %s") or remove the extra argument so the number of %s matches the
provided values—refer to the variables wPnkAddress, foreignGateway.address,
weth.address and the console.log call to locate and fix it.
In `@contracts/scripts/disputeRelayerBot.ts`:
- Around line 104-116: Check the disputeRequests array before indexing: after
building disputeRequests from fullTxReceipt.logs using disputeRequestTopic and
arbitrableInterface.parseLog, if disputeRequests.length === 0 return/skip
processing (no indexing), and only call logger.warn when disputeRequests.length
> 1; then safely use disputeRequests[0] to set disputeRequest. This ensures you
never access disputeRequests[0] when empty and only warn on duplicate events.
---
Nitpick comments:
In `@contracts/test/integration/getContractsEthers.test.ts`:
- Around line 126-131: The test currently hard-codes public RPC endpoints when
constructing ethers.JsonRpcProvider for arbitrumSepoliaProvider and
arbitrumProvider; change these to read from environment variables (e.g.,
process.env.ARBITRUM_SEPOLIA_RPC, process.env.ARBITRUM_MAINNET_RPC) with the
current URLs as local fallbacks, and use those values when calling new
ethers.JsonRpcProvider(...) so CI can override endpoints while local runs still
work.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 68c71473-a4a0-485a-8eda-1aa7e20de068
📒 Files selected for processing (14)
contracts/deploy/04-klerosliquid-to-v2-gnosis.tscontracts/package.jsoncontracts/scripts/disputeRelayerBot.tscontracts/scripts/getPoliciesV1.tscontracts/scripts/populateCourts.tscontracts/scripts/storage-layout.tscontracts/scripts/viemTest.tscontracts/scripts/wagmiHelpers.mtscontracts/test/integration/getContractsEthers.test.tscontracts/tsconfig.jsoncontracts/wagmi.config.devnet.mtscontracts/wagmi.config.hardhat.mtscontracts/wagmi.config.mainnet.mtscontracts/wagmi.config.testnet.mts
🚧 Files skipped from review as they are similar to previous changes (1)
- contracts/scripts/getPoliciesV1.ts
Contributor
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
contracts/scripts/storage-layout.ts (1)
22-24:⚠️ Potential issue | 🟠 Major | ⚡ Quick winGuard nested lookup before indexing
"KlerosCore"On Line 22-24, this can still throw before your explicit guard: if
buildInfo.output.contracts["src/arbitration/KlerosCore.sol"]is missing, indexing["KlerosCore"]crashes withTypeError. Resolve the nested object safely first, then apply the targeted error.Suggested minimal fix
- const contractOutput = buildInfo.output.contracts[ - "src/arbitration/KlerosCore.sol" - ]["KlerosCore"] as ContractOutputWithStorageLayout; + const contractOutput = buildInfo.output.contracts[ + "src/arbitration/KlerosCore.sol" + ]?.["KlerosCore"] as ContractOutputWithStorageLayout | undefined; if (!contractOutput) { throw new Error( "Contract output not found for src/arbitration/KlerosCore.sol:KlerosCore", ); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@contracts/scripts/storage-layout.ts` around lines 22 - 24, The current lookup assumes buildInfo.output.contracts["src/arbitration/KlerosCore.sol"] exists before indexing ["KlerosCore"], which can throw; first safely retrieve the file-level entry (e.g. const fileContracts = buildInfo.output?.contracts?.["src/arbitration/KlerosCore.sol"]) and guard it, then extract the specific contract (const contractOutput = fileContracts?.["KlerosCore"] as ContractOutputWithStorageLayout) and only after those null-checks throw the explicit error; update references to contractOutput and the error path accordingly so the code never indexes into undefined.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Duplicate comments:
In `@contracts/scripts/storage-layout.ts`:
- Around line 22-24: The current lookup assumes
buildInfo.output.contracts["src/arbitration/KlerosCore.sol"] exists before
indexing ["KlerosCore"], which can throw; first safely retrieve the file-level
entry (e.g. const fileContracts =
buildInfo.output?.contracts?.["src/arbitration/KlerosCore.sol"]) and guard it,
then extract the specific contract (const contractOutput =
fileContracts?.["KlerosCore"] as ContractOutputWithStorageLayout) and only after
those null-checks throw the explicit error; update references to contractOutput
and the error path accordingly so the code never indexes into undefined.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 45972525-49b3-45eb-be4e-cf99e8ab5959
📒 Files selected for processing (1)
contracts/scripts/storage-layout.ts
kyrers
force-pushed
the
feat/pre-commit-type-check
branch
from
dc582f3 to
2981ede
Compare
kyrers
force-pushed
the
feat/pre-commit-type-check-contracts
branch
from
34963f3 to
a99ab1b
Compare
kyrers
force-pushed
the
feat/pre-commit-type-check-contracts
branch
from
a99ab1b to
93b2600
Compare
tractorss
reviewed
May 19, 2026
Contributor
tractorss
left a comment
tractorss
left a comment
There was a problem hiding this comment.
looks good, left a couple comments 🙌
| const updateOwner = async ( | ||
| contractName: string, | ||
| // eslint-disable-next-line @typescript-eslint/no-explicit-any | ||
| contractInstance: any, |
Contributor
There was a problem hiding this comment.
Can we switch to unknown here ?
Contributor
Author
There was a problem hiding this comment.
unknown doesn't work here because it errors out when trying to call changeOwner below. I did try a simpler solution. I defined a simple type:
type ContractInstance = {
changeOwner: (newOwner: string) => Promise<ContractTransactionResponse>;
}
The problem with this is that it exposes another problem: EvidenceModule does not have a changeOwner function. So, I just assumed that was the reason any was used (so we can always call it, it will just error out) and reverted back.
Bottom line is, this can indeed be improved, but I think it needs some deeper changes.
| .filter((log: Log) => log.topics[0] === disputeRequestTopic) | ||
| .map( | ||
| (log: Log) => | ||
| arbitrableInterface.parseLog(log)! |
Contributor
There was a problem hiding this comment.
Is non-null assertion safe here? 🤔 , can we add a defined check instead
Contributor
Author
There was a problem hiding this comment.
Technically, it should be safe because of the filter just before. But it's a better practice to add the defined check, so I addressed it in 622ad9f.
| "relayCreateDispute((bytes32,uint256,address,uint256,uint256,uint256,string,uint256,bytes))" | ||
| const cost = await core["arbitrationCost(bytes)"](extraData); | ||
| tx = await homeGateway[ | ||
| "relayCreateDispute((bytes32,uint256,address,uint256,uint256,uint256,bytes))" |
Contributor
There was a problem hiding this comment.
I see some arguments were removed in the function specifier, is this intentional
Contributor
Author
There was a problem hiding this comment.
Yes, intentional. It seems there was a disconnect between the script and what the RelayCreateDisputeParams struct contains. The type checks surfaced this error, so I had to adjust the call.
|
|
||
| // @ts-expect-error populateCourts additionalCourtParams types | ||
| // Currently, there seems to be a misalignment between the expected way to access additional court params and the actual way to access them. | ||
| // This comment allows the check-types script to pass, but the script needs updating to deal with current `courts()` return type. |
Contributor
There was a problem hiding this comment.
let's keep a note of this 👍
Contributor
Author
There was a problem hiding this comment.
@coderabbitai create an issue to track this.
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
@kyrers, I've created a GitHub issue to track the courts() return type misalignment in the populateCourts.ts script. Once the script is done creating, I'll share the link below.
🏁 Script executed:
#!/bin/bash
gh issue create \
--repo kleros/kleros-v2 \
--title "fix: resolve \`courts()\` return type misalignment in populateCourts script" \
--body "## Summary
The \`contracts/scripts/populateCourts.ts\` script currently uses \`@ts-expect-error\` suppressions to work around a misalignment between the expected way to access additional court parameters (e.g., \`hiddenVotes\`, \`jurorsForCourtJump\`) from the \`courts()\` return type and the actual runtime shape.
## Context
The following \`@ts-expect-error\` comments were added as a temporary workaround to allow the \`check-types\` script to pass:
\`\`\`ts
// `@ts-expect-error` populateCourts additionalCourtParams types
// Currently, there seems to be a misalignment between the expected way to access additional court params and the actual way to access them.
// This comment allows the check-types script to pass, but the script needs updating to deal with current \`courts()\` return type.
\`\`\`
These suppressions affect access to \`courtPresent.hiddenVotes\` and \`courtPresent.jurorsForCourtJump\` in the court update loop.
## Action Required
Investigate and fix the type mismatch between the TypeChain-generated \`KlerosCore.courts()\` return type and the actual fields being accessed in the script, removing the \`@ts-expect-error\` suppressions once the types are properly aligned.
## References
- PR: https://github.com/kleros/kleros-v2/pull/2278
- Comment: https://github.com/kleros/kleros-v2/pull/2278#discussion_r3267735347
/cc `@kyrers`" \
--assignee kyrers
Length of output: 48
@kyrers, the issue has been created and assigned to you: #2317
It tracks the courts() return type misalignment in populateCourts.ts and the @ts-expect-error suppressions that need to be resolved once the types are properly aligned.
| @@ -0,0 +1,7 @@ | |||
| { | |||
| "*.{js,jsx,ts,tsx}": [ | |||
Contributor
There was a problem hiding this comment.
since wagmi config's extension was updated to mts should we include that here?
Contributor
Author
There was a problem hiding this comment.
Good catch, oversight on my part. Address on e7bdf69.
…more unnecessary files
kyrers
force-pushed
the
feat/pre-commit-type-check-contracts
branch
from
e7bdf69 to
bfe1815
Compare
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Part of #2277, but isolates the
contractsworkspace changes for easier review.PR-Codex overview
This PR introduces various enhancements and fixes across multiple files in the project. Key improvements include updates to type definitions, formatting changes, and the addition of new deployment scripts for better contract management.
Detailed summary
lintstagedrc.jsonfor linting configurations.deployUpgradable.tsto include trailing commas in function parameters.index.tsfor better type management.disputeRelayerBotFromChiado.ts,disputeRelayerBotFromHardhat.ts, anddisputeRelayerBotFromSepolia.tsfor improved imports.tsconfig.jsonto exclude unnecessary files and include specific TypeScript files.env.tsto use object syntax for environment variable handling.HomeGatewayandDisputeKitGated.getActualAddressfunction for better error handling.package.jsonto reflect new script commands and dependencies.Summary by CodeRabbit
New Features
Tests
Chores