Feat/happy path(claimer) in validator-cli #396
Conversation
WalkthroughThis pull request introduces significant enhancements to the validator CLI for managing cross-chain claims and transactions between Arbitrum and Gnosis networks. The changes include updates to RPC URLs and address configurations, the addition of new functions for claim management, epoch handling, transaction processing, and event logging. A new watcher mechanism has been implemented to monitor bridge activities, validate claims, and handle potential challenges. The modifications also involve adding Jest testing infrastructure, creating new utility modules, and refactoring existing code to support more flexible and robust blockchain interaction. Changes
Possibly related PRs
Suggested Labels
Suggested Reviewers
Poem
Tip 🌐 Web search-backed reviews and chat
✨ Finishing Touches
🪧 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'); | |
| } |
| } 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.
| 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; | |
| } | |
| }; |
| 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); | |
| } | |
| })(); | |
| } |
| const veaInbox = getVeaInbox(veaBridge.inboxAddress, process.env.PRIVATE_KEY, veaBridge.inboxRPC, chainId); | ||
| const veaOutbox = getVeaOutbox(veaBridge.outboxAddress, process.env.PRIVATE_KEY, veaBridge.outboxRPC, chainId); |
There was a problem hiding this comment.
Ensure 'PRIVATE_KEY' environment variable is defined
The process.env.PRIVATE_KEY is used to initialize veaInbox and veaOutbox, but there is no check to ensure it's defined. If it's missing, it may cause runtime errors or unintended behavior. Add a validation step to check for its existence.
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);📝 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 veaInbox = getVeaInbox(veaBridge.inboxAddress, process.env.PRIVATE_KEY, veaBridge.inboxRPC, chainId); | |
| const veaOutbox = getVeaOutbox(veaBridge.outboxAddress, process.env.PRIVATE_KEY, veaBridge.outboxRPC, chainId); | |
| 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); |
| 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.
|
|
||
| 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); |
| public async checkTransactionStatus(trnxHash: string | null, contract: ContractType): Promise<TransactionStatus> { | ||
| let provider: JsonRpcProvider; | ||
| if (contract === ContractType.INBOX) { | ||
| provider = this.veaInboxProvider; | ||
| } else if (contract === ContractType.OUTBOX) { | ||
| provider = this.veaOutboxProvider; | ||
| } | ||
|
|
There was a problem hiding this comment.
Handle Unknown Contract Types in checkTransactionStatus
The provider variable will be undefined if the contract parameter is not INBOX or OUTBOX, potentially causing runtime errors. Include a default case to handle unexpected contract types.
Apply this diff to handle unknown contract types:
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}`);
}📝 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.
| public async checkTransactionStatus(trnxHash: string | null, contract: ContractType): Promise<TransactionStatus> { | |
| let provider: JsonRpcProvider; | |
| if (contract === ContractType.INBOX) { | |
| provider = this.veaInboxProvider; | |
| } else if (contract === ContractType.OUTBOX) { | |
| provider = this.veaOutboxProvider; | |
| } | |
| public async checkTransactionStatus(trnxHash: string | null, contract: ContractType): Promise<TransactionStatus> { | |
| 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}`); | |
| } | |
| 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.
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}`); | |
| } | |
| }; |
PR-Codex overview
This PR focuses on enhancing the functionality and structure of the
validator-cliby introducing new features, refactoring existing code, and improving error handling. It also integrates testing capabilities and updates configuration files.Detailed summary
ShutdownSignalclass for shutdown management.MockEmitterto prevent console logs during tests.ClaimNotFoundError,ClaimNotSetError, and other custom errors.getBotPathfunction for command line argument parsing.getClaimand related functions for better claim handling.watchfunction to manage watching logic.initializefor event tracking.watcherArbToEth.tsfile and updated paths inpackage.json.Summary by CodeRabbit
Release Notes for VEA Validator CLI
New Features
Improvements
Bug Fixes
Testing
Chores