Feat/added save snapshot in validator-cli #422
Conversation
WalkthroughThis update introduces a major refactor and modularization of the validator bot CLI. It removes legacy scripts and devnet-specific files, consolidates environment configuration, and introduces network- and path-aware logic for multi-network support. New modules add robust error handling, GraphQL subgraph querying, and snapshot management. The transaction handler and claimer logic are refactored for improved modularity and testability, with expanded event logging and granular state tracking. Test coverage is significantly increased for critical modules. The TypeScript configuration is updated for modern module support, and PM2/CLI scripts are streamlined for a unified, flexible deployment. Changes
Sequence Diagram(s)sequenceDiagram
participant CLI
participant Env
participant BotConfig
participant Watcher
participant Network
participant Epoch
participant Claimer
participant Challenger
participant TxHandler
participant GraphQL
participant Logger
CLI->>Env: Load environment variables
CLI->>BotConfig: Parse CLI args and env for bot path/networks
BotConfig->>Env: Read NETWORKS, VEAOUTBOX_CHAINS
CLI->>Watcher: Start watcher with configs
loop For each NetworkConfig
Watcher->>Network: Initialize per-network state
loop For each epoch
Watcher->>Epoch: Determine block range, fetch claim
Epoch->>GraphQL: (if needed) Fetch claim/subgraph data
alt BotPath == Claimer
Epoch->>Claimer: Check and make claim if needed
Claimer->>TxHandler: Submit claim/tx
else BotPath == Challenger
Epoch->>Challenger: Challenge and resolve claim
Challenger->>TxHandler: Submit challenge/tx
end
TxHandler->>Logger: Emit events for state/tx
end
end
Suggested labels
Suggested reviewers
Poem
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
❌ Deploy Preview for veascan failed. Why did it fail? →
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 15
🔭 Outside diff range comments (1)
validator-cli/src/ArbToEth/transactionHandler.ts (1)
296-337:⚠️ Potential issueRisk of zero-gas transactions when challenging
maxFeePerGasProfitable = deposit / (gasEstimate * 6n)can evaluate to0nifdepositis small orgasEstimatelarge, producing a transaction that will be rejected by the RPC node.
Guard against this by enforcing a sensible minimum (e.g.,> 0n) or falling back to the current base fee.let maxFeePerGasProfitable = deposit / (gasEstimate * 6n); -if (maxFeePerGasProfitable === 0n) { - maxFeePerGasProfitable = BigInt(await this.veaOutboxProvider.getFeeData()).maxFeePerGas ?? 1n; -} +if (maxFeePerGasProfitable === 0n) { + const feeData = await this.veaOutboxProvider.getFeeData(); + maxFeePerGasProfitable = (feeData.maxFeePerGas ?? 1n); +}
🧹 Nitpick comments (17)
validator-cli/src/utils/errors.ts (1)
19-25: Fix the error name property in NotDefinedError class.The class has been renamed from
TransactionHandlerNotDefinedErrortoNotDefinedError, but thenameproperty still contains the old name.class NotDefinedError extends Error { constructor(param: string) { super(); - this.name = "TransactionHandlerNotDefinedError"; + this.name = "NotDefinedError"; this.message = `${param} is not defined`; } }validator-cli/src/utils/snapshot.ts (1)
6-11: Consider using a more specific type for veaInbox.The
veaInboxparameter has typeany, which could be made more specific if there's a proper type or interface for this contract interface. This would improve type safety and developer experience.validator-cli/src/consts/bridgeRoutes.ts (2)
86-90: Return the already-validated object & improve error messageYou correctly store the looked-up bridge in
const bridge, but immediately ignore it when returning.
Returningbridgemakes the intent clearer and avoids a second dictionary lookup.
While touching the code, enrich the error so operators know which chainId failed.-const getBridgeConfig = (chainId: number): Bridge => { - const bridge = bridges[chainId]; - if (!bridge) throw new Error(`Bridge not found for chain`); - return bridges[chainId]; +const getBridgeConfig = (chainId: number): Bridge => { + const bridge = bridges[chainId]; + if (!bridge) { + throw new Error(`Bridge not found for chain ID ${chainId}`); + } + return bridge; }
1-1: Mixedrequire/importstyle may break under ESModule builds
require("dotenv").config();sits atop an otherwise ESM‐style file that usesimportstatements.
Iftype: "module"oresModuleInteropsettings change, this hybrid style can fail at runtime.
Consider replacing with:import "dotenv/config";or moving dotenv loading into the entry script.
Not blocking, but worth standardising.validator-cli/src/ArbToEth/transactionHandlerDevnet.ts (1)
51-71: Minor polish: copy-paste artefacts & magic constants
startVerifTrxis a leftover name from other methods.
Rename toadvanceStateTxfor clarity.
CHAIN_IDis hard-coded to11155111.
If the handler is ever reused for other devnets, parameterising this value (e.g. via constructor or bridge config) will avoid accidental misuse.Not critical but improves readability and reuse.
validator-cli/src/utils/botConfig.ts (2)
21-42: Simplify boolean conversionThe function is well-structured and correctly handles path validation, but the boolean conversion can be simplified.
- const saveSnapshotFlag = args.find((a) => a.startsWith("--saveSnapshot")); - const toSaveSnapshot = saveSnapshotFlag ? true : false; + const saveSnapshotFlag = args.find((a) => a.startsWith("--saveSnapshot")); + const toSaveSnapshot = !!saveSnapshotFlag;🧰 Tools
🪛 Biome (1.9.4)
[error] 40-40: Unnecessary use of boolean literals in conditional expression.
Simplify your code by directly assigning the result without using a ternary operator.
If your goal is negation, you may use the logical NOT (!) or double NOT (!!) operator for clearer and concise code.
Check for more details about NOT operator.
Unsafe fix: Remove the conditional expression with(lint/complexity/noUselessTernary)
68-76: Type casting should be more explicitWhile the validation checks if networks are valid, the type casting approach could be improved for better type safety.
- return networks as unknown as Network[]; + return networks.map(network => network as Network);validator-cli/src/utils/epochHandler.ts (2)
44-48: Array construction could be simplifiedThe current array construction logic is slightly complex. Consider using a more direct approach for better readability.
- const length = veaEpochOutboxClaimableNow - veaEpochOutboxWatchLowerBound; - const veaEpochOutboxCheckClaimsRangeArray: number[] = Array.from( - { length }, - (_, i) => veaEpochOutboxWatchLowerBound + i + 1 - ); + const veaEpochOutboxCheckClaimsRangeArray: number[] = []; + for (let epoch = veaEpochOutboxWatchLowerBound + 1; epoch <= veaEpochOutboxClaimableNow; epoch++) { + veaEpochOutboxCheckClaimsRangeArray.push(epoch); + }
65-72: Block offset updated as specifiedThe function changes the block offset from 1000 to 500 as mentioned in the PR description. However, consider adding error handling for provider operations.
const getBlockFromEpoch = async (epoch: number, epochPeriod: number, provider: JsonRpcProvider): Promise<number> => { const epochTimestamp = epoch * epochPeriod; - const latestBlock = await provider.getBlock("latest"); - const baseBlock = await provider.getBlock(latestBlock.number - 500); - const secPerBlock = (latestBlock.timestamp - baseBlock.timestamp) / (latestBlock.number - baseBlock.number); - const blockFallBack = Math.floor((latestBlock.timestamp - epochTimestamp) / secPerBlock); - return latestBlock.number - blockFallBack; + try { + const latestBlock = await provider.getBlock("latest"); + if (!latestBlock) throw new Error("Failed to get latest block"); + + const baseBlock = await provider.getBlock(latestBlock.number - 500); + if (!baseBlock) throw new Error("Failed to get base block"); + + const secPerBlock = (latestBlock.timestamp - baseBlock.timestamp) / (latestBlock.number - baseBlock.number); + const blockFallBack = Math.floor((latestBlock.timestamp - epochTimestamp) / secPerBlock); + return latestBlock.number - blockFallBack; + } catch (error) { + console.error("Error estimating block from epoch:", error); + throw error; + } };validator-cli/src/ArbToEth/claimer.ts (1)
94-99: Redundant conditional check leads to unreachable branch
newMessagesToBridgeis already defined as
savedSnapshot !== outboxStateRoot && savedSnapshot !== ethers.ZeroHash.
The follow-up guard&& savedSnapshot !== ethers.ZeroHashtherefore never changes the outcome and only adds cognitive noise.- if ((newMessagesToBridge || lastClaimChallenged) && savedSnapshot != ethers.ZeroHash) { + if (newMessagesToBridge || lastClaimChallenged) {Removing the duplication makes the intent clearer and avoids the risk of future edits getting the two conditions out of sync.
validator-cli/src/ArbToEth/transactionHandler.test.ts (1)
12-14: Unused import generates linter/TypeScript error
saveSnapshotis imported but never referenced in this test file.-import { saveSnapshot } from "src/utils/snapshot";Remove the import or add tests that actually cover
saveSnapshotto keep the suite meaningful.validator-cli/src/utils/graphQueries.ts (1)
128-134: Minor: GraphQL naming typo
type SenSnapshotResponse→type SentSnapshotResponsewould better reflect the payload and avoid confusion.validator-cli/src/utils/ethers.ts (1)
84-90: Unusednetworkparameter hides potential branching bugs
getClaimValidatortakesnetworkbut never references it.
Either drop the argument or add the intended branching (e.g. different validators for devnet vs testnet) to avoid misleading callers.validator-cli/src/utils/claim.ts (1)
164-166:fetchMessageStatusmay throw – wrap it to keep state object consistentIf the RPC call inside
fetchMessageStatusfails the whole function will reject, erasing the partially builtclaimResolveState.
Surround the call with try/catch and keep the original state for better error tolerance.validator-cli/src/ArbToEth/transactionHandler.ts (3)
65-67: Comment & value out of sync – decide which one you want
MAX_PENDING_TIMEis set to 5 minutes (5 * 60 * 1000) while the inline comment still says “3 minutes”.
Either adjust the constant or fix the comment to avoid confusion for future maintainers.-export const MAX_PENDING_TIME = 5 * 60 * 1000; // 3 minutes +// 5 minutes +export const MAX_PENDING_TIME = 5 * 60 * 1000;
79-90:devnetAdvanceStateTxnmay beundefinedat runtime
TransactionsmarksdevnetAdvanceStateTxnas optional, yet the base handler later accesses it from subclasses.
Initialising it tonullavoidsundefinedchecks and keeps the shape oftransactionspredictable.public transactions: Transactions = { claimTxn: null, withdrawClaimDepositTxn: null, startVerificationTxn: null, verifySnapshotTxn: null, challengeTxn: null, withdrawChallengeDepositTxn: null, saveSnapshotTxn: null, sendSnapshotTxn: null, executeSnapshotTxn: null, + devnetAdvanceStateTxn: null, };
119-149:checkTransactionStatuscan emit duplicate events in tight loops
Every poll emitsTXN_PENDING/TXN_NOT_FINAL/TXN_FINAL, potentially spamming logs & websockets.
Consider caching the last status per hash (e.g., inthis.transactions) and only emitting when the status actually changes.
📜 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 (30)
validator-cli/.env.dist(1 hunks)validator-cli/ecosystem.config.js(1 hunks)validator-cli/package.json(1 hunks)validator-cli/src/ArbToEth/claimer.test.ts(1 hunks)validator-cli/src/ArbToEth/claimer.ts(1 hunks)validator-cli/src/ArbToEth/transactionHandler.test.ts(8 hunks)validator-cli/src/ArbToEth/transactionHandler.ts(5 hunks)validator-cli/src/ArbToEth/transactionHandlerDevnet.ts(1 hunks)validator-cli/src/ArbToEth/validator.test.ts(2 hunks)validator-cli/src/ArbToEth/validator.ts(4 hunks)validator-cli/src/consts/bridgeRoutes.ts(1 hunks)validator-cli/src/devnet/arbToChiado/happyPath.ts(0 hunks)validator-cli/src/devnet/arbToSepolia/happyPath.ts(0 hunks)validator-cli/src/utils/botConfig.test.ts(1 hunks)validator-cli/src/utils/botConfig.ts(1 hunks)validator-cli/src/utils/botEvents.ts(1 hunks)validator-cli/src/utils/claim.test.ts(7 hunks)validator-cli/src/utils/claim.ts(5 hunks)validator-cli/src/utils/devnet.ts(0 hunks)validator-cli/src/utils/epochHandler.test.ts(1 hunks)validator-cli/src/utils/epochHandler.ts(3 hunks)validator-cli/src/utils/errors.ts(2 hunks)validator-cli/src/utils/ethers.ts(3 hunks)validator-cli/src/utils/graphQueries.ts(1 hunks)validator-cli/src/utils/logger.ts(5 hunks)validator-cli/src/utils/snapshot.test.ts(1 hunks)validator-cli/src/utils/snapshot.ts(1 hunks)validator-cli/src/watcher.ts(2 hunks)validator-cli/src/watcherDevnet.ts(0 hunks)validator-cli/tsconfig.json(1 hunks)
💤 Files with no reviewable changes (4)
- validator-cli/src/devnet/arbToSepolia/happyPath.ts
- validator-cli/src/devnet/arbToChiado/happyPath.ts
- validator-cli/src/watcherDevnet.ts
- validator-cli/src/utils/devnet.ts
🧰 Additional context used
🧠 Learnings (9)
validator-cli/src/ArbToEth/validator.test.ts (1)
Learnt from: mani99brar
PR: kleros/vea#344
File: validator-cli/src/ArbToEth/watcherArbToEth.ts:732-745
Timestamp: 2024-11-27T04:18:05.872Z
Learning: In `validator-cli/src/ArbToEth/watcherArbToEth.ts`, input validation inside the `hashClaim` function is unnecessary because the upper layer ensures valid input before calling it.
validator-cli/src/utils/errors.ts (1)
Learnt from: mani99brar
PR: kleros/vea#388
File: validator-cli/src/utils/ethers.ts:67-72
Timestamp: 2025-01-06T06:27:38.796Z
Learning: A custom error named `TransactionHandlerNotDefinedError` is now thrown for unsupported chain IDs in `getTransactionHandler` within `validator-cli/src/utils/ethers.ts`.
validator-cli/src/ArbToEth/validator.ts (2)
Learnt from: madhurMongia
PR: kleros/vea#359
File: validator-cli/src/ArbToEth/watcherArbToGnosis.ts:24-24
Timestamp: 2024-11-27T10:48:48.433Z
Learning: In the `validator-cli` codebase, both `arbitrumToEth` and `arbitrumToGnosis` modules use the same `ClaimStruct`, so importing `ClaimStruct` from either module is acceptable.
Learnt from: mani99brar
PR: kleros/vea#344
File: validator-cli/src/ArbToEth/watcherArbToEth.ts:732-745
Timestamp: 2024-11-27T04:18:05.872Z
Learning: In `validator-cli/src/ArbToEth/watcherArbToEth.ts`, input validation inside the `hashClaim` function is unnecessary because the upper layer ensures valid input before calling it.
validator-cli/src/consts/bridgeRoutes.ts (1)
Learnt from: mani99brar
PR: kleros/vea#370
File: bridger-cli/src/consts/bridgeRoutes.ts:28-30
Timestamp: 2024-12-10T08:00:35.645Z
Learning: In `bridger-cli/src/consts/bridgeRoutes.ts`, additional validation in the `getBridgeConfig` function is unnecessary because error handling and validation are managed by upper layers in the application.
validator-cli/src/utils/epochHandler.ts (1)
Learnt from: mani99brar
PR: kleros/vea#370
File: bridger-cli/src/utils/epochHandler.ts:13-24
Timestamp: 2024-12-10T08:26:12.411Z
Learning: In `bridger-cli/src/utils/epochHandler.ts`, the `setEpochRange` function does not require validation checks for `startEpoch` being non-negative or the epoch range being too large because the upper layer ensures `startEpoch` is always valid and the epoch range is appropriately managed.
validator-cli/src/utils/epochHandler.test.ts (2)
Learnt from: mani99brar
PR: kleros/vea#370
File: bridger-cli/src/utils/epochHandler.ts:13-24
Timestamp: 2024-12-10T08:26:12.411Z
Learning: In `bridger-cli/src/utils/epochHandler.ts`, the `setEpochRange` function does not require validation checks for `startEpoch` being non-negative or the epoch range being too large because the upper layer ensures `startEpoch` is always valid and the epoch range is appropriately managed.
Learnt from: mani99brar
PR: kleros/vea#370
File: bridger-cli/src/utils/epochHandler.ts:63-68
Timestamp: 2024-12-10T08:27:15.815Z
Learning: In the file `bridger-cli/src/utils/epochHandler.ts`, the calculation within the `checkForNewEpoch` function is correct as implemented and has been tested.
validator-cli/src/utils/graphQueries.ts (1)
Learnt from: mani99brar
PR: kleros/vea#370
File: bridger-cli/src/utils/graphQueries.ts:36-58
Timestamp: 2024-12-09T09:04:04.819Z
Learning: In `bridger-cli/src/utils/graphQueries.ts` (TypeScript), the `getVerificationStatus` function is no longer needed and has been removed.
validator-cli/src/utils/claim.ts (1)
Learnt from: mani99brar
PR: kleros/vea#370
File: bridger-cli/src/utils/graphQueries.ts:12-34
Timestamp: 2024-12-09T09:03:31.474Z
Learning: In `bridger-cli/src/utils/graphQueries.ts`, within the `getClaimForEpoch` function, returning `result['claims'][0]` is sufficient because it will be `undefined` if `result['claims']` is an empty array, making additional checks unnecessary.
validator-cli/.env.dist (1)
Learnt from: madhurMongia
PR: kleros/vea#359
File: validator-cli/.env.dist:3-4
Timestamp: 2024-11-20T11:50:31.944Z
Learning: In 'validator-cli/.env.dist', the `RPC_GNOSIS` variable may be intentionally set to `https://rpc.chiadochain.net` as an example.
🧬 Code Graph Analysis (12)
validator-cli/src/utils/botConfig.test.ts (2)
validator-cli/src/utils/botConfig.ts (1)
getBotPath(21-42)validator-cli/src/utils/errors.ts (1)
InvalidBotPathError(63-63)
validator-cli/src/utils/claim.test.ts (2)
validator-cli/src/utils/graphQueries.ts (2)
getVerificationForClaim(192-192)getChallengerForClaim(193-193)validator-cli/src/utils/claim.ts (1)
getClaim(193-193)
validator-cli/src/utils/errors.ts (1)
relayer-cli/src/consts/bridgeRoutes.ts (1)
Network(88-88)
validator-cli/src/utils/snapshot.ts (3)
validator-cli/src/utils/graphQueries.ts (1)
getLastMessageSaved(195-195)relayer-cli/src/consts/bridgeRoutes.ts (1)
Network(88-88)validator-cli/src/utils/emitter.ts (1)
defaultEmitter(4-4)
validator-cli/src/watcher.ts (10)
validator-cli/src/utils/errors.ts (1)
MissingEnvError(66-66)validator-cli/src/utils/botConfig.ts (3)
getBotPath(21-42)getNetworkConfig(53-66)NetworkConfig(44-47)validator-cli/src/utils/emitter.ts (1)
defaultEmitter(4-4)validator-cli/src/consts/bridgeRoutes.ts (1)
getBridgeConfig(92-92)validator-cli/src/utils/epochHandler.ts (2)
setEpochRange(74-74)getBlockFromEpoch(74-74)validator-cli/src/utils/ethers.ts (5)
getVeaInbox(124-124)getVeaOutbox(125-125)getTransactionHandler(131-131)getClaimValidator(129-129)getClaimer(130-130)validator-cli/src/utils/snapshot.ts (1)
SaveSnapshotParams(12-21)validator-cli/src/utils/claim.ts (1)
getClaim(193-193)validator-cli/src/ArbToEth/claimer.ts (2)
checkAndClaim(146-146)CheckAndClaimParams(146-146)validator-cli/src/ArbToEth/validator.ts (1)
ChallengeAndResolveClaimParams(15-28)
validator-cli/src/consts/bridgeRoutes.ts (2)
relayer-cli/src/consts/bridgeRoutes.ts (1)
Network(88-88)veascan-web/src/consts/bridges.ts (1)
bridges(39-58)
validator-cli/src/utils/epochHandler.ts (1)
validator-cli/src/consts/bridgeRoutes.ts (1)
getBridgeConfig(92-92)
validator-cli/src/ArbToEth/claimer.ts (6)
relayer-cli/src/consts/bridgeRoutes.ts (1)
Network(88-88)validator-cli/src/ArbToEth/transactionHandler.ts (2)
ArbToEthTransactionHandler(69-433)makeClaim(156-181)validator-cli/src/utils/claim.ts (2)
getClaim(193-193)ClaimHonestState(193-193)validator-cli/src/utils/graphQueries.ts (1)
getLastClaimedEpoch(191-191)validator-cli/src/utils/ethers.ts (1)
getTransactionHandler(131-131)validator-cli/src/ArbToEth/transactionHandlerDevnet.ts (1)
ArbToEthDevnetTransactionHandler(19-72)
validator-cli/src/ArbToEth/transactionHandlerDevnet.ts (2)
validator-cli/src/ArbToEth/transactionHandler.ts (4)
Transactions(39-50)Transaction(34-37)ArbToEthTransactionHandler(69-433)TransactionHandlerConstructor(23-32)validator-cli/src/consts/bridgeRoutes.ts (1)
getBridgeConfig(92-92)
validator-cli/src/utils/botConfig.ts (1)
validator-cli/src/utils/errors.ts (2)
InvalidBotPathError(63-63)InvalidNetworkError(65-65)
validator-cli/src/utils/graphQueries.ts (1)
validator-cli/src/utils/errors.ts (1)
ClaimNotFoundError(60-60)
validator-cli/src/utils/claim.ts (2)
validator-cli/src/utils/graphQueries.ts (4)
getClaimForEpoch(190-190)getVerificationForClaim(192-192)getChallengerForClaim(193-193)getSnapshotSentForEpoch(194-194)validator-cli/src/utils/errors.ts (1)
ClaimNotFoundError(60-60)
🪛 Biome (1.9.4)
validator-cli/src/utils/botConfig.ts
[error] 40-40: Unnecessary use of boolean literals in conditional expression.
Simplify your code by directly assigning the result without using a ternary operator.
If your goal is negation, you may use the logical NOT (!) or double NOT (!!) operator for clearer and concise code.
Check for more details about NOT operator.
Unsafe fix: Remove the conditional expression with
(lint/complexity/noUselessTernary)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Redirect rules - veascan
- GitHub Check: Header rules - veascan
- GitHub Check: Pages changed - veascan
🔇 Additional comments (53)
validator-cli/package.json (1)
21-22: Dependency updates look good.The upgrade of pm2 to version 6.0.5 and keeping typescript at version 4.9.5 aligns with the simplified package structure. The removal of web3-related dependencies and network-specific start scripts supports the consolidation of startup procedures mentioned in the summary.
validator-cli/src/utils/epochHandler.test.ts (4)
14-14: The additional offset adjustment looks correct.This change to the startEpoch calculation aligns with the PR objective of fixing the block heuristic search offset.
21-27: The parameter object refactoring improves code structure.The test now properly uses a named parameter object instead of multiple positional arguments, making the code more maintainable and less prone to ordering errors. This change aligns with modern JavaScript/TypeScript best practices.
28-28: Epoch range validation is now more comprehensive.The test now correctly verifies that the epoch range includes the current epoch, which ensures the function behaves as expected.
36-37: Function signature simplification looks good.The test now calls getLatestChallengeableEpoch with only the required parameters (epoch period and current time), reflecting the function's decoupling from chain-specific config.
validator-cli/tsconfig.json (1)
5-13: TypeScript configuration is properly set up.The compiler options added to tsconfig.json correctly configure module resolution, output directory, and interoperability settings. This ensures consistent TypeScript compilation across development and deployment environments, which is essential for the refactored application structure.
validator-cli/src/ArbToEth/validator.test.ts (3)
43-43: Updated claim dependency handling looks good.The test now directly assigns the mock claim to
mockDeps.claimproperty, which aligns with the refactored validator function that no longer fetches claims internally but expects them to be passed explicitly.
56-58: Good test hygiene with afterEach cleanup.Adding an
afterEachhook to clear all mocks ensures proper test isolation between test cases.
61-61: Direct claim assignment matches updated function signature.Setting
mockDeps.claim = nulldirectly aligns with the refactored function signature where the claim is passed as a parameter rather than fetched internally.validator-cli/src/ArbToEth/validator.ts (3)
9-10: Explicit imports for Network enum and ClaimStruct type.These imports support the changes to handle network-specific behavior and explicit claim typing.
16-16: Updated interface to require explicit claim passing.The function now requires a
claimparameter of typeClaimStructdirectly instead of fetching it internally, which clearly separates concerns and improves testability.
31-31: Early return when claim is null improves code clarity.The function now immediately checks if the claim exists and returns early if not, emitting an appropriate event. This simplifies the control flow and makes the function more readable.
Also applies to: 43-46
validator-cli/ecosystem.config.js (3)
4-8: Simplified PM2 configuration improves maintainability.Consolidating to a single app that uses the TypeScript watcher directly with proper interpreter configuration is cleaner than having multiple network-specific scripts.
9-10: Enhanced developer experience with watch and autorestart.Adding watch and autorestart options improves development workflow by automatically applying changes without manual restarts.
13-13: Explicit TypeScript project configuration.Setting TS_NODE_PROJECT environment variable ensures consistent TypeScript configuration across different execution environments.
validator-cli/src/utils/claim.test.ts (3)
17-19: Well-structured test parameter consolidation.Consolidating all parameters into a single
mockClaimParamsobject improves test readability and maintenance, matching the updatedgetClaimfunction signature that accepts a parameter object.Also applies to: 43-52
135-159: Excellent test coverage for fallback mechanism.This new test case verifies that claim data can be successfully retrieved from subgraph queries when blockchain event logs are unavailable, ensuring the robustness of the claim fetching logic.
175-179: Complete mock coverage for all fallback paths.Properly mocking all three fallback functions (
fetchClaimForEpoch,fetchVerificationForClaim, andfetchChallengerForClaim) ensures thorough testing of error scenarios.validator-cli/src/utils/logger.ts (7)
23-31: LGTM: Enhanced bot startup logging with role and network information.The updated
STARTEDevent handler now properly logs the bot's role (claimer, challenger, or both) and the networks it operates on, providing better visibility into the bot's configuration at startup.
33-35: LGTM: Added chain-specific watching logs.New log for monitoring specific chains on different networks, which enhances observability of the multi-network support.
45-47: LGTM: Added "No claim required" event logging.This helps in understanding when the bot decides not to make a claim for a specific epoch.
76-78: LGTM: Added transaction expiration logging.This event is helpful for tracking transaction lifecycle, especially for expired transactions that may need investigation.
80-86: LGTM: Added snapshot-specific event logging.These events directly support the new snapshot saving feature, providing visibility into when snapshots are saved and when the bot is waiting for the appropriate time to save snapshots.
89-113: LGTM: Added detailed claim state event logging.The enhanced claim state logs provide granular visibility into the claim lifecycle, which helps with monitoring and debugging the claim process.
141-143: LGTM: Added challenger outcome logging.This log provides visibility when a challenger has successfully challenged a claim.
validator-cli/src/utils/errors.ts (5)
27-33: LGTM: New error for invalid bot path.This error properly supports the multi-path architecture by providing clear feedback when an invalid path is provided.
35-41: LGTM: Added Devnet-specific error.This error provides a clear message when the Devnet owner address is not set, which is required for Devnet operation.
43-49: LGTM: Added network validation error.This error supports the multi-network functionality by validating network parameters and providing a helpful error message showing the allowed values.
51-57: LGTM: Added environment variable validation.This error helps identify missing environment variables with a clear error message.
59-67: LGTM: Updated exports.The exports have been properly updated to include all the new error classes.
validator-cli/src/utils/snapshot.test.ts (5)
19-68: LGTM: Comprehensive tests for isSnapshotNeeded function.These tests cover all the important scenarios for the
isSnapshotNeededfunction, including:
- No new messages when count is -1
- Count equals current count
- Snapshot already saved for current count
- Snapshot needed for new messages
This ensures the function correctly identifies when a new snapshot is needed.
69-100: LGTM: Fallback testing for isSnapshotNeeded.This test verifies the fallback mechanism when
queryFilterfails, ensuring that the function can still determine if a snapshot is needed using thefetchLastSavedMessagefunction.
103-152: LGTM: Tests for Testnet snapshot timing.These tests verify the Testnet-specific behavior where snapshots are only saved during the last 10 minutes of an epoch, aligning with the PR objectives.
153-176: LGTM: Test for snapshot necessity check.This test verifies that snapshots are not saved if there are no new messages, regardless of the timing, which is an important optimization.
178-202: LGTM: Test for Devnet snapshot behavior.This test verifies the Devnet-specific behavior where snapshots are saved at any time if needed, regardless of the epoch timing, aligning with the PR objectives.
validator-cli/src/utils/snapshot.ts (4)
12-21: LGTM: Well-structured interface for saveSnapshot parameters.The interface clearly defines all parameters needed for the saveSnapshot function, including optional parameters for better testability through dependency injection.
23-49: LGTM: Correctly implements network-specific snapshot saving logic.The
saveSnapshotfunction correctly implements the requirements outlined in the PR objectives:
- For non-Devnet networks (like Testnet), snapshots are only saved during the last 10 minutes of an epoch
- For all networks, snapshots are only saved if there are new messages to save
- The function emits appropriate events and returns updated state
This aligns perfectly with the PR objectives.
51-75: LGTM: Robust snapshot necessity check with fallback.The
isSnapshotNeededfunction properly checks if a new snapshot is needed by:
- Comparing current message count with known count
- Fetching the last saved snapshot count from logs
- Falling back to an alternative method if logs can't be queried
- Making the final determination based on message counts
The error handling and fallback mechanism make this function robust.
77-89: LGTM: Thorough message ID parsing with error handling.The
extractMessageIndexhelper function includes comprehensive error handling for invalid message ID formats and parsing errors, making it robust against unexpected inputs.validator-cli/src/utils/botEvents.ts (1)
17-19: Mis-aligned enum naming/value will confuse downstream consumers
SNAPSHOT_WAITINGis mapped to the string"snapshot_saving".
The name implies a waiting state while the value implies a saving state, which breaks the otherwise 1-to-1 mapping used throughout the enum and can easily lead to missedswitch/ifbranches in the logger and UI.- SNAPSHOT_WAITING = "snapshot_saving", + SNAPSHOT_WAITING = "snapshot_waiting", // or rename the key to SNAPSHOT_SAVINGPlease verify which semantic you actually need and align the identifier/value pair accordingly.
validator-cli/.env.dist (3)
3-4: Good addition of network configurationAdding the network configuration variable with clear documentation helps with multi-network support.
15-15: Chain IDs are well-definedGood practice to specify the chain IDs for outbox chains as environment variables rather than hardcoding them.
17-19: Nice addition of subgraph endpointsAdding GraphQL subgraph endpoints supports the new snapshot management feature and helps with claim verification, aligning with the PR objective to introduce snapshot saving capability.
validator-cli/src/utils/botConfig.ts (2)
5-9: Good bot path enumerationThe BotPaths enum is well-defined with clear comments explaining each path's purpose.
49-66: Robust network configuration parsingThe function correctly retrieves and validates network configurations from environment variables, enabling multi-network support as specified in the PR objectives.
validator-cli/src/utils/epochHandler.ts (3)
4-10: Good interface definition for parametersUsing a typed interface to replace positional parameters improves code readability and maintainability.
23-31: Well-structured parameter handlingThe refactored function correctly uses the new interface and properly initializes default values.
52-54: Clean and simple epoch calculationThe function provides a clear way to determine the latest challengeable epoch, which is helpful for the snapshot management feature.
validator-cli/src/ArbToEth/claimer.test.ts (4)
6-98: Well-structured test setupThe test setup is comprehensive with thorough mocking of dependencies and proper setup/teardown hooks. This ensures the tests are isolated and deterministic.
134-163: Good coverage of negative casesTests for scenarios where no action should be taken (no claim, no snapshot, no new messages) are well-implemented.
164-181: Devnet-specific behavior properly testedThe test correctly verifies that devnet environment advances state, which aligns with the PR description about saving snapshots in the devnet environment.
182-232: Comprehensive testnet case coverageTests cover all the important testnet-specific behaviors like claim creation, withdrawal, verification, etc. This aligns with the PR objectives for testnet snapshot handling.
validator-cli/src/ArbToEth/transactionHandler.ts (1)
366-385:saveSnapshot()sends a tx without a gas limit
Unlike other mutating calls, this method skipsestimateGas, relying on the node’s automatic limits which can be error-prone and expensive.
Align it with the rest of the handler: estimate gas first and pass an explicitgasLimit.-const saveSnapshotTxn = await this.veaInbox.saveSnapshot(); +const gasLimit = await this.veaInbox.estimateGas.saveSnapshot(); +const saveSnapshotTxn = await this.veaInbox.saveSnapshot({ gasLimit });
| import { getBotPath, BotPaths } from "./botConfig"; | ||
| import { InvalidBotPathError } from "./errors"; | ||
| describe("cli", () => { | ||
| describe("getBotPath", () => { | ||
| const defCommand = ["yarn", "start"]; | ||
| it("should return the default path", () => { | ||
| const path = getBotPath({ cliCommand: defCommand }); | ||
| expect(path).toEqual(BotPaths.BOTH); | ||
| }); | ||
| it("should return the claimer path", () => { | ||
| const command = ["yarn", "start", "--path=claimer"]; | ||
| const path = getBotPath({ cliCommand: command }); | ||
| expect(path).toEqual(BotPaths.CLAIMER); | ||
| }); | ||
| it("should return the challenger path", () => { | ||
| const command = ["yarn", "start", "--path=challenger"]; | ||
| const path = getBotPath({ cliCommand: command }); | ||
| expect(path).toEqual(BotPaths.CHALLENGER); | ||
| }); | ||
| it("should throw an error for invalid path", () => { | ||
| const command = ["yarn", "start", "--path=invalid"]; | ||
| expect(() => getBotPath({ cliCommand: command })).toThrow(new InvalidBotPathError()); | ||
| }); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Tests are missing coverage for saveSnapshot flag.
While the tests thoroughly cover path parsing logic, they don't test the --saveSnapshot flag handling which is a key feature mentioned in the PR objectives.
Add test cases to verify the snapshot saving behavior:
+ it("should detect saveSnapshot flag when present", () => {
+ const command = ["yarn", "start", "--saveSnapshot"];
+ const result = getBotPath({ cliCommand: command });
+ expect(result).toEqual({ path: BotPaths.BOTH, toSaveSnapshot: true });
+ });
+
+ it("should not detect saveSnapshot flag when absent", () => {
+ const command = ["yarn", "start"];
+ const result = getBotPath({ cliCommand: command });
+ expect(result).toEqual({ path: BotPaths.BOTH, toSaveSnapshot: false });
+ });
+
+ it("should handle both path and saveSnapshot flags together", () => {
+ const command = ["yarn", "start", "--path=claimer", "--saveSnapshot"];
+ const result = getBotPath({ cliCommand: command });
+ expect(result).toEqual({ path: BotPaths.CLAIMER, toSaveSnapshot: true });
+ });| transactionHandler = new ArbToEthTransactionHandler({ | ||
| network: Network.TESTNET, // Hardcoded as TESTNET & MAINNET have same contracts | ||
| epoch, | ||
| veaInbox, | ||
| veaOutbox, | ||
| veaInboxProvider, | ||
| veaOutboxProvider, | ||
| defaultEmitter, | ||
| claim | ||
| ); | ||
| emitter: defaultEmitter, | ||
| claim, | ||
| }); |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Consider making Network configurable rather than hardcoded.
While the comment explains why TESTNET is hardcoded, this approach could limit flexibility if contract addresses ever diverge between networks.
- network: Network.TESTNET, // Hardcoded as TESTNET & MAINNET have same contracts
+ network: process.env.NETWORK_TYPE as Network || Network.TESTNET, // Default to TESTNET as both networks have same contracts🏁 Script executed:
#!/bin/bash
# Check if there's any provision for network configuration in environment variables
grep -r "NETWORK_TYPE" --include="*.ts" --include="*.js" --include=".env*" .Length of output: 76
I’ll check for any existing network configuration support and inspect the validator implementation.
#!/bin/bash
# Search for any environment‐based or CLI‐based network config
grep -R "process.env" -n .
grep -R "import.*Network" -n .
grep -R "Network.TESTNET" -n .
grep -R "option(" -n validator-cli/src
grep -R "yargs" -n validator-cli/src
# Preview the top of the validator to see context around the hardcoded network
sed -n '1,200p' validator-cli/src/ArbToEth/validator.tsLet’s verify whether the CLI already exposes a network option or similar config before suggesting changes:
🏁 Script executed:
#!/bin/bash
# Check for a CLI flag or option for network
grep -R "option.*network" -n validator-cli/src
# Look for any yargs usage in the CLI
grep -R "yargs" -n validator-cli/src
# Find all occurrences of Network enum usage
grep -R "Network\." -n validator-cli/src
# Preview the top of the validator for context around imports and parsing
sed -n '1,200p' validator-cli/src/ArbToEth/validator.tsLength of output: 6317
Enable configurable network instead of hardcoding TESTNET
There’s currently no CLI flag or environment variable to select Network, so Network.TESTNET is always used—even though DEVNET or MAINNET configs might diverge in the future. To improve flexibility:
• Introduce a new CLI option (e.g. --network <DEVNET|TESTNET|MAINNET>) or read from process.env.NETWORK.
• Default to Network.TESTNET to preserve existing behavior.
• Update the handler instantiation in validator-cli/src/ArbToEth/validator.ts:
- transactionHandler = new ArbToEthTransactionHandler({
- network: Network.TESTNET, // Hardcoded as TESTNET & MAINNET have same contracts
+ const selectedNetwork = options.network
+ ? (options.network as Network)
+ : (process.env.NETWORK as Network) || Network.TESTNET;
+ transactionHandler = new ArbToEthTransactionHandler({
+ network: selectedNetwork, // Configurable network, defaulting to TESTNET
epoch,
veaInbox,
veaOutbox,
veaInboxProvider,
veaOutboxProvider,
emitter: defaultEmitter,
claim,
});This change ensures the CLI can target any supported network without modifying code.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| transactionHandler = new ArbToEthTransactionHandler({ | |
| network: Network.TESTNET, // Hardcoded as TESTNET & MAINNET have same contracts | |
| epoch, | |
| veaInbox, | |
| veaOutbox, | |
| veaInboxProvider, | |
| veaOutboxProvider, | |
| defaultEmitter, | |
| claim | |
| ); | |
| emitter: defaultEmitter, | |
| claim, | |
| }); | |
| const selectedNetwork = options.network | |
| ? (options.network as Network) | |
| : (process.env.NETWORK as Network) || Network.TESTNET; | |
| transactionHandler = new ArbToEthTransactionHandler({ | |
| network: selectedNetwork, // Configurable network, defaulting to TESTNET | |
| epoch, | |
| veaInbox, | |
| veaOutbox, | |
| veaInboxProvider, | |
| veaOutboxProvider, | |
| emitter: defaultEmitter, | |
| claim, | |
| }); |
| veaInbox, | ||
| veaOutbox, | ||
| veaInboxProvider, | ||
| veaOutboxProvider, | ||
| epoch, | ||
| emitter, | ||
| }: TransactionHandlerConstructor) { | ||
| super({ | ||
| epoch, | ||
| veaInbox, | ||
| veaOutbox, | ||
| veaInboxProvider, | ||
| veaOutboxProvider, | ||
| emitter, | ||
| } as TransactionHandlerConstructor); | ||
| this.veaOutboxDevnet = veaOutbox as VeaOutboxArbToEthDevnet; | ||
| } |
There was a problem hiding this comment.
Constructor discards mandatory network parameter by using an unsafe cast
TransactionHandlerConstructor requires a network field, yet the subclass omits it and then force-casts the
argument list with as TransactionHandlerConstructor.
Type safety is bypassed and super() receives network: undefined, which will break any logic relying on this.network
in the base class.
- constructor({
- veaInbox,
- veaOutbox,
- veaInboxProvider,
- veaOutboxProvider,
- epoch,
- emitter,
- }: TransactionHandlerConstructor) {
+ constructor({
+ network,
+ veaInbox,
+ veaOutbox,
+ veaInboxProvider,
+ veaOutboxProvider,
+ epoch,
+ emitter,
+ }: TransactionHandlerConstructor) {
super({
- epoch,
+ network,
+ epoch,
veaInbox,
veaOutbox,
veaInboxProvider,
veaOutboxProvider,
emitter,
} as TransactionHandlerConstructor);Remove the cast once the parameter list is complete so the compiler can protect you.
| }: CheckAndClaimParams) { | ||
| let outboxStateRoot = await veaOutbox.stateRoot(); | ||
| const claimAbleEpoch = Math.floor(now / (1000 * epochPeriod)) - 1; | ||
| if (!transactionHandler) { | ||
| const TransactionHandler = fetchTransactionHandler(chainId, network); | ||
| transactionHandler = new TransactionHandler({ | ||
| network, | ||
| epoch, | ||
| veaInbox, | ||
| veaOutbox, | ||
| veaInboxProvider, | ||
| veaOutboxProvider, | ||
| emitter, | ||
| claim, | ||
| }); | ||
| } else { | ||
| transactionHandler.claim = claim; | ||
| } | ||
|
|
There was a problem hiding this comment.
Protect remote RPC calls with try/catch to avoid crashing the watcher
All external calls (veaOutbox.stateRoot(), contract reads, etc.) are awaited without any error handling.
If the provider momentarily fails or returns a “connection reset” error the whole watcher will throw and stop, taking the validator offline.
Wrap the body of checkAndClaim() in a try/catch (and likewise the helper functions) and emit BotEvents.ERROR (or similar) so the supervisor can decide whether to retry or exit gracefully.
| const transactionHandlers: { [epoch: number]: any } = {}; | ||
| const toWatch: { [key: string]: { count: number; epochs: number[] } } = {}; | ||
| while (!shutDownSignal.getIsShutdownSignal()) { | ||
| for (const networkConfig of networkConfigs) { | ||
| await processNetwork(path, toSaveSnapshot, networkConfig, transactionHandlers, toWatch, emitter); | ||
| } | ||
| await wait(1000 * 10); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Transaction-handler map may collide across networks
transactionHandlers is keyed only by epoch, but the watcher iterates over multiple chains/networks.
Two different networks can share the same epoch number, causing handlers to be overwritten or reused incorrectly, leading to wrong gas estimations and double-spends.
Refactor to key by networkKey + epoch, e.g.:
-const transactionHandlers: { [epoch: number]: any } = {};
+const transactionHandlers: { [network: string]: { [epoch: number]: any } } = {};and update reads/writes:
-transactionHandlers[currentEpoch] = updatedTransactionHandler;
+transactionHandlers[networkKey] ??= {};
+transactionHandlers[networkKey][currentEpoch] = updatedTransactionHandler;Apply similar changes where transactionHandlers[epoch] is accessed.
Also applies to: 141-160, 210-215
| function getVeaInbox(veaInboxAddress: string, privateKey: string, rpcUrl: string, chainId: number, network) { | ||
| switch (chainId) { | ||
| case 11155111: | ||
| return VeaInboxArbToEth__factory.connect(veaInboxAddress, getWallet(privateKey, web3ProviderURL)); | ||
| return VeaInboxArbToEth__factory.connect(veaInboxAddress, getWallet(privateKey, rpcUrl)); | ||
| case 10200: | ||
| return VeaInboxArbToGnosis__factory.connect(veaInboxAddress, getWallet(privateKey, web3ProviderURL)); | ||
| return VeaInboxArbToGnosis__factory.connect(veaInboxAddress, getWallet(privateKey, rpcUrl)); | ||
| default: | ||
| throw new NotDefinedError("VeaInbox"); | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Missing type annotation for network breaks type-safety
getVeaInbox receives a network parameter but its type is implicit any.
Every other helper explicitly uses the Network enum, so this one should too for consistency and to enable exhaustive checks.
-function getVeaInbox(veaInboxAddress: string, privateKey: string, rpcUrl: string, chainId: number, network) {
+function getVeaInbox(
+ veaInboxAddress: string,
+ privateKey: string,
+ rpcUrl: string,
+ chainId: number,
+ network: Network
+) {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| function getVeaInbox(veaInboxAddress: string, privateKey: string, rpcUrl: string, chainId: number, network) { | |
| switch (chainId) { | |
| case 11155111: | |
| return VeaInboxArbToEth__factory.connect(veaInboxAddress, getWallet(privateKey, web3ProviderURL)); | |
| return VeaInboxArbToEth__factory.connect(veaInboxAddress, getWallet(privateKey, rpcUrl)); | |
| case 10200: | |
| return VeaInboxArbToGnosis__factory.connect(veaInboxAddress, getWallet(privateKey, web3ProviderURL)); | |
| return VeaInboxArbToGnosis__factory.connect(veaInboxAddress, getWallet(privateKey, rpcUrl)); | |
| default: | |
| throw new NotDefinedError("VeaInbox"); | |
| } | |
| } | |
| function getVeaInbox( | |
| veaInboxAddress: string, | |
| privateKey: string, | |
| rpcUrl: string, | |
| chainId: number, | |
| network: Network | |
| ) { | |
| switch (chainId) { | |
| case 11155111: | |
| return VeaInboxArbToEth__factory.connect(veaInboxAddress, getWallet(privateKey, rpcUrl)); | |
| case 10200: | |
| return VeaInboxArbToGnosis__factory.connect(veaInboxAddress, getWallet(privateKey, rpcUrl)); | |
| default: | |
| throw new NotDefinedError("VeaInbox"); | |
| } | |
| } |
| function getVeaRouter(veaRouterAddress: string, privateKey: string, rpcUrl: string, chainId: number) { | ||
| switch (chainId) { | ||
| case 10200: | ||
| return RouterArbToGnosis__factory.connect(veaRouterAddress, getWallet(privateKey, web3ProviderURL)); | ||
| return RouterArbToGnosis__factory.connect(veaRouterAddress, getWallet(privateKey, rpcUrl)); | ||
| } | ||
| } |
There was a problem hiding this comment.
getVeaRouter may silently return undefined
If chainId ≠ 10200 the function falls through without returning or throwing, yielding undefined and leading to downstream TypeErrors.
switch (chainId) {
case 10200:
return RouterArbToGnosis__factory.connect(veaRouterAddress, getWallet(privateKey, rpcUrl));
- }
+ default:
+ throw new NotDefinedError("VeaRouter");
+ }| try { | ||
| const [claimLogs, challengeLogs, verificationLogs] = await Promise.all([ | ||
| veaOutbox.queryFilter(veaOutbox.filters.Claimed(null, epoch, null), fromBlock, toBlock), | ||
| veaOutbox.queryFilter(veaOutbox.filters.Challenged(epoch, null), fromBlock, toBlock), | ||
| veaOutbox.queryFilter(veaOutbox.filters.VerificationStarted(epoch), fromBlock, toBlock), | ||
| ]); | ||
| claim.stateRoot = claimLogs[0].data; | ||
| claim.claimer = `0x${claimLogs[0].topics[1].slice(26)}`; | ||
| claim.timestampClaimed = (await veaOutboxProvider.getBlock(claimLogs[0].blockNumber)).timestamp; | ||
| if (verificationLogs.length > 0) { | ||
| claim.blocknumberVerification = verificationLogs[0].blockNumber; | ||
| claim.timestampVerification = (await veaOutboxProvider.getBlock(verificationLogs[0].blockNumber)).timestamp; | ||
| } | ||
| if (challengeLogs.length > 0) claim.challenger = "0x" + challengeLogs[0].topics[2].substring(26); | ||
| } catch { |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Accessing [0] without length checks relies on catch-all exception flow
claimLogs[0], challengeLogs[0], etc. are dereferenced before verifying that the arrays contain elements.
If the filters return no logs, the code throws and immediately falls back to the GraphQL path. While this “works”, it:
- Obscures the real failure reason.
- Incurs avoidable try/catch overhead.
- Masks legitimate on-chain data gaps.
Consider an explicit length check with an early fallback instead of relying on an exception:
- claim.stateRoot = claimLogs[0].data;
+ if (claimLogs.length === 0) throw new Error("Claim log not found");
+ claim.stateRoot = claimLogs[0].data;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| try { | |
| const [claimLogs, challengeLogs, verificationLogs] = await Promise.all([ | |
| veaOutbox.queryFilter(veaOutbox.filters.Claimed(null, epoch, null), fromBlock, toBlock), | |
| veaOutbox.queryFilter(veaOutbox.filters.Challenged(epoch, null), fromBlock, toBlock), | |
| veaOutbox.queryFilter(veaOutbox.filters.VerificationStarted(epoch), fromBlock, toBlock), | |
| ]); | |
| claim.stateRoot = claimLogs[0].data; | |
| claim.claimer = `0x${claimLogs[0].topics[1].slice(26)}`; | |
| claim.timestampClaimed = (await veaOutboxProvider.getBlock(claimLogs[0].blockNumber)).timestamp; | |
| if (verificationLogs.length > 0) { | |
| claim.blocknumberVerification = verificationLogs[0].blockNumber; | |
| claim.timestampVerification = (await veaOutboxProvider.getBlock(verificationLogs[0].blockNumber)).timestamp; | |
| } | |
| if (challengeLogs.length > 0) claim.challenger = "0x" + challengeLogs[0].topics[2].substring(26); | |
| } catch { | |
| try { | |
| const [claimLogs, challengeLogs, verificationLogs] = await Promise.all([ | |
| veaOutbox.queryFilter(veaOutbox.filters.Claimed(null, epoch, null), fromBlock, toBlock), | |
| veaOutbox.queryFilter(veaOutbox.filters.Challenged(epoch, null), fromBlock, toBlock), | |
| veaOutbox.queryFilter(veaOutbox.filters.VerificationStarted(epoch), fromBlock, toBlock), | |
| ]); | |
| + if (claimLogs.length === 0) throw new Error("Claim log not found"); | |
| claim.stateRoot = claimLogs[0].data; | |
| claim.claimer = `0x${claimLogs[0].topics[1].slice(26)}`; | |
| claim.timestampClaimed = (await veaOutboxProvider.getBlock(claimLogs[0].blockNumber)).timestamp; | |
| if (verificationLogs.length > 0) { | |
| claim.blocknumberVerification = verificationLogs[0].blockNumber; | |
| claim.timestampVerification = (await veaOutboxProvider.getBlock(verificationLogs[0].blockNumber)).timestamp; | |
| } | |
| if (challengeLogs.length > 0) claim.challenger = "0x" + challengeLogs[0].topics[2].substring(26); | |
| } catch { |
| const startVerificationTxHash = verificationFromGraph.startTxHash; | ||
| const txReceipt = await veaOutboxProvider.getTransactionReceipt(startVerificationTxHash); | ||
| claim.blocknumberVerification = txReceipt.blockNumber; | ||
| } | ||
| if (challengeFromGraph) claim.challenger = challengeFromGraph.challenger; |
There was a problem hiding this comment.
💡 Verification agent
❓ Verification inconclusive
getTransactionReceipt can return null for dropped or pruned transactions
txReceipt!.blockNumber is accessed without null check.
If the node has pruned the receipt (common on devnets) the call returns null, causing a crash.
- const txReceipt = await veaOutboxProvider.getTransactionReceipt(startVerificationTxHash);
- claim.blocknumberVerification = txReceipt.blockNumber;
+ const txReceipt = await veaOutboxProvider.getTransactionReceipt(
+ startVerificationTxHash
+ );
+ if (txReceipt) {
+ claim.blocknumberVerification = txReceipt.blockNumber;
+ }Ensure the provider you use guarantees historical receipts or add a retry with an archive node.
Guard against null TransactionReceipt
veaOutboxProvider.getTransactionReceipt returns Promise<TransactionReceipt | null>, so accessing blockNumber without a null check can crash if the receipt is missing (e.g., pruned or dropped transactions).
Apply a null check around the receipt and consider retrying against an archive node:
--- a/validator-cli/src/utils/claim.ts
+++ b/validator-cli/src/utils/claim.ts
@@ -84,7 +84,12 @@ async function buildClaim(...) {
const startVerificationTxHash = verificationFromGraph.startTxHash;
- const txReceipt = await veaOutboxProvider.getTransactionReceipt(startVerificationTxHash);
- claim.blocknumberVerification = txReceipt.blockNumber;
+ const txReceipt = await veaOutboxProvider.getTransactionReceipt(startVerificationTxHash);
+ if (txReceipt) {
+ claim.blocknumberVerification = txReceipt.blockNumber;
+ } else {
+ // TODO: receipt is null—either retry with an archive‐enabled RPC or log a warning
+ }Action items:
- File:
validator-cli/src/utils/claim.ts(lines 84–88) - Add a null check before using
txReceipt.blockNumber - Ensure your RPC provider supports historical receipts or implement a retry/backoff strategy against an archive node
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const startVerificationTxHash = verificationFromGraph.startTxHash; | |
| const txReceipt = await veaOutboxProvider.getTransactionReceipt(startVerificationTxHash); | |
| claim.blocknumberVerification = txReceipt.blockNumber; | |
| } | |
| if (challengeFromGraph) claim.challenger = challengeFromGraph.challenger; | |
| const startVerificationTxHash = verificationFromGraph.startTxHash; | |
| const txReceipt = await veaOutboxProvider.getTransactionReceipt(startVerificationTxHash); | |
| if (txReceipt) { | |
| claim.blocknumberVerification = txReceipt.blockNumber; | |
| } else { | |
| // TODO: receipt is null—either retry with an archive-enabled RPC or log a warning | |
| } | |
| } | |
| if (challengeFromGraph) claim.challenger = challengeFromGraph.challenger; |
| const CHAIN_ID = 11155111; | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Hard-coded CHAIN_ID severely limits re-use
The handler receives a network param but ignores it when fetching bridge config, always using the Sepolia ID 11155111.
This prevents running the bot on mainnet, devnet or any future chain without touching source code. Please make the chain id:
- an additional constructor argument (or derive it from the provider), and
- store it on the instance (
this.chainId) so every method can pass the correct value togetBridgeConfig.
-const CHAIN_ID = 11155111;
+export interface TransactionHandlerConstructor {
+ chainId: number;
+ /* …existing fields… */
+}
+
+private readonly chainId: number;
+
+constructor({ chainId, /* … */ }: TransactionHandlerConstructor) {
+ this.chainId = chainId;
+ /* … */
+}
+
-const { deposit } = getBridgeConfig(CHAIN_ID);
+const { deposit } = getBridgeConfig(this.chainId);
Save Snapshot
Added save snapshot functionality in validator-cli which is only triggered when validator-cli is ran with the flag
--saveSnapshot. For example with pm2:yarn pm2 start -- --saveSnapshotDifferent networks behaviour:
Fixes
PR-Codex overview
This PR primarily refactors the
validator-cliproject by removing unused files, updating configuration settings, enhancing error handling, and adding new features related to the devnet. It also improves the testing coverage and ensures better handling of claims.Detailed summary
devnet.ts,watcherDevnet.ts, and specific happy path scripts.tsconfig.jsonwith new compiler options.ecosystem.config.jsto change app names and scripts.package.jsonscripts related to devnet.getBotPathandepochHandler.Summary by CodeRabbit
New Features
Bug Fixes
Refactor
Tests
Chores