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 new utility functions for epoch handling, claim management, 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 updating configuration files, adding Jest testing infrastructure, and refactoring existing code to support more flexible and robust blockchain interaction. Changes
Sequence DiagramsequenceDiagram
participant Bot as Validator Bot
participant Bridge as Cross-Chain Bridge
participant Inbox as VEA Inbox
participant Outbox as VEA Outbox
Bot->>Bridge: Get Epoch Range
Bridge-->>Bot: Return Claimable Epochs
loop For Each Epoch
Bot->>Outbox: Check Claim Status
alt No Claim
Bot->>Inbox: Validate Snapshot
else Claim Exists
Bot->>Bridge: Challenge/Resolve Claim
end
end
Possibly related PRs
Suggested Labels
Suggested Reviewers
Poem
✨ 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/jest@29.5.14 jest@29.7.0 ts-jest@29.2.5 # Check for compatibility between versions npm info @types/jest@29.5.14 peerDependencies npm info ts-jest@29.2.5 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
PR-Codex overview
This PR focuses on enhancing the
validator-cliproject by introducing new functionalities, improving error handling, and organizing the code structure. It includes the addition of classes, utility functions, and tests to ensure robustness in the claim validation and processing workflow.Detailed summary
ShutdownSignalclass for managing shutdown states.MockEmitterclass to prevent logging during tests.ClaimNotFoundError,ClaimNotSetError, and other error classes.getBotPathfunction to determine bot paths from CLI arguments.getClaimForEpochandgetLastClaimedEpochfunctions for fetching claims.messageExecutorandgetMessageStatusfunctions with improved parameters.setEpochRangeandgetLatestChallengeableEpochfor epoch management.checkAndClaimfunction for claim processing with proper transaction handling.initializefunction.challengeAndResolveClaimandcheckAndClaim.Summary by CodeRabbit
Based on the comprehensive changes, here are the release notes:
Release Notes for VEA Validator CLI
New Features
Improvements
Bug Fixes
Testing
Chores