Feat/happy path(claimer) in validator-cli #396
Conversation
WalkthroughThe pull request encompasses extensive updates across the repository. The changes include updates to environment configurations, the introduction of new functions and classes for handling cross-chain claims and transactions, enhancements to error handling and logging utilities, and the addition of comprehensive unit tests. New utility modules manage epoch calculations, claim resolution, and configuration parsing, while a new watcher process orchestrates blockchain state monitoring. Overall, the modifications aim to improve claim management, transaction handling, and system observability across multiple modules. Changes
Sequence Diagram(s)sequenceDiagram
participant W as Watcher
participant L as Logger
participant B as BridgeConfig
participant P as Provider
participant C as ClaimModule
participant T as TxHandler
W->>L: Emit STARTED event (bot path, network)
W->>B: Retrieve bridge configuration
B-->>W: Return config data
W->>P: Initialize inbox/outbox providers
W->>P: Fetch latest block timestamp and derive epoch range
loop For each Epoch
W->>C: Fetch and evaluate claim state
alt Valid/unchallenged claim
C-->>W: Return VALID_CLAIM event
else No/invalid claim
W->>T: Initiate claim/challenge process
T-->>W: Process transaction and update state
end
W->>L: Log event for processed epoch
end
Suggested reviewers
Poem
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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: 23
🔭 Outside diff range comments (2)
validator-cli/src/utils/arbMsgExecutor.ts (1)
Line range hint
72-78: Inconsistent error handling between functions.In
getMessageStatus, the error for missing messages is logged to console, while inmessageExecutorit throws an error. Consider standardizing the error handling approach:- console.error("No child-to-parent messages found"); + throw new Error("No child-to-parent messages found");validator-cli/src/ArbToEth/claimer.test.ts (1)
Line range hint
386-386: Implement the pending test case.The test case is marked with
it.todo()but should be implemented to ensure complete test coverage.Implement the pending test:
it("should set challengeTxn as completed when txn is final", async () => { jest.spyOn(transactionHandler, "checkTransactionStatus").mockResolvedValue(3); const mockChallenge = jest.fn().mockResolvedValue({ hash: "0x1234" }) as any; (mockChallenge as any).estimateGas = jest.fn().mockResolvedValue(BigInt(100000)); veaOutbox["challenge(uint256,(bytes32,address,uint32,uint32,uint32,uint8,address))"] = mockChallenge; await transactionHandler.challengeClaim(); expect(transactionHandler.transactions.challengeTxn).toBeNull(); });
🧹 Nitpick comments (36)
validator-cli/src/utils/graphQueries.ts (1)
23-32: Improve GraphQL query formatting and add input validation.The query formatting could be improved, and the epoch parameter should be validated.
+ if (epoch < 0) { + throw new Error('Epoch must be a non-negative number'); + } const query = ` query GetClaimForEpoch($epoch: Int!) { claims(where: { epoch: $epoch }) { id bridger stateroot timestamp txHash challenged } } `; - const result = await request( - `${subgraph}`, - `{ - claims(where: {epoch: ${epoch}}) { - id - bridger - stateroot - timestamp - txHash - challenged - } - }` - ); + const result = await request( + subgraph, + query, + { epoch } + );validator-cli/src/utils/shutdown.ts (1)
11-17: Rename methods for clarity and conventionFor improved readability and adherence to conventions, consider renaming the methods in
ShutdownSignal:
- Rename
getIsShutdownSignaltoisShutdownto reflect a boolean returning method.- Rename
setShutdownSignaltosignalShutdownortriggerShutdownto indicate the action being performed.Apply this diff to rename the methods:
export class ShutdownSignal { private isShutdownSignal: boolean; constructor(initialState: boolean = false) { this.isShutdownSignal = initialState; } - public getIsShutdownSignal(): boolean { + public isShutdown(): boolean { return this.isShutdownSignal; } - public setShutdownSignal(): void { + public signalShutdown(): void { this.isShutdownSignal = true; } }Remember to update all references to these methods in other parts of the codebase accordingly.
validator-cli/src/utils/emitter.ts (1)
9-11: Optimize retrieval of 'BotEvents' valuesIn the
emitmethod,Object.values(BotEvents)is called every time an event is emitted, which could impact performance if the method is called frequently. Cache the values outside the method to prevent unnecessary computation.Apply this diff to cache the values:
import { EventEmitter } from "node:events"; import { BotEvents } from "./botEvents"; +const botEventValues = Object.values(BotEvents); + export const defaultEmitter = new EventEmitter(); export class MockEmitter extends EventEmitter { emit(event: string | symbol, ...args: any[]): boolean { // Prevent console logs for BotEvents during tests - if (Object.values(BotEvents).includes(event as BotEvents)) { + if (botEventValues.includes(event as BotEvents)) { return true; } return super.emit(event, ...args);This change reduces the overhead of converting the
BotEventsenum to an array on every emit.validator-cli/src/utils/botEvents.ts (2)
1-35: Add JSDoc documentation and consider state organization improvements.The enum provides a comprehensive set of states, but could benefit from:
- JSDoc comments explaining the purpose and context of each state
- A state transition diagram showing valid state progressions
- Consider prefixing related states (e.g.,
CLAIM_STARTED,CLAIM_VERIFYING) for better groupingExample documentation structure:
/** * Events emitted by the validator bot during its lifecycle. * * State transitions: * ``` * STARTED -> CHECKING -> [NO_CLAIM | VALID_CLAIM] * CLAIMING -> STARTING_VERIFICATION -> [VERIFICATION_CANT_START | VERIFYING_SNAPSHOT] * ... * ``` */ export enum BotEvents { /** * Emitted when the bot starts up. * @param chainId - The chain ID the bot is monitoring * @param path - The bot's operational path (challenger/claimer) */ STARTED = "started", // ... similar docs for other states }
14-28: Consider adding error states for claim operations.The claim states could be more robust by including failure states for key operations:
Consider adding:
// Claim state CLAIM_SUBMISSION_FAILED = "claim_submission_failed", CHALLENGE_SUBMISSION_FAILED = "challenge_submission_failed", DEPOSIT_WITHDRAWAL_FAILED = "deposit_withdrawal_failed",validator-cli/src/utils/logger.ts (1)
16-18: Remove redundant initialize function.The
initializefunction adds unnecessary indirection as it simply callsconfigurableInitialize.Consider removing it and exporting
configurableInitializeasinitialize:export const initialize = (emitter: EventEmitter) => { // Bridger state logs emitter.on(BotEvents.STARTED, (chainId: number, path: number) => { // ... }); // ... };validator-cli/src/ArbToEth/claimer.ts (1)
35-35: Correct the spelling of 'claimAbleEpoch' to 'claimableEpoch'The variable name
claimAbleEpochseems to have a typo. Rename it toclaimableEpochfor clarity and consistency with standard naming conventions.Apply this diff to fix the issue:
- const claimAbleEpoch = Math.floor(finalizedOutboxBlock.timestamp / epochPeriod); + const claimableEpoch = Math.floor(finalizedOutboxBlock.timestamp / epochPeriod);validator-cli/src/utils/ethers.ts (1)
74-81: Include 'chainId' in 'TransactionHandlerNotDefinedError' message for better debuggingWhen throwing
TransactionHandlerNotDefinedError, include thechainIdin the error message to aid in debugging.Assuming
TransactionHandlerNotDefinedErroraccepts a message parameter, apply this diff:const getTransactionHandler = (chainId: number) => { switch (chainId) { case 11155111: return ArbToEthTransactionHandler; default: - throw new TransactionHandlerNotDefinedError(); + throw new TransactionHandlerNotDefinedError(`Transaction handler not defined for chainId: ${chainId}`); } };validator-cli/src/utils/arbToEthState.ts (1)
176-179: Improve Error Handling in Event RetrievalSwallowing errors in the catch block may obscure underlying issues. Consider logging the actual error message to aid in debugging.
Apply this diff to log the error details:
} catch (e) { - console.log("Error finding batch containing block, searching heuristically..."); + console.error("Error finding batch containing block:", e); + console.log("Searching heuristically..."); }validator-cli/src/ArbToEth/transactionHandler.ts (1)
130-144: Add Documentation for Class MethodsEnhance code readability and maintainability by adding JSDoc comments to all public methods in the
ArbToEthTransactionHandlerclass. This will help other developers understand the purpose, parameters, and return values of each method.Example:
/** * Make a claim on the VeaOutbox (ETH). + * + * @param stateRoot - The state root to claim. */ const makeClaim = async (stateRoot: string) => { // method implementation };validator-cli/src/utils/errors.ts (2)
13-14: Fix inconsistent error name.The error name in the message ("NoClaimSetError") doesn't match the class name ("ClaimNotSetError").
- this.name = "NoClaimSetError"; + this.name = "ClaimNotSetError";
30-30: Fix message formatting in template literal.The current format will result in an extra comma and space after the last path. Consider using
join(", ")instead.- this.message = `Invalid path provided, Use one of: ${Object.keys(BotPaths).join("), ")}`; + this.message = `Invalid path provided. Use one of: ${Object.keys(BotPaths).join(", ")}`;validator-cli/src/utils/cli.test.ts (1)
20-23: Enhance error test case and add missing test cases.Consider the following improvements:
- Verify the error message in the invalid path test
- Add test cases for:
- Empty path value (--path=)
- Missing path value (--path)
- Case sensitivity (--path=CLAIMER vs --path=claimer)
Example enhancement for the current error test:
it("should throw an error for invalid path", () => { const command = ["yarn", "start", "--path=invalid"]; - expect(() => getBotPath({ cliCommand: command })).toThrow(new InvalidBotPathError()); + expect(() => getBotPath({ cliCommand: command })).toThrow(InvalidBotPathError); + expect(() => getBotPath({ cliCommand: command })).toThrow(/Invalid path provided/); });validator-cli/src/utils/cli.ts (1)
13-17: Improve JSDoc documentation.The current JSDoc is missing:
@param cliCommanddescription@throwssection for InvalidBotPathError- Parameter types
/** * Get the bot path from the command line arguments + * @param {Object} params - The parameters object + * @param {string[]} params.cliCommand - The command line arguments array + * @param {BotPaths} [params.defaultPath] - Default path to use if not specified * @returns {BotPaths} - The bot path + * @throws {InvalidBotPathError} When an invalid path is provided */validator-cli/src/consts/bridgeRoutes.ts (2)
24-27: Add type safety for environment variables.Consider adding runtime checks for required environment variables to fail fast if they're missing.
function getRequiredEnvVar(name: string): string { const value = process.env[name]; if (!value) { throw new Error(`Missing required environment variable: ${name}`); } return value; } // Usage in bridges object: inboxRPC: getRequiredEnvVar("RPC_ARB"), outboxRPC: getRequiredEnvVar("RPC_ETH"), // ... etcAlso applies to: 35-40
17-42: Document bridge configurations.Add JSDoc comments to document:
- Purpose of each chain configuration
- Units for time-based values (epochPeriod, minChallengePeriod, sequencerDelayLimit)
- Expected format for RPC URLs and addresses
Example:
/** * Bridge configurations for supported chains. * @property {Object} 11155111 - Sepolia testnet configuration * @property {Object} 10200 - Chiado testnet configuration */ const bridges: { [chainId: number]: Bridge } = { /** Sepolia testnet bridge configuration */ // ... existing config }, /** Chiado testnet bridge configuration */ // ... existing config }, };validator-cli/src/utils/epochHandler.test.ts (2)
15-23: Add test cases for edge scenarios insetEpochRange.Consider adding test cases for:
- Zero or negative current epoch
- Very large epoch numbers to check for overflow
- Edge cases around the cooldown period
27-35: Enhance test coverage forgetLatestChallengeableEpoch.The current test only covers a basic scenario. Consider adding tests for:
- Edge cases around epoch boundaries
- Error scenarios with invalid chain IDs
- Different timestamp values
validator-cli/src/utils/epochHandler.ts (2)
21-22: Extract magic numbers into named constants.The value
7 * 24 * 60 * 60represents a week in seconds. Consider extracting it into a named constant for better readability:+const ONE_WEEK_IN_SECONDS = 7 * 24 * 60 * 60; -const coldStartBacklog = 7 * 24 * 60 * 60; // when starting the watcher, specify an extra backlog to check +const coldStartBacklog = ONE_WEEK_IN_SECONDS; // when starting the watcher, specify an extra backlog to check
52-54: Update JSDoc example to match implementation.The example in the JSDoc comment shows
checkForNewEpochbut the function is namedgetLatestChallengeableEpochwith different parameters.validator-cli/src/ArbToEth/validator.test.ts (3)
33-41: Extract mock claim data into a fixture.The mock claim data contains magic strings and numbers. Consider extracting it into a separate test fixture file for reusability and maintainability:
// fixtures/mockClaim.ts export const mockClaim = { stateRoot: "0x1234", claimer: "0xFa00D29d378EDC57AA1006946F0fc6230a5E3288", timestampClaimed: 1234, timestampVerification: 0, blocknumberVerification: 0, honest: 0, challenger: ethers.ZeroAddress, };
58-87: Add negative test cases for error scenarios.The test suite should include cases for:
- Network failures
- Invalid transaction responses
- Error responses from contract calls
5-57: Consider using a test helper for mock setup.The mock setup in
beforeEachis quite lengthy. Consider creating a test helper function to encapsulate the mock setup:function createMockDependencies(overrides = {}) { const veaInbox = { snapshots: jest.fn(), provider: { getBlock: jest.fn(), }, }; // ... rest of the mock setup return { ...mockDeps, ...overrides }; }validator-cli/src/ArbToEth/claimer.test.ts (6)
121-121: Fix typo in test name.The test name contains a typo: "calim" should be "claim".
- it("should make a valid calim if no claim is made", async () => { + it("should make a valid claim if no claim is made", async () => {
135-135: Fix typo in test name.The test name contains a typo: "calim" should be "claim".
- it("should make a valid calim if last claim was challenged", async () => { + it("should make a valid claim if last claim was challenged", async () => {
92-98: Add test cases for invalid inputs.The test suite should include error cases for invalid stateRoot and epoch values to ensure proper error handling.
Add the following test cases:
it("should throw error for invalid stateRoot", async () => { mockGetClaim = jest.fn().mockReturnValue(null); await expect(checkAndClaim({ ...mockDeps, stateRoot: "invalid" })) .rejects.toThrow("Invalid stateRoot"); }); it("should throw error for invalid epoch", async () => { mockGetClaim = jest.fn().mockReturnValue(null); await expect(checkAndClaim({ ...mockDeps, epoch: -1 })) .rejects.toThrow("Invalid epoch"); });
83-83: Fix typo in comment.Remove the extra "ß" character from the comment.
- .mockImplementationOnce(() => Promise.resolve([])); // For VerificationStartedß + .mockImplementationOnce(() => Promise.resolve([])); // For VerificationStarted
Line range hint
188-243: Add test for concurrent claim resolution.The test suite should verify that the claim resolution process handles concurrent operations correctly.
Add a test case to verify concurrent claim resolution:
it("should handle concurrent claim resolution", async () => { const promises = [ getClaimResolveState(veaInbox, veaInboxProvider, veaOutboxProvider, epoch, blockNumberOutboxLowerBound, toBlock), getClaimResolveState(veaInbox, veaInboxProvider, veaOutboxProvider, epoch, blockNumberOutboxLowerBound, toBlock) ]; const results = await Promise.all(promises); expect(results[0]).toEqual(results[1]); });
Line range hint
142-182: Add test for gas estimation failure.The test suite should verify error handling when gas estimation fails.
Add a test case for gas estimation failure:
it("should handle gas estimation failure", async () => { jest.spyOn(transactionHandler, "checkTransactionStatus").mockResolvedValue(0); const mockClaim = jest.fn() as any; (mockClaim as any).estimateGas = jest.fn().mockRejectedValue(new Error("Gas estimation failed")); veaOutbox["claim(uint256,bytes32)"] = mockClaim; await expect(transactionHandler.makeClaim(claim.stateRoot as string)) .rejects.toThrow("Gas estimation failed"); });validator-cli/.env.dist (4)
9-11: Consider adding fallback RPC providers.Using public RPC endpoints may lead to rate limiting issues in production. Consider:
- Adding backup RPC endpoints
- Using dedicated RPC providers
- Documenting rate limits of these public endpoints
14-21: Enhance address documentation with network information.While the addresses are well-labeled, consider adding:
- The specific network/chain these addresses are deployed on (e.g., Sepolia, Chiado)
- Links to block explorers for address verification
- Version/deployment date of these contracts
23-23: Document the chain ID's network.Add a comment to clarify that 421611 is the Arbitrum Sepolia testnet chain ID. This helps prevent confusion and makes it easier to update for different environments.
+# Chain ID for Arbitrum Sepolia testnet VEAOUTBOX_CHAIN_ID=421611
Line range hint
1-1: Enhance private key security guidance.Consider adding comments about:
- Never committing actual private keys
- Using secure key management solutions
- Required key permissions/capabilities
+# WARNING: Never commit actual private keys. Use secure key management in production. +# Required permissions: transaction signing for bridge operations PRIVATE_KEY=validator-cli/src/utils/claim.test.ts (1)
188-245: Consider adding more test cases forgetClaimResolveState.While the current tests cover the basic scenarios, consider adding tests for:
- Error handling
- Edge cases with different message statuses
- Invalid inputs
validator-cli/src/ArbToEth/transactionHandler.test.ts (2)
477-513: Add more test cases forresolveChallengedClaim.Consider adding tests for:
- Error handling when
mockMessageExecutorfails- Validation of input parameters
- Edge cases with different transaction states
8-45: Consider using TypeScript interfaces for mock objects.Instead of using
anytype for mock objects likeveaInbox,veaOutbox, etc., consider defining interfaces to improve type safety and maintainability.interface MockVeaInbox { sendSnapshot: jest.Mock; } interface MockVeaOutbox { estimateGas: { claim: jest.Mock; }; withdrawChallengeDeposit: jest.Mock; claim: jest.Mock; // ... other methods }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (27)
validator-cli/.env.dist(1 hunks)validator-cli/jest.config.ts(1 hunks)validator-cli/package.json(2 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(1 hunks)validator-cli/src/ArbToEth/transactionHandler.ts(1 hunks)validator-cli/src/ArbToEth/validator.test.ts(1 hunks)validator-cli/src/ArbToEth/validator.ts(1 hunks)validator-cli/src/ArbToEth/watcherArbToEth.ts(0 hunks)validator-cli/src/consts/bridgeRoutes.ts(1 hunks)validator-cli/src/utils/arbMsgExecutor.ts(3 hunks)validator-cli/src/utils/arbToEthState.ts(1 hunks)validator-cli/src/utils/botEvents.ts(1 hunks)validator-cli/src/utils/claim.test.ts(1 hunks)validator-cli/src/utils/claim.ts(1 hunks)validator-cli/src/utils/cli.test.ts(1 hunks)validator-cli/src/utils/cli.ts(1 hunks)validator-cli/src/utils/emitter.ts(1 hunks)validator-cli/src/utils/epochHandler.test.ts(1 hunks)validator-cli/src/utils/epochHandler.ts(1 hunks)validator-cli/src/utils/errors.ts(1 hunks)validator-cli/src/utils/ethers.ts(2 hunks)validator-cli/src/utils/graphQueries.ts(1 hunks)validator-cli/src/utils/logger.ts(1 hunks)validator-cli/src/utils/shutdown.ts(1 hunks)validator-cli/src/watcher.ts(1 hunks)
💤 Files with no reviewable changes (1)
- validator-cli/src/ArbToEth/watcherArbToEth.ts
✅ Files skipped from review due to trivial changes (1)
- validator-cli/jest.config.ts
🧰 Additional context used
📓 Learnings (6)
validator-cli/src/utils/epochHandler.test.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/graphQueries.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/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/arbMsgExecutor.ts (1)
Learnt from: madhurMongia
PR: kleros/vea#359
File: validator-cli/src/utils/arbMsgExecutor.ts:44-44
Timestamp: 2024-11-20T11:50:19.381Z
Learning: In `validator-cli/src/utils/arbMsgExecutor.ts`, when calling `childToParentMessage.execute()`, pass `childProvider` as the argument since `childToParentMessage` already contains the parent (Ethereum) RPC context internally.
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/.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.
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: test
- GitHub Check: Scorecard analysis
- GitHub Check: dependency-review
- GitHub Check: Analyze (javascript)
🔇 Additional comments (21)
validator-cli/src/utils/graphQueries.ts (2)
3-10: LGTM! Well-structured interface definition.The
ClaimDatainterface clearly defines the shape of claim data with appropriate types.
65-65: LGTM! Clean exports.The exports are well-organized and follow TypeScript best practices.
validator-cli/src/watcher.ts (1)
60-65:⚠️ Potential issueCheck for unintended overwriting of 'updatedTransactions'
The
updatedTransactionsvariable may be assigned in both conditional blocks. If both conditions are true, the second assignment will overwrite the first. Verify if overwriting is intended; otherwise, consider combining the updates or using separate variables.validator-cli/src/ArbToEth/claimer.ts (3)
72-72: Consider handling the case when 'claim' is null and 'epoch' does not equal 'claimableEpoch'In the
elseblock, you're emittingCLAIM_EPOCH_PASSEDwhenclaimisnull. However, this might not adequately handle scenarios whereepochis not equal toclaimableEpoch. Ensure that all possible cases are correctly handled to avoid unexpected behavior.
51-51: Check for potential race conditions when fetching 'savedSnapshot' and 'claimData'Fetching
savedSnapshotandclaimDataconcurrently is efficient, but ensure that both values are up-to-date and consistent with each other to prevent race conditions.
36-36:⚠️ Potential issueValidate the division operation for 'claimableEpoch'
Ensure that
epochPeriodis not zero to prevent a potential division by zero error when calculatingclaimableEpoch.Add a check before the calculation:
+ if (epochPeriod === 0) { + throw new Error("epochPeriod must not be zero."); + } const claimableEpoch = Math.floor(finalizedOutboxBlock.timestamp / epochPeriod);Likely invalid or redundant comment.
validator-cli/src/ArbToEth/validator.ts (4)
46-54: Validate calculation of 'blockNumberOutboxLowerBound'Ensure that the calculation of
blockNumberOutboxLowerBoundaccurately reflects the intended block range and handles edge cases appropriately. Incorrect calculations may lead to inefficient queries or missed events.
94-94: Handle potential errors when sending snapshotsWhen calling
transactionHandler.sendSnapshot(), ensure that errors are properly caught and handled to prevent the application from crashing.[error_handling]
Consider wrapping the call in a try-catch block:
if (!claimResolveState.sendSnapshot.status) { - await transactionHandler.sendSnapshot(); + try { + await transactionHandler.sendSnapshot(); + } catch (error) { + // Handle the error appropriately + emitter.emit(BotEvents.ERROR_SENDING_SNAPSHOT, error); + } } else if (claimResolveState.execution.status == 1) {
93-102: Ensure all claim resolve states are handledVerify that all possible
statusvalues forclaimResolveState.execution.statusare handled. Missing cases may lead to unhandled scenarios and unexpected behavior.
77-77:⚠️ Potential issueCorrect the use of 'ethers.ZeroAddress' to 'ethers.constants.AddressZero'
The property
ethers.ZeroAddressdoes not exist. The correct constant for the zero address in ethers.js isethers.constants.AddressZero. Update the code to use the correct constant.Apply this diff to fix the issue:
- if (claimSnapshot != claim.stateRoot && claim.challenger == ethers.ZeroAddress) { + if (claimSnapshot != claim.stateRoot && claim.challenger == ethers.constants.AddressZero) {Likely invalid or redundant comment.
validator-cli/src/utils/claim.ts (1)
129-142: Verify Hashing Logic Matches Solidity ImplementationEnsure that the
hashClaimfunction accurately replicates the hashing logic used in the Solidity smart contract. Any mismatch in field order or types could result in incorrect hash computations.Run the following script to compare the JavaScript hash with the Solidity hash:
✅ Verification successful
Hashing Logic Matches Solidity Implementation ✅
The JavaScript implementation in
hashClaimcorrectly matches the Solidity contract. Both use the same field order, types, and hashing mechanism:
- Same field ordering for the Claim struct
- Matching types (bytes32, address, uint32, uint8 for enum, etc.)
- Both use keccak256 with packed encoding
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that the hash computed in JavaScript matches the one in the Solidity contract. # Navigate to the directory containing the contract cd contracts/arbitrumToEth # Compile the contract and extract the hashing logic cat <<EOF > VerifyHash.sol pragma solidity ^0.8.0; import "./VeaInboxArbToEth.sol"; contract VerifyHash { function hashClaim(VeaInboxArbToEth.Claim memory claim) public pure returns (bytes32) { return keccak256(abi.encodePacked( claim.stateRoot, claim.claimer, claim.timestampClaimed, claim.timestampVerification, claim.blocknumberVerification, claim.honest, claim.challenger )); } } EOF # Compile the contract using solc solc --bin --abi VerifyHash.sol -o build/ # Note: Ensure the ABI and bytecode match the JavaScript implementation. # Compare the hashes (This requires further implementation based on your testing framework)Length of output: 300
Script:
#!/bin/bash # Find Solidity contract files fd -e sol # Search for claim hashing implementation rg -U "function.*claim.*keccak256" --type solLength of output: 3117
Script:
#!/bin/bash # Search for claim hashing implementation without file type restriction rg -U "keccak256.*claim" # Search for Claim struct definition rg -U "struct Claim" # Get content of the main inbox contract cat contracts/src/arbitrumToEth/VeaInboxArbToEth.solLength of output: 113755
Script:
#!/bin/bash # Get the Claim struct definition cat contracts/src/interfaces/types/VeaClaim.sol # Get the hashClaim implementation from VeaOutboxArbToEth cat contracts/src/arbitrumToEth/VeaOutboxArbToEth.solLength of output: 26016
validator-cli/src/utils/arbToEthState.ts (1)
231-235: Ensure Correct Binary Search ImplementationVerify that the binary search logic in
findLatestL2BatchAndBlockis correctly implemented to prevent off-by-one errors, which could lead to incorrect batch and block numbers.validator-cli/src/ArbToEth/transactionHandler.ts (1)
246-267: 🛠️ Refactor suggestionReview Gas Fee Calculations in
challengeClaimThe calculations for
maxFeePerGasProfitableandmaxPriorityFeePerGasMEVmay result in transactions with insufficient gas fees, especially under volatile network conditions. It's important to ensure gas prices are set appropriately to prevent transaction failures.Consider revising the gas fee logic as follows:
const gasEstimate: bigint = await this.veaOutbox[ "challenge(uint256,(bytes32,address,uint32,uint32,uint32,uint8,address))" ].estimateGas(this.epoch, this.claim, { value: deposit }); -const maxFeePerGasProfitable = deposit / (gasEstimate * BigInt(6)); - -// Set a reasonable maxPriorityFeePerGas but ensure it's lower than maxFeePerGas -let maxPriorityFeePerGasMEV = BigInt(6667000000000); // 6667 gwei - -// Ensure maxPriorityFeePerGas <= maxFeePerGas -if (maxPriorityFeePerGasMEV > maxFeePerGasProfitable) { - maxPriorityFeePerGasMEV = maxFeePerGasProfitable; -} +const gasPrice = await this.veaOutboxProvider.getGasPrice(); +// Adjust gas price based on network conditions +const maxFeePerGas = gasPrice.mul(2); // For example, set max fee per gas to twice the current gas price +const maxPriorityFeePerGas = gasPrice; // Set priority fee equal to current gas price const challengeTxn = await this.veaOutbox[ "challenge(uint256,(bytes32,address,uint32,uint32,uint32,uint8,address))" ](this.epoch, this.claim, { - maxFeePerGas: maxFeePerGasProfitable, - maxPriorityFeePerGas: maxPriorityFeePerGasMEV, + maxFeePerGas, + maxPriorityFeePerGas, value: deposit, gasLimit: gasEstimate, });Run the following script to verify gas price sufficiency:
validator-cli/src/utils/arbMsgExecutor.ts (1)
22-26: 🛠️ Refactor suggestionAdd validation for environment variables.
The
PRIVATE_KEYenvironment variable is used without validation. Consider adding a check at the start of the function:async function messageExecutor( trnxHash: string, childJsonRpc: JsonRpcProvider, parentProvider: JsonRpcProvider ): Promise<ContractTransaction> { const PRIVATE_KEY = process.env.PRIVATE_KEY; + if (!PRIVATE_KEY) { + throw new Error("PRIVATE_KEY environment variable is not set"); + } const childProvider = new ArbitrumProvider(childJsonRpc);⛔ Skipped due to learnings
Learnt from: fcanela PR: kleros/vea#344 File: validator-cli/src/utils/arbMsgExecutor.ts:14-14 Timestamp: 2024-11-19T10:25:55.588Z Learning: In the `validator-cli/src/utils/arbMsgExecutor.ts` file, checks for commonly required environment variables like `PRIVATE_KEY` should be performed earlier in the application lifecycle, before individual functions are executed.Learnt from: madhurMongia PR: kleros/vea#359 File: validator-cli/src/utils/arbMsgExecutor.ts:13-13 Timestamp: 2024-11-20T11:50:12.303Z Learning: In the `validator-cli` codebase, when initializing `Wallet` instances in TypeScript, it's acceptable to rely on the `Wallet` class's built-in validation of the private key retrieved from environment variables, without adding explicit checks for the existence of `PRIVATE_KEY`.validator-cli/src/ArbToEth/claimer.test.ts (1)
13-18: LGTM! Good test setup.The changes to package.json are well-structured:
- Added Jest dependencies with appropriate versions
- Added test script with coverage reporting
- Updated start script path correctly
Also applies to: 32-34
validator-cli/src/utils/claim.test.ts (3)
1-11: LGTM! Well-structured test setup.The imports are appropriate, and the mock data is well-organized with clear documentation.
12-140: LGTM! Comprehensive test coverage forgetClaim.The test suite thoroughly covers:
- Happy path with valid claims
- Claims with challengers
- Claims with verification
- Error cases for missing claims
- Input validation
162-186: LGTM! Good test coverage forhashClaim.Tests verify both successful hash generation and hash validation when claim properties change.
validator-cli/src/ArbToEth/transactionHandler.test.ts (1)
386-386: Implement the pending test case.The
it.todo()for checking final transaction state should be implemented to ensure complete test coverage.validator-cli/package.json (2)
13-18: LGTM! Good test configuration setup.The script changes and test configuration look appropriate:
- Start script path updated correctly
- Test script includes coverage reporting
32-34: Verify Jest dependency versions.The Jest-related dependencies use caret ranges. While this is generally acceptable, consider if you want to pin these versions for more predictable builds.
Run this script to check for any known issues with the specified versions:
✅ Verification successful
Jest dependencies are properly configured ✓
The specified versions are mutually compatible, with all packages on major version 29. The caret ranges are appropriate for dev dependencies, allowing for minor updates while preventing breaking changes.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for any known issues with Jest dependencies # Check npm for any security advisories npm audit @types/[email protected] [email protected] [email protected] # Check for compatibility between versions npm info @types/[email protected] peerDependencies npm info [email protected] peerDependenciesLength of output: 674
| * */ | ||
| const getClaimForEpoch = async (epoch: number): Promise<ClaimData | undefined> => { | ||
| try { | ||
| const subgraph = process.env.VEAOUTBOX_SUBGRAPH; |
There was a problem hiding this comment.
Add validation for the subgraph URL.
The subgraph URL from environment variables should be validated to ensure it's defined and properly formatted.
- const subgraph = process.env.VEAOUTBOX_SUBGRAPH;
+ const subgraph = process.env.VEAOUTBOX_SUBGRAPH;
+ if (!subgraph) {
+ throw new Error('VEAOUTBOX_SUBGRAPH environment variable is not defined');
+ }
+ if (!subgraph.startsWith('http')) {
+ throw new Error('VEAOUTBOX_SUBGRAPH must be a valid URL');
+ }📝 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 subgraph = process.env.VEAOUTBOX_SUBGRAPH; | |
| const subgraph = process.env.VEAOUTBOX_SUBGRAPH; | |
| if (!subgraph) { | |
| throw new Error('VEAOUTBOX_SUBGRAPH environment variable is not defined'); | |
| } | |
| if (!subgraph.startsWith('http')) { | |
| throw new Error('VEAOUTBOX_SUBGRAPH must be a valid URL'); | |
| } |
There was a problem hiding this comment.
@mani99brar Thanks for confirming! Adding this validation will help make your code more robust by preventing runtime errors when the environment variable is missing or malformed. Let me know if you need any other assistance with your implementation.
🧠 Learnings used
Learnt from: mani99brar
PR: kleros/vea#370
File: bridger-cli/src/utils/graphQueries.ts:12-34
Timestamp: 2025-03-28T01:51:35.847Z
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.
| } catch (e) { | ||
| console.log(e); | ||
| return undefined; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve error handling.
The current error handling could be enhanced by:
- Using a more specific error type
- Implementing proper error logging
- } catch (e) {
- console.log(e);
+ } catch (e: unknown) {
+ console.error('Failed to fetch claim for epoch:', epoch, e instanceof Error ? e.message : e);Committable suggestion skipped: line range outside the PR's diff.
There was a problem hiding this comment.
Will add a custom error.
There was a problem hiding this comment.
@mani99brar That's a good approach! Creating a custom error class will make error handling more robust and consistent with the rest of your codebase. You might consider something like ClaimFetchError or GraphQueryError to maintain naming consistency with your existing error classes.
🧠 Learnings used
Learnt from: mani99brar
PR: kleros/vea#370
File: bridger-cli/src/utils/graphQueries.ts:12-34
Timestamp: 2025-03-28T01:51:35.847Z
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.
| const getLastClaimedEpoch = async (): Promise<ClaimData> => { | ||
| const subgraph = process.env.VEAOUTBOX_SUBGRAPH; | ||
|
|
||
| const result = await request( | ||
| `${subgraph}`, | ||
| `{ | ||
| claims(first:1, orderBy:timestamp, orderDirection:desc){ | ||
| id | ||
| bridger | ||
| stateroot | ||
| timestamp | ||
| challenged | ||
| txHash | ||
| } | ||
|
|
||
| }` | ||
| ); | ||
| return result[`claims`][0]; | ||
| }; |
There was a problem hiding this comment.
Add error handling and improve robustness.
The function needs error handling and better handling of edge cases.
const getLastClaimedEpoch = async (): Promise<ClaimData> => {
- const subgraph = process.env.VEAOUTBOX_SUBGRAPH;
-
- const result = await request(
- `${subgraph}`,
- `{
- claims(first:1, orderBy:timestamp, orderDirection:desc){
- id
- bridger
- stateroot
- timestamp
- challenged
- txHash
- }
-
- }`
- );
- return result[`claims`][0];
+ try {
+ const subgraph = process.env.VEAOUTBOX_SUBGRAPH;
+ if (!subgraph) {
+ throw new Error('VEAOUTBOX_SUBGRAPH environment variable is not defined');
+ }
+
+ const query = `
+ query GetLastClaimedEpoch {
+ claims(first: 1, orderBy: timestamp, orderDirection: desc) {
+ id
+ bridger
+ stateroot
+ timestamp
+ challenged
+ txHash
+ }
+ }
+ `;
+
+ const result = await request(subgraph, query);
+
+ if (!result.claims?.[0]) {
+ throw new Error('No claims found');
+ }
+
+ return result.claims[0];
+ } catch (e: unknown) {
+ console.error('Failed to fetch last claimed epoch:', e instanceof Error ? e.message : e);
+ throw e;
+ }
};📝 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 getLastClaimedEpoch = async (): Promise<ClaimData> => { | |
| const subgraph = process.env.VEAOUTBOX_SUBGRAPH; | |
| const result = await request( | |
| `${subgraph}`, | |
| `{ | |
| claims(first:1, orderBy:timestamp, orderDirection:desc){ | |
| id | |
| bridger | |
| stateroot | |
| timestamp | |
| challenged | |
| txHash | |
| } | |
| }` | |
| ); | |
| return result[`claims`][0]; | |
| }; | |
| const getLastClaimedEpoch = async (): Promise<ClaimData> => { | |
| try { | |
| const subgraph = process.env.VEAOUTBOX_SUBGRAPH; | |
| if (!subgraph) { | |
| throw new Error('VEAOUTBOX_SUBGRAPH environment variable is not defined'); | |
| } | |
| const query = ` | |
| query GetLastClaimedEpoch { | |
| claims(first: 1, orderBy: timestamp, orderDirection: desc) { | |
| id | |
| bridger | |
| stateroot | |
| timestamp | |
| challenged | |
| txHash | |
| } | |
| } | |
| `; | |
| const result = await request(subgraph, query); | |
| if (!result.claims?.[0]) { | |
| throw new Error('No claims found'); | |
| } | |
| return result.claims[0]; | |
| } catch (e: unknown) { | |
| console.error('Failed to fetch last claimed epoch:', e instanceof Error ? e.message : e); | |
| throw e; | |
| } | |
| }; |
validator-cli/src/watcher.ts
Outdated
| if (require.main === module) { | ||
| const shutDownSignal = new ShutdownSignal(false); | ||
| watch(shutDownSignal); | ||
| } |
There was a problem hiding this comment.
Handle unhandled Promise rejections when calling 'watch'
The watch function is asynchronous but is invoked without await or a .catch() handler. This may lead to unhandled Promise rejections. Wrap the call in an async IIFE (Immediately Invoked Function Expression) and add error handling to catch potential errors.
Apply this diff to handle the Promise properly:
if (require.main === module) {
const shutDownSignal = new ShutdownSignal(false);
- watch(shutDownSignal);
+ (async () => {
+ try {
+ await watch(shutDownSignal);
+ } catch (error) {
+ // Handle error appropriately
+ console.error("An error occurred during watch execution:", error);
+ process.exit(1);
+ }
+ })();
}📝 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.
| if (require.main === module) { | |
| const shutDownSignal = new ShutdownSignal(false); | |
| watch(shutDownSignal); | |
| } | |
| if (require.main === module) { | |
| const shutDownSignal = new ShutdownSignal(false); | |
| (async () => { | |
| try { | |
| await watch(shutDownSignal); | |
| } catch (error) { | |
| // Handle error appropriately | |
| console.error("An error occurred during watch execution:", error); | |
| process.exit(1); | |
| } | |
| })(); | |
| } |
There was a problem hiding this comment.
@mani99brar Understood, I'll respect your decision to let it fail without additional error handling. The suggestion has been withdrawn.
validator-cli/src/utils/claim.ts
Outdated
| claim.stateRoot = claimLogs[0].data; | ||
| claim.claimer = `0x${claimLogs[0].topics[1].slice(26)}`; | ||
| claim.timestampClaimed = (await veaOutboxProvider.getBlock(claimLogs[0].blockNumber)).timestamp; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use Contract Interface to Parse Event Logs
Directly accessing the data and topics fields of event logs can be error-prone and less readable. It's recommended to use the contract's interface to parse logs for better reliability and maintainability.
Apply this diff to utilize the contract interface for parsing:
-claim.stateRoot = claimLogs[0].data;
-claim.claimer = `0x${claimLogs[0].topics[1].slice(26)}`;
+const parsedClaimLog = veaOutbox.interface.parseLog(claimLogs[0]);
+claim.stateRoot = parsedClaimLog.args._stateRoot;
+claim.claimer = parsedClaimLog.args._claimer;Committable suggestion skipped: line range outside the PR's diff.
There was a problem hiding this comment.
veaOutbox has any type so it doesn't seem much useful to use it of parsing.
There was a problem hiding this comment.
@mani99brar You're absolutely right. Since veaOutbox is typed as any, TypeScript won't provide type checking benefits either way, so using interface.parseLog() doesn't give you any advantages from a typing perspective. Your current approach works fine given the type constraints you're working with.
✏️ Learnings added
Learnt from: mani99brar
PR: kleros/vea#396
File: validator-cli/src/utils/claim.ts:46-48
Timestamp: 2025-03-31T09:03:12.046Z
Learning: When dealing with blockchain event logs in the validator-cli, direct access to log properties (like data and topics) is preferred when the contract variable is typed as `any`, rather than using interface parsing methods like parseLog().
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
|
|
||
| const l2Network = await getArbitrumNetwork(ArbProvider); | ||
| const sequencer = SequencerInbox__factory.connect(l2Network.ethBridge.sequencerInbox, EthProvider); | ||
| const maxDelaySeconds = Number(await sequencer.maxTimeVariation()); |
There was a problem hiding this comment.
Fix Incorrect Usage of maxTimeVariation
The maxTimeVariation function returns a struct containing multiple fields (delayBlocks, futureBlocks, delaySeconds, futureSeconds), not a single number. Converting it directly to a number will lead to incorrect results.
Apply this diff to correct the usage:
-const maxDelaySeconds = Number(await sequencer.maxTimeVariation());
+const maxTimeVariation = await sequencer.maxTimeVariation();
+const maxDelaySeconds = Number(maxTimeVariation.delaySeconds);📝 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 maxDelaySeconds = Number(await sequencer.maxTimeVariation()); | |
| const maxTimeVariation = await sequencer.maxTimeVariation(); | |
| const maxDelaySeconds = Number(maxTimeVariation.delaySeconds); |
There was a problem hiding this comment.
The usage is correct, why do you think it returns multiple fields?
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Let's check the structure of SequencerInbox in the Arbitrum SDK
cd validator-cli
npm list @arbitrum/sdk || echo "Package not found"
find node_modules/@arbitrum -type f -name "SequencerInbox*.ts" | xargs grep -l "maxTimeVariation"
Length of output: 286
🏁 Script executed:
#!/bin/bash
# Let's check for imports of SequencerInbox in the codebase
find . -type f -name "*.ts" | xargs grep -l "SequencerInbox"
# Let's look specifically for maxTimeVariation usage
find . -type f -name "*.ts" | xargs grep -l "maxTimeVariation"
# Let's also check if there are any type definitions for SequencerInbox
find . -type f -name "*.d.ts" | xargs grep -l "SequencerInbox"
Length of output: 490
🏁 Script executed:
#!/bin/bash
# Let's check how maxTimeVariation is used in the other file
cat ./validator-cli/src/ArbToGnosis/watcherArbToGnosis.ts | grep -A 5 -B 5 "maxTimeVariation"
# Let's look at the arbToEthState.ts file to see the actual implementation
cat ./validator-cli/src/utils/arbToEthState.ts | grep -A 15 -B 5 "maxTimeVariation"
# Let's also check if we can find the definition of the SequencerInbox interface
grep -r "interface SequencerInbox" --include="*.ts" .
Length of output: 1993
🏁 Script executed:
#!/bin/bash
# Let's find all files where maxTimeVariation is used
grep -r "maxTimeVariation" --include="*.ts" . --context=5
# Let's look at the SequencerInbox__factory to understand the interface
find . -name "SequencerInbox__factory.ts" -o -name "SequencerInbox.ts" | xargs cat 2>/dev/null | grep -A 10 -B 10 "maxTimeVariation"
# Let's try to find if there's a documentation or comment about maxTimeVariation
find node_modules -path "*arbitrum*" -name "*.ts" | xargs grep -l "maxTimeVariation" 2>/dev/null
Length of output: 2746
@mani99brar I apologize for the confusion. Looking at the codebase, I found inconsistent usage of maxTimeVariation():
In validator-cli/src/ArbToGnosis/watcherArbToGnosis.ts, it's used as:
const maxDelaySeconds = Number((await retryOperation(() => sequencer.maxTimeVariation(), 1000, 10))[1] as bigint);While in validator-cli/src/utils/arbToEthState.ts, it's used as:
const maxDelaySeconds = Number(await sequencer.maxTimeVariation());The first implementation suggests that maxTimeVariation() returns an array or tuple, where the desired value is at index [1]. This indicates that the function likely returns a structured value with multiple fields, not just a single value.
Without access to the Arbitrum SDK documentation in this environment, I can't verify the exact return type, but the indexed access in the other file strongly suggests that your implementation in arbToEthState.ts might need to be adjusted to use the specific field at index 1 rather than the entire return value.
validator-cli/src/utils/cli.ts
Outdated
| export function getBotPath({ cliCommand, defaultPath = BotPaths.BOTH }: BotPathParams): number { | ||
| const args = cliCommand.slice(2); | ||
| const pathFlag = args.find((arg) => arg.startsWith("--path=")); | ||
|
|
||
| const path = pathFlag ? pathFlag.split("=")[1] : null; | ||
|
|
||
| const pathMapping: Record<string, BotPaths> = { | ||
| claimer: BotPaths.CLAIMER, | ||
| challenger: BotPaths.CHALLENGER, | ||
| both: BotPaths.BOTH, | ||
| }; | ||
|
|
||
| if (path && !(path in pathMapping)) { | ||
| throw new InvalidBotPathError(); | ||
| } | ||
|
|
||
| return path ? pathMapping[path] : defaultPath; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Handle edge cases and improve type consistency.
The function needs improvements in:
- Return type should be BotPaths instead of number
- Edge cases handling:
- Empty path value
- Case-insensitive path matching
-export function getBotPath({ cliCommand, defaultPath = BotPaths.BOTH }: BotPathParams): number {
+export function getBotPath({ cliCommand, defaultPath = BotPaths.BOTH }: BotPathParams): BotPaths {
const args = cliCommand.slice(2);
const pathFlag = args.find((arg) => arg.startsWith("--path="));
- const path = pathFlag ? pathFlag.split("=")[1] : null;
+ const path = pathFlag ? pathFlag.split("=")[1].toLowerCase() : null;
+
+ if (pathFlag && !path) {
+ throw new InvalidBotPathError();
+ }
const pathMapping: Record<string, BotPaths> = {
claimer: BotPaths.CLAIMER,
challenger: BotPaths.CHALLENGER,
both: BotPaths.BOTH,
};
if (path && !(path in pathMapping)) {
throw new InvalidBotPathError();
}
return path ? pathMapping[path] : defaultPath;
}📝 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.
| export function getBotPath({ cliCommand, defaultPath = BotPaths.BOTH }: BotPathParams): number { | |
| const args = cliCommand.slice(2); | |
| const pathFlag = args.find((arg) => arg.startsWith("--path=")); | |
| const path = pathFlag ? pathFlag.split("=")[1] : null; | |
| const pathMapping: Record<string, BotPaths> = { | |
| claimer: BotPaths.CLAIMER, | |
| challenger: BotPaths.CHALLENGER, | |
| both: BotPaths.BOTH, | |
| }; | |
| if (path && !(path in pathMapping)) { | |
| throw new InvalidBotPathError(); | |
| } | |
| return path ? pathMapping[path] : defaultPath; | |
| } | |
| export function getBotPath({ cliCommand, defaultPath = BotPaths.BOTH }: BotPathParams): BotPaths { | |
| const args = cliCommand.slice(2); | |
| const pathFlag = args.find((arg) => arg.startsWith("--path=")); | |
| const path = pathFlag ? pathFlag.split("=")[1].toLowerCase() : null; | |
| if (pathFlag && !path) { | |
| throw new InvalidBotPathError(); | |
| } | |
| const pathMapping: Record<string, BotPaths> = { | |
| claimer: BotPaths.CLAIMER, | |
| challenger: BotPaths.CHALLENGER, | |
| both: BotPaths.BOTH, | |
| }; | |
| if (path && !(path in pathMapping)) { | |
| throw new InvalidBotPathError(); | |
| } | |
| return path ? pathMapping[path] : defaultPath; | |
| } |
| RPC_SEPOLIA= | ||
|
|
||
| # Testnet or Mainnet RPCs | ||
| RPC_ARB= | ||
| RPC_ARB=https://sepolia-rollup.arbitrum.io/rpc |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Remove duplicate RPC URL configuration.
The Arbitrum RPC URL is defined twice:
- Line 4:
RPC_ARB_SEPOLIA=https://sepolia-rollup.arbitrum.io/rpc - Line 9:
RPC_ARB=https://sepolia-rollup.arbitrum.io/rpc
This could lead to confusion. Consider removing one of them or clearly documenting the difference if both are needed.
Also applies to: 4-4
There was a problem hiding this comment.
Will use a single RPC var for both networks on a chain 👍
There was a problem hiding this comment.
@mani99brar Great! Using a single RPC variable per chain is a good simplification that will make the configuration more maintainable. This is a change from your previous approach where you had separate variables for devnet and testnet/mainnet usage, but it should make the codebase cleaner and reduce potential confusion. Let me know if you need any help with implementing this change.
✏️ Learnings added
Learnt from: mani99brar
PR: kleros/vea#396
File: validator-cli/.env.dist:15-15
Timestamp: 2025-03-31T09:03:04.957Z
Learning: User (mani99brar) has decided to simplify the approach by using a single RPC variable per chain instead of maintaining separate variables for devnet and testnet/mainnet environments, even when they point to the same URL.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
🧠 Learnings used
Learnt from: madhurMongia
PR: kleros/vea#359
File: validator-cli/.env.dist:3-4
Timestamp: 2025-03-28T01:51:52.423Z
Learning: In 'validator-cli/.env.dist', the `RPC_GNOSIS` variable may be intentionally set to `https://rpc.chiadochain.net` as an example.
Learnt from: mani99brar
PR: kleros/vea#388
File: validator-cli/.env.dist:9-11
Timestamp: 2025-03-28T01:51:48.908Z
Learning: User (mani99brar) intentionally keeps the same RPC URLs under different environment variables to distinguish between devnet and testnet/mainnet usage, and this is by design rather than an oversight.
|
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
validator-cli/src/watcher.ts (3)
28-29:⚠️ Potential issueAdd validation for 'VEAOUTBOX_CHAIN_ID' environment variable.
The code assumes that
process.env.VEAOUTBOX_CHAIN_IDis defined and can be converted to a valid number. If it's missing or invalid,chainIdwill beNaN, which may lead to unexpected behavior.Apply this diff to add validation:
+if (!process.env.VEAOUTBOX_CHAIN_ID || isNaN(Number(process.env.VEAOUTBOX_CHAIN_ID))) { + throw new Error("Invalid or missing VEAOUTBOX_CHAIN_ID environment variable"); +} const chainId = Number(process.env.VEAOUTBOX_CHAIN_ID); emitter.emit(BotEvents.STARTED, chainId, path);
31-32:⚠️ Potential issueEnsure 'PRIVATE_KEY' environment variable is defined.
The
process.env.PRIVATE_KEYis used without validation, which could cause runtime errors if missing.Apply this diff to add validation:
+if (!process.env.PRIVATE_KEY) { + throw new Error("Missing PRIVATE_KEY environment variable"); +} const veaInbox = getVeaInbox(veaBridge.inboxAddress, process.env.PRIVATE_KEY, veaBridge.inboxRPC, chainId); const veaOutbox = getVeaOutbox(veaBridge.outboxAddress, process.env.PRIVATE_KEY, veaBridge.outboxRPC, chainId);
91-94:⚠️ Potential issueHandle unhandled Promise rejections when calling 'watch'.
The
watchfunction is called without proper error handling.Apply this diff to handle the Promise properly:
if (require.main === module) { const shutDownSignal = new ShutdownSignal(false); - watch(shutDownSignal); + (async () => { + try { + await watch(shutDownSignal); + } catch (error) { + console.error("An error occurred during watch execution:", error); + process.exit(1); + } + })(); }
🧹 Nitpick comments (10)
validator-cli/src/watcher.ts (5)
13-19: Enhance JSDoc documentation.Consider adding more details to the documentation:
- Return type
- Possible errors/exceptions
- Example usage
/** * @file This file contains the logic for watching a bridge and validating/resolving for claims. * * @param shutDownSignal - The signal to shut down the watcher * @param emitter - The emitter to emit events + * @returns {Promise<void>} + * @throws {Error} When required environment variables are missing + * @example + * const signal = new ShutdownSignal(); + * await watch(signal); */
40-40: Use a more specific type for transaction handlers.The type for
transactionHandlerscould be more strictly defined using a specific type instead of relying onInstanceType.-const transactionHandlers: { [epoch: number]: InstanceType<typeof TransactionHandler> } = {}; +type TransactionHandlerInstance = InstanceType<typeof TransactionHandler>; +const transactionHandlers: Record<number, TransactionHandlerInstance> = {};
71-75: Refactor array manipulation for better maintainability.Modifying an array while iterating over it using splice can be error-prone and harder to maintain.
Consider using filter to create a new array instead:
- } else if (epoch != latestEpoch) { - delete transactionHandlers[epoch]; - epochRange.splice(i, 1); - continue; - } + } else if (epoch !== latestEpoch) { + delete transactionHandlers[epoch]; + epochRange = epochRange.filter(e => e !== epoch); + i--; + }
85-85: Extract wait duration to a configurable constant.The wait duration is hardcoded, making it less flexible for different environments.
+const POLLING_INTERVAL_MS = 10 * 1000; // 10 seconds + // In the watch function - await wait(1000 * 10); + await wait(POLLING_INTERVAL_MS);
89-89: Add TypeScript types to the wait function.The wait function lacks type information for its parameter and return type.
-const wait = (ms) => new Promise((r) => setTimeout(r, ms)); +const wait = (ms: number): Promise<void> => new Promise((resolve) => setTimeout(resolve, ms));validator-cli/src/utils/epochHandler.ts (3)
22-22: Consider making the cold start backlog configurable.The cold start backlog is currently hardcoded to 7 days. Consider making this configurable through the bridge config or environment variables for better flexibility.
- const coldStartBacklog = 7 * 24 * 60 * 60; // when starting the watcher, specify an extra backlog to check + const DAYS_IN_WEEK = 7; + const SECONDS_IN_DAY = 24 * 60 * 60; + const coldStartBacklog = (process.env.COLD_START_BACKLOG_DAYS || DAYS_IN_WEEK) * SECONDS_IN_DAY;
28-28: Extract magic number as a named constant.The "-2" adjustment factor should be extracted as a named constant with a descriptive name to improve code maintainability and document its purpose.
+ const SAFETY_MARGIN_EPOCHS = 2; const veaEpochOutboxWatchLowerBound = - Math.floor((currentTimestamp - L2SyncPeriod - coldStartBacklog) / epochPeriod) - 2; + Math.floor((currentTimestamp - L2SyncPeriod - coldStartBacklog) / epochPeriod) - SAFETY_MARGIN_EPOCHS;
61-61: Share safety margin constant with setEpochRange.The "-2" adjustment factor appears in both functions. Consider sharing the
SAFETY_MARGIN_EPOCHSconstant between them.+const SAFETY_MARGIN_EPOCHS = 2; + const getLatestChallengeableEpoch = ( chainId: number, now: number = Date.now(), fetchBridgeConfig: typeof getBridgeConfig = getBridgeConfig ): number => { const { epochPeriod } = fetchBridgeConfig(chainId); - return Math.floor(now / 1000 / epochPeriod) - 2; + return Math.floor(now / 1000 / epochPeriod) - SAFETY_MARGIN_EPOCHS; };validator-cli/src/ArbToEth/transactionHandler.ts (2)
301-301: Document the hardcoded maxPriorityFeePerGasMEV valueAdd a comment explaining why 6667 gwei was chosen as the MEV-aware priority fee.
- let maxPriorityFeePerGasMEV = BigInt(6667000000000); // 6667 gwei + // Set MEV-aware priority fee to 6667 gwei based on historical MEV-boost auction data + // This value represents a balance between transaction inclusion probability and cost + let maxPriorityFeePerGasMEV = BigInt(6667000000000); // 6667 gwei
52-52: Fix comment inconsistency in MAX_PENDING_TIMEThe comment states "3 minutes" but the actual value is 5 minutes (300000 ms).
-export const MAX_PENDING_TIME = 5 * 60 * 1000; // 3 minutes +export const MAX_PENDING_TIME = 5 * 60 * 1000; // 5 minutes
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
validator-cli/.env.dist(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(1 hunks)validator-cli/src/ArbToEth/transactionHandler.ts(1 hunks)validator-cli/src/ArbToEth/validator.test.ts(1 hunks)validator-cli/src/ArbToEth/validator.ts(1 hunks)validator-cli/src/utils/botEvents.ts(1 hunks)validator-cli/src/utils/epochHandler.ts(1 hunks)validator-cli/src/utils/logger.ts(1 hunks)validator-cli/src/watcher.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (6)
- validator-cli/src/utils/botEvents.ts
- validator-cli/src/ArbToEth/claimer.test.ts
- validator-cli/src/utils/logger.ts
- validator-cli/src/ArbToEth/validator.test.ts
- validator-cli/.env.dist
- validator-cli/src/ArbToEth/transactionHandler.test.ts
🧰 Additional context used
📓 Learnings (2)
validator-cli/src/ArbToEth/claimer.ts (1)
Learnt from: mani99brar
PR: kleros/vea#388
File: validator-cli/src/ArbToEth/validator.ts:16-17
Timestamp: 2025-01-27T04:12:05.847Z
Learning: In the validator-cli, `any` type is intentionally used for `veaInbox` and `veaOutbox` parameters to maintain chain agnosticism. This is because different chains have their own specific contract types (e.g., VeaInboxArbToEth, VeaOutboxArbToEth, VeaOutboxArbToGnosis) and using union types would make the code more complex and less maintainable.
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.
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: dependency-review
- GitHub Check: Analyze (javascript)
🔇 Additional comments (4)
validator-cli/src/ArbToEth/claimer.ts (2)
37-37: LGTM! Proper epoch calculationThe epoch calculation correctly uses Math.floor to ensure integer epoch numbers.
53-53:⚠️ Potential issueUpdate ethers.ZeroHash to ethers.constants.HashZero
The property
ethers.ZeroHashis not available in ethers.js v5. Useethers.constants.HashZeroinstead.-const newMessagesToBridge: boolean = savedSnapshot != outboxStateRoot && savedSnapshot != ethers.ZeroHash; +const newMessagesToBridge: boolean = savedSnapshot != outboxStateRoot && savedSnapshot != ethers.constants.HashZero;Likely invalid or redundant comment.
validator-cli/src/ArbToEth/validator.ts (1)
78-78: LGTM! Proper use of ethers.ZeroAddressThe code correctly uses
ethers.ZeroAddressto check for an unset challenger address.validator-cli/src/ArbToEth/transactionHandler.ts (1)
108-108:⚠️ Potential issueAdd error handling for unknown contract types
The contract type check should handle unknown values to prevent undefined behavior.
-const provider = contract === ContractType.INBOX ? this.veaInboxProvider : this.veaOutboxProvider; +let provider: JsonRpcProvider; +if (contract === ContractType.INBOX) { + provider = this.veaInboxProvider; +} else if (contract === ContractType.OUTBOX) { + provider = this.veaOutboxProvider; +} else { + throw new Error(`Unknown contract type: ${contract}`); +}Likely invalid or redundant comment.
| 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 - 100); | ||
| const secPerBlock = (latestBlock.timestamp - baseBlock.timestamp) / (latestBlock.number - baseBlock.number); | ||
| const blockFallBack = Math.floor((latestBlock.timestamp - epochTimestamp) / secPerBlock); | ||
| return latestBlock.number - blockFallBack; | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling and input validation.
The function needs additional error handling and validation:
- Provider calls could fail
- Block sampling window is fixed
- No validation of input parameters
- No handling of edge cases
Consider this improved implementation:
+const DEFAULT_BLOCK_SAMPLE_SIZE = 100;
+
const getBlockFromEpoch = async (
epoch: number,
epochPeriod: number,
provider: JsonRpcProvider,
+ sampleSize: number = DEFAULT_BLOCK_SAMPLE_SIZE
): Promise<number> => {
+ if (epoch < 0) throw new Error("Epoch must be non-negative");
+ if (epochPeriod <= 0) throw new Error("Epoch period must be positive");
+
const epochTimestamp = epoch * epochPeriod;
- const latestBlock = await provider.getBlock("latest");
- const baseBlock = await provider.getBlock(latestBlock.number - 100);
+
+ try {
+ const latestBlock = await provider.getBlock("latest");
+ if (!latestBlock) throw new Error("Failed to fetch latest block");
+
+ const baseBlock = await provider.getBlock(latestBlock.number - sampleSize);
+ if (!baseBlock) throw new Error("Failed to fetch base block");
+
+ if (baseBlock.number >= latestBlock.number)
+ throw new Error("Invalid block range");
+
const secPerBlock = (latestBlock.timestamp - baseBlock.timestamp) / (latestBlock.number - baseBlock.number);
const blockFallBack = Math.floor((latestBlock.timestamp - epochTimestamp) / secPerBlock);
- return latestBlock.number - blockFallBack;
+
+ const targetBlock = latestBlock.number - blockFallBack;
+ return Math.max(0, targetBlock); // Ensure non-negative block number
+ } catch (error) {
+ throw new Error(`Failed to calculate block from epoch: ${error.message}`);
+ }
};📝 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 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 - 100); | |
| const secPerBlock = (latestBlock.timestamp - baseBlock.timestamp) / (latestBlock.number - baseBlock.number); | |
| const blockFallBack = Math.floor((latestBlock.timestamp - epochTimestamp) / secPerBlock); | |
| return latestBlock.number - blockFallBack; | |
| }; | |
| const DEFAULT_BLOCK_SAMPLE_SIZE = 100; | |
| const getBlockFromEpoch = async ( | |
| epoch: number, | |
| epochPeriod: number, | |
| provider: JsonRpcProvider, | |
| sampleSize: number = DEFAULT_BLOCK_SAMPLE_SIZE | |
| ): Promise<number> => { | |
| if (epoch < 0) throw new Error("Epoch must be non-negative"); | |
| if (epochPeriod <= 0) throw new Error("Epoch period must be positive"); | |
| const epochTimestamp = epoch * epochPeriod; | |
| try { | |
| const latestBlock = await provider.getBlock("latest"); | |
| if (!latestBlock) throw new Error("Failed to fetch latest block"); | |
| const baseBlock = await provider.getBlock(latestBlock.number - sampleSize); | |
| if (!baseBlock) throw new Error("Failed to fetch base block"); | |
| if (baseBlock.number >= latestBlock.number) | |
| throw new Error("Invalid block range"); | |
| const secPerBlock = | |
| (latestBlock.timestamp - baseBlock.timestamp) / | |
| (latestBlock.number - baseBlock.number); | |
| const blockFallBack = Math.floor((latestBlock.timestamp - epochTimestamp) / secPerBlock); | |
| const targetBlock = latestBlock.number - blockFallBack; | |
| return Math.max(0, targetBlock); // Ensure non-negative block number | |
| } catch (error) { | |
| throw new Error(`Failed to calculate block from epoch: ${error.message}`); | |
| } | |
| }; |
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (7)
validator-cli/src/watcher.ts (2)
122-125:⚠️ Potential issueHandle unhandled promise rejection in main entry point.
This concern was flagged in a previous review. Wrapping the
watch(shutDownSignal)call in an async IIFE and awaiting it ensures rejections are caught. Not doing so could lead to unhandled promise rejections that terminate the process abruptly.if (require.main === module) { const shutDownSignal = new ShutdownSignal(false); - watch(shutDownSignal); + (async () => { + try { + await watch(shutDownSignal); + } catch (error) { + console.error("An error occurred during watch execution:", error); + process.exit(1); + } + })(); }
38-51: 🛠️ Refactor suggestionValidate the
PRIVATE_KEYenvironment variable before use.The code references
process.env.PRIVATE_KEYwithout a preceding validation step. IfPRIVATE_KEYis unset or invalid, runtime errors can occur. Consider adding a check to ensurePRIVATE_KEYis defined and properly formatted before using it.+if (!process.env.PRIVATE_KEY) { + throw new Error("Missing or invalid PRIVATE_KEY environment variable."); +} const veaInbox = getVeaInbox( routeConfig[network].veaInbox.address, ...validator-cli/src/utils/logger.ts (1)
21-132: 🛠️ Refactor suggestionAdd error handling to event listeners.
The event listeners lack error handling which could lead to silent failures or unhandled promise rejections.
Consider wrapping the handlers with a try-catch block:
export const configurableInitialize = (emitter: EventEmitter) => { const wrapHandler = <T extends (...args: any[]) => void>(handler: T) => { return (...args: Parameters<T>) => { try { handler(...args); } catch (error) { console.error('Error in event handler:', error); // Consider emitting an error event that can be handled by the application emitter.emit(BotEvents.ERROR, error); } }; }; // Then use wrapHandler for each event listener emitter.on(BotEvents.STARTED, wrapHandler((path: BotPaths, networks: Network[]) => { // ... rest of the handler })); // And so on for other event handlers }validator-cli/src/utils/graphQueries.ts (2)
17-39: 🛠️ Refactor suggestionImprove error handling in getClaimForEpoch function.
The current error handling could be enhanced:
- Using a more specific error type
- Implementing proper error logging
- Adding validation for the subgraph URL
const getClaimForEpoch = async (epoch: number, outbox: string): Promise<ClaimData | undefined> => { try { const subgraph = process.env.VEAOUTBOX_SUBGRAPH; + if (!subgraph) { + throw new Error('VEAOUTBOX_SUBGRAPH environment variable is not defined'); + } + if (!subgraph.startsWith('http')) { + throw new Error('VEAOUTBOX_SUBGRAPH must be a valid URL'); + } const result = await request( `${subgraph}`, `{ claims(where: {epoch: ${epoch}, outbox: "${outbox}"}) { id bridger stateroot timestamp txHash challenged } }` ); return result[`claims`][0]; - } catch (e) { - console.log(e); + } catch (e: unknown) { + console.error('Failed to fetch claim for epoch:', epoch, e instanceof Error ? e.message : e); return undefined; } };
45-63:⚠️ Potential issueAdd error handling to getLastClaimedEpoch function.
The
getLastClaimedEpochfunction is missing error handling, which could lead to uncaught exceptions when the subgraph URL is missing or the request fails.const getLastClaimedEpoch = async (outbox: string): Promise<ClaimData> => { - const subgraph = process.env.VEAOUTBOX_SUBGRAPH; - - const result = await request( - `${subgraph}`, - `{ - claims(first:1, orderBy:timestamp, orderDirection:desc, where: {outbox: "${outbox}"}) { - id - bridger - stateroot - timestamp - challenged - txHash - } - - }` - ); - return result[`claims`][0]; + try { + const subgraph = process.env.VEAOUTBOX_SUBGRAPH; + if (!subgraph) { + throw new Error('VEAOUTBOX_SUBGRAPH environment variable is not defined'); + } + if (!subgraph.startsWith('http')) { + throw new Error('VEAOUTBOX_SUBGRAPH must be a valid URL'); + } + + const result = await request( + `${subgraph}`, + `{ + claims(first:1, orderBy:timestamp, orderDirection:desc, where: {outbox: "${outbox}"}) { + id + bridger + stateroot + timestamp + challenged + txHash + } + + }` + ); + + return result[`claims`][0]; + } catch (e: unknown) { + console.error('Failed to fetch last claimed epoch:', e instanceof Error ? e.message : e); + throw e; + } };validator-cli/src/utils/ethers.ts (2)
57-62: 🛠️ Refactor suggestion
getVeaRoutermissing default
Currently, onlychainId === 10200is handled. If other chain IDs are used, this will yieldundefined. Add a default or error-based fallback to improve reliability.function getVeaRouter(...) { switch (chainId) { case 10200: return ... + default: + throw new Error(`Unsupported chainId: ${chainId} in getVeaRouter.`); } }
28-34: 🛠️ Refactor suggestionMissing default case in
getVeaInbox
If thechainIdis not11155111or10200, the function silently returnsundefined. Consider adding a fallback to throw an error for unsupported chain IDs.switch (chainId) { case 11155111: return ... case 10200: return ... + default: + throw new Error(`Unsupported chainId: ${chainId} in getVeaInbox.`); }
🧹 Nitpick comments (29)
validator-cli/src/watcher.ts (2)
54-59: Useletorconstinstead ofvarforepochRange.Using
varcan introduce unintentional scoping behavior. Preferletorconstto limit scope and improve code clarity.- var epochRange = setEpochRange({ + let epochRange = setEpochRange({
116-116: Consider making the polling interval configurable.The code sleeps for 10 seconds between iterations. If the polling interval needs to be adjusted or optimized for different environments, expose it as a variable or configuration parameter.
validator-cli/src/utils/epochHandler.test.ts (2)
20-33: Remove duplication in test parameters.There's unnecessary duplication between the
mockParamsobject (lines 20-26) and the parameters passed tosetEpochRange(lines 27-33). This makes the test harder to maintain since changes would need to be made in two places.- const result = setEpochRange({ - chainId: 1, - currentTimestamp, - epochPeriod: mockedEpochPeriod, - now, - fetchBridgeConfig: mockedFetchBridgeConfig as any, - }); + const result = setEpochRange(mockParams);
39-45: Improve readability of epoch calculation test.The expected value calculation is not immediately clear. Consider extracting the calculation to a variable with a descriptive name to make the test more maintainable and self-documenting.
it("should return the correct epoch number", () => { const now = 1626325200000; const result = getLatestChallengeableEpoch(600, now); - expect(result).toEqual(now / (600 * 1000) - 2); + const expectedEpoch = Math.floor(now / (600 * 1000)) - 2; + expect(result).toEqual(expectedEpoch); });validator-cli/src/utils/logger.ts (4)
107-107: Add type annotation for the epoch parameter.The
epochparameter in the EXECUTING_SNAPSHOT event handler is missing a type annotation, unlike other similar event handlers.- emitter.on(BotEvents.EXECUTING_SNAPSHOT, (epoch) => { + emitter.on(BotEvents.EXECUTING_SNAPSHOT, (epoch: number) => { console.log(`Executing snapshot to resolve dispute for epoch ${epoch}`); });
55-60: Standardize parameter naming across similar events.The transaction parameter is named inconsistently between related event handlers. Some use
transactionwhile others usehash.- emitter.on(BotEvents.TXN_MADE, (transaction: string, epoch: number, state: string) => { - console.log(`${state} transaction for ${epoch} made with hash: ${transaction}`); + emitter.on(BotEvents.TXN_MADE, (txHash: string, epoch: number, state: string) => { + console.log(`${state} transaction for ${epoch} made with hash: ${txHash}`); }); - emitter.on(BotEvents.TXN_PENDING, (transaction: string) => { - console.log(`Transaction is still pending with hash: ${transaction}`); + emitter.on(BotEvents.TXN_PENDING, (txHash: string) => { + console.log(`Transaction is still pending with hash: ${txHash}`); });
115-117: Add missing epoch parameter to log message.The withdrawing challenge deposit log message says "for epoch" but doesn't include the epoch parameter in the output.
- emitter.on(BotEvents.WITHDRAWING_CHALLENGE_DEPOSIT, () => { - console.log(`Withdrawing challenge deposit for epoch`); + emitter.on(BotEvents.WITHDRAWING_CHALLENGE_DEPOSIT, (epoch: number) => { + console.log(`Withdrawing challenge deposit for epoch ${epoch}`); });
21-132: Consider using a structured logging library.Using plain
console.logfor all logging is not ideal for production applications. A structured logging library would provide better log management capabilities.import winston from 'winston'; const logger = winston.createLogger({ level: 'info', format: winston.format.json(), defaultMeta: { service: 'validator-bot' }, transports: [ new winston.transports.Console({ format: winston.format.simple(), }), ], }); export const configurableInitialize = (emitter: EventEmitter) => { emitter.on(BotEvents.STARTED, (path: BotPaths, networks: Network[]) => { let pathString = "claimer and challenger"; if (path === BotPaths.CLAIMER) { pathString = "claimer"; } else if (path === BotPaths.CHALLENGER) { pathString = "challenger"; } logger.info(`Bot started for ${pathString} on ${networks}`, { path, networks, event: BotEvents.STARTED, }); }); // And so on for other event handlers };validator-cli/src/utils/graphQueries.ts (1)
21-34: Improve GraphQL query formatting for better readability.The GraphQL queries are not properly indented, making them hard to read and maintain.
const result = await request( `${subgraph}`, - `{ - claims(where: {epoch: ${epoch}, outbox: "${outbox}"}) { - id - bridger - stateroot - timestamp - txHash - challenged - } - }` + ` + { + claims(where: {epoch: ${epoch}, outbox: "${outbox}"}) { + id + bridger + stateroot + timestamp + txHash + challenged + } + } + ` ); // And similarly for the second query: const result = await request( `${subgraph}`, - `{ - claims(first:1, orderBy:timestamp, orderDirection:desc, where: {outbox: "${outbox}"}) { - id - bridger - stateroot - timestamp - challenged - txHash - } - - }` + ` + { + claims(first:1, orderBy:timestamp, orderDirection:desc, where: {outbox: "${outbox}"}) { + id + bridger + stateroot + timestamp + challenged + txHash + } + } + ` );Also applies to: 48-62
validator-cli/src/utils/errors.ts (4)
3-9: Enhance Error implementation to preserve stack traces.The current error implementation doesn't pass the message to the parent constructor and doesn't preserve stack traces.
class ClaimNotFoundError extends Error { constructor(epoch: number) { - super(); + super(`No claim was found for ${epoch}`); this.name = "ClaimNotFoundError"; - this.message = `No claim was found for ${epoch}`; + Error.captureStackTrace(this, this.constructor); } }
11-17: Fix error name inconsistency.The error name in the message ("NoClaimSetError") doesn't match the class name ("ClaimNotSetError").
class ClaimNotSetError extends Error { constructor() { - super(); + super("Claim is not set"); this.name = "ClaimNotSetError"; - this.message = "Claim is not set"; + Error.captureStackTrace(this, this.constructor); } }
19-25: Apply consistent error implementation pattern across all error classes.All error classes should follow the same pattern for initializing the error with proper stack traces.
Here's an example for one of the classes:
class TransactionHandlerNotDefinedError extends Error { constructor() { - super(); + super("TransactionHandler is not defined"); this.name = "TransactionHandlerNotDefinedError"; - this.message = "TransactionHandler is not defined"; + Error.captureStackTrace(this, this.constructor); } }Apply the same pattern to all remaining error classes.
Also applies to: 27-33, 35-41, 43-49
31-31: Fix potential string interpolation issue in InvalidBotPathError.The message uses string interpolation to join the available paths with a comma and space, but the closing parenthesis is inside the interpolation which could lead to formatting issues.
- this.message = `Invalid path provided, Use one of: ${Object.keys(BotPaths).join("), ")}`; + this.message = `Invalid path provided, Use one of: ${Object.keys(BotPaths).join(", ")}`;validator-cli/src/utils/botConfig.ts (4)
1-3: Use ES module import syntax for dotenv.The code uses
require()for importing dotenv while using ES module imports for other dependencies. Consider using consistent import syntax.import { InvalidBotPathError, DevnetOwnerNotSetError, InvalidNetworkError } from "./errors"; import { Network } from "../consts/bridgeRoutes"; -require("dotenv").config(); +import dotenv from "dotenv"; + +dotenv.config();
24-25: Add validation for path flag format.The code splits the path flag on
=without first checking if the split results in at least 2 elements, which could lead to accessing undefined values.- const pathFlag = args.find((arg) => arg.startsWith("--path=")); + const pathFlag = args.find((arg) => arg.startsWith("--path=")); - const path = pathFlag ? pathFlag.split("=")[1] : null; + const path = pathFlag && pathFlag.includes("=") ? pathFlag.split("=")[1] : null;
50-67: Add validation for chainIds.The function doesn't validate that
chainIdsis not empty, which could result in returning an empty array without any error indication.export function getNetworkConfig(): NetworkConfig[] { const chainIds = process.env.VEAOUTBOX_CHAINS ? process.env.VEAOUTBOX_CHAINS.split(",") : []; + if (chainIds.length === 0) { + throw new Error("No chain IDs specified in VEAOUTBOX_CHAINS environment variable"); + } const devnetOwner = process.env.DEVNET_OWNER; const rawNetwork = process.env.NETWORKS ? process.env.NETWORKS.split(",") : []; + if (rawNetwork.length === 0) { + throw new Error("No networks specified in NETWORKS environment variable"); + } const networks = validateNetworks(rawNetwork); if (networks.includes(Network.DEVNET) && !devnetOwner) { throw new DevnetOwnerNotSetError(); } const networkConfig: NetworkConfig[] = []; for (const chainId of chainIds) { networkConfig.push({ chainId: Number(chainId), networks, devnetOwner, }); } return networkConfig; }
76-76: Avoid type assertion usingunknown.The code uses
as unknown as Network[]for type assertion, which bypasses TypeScript's type checking. Consider using a more type-safe approach.- return networks as unknown as Network[]; + return networks.map(network => network as Network);validator-cli/src/ArbToEth/validator.ts (1)
30-108: Evaluate nested condition blocks for clarity & ensure error handling.Overall, the control flow is correct:
• It checks for a missing claim, finality, and snapshot mismatches.
• Correctly initializes a newtransactionHandlerif not provided, or reuses the current one.
• Diligently branches on claim validity and handles challenge, snapshot sending, resolution, and deposit withdrawal.However, the nested
if ... elseblocks (lines 80–106) can make readability challenging. Consider simplifying them using guard clauses or extracting logic into smaller helper functions to reduce nesting. Also, verify proper error-handling for transaction calls (e.g.,challengeClaim(),sendSnapshot(), etc.) to prevent unhandled promise rejections if failures occur.validator-cli/src/ArbToEth/transactionHandlerDevnet.ts (2)
13-31: Keep configuration flexible for different devnet transaction needs.Introducing
devnetAdvanceStateTxnin the extendedtransactionsobject is fine for tracking devnet-specific transactions. However, ensure that referencing a singleCHAIN_ID(line 17) always aligns with your environment. A more flexible approach might either retrieve it from parameters or the existingbridgeRouteslogic, allowing you to handle multiple devnet configurations seamlessly.
50-70: Confirm error handling when advancing devnet state.The
devnetAdvanceStatemethod (lines 50–70) emits events and checks transaction status appropriately. As an extra safety measure, consider handling the scenario wheregetBridgeConfig(CHAIN_ID)returnsundefined(for example, if the chain ID is unrecognized). While this might be unlikely in your setup, adding a safeguard would avoid runtime errors.Do you want me to open a separate issue with a proposed fix for handling undefined configurations?
validator-cli/src/consts/bridgeRoutes.ts (1)
14-23: Consolidate typed references for bridging.The
Bridgeinterface (lines 14–23) effectively captures bridging parameters. If you prefer a stricter approach, consider defining known types instead ofanyinRouteConfigsto gain compile-time checks on the JSON artifacts (e.g.,typeof veaInboxArbToEthDevnet).validator-cli/src/ArbToEth/claimer.ts (1)
91-93: Consider adding error handling for the claim transaction.
Here, you callmakeClaim(...)and then immediately return. In the event of a failed or reverted transaction, it’s unclear whether the caller can handle the error. Consider adding error handling or a try/catch block to manage transaction failures gracefully.validator-cli/src/utils/epochHandler.ts (2)
34-37: Double-check the L2 sync period margins.
L2SyncPeriodis set tosequencerDelayLimit + epochPeriod. Depending on the chain’s finalization times, occasional overhead or additional buffer might be required. Consider introducing a small safety margin to reduce race conditions.
45-48: Watch out for potential performance issues when creating large arrays.
veaEpochOutboxRangecould be large, causing a significant performance overhead in.filland.map. If the range can grow large (thousands or more), consider streaming or chunking the iteration logic.validator-cli/src/ArbToEth/transactionHandler.ts (3)
64-64: Correct the comment to match the actual duration.
The code calculates 5 minutes (5 * 60 * 1000) but the comment says 3 minutes. Please update the comment or the calculation for clarity and consistency.-export const MAX_PENDING_TIME = 5 * 60 * 1000; // 3 minutes +export const MAX_PENDING_TIME = 5 * 60 * 1000; // 5 minutes
66-66: Avoid hardcodingCHAIN_ID.
You are usingCHAIN_ID = 11155111directly. If you intend to support multiple networks, consider passingchainIdas a parameter or retrieving it from configuration.
314-320: Review advanced gas logic for corner cases.
SettingmaxFeePerGasProfitableandmaxPriorityFeePerGasMEVis clever, but could become brittle in real-world conditions where gas spikes occur. Consider providing fallbacks or dynamic updating to avoid out-of-gas or underpriced transactions.validator-cli/src/utils/ethers.ts (2)
76-81:getClaimValidatormissing fallback
OnlychainId === 11155111is handled, returningchallengeAndResolveClaimArbToEth. A default or additional cases for other networks might help avoid silent failures.switch (chainId) { case 11155111: return challengeAndResolveClaimArbToEth; + default: + throw new Error(`Unsupported chainId: ${chainId} in getClaimValidator.`); }
82-92:getClaimermissing fallback
Same pattern asgetClaimValidator. Consider adding a default in both thechainIdandnetworkswitches to handle unexpected inputs explicitly.switch (chainId) { case 11155111: switch (network) { case Network.DEVNET: case Network.TESTNET: return checkAndClaim; + default: + throw new Error(`Unsupported network: ${network} for chainId 11155111 in getClaimer.`); } + default: + throw new Error(`Unsupported chainId: ${chainId} in getClaimer.`); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro (Legacy)
📒 Files selected for processing (19)
validator-cli/.env.dist(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(1 hunks)validator-cli/src/ArbToEth/transactionHandler.ts(1 hunks)validator-cli/src/ArbToEth/transactionHandlerDevnet.ts(1 hunks)validator-cli/src/ArbToEth/validator.ts(1 hunks)validator-cli/src/consts/bridgeRoutes.ts(1 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/epochHandler.test.ts(1 hunks)validator-cli/src/utils/epochHandler.ts(1 hunks)validator-cli/src/utils/errors.ts(1 hunks)validator-cli/src/utils/ethers.ts(1 hunks)validator-cli/src/utils/graphQueries.ts(1 hunks)validator-cli/src/utils/logger.ts(1 hunks)validator-cli/src/watcher.ts(1 hunks)validator-cli/tsconfig.json(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- validator-cli/src/ArbToEth/claimer.test.ts
- validator-cli/.env.dist
🧰 Additional context used
🧠 Learnings (5)
validator-cli/src/utils/epochHandler.test.ts (1)
Learnt from: mani99brar
PR: kleros/vea#370
File: bridger-cli/src/utils/epochHandler.ts:13-24
Timestamp: 2025-03-28T01:51:43.095Z
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/graphQueries.ts (1)
Learnt from: mani99brar
PR: kleros/vea#370
File: bridger-cli/src/utils/graphQueries.ts:12-34
Timestamp: 2025-03-28T01:51:35.847Z
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/src/ArbToEth/claimer.ts (1)
Learnt from: mani99brar
PR: kleros/vea#388
File: validator-cli/src/ArbToEth/validator.ts:16-17
Timestamp: 2025-03-28T01:51:35.847Z
Learning: In the validator-cli, `any` type is intentionally used for `veaInbox` and `veaOutbox` parameters to maintain chain agnosticism. This is because different chains have their own specific contract types (e.g., VeaInboxArbToEth, VeaOutboxArbToEth, VeaOutboxArbToGnosis) and using union types would make the code more complex and less maintainable.
validator-cli/src/utils/epochHandler.ts (3)
Learnt from: mani99brar
PR: kleros/vea#370
File: bridger-cli/src/utils/epochHandler.ts:13-24
Timestamp: 2025-03-28T01:51:43.095Z
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: 2025-03-28T01:51:30.640Z
Learning: In the file `bridger-cli/src/utils/epochHandler.ts`, the calculation within the `checkForNewEpoch` function is correct as implemented and has been tested.
Learnt from: mani99brar
PR: kleros/vea#370
File: bridger-cli/src/utils/epochHandler.ts:38-50
Timestamp: 2025-03-28T01:51:48.908Z
Learning: The `getBlockNumberFromEpoch` function in `bridger-cli/src/utils/epochHandler.ts` is currently unused and will be addressed in a future issue.
validator-cli/src/consts/bridgeRoutes.ts (1)
Learnt from: mani99brar
PR: kleros/vea#370
File: bridger-cli/src/consts/bridgeRoutes.ts:28-30
Timestamp: 2025-03-28T01:51:48.908Z
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.
🧬 Code Definitions (11)
validator-cli/src/utils/botConfig.test.ts (2)
validator-cli/src/utils/botConfig.ts (1)
getBotPath(21-38)validator-cli/src/utils/errors.ts (1)
InvalidBotPathError(55-55)
validator-cli/src/utils/epochHandler.test.ts (1)
validator-cli/src/utils/epochHandler.ts (3)
EpochRangeParams(76-76)setEpochRange(76-76)getLatestChallengeableEpoch(76-76)
validator-cli/src/ArbToEth/validator.ts (3)
validator-cli/src/ArbToEth/transactionHandler.ts (1)
ArbToEthTransactionHandler(68-409)validator-cli/src/utils/claim.ts (2)
getClaim(144-144)getClaimResolveState(144-144)validator-cli/src/utils/arbToEthState.ts (1)
getBlocksAndCheckFinality(243-243)
validator-cli/src/ArbToEth/transactionHandlerDevnet.ts (2)
validator-cli/src/ArbToEth/transactionHandler.ts (4)
Transactions(39-49)Transaction(34-37)ArbToEthTransactionHandler(68-409)TransactionHandlerConstructor(23-32)validator-cli/src/consts/bridgeRoutes.ts (1)
getBridgeConfig(90-90)
validator-cli/src/ArbToEth/claimer.ts (4)
validator-cli/src/ArbToEth/transactionHandler.ts (1)
ArbToEthTransactionHandler(68-409)validator-cli/src/utils/claim.ts (1)
ClaimHonestState(144-144)validator-cli/src/utils/graphQueries.ts (1)
getLastClaimedEpoch(65-65)validator-cli/src/utils/ethers.ts (1)
getTransactionHandler(116-116)
validator-cli/src/watcher.ts (7)
validator-cli/src/utils/shutdown.ts (1)
ShutdownSignal(4-18)validator-cli/src/utils/botConfig.ts (2)
getBotPath(21-38)getNetworkConfig(50-67)validator-cli/src/consts/bridgeRoutes.ts (1)
getBridgeConfig(90-90)validator-cli/src/utils/ethers.ts (4)
getVeaInbox(109-109)getVeaOutbox(110-110)getClaimValidator(114-114)getClaimer(115-115)validator-cli/src/utils/epochHandler.ts (2)
setEpochRange(76-76)getBlockFromEpoch(76-76)validator-cli/src/utils/claim.ts (1)
getClaim(144-144)validator-cli/src/ArbToEth/claimer.ts (1)
checkAndClaim(116-116)
validator-cli/src/utils/botConfig.ts (1)
validator-cli/src/utils/errors.ts (3)
InvalidBotPathError(55-55)DevnetOwnerNotSetError(56-56)InvalidNetworkError(57-57)
validator-cli/src/utils/epochHandler.ts (1)
validator-cli/src/consts/bridgeRoutes.ts (1)
getBridgeConfig(90-90)
validator-cli/src/ArbToEth/transactionHandler.ts (4)
validator-cli/src/utils/emitter.ts (1)
defaultEmitter(4-4)validator-cli/src/consts/bridgeRoutes.ts (1)
getBridgeConfig(90-90)validator-cli/src/utils/errors.ts (1)
ClaimNotSetError(53-53)validator-cli/src/utils/arbMsgExecutor.ts (1)
messageExecutor(82-82)
validator-cli/src/utils/ethers.ts (4)
validator-cli/src/ArbToEth/claimer.ts (1)
checkAndClaim(116-116)validator-cli/src/ArbToEth/transactionHandlerDevnet.ts (1)
ArbToEthDevnetTransactionHandler(19-71)validator-cli/src/ArbToEth/transactionHandler.ts (1)
ArbToEthTransactionHandler(68-409)validator-cli/src/utils/errors.ts (1)
TransactionHandlerNotDefinedError(54-54)
validator-cli/src/ArbToEth/transactionHandler.test.ts (4)
validator-cli/src/ArbToEth/transactionHandler.ts (4)
TransactionHandlerConstructor(23-32)ArbToEthTransactionHandler(68-409)MAX_PENDING_CONFIRMATIONS(65-65)Transaction(34-37)validator-cli/src/utils/emitter.ts (2)
MockEmitter(6-14)defaultEmitter(4-4)validator-cli/src/consts/bridgeRoutes.ts (1)
getBridgeConfig(90-90)validator-cli/src/utils/errors.ts (1)
ClaimNotSetError(53-53)
🪛 Biome (1.9.4)
validator-cli/src/utils/ethers.ts
[error] 39-45: This case is falling through to the next case.
Add a break or return statement to the end of this case to prevent fallthrough.
(lint/suspicious/noFallthroughSwitchClause)
[error] 95-101: This case is falling through to the next case.
Add a break or return statement to the end of this case to prevent fallthrough.
(lint/suspicious/noFallthroughSwitchClause)
🔇 Additional comments (32)
validator-cli/src/watcher.ts (3)
22-24: Consider error handling within thewatchsignature.The
watchfunction returns a Promise that may reject under certain errors (e.g., provider connectivity issues). Consider adding a try-catch block internally or returning specific error states that the caller can handle. This helps avoid potentially unhandled Promise rejections.
64-71: Revisit epoch logic for already-watched networks.When the watcher or
DEVNETis detected, the code setsepochRangeto a single value. If the watcher was briefly offline or missed epochs, this logic might skip unprocessed epochs. Verify if this is intentional. If not, consider preserving a range of missed epochs to avoid gaps in claim processing.
96-101: Ensure claim checking order aligns with the intended logic.The conditional blocks check whether to challenge or claim based on
pathandclaim != null. Confirm that the order of operations (challenger > claimer) is correct for your use case. If concurrent challenging and claiming is needed for the same execution, consider refactoring.validator-cli/src/utils/botEvents.ts (1)
1-41: Well-organized event enumeration with comprehensive state coverage.The
BotEventsenum is well-structured with clear categorization into five logical groups (Bridger, Epoch, Claim, Devnet, and Transaction states). This organization will make event tracking and logging more maintainable and consistent throughout the application.validator-cli/tsconfig.json (1)
3-8: Good TypeScript configuration enhancement for improved module handling.Adding
resolveJsonModuleandesModuleInteropoptions is a best practice for TypeScript projects. These changes will enable:
- Importing JSON files directly as modules
- Better interoperability with CommonJS modules (important for testing frameworks like Jest)
This aligns well with the broader testing improvements in this PR.
validator-cli/src/utils/botConfig.test.ts (1)
1-25: Good test coverage for the getBotPath function.The test suite thoroughly covers all key scenarios:
- Default path behavior
- Claimer path option
- Challenger path option
- Error handling for invalid paths
The test structure follows Jest best practices with clear organization and assertions.
validator-cli/src/ArbToEth/validator.ts (1)
19-28: Consider replacinganyand removing unused parameters.This interface uses
anyforveaInboxandveaOutbox(lines 19, 22), which undermines type safety. Also,fetchClaim(line 25) appears unused in the function. To improve maintainability and reliability, consider replacinganywith the appropriate contract interfaces (e.g.,VeaInboxArbToEth,VeaOutboxArbToEth) and removing or implementingfetchClaimif needed.validator-cli/src/consts/bridgeRoutes.ts (1)
86-88: Minimal bridging lookup seems sufficient.Returning
undefinedfor unknown chain IDs (line 87) is consistent with your existing design patterns. The upper layers can handle error reporting or fallback logic. No further validation is required given the retrieved learnings about centralized error handling in upper layers.validator-cli/src/ArbToEth/claimer.ts (1)
18-20: Clarify usage ofanyforveaInboxandveaOutbox.
From previous reviews, there's a suggestion to replaceanywith stronger contract interfaces. However, according to the retrieved learnings, this is intentionally done to keep the code chain-agnostic. Consider adding a code comment to clarify thatanytypes are used by design.validator-cli/src/utils/ethers.ts (12)
5-5: No issues with the newly added import
It referencesVeaOutboxArbToGnosisDevnet__factoryand is consistent with the rest of the codebase.
13-14: New imports for claim logic
ImportingchallengeAndResolveClaimArbToEthandcheckAndClaimis appropriate for reusing common claim-handling functionality.
15-16: Importing transaction handlers
Bringing inArbToEthTransactionHandlerandArbToEthDevnetTransactionHandlerensures the correct environment-based logic.
17-17: Custom error import
TheTransactionHandlerNotDefinedErrorcentralizes error handling for unsupported scenarios. Looks good.
18-18: Network enum import
Using a typedNetworkenum increases clarity and maintainability across the codebase.
20-21:getWalletfunction
Constructing aWalletwith aJsonRpcProvideris straightforward and appropriate for customized RPC endpoints.
37-55: Potential fallthrough ingetVeaOutbox
Static analysis notes the outer switch might fall through if the inner switch conditions are not met. If this is unintentional, add defaults at both levels or explicitly throw errors whenchainIdornetworkis unsupported.Please confirm your intended behavior. Consider adding explicit default cases:
function getVeaOutbox(...) { switch (chainId) { case 11155111: switch (network) { case Network.DEVNET: return ... case Network.TESTNET: return ... + default: + throw new Error(`Unsupported network: ${network} for chainId 11155111 in getVeaOutbox.`); } + break; case 10200: switch (network) { case Network.DEVNET: return ... case Network.TESTNET: return ... + default: + throw new Error(`Unsupported network: ${network} for chainId 10200 in getVeaOutbox.`); } + break; + default: + throw new Error(`Unsupported chainId: ${chainId} in getVeaOutbox.`); } }🧰 Tools
🪛 Biome (1.9.4)
[error] 39-45: This case is falling through to the next case.
Add a
breakorreturnstatement to the end of this case to prevent fallthrough.(lint/suspicious/noFallthroughSwitchClause)
64-66:getWETH
This function is concise, straightforward, and aligns with the typical usage ofIWETH__factory.
68-70:getVeaOutboxArbToEthDevnet
No issues found. This specialized helper for devnet usage appears consistent with existing*Devnetpatterns.
72-74:getAMB
Connecting to theIAMBcontract withgetWalletis standard. Looks fine.
93-105: Nested switch fallback
The code falls through to the outer default if the inner switch fornetworkis unmatched. This is valid if intentional.🧰 Tools
🪛 Biome (1.9.4)
[error] 95-101: This case is falling through to the next case.
Add a
breakorreturnstatement to the end of this case to prevent fallthrough.(lint/suspicious/noFallthroughSwitchClause)
109-110: Export additions
Including the newly introduced methods (getVeaInbox,getVeaOutbox, etc.) in the export list is consistent with the changes made.Also applies to: 114-116, 117-117
validator-cli/src/ArbToEth/transactionHandler.test.ts (11)
1-13: Imports and setup
All relevant classes, types, events, and errors are imported, ensuring test coverage for claims, transactions, and event emissions.
14-82: Constructor tests
Thorough coverage of both scenarios (with and without a claim). Ensures initial properties are set as expected.
84-146:checkTransactionStatussuite
Tests multiple states: final, pending, not final, and null hash. This approach robustly ensures various transaction outcomes are handled correctly.
148-185:makeClaimtests
Validates claim creation and checks for pending transactions. This ensures correct usage ofveaOutbox.claim.
187-239:startVerificationtests
Covering normal operation, pending transactions, missing claims, and insufficient timeouts. This effectively tests the gating conditions for verification logic.
241-290:verifySnapshottests
Ensures snapshot verification respects pending transactions, missing claims, and timeout logic. Good detail in error handling checks.
292-335:withdrawClaimDeposittests
Confirms that deposits are withdrawn only when not pending and checks for null claims. Straightforward coverage.
338-384:challengeClaimtests
Validates challenging logic, including pending challenges and an unset claim scenario. Properly emits and checks relevant events.
386-430:withdrawChallengeDeposittests
Mirrors the structure ofwithdrawClaimDeposittests, focusing on concurrency checks and error conditions for claiming logic.
432-472:sendSnapshottests
Ensures snapshots are sent only when no transaction is pending, covering the null-claim path as well. Proper event emission is verified.
474-506:resolveChallengedClaimtests
Checks that finalize logic runs only if the previous snapshot transaction is not pending. Good detail on verifying concurrency gating.
| const bridges: { [chainId: number]: Bridge } = { | ||
| 11155111: { | ||
| chain: "sepolia", | ||
| deposit: BigInt("1000000000000000000"), | ||
| minChallengePeriod: 10800, | ||
| sequencerDelayLimit: 86400, | ||
| inboxRPC: process.env.RPC_ARB, | ||
| outboxRPC: process.env.RPC_ETH, | ||
| routeConfig: arbToEthConfigs, | ||
| }, | ||
| 10200: { | ||
| chain: "chiado", | ||
| deposit: BigInt("1000000000000000000"), | ||
| minChallengePeriod: 10800, | ||
| sequencerDelayLimit: 86400, | ||
| inboxRPC: process.env.RPC_ARB, | ||
| outboxRPC: process.env.RPC_GNOSIS, | ||
| routerRPC: process.env.RPC_ETH, | ||
| routeConfig: arbToGnosisConfigs, | ||
| }, | ||
| }; |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Ensure environment variables align with bridging logic.
Populating bridges with environment-driven RPCs (lines 70, 71, 79–81) is an effective mechanism. Confirm that the .env and environment variables (e.g., RPC_ARB, RPC_ETH, RPC_GNOSIS) are set for each supported chain. Missing or incorrect variables could cause runtime failures.
Run this script to confirm all required environment variables are set:
🏁 Script executed:
#!/bin/bash
set -e
required_vars=("RPC_ARB" "RPC_ETH" "RPC_GNOSIS")
for var in "${required_vars[@]}"; do
if [ -z "${!var}" ]; then
echo "Environment variable $var is not set."
exit 1
else
echo "$var is set to '${!var}'"
fi
doneLength of output: 210
Missing Required Environment Variable for Bridging Logic
The bridge configuration in validator-cli/src/consts/bridgeRoutes.ts depends on environment-driven RPCs. Our verification script revealed that RPC_ARB is not set, which is critical for the sepolia chain setup. Please ensure that the .env (or equivalent environment configuration) includes all required variables (RPC_ARB, RPC_ETH, RPC_GNOSIS) to prevent runtime failures.
There was a problem hiding this comment.
the .env.dist is an example file
There was a problem hiding this comment.
@mani99brar You're absolutely right. The .env.dist file is indeed just an example template that developers would use to create their own .env file with actual values. Thanks for the clarification. The environment variables like RPC_ARB would be properly set in the actual runtime environment, not in the example file.
🧠 Learnings used
Learnt from: mani99brar
PR: kleros/vea#370
File: bridger-cli/src/consts/bridgeRoutes.ts:28-30
Timestamp: 2025-03-28T01:51:48.908Z
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.
| fetchLatestClaimedEpoch(veaOutbox.target), | ||
| ]); | ||
|
|
||
| newMessagesToBridge = savedSnapshot != outboxStateRoot && savedSnapshot != ethers.ZeroHash; |
There was a problem hiding this comment.
Replace ethers.ZeroHash & ethers.ZeroAddress with standardized constants.
ethers.ZeroHash and ethers.ZeroAddress may not exist in certain Ethers versions. The standard constants are ethers.constants.HashZero and ethers.constants.AddressZero.
Apply this diff to fix the references:
- newMessagesToBridge = savedSnapshot != outboxStateRoot && savedSnapshot != ethers.ZeroHash;
+ newMessagesToBridge = savedSnapshot != outboxStateRoot && savedSnapshot != ethers.constants.HashZero;
- if (claim.challenger != ethers.ZeroAddress) {
+ if (claim.challenger != ethers.constants.AddressZero) {Also applies to: 99-99
There was a problem hiding this comment.
The constants are correct for "ethers v6"
There was a problem hiding this comment.
🧩 Analysis chain
@mani99brar You're absolutely correct. In ethers.js v6, the constants were reorganized and ethers.ZeroHash and ethers.ZeroAddress are indeed the proper way to reference these values, replacing the v5 approach of ethers.constants.HashZero and ethers.constants.AddressZero. Thank you for the clarification.
🌐 Web query:
What are the correct constants for zero hash and zero address in ethers.js v6?
💡 Result:
In Ethers.js v6, the constants for zero hash and zero address have been updated from their v5 counterparts. Here are the correct constants:
Zero Address
ethers.ZeroAddress
Represents the Ethereum zero address as a string:
"0x0000000000000000000000000000000000000000" [6][7]
Zero Hash
ethers.ZeroHash
Represents the 32-byte zero hash as a string:
"0x0000000000000000000000000000000000000000000000000000000000000000" [6][7]
Key changes from v5 to v6:
- Removed nested
constantsobject - Renamed
AddressZero→ZeroAddress - Renamed
HashZero→ZeroHash - Now uses direct top-level exports instead of properties under
ethers.constants[8][12]
These constants are commonly used for:
- Initializing empty address fields in smart contracts
- Checking for unset values
- Default parameters in transaction configurations
- Hash comparisons in cryptographic operations [1][6][13]
Example usage:
import { ZeroAddress, ZeroHash } from 'ethers';
console.log(ZeroAddress); // 0x0000...0000
console.log(ZeroHash); // 0x0000...0000Citations:
- 1: https://docs.ethers.org/v5/api/utils/constants/
- 2: https://docs.ethers.org/v4/api-utils.html
- 3: https://docs.zksync.io/zksync-era/sdk/js/ethers/api/v6/providers/provider
- 4: https://forum.openzeppelin.com/t/how-to-sign-hashes-with-ethers-js-v6-to-get-the-signer-with-solidity/41895
- 5: https://www.youtube.com/watch?v=VJYeqTbZ8lY
- 6: https://docs.ethers.org/v6/api/constants/
- 7: https://docs.ethers.org/v6/single-page/
- 8: https://docs.ethers.org/v6/migrating/
- 9: https://docs.ethers.org/v5/api/utils/address/
- 10: https://docs.ethers.org/v6/getting-started/
- 11: Use ethers.constants.ZeroAddress instead of ZERO_ADDRESS ethereum-optimism/contracts#329
- 12: [v6] Migration Notes: Things that changed ethers-io/ethers.js#3795
- 13: https://docs.ethers.org/v6/api/hashing/
- 14: v6: getBlock doesn't return transaction objects ethers-io/ethers.js#3683
@mani99brar You're absolutely correct. In ethers.js v6, the constants were reorganized and ethers.ZeroHash and ethers.ZeroAddress are indeed the proper way to reference these values, replacing the v5 approach of ethers.constants.HashZero and ethers.constants.AddressZero. Thank you for the clarification.
✏️ Learnings added
Learnt from: mani99brar
PR: kleros/vea#396
File: validator-cli/src/ArbToEth/claimer.ts:77-77
Timestamp: 2025-03-31T09:03:43.595Z
Learning: In ethers.js v6, the correct constants for zero values are `ethers.ZeroHash` and `ethers.ZeroAddress`, which replaced the v5 constants `ethers.constants.HashZero` and `ethers.constants.AddressZero`.
Learnt from: mani99brar
PR: kleros/vea#396
File: validator-cli/src/ArbToEth/claimer.ts:77-77
Timestamp: 2025-03-31T09:03:43.595Z
Learning: In ethers.js v6, the correct constants for zero values are `ethers.ZeroHash` and `ethers.ZeroAddress`, which replaced the v5 constants `ethers.constants.HashZero` and `ethers.constants.AddressZero`.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
| 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 - 1000); | ||
| const secPerBlock = (latestBlock.timestamp - baseBlock.timestamp) / (latestBlock.number - baseBlock.number); | ||
| const blockFallBack = Math.floor((latestBlock.timestamp - epochTimestamp) / secPerBlock); | ||
| return latestBlock.number - blockFallBack; | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling for fetching blocks from the provider.
getBlock("latest") and getBlock(...) can fail or return null, and there’s no retry or error handling. Consider adding a try/catch block to handle any unexpected null values or network errors.
There was a problem hiding this comment.
@mani99brar Understood. I'll respect your design choice to let errors propagate up the call stack rather than handling them within this function. This approach is valid and can simplify the code at this level.
(_/)
(•ᴗ•)
/ >📝
✏️ Learnings added
Learnt from: mani99brar
PR: kleros/vea#396
File: validator-cli/src/utils/epochHandler.ts:67-74
Timestamp: 2025-03-31T09:03:06.354Z
Learning: In `validator-cli/src/utils/epochHandler.ts`, the `getBlockFromEpoch` function deliberately omits error handling for provider calls like `getBlock("latest")` and `getBlock(...)`. The developer prefers to let errors propagate up the call stack rather than handling them within this function.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
🧠 Learnings used
Learnt from: mani99brar
PR: kleros/vea#370
File: bridger-cli/src/utils/epochHandler.ts:13-24
Timestamp: 2025-03-28T01:51:43.095Z
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: 2025-03-28T01:51:30.640Z
Learning: In the file `bridger-cli/src/utils/epochHandler.ts`, the calculation within the `checkForNewEpoch` function is correct as implemented and has been tested.
Learnt from: mani99brar
PR: kleros/vea#370
File: bridger-cli/src/utils/epochHandler.ts:38-50
Timestamp: 2025-03-28T01:51:48.908Z
Learning: The `getBlockNumberFromEpoch` function in `bridger-cli/src/utils/epochHandler.ts` is currently unused and will be addressed in a future issue.
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (5)
validator-cli/src/utils/errors.ts (1)
19-25: Consider updating the error name property to match the renamed class.Currently, the class is named
NotDefinedError, butthis.nameis set to"TransactionHandlerNotDefinedError". This can create confusion when logging or debugging errors.Apply this diff to ensure consistency:
- this.name = "TransactionHandlerNotDefinedError"; + this.name = "NotDefinedError";validator-cli/src/utils/ethers.ts (1)
84-99: Consider combining DEVNET and TESTNET cases for the claimer function.The fallthrough logic suggests the same claimer applies to both networks. For clarity, you can combine them:
switch (network) { - case Network.DEVNET: - case Network.TESTNET: - return checkAndClaim; - default: - throw new InvalidNetworkError(`${network}(claimer)`); + case Network.DEVNET: + case Network.TESTNET: + return checkAndClaim; + default: + throw new InvalidNetworkError(`${network}(claimer)`); }validator-cli/src/consts/bridgeRoutes.ts (1)
86-89: Improve error message when no bridge is found.Currently, the error only displays
"Bridge not found for chain". Including the chain ID would ease debugging.- if (!bridge) throw new Error(`Bridge not found for chain`); + if (!bridge) throw new Error(`Bridge not found for chain: ${chainId}`);validator-cli/src/watcher.ts (2)
65-69: Consider the epoch processing orderThe loop processes epochs from newest to oldest (i starts at the end of the array and decrements). While this works, processing older epochs first might be more intuitive in some cases, especially when handling historical data. Consider whether this reverse processing order is intentional and necessary.
39-44: Add validation for chainIdThe code uses
chainIdwithout validating that it's a valid number. If it's invalid, it could lead to unexpected behavior.const { chainId, networks } = networkConfig; + if (isNaN(chainId)) { + throw new Error("Invalid or missing chainId in network configuration"); + } const { routeConfig, inboxRPC, outboxRPC } = getBridgeConfig(chainId);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro (Legacy)
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (7)
validator-cli/ecosystem.config.js(1 hunks)validator-cli/package.json(1 hunks)validator-cli/src/consts/bridgeRoutes.ts(1 hunks)validator-cli/src/utils/errors.ts(2 hunks)validator-cli/src/utils/ethers.ts(3 hunks)validator-cli/src/watcher.ts(2 hunks)validator-cli/tsconfig.json(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
validator-cli/src/consts/bridgeRoutes.ts (1)
Learnt from: mani99brar
PR: kleros/vea#370
File: bridger-cli/src/consts/bridgeRoutes.ts:28-30
Timestamp: 2025-03-31T10:11:27.845Z
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.
🧬 Code Definitions (2)
validator-cli/src/watcher.ts (6)
validator-cli/src/utils/errors.ts (1)
MissingEnvError(66-66)validator-cli/src/utils/botConfig.ts (2)
getBotPath(21-38)getNetworkConfig(50-67)validator-cli/src/consts/bridgeRoutes.ts (1)
getBridgeConfig(92-92)validator-cli/src/utils/epochHandler.ts (2)
setEpochRange(76-76)getBlockFromEpoch(76-76)validator-cli/src/utils/claim.ts (1)
getClaim(144-144)validator-cli/src/ArbToEth/claimer.ts (2)
checkAndClaim(116-116)CheckAndClaimParams(116-116)
validator-cli/src/utils/ethers.ts (4)
validator-cli/src/utils/errors.ts (2)
NotDefinedError(62-62)InvalidNetworkError(65-65)validator-cli/src/ArbToEth/claimer.ts (1)
checkAndClaim(116-116)validator-cli/src/ArbToEth/transactionHandlerDevnet.ts (1)
ArbToEthDevnetTransactionHandler(19-71)validator-cli/src/ArbToEth/transactionHandler.ts (1)
ArbToEthTransactionHandler(68-409)
🪛 Biome (1.9.4)
validator-cli/src/utils/ethers.ts
[error] 39-45: This case is falling through to the next case.
Add a break or return statement to the end of this case to prevent fallthrough.
(lint/suspicious/noFallthroughSwitchClause)
[error] 102-108: This case is falling through to the next case.
Add a break or return statement to the end of this case to prevent fallthrough.
(lint/suspicious/noFallthroughSwitchClause)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Analyze (javascript)
🔇 Additional comments (40)
validator-cli/tsconfig.json (2)
2-4: Review "include" Array ConfigurationThe
"include"field is correctly configured to target the"src"directory. This setup is ideal if all source files reside there. Ensure that any additional directories containing TypeScript files are added if needed in the future.
5-13: Review New Compiler OptionsThe addition of the new
compilerOptionsis well-aligned with modern TypeScript practices:
baseUrl: "."establishes the root for module resolution.module: "commonjs"andmoduleResolution: "node"are appropriate choices for Node-based environments.esModuleInterop: true,resolveJsonModule: true, andallowSyntheticDefaultImports: trueprovide enhanced compatibility when importing modules, especially for projects mixing CommonJS and ES modules.outDir: "dist"designates a conventional output directory for compiled JavaScript files.These settings will enable smoother development and build processes. Make sure that these options match the overall project requirements and that any external tooling (e.g., linters, bundlers) is configured accordingly.
validator-cli/package.json (2)
25-26: Dependency Update: PM2 and TypeScript Versions
The upgrade ofpm2to"^6.0.5"is a noteworthy improvement. Please double-check PM2’s release notes for any breaking changes or behavioral differences that might impact your runtime environment. Thetypescriptdependency remains at"^4.9.5", which is consistent with previous settings.
19-27: Dependency Cleanup: Removal of Unused Blockchain Libraries
The removal of theweb3andweb3-batched-senddependencies signals a significant architectural change. Please ensure that there are no remaining references to these packages in the codebase, and that all blockchain interaction logic has been updated accordingly to reflect this cleanup.validator-cli/src/utils/errors.ts (5)
27-33: New error class for invalid bot paths looks good.No issues found; the implementation is clear and straightforward.
35-41: Devnet owner error definition is clear and consistent.No concerns here; the name and message accurately convey the error’s intent.
43-49: Invalid network error is well-defined.The class name and error message align well, ensuring better debugging for unsupported networks.
51-57: Missing environment variable error is properly implemented.Having a dedicated error class for missing environment variables will improve configuration troubleshooting.
59-67: Export block changes are good to go.All error classes are correctly exported for use elsewhere in the application.
validator-cli/src/utils/ethers.ts (13)
5-5: Import changes for Gnosis Devnet factory look fine.
14-14: ImportingcheckAndClaimis appropriate.
16-16: Devnet transaction handler import is properly referenced.
17-17: Importing new errors classes is correct.
18-18:Networkenum import is aligned with the new usage.
20-21: RefinedgetWalletsignature is straightforward and correct.
28-28:getVeaInboxsignature update to includenetworkis consistent with multi-network support.
57-60: Router factory logic for chain 10200 looks correct.If other chains are supported in the future, consider adding a default case.
64-65:getWETHupdated signature is acceptable.
68-69:getVeaOutboxArbToEthDevnetaddition is consistent with Devnet usage.
72-73:getAMBupdate to acceptrpcUrlis fine.
76-83: RevisedgetClaimValidatoradheres to the new multi-network structure.
122-122: ExportinggetClaimercompletes the multi-network refactor.validator-cli/src/consts/bridgeRoutes.ts (9)
3-7: Devnet and testnet deployment imports for Arbitrum→Ethereum are correctly added.
8-10: Devnet deployment imports for Arbitrum→Gnosis are correctly added.
21-22:routerRPCandrouteConfigfields inBridgeinterface appear logical for multi-network support.
25-30:RouteConfigstype definition is well-structured, enabling flexible expansions.
35-35:Networkenum is a clear representation of supported networks.
37-48:arbToEthConfigsentries align with the newly imported JSON deployments.
50-62:arbToGnosisConfigsentries follow the same pattern for devnet and testnet.
72-72: AssigningrouteConfig: arbToEthConfigsfor chain 11155111 is consistent with the new approach.
81-82:routerRPCplusrouteConfig: arbToGnosisConfigsfor chain 10200 is correct.validator-cli/ecosystem.config.js (1)
4-8: PM2 configuration setup looks good!The configuration correctly sets up the TypeScript environment for the validator-cli application, using ts-node with the proper arguments and environment variables.
Also applies to: 13-13
validator-cli/src/watcher.ts (8)
29-30: Good addition of private key validationThe private key validation properly checks for the required environment variable and throws a specific error type, which improves error handling.
31-34: Well-structured bot configuration initializationThe code now properly extracts bot path from command line arguments and network configuration from environment variables, which provides better flexibility and configurability.
36-38: Good optimization with watched networks trackingMaintaining a list of watched networks and only processing the latest epoch for previously watched networks is an excellent optimization that reduces redundant processing.
Also applies to: 114-116
48-64: Improved epoch range handling with network-specific logicThe code now properly handles epoch ranges differently based on the network type, with special handling for DEVNET. This is a good improvement that accommodates different network characteristics.
75-88: Clean conditional logic for validator functionalityThe code clearly separates the validation logic based on the bot path, ensuring validators only run when appropriate. The parameter object pattern used here improves code readability and maintainability.
89-104: Well-structured claimer implementationThe claimer functionality is properly implemented with a clean parameter object pattern, making the code more readable and maintainable. The conditional execution based on bot path correctly implements the PR objectives.
106-112: Efficient transaction handling with memory managementThe transaction handling logic now properly manages memory by deleting transactions for old epochs that are no longer needed, which helps prevent memory leaks during long-running operations.
123-123: Clean implementation of wait functionThe wait function is implemented as a standalone function using a Promise, which is a good practice for making the code more readable and reusable.
| const getTransactionHandler = (chainId: number, network: Network) => { | ||
| switch (chainId) { | ||
| case 11155111: | ||
| return ArbToEthTransactionHandler; | ||
| switch (network) { | ||
| case Network.DEVNET: | ||
| return ArbToEthDevnetTransactionHandler; | ||
| case Network.TESTNET: | ||
| return ArbToEthTransactionHandler; | ||
| } | ||
| default: | ||
| throw new TransactionHandlerNotDefinedError(); | ||
| throw new NotDefinedError("Transaction Handler"); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add a default case for the nested switch covering network.
When chainId === 11155111, if network is neither devnet nor testnet, the function returns undefined. This should explicitly error out instead:
case Network.TESTNET:
return ArbToEthTransactionHandler;
+ default:
+ throw new InvalidNetworkError(`${network}(transactionHandler)`);📝 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 getTransactionHandler = (chainId: number, network: Network) => { | |
| switch (chainId) { | |
| case 11155111: | |
| return ArbToEthTransactionHandler; | |
| switch (network) { | |
| case Network.DEVNET: | |
| return ArbToEthDevnetTransactionHandler; | |
| case Network.TESTNET: | |
| return ArbToEthTransactionHandler; | |
| } | |
| default: | |
| throw new TransactionHandlerNotDefinedError(); | |
| throw new NotDefinedError("Transaction Handler"); | |
| } | |
| const getTransactionHandler = (chainId: number, network: Network) => { | |
| switch (chainId) { | |
| case 11155111: | |
| switch (network) { | |
| case Network.DEVNET: | |
| return ArbToEthDevnetTransactionHandler; | |
| case Network.TESTNET: | |
| return ArbToEthTransactionHandler; | |
| default: | |
| throw new InvalidNetworkError(`${network}(transactionHandler)`); | |
| } | |
| default: | |
| throw new NotDefinedError("Transaction Handler"); | |
| } | |
| }; |
🧰 Tools
🪛 Biome (1.9.4)
[error] 102-108: This case is falling through to the next case.
Add a break or return statement to the end of this case to prevent fallthrough.
(lint/suspicious/noFallthroughSwitchClause)
| function getVeaOutbox(veaOutboxAddress: string, privateKey: string, rpcUrl: string, chainId: number, network: Network) { | ||
| switch (chainId) { | ||
| case 11155111: | ||
| return VeaOutboxArbToEth__factory.connect(veaOutboxAddress, getWallet(privateKey, web3ProviderURL)); | ||
| switch (network) { | ||
| case Network.DEVNET: | ||
| return VeaOutboxArbToEthDevnet__factory.connect(veaOutboxAddress, getWallet(privateKey, rpcUrl)); | ||
| case Network.TESTNET: | ||
| return VeaOutboxArbToEth__factory.connect(veaOutboxAddress, getWallet(privateKey, rpcUrl)); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Prevent potential switch fallthrough for chainId === 11155111.
Currently, if network doesn’t match DEVNET or TESTNET, the function will return undefined instead of throwing an error. This can cause unexpected behaviors.
Add an explicit default case to handle unsupported networks, for example:
case Network.TESTNET:
return VeaOutboxArbToEth__factory.connect(veaOutboxAddress, getWallet(privateKey, rpcUrl));
+ default:
+ throw new InvalidNetworkError(`${network}(outbox)`);
}📝 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 getVeaOutbox(veaOutboxAddress: string, privateKey: string, rpcUrl: string, chainId: number, network: Network) { | |
| switch (chainId) { | |
| case 11155111: | |
| return VeaOutboxArbToEth__factory.connect(veaOutboxAddress, getWallet(privateKey, web3ProviderURL)); | |
| switch (network) { | |
| case Network.DEVNET: | |
| return VeaOutboxArbToEthDevnet__factory.connect(veaOutboxAddress, getWallet(privateKey, rpcUrl)); | |
| case Network.TESTNET: | |
| return VeaOutboxArbToEth__factory.connect(veaOutboxAddress, getWallet(privateKey, rpcUrl)); | |
| } | |
| function getVeaOutbox(veaOutboxAddress: string, privateKey: string, rpcUrl: string, chainId: number, network: Network) { | |
| switch (chainId) { | |
| case 11155111: | |
| switch (network) { | |
| case Network.DEVNET: | |
| return VeaOutboxArbToEthDevnet__factory.connect(veaOutboxAddress, getWallet(privateKey, rpcUrl)); | |
| case Network.TESTNET: | |
| return VeaOutboxArbToEth__factory.connect(veaOutboxAddress, getWallet(privateKey, rpcUrl)); | |
| default: | |
| throw new InvalidNetworkError(`${network}(outbox)`); | |
| } | |
| // ... possibly other cases for different chainIds | |
| } | |
| } |
🧰 Tools
🪛 Biome (1.9.4)
[error] 39-45: This case is falling through to the next case.
Add a break or return statement to the end of this case to prevent fallthrough.
(lint/suspicious/noFallthroughSwitchClause)
| import veaInboxArbToGnosisTestnet from "@kleros/vea-contracts/deployments/sepolia/VeaOutboxArbToEthTestnet.json"; | ||
| import veaOutboxArbToGnosisTestnet from "@kleros/vea-contracts/deployments/chiado/VeaOutboxArbToGnosisTestnet.json"; | ||
| import veaRouterArbToGnosisTestnet from "@kleros/vea-contracts/deployments/sepolia/RouterArbToGnosisTestnet.json"; |
There was a problem hiding this comment.
Possible mismatch in naming vs. imported JSON for Gnosis testnet.
The variable veaInboxArbToGnosisTestnet references a file named VeaOutboxArbToEthTestnet.json. This may lead to confusion or errors.
Please confirm that this import is correct. If it truly is an inbox deployment file for Arbitrum→Gnosis testnet, update the file reference; otherwise, rename the variable to match the outbox reference.
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (8)
validator-cli/src/utils/epochHandler.ts (2)
68-75: Add JSDoc comment for the new getBlockFromEpoch function.While the function implementation looks good, it's missing documentation. Consider adding a JSDoc comment explaining the purpose, parameters, and return value similar to other functions in the file.
+/** + * Converts an epoch number to the corresponding block number. + * + * @param epoch - The epoch number to convert + * @param epochPeriod - The epoch period in seconds + * @param provider - The JSON RPC provider to use for fetching blocks + * + * @returns The block number corresponding to the epoch + */ 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 - 1000); const secPerBlock = (latestBlock.timestamp - baseBlock.timestamp) / (latestBlock.number - baseBlock.number); const blockFallBack = Math.floor((latestBlock.timestamp - epochTimestamp) / secPerBlock); return latestBlock.number - blockFallBack; };
70-71: Consider making the sample window size configurable.The block sampling window is hardcoded to 1000 blocks. Consider extracting this as a constant or optional parameter for better flexibility.
const getBlockFromEpoch = async (epoch: number, epochPeriod: number, provider: JsonRpcProvider): Promise<number> => { + const BLOCK_SAMPLE_SIZE = 1000; const epochTimestamp = epoch * epochPeriod; const latestBlock = await provider.getBlock("latest"); - const baseBlock = await provider.getBlock(latestBlock.number - 1000); + const baseBlock = await provider.getBlock(latestBlock.number - BLOCK_SAMPLE_SIZE); const secPerBlock = (latestBlock.timestamp - baseBlock.timestamp) / (latestBlock.number - baseBlock.number); const blockFallBack = Math.floor((latestBlock.timestamp - epochTimestamp) / secPerBlock); return latestBlock.number - blockFallBack; };validator-cli/src/utils/logger.ts (4)
23-31: Update event handler to properly handle networks array.The current implementation logs networks directly as an array object, which will display as "[object Object],[object Object]" in the console. Consider formatting the networks array for better readability.
- console.log(`Bot started for ${pathString} on ${networks}`); + const networkNames = networks.map(n => n.name).join(', '); + console.log(`Bot started for ${pathString} on networks: ${networkNames}`);
82-98: Add time estimates to verification wait messages.For logs involving time left (lines 89-91, 96-98), consider formatting the time in a human-readable format instead of showing raw numbers.
- console.log(`Verification cant start for epoch ${epoch}, time left: ${timeLeft}`); + const timeLeftFormatted = new Date(timeLeft * 1000).toISOString().substr(11, 8); // Format as HH:MM:SS + console.log(`Verification cant start for epoch ${epoch}, time left: ${timeLeftFormatted}`);Apply similar formatting to the CANT_VERIFY_SNAPSHOT message.
76-78: Improve TXN_EXPIRED log message clarity.The expired transaction message should provide more context about why a transaction expired and what actions might be needed.
- console.log(`Transaction(${transaction}) is expired`); + console.log(`Transaction(${transaction}) has expired. Consider retrying with higher gas or checking network conditions.`);
111-113: Add type annotation to EXECUTING_SNAPSHOT event handler.The epoch parameter is missing a type annotation, unlike other event handlers that properly type their parameters.
- emitter.on(BotEvents.EXECUTING_SNAPSHOT, (epoch) => { + emitter.on(BotEvents.EXECUTING_SNAPSHOT, (epoch: number) => {validator-cli/src/utils/botEvents.ts (2)
1-42: Add JSDoc comments to describe bot event groups and their purpose.The enum lacks documentation explaining each event's purpose and when it's triggered, which would improve code maintainability.
Consider adding JSDoc comments to document each event group's purpose:
export enum BotEvents { + /** + * Events related to the overall state of the bridger bot + */ // Bridger state STARTED = "started", WATCHING = "watching", // ... other events + /** + * Events related to epoch processing and state + */ // Epoch state // ... events + /** + * Events related to claim processing, verification, and challenges + */ // Claim state // ... events
33-33: Standardize naming convention for ADV_DEVNET event.Most events use full words (like WITHDRAWING_CHALLENGE_DEPOSIT), but ADV_DEVNET uses an abbreviation. Consider using the full name for consistency.
- ADV_DEVNET = "advance_devnet", + ADVANCE_DEVNET = "advance_devnet",
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro (Legacy)
📒 Files selected for processing (5)
validator-cli/src/ArbToEth/claimer.ts(1 hunks)validator-cli/src/utils/botEvents.ts(1 hunks)validator-cli/src/utils/epochHandler.test.ts(2 hunks)validator-cli/src/utils/epochHandler.ts(4 hunks)validator-cli/src/utils/logger.ts(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- validator-cli/src/ArbToEth/claimer.ts
🧰 Additional context used
🧠 Learnings (2)
validator-cli/src/utils/epochHandler.test.ts (1)
Learnt from: mani99brar
PR: kleros/vea#370
File: bridger-cli/src/utils/epochHandler.ts:13-24
Timestamp: 2025-03-31T10:11:18.544Z
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.ts (2)
Learnt from: mani99brar
PR: kleros/vea#370
File: bridger-cli/src/utils/epochHandler.ts:13-24
Timestamp: 2025-03-31T10:11:18.544Z
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#396
File: validator-cli/src/utils/epochHandler.ts:67-74
Timestamp: 2025-03-31T10:11:36.881Z
Learning: In `validator-cli/src/utils/epochHandler.ts`, the `getBlockFromEpoch` function deliberately omits error handling for provider calls like `getBlock("latest")` and `getBlock(...)`. The developer prefers to let errors propagate up the call stack rather than handling them within this function.
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: dependency-review
- GitHub Check: Analyze (javascript)
🔇 Additional comments (12)
validator-cli/src/utils/epochHandler.test.ts (5)
1-1: Import statement updated correctly to include EpochRangeParams.The import statement has been properly updated to include the new
EpochRangeParamsinterface, maintaining compatibility with the implementation file.
14-14: Adjustment in startEpoch calculation looks good.The change from
-2to-1is consistent with the updated calculation in the implementation.
20-33: Test correctly updated to use the new parameter object pattern.The test has been properly updated to use the new
EpochRangeParamsobject structure, matching the implementation changes inepochHandler.ts. The mockParams object is correctly typed and passed to the function.
34-34: Updated assertion matches new behavior.The assertion has been correctly updated to expect the last element to equal
currentEpochinstead of the previous behavior, aligning with implementation changes.
42-43: Simplified getLatestChallengeableEpoch test.The test has been appropriately simplified to directly pass the epoch period parameter, matching the function's new signature which no longer requires chainId and fetchBridgeConfig.
validator-cli/src/utils/epochHandler.ts (5)
4-10: Well-structured interface definition.The new
EpochRangeParamsinterface is clearly defined with appropriate types and optional parameters marked with?. This provides better type safety and documentation for the function parameters.
23-29: Good refactoring to use parameter object pattern.Converting from individual parameters to a structured object improves readability and maintainability, especially for functions with multiple parameters. The destructuring with default values is implemented correctly.
44-48: More concise array generation using Array.from.The refactored code uses
Array.fromto generate the epoch range array, which is more concise and readable than the previous implementation.
64-66: Function signature simplified appropriately.The
getLatestChallengeableEpochfunction has been correctly simplified to only require essential parameters, removing the dependency onchainIdandfetchBridgeConfig.
77-77: Export statement properly updated.All necessary exports are correctly included, making the new function and interface available to other modules.
validator-cli/src/utils/logger.ts (1)
1-136: Enhance logging implementation with structured logging and proper type safety.The logging implementation still uses direct console.log calls without structured logging or error handling, which was previously flagged.
Would you like me to provide a complete implementation with structured logging using Winston or another logging library?
validator-cli/src/utils/botEvents.ts (1)
4-9:❓ Verification inconclusive
Verify event handlers exist for all bridger state events.
Ensure that all newly added events like WATCHING and NO_CLAIM_REQUIRED have corresponding handlers in the logger and other relevant files.
🏁 Script executed:
#!/bin/bash # Check for event handling across the codebase echo "Checking for event handlers for WATCHING and NO_CLAIM_REQUIRED..." # Find emitter.emit calls for these events echo "WATCHING event emits:" rg "emit\(.*WATCHING" --type ts echo "NO_CLAIM_REQUIRED event emits:" rg "emit\(.*NO_CLAIM_REQUIRED" --type ts # Ensure these events are handled in other files besides logger.ts echo "Other handlers for these events:" rg "on\(.*WATCHING" --type ts | grep -v "logger\.ts" rg "on\(.*NO_CLAIM_REQUIRED" --type ts | grep -v "logger\.ts"Length of output: 716
Action Required: Confirm Event Handler Registrations for New Bot Events
The new events WATCHING and NO_CLAIM_REQUIRED are being correctly emitted in:
validator-cli/src/watcher.ts(WATCHING)validator-cli/src/ArbToEth/claimer.ts(NO_CLAIM_REQUIRED)However, our search for event listener registrations (using patterns like
on(…) outside oflogger.tsyielded no results. Please ensure that:
- Logger Handling: The
logger.tsfile properly registers and handles these new events.- Additional Subscribers: If any modules beyond logging are expected to react to these events, add the corresponding event listeners.
Review and verify these points manually to maintain consistent handling for all bridger state events.
|
Opening a new PR for this as there's been many changes since this PR was drafted. |
PR-Codex overview
This PR focuses on enhancing the
validator-cliproject by updating configurations, improving error handling, and adding new functionalities for claim processing and transaction management in the devnet and testnet environments.Detailed summary
watcherDevnet.tsand updatedtsconfig.jsonwith new compiler options.package.json.ecosystem.config.jsto configure the newvalidator-cliscript.getClaimResolveStateinclaim.ts.graphQueries.tsanderrors.ts.botConfig.tsfor command line argument parsing with new bot paths.epochHandler.tsto improve epoch range handling.transactionHandler.tsto support devnet transactions and added new transaction management logic.claimer.test.ts.logger.tsfor better event tracking.Summary by CodeRabbit
New Features
Improvements