Feat(validator)/claimer #417
Conversation
WalkthroughThis pull request introduces extensive configuration and code updates for the validator-cli project. Changes include modifications to environment settings (RPC endpoints, network and subgraph configurations), revisions to transaction handling and claim processing logic, and the addition of new error classes, utilities, and tests. Several obsolete or devnet-specific files were removed, while the watcher, bot configuration, logger, and graph query modules were enhanced for clarity and robustness. Overall, the refactoring focuses on streamlining network-specific behavior, improving error handling, and ensuring consistency across the codebase. Changes
Sequence Diagram(s)sequenceDiagram
participant Caller as Watcher/Caller
participant Claimer as checkAndClaim
participant Provider as VeaOutbox/Provider
participant TxHandler as TransactionHandler
participant Evt as EventEmitter
Caller->>Claimer: Invoke checkAndClaim(params)
Claimer->>Provider: Fetch state root & finalized block
Provider-->>Claimer: Return state details
Claimer->>Claimer: Calculate claimable epoch
alt No existing claim & DEVNET
Claimer->>Claimer: Retrieve snapshot & last claim epoch
else Claim exists or non-DEVNET
Claimer->>TxHandler: Process claim (withdraw, verify, etc.)
end
Claimer->>Evt: Emit relevant events
Evt-->>Caller: Notify status update
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
✅ Deploy Preview for veascan ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 13
🧹 Nitpick comments (41)
validator-cli/src/utils/botConfig.ts (4)
1-1: Remove unused importDevnetOwnerNotSetError.
It seems you've importedDevnetOwnerNotSetErrorbut never used it in this file. Consider removing it to keep the imports clean.
16-38: Clarify the return type ofgetBotPath.
Your doc states the function returnsBotPaths, but its signature specifiesnumber. Consider updating the function signature to explicitly returnBotPathsfor stronger type safety.
45-62: Consider logging a warning when no chain IDs are found.
IfVEAOUTBOX_CHAINSis not set, this function silently returns an empty configuration array. Logging a warning could help diagnose misconfigurations.
64-72: Refine type casting invalidateNetworks.
Castingnetworksasunknownand then toNetwork[]is risky. Consider more explicit type checks or a guarded approach to enhance reliability and maintain strict type safety.validator-cli/src/ArbToEth/transactionHandlerDevnet.ts (1)
13-15: Consider makingdevnetAdvanceStateTxnmandatory.
If devnet usage always sets or needsdevnetAdvanceStateTxn, consider making it non-null to simplify your logic and reduce branching.validator-cli/src/ArbToEth/claimer.ts (1)
30-68: Solid setup forcheckAndClaim.
The calculation ofclaimAbleEpochand conditional creation oftransactionHandleris intuitive. Consider adding logs or error handling ifoutboxStateRootor the finalized block retrieval fails.validator-cli/src/watcher.ts (11)
33-35: Clarify CLI argument usage in documentation.
You createdcliCommandand derive the path from it. Provide inline comments or function docstrings to clarify how command-line arguments map togetBotPath.
36-36: ValidatenetworkConfigslength before indexing.
When you donetworkConfigs[0], ensure that the array is not empty. If it is, you could run into an out-of-bounds error.if (networkConfigs.length === 0) { throw new Error("No network configurations found. Please check your environment variables."); } emitter.emit(BotEvents.STARTED, path, networkConfigs[0].networks);
37-38: Consider using a Map for better clarity.
transactionHandlersandtoWatchare keyed by[epoch: number]or[key: string]. AMap<string, number[]>could be more self-documenting, but it’s fine if you prefer plain objects.
43-45: Emit watchers in a structured manner.
You emitBotEvents.WATCHINGfor each network. Consider grouping or labeling them more explicitly, especially if you manage logs across multiple watchers.
46-48: Initialize arrays only if needed.
This logic is fine, but you might consider a small helper function to managetoWatch[networkKey]initialization, to improve readability.
51-53: Consider concurrency or repeated calls.
You fetchlatestBlockrepeatedly. Evaluate if it might be more efficient to do a singleprovider.getBlock("latest")outside or cache it for a short duration.
67-69: Handle empty epoch arrays gracefully.
IftoWatch[networkKey]ever ends up empty, ensureiwon't become-1at runtime. The while loop won't run, but just watch for a potential logic corner case.
83-96: Separate logic for challenge resolution vs claim.
You're toggling challenge resolution based onpath > BotPaths.CLAIMERin the same loop. If more paths are added, consider factoring these into separate watchers or modules for clarity.
97-112: Collapse the if-block when path == BOTH.
If you anticipate adding new watchers, consolidating repeated logic for “CLAIMER or BOTH” ensures fewer code branches.
122-128: Check epoch push for duplicates.
You pushcurrentLatestEpochif>last epoch. If there's a big time jump or slow loop, you might skip epochs in between. Usually that's fine, but confirm it matches your strategy.
135-135: Makewaitreusable or configurable.
wait(10s)is a basic approach. If you want dynamic intervals or logging, consider turning this wrapper into a more robust utility.validator-cli/src/consts/bridgeRoutes.ts (4)
3-10: Maintain consistent import ordering.
You’re pulling in multiple JSON ABIs for devnet/testnet. Keep them organized (e.g., alphabetical or grouped by network) to avoid confusion as the file grows.
37-48: Validate epoch periods for each environment.
DEVNET: 1800/3600,TESTNET: 7200—these are presumably correct. Make sure devnet and testnet intervals align with actual chain configs or reason about finality times.
50-62: Check the Gnosis testnet router.
You’ve includedveaRouterforarbToGnosisConfigs[Network.TESTNET]. If a user sets up a devnet with a new router, you might need symmetrical logic.
86-89: Improve the error message.
When the bridge is not found, specify the chainId in the error:if (!bridge) { - throw new Error("Bridge not found for chain"); + throw new Error(`Bridge not found for chainId ${chainId}`); }validator-cli/src/utils/claim.test.ts (5)
1-1: Confirm usage ofgetAddress.
You importgetAddressfromethers, but usage in this file might be minimal. If you are not using it, consider removing it.
5-5: Avoid confusion withmockfromnode:test.
If you’re using Jest, mixing thenode:testmocking library can be confusing. Ensure your setup is consistent across the codebase.
18-21: Encapsulate mock functions with a naming convention.
getClaimForEpoch,getVerificationForClaim, andgetChallengerForClaimare all jest mocks. Use a clear naming strategy so it’s obvious these are test doubles.
40-42: Check dynamic/undeclared properties.
veaOutbox.getAddress = jest.fn()might be unused or leftover from older code. If it’s not needed, remove it.
204-204: Validate finalClaimNotFoundErrorusage.
Confirm that throwingClaimNotFoundErroris the desired outcome for an invalid claim. This helps ensure consistent error handling across the codebase.validator-cli/src/utils/claim.ts (3)
36-45: Update JSDoc comment to reflect new parameter structureThe function signature has been updated to use the
ClaimParamsinterface, but the JSDoc comment (lines 30-35) still refers to individual parameters.Consider updating the JSDoc to reflect the new parameter structure:
/** * - * @param veaOutbox VeaOutbox contract instance - * @param epoch epoch number of the claim to be fetched + * @param params ClaimParams object containing the parameters for fetching a claim * @returns claim type of ClaimStruct */
58-89: LGTM! Robust error handling with fallback mechanismThe try-catch block with fallback to graph queries is a great implementation of the PR objective to use subgraphs as fallbacks for contract event logs. This enhances resilience when searching the block range heuristically fails.
However, on line 82, consider using optional chaining:
- if (verificationFromGraph && verificationFromGraph.startTimestamp) { + if (verificationFromGraph?.startTimestamp) {🧰 Tools
🪛 Biome (1.9.4)
[error] 82-82: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
135-135: Use const instead of var for consistent styleModern JavaScript/TypeScript practices recommend using
constfor variables that aren't reassigned.- var claimResolveState: ClaimResolveState = { + const claimResolveState: ClaimResolveState = {validator-cli/src/utils/epochHandler.ts (1)
23-29: Update JSDoc comment to reflect new parameter structureThe function signature has been updated to use the
EpochRangeParamsinterface, but the JSDoc comment (lines 12-21) still refers to individual parameters.Consider updating the JSDoc to reflect the new parameter structure:
/** * Sets the epoch range to check for claims. * - * @param currentTimestamp - The current timestamp - * @param chainId - The chain ID - * @param now - The current time in milliseconds (optional, defaults to Date.now()) - * @param fetchBridgeConfig - The function to fetch the bridge configuration (optional, defaults to getBridgeConfig) + * @param params - EpochRangeParams object containing the parameters for setting epoch range * * @returns The epoch range to check for claims */validator-cli/src/utils/graphQueries.ts (1)
19-46: Consider parameterizing the GraphQL query.Inline string interpolation in GraphQL queries can sometimes be brittle or expose potential injection paths. Though the risk may be limited in your environment, using the variables feature of
graphql-requestcan make queries more maintainable and secure.- `{ - claims(where: {epoch: ${epoch}, outbox: "${outbox}"}) { ... } - }` + `query getClaimForEpoch($epoch: Int!, $outbox: String!) { + claims(where: {epoch: $epoch, outbox: $outbox}) { ... } + }`, + { epoch, outbox }validator-cli/src/ArbToEth/transactionHandler.ts (2)
64-65: Inaccurate code comment vs. actual duration.
MAX_PENDING_TIMEis defined as5 * 60 * 1000, i.e. 300,000ms (5 minutes), whereas the comment states “3 minutes.” Please reconcile one or the other for clarity.-export const MAX_PENDING_TIME = 5 * 60 * 1000; // 3 minutes +export const MAX_PENDING_TIME = 5 * 60 * 1000; // 5 minutes
294-335: Challenge deposit logic is sound but consider fallback checks.The logic ensures deposit coverage and sets safe upper bounds on maxPriorityFeePerGas. In a production environment, verifying that the deposit is sufficient and the user’s wallet has enough ETH for gas might help avoid silent transaction failures.
validator-cli/src/utils/logger.ts (6)
23-31: Consider clarifying array logging.
When multiple networks are provided, directly embedding thenetworksarray in the log could produce a raw array printout. For more readable logs, consider joining them or formatting them more explicitly (e.g.,networks.join(", ")).
49-51: Include epoch detail if relevant.
Currently, “No snapshot saved for epoch” does not show the epoch. If the data is available from the emitter, consider adding it for more diagnostic clarity.
89-91: Minor grammar refinement.
Consider changing “Verification cant start” to “Verification can’t start” or “Verification cannot start.”
93-95: Improve textual consistency.
Likewise, consider “Can't verify snapshot” or “Cannot verify snapshot” for clarity and professionalism.
119-120: Consider logging the epoch.
The message “Withdrawing challenge deposit for epoch” omits which epoch. If that data is known, including it can aid debugging.
133-135: Event logging is correct.
Optionally, you could include which epoch the challenger won.validator-cli/src/utils/ethers.ts (2)
28-28: Add an explicitNetworktype.
Althoughnetworkis used, it’s untyped in this function signature. Explicitly typing it asnetwork: Networkwould enhance type safety.
31-33: Potential missing default for chain IDs.
Thesecasestatements return contracts for known chain IDs, but if an unrecognized chain ID is passed, the function returns nothing. Consider adding adefaultcase to throw an error for unknown chain IDs.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (28)
validator-cli/.env.dist(1 hunks)validator-cli/ecosystem.config.js(1 hunks)validator-cli/package.json(1 hunks)validator-cli/src/ArbToEth/claimer.test.ts(1 hunks)validator-cli/src/ArbToEth/claimer.ts(1 hunks)validator-cli/src/ArbToEth/transactionHandler.test.ts(8 hunks)validator-cli/src/ArbToEth/transactionHandler.ts(5 hunks)validator-cli/src/ArbToEth/transactionHandlerDevnet.ts(1 hunks)validator-cli/src/ArbToEth/validator.test.ts(2 hunks)validator-cli/src/ArbToEth/validator.ts(4 hunks)validator-cli/src/consts/bridgeRoutes.ts(1 hunks)validator-cli/src/devnet/arbToChiado/happyPath.ts(0 hunks)validator-cli/src/devnet/arbToSepolia/happyPath.ts(0 hunks)validator-cli/src/utils/botConfig.test.ts(1 hunks)validator-cli/src/utils/botConfig.ts(1 hunks)validator-cli/src/utils/botEvents.ts(1 hunks)validator-cli/src/utils/claim.test.ts(8 hunks)validator-cli/src/utils/claim.ts(5 hunks)validator-cli/src/utils/devnet.ts(0 hunks)validator-cli/src/utils/epochHandler.test.ts(2 hunks)validator-cli/src/utils/epochHandler.ts(4 hunks)validator-cli/src/utils/errors.ts(2 hunks)validator-cli/src/utils/ethers.ts(3 hunks)validator-cli/src/utils/graphQueries.ts(1 hunks)validator-cli/src/utils/logger.ts(5 hunks)validator-cli/src/watcher.ts(2 hunks)validator-cli/src/watcherDevnet.ts(0 hunks)validator-cli/tsconfig.json(1 hunks)
💤 Files with no reviewable changes (4)
- validator-cli/src/devnet/arbToSepolia/happyPath.ts
- validator-cli/src/devnet/arbToChiado/happyPath.ts
- validator-cli/src/watcherDevnet.ts
- validator-cli/src/utils/devnet.ts
🧰 Additional context used
🧬 Code Definitions (13)
validator-cli/src/utils/botConfig.test.ts (2)
validator-cli/src/utils/botConfig.ts (1)
getBotPath(21-38)validator-cli/src/utils/errors.ts (1)
InvalidBotPathError(63-63)
validator-cli/src/ArbToEth/claimer.ts (4)
validator-cli/src/ArbToEth/transactionHandler.ts (1)
ArbToEthTransactionHandler(68-409)validator-cli/src/utils/claim.ts (1)
ClaimHonestState(193-193)validator-cli/src/utils/graphQueries.ts (1)
getLastClaimedEpoch(148-148)validator-cli/src/utils/ethers.ts (1)
getTransactionHandler(123-123)
validator-cli/src/ArbToEth/transactionHandlerDevnet.ts (2)
validator-cli/src/ArbToEth/transactionHandler.ts (4)
Transactions(39-49)Transaction(34-37)ArbToEthTransactionHandler(68-409)TransactionHandlerConstructor(23-32)validator-cli/src/consts/bridgeRoutes.ts (1)
getBridgeConfig(92-92)
validator-cli/src/watcher.ts (6)
validator-cli/src/utils/errors.ts (1)
MissingEnvError(66-66)validator-cli/src/utils/botConfig.ts (2)
getBotPath(21-38)getNetworkConfig(49-62)validator-cli/src/consts/bridgeRoutes.ts (1)
getBridgeConfig(92-92)validator-cli/src/utils/epochHandler.ts (2)
setEpochRange(77-77)getBlockFromEpoch(77-77)validator-cli/src/utils/claim.ts (1)
getClaim(193-193)validator-cli/src/ArbToEth/claimer.ts (1)
CheckAndClaimParams(117-117)
validator-cli/src/ArbToEth/claimer.test.ts (2)
validator-cli/src/ArbToEth/claimer.ts (2)
CheckAndClaimParams(117-117)checkAndClaim(117-117)validator-cli/src/utils/claim.ts (1)
ClaimHonestState(193-193)
validator-cli/src/utils/graphQueries.ts (1)
validator-cli/src/utils/errors.ts (1)
ClaimNotFoundError(60-60)
validator-cli/src/utils/claim.test.ts (2)
validator-cli/src/utils/graphQueries.ts (3)
getClaimForEpoch(147-147)getVerificationForClaim(149-149)getChallengerForClaim(150-150)validator-cli/src/utils/claim.ts (1)
getClaim(193-193)
validator-cli/src/utils/claim.ts (2)
validator-cli/src/utils/graphQueries.ts (4)
getClaimForEpoch(147-147)getVerificationForClaim(149-149)getChallengerForClaim(150-150)getSnapshotSentForEpoch(151-151)validator-cli/src/utils/errors.ts (1)
ClaimNotFoundError(60-60)
validator-cli/src/ArbToEth/validator.ts (2)
validator-cli/src/ArbToEth/transactionHandler.ts (1)
ArbToEthTransactionHandler(68-409)validator-cli/src/utils/emitter.ts (1)
defaultEmitter(4-4)
validator-cli/src/ArbToEth/transactionHandler.test.ts (4)
validator-cli/src/ArbToEth/transactionHandler.ts (3)
TransactionHandlerConstructor(23-32)MAX_PENDING_CONFIRMATIONS(65-65)Transaction(34-37)validator-cli/src/utils/emitter.ts (2)
MockEmitter(6-14)defaultEmitter(4-4)validator-cli/src/consts/bridgeRoutes.ts (1)
getBridgeConfig(92-92)validator-cli/src/utils/errors.ts (1)
ClaimNotSetError(61-61)
validator-cli/src/consts/bridgeRoutes.ts (1)
veascan-web/src/consts/bridges.ts (1)
bridges(39-58)
validator-cli/src/ArbToEth/transactionHandler.ts (4)
validator-cli/src/utils/emitter.ts (1)
defaultEmitter(4-4)validator-cli/src/consts/bridgeRoutes.ts (1)
getBridgeConfig(92-92)validator-cli/src/utils/errors.ts (1)
ClaimNotSetError(61-61)validator-cli/src/utils/arbMsgExecutor.ts (1)
messageExecutor(82-82)
validator-cli/src/utils/ethers.ts (4)
validator-cli/src/utils/errors.ts (2)
NotDefinedError(62-62)InvalidNetworkError(65-65)validator-cli/src/ArbToEth/claimer.ts (1)
checkAndClaim(117-117)validator-cli/src/ArbToEth/transactionHandlerDevnet.ts (1)
ArbToEthDevnetTransactionHandler(19-71)validator-cli/src/ArbToEth/transactionHandler.ts (1)
ArbToEthTransactionHandler(68-409)
🪛 Biome (1.9.4)
validator-cli/src/utils/claim.ts
[error] 82-82: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
validator-cli/src/utils/ethers.ts
[error] 39-45: This case is falling through to the next case.
Add a break or return statement to the end of this case to prevent fallthrough.
(lint/suspicious/noFallthroughSwitchClause)
[error] 102-108: This case is falling through to the next case.
Add a break or return statement to the end of this case to prevent fallthrough.
(lint/suspicious/noFallthroughSwitchClause)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: test
- GitHub Check: Analyze (javascript)
- GitHub Check: dependency-review
- GitHub Check: Scorecard analysis
🔇 Additional comments (82)
validator-cli/tsconfig.json (2)
2-4: Validate "include" configuration.The "include" array now clearly specifies the "src" directory and is properly closed, ensuring that all intended source files are included during compilation.
5-13: Enhance TypeScript compiler settings.The added "compilerOptions" block sets essential settings such as "baseUrl", "module", "moduleResolution", and output directory ("outDir"). These improvements promote better module resolution, compatibility between CommonJS and ES Modules, and facilitate the build process.
validator-cli/src/utils/epochHandler.test.ts (5)
1-1: Updated the imports to include the newEpochRangeParamstypeThe import statement now includes
EpochRangeParamswhich aligns with the refactoring of thesetEpochRangefunction to use an object parameter pattern instead of individual arguments.
14-14: Adjust the startEpoch calculationThe subtraction value in the calculation of
startEpochhas been changed from2to1, which affects the range of epochs being tested. Ensure this change correctly aligns with the updated implementation of thesetEpochRangefunction.
20-33: Good refactoring to use object parametersThe refactoring to use an object parameter pattern is a good practice for functions with multiple parameters. This improves code readability and makes the parameter passing more explicit.
34-34: Updated assertion to match the modified function behaviorThe expectation has been updated to check for
currentEpochinstead ofcurrentEpoch - 1, reflecting changes in the underlying implementation ofsetEpochRange.
42-43: Simplified function call with direct valueThe call to
getLatestChallengeableEpochhas been simplified by passing a hardcoded value directly instead of using a variable, which is cleaner. The removedfetchBridgeConfigmock suggests the function no longer requires this parameter.validator-cli/src/utils/errors.ts (7)
1-2: Added new imports for error handlingThe added imports for
BotPathsandNetworksupport the new error classes that need to reference valid paths and networks.
19-25: Improved error class for reusabilityRenamed
TransactionHandlerNotDefinedErrorto the more genericNotDefinedErrorand updated it to accept a parameter, making it more reusable across different scenarios. This is a good practice for improving code maintainability.
27-33: Added error class for invalid bot pathsThe
InvalidBotPathErrorclass provides clear error messages that include the valid options fromBotPaths, helping developers quickly identify and fix bot path issues.
35-41: Added error class for devnet owner configurationThis error supports the PR's devnet functionality which requires the
PRIVATE_KEYof the devnet owner to be set.
43-49: Added error class for network validationThis error aligns with the PR's multi-network support feature, allowing clear validation of supported networks. The error message helpfully lists all valid networks from the
Networkenum.
51-57: Added error class for missing environment variablesThis is useful for validating required environment variables, which is especially important given the PR's mention of configurable functionality through the
.envfile.
59-67: Updated exports to include all error classesThe export statement has been properly updated to include all new error classes, making them available for use throughout the application.
validator-cli/ecosystem.config.js (1)
4-14: Updated PM2 configuration for the validatorThe configuration has been significantly improved to support the validator-cli with the following enhancements:
- Created a single entry point named "validator-cli" using the watcher script
- Added proper TypeScript configuration with interpreter arguments
- Enabled
watchandautorestartto improve reliability and automatic recovery- Added
TS_NODE_PROJECTenvironment variable to ensure correct TypeScript configurationThese changes align well with the PR objective of adding PM2 support for better process management.
validator-cli/src/utils/botConfig.test.ts (1)
1-25: Well-structured unit tests with good coverage.The new test file provides excellent coverage for the
getBotPathfunction, thoroughly testing all its code paths including:
- Default path behavior
- Claimer path detection
- Challenger path detection
- Error handling for invalid paths
The test implementations are clear, concise, and follow Jest best practices.
validator-cli/src/ArbToEth/validator.ts (4)
9-10: LGTM: Network and ClaimStruct imports added appropriately.These imports support the multi-network functionality mentioned in the PR objectives.
15-16: Good enhancement: Added claim parameter to interface.Adding the
claimparameter to theChallengeAndResolveClaimParamsinterface properly reflects the changes in function signature.
30-32: Improved parameter handling with better error checking.The function now directly accepts the claim as a parameter and includes proper null checking, which improves error handling. This approach is cleaner than fetching the claim within the function.
Also applies to: 43-46
64-73: Network configuration hardcoded for transaction handler.The network is explicitly set to
Network.TESTNETwith a comment explaining this is appropriate since TESTNET and MAINNET use the same contracts. This aligns with the multi-network support objective of the PR.validator-cli/src/ArbToEth/validator.test.ts (3)
43-43: Simplified test setup with direct claim property assignment.The test now directly assigns the mock claim to the dependency object, which is a cleaner approach that matches the updated function signature in
validator.ts.
56-58: Added test cleanup to prevent state leakage.Adding
afterEachwithjest.clearAllMocks()ensures that mock state doesn't leak between tests, which is a best practice for test isolation.
61-61: Updated test to use direct claim assignment.This change correctly reflects the new implementation where claims are passed directly rather than fetched within the function.
validator-cli/src/ArbToEth/claimer.test.ts (5)
1-5: Appropriate imports for the claimer test suite.The imports correctly include the necessary dependencies for testing the claimer functionality, including ethers, the function under test, and required types.
6-133: Well-structured test setup with comprehensive mocks.The test setup is thorough and well-organized:
- Mock objects for all dependencies
- Transaction handler with appropriate mock implementations
- Clear structure with beforeEach and afterEach hooks
- Type safety with CheckAndClaimParams
This provides a solid foundation for testing the claimer functionality across different scenarios.
134-163: Comprehensive error case testing.These tests validate that the
checkAndClaimfunction correctly handles various error scenarios:
- No claim for a passed epoch
- No snapshot saved
- No new messages in the inbox
Thorough error case testing helps ensure robustness of the implementation.
164-181: Devnet-specific functionality well tested.The test properly validates the devnet-specific behavior mentioned in the PR objectives, confirming that the
devnetAdvanceStatefunction is called for devnet environments to handle both claim and verification in a single transaction.
182-232: Comprehensive testnet scenario coverage.The testnet tests cover all the key scenarios:
- Making a new claim when none exists
- Making a claim when the last one was challenged
- Withdrawing claim deposit when claimer is honest
- Starting verification when not yet started
- Verifying snapshot when verification is started
This thorough coverage ensures the testnet-specific functionality works as expected.
validator-cli/src/utils/botConfig.ts (2)
5-9: EnumBotPathsis clear and maintainable.
No issues found here. This numeric enum effectively organizes the possible bot paths.
40-43: InterfaceNetworkConfiglooks good.
No concerns found; it's straightforward and well-defined.validator-cli/src/ArbToEth/transactionHandlerDevnet.ts (4)
1-12: Imports appear in good order.
All imported entities are used and relevant to devnet transaction handling.
17-17: Ensure the correct chain ID.
11155111corresponds to the Sepolia test network. Please confirm if this constant is intentional for devnet usage, or if a separate chain ID is required.Would you like to confirm references to devnet in your environment variables or code?
19-31: Subclass design is clear.
This extension ofArbToEthTransactionHandleris well-structured, neatly adding devnet-specific transaction properties while preserving existing logic.
32-49: Caution with forced devnet casting.
veaOutboxis cast toVeaOutboxArbToEthDevnetwithout runtime checks. Ensure at runtime that the provided contract truly matches the devnet interface to avoid undefined behavior.validator-cli/src/ArbToEth/claimer.ts (4)
1-3: Imports appear consistent.
All imported modules (Events, ethers, providers) are pertinent to the claiming logic.
4-28: InterfaceCheckAndClaimParamsis comprehensive.
It clearly outlines all parameters for claim-related operations. No immediate suggestions.
82-95: Conditional claim creation logic is consistent.
The checks for new messages or challenged claims appear correct. No issues found in this block.
96-115: Check transitions forclaim.honestexhaustively.
There's no explicit handling forClaimHonestState.CHALLENGER. Ensure no use case is omitted if the claim’s honesty state is set toCHALLENGER.validator-cli/src/watcher.ts (5)
31-32: Ensure consistency in environment variable management.
It's good that you're checking forPRIVATE_KEYearly. Consider extending this pattern to other essential environment variables so you fail fast if something else is missing.
78-82: Handle null claims carefully.
getClaimmay returnnull; you handle it in line 83. That's correct. Just watch for a second pass logic if the same epoch remains intoWatch.
114-119: Remove epoch fromtoWatchonly after final settlement.
Line 116-118 deletes the epoch if no updates. This can be correct. However, if a late transaction finalizes an older epoch, you risk missing it. Thoroughly confirm your business logic.
55-65:✅ Verification successful
Ensure correct epoch detection logic.
Your logic sets the epoch to the single latest one on devnet or a range on testnet. Verify that the boundary conditions (e.g., crossing from devnet to testnet in the same run) never happen or are handled.Follow-up: Confirm that
setEpochRangewill produce correct results for short epoch periods and that no off-by-one errors occur for boundary epochs.
🏁 Script executed:
#!/bin/bash # Searching for setEpochRange usage to confirm no off-by-one issues rg -A 2 'setEpochRange'Length of output: 1523
Confirm Epoch Detection Logic and Boundary Conditions
- The code in
validator-cli/src/watcher.tscorrectly distinguishes DEVNET (using a single epoch based on a floored calculation) from other networks (using a range generated bysetEpochRange).- A scan of the codebase confirms that
setEpochRangeis thoroughly exercised in tests (seevalidator-cli/src/utils/epochHandler.test.ts), which should help ensure correct behavior even with short epoch periods and around boundary values.- Please double-check that any potential scenario where a network might change (e.g., DEVNET to testnet mid-run) is either impossible or explicitly handled.
70-76: 💡 Verification agent🧩 Analysis chain
Validate
epochBlockfor correctness.
You computeepochBlockfromgetBlockFromEpoch. If for any reason that function returns an invalid or negative block number, be sure to handle it.Ask the user to verify if the underlying block retrieval is robust:
🏁 Script executed:
#!/bin/bash # Checking references to getBlockFromEpoch for error handling rg -A 5 'getBlockFromEpoch'Length of output: 1972
Validate and Safeguard
epochBlockCalculationAfter reviewing the implementation, it appears that the function
getBlockFromEpoch(defined invalidator-cli/src/utils/epochHandler.ts) does not currently validate its output. In particular, ifgetBlockFromEpochhappens to return an invalid or negative block number, the subsequent arithmetic inwatcher.tsmay lead to unintended behavior. For robust error handling, please add a safeguard to verify thatepochBlockis a valid (non-negative) block number before it is used.
- In
validator-cli/src/watcher.ts(lines 70–76), perform a check to ensure thatepochBlock >= 0(or meets your defined validity criteria).- If an invalid value is detected, handle the condition appropriately (e.g., log an error, throw an exception, or apply a fallback strategy).
- Alternatively, consider adding validation inside
getBlockFromEpochitself so that it returns either a guaranteed valid block number or a clear error state.⛔ Skipped due to learnings
Learnt from: mani99brar PR: kleros/vea#396 File: validator-cli/src/utils/epochHandler.ts:67-74 Timestamp: 2025-04-07T05:14:00.213Z Learning: In `validator-cli/src/utils/epochHandler.ts`, the `getBlockFromEpoch` function deliberately omits error handling for provider calls like `getBlock("latest")` and `getBlock(...)`. The developer prefers to let errors propagate up the call stack rather than handling them within this function.validator-cli/src/consts/bridgeRoutes.ts (5)
11-13: Double-checkveaInboxArbToGnosisTestnetimport.
Line 11 referencesVeaOutboxArbToEthTestnet.jsonbut is namedveaInboxArbToGnosisTestnet. Confirm it’s not a copy-paste error.
21-22: Use typed optional fields consistently.
You introducedrouterRPC?: string;and placedrouteConfigas[key in Network]: RouteConfigs. Confirm that each route expects or tolerates a missingrouterRPC.
32-35: Enum usage is clear.
UsingNetwork.DEVNET/Network.TESTNETis a neat approach. Just ensure no additional networks slip in unhandled.
72-72: Maintain consistency in bridging data.
You haverouteConfig: arbToEthConfigsfor chainId11155111. Ensure this corresponds to the correct bridging route (Arb → Eth).
81-82: Router RPC usage.
You definerouterRPCfor chainId=10200 withprocess.env.RPC_ETH. Confirm it’s correct for Gnosis bridging.validator-cli/src/utils/claim.test.ts (5)
58-58: Validate synergy withhashClaim.
You mock theclaimHashescall. Check that the hashed result matches the claim’s structure to avoid mismatch in some test scenarios.
71-72: Reassigning tomockClaimParams.veaOutboxright before calling.
This ensures the function sees the fresh mock. Good practice. Consider more descriptive variable names so it’s clearer which version ofveaOutboxyou’re injecting.
137-161: Fallback on subgraphs test scenario is robust.
You simulate “Logs not available” and confirm the subgraph approach. Great coverage. Just ensure subgraph queries are tested for real volumes or partial data.
163-166: Testing for no claim found.
You properly checkethers.ZeroHashfromclaimHashes. This negative test scenario is valuable.
172-180: Assertion on both on-chain logs and subgraph data.
You handle “no logs found” by also returningnullfrom subgraph mocks. This setup is consistent with your fallback approach. Good job.validator-cli/src/utils/botEvents.ts (1)
4-4: LGTM! Well-structured event organizationThe additions to BotEvents align well with the multi-network support and claimer functionality goals of this PR, particularly:
- Bridger state events like
WATCHINGandNO_CLAIM_REQUIRED- More granular claim state events that describe the process flow
- New
ADV_DEVNETsection specifically for devnet state- Transaction monitoring capabilities with
TXN_EXPIREDThe organization by event category (Bridger, Epoch, Claim, Devnet, Transaction) improves code readability and maintainability.
Also applies to: 9-9, 14-14, 17-22, 24-24, 27-30, 33-34, 41-41
validator-cli/.env.dist (1)
15-19: LGTM! Improved configuration for multi-network supportThe addition of chain IDs and subgraph endpoints directly fulfills the PR objectives by:
- Supporting multiple networks through
VEAOUTBOX_CHAINSconfiguration- Implementing subgraphs as fallbacks for contract event logs
This approach is more flexible than the previous network-specific configuration.
validator-cli/src/utils/claim.ts (4)
6-11: LGTM! Well-structured imports for fallback mechanismsThe addition of graph query functions provides the necessary fallback mechanisms for retrieving data when direct contract event logs fail, which aligns with the PR objective of using subgraphs as fallbacks.
13-17: LGTM! Properly exported enumThe
ClaimHonestStateenum is correctly defined and exported, making it accessible for other parts of the codebase.
19-28: LGTM! Well-structured interfaceThe
ClaimParamsinterface provides a clean way to organize parameters for thegetClaimfunction, which improves code maintainability and readability.
146-162: LGTM! Consistent error handling patternThe error handling in
getClaimResolveStatefollows the same pattern as ingetClaim, using graph queries as fallbacks when direct contract event logs fail. This consistency is good for maintainability.validator-cli/src/utils/epochHandler.ts (2)
4-10: LGTM! Well-structured interfaceThe
EpochRangeParamsinterface provides a clear organization of parameters for thesetEpochRangefunction, which improves code maintainability and readability.
44-48: LGTM! Improved array creationUsing
Array.fromis a more elegant approach than the previous method of creating an array withnew Array().fill().map().validator-cli/src/utils/graphQueries.ts (2)
4-17: Well-defined interface.Your
ClaimDatainterface thoroughly documents all claim-related fields (e.g.,id,bridger,stateroot, etc.). This promotes clarity for other parts of the codebase consuming or providing such data.
48-74: Return type mismatch risk.The function’s signature returns
Promise<ClaimData>, but ifclaimsis empty,result['claims'][0]will beundefined. Consider throwing aClaimNotFoundErrorif no claim is retrieved, or adjust the return type toPromise<ClaimData | undefined>to accurately reflect the possibility of no results.validator-cli/src/ArbToEth/transactionHandler.test.ts (3)
1-8: Imports and new references look good.All the updated imports for
transactionHandlerand related interfaces/classes are correct and consistent with the usage patterns in this test file.
148-185: Well-structured claim tests.The new tests for
makeClaim()confirm correct transaction creation and status checking. This coverage ensures repeated or pending transactions don’t trigger additional claims, which is crucial for robust bridging logic.
294-365: Comprehensive challenge claim tests.These tests effectively verify both happy and unhappy paths (claim set vs. not set, pending transaction vs. new transaction). Good work ensuring challenge logic is exercised thoroughly.
validator-cli/src/ArbToEth/transactionHandler.ts (1)
117-147: Smart use ofTXN_EXPIRED.The newly introduced expiration logic for pending transactions prevents indefinite waiting when transactions are stuck or dropped, providing a practical “escape hatch” in the bridging workflow.
validator-cli/src/utils/logger.ts (8)
3-4: All good on new imports.
The imports forBotPathsandNetworkare straightforward and appear to be used properly further in the file.
33-34: Straightforward chain-watching logs.
No issues found. The new event listener appears correct.
45-46: Clear logging for no-claim scenario.
No concerns. This event clarifies that no claim is necessary for the given epoch.
54-55: Consistent epoch logging.
This message cleanly indicates a passed epoch for claiming. No issues.
76-78: Transaction expiration logging looks good.
The event name and message align well to indicate an expired transaction.
81-84: Accurate log for claim initiation.
This clarifies that a claim process is starting for the specified epoch.
85-88: Verification start logging is comprehensive.
Indicates precisely when verification is initiated.
103-105: Log alignment is consistent.
This event mapping for a challenged claim matches other epoch-based logs, no issues noticed.validator-cli/src/utils/ethers.ts (7)
5-5: Import addition looks correct.
This factory import is used to support Gnosis Devnet functionality.
14-18: New imports appear consistent.
They align with updated references tocheckAndClaim,ArbToEthDevnetTransactionHandler, and new error classes.
20-21: Renaming parameter torpcUrlis coherent.
This improves clarity and consistency with other functions.
37-37: Function signature now includesnetwork.
This refactor to acceptnetwork: Networkaligns well with multi-network handling.
77-83: Claim validator looks fine.
Addingnetworkto the signature is consistent. The default case properly throwsNotDefinedErrorfor unsupported chain IDs.
84-98: Claimer logic is robust.
The nested switch handles bothchainIdandnetworkwith appropriate errors in the default cases.
122-122: Export addition forgetClaimer.
No issues. This preserves consistency with other exported functions.
| "pm2": "^6.0.5", | ||
| "typescript": "^4.9.5" |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Updated PM2 and removed Web3 dependencies
- PM2 has been upgraded from
^5.2.2to^6.0.5, which aligns with the PR objective of adding PM2 support. - The
web3andweb3-batched-senddependencies have been removed, suggesting a shift in how the application interacts with the blockchain.
🏁 Script executed:
#!/bin/bash
# Check for any remaining web3 imports in the codebase that might break after removing these dependencies
rg -i "require\(['\"]web3['\"]" --type ts
rg -i "import.*from ['\"]web3['\"]" --type ts
rg -i "web3-batched-send" --type tsLength of output: 277
Action Required: Remove Remaining Web3 References
The changes in validator-cli/package.json correctly update PM2 to ^6.0.5 and remove the Web3-related dependencies. However, verification revealed that the file relayer-cli/src/utils/relay.ts still imports:
import Web3 from "web3";import initializeBatchedSend from "web3-batched-send";
Please update or remove these imports to ensure consistency with the dependency removals and to prevent potential runtime issues.
| public async devnetAdvanceState(stateRoot: string): Promise<void> { | ||
| this.emitter.emit(BotEvents.ADV_DEVNET, this.epoch); | ||
| const currentTime = Date.now(); | ||
| const transactionStatus = await this.checkTransactionStatus( | ||
| this.transactions.devnetAdvanceStateTxn, | ||
| ContractType.OUTBOX, | ||
| currentTime | ||
| ); | ||
| if (transactionStatus != TransactionStatus.NOT_MADE && transactionStatus != TransactionStatus.EXPIRED) { | ||
| return; | ||
| } | ||
| const deposit = getBridgeConfig(CHAIN_ID).deposit; | ||
| const startVerifTrx = await this.veaOutboxDevnet.devnetAdvanceState(this.epoch, stateRoot, { | ||
| value: deposit, | ||
| }); | ||
| this.emitter.emit(BotEvents.TXN_MADE, startVerifTrx.hash, this.epoch, "Advance Devnet State"); | ||
| this.transactions.devnetAdvanceStateTxn = { | ||
| hash: startVerifTrx.hash, | ||
| broadcastedTimestamp: currentTime, | ||
| }; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add robust error handling around devnetAdvanceState.
If the transaction reverts or fails, the code doesn't catch exceptions. Consider a try/catch to log errors or revert state if the call fails.
| if (network == Network.DEVNET) { | ||
| const devnetTransactionHandler = transactionHandler as ArbToEthDevnetTransactionHandler; | ||
| if (claim == null) { | ||
| [savedSnapshot, claimData] = await Promise.all([ | ||
| veaInbox.snapshots(epoch), | ||
| fetchLatestClaimedEpoch(veaOutbox.target), | ||
| ]); | ||
|
|
||
| newMessagesToBridge = savedSnapshot != outboxStateRoot && savedSnapshot != ethers.ZeroHash; | ||
| if (newMessagesToBridge && savedSnapshot != ethers.ZeroHash) { | ||
| await devnetTransactionHandler.devnetAdvanceState(savedSnapshot); | ||
| return devnetTransactionHandler; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Double-check concurrency for DEVNET state advancement.
After devnetAdvanceState, there's no explicit confirmation of successful finality. Adding a verification step or error-handling logic could ensure the devnet state advanced properly.
validator-cli/src/watcher.ts
Outdated
| const veaInbox = getVeaInbox(routeConfig[network].veaInbox.address, privKey, inboxRPC, chainId, network); | ||
| const veaOutbox = getVeaOutbox(routeConfig[network].veaOutbox.address, privKey, outboxRPC, chainId, network); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Validate contract addresses.
Ensure routeConfig[network].veaInbox.address and .veaOutbox.address are not undefined or invalid. If environment misconfiguration occurs, it can break these calls.
validator-cli/src/watcher.ts
Outdated
| for (const networkConfig of networkConfigs) { | ||
| const { chainId, networks } = networkConfig; | ||
| const { routeConfig, inboxRPC, outboxRPC } = getBridgeConfig(chainId); |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Catch potential errors from getBridgeConfig.
If bridgeRoutes.ts throws when a config is missing, ensure you handle it gracefully, logging relevant info.
🏁 Script executed:
#!/bin/bash
# Checking references to getBridgeConfig calls to confirm they are wrapped in try/catch or properly handled
rg -A 3 'getBridgeConfig'Length of output: 9949
Error Handling for Missing Bridge Configurations
The current implementation in validator-cli/src/watcher.ts (lines 40–42) calls getBridgeConfig(chainId) directly, which will throw an error when a configuration is missing. To avoid runtime crashes, please wrap the call in a try/catch block and log sufficient details (e.g., the chainId and error message) to help diagnose the issue. For example:
- const { routeConfig, inboxRPC, outboxRPC } = getBridgeConfig(chainId);
+ let routeConfig, inboxRPC, outboxRPC;
+ try {
+ ({ routeConfig, inboxRPC, outboxRPC } = getBridgeConfig(chainId));
+ } catch (error) {
+ console.error(`Failed to load bridge config for chainId ${chainId}:`, error);
+ continue; // or handle the failure as required
+ }This change will ensure that if a config is missing, the error is managed gracefully without crashing the application.
📝 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.
| for (const networkConfig of networkConfigs) { | |
| const { chainId, networks } = networkConfig; | |
| const { routeConfig, inboxRPC, outboxRPC } = getBridgeConfig(chainId); | |
| for (const networkConfig of networkConfigs) { | |
| const { chainId, networks } = networkConfig; | |
| - const { routeConfig, inboxRPC, outboxRPC } = getBridgeConfig(chainId); | |
| + let routeConfig, inboxRPC, outboxRPC; | |
| + try { | |
| + ({ routeConfig, inboxRPC, outboxRPC } = getBridgeConfig(chainId)); | |
| + } catch (error) { | |
| + console.error(`Failed to load bridge config for chainId ${chainId}:`, error); | |
| + continue; // or handle the failure as required | |
| + } | |
| // ... rest of the loop code | |
| } |
| const getBlockFromEpoch = async (epoch: number, epochPeriod: number, provider: JsonRpcProvider): Promise<number> => { | ||
| const epochTimestamp = epoch * epochPeriod; | ||
| const latestBlock = await provider.getBlock("latest"); | ||
| const baseBlock = await provider.getBlock(latestBlock.number - 1000); | ||
| const secPerBlock = (latestBlock.timestamp - baseBlock.timestamp) / (latestBlock.number - baseBlock.number); | ||
| const blockFallBack = Math.floor((latestBlock.timestamp - epochTimestamp) / secPerBlock); | ||
| return latestBlock.number - blockFallBack; | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add validation for calculated block number
The getBlockFromEpoch function calculates a block number based on the average block time, but doesn't validate that the result is positive and within range.
Consider adding validation to ensure the returned block number is valid:
const blockFallBack = Math.floor((latestBlock.timestamp - epochTimestamp) / secPerBlock);
- return latestBlock.number - blockFallBack;
+ const calculatedBlock = latestBlock.number - blockFallBack;
+ // Ensure block number is valid (not negative and not greater than latest block)
+ return Math.max(0, Math.min(calculatedBlock, latestBlock.number));Also, consider adding a comment about the assumption of constant block time:
// Note: This calculation assumes relatively consistent block times over the range,
// which may not hold during periods of network congestion.| const getSnapshotSentForEpoch = async (epoch: number, veaInbox: any): Promise<{ txHash: string }> => { | ||
| try { | ||
| const subgraph = process.env.VEAINBOX_SUBGRAPH; | ||
| const veaInboxAddress = veaInbox.toLowerCase(); | ||
|
|
||
| const result: SenSnapshotResponse = await request( | ||
| `${subgraph}`, | ||
| `{ | ||
| snapshots(where: {epoch: ${epoch}, inbox_: { id: "${veaInboxAddress}" }}) { | ||
| fallback{ | ||
| txHash | ||
| } | ||
| } | ||
| }` | ||
| ); | ||
| return result.snapshots[0].fallback[0]; | ||
| } catch (e) { | ||
| console.log(e); | ||
| return undefined; | ||
| } | ||
| }; |
There was a problem hiding this comment.
Potential undefined return from the catch block.
The function’s signature enforces a return type of { txHash: string }, yet the catch block returns undefined. You might want to:
- Throw a dedicated error if the call fails, or
- Change the return type to
Promise<{ txHash: string } | undefined>
Additionally, if result.snapshots is empty, result.snapshots[0] could be undefined, causing a runtime error. Consider validating the array length first.
|
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
validator-cli/src/utils/claim.test.ts (1)
17-19: Use a typed interface or type alias for mockClaimParams.
This aligns with the prior comment in previous reviews, improving readability and maintainability.
🧹 Nitpick comments (19)
validator-cli/src/utils/botConfig.ts (6)
1-3: Consider placing dotenv config at the entry point.
In many Node.js projects, callingrequire("dotenv").config()at the main entry point (rather than within a utility file) helps centralize environment loading.
16-21: Doc comments are clear.
Minor note: You may include parameter details in JSDoc for better tooling support.
21-38: Path parsing logic is straightforward.
Overall logic is correct. One minor improvement could involve validating the extractedpathis non-empty.
40-43: Naming might be more expressive.
NetworkConfigis acceptable, but consider a more descriptive name (e.g.,ChainNetworksConfig) if multiple networks map to a single chain ID.
45-62: Configuration struct creation is effective.
Returning an array ofNetworkConfigis fine, though you might align it with a dictionary keyed by chain ID if you foresee direct chain lookups in the future.
64-72: Validate networks logic is correct.
Theas unknown as Network[]cast works, but consider a more direct approach (e.g., mapping and narrowing the string to valid enum members) to avoid double casting.validator-cli/src/consts/bridgeRoutes.ts (3)
3-13: Importing multiple devnet/testnet artifacts.
These JSON imports are untyped (inferred asany). If desired, consider generating contract type definitions to benefit from compile-time checks.
25-30: Definition of RouteConfigs is clear.
If you want stricter checks, you could define a specific type for these contract artifacts instead ofany.
86-90: Improves error handling in getBridgeConfig.
Consider including the chain ID in the error message to help with debugging, e.g., `Bridge not found for chain ${chainId}`.- if (!bridge) throw new Error(`Bridge not found for chain`); + if (!bridge) throw new Error(`Bridge not found for chain ${chainId}`);validator-cli/src/ArbToEth/claimer.ts (3)
30-81: Central logic in checkAndClaim is well-structured.
The flow for devnet vs. standard networks is clear. Consider logging any caught exceptions fromveaOutbox.stateRoot()to aid debugging.
104-122: makeClaim logic for testnet / mainnet.
Properly checksnewMessagesToBridgeorlastClaimChallenged. Potential extension: log ifsavedSnapshotmatches the outbox state root for easier visibility.
124-144: verifyClaim covers multiple claim states.
The transitions forCLAIMER,NONE, or presence ofchallengerare well-defined. Consider verifying block finality for advanced reorg safety.validator-cli/src/utils/claim.ts (4)
13-17: Consider adding inline documentation for the enum values.
Adding brief comments for each enum value (e.g.,NONE,CLAIMER,CHALLENGER) can improve readability for future maintainers.enum ClaimHonestState { NONE = 0, + // No honest party identified CLAIMER = 1, + // Claimer is honest CHALLENGER = 2, + // Challenger is honest }
19-28: Replaceanywith a more specific type forveaOutbox.
Usinganymay mask potential type errors. Consider defining or importing the correct type from the contract’s type definitions.interface ClaimParams { - veaOutbox: any; + veaOutbox: VeaOutbox; // or another suitable contract interface veaOutboxProvider: JsonRpcProvider; epoch: number;
72-89: Catch block is too broad.
Catching all errors without logging critical details (e.g., stack trace) may complicate debugging. Consider logging or narrowing error handling.} catch (error) { + console.error("Error in getClaim (on-chain filter):", error); const claimFromGraph = await fetchClaimForEpoch(epoch, await veaOutbox.getAddress()); ... }
146-162: Log caught errors when querying for snapshot logs.
Similar to the pattern ingetClaim, consider logging or differentiating relevant errors to ease diagnosis.} catch (error) { + console.error("Error fetching SnapshotSent logs from chain:", error); const sentSnapshotFromGraph = await getSnapshotSentForEpoch(epoch, await veaInbox.getAddress()); ... }validator-cli/src/watcher.ts (3)
31-38: ValidatePRIVATE_KEYin a single place.
ThrowingMissingEnvErrorearly is good, but consider logging the environment or referencing an.envfile for user clarity.const privKey = process.env.PRIVATE_KEY; if (!privKey) { + console.warn("Ensure PRIVATE_KEY is set in the .env file or environment variables."); throw new MissingEnvError("PRIVATE_KEY"); }
47-98: Parallelizing processNetwork calls for each route.
InvokingprocessNetworkin a loop is fine if concurrency is acceptable. Alternatively, consider concurrency management or rate limiting if needed.for (const networkConfig of networkConfigs) { - await processNetwork(path, networkConfig, transactionHandlers, toWatch, emitter); + // Consider concurrency or wrapping in Promise.all + // await Promise.all(networkConfigs.map((networkConfig) => processNetwork(...))); }
174-175: Utility for controlled delays.
waitfunction is a typical async pattern. If more precise scheduling or cancellation is needed, consider advanced queue management.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro (Legacy)
📒 Files selected for processing (9)
validator-cli/src/ArbToEth/claimer.ts(1 hunks)validator-cli/src/ArbToEth/transactionHandler.test.ts(8 hunks)validator-cli/src/consts/bridgeRoutes.ts(1 hunks)validator-cli/src/utils/botConfig.ts(1 hunks)validator-cli/src/utils/claim.test.ts(7 hunks)validator-cli/src/utils/claim.ts(5 hunks)validator-cli/src/utils/epochHandler.test.ts(2 hunks)validator-cli/src/utils/ethers.ts(3 hunks)validator-cli/src/watcher.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- validator-cli/src/utils/epochHandler.test.ts
- validator-cli/src/utils/ethers.ts
🧰 Additional context used
🧬 Code Definitions (5)
validator-cli/src/ArbToEth/claimer.ts (5)
validator-cli/src/ArbToEth/transactionHandler.ts (2)
ArbToEthTransactionHandler(68-409)makeClaim(154-179)validator-cli/src/utils/claim.ts (2)
getClaim(193-193)ClaimHonestState(193-193)validator-cli/src/utils/graphQueries.ts (1)
getLastClaimedEpoch(148-148)validator-cli/src/utils/ethers.ts (1)
getTransactionHandler(133-133)validator-cli/src/ArbToEth/transactionHandlerDevnet.ts (1)
ArbToEthDevnetTransactionHandler(19-71)
validator-cli/src/utils/claim.ts (2)
validator-cli/src/utils/graphQueries.ts (4)
getClaimForEpoch(147-147)getVerificationForClaim(149-149)getChallengerForClaim(150-150)getSnapshotSentForEpoch(151-151)validator-cli/src/utils/errors.ts (1)
ClaimNotFoundError(60-60)
validator-cli/src/utils/claim.test.ts (2)
validator-cli/src/utils/graphQueries.ts (2)
getVerificationForClaim(149-149)getChallengerForClaim(150-150)validator-cli/src/utils/claim.ts (1)
getClaim(193-193)
validator-cli/src/consts/bridgeRoutes.ts (1)
veascan-web/src/consts/bridges.ts (1)
bridges(39-58)
validator-cli/src/ArbToEth/transactionHandler.test.ts (4)
validator-cli/src/ArbToEth/transactionHandler.ts (4)
TransactionHandlerConstructor(23-32)ArbToEthTransactionHandler(68-409)MAX_PENDING_CONFIRMATIONS(65-65)Transaction(34-37)validator-cli/src/utils/emitter.ts (2)
MockEmitter(6-14)defaultEmitter(4-4)validator-cli/src/consts/bridgeRoutes.ts (1)
getBridgeConfig(92-92)validator-cli/src/utils/errors.ts (1)
ClaimNotSetError(61-61)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Analyze (javascript)
- GitHub Check: Scorecard analysis
🔇 Additional comments (41)
validator-cli/src/ArbToEth/transactionHandler.test.ts (15)
1-12: Updated imports align with new architecture.The imports have been properly updated to support multi-network functionality, with the addition of
NetworkandgetBridgeConfigfrom bridgeRoutes. The newTransactionHandlerConstructorinterface has been correctly imported to support the refactored constructor parameters.
15-22: LGTM - Test setup with proper chainId value.Setting the chainId to 11155111 (Sepolia testnet) aligns with the multi-network support objective mentioned in the PR, and the test variables are properly initialized.
22-61: Test setup refactored for parameter object pattern.The introduction of the
transactionHandlerParamsobject follows good practice by encapsulating all constructor parameters, making the code more maintainable and easier to extend with new parameters in the future.
29-39: Mock setup properly includes new transaction methods.The mock setup correctly includes all new methods that have been added to the
ArbToEthTransactionHandlerclass (claim,startVerification,verifySnapshot,withdrawClaimDeposit), ensuring comprehensive test coverage for the new functionality.
65-81: Constructor tests updated to use parameter object.The tests have been properly refactored to use the new
transactionHandlerParamsobject, ensuring that all properties are correctly initialized in both scenarios (with and without a claim).
84-106: Transaction status test updated to use Transaction objects.The tests now correctly use the Transaction object type with
broadcastedTimestampas required by the updated implementation. The constantMAX_PENDING_CONFIRMATIONSis correctly used for consistency.
148-185: Comprehensive tests for makeClaim functionality.The new tests for
makeClaimcover key scenarios:
- Successfully making a claim when no transaction is pending
- Not making a claim when a transaction is already pending
The implementation correctly retrieves gas estimates and includes the required deposit value from the bridge configuration.
187-238: Thorough test coverage for startVerification method.The tests cover all important scenarios:
- Successfully starting verification when conditions are met
- Not starting when a transaction is already pending
- Throwing an error when claim is not set
- Not starting when the timeout hasn't passed yet
The test properly uses configuration values from the bridge config to determine the verification flip time.
240-288: Complete test coverage for verifySnapshot method.Similar to startVerification, these tests thoroughly cover successful verification, pending transaction handling, error cases when claim is not set, and timeout validation. The implementation correctly calculates the verification flip time using the minChallengePeriod from bridge configuration.
290-332: Robust tests for withdrawClaimDeposit functionality.The tests ensure deposits can be withdrawn when conditions are met, prevent duplicate transactions when a withdrawal is pending, and throw appropriate errors when the claim is not set. This is consistent with the PR's objective of enhancing claimer functionality.
334-380: Tests for challenger path properly refactored.The tests for challenging claims have been correctly updated to use the new Transaction object format and contract type constants. Error handling for missing claims is properly tested.
380-380: Missing test implementation for final transaction case.The test.todo entry indicates a planned test for checking when the challenge transaction is completed that hasn't been implemented yet.
Would you like me to implement the missing test case for the "should set challengeTxn as completed when txn is final" scenario?
422-426: Event name updated for consistency.The test now correctly checks for the
WITHDRAWING_CHALLENGE_DEPOSITevent instead of the previously usedWITHDRAWING, which makes the event naming more specific and clear about what type of withdrawal is occurring.
429-469: Tests for sendSnapshot properly updated.The tests have been correctly modified to use the new Transaction object format and properly check ContractType.INBOX. Error handling when claim is not set is also properly tested.
471-502: resolveChallengedClaim tests correctly refactored.The tests have been properly updated to use the Transaction object format and the correct contract type constant. Both success and pending transaction cases are handled.
validator-cli/src/utils/botConfig.ts (2)
5-9: Enum usage looks good.
No issues found.
11-14: Interface declaration is concise.
No concerns here.validator-cli/src/utils/claim.test.ts (7)
56-70: Mocking out claim hashes and parameters.
The approach to mocking outbox calls withmockResolvedValueOnceis sound, ensuring the test focuses ongetClaim.
98-99: No issues found with the challenger test.
It cleanly covers challenger data retrieval.
127-129: Sets provider for verification context.
AttachingveaOutboxProvideris appropriate for verifying claims.
135-159: Fallback to subgraphs test is comprehensive.
This effectively simulates a fallback scenario when logs aren’t available on-chain.
163-164: Null claim scenario test is valid.
EnsuresgetClaimhandles missing on-chain data gracefully.
175-180: Throws error when no claim is found.
Logic is correct, verifying that the function properly raisesClaimNotFoundError.
202-202: Checks invalid claim path.
Ensures handling forhoneststate mismatch.validator-cli/src/consts/bridgeRoutes.ts (5)
21-22: Added optionalrouterRPCandrouteConfig.
This flexible approach helps unify RPC endpoints and route details.
32-35: Enum for Network.
Straightforward usage; good for clarity among devnet/testnet distinctions.
37-48: ArbToEth configs properly segmented by devnet/testnet.
TheepochPerioddifference is intuitive.
50-62: ArbToGnosis configs similarly structured.
Consistent approach with optionalveaRouterfor testnet.
72-82: RouterRPC usage introduced.
Ensures bridging routes can be expanded as needed for cross-chain calls.validator-cli/src/ArbToEth/claimer.ts (2)
1-28: CheckAndClaimParams interface is comprehensive.
These optional fetch functions are useful for dependency injection in tests.
83-102: makeClaimDevnet handles devnet logic.
Recognizes new messages vs. zero hash. If conditions are not met, it’s correct to emitNO_CLAIM_REQUIRED.validator-cli/src/utils/claim.ts (4)
6-11: Imports appear consistent and well-organized.
They streamline access to fallback retrieval methods fromgraphQueries.
36-45: Good use of destructuring with default fetch functions.
This approach supports dependency injection, making the function more testable and flexible.
115-125: Documentation looks great.
Explicit doc comments for parameters (e.g.,fromBlock,toBlock) are helpful for external collaborators.
193-193: Exporting these declarations centralizes functionality effectively.
This organized export provides clarity for importing modules.validator-cli/src/watcher.ts (6)
2-5: Imports for bridging and Ethers utilities are clear and concise.
They align with the subsequent usage in network setup and transaction handling.
10-14: Centralizing configuration imports.
ReadingBotPaths,NetworkConfig, and claim param types from separate modules helps keep this file focused on watcher logic.
16-16: Constant naming is descriptive.
RPC_BLOCK_LIMITis clearly documented, making it easy to adjust if RPC constraints change.
19-19: Doc comment addition.
Adding a high-level explanation about the watcher's purpose benefits maintainability.
40-43: Graceful loop approach.
Thewhileloop and10swait cycle ensures minimal overhead for watchers. Reevaluate the interval if more frequent checks are needed.Also applies to: 45-45
100-172: Effective decomposition intoprocessEpochsForNetwork.
This modular approach keepsprocessNetworklean and more testable. The iteration logic for epochs is straightforward.
| try { | ||
| const [claimLogs, challengeLogs, verificationLogs] = await Promise.all([ | ||
| veaOutbox.queryFilter(veaOutbox.filters.Claimed(null, epoch, null), fromBlock, toBlock), | ||
| veaOutbox.queryFilter(veaOutbox.filters.Challenged(epoch, null), fromBlock, toBlock), | ||
| veaOutbox.queryFilter(veaOutbox.filters.VerificationStarted(epoch), fromBlock, toBlock), | ||
| ]); | ||
| claim.stateRoot = claimLogs[0].data; | ||
| claim.claimer = `0x${claimLogs[0].topics[1].slice(26)}`; |
There was a problem hiding this comment.
Check for empty claimLogs to avoid index out-of-range errors.
If claimLogs is empty, accessing claimLogs[0] will throw an error, bypassing the fallback logic in the catch block.
try {
const [claimLogs, ...] = await Promise.all([...]);
- claim.stateRoot = claimLogs[0].data;
- claim.claimer = `0x${claimLogs[0].topics[1].slice(26)}`;
+ if (claimLogs.length > 0) {
+ claim.stateRoot = claimLogs[0].data;
+ claim.claimer = `0x${claimLogs[0].topics[1].slice(26)}`;
+ } else {
+ throw new Error("No claim logs found");
+ }
...
} catch {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| try { | |
| const [claimLogs, challengeLogs, verificationLogs] = await Promise.all([ | |
| veaOutbox.queryFilter(veaOutbox.filters.Claimed(null, epoch, null), fromBlock, toBlock), | |
| veaOutbox.queryFilter(veaOutbox.filters.Challenged(epoch, null), fromBlock, toBlock), | |
| veaOutbox.queryFilter(veaOutbox.filters.VerificationStarted(epoch), fromBlock, toBlock), | |
| ]); | |
| claim.stateRoot = claimLogs[0].data; | |
| claim.claimer = `0x${claimLogs[0].topics[1].slice(26)}`; | |
| try { | |
| const [claimLogs, challengeLogs, verificationLogs] = await Promise.all([ | |
| veaOutbox.queryFilter(veaOutbox.filters.Claimed(null, epoch, null), fromBlock, toBlock), | |
| veaOutbox.queryFilter(veaOutbox.filters.Challenged(epoch, null), fromBlock, toBlock), | |
| veaOutbox.queryFilter(veaOutbox.filters.VerificationStarted(epoch), fromBlock, toBlock), | |
| ]); | |
| if (claimLogs.length > 0) { | |
| claim.stateRoot = claimLogs[0].data; | |
| claim.claimer = `0x${claimLogs[0].topics[1].slice(26)}`; | |
| } else { | |
| throw new Error("No claim logs found"); | |
| } | |
| // ...other code | |
| } catch (error) { | |
| // existing fallback error handling | |
| } |
Multi network support
Now the validator can support multiple networks (devnet/tesnet on multiple chains) with config provided in
.envfile.claimer
Added claimer (
checkAndClaim()) for ArbToEth route which is responsible for making claims, starting verification and verifying them for saved snapshots on testnet:For devnet it uses the special
devnetAdvanceState()to complete claim and verification in a single transaction, thePRIVATE_KEYshould be of the devnet owner.Added pm2 support
subgraphs as fallbacks for contract event logs
When resolving older epochs the block range is searched heuristically which results into errors due to exceeding rpc block limit , so in case of error in logs subgraph is used. It uses the
veascan-subgraphendpoints.Summary by CodeRabbit
New Features
Refactor
Chores