feat/validator tests #388
feat/validator tests #388
Conversation
WalkthroughThe pull request introduces significant enhancements to the validator CLI, focusing on blockchain bridge monitoring and transaction validation. The changes include updates to RPC configurations, the addition of new address variables, comprehensive test suites, and the implementation of new transaction handling logic. Furthermore, it enhances event logging and state management utilities, along with the introduction of error handling mechanisms and a structured watcher for tracking claims and epochs across Arbitrum and Ethereum networks. Changes
Sequence DiagramsequenceDiagram
participant Watcher
participant EpochHandler
participant ClaimValidator
participant TransactionHandler
participant Blockchain
Watcher->>EpochHandler: Get Challengeable Epochs
EpochHandler-->>Watcher: Return Epoch Range
loop For Each Epoch
Watcher->>ClaimValidator: Check Claim Status
ClaimValidator->>Blockchain: Query Claim
ClaimValidator-->>Watcher: Claim Details
Watcher->>TransactionHandler: Process Claim
TransactionHandler->>Blockchain: Submit Transactions
end
Poem
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
❌ Deploy Preview for veascan failed. Why did it fail? →
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (24)
validator-cli/.env.dist (2)
23-23: Document chain ID and consider grouping with network configurations.The chain ID
421611corresponds to Arbitrum Sepolia testnet. Consider:
- Adding a comment to document this
- Grouping it with other network-specific configurations
-VEAOUTBOX_CHAIN_ID=421611 +# Chain IDs +VEAOUTBOX_CHAIN_ID=421611 # Arbitrum Sepolia testnet
Line range hint
1-32: Improve overall file organization and documentation.Consider restructuring the file to group related configurations more logically:
# Authentication PRIVATE_KEY= -# Devnet RPCs -RPC_CHIADO=https://rpc.chiadochain.net -RPC_ARB_SEPOLIA=https://sepolia-rollup.arbitrum.io/rpc -RPC_SEPOLIA= +# Network Configuration +## Chain IDs +ARBITRUM_CHAIN_ID=421611 # Arbitrum Sepolia testnet -# Testnet or Mainnet RPCs -RPC_ARB=https://sepolia-rollup.arbitrum.io/rpc -RPC_ETH= -RPC_GNOSIS=https://rpc.chiadochain.net +## RPC Endpoints +### Testnet +RPC_ARB_TESTNET=https://sepolia-rollup.arbitrum.io/rpc # Arbitrum Sepolia +RPC_ETH_TESTNET= # Ethereum Sepolia +RPC_GNOSIS_TESTNET=https://rpc.chiadochain.net # Gnosis Chiado -# Testnet or Mainnet Addresses -# VEA Arbitrum to Ethereum +# Bridge Configuration +## VEA Arbitrum to Ethereum Bridge VEAINBOX_ARB_TO_ETH_ADDRESS=0xE12daFE59Bc3A996362d54b37DFd2BA9279cAd06 VEAOUTBOX_ARB_TO_ETH_ADDRESS=0x209BFdC6B7c66b63A8382196Ba3d06619d0F12c9 -# VEA Arbitrum to GNOSIS +## VEA Arbitrum to Gnosis Bridge VEAINBOX_ARB_TO_GNOSIS_ADDRESS=0x854374483572FFcD4d0225290346279d0718240b VEAOUTBOX_ARB_TO_GNOSIS_ADDRESS=0x2f1788F7B74e01c4C85578748290467A5f063B0b VEAROUTER_ARB_TO_GNOSIS_ADDRESS=0x5BE03fDE7794Bc188416ba16932510Ed1277b193 GNOSIS_AMB_ADDRESS=0x8448E15d0e706C0298dECA99F0b4744030e59d7d -VEAOUTBOX_CHAIN_ID=421611 - -# Devnet Addresses +# Transaction Batcher Configuration VEAINBOX_ARBSEPOLIA_TO_SEPOLIA_ADDRESS=0x906dE43dBef27639b1688Ac46532a16dc07Ce410 VEAOUTBOX_ARBSEPOLIA_TO_SEPOLIA_ADDRESS=0x906dE43dBef27639b1688Ac46532a16dc07Ce410 - - TRANSACTION_BATCHER_CONTRACT_ADDRESS_SEPOLIA=0xe7953da7751063d0a41ba727c32c762d3523ade8 TRANSACTION_BATCHER_CONTRACT_ADDRESS_CHIADO=0xcC0a08D4BCC5f91ee9a1587608f7a2975EA75d73validator-cli/src/watcher.ts (2)
31-54: Be cautious with array splicing during iteration.Inside the nested loop, you’re removing elements from
epochRangeusingsplice(i, 1)while incrementingi. Though you decrementiin that same branch, this pattern can still become error-prone if any new logic is added or altered. Consider simplifying iteration logic to reduce the risk of off-by-one errors or unexpected skips.
66-66: Add an explicit type annotation for themsparameter.Explicitly annotate the parameter type (e.g.,
ms: number) to enhance readability and ensure type safety, especially if you plan to share or reuse this utility function.validator-cli/src/utils/claim.ts (2)
29-40: Unify the approach when no claim is found.Returning
nullon line 30 versus throwingClaimNotFoundErroron line 38 may confuse callers relying on a single code path. Consider consistently handling the “no claim found” state to avoid ambiguity in upstream logic.
52-63: Revisit the logic assigning thehonestfield.Assigning
claim.honest = 1or2tries to guess which party is honest and then re-hashes. This approach might become brittle if more than two possibilities or additional states are introduced. Consider a dedicated function or an enumerated type for claim plausibility to improve extensibility.validator-cli/src/ArbToEth/transactionHandler.ts (2)
34-34: Avoid hard-coding the chain ID in the class.Storing
chainId = 11155111directly in the class might require multiple code edits if you expand to other networks. PasschainIdas a constructor parameter or retrieve it from a configuration to ensure flexibility and maintainability.
76-105: Clarify return semantics ofcheckTransactionStatus.Currently, returning
truesignifies the transaction is pending or not final, whilefalsemeans it is fully confirmed. This can be confusing for consumers of the function. Consider inverting the return value or renaming the method to more explicitly convey thattruemeans “still in progress.”validator-cli/src/utils/arbToEthState.ts (1)
56-58: Potentially large block range queries.
fromBlockEthFinalizedis computed in a way that may result in scanning a large range on the Ethereum chain. Ensure these queries are optimized or batched to handle mainnet-scale data efficiently.validator-cli/src/utils/shutdown.ts (3)
4-4: Consider using a more descriptive class nameWhile
ShutdownSignalis acceptable, a more explicit name likeShutdownManagerorShutdownControllercould improve readability and clarity of purpose in the codebase.
11-13: Use a conventional boolean accessor name“
getIsShutdownSignal” works, but adopting a name like “isShutdownRequested” or “isShutdown” can make the code more readable and standard.- public getIsShutdownSignal(): boolean { + public isShutdown(): boolean { return this.isShutdownSignal; }
15-17: Add logging or hooks to facilitate debuggingSetting the shutdown signal indicates a key transition. Consider adding logging or optional callback hooks to help debug or manage shutdown lifecycle events.
validator-cli/src/utils/emitter.ts (1)
9-9: Optimize repeated calls toObject.values(...)for performanceRepeatedly calling
Object.values(BotEvents).includes(event)can introduce overhead in large-scale or frequent event emissions. Consider using aSetof allowed events for more efficient lookups, if performance becomes critical.+const botEventsSet = new Set(Object.values(BotEvents)); export class MockEmitter extends EventEmitter { emit(event: string | symbol, ...args: any[]): boolean { - if (Object.values(BotEvents).includes(event as BotEvents)) { + if (botEventsSet.has(event as BotEvents)) { return true; } return super.emit(event, ...args); } }validator-cli/src/utils/errors.ts (1)
1-7: Pass the message directly tosuper(...)By passing the error message directly into
super(...), you preserve stack traces more seamlessly. Assigningthis.messageworks, but it’s often safer to rely on the parent class constructor.class ClaimNotFoundError extends Error { constructor(epoch: number) { - super(); - this.name = "ClaimNotFoundError"; - this.message = `No claim was found for ${epoch}`; + super(`No claim was found for ${epoch}`); + this.name = "ClaimNotFoundError"; } }validator-cli/src/utils/botEvents.ts (1)
19-19: Normalize naming conventionThe enum key
CANT_EXECUTE_SNAPSHOTis capitalized differently than others (most are lowercase snake case). Consider unifying naming conventions to maintain code consistency.- CANT_EXECUTE_SNAPSHOT = "CANT_EXECUTE_SNAPSHOT", + CANT_EXECUTE_SNAPSHOT = "cant_execute_snapshot",validator-cli/src/consts/bridgeRoutes.ts (1)
1-15: Consider adding a fallback or sanity check if environment variables are undefined.While upper layers may handle errors, undefined environment variables can lead to runtime issues. A short check or fallback can make the code more resilient, especially in production environments.
- ... + if (!process.env.RPC_ARB || !process.env.RPC_ETH) { + console.warn("Required RPC environment variables are missing. The bridge configuration might be incomplete."); + } ...Also applies to: 17-42
validator-cli/src/utils/epochHandler.test.ts (1)
34-35: Validate the rounding operation in the new test.When the function returns
Math.floor(now / (600 * 1000)) - 2, confirm that integer division is the desired approach. If partial epochs are relevant, the floor might skip potentially challengeable epochs close to transition boundaries.validator-cli/src/utils/epochHandler.ts (2)
9-9: Handle missing or invalid bridge configuration.If
fetchBridgeConfig(chainId)returnsundefinedor missing fields, it may cause a runtime error. Even though upstream layers might already validate, a graceful early return can improve maintainability and debugging.- const { sequencerDelayLimit, epochPeriod } = fetchBridgeConfig(chainId); + const config = fetchBridgeConfig(chainId); + if (!config) { + throw new Error(`Bridge config not found for chainId: ${chainId}`); + } + const { sequencerDelayLimit, epochPeriod } = config;
49-51: Consider adding a fallback for missing epochPeriod.If
epochPeriodis missing or invalid, this could produce unexpected results. A short check would help avoid silent failures.validator-cli/src/utils/arbMsgExecutor.ts (1)
42-43: Log or handle the scenario for missing messages more explicitly.Currently, a console error is printed if no messages are found. You might want to throw an exception (similar to
messageExecutor) or return a fallback status to ensure consistent error handling.- console.error("No child-to-parent messages found"); + throw new Error("No child-to-parent messages found");Also applies to: 55-55
validator-cli/src/utils/ethers.ts (2)
41-45: Consider multi-chain logic expansion
getVeaRouteronly returns the router for chain ID10200. If future expansions add new router addresses for other networks, you may want to extend or centralize the chain-based logic.
52-53: Naming consistency for Devnet
getVeaOutboxArbToEthDevnetis specialized. If you add more devnet functions, ensure naming is consistent and consider a unified approach for dev/test networks.validator-cli/src/utils/logger.ts (1)
1-99: Add log levels or verbosity control
While the logger is straightforward, consider introducing standard log levels (e.g., debug, info, warn, error) for more granular control during production. This can help reduce noise and streamline debugging.export const configurableInitialize = (emitter: EventEmitter) => { // Bridger state logs emitter.on(BotEvents.STARTED, () => { - console.log("Validator started"); + console.info("[STARTED] Validator started"); }); // ... }validator-cli/src/ArbToEth/validator.test.ts (1)
1-219: Comprehensive tests
The tests forchallengeAndResolveClaimmethodically verify each scenario. For concurrency or race conditions (e.g., multiple challenges in quick succession), you might want to add further scenario coverage or concurrency safety checks if relevant.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (22)
validator-cli/.env.dist(1 hunks)validator-cli/jest.config.ts(1 hunks)validator-cli/package.json(2 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/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/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.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.
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.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/ethers.ts (1)
Learnt from: mani99brar
PR: kleros/vea#370
File: bridger-cli/src/utils/ethers.ts:31-37
Timestamp: 2024-12-10T04:59:11.283Z
Learning: In the project, `chainId` validation is performed in outer layers, so individual functions like `getVeaInboxProvider` do not need to handle unsupported `chainId`s.
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.
🪛 Biome (1.9.4)
validator-cli/src/utils/arbToEthState.ts
[error] 18-18: Don't use 'Boolean' as a type.
Use lowercase primitives for consistency.
Safe fix: Use 'boolean' instead
(lint/complexity/noBannedTypes)
🔇 Additional comments (17)
validator-cli/.env.dist (1)
14-21: Verify bridge contract addresses and checksum format.The contract addresses need to be verified for correctness and proper checksum format.
Run the following script to validate the addresses:
✅ Verification successful
Let me gather more information about these addresses by checking for any configuration or documentation files that might help verify them.
Let me check the deployment files to verify these addresses.
Based on the verification results, I can now provide the final response:
✓ Contract addresses verified and properly checksummed
All addresses in the
.env.distfile have been verified:
VEA Arbitrum to Ethereum:
- VeaInboxArbToEth (0xE12daFE59Bc3A996362d54b37DFd2BA9279cAd06) - Found in deployment files and properly checksummed
- VeaOutboxArbToEth (0x209BFdC6B7c66b63A8382196Ba3d06619d0F12c9) - Found in deployment files and properly checksummed
VEA Arbitrum to GNOSIS:
- VeaInboxArbToGnosis (0x854374483572FFcD4d0225290346279d0718240b) - Found in deployment files and properly checksummed
- VeaOutboxArbToGnosis (0x2f1788F7B74e01c4C85578748290467A5f063B0b) - Found in deployment files and properly checksummed
- VeaRouterArbToGnosis (0x5BE03fDE7794Bc188416ba16932510Ed1277b193) - Found in deployment files
- GnosisAMB (0x8448E15d0e706C0298dECA99F0b4744030e59d7d) - Found in deployment configuration
All addresses are properly checksummed and match the deployed contract addresses in the deployment files.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify contract addresses exist and are properly checksummed # Function to check if address is properly checksummed using web3 check_addresses() { while IFS='=' read -r key value; do if [[ $value =~ ^0x[a-fA-F0-9]{40}$ ]]; then echo "Checking $key: $value" # Use ast-grep to find any references to this address in the codebase ast-grep --pattern "$value" fi done } # Extract and check all contract addresses grep -E "^VEA.*ADDRESS=" validator-cli/.env.dist | check_addresses grep "GNOSIS_AMB_ADDRESS" validator-cli/.env.dist | check_addressesLength of output: 2940
Script:
#!/bin/bash # Search for documentation or configuration files that might contain these addresses echo "Searching for documentation files..." fd -e md -e txt -e json echo -e "\nSearching for address references in markdown files..." rg -i "0x[a-fA-F0-9]{40}" -t md echo -e "\nChecking for deployment or configuration files..." fd -e yaml -e yml -e json -e config echo -e "\nSearching for any test files that might use these addresses..." fd -e test.js -e test.ts -e spec.js -e spec.tsLength of output: 5977
Script:
#!/bin/bash # Check deployment files for the addresses echo "Checking VEA Arbitrum to Ethereum addresses..." for addr in "0xE12daFE59Bc3A996362d54b37DFd2BA9279cAd06" "0x209BFdC6B7c66b63A8382196Ba3d06619d0F12c9"; do rg -l "$addr" contracts/deployments/ done echo -e "\nChecking VEA Arbitrum to GNOSIS addresses..." for addr in "0x854374483572FFcD4d0225290346279d0718240b" "0x2f1788F7B74e01c4C85578748290467A5f063B0b" "0x5BE03fDE7794Bc188416ba16932510Ed1277b193" "0x8448E15d0e706C0298dECA99F0b4744030e59d7d"; do rg -l "$addr" contracts/deployments/ done echo -e "\nChecking contract addresses in deployment files..." cat contracts/deployments/arbitrumSepolia/VeaInboxArbToEthTestnet.json contracts/deployments/arbitrumSepolia/VeaOutboxArbToEthTestnet.json 2>/dev/null || trueLength of output: 50414
validator-cli/src/watcher.ts (1)
68-71: LGTM for the module entrypoint condition.The approach correctly prevents
watch()from being auto-invoked when the file is imported rather than run directly. No issues spotted here.validator-cli/src/ArbToEth/validator.ts (1)
63-65: Verify concurrency handling on claim challenges.When
claimSnapshot != claim.stateRoot && claim.challenger == ethers.ZeroAddress, multiple watchers or parallel instances could attempt to initiate a challenge simultaneously. Ensure the contract handles concurrency gracefully—otherwise, subsequent attempts might throw or revert.✅ Verification successful
Let me search for more information about the contract implementation and challenge handling.
Let me search for the contract implementation to understand how challenges are handled.
Let me try a different search pattern to find the challenge claim implementation.
Let me search for the contract's challenge handling implementation.
Let me search for the transaction handler's challenge implementation with a different pattern.
Let me search for more context about the contract's challenge handling and concurrency protection.
Based on the code analysis, I can now provide the final response:
Concurrency is handled safely through state checks in the contract
The contract's
_challengefunction has built-in protection against concurrent challenges through state checks:
- It verifies
_claim.challenger == address(0)to ensure the claim hasn't already been challenged- It checks
_claim.honest == Party.Noneto confirm the claim hasn't been verified- The state updates are atomic within the transaction, updating both the challenger address and claim hash
These checks ensure that only the first challenge transaction will succeed, while subsequent concurrent challenge attempts will revert with "Claim already challenged." This makes it safe for multiple watchers to attempt challenges simultaneously.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for all invocations of 'challengeAndResolveClaim' to see if concurrency is managed at the call sites. rg -A 5 "challengeAndResolveClaim"Length of output: 4730
Script:
#!/bin/bash # Search for the contract implementation of challengeClaim ast-grep --pattern 'function challengeClaim($$$)' # Search for the transaction handler implementation rg -A 10 "class ArbToEthTransactionHandler" # Search for any race condition handling or mutex patterns rg -i "mutex|lock|concurrent|race"Length of output: 83715
Script:
#!/bin/bash # Search for the contract implementation rg -A 10 "function challengeClaim" # Search for the transaction handler implementation rg -A 10 "async function challengeClaim"Length of output: 79
Script:
#!/bin/bash # Search for the transaction handler implementation rg -A 10 "class ArbToEthTransactionHandler" # Search for challenge-related functions in the transaction handler ast-grep --pattern 'async function $FUNC($$$) { $$$ }' --pattern-ids FUNC=challengeClaim # Look for the contract implementation rg -A 10 "VeaOutboxArbToEth"Length of output: 69776
Script:
#!/bin/bash # Search for the contract's challenge implementation rg -A 10 "function challenge" # Search for the transaction handler's challenge implementation rg -A 10 "async function challengeClaim" validator-cli/src/ArbToEth/transactionHandler.tsLength of output: 68762
Script:
#!/bin/bash # Search for the transaction handler's challenge implementation rg -A 10 "challengeClaim" validator-cli/src/ArbToEth/transactionHandler.ts # Search for the contract's challenge handling rg -A 10 "function _challenge" contracts/src/arbitrumToEth/VeaOutboxArbToEth.solLength of output: 1159
Script:
#!/bin/bash # Search for the contract's challenge event and state changes rg -A 10 "event Challenged" contracts/src/arbitrumToEth/VeaOutboxArbToEth.sol # Search for any mutex or lock patterns in the contract rg -A 10 "mapping.*=>.*challenger" contracts/src/arbitrumToEth/VeaOutboxArbToEth.sol # Search for the transaction handler's challenge implementation rg -A 20 "challengeClaim" validator-cli/src/ArbToEth/transactionHandler.tsLength of output: 1784
validator-cli/src/utils/epochHandler.test.ts (1)
20-23: Double-check edge cases for large epoch ranges.The test ensures the correct range for typical epoch values. However, consider adding a test for extremely large
currentEpochor situations wherestartEpochmight approach zero. Even if upper layers typically guard this, extra coverage can prevent future regressions.validator-cli/src/utils/epochHandler.ts (1)
21-30: Verify correctness of epoch calculation logic.The final array includes epochs up to
veaEpochOutboxClaimableNow - 1. Make sure that's the intended boundary. Off-by-one errors are common in epoch-based logic, so confirm that you don’t miss or overshoot the desired final epoch.✅ Verification successful
Let me gather more information about the epoch calculation logic and its usage.
Based on the test file and implementation details gathered, I can now provide the final response:
The epoch calculation logic is correctly implemented
The epoch boundary calculation in
setEpochRangeis correct and intentional. Here's why:
- The test file explicitly verifies that
result[result.length - 1]equalscurrentEpoch - 1- This aligns with the system's design where:
- Only past epochs are claimable (as noted in the code comment)
- The epoch calculation uses floor division and subtracts 1 to ensure we're working with completed epochs
- The contract deployment configurations (found in deploy files) show this is consistent across different chains with varying epoch periods (e.g., 600s for devnet, 1800s for testnet, 7200s for mainnet)
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Checking for usage of setEpochRange beyond tests to confirm alignment with epoch boundaries rg -A 8 'setEpochRange'Length of output: 3979
Script:
#!/bin/bash # Check the test file to understand the expected behavior cat validator-cli/src/utils/epochHandler.test.ts # Look for any other files that might be using or defining epoch-related constants rg "epochPeriod|sequencerDelayLimit" -A 3Length of output: 76918
validator-cli/src/utils/arbMsgExecutor.ts (1)
13-17: Clarify which message is being executed.Only the first child-to-parent message (
messages[0]) is handled. If multiple messages exist, these lines ignore subsequent messages. Is that intentional? If not, consider iterating through all messages or clarifying why only the first one matters.validator-cli/src/utils/ethers.ts (5)
12-13: Confirm usage of chain-specific imports
The imports forchallengeAndResolveClaimArbToEthandArbToEthTransactionHandlerappear correct; just ensure they're needed only for chain 11155111 to avoid confusion.
48-49: No concerns found
ThegetWETHfunction looks straightforward and matches established patterns.
76-83: Good to export
Exporting all utility functions centrally is helpful; this looks consistent and well-organized.
23-29: Handle unsupported chain IDs
ThegetVeaInboxfunction only handles chain IDs11155111and10200. For any other chain ID, no inbox is returned, which can lead to silent failures if a different chain ID is passed. Consider throwing an error or logging a warning for better visibility.function getVeaInbox(veaInboxAddress: string, privateKey: string, web3ProviderURL: string, chainId: number) { switch (chainId) { case 11155111: return VeaInboxArbToEth__factory.connect(veaInboxAddress, getWallet(privateKey, web3ProviderURL)); case 10200: return VeaInboxArbToGnosis__factory.connect(veaInboxAddress, getWallet(privateKey, web3ProviderURL)); + default: + throw new Error(`Chain ID ${chainId} is not supported for getVeaInbox`); } }
60-65: Verify chain-specific validators
getClaimValidatoronly explicitly supports chain ID11155111. If other networks might require a claim validator, consider adding explicit error handling or more logic.+ default: + throw new Error(`No claim validator configured for chain ID: ${chainId}`);validator-cli/src/utils/claim.test.ts (1)
1-188: Thorough test coverage
Your tests cover multiple scenarios, including valid claims, challengers, and verification. This level of detail is excellent. Consider adding boundary cases such as extreme block timestamps or network failures if feasible.validator-cli/src/ArbToEth/transactionHandler.test.ts (2)
126-130: Good Null Transaction HandlingThe test correctly ensures that a
nulltransaction hash immediately yields a non-final status without errors.
176-176: Complete or Remove the TODO TestThere is a pending
it.todo("should set challengeTxn as completed when txn is final"). To ensure full coverage, either implement the test or remove the TODO if it's no longer needed.Would you like me to open a new issue to track this?
validator-cli/package.json (3)
13-17: Confirm Updated Entry Point and Watcher ConsistencyThe
startscript has moved from./src/ArbToEth/watcher.tsto./src/watcher.ts. Verify that any references to the old watcher file have been updated for consistent entry points.
18-18: New Test Script AddedThe
testscript ("jest --coverage") is a good addition. It improves confidence in changes by measuring test coverage.
32-34: Jest Dependencies Look GoodAdding
@types/jest,jest, andts-jestensures a proper TypeScript testing environment.
| RPC_ARB=https://sepolia-rollup.arbitrum.io/rpc | ||
| RPC_ETH= | ||
| RPC_GNOSIS=https://rpc.chiadochain.net |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consolidate duplicate RPC endpoints.
The same RPC URLs are defined multiple times:
RPC_ARB_SEPOLIAandRPC_ARBboth point tohttps://sepolia-rollup.arbitrum.io/rpcRPC_CHIADOandRPC_GNOSISboth point tohttps://rpc.chiadochain.net
Consider consolidating these into single variables with clear naming to avoid confusion:
# Testnet RPCs
-RPC_ARB=https://sepolia-rollup.arbitrum.io/rpc
+RPC_ARB_TESTNET=https://sepolia-rollup.arbitrum.io/rpc # Arbitrum Sepolia
-RPC_GNOSIS=https://rpc.chiadochain.net
+RPC_GNOSIS_TESTNET=https://rpc.chiadochain.net # Gnosis Chiado📝 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.
| RPC_ARB=https://sepolia-rollup.arbitrum.io/rpc | |
| RPC_ETH= | |
| RPC_GNOSIS=https://rpc.chiadochain.net | |
| # Testnet RPCs | |
| RPC_ARB_TESTNET=https://sepolia-rollup.arbitrum.io/rpc # Arbitrum Sepolia | |
| RPC_ETH= | |
| RPC_GNOSIS_TESTNET=https://rpc.chiadochain.net # Gnosis Chiado |
There was a problem hiding this comment.
RPC urls are same but the env vars are different for devnet and testnet/mainnet. So it's fine.
There was a problem hiding this comment.
@mani99brar, understood. Thanks for clarifying your approach regarding separate environment variables for devnet vs. testnet/mainnet. No changes needed here!
✏️ Learnings added
Learnt from: mani99brar
PR: kleros/vea#388
File: validator-cli/.env.dist:9-11
Timestamp: 2025-01-06T06:17:34.081Z
Learning: User (mani99brar) intentionally keeps the same RPC URLs under different environment variables to distinguish between devnet and testnet/mainnet usage, and this is by design rather than an oversight.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
| ArbProvider: JsonRpcProvider, | ||
| veaEpoch: number, | ||
| veaEpochPeriod: number | ||
| ): Promise<[Block, Block, Boolean] | undefined> => { |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use boolean instead of Boolean.
Per the static analysis hint, prefer lowercase boolean over the uppercase Boolean in the return type for consistency and best practices.
-): Promise<[Block, Block, Boolean] | undefined> => {
+): Promise<[Block, Block, boolean] | undefined> => {📝 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.
| ): Promise<[Block, Block, Boolean] | undefined> => { | |
| ): Promise<[Block, Block, boolean] | undefined> => { |
🧰 Tools
🪛 Biome (1.9.4)
[error] 18-18: Don't use 'Boolean' as a type.
Use lowercase primitives for consistency.
Safe fix: Use 'boolean' instead
(lint/complexity/noBannedTypes)
There was a problem hiding this comment.
@mani99brar Thanks for following through! Let me know if there's anything else I can help you with.
((
(•–•)
o_(")(") hops happily
validator-cli/src/utils/errors.ts
Outdated
| class ContractNotSupportedError extends Error { | ||
| constructor(contract: string) { | ||
| super(); | ||
| this.name = "ContractNotSupportedError"; | ||
| this.message = `Unsupported contract type: ${contract}`; | ||
| } | ||
| } |
There was a problem hiding this comment.
💡 Codebase verification
Based on the search results, I can now provide a final response:
Custom error handling needs to be implemented for ContractNotSupportedError
While ClaimNotSetError and ClaimNotFoundError are properly used and handled in the codebase (in transactionHandler.ts, claim.ts, and their respective test files), there are no visible usages or error handling patterns for ContractNotSupportedError. This error class is defined but appears to be unused, which could lead to unhandled exceptions if it's thrown anywhere in the codebase.
- Consider implementing proper error handling for
ContractNotSupportedErrorwhere contract type validations occur - If this error class is not being used, consider removing it to maintain clean code
🔗 Analysis chain
Confirm consistent handling of custom errors
Ensure that other parts of the application catch and handle ContractNotSupportedError (and the other new errors) appropriately to prevent unhandled exceptions.
Let me try a different approach to verify the error handling.
Let me try one more search to check for error handling patterns.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Searches for catch blocks or explicit error handling for each custom error class
rg "catch" -A 5 | rg "ContractNotSupportedError|ClaimNotFoundError|ClaimNotSetError"
Length of output: 84
Script:
#!/bin/bash
# Search for any usage or references of the custom error classes
rg "ContractNotSupportedError|ClaimNotFoundError|ClaimNotSetError" -A 3
# Also search for error handling patterns in TypeScript files
rg "try\s*{" -t ts -A 10
Length of output: 5956
Script:
#!/bin/bash
# Search for try-catch blocks in TypeScript files
rg "try {" -t ts -A 10
# Search for any direct references to ContractNotSupportedError
rg "ContractNotSupportedError" -A 3 -B 3
Length of output: 838
There was a problem hiding this comment.
No need for this error, an invalid contract will never be called, insured by the upper layer.
There was a problem hiding this comment.
@mani99brar, understood! If this error class is truly unnecessary (because an invalid contract scenario is prevented by the upper layer), removing it or leaving it as-is won’t affect functionality. Feel free to proceed without adding error handling for ContractNotSupportedError.
✏️ Learnings added
Learnt from: mani99brar
PR: kleros/vea#388
File: validator-cli/src/utils/errors.ts:17-23
Timestamp: 2025-01-06T06:19:48.531Z
Learning: According to the user, an invalid contract will never be triggered due to the upper-layer logic, so the `ContractNotSupportedError` is not needed.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
| routerAddress?: string; | ||
| roueterProvider?: string; |
There was a problem hiding this comment.
Correct the spelling of "roueterProvider".
There's a potential typo in the property name roueterProvider and its usage. Consider renaming it to routerProvider to maintain consistency and clarity.
Apply this diff to fix the property name:
- roueterProvider?: string;
+ routerProvider?: string;And update its usage:
- roueterProvider: process.env.RPC_ETH,
+ routerProvider: process.env.RPC_ETH,Also applies to: 37-37
There was a problem hiding this comment.
@mani99brar, thanks for taking care of that fix! Let me know if you need anything else.
| function getVeaOutbox(veaOutboxAddress: string, privateKey: string, web3ProviderURL: string, chainId: number) { | ||
| switch (chainId) { | ||
| case 11155111: | ||
| return VeaOutboxArbToEth__factory.connect(veaOutboxAddress, getWallet(privateKey, web3ProviderURL)); | ||
| case 10200: | ||
| return VeaOutboxArbToGnosis__factory.connect(veaOutboxAddress, getWallet(privateKey, web3ProviderURL)); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Ensure uniform chain ID handling in outbox
Similar to getVeaInbox, this switch statement only covers two chain IDs. Consider handling other chain IDs or at least providing a fallback error.
There was a problem hiding this comment.
Current bot supports only 2 bridges, for a new bridge, it will be added here. env validation will be added in future PRs thus fallback error not needed.
There was a problem hiding this comment.
@mani99brar Understood. Your approach of supporting only two chain IDs for now makes sense, and adding a fallback or raising an error isn’t strictly necessary if you plan to handle new IDs in later PRs. Thanks for clarifying!
✏️ Learnings added
Learnt from: mani99brar
PR: kleros/vea#388
File: validator-cli/src/utils/ethers.ts:32-38
Timestamp: 2025-01-06T06:24:17.238Z
Learning: We only support chain IDs 11155111 and 10200 for bridging in the current bot. For future bridges, chain IDs will be added as needed, and environment validation (fallback handling) will be addressed in a future PR.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
validator-cli/src/utils/ethers.ts
Outdated
| const getTransactionHandler = (chainId: number) => { | ||
| switch (chainId) { | ||
| case 11155111: | ||
| return ArbToEthTransactionHandler; | ||
| } | ||
| }; |
There was a problem hiding this comment.
💡 Codebase verification
Based on the search results, I can now provide a final response:
Add default error case for unsupported chain IDs
The codebase shows that chain ID 10200 is supported in other switch statements for VeaInbox and VeaOutbox connections, but getTransactionHandler only handles 11155111 (Sepolia) without any error handling for unsupported chains. Adding a default case is necessary for consistent error handling across the codebase.
validator-cli/src/utils/ethers.ts: Add default case to handle unsupported chain IDs ingetTransactionHandler
+ default:
+ throw new Error(`No transaction handler configured for chain ID: ${chainId}`);🔗 Analysis chain
Confirm transaction handler coverage
getTransactionHandler only covers chain ID 11155111. If other networks need transaction handling, add separate implementations or an error.
+ default:
+ throw new Error(`No transaction handler configured for chain ID: ${chainId}`);Let me gather information about transaction handlers and chain IDs in the codebase.
Let me search for more specific information about transaction handlers and chain support.
🏁 Scripts executed
The following scripts were executed for the analysis:
Error: No shell scripts found
Script:
#!/bin/bash
# Search for chain ID references and transaction handlers
rg -A 5 "chainId.*11155111|case.*11155111"
# Look for other transaction handler implementations
ast-grep --pattern 'class $_TransactionHandler'
# Search for imports of transaction handlers
rg "import.*TransactionHandler"
Length of output: 4072
Script:
#!/bin/bash
# Look for other chain IDs in switch statements
rg -A 3 "switch.*chainId"
# Search for other TransactionHandler implementations
fd -e ts -e js transactionHandler
# Look for any error handling related to unsupported chains
rg "throw.*chain.*[Ii]nvalid|throw.*chain.*support"
Length of output: 1841
There was a problem hiding this comment.
Done, added a default and throwing custom TransactionHandlerNotDefinedError error
There was a problem hiding this comment.
@mani99brar Great! Thanks for adding the default and throwing the custom TransactionHandlerNotDefinedError. This ensures robust error handling for unsupported chain IDs in getTransactionHandler. Let me know if there's anything else you need.
✏️ Learnings added
Learnt from: mani99brar
PR: kleros/vea#388
File: validator-cli/src/utils/ethers.ts:67-72
Timestamp: 2025-01-06T06:27:38.628Z
Learning: A custom error named `TransactionHandlerNotDefinedError` is now thrown for unsupported chain IDs in `getTransactionHandler` within `validator-cli/src/utils/ethers.ts`.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
| it("should return false if transaction is not final", async () => { | ||
| jest.spyOn(mockEmitter, "emit"); | ||
| veaInboxProvider.getTransactionReceipt.mockResolvedValue({ | ||
| blockNumber: finalityBlock - (transactionHandler.requiredConfirmations - 1), | ||
| }); | ||
| const trnxHash = "0x123456"; | ||
| const status = await transactionHandler.checkTransactionStatus(trnxHash, ContractType.INBOX); | ||
| expect(status).toBeTruthy(); | ||
| expect(mockEmitter.emit).toHaveBeenCalledWith( | ||
| BotEvents.TXN_NOT_FINAL, | ||
| trnxHash, | ||
| transactionHandler.requiredConfirmations - 1 | ||
| ); | ||
| }); | ||
|
|
There was a problem hiding this comment.
Mismatch Between Test Name and Assertions
The test title says "should return false if transaction is not final", but it checks expect(status).toBeTruthy(). This contradictory behavior can confuse maintainers.
Use one of the following fixes to align the test name or logic:
-it("should return false if transaction is not final", async () => {
+it("should return true if transaction is not final", async () => {Or invert the assertion:
-expect(status).toBeTruthy();
+expect(status).toBeFalsy();Committable suggestion skipped: line range outside the PR's diff.
There was a problem hiding this comment.
@mani99brar, thanks for confirming the fix! Let me know if you need further assistance.
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (15)
validator-cli/src/watcher.ts (2)
11-18: Enhance function documentation.The JSDoc comments could be more comprehensive. Consider adding:
- Return type and description
- More detailed parameter descriptions
- Example usage
- Error scenarios and handling
/** * @file This file contains the logic for watching a bridge and validating/resolving for claims. * + * @returns {Promise<void>} Resolves when the watcher is shut down * - * @param shutDownSignal - The signal to shut down the watcher - * @param emitter - The emitter to emit events + * @param {ShutdownSignal} shutDownSignal - Signal object to control the watcher's shutdown + * @param {typeof defaultEmitter} emitter - Event emitter for logging and monitoring + * + * @throws {Error} If RPC connection fails + * @throws {Error} If bridge configuration is invalid + * + * @example + * const signal = new ShutdownSignal(); + * await watch(signal); */
74-79: Add type safety and improve shutdown handling.The utility function lacks type information, and the shutdown handling could be improved.
- const wait = (ms) => new Promise((r) => setTimeout(r, ms)); + const wait = (ms: number): Promise<void> => new Promise((r) => setTimeout(r, ms)); if (require.main === module) { const shutDownSignal = new ShutdownSignal(false); + // Handle process signals for graceful shutdown + process.on('SIGINT', () => shutDownSignal.shutdown()); + process.on('SIGTERM', () => shutDownSignal.shutdown()); watch(shutDownSignal); }validator-cli/src/consts/bridgeRoutes.ts (1)
3-15: Consider adding runtime type validation for the Bridge interface.While TypeScript provides compile-time type checking, consider adding runtime validation for the Bridge interface properties, especially for critical values like
depositand addresses.validator-cli/src/utils/epochHandler.ts (2)
60-60: Address the TODO comment.The comment suggests adding claim verification logic. This should be clarified and implemented.
Would you like me to help implement the claim verification logic or create a GitHub issue to track this task?
21-21: Extract magic number into a named constant.The cold start backlog calculation (
7 * 24 * 60 * 60) should be extracted into a named constant for better readability and maintainability.+const COLD_START_BACKLOG_DAYS = 7; +const SECONDS_PER_DAY = 24 * 60 * 60; +const COLD_START_BACKLOG = COLD_START_BACKLOG_DAYS * SECONDS_PER_DAY; - const coldStartBacklog = 7 * 24 * 60 * 60; // when starting the watcher, specify an extra backlog to check + const coldStartBacklog = COLD_START_BACKLOG; // when starting the watcher, specify an extra backlog to checkvalidator-cli/src/utils/ethers.ts (1)
24-46: Consider adding default cases to switch statements.While the learnings indicate that only chain IDs 11155111 and 10200 are currently supported, adding default cases would improve error handling and make the code more maintainable.
function getVeaInbox(veaInboxAddress: string, privateKey: string, web3ProviderURL: string, chainId: number) { switch (chainId) { case 11155111: return VeaInboxArbToEth__factory.connect(veaInboxAddress, getWallet(privateKey, web3ProviderURL)); case 10200: return VeaInboxArbToGnosis__factory.connect(veaInboxAddress, getWallet(privateKey, web3ProviderURL)); + default: + throw new Error(`Unsupported chain ID: ${chainId}`); } }validator-cli/src/utils/claim.ts (1)
13-64: Refactor repeated hash checking logic.The repeated hash checking with different honest values could be extracted into a helper function for better maintainability.
+const tryClaimWithHonestValue = (claim: ClaimStruct, claimHash: string, honestValue: number): boolean => { + claim.honest = honestValue; + return hashClaim(claim) === claimHash; +}; const getClaim = async ( // ... parameters ... ): Promise<ClaimStruct | null> => { // ... existing code until line 51 ... - if (hashClaim(claim) == claimHash) { + if (tryClaimWithHonestValue(claim, claimHash, 0)) { return claim; } - claim.honest = 1; // Assuming claimer is honest - if (hashClaim(claim) == claimHash) { + if (tryClaimWithHonestValue(claim, claimHash, 1)) { return claim; } - claim.honest = 2; // Assuming challenger is honest - if (hashClaim(claim) == claimHash) { + if (tryClaimWithHonestValue(claim, claimHash, 2)) { return claim; } throw new ClaimNotFoundError(epoch); };Also, consider adding JSDoc comments to explain the meaning of different honest values (0, 1, 2).
validator-cli/src/ArbToEth/transactionHandler.ts (3)
31-66: Consider making hardcoded values configurable.The
requiredConfirmationsandchainIdvalues are hardcoded. Consider making these configurable through constructor parameters or environment variables for better flexibility.export class ArbToEthTransactionHandler { - public requiredConfirmations = 10; - public chainId = 11155111; + public requiredConfirmations: number; + public chainId: number; constructor( epoch: number, veaInbox: VeaInboxArbToEth, veaOutbox: VeaOutboxArbToEth, veaInboxProvider: JsonRpcProvider, veaOutboxProvider: JsonRpcProvider, emitter: typeof defaultEmitter = defaultEmitter, - claim: ClaimStruct | null = null + claim: ClaimStruct | null = null, + requiredConfirmations: number = 10, + chainId: number = 11155111 ) { // ... existing assignments ... + this.requiredConfirmations = requiredConfirmations; + this.chainId = chainId; }
76-106: Address the transaction timeout TODO.The TODO comment indicates missing timeout handling for pending transactions. Consider implementing a timeout mechanism to prevent transactions from being stuck indefinitely.
Would you like me to help implement the transaction timeout handling or create a GitHub issue to track this task?
124-132: Extract gas fee calculations into configurable parameters.The gas fee calculations use magic numbers (6 and 6667 gwei). Consider extracting these values into configurable parameters for better maintainability and flexibility.
+ public readonly GAS_PROFIT_DIVISOR = 6; + public readonly DEFAULT_MAX_PRIORITY_FEE = BigInt(6667000000000); // 6667 gwei public async challengeClaim() { // ... existing code ... - const maxFeePerGasProfitable = deposit / (gasEstimate * BigInt(6)); + const maxFeePerGasProfitable = deposit / (gasEstimate * BigInt(this.GAS_PROFIT_DIVISOR)); // Set a reasonable maxPriorityFeePerGas but ensure it's lower than maxFeePerGas - let maxPriorityFeePerGasMEV = BigInt(6667000000000); // 6667 gwei + let maxPriorityFeePerGasMEV = this.DEFAULT_MAX_PRIORITY_FEE;validator-cli/src/utils/arbToEthState.ts (5)
44-44: Extract magic number into a named constant.The magic number 300 (5 minutes) should be extracted into a named constant for better maintainability and readability.
- const finalityBuffer = 300; // 5 minutes, allows for network delays + const FINALITY_BUFFER_SECONDS = 300; // 5 minutes, allows for network delays
53-53: Extract magic number into a named constant.The magic number for max delay should be defined as a named constant at the top of the file.
- const maxDelayInSeconds = 7 * 24 * 60 * 60; // 7 days + const MAX_DELAY_SECONDS = 7 * 24 * 60 * 60; // 7 days
142-143: Complete the unfinished comment.The comment on line 142 appears to be incomplete. Please complete the thought or remove it if unnecessary.
81-86: Consider implementing structured logging.Multiple
console.errorcalls are used for error logging. Consider implementing structured logging with proper error levels and consistent format.Consider using a logging library like Winston or creating a custom logger:
interface BlockFinalityError { type: 'FINALITY' | 'DRIFT' | 'SEQUENCER_OFFLINE'; message: string; metadata?: Record<string, unknown>; } function logFinalityError(error: BlockFinalityError): void { // Implementation with proper error levels and structured format }Also applies to: 116-117, 121-126, 131-132
230-236: Improve error handling in binary search.The try-catch block is used for control flow but doesn't distinguish between expected and unexpected errors. Consider adding error type checking.
try { (await nodeInterface.functions.findBatchContainingBlock(mid)) as any; low = mid + 1; } catch (e) { + // Verify this is an expected "batch not found" error + if (e instanceof Error && !e.message.includes("batch not found")) { + throw e; // Re-throw unexpected errors + } high = mid - 1; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
validator-cli/src/ArbToEth/transactionHandler.test.ts(1 hunks)validator-cli/src/ArbToEth/transactionHandler.ts(1 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/claim.test.ts(1 hunks)validator-cli/src/utils/claim.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/watcher.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- validator-cli/src/utils/claim.test.ts
- validator-cli/src/ArbToEth/transactionHandler.test.ts
- validator-cli/src/utils/arbMsgExecutor.ts
🧰 Additional context used
📓 Learnings (4)
validator-cli/src/utils/errors.ts (1)
Learnt from: mani99brar
PR: kleros/vea#388
File: validator-cli/src/utils/errors.ts:17-23
Timestamp: 2025-01-06T06:19:48.719Z
Learning: According to the user, an invalid contract will never be triggered due to the upper-layer logic, so the `ContractNotSupportedError` is not needed.
validator-cli/src/utils/epochHandler.ts (1)
Learnt from: mani99brar
PR: kleros/vea#370
File: bridger-cli/src/utils/epochHandler.ts:13-24
Timestamp: 2024-12-10T08:26:12.411Z
Learning: In `bridger-cli/src/utils/epochHandler.ts`, the `setEpochRange` function does not require validation checks for `startEpoch` being non-negative or the epoch range being too large because the upper layer ensures `startEpoch` is always valid and the epoch range is appropriately managed.
validator-cli/src/utils/ethers.ts (3)
Learnt from: mani99brar
PR: kleros/vea#388
File: validator-cli/src/utils/ethers.ts:67-72
Timestamp: 2025-01-06T06:27:38.796Z
Learning: A custom error named `TransactionHandlerNotDefinedError` is now thrown for unsupported chain IDs in `getTransactionHandler` within `validator-cli/src/utils/ethers.ts`.
Learnt from: mani99brar
PR: kleros/vea#388
File: validator-cli/src/utils/ethers.ts:32-38
Timestamp: 2025-01-06T06:24:17.556Z
Learning: We only support chain IDs 11155111 and 10200 for bridging in the current bot. For future bridges, chain IDs will be added as needed, and environment validation (fallback handling) will be addressed in a future PR.
Learnt from: mani99brar
PR: kleros/vea#370
File: bridger-cli/src/utils/ethers.ts:31-37
Timestamp: 2024-12-10T04:59:11.283Z
Learning: In the project, `chainId` validation is performed in outer layers, so individual functions like `getVeaInboxProvider` do not need to handle unsupported `chainId`s.
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.
🔇 Additional comments (11)
validator-cli/src/watcher.ts (1)
1-79: Add comprehensive test coverage.This is a critical component handling blockchain interactions and claim validation, but it lacks test coverage. Please add tests for:
- Epoch range management
- Transaction handling
- Error scenarios
- Shutdown behavior
- Event emissions
Let me check for existing test files:
Would you like me to help create a test suite for this component?
validator-cli/src/utils/errors.ts (1)
1-15: LGTM! Well-structured error classes.The error classes are well-implemented with clear names and descriptive messages. They follow TypeScript best practices by extending the base
Errorclass and setting both name and message properties.Also applies to: 25-33
validator-cli/src/utils/epochHandler.ts (1)
14-41: LGTM! Well-implemented epoch range calculation.The
setEpochRangefunction is well-documented and implements a robust calculation for determining the epoch range, including handling of cold start scenarios and L2 synchronization periods.validator-cli/src/utils/ethers.ts (3)
12-14: LGTM! Clean import statements.The new imports are well-organized and necessary for the added functionality.
68-75: LGTM! Well-implemented transaction handler.The function includes proper error handling for unsupported chain IDs using a custom error class.
61-66: 🛠️ Refactor suggestionAdd default case to getClaimValidator.
Similar to getTransactionHandler, add a default case to handle unsupported chain IDs.
const getClaimValidator = (chainId: number) => { switch (chainId) { case 11155111: return challengeAndResolveClaimArbToEth; + default: + throw new Error(`No claim validator configured for chain ID: ${chainId}`); } };Likely invalid or redundant comment.
validator-cli/src/utils/claim.ts (2)
66-103: LGTM! Well-structured state management.The type definition and function implementation show good practices:
- Clear type definitions for state tracking
- Proper error handling
- Good use of default parameters
105-126: LGTM! Clean and well-documented hashing function.The function is pure, well-documented, and follows Solidity hashing conventions.
validator-cli/src/utils/arbToEthState.ts (3)
1-12: LGTM! Well-documented constants with source reference.The imports are properly organized, and the consensus constants are well-documented with a reference to Prysm's implementation.
243-243: LGTM! Clean exports.The exports are properly defined, exporting only the main function that external modules need.
109-111: Consider using block timestamps instead of local time.Using local time as a proxy for the "latest" L1 time could be problematic due to potential clock synchronization issues. Consider using block timestamps exclusively or documenting the rationale for using local time.
✅ Verification successful
Local time usage is appropriately documented and safely implemented
The use of local time as a proxy for L1 time is explicitly documented and appears to be intentionally designed for monitoring purposes, while consensus-critical operations properly rely on block timestamps. The code comment acknowledges the clock synchronization assumption, making this a valid implementation pattern.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check if local time is used elsewhere in the codebase rg -l "Date.now()" | grep -v "arbToEthState.ts"Length of output: 421
Script:
#!/bin/bash # Check for block timestamp usage rg -i "block.*timestamp" -A 2 # Get context around Date.now() usages echo "=== Date.now() usage context ===" rg "Date\.now\(\)" -A 2 -B 2Length of output: 70501
| const chainId = Number(process.env.VEAOUTBOX_CHAIN_ID); | ||
| const veaBridge: Bridge = getBridgeConfig(chainId); | ||
| 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.
Add environment variable validation.
Critical configuration values are being used without validation. Add checks to ensure required environment variables are present and valid.
+ const requiredEnvVars = [
+ 'VEAOUTBOX_CHAIN_ID',
+ 'PRIVATE_KEY',
+ ];
+
+ for (const envVar of requiredEnvVars) {
+ if (!process.env[envVar]) {
+ throw new Error(`Missing required environment variable: ${envVar}`);
+ }
+ }
+
const chainId = Number(process.env.VEAOUTBOX_CHAIN_ID);
+ if (isNaN(chainId)) {
+ throw new Error('Invalid VEAOUTBOX_CHAIN_ID: must be a number');
+ }📝 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 chainId = Number(process.env.VEAOUTBOX_CHAIN_ID); | |
| const veaBridge: Bridge = getBridgeConfig(chainId); | |
| const veaInbox = getVeaInbox(veaBridge.inboxAddress, process.env.PRIVATE_KEY, veaBridge.inboxRPC, chainId); | |
| const veaOutbox = getVeaOutbox(veaBridge.outboxAddress, process.env.PRIVATE_KEY, veaBridge.outboxRPC, chainId); | |
| const requiredEnvVars = [ | |
| 'VEAOUTBOX_CHAIN_ID', | |
| 'PRIVATE_KEY', | |
| ]; | |
| for (const envVar of requiredEnvVars) { | |
| if (!process.env[envVar]) { | |
| throw new Error(`Missing required environment variable: ${envVar}`); | |
| } | |
| } | |
| const chainId = Number(process.env.VEAOUTBOX_CHAIN_ID); | |
| if (isNaN(chainId)) { | |
| throw new Error('Invalid VEAOUTBOX_CHAIN_ID: must be a number'); | |
| } | |
| const veaBridge: Bridge = getBridgeConfig(chainId); | |
| 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.
An issue is already created for env validation and it will be resolved in a future PR.
There was a problem hiding this comment.
@mani99brar Understood, thanks for clarifying. I'll move on with the review.
validator-cli/src/watcher.ts
Outdated
| veaInbox as any, | ||
| veaInboxProvider, | ||
| veaOutboxProvider, | ||
| veaOutbox as any, | ||
| transactionHandlers[epoch], |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Remove type assertions and improve type safety.
The code uses unsafe type assertions with any. Consider creating proper interface types for the inbox and outbox contracts.
- veaInbox as any,
+ veaInbox as VeaInbox, // Create this interface
- veaOutbox as any,
+ veaOutbox as VeaOutbox, // Create this interfaceCommittable suggestion skipped: line range outside the PR's diff.
There was a problem hiding this comment.
Will consider this when updates are made for another route in a future PR.
There was a problem hiding this comment.
@mani99brar Understood, that makes sense to handle all routes together. Would you like me to create a GitHub issue to track this improvement for future implementation?
validator-cli/src/watcher.ts
Outdated
| let latestEpoch = getLatestChallengeableEpoch(chainId); | ||
| while (!shutDownSignal.getIsShutdownSignal()) { | ||
| let i = 0; | ||
| while (i < epochRange.length) { | ||
| const epoch = epochRange[i]; | ||
| emitter.emit(BotEvents.CHECKING, epoch); | ||
| const updatedTransactions = await checkAndChallengeResolve( | ||
| epoch, | ||
| veaBridge.epochPeriod, | ||
| veaInbox as any, | ||
| veaInboxProvider, | ||
| veaOutboxProvider, | ||
| veaOutbox as any, | ||
| transactionHandlers[epoch], | ||
| emitter | ||
| ); | ||
| if (updatedTransactions) { | ||
| transactionHandlers[epoch] = updatedTransactions; | ||
| } else { | ||
| delete transactionHandlers[epoch]; | ||
| epochRange.splice(i, 1); | ||
| i--; | ||
| } | ||
| i++; | ||
| } | ||
| const newEpoch = getLatestChallengeableEpoch(chainId); | ||
| if (newEpoch > latestEpoch) { | ||
| epochRange.push(newEpoch); | ||
| latestEpoch = newEpoch; | ||
| } else { | ||
| emitter.emit(BotEvents.WAITING, latestEpoch); | ||
| } | ||
| await wait(1000 * 10); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling and improve configuration.
The main loop lacks error handling and uses hard-coded values. Consider these improvements:
- Add try-catch blocks for async operations
- Make the wait time configurable
- Implement cleanup for transactionHandlers
+ const POLL_INTERVAL_MS = process.env.POLL_INTERVAL_MS ?
+ Number(process.env.POLL_INTERVAL_MS) : 10000;
while (!shutDownSignal.getIsShutdownSignal()) {
+ try {
let i = 0;
while (i < epochRange.length) {
const epoch = epochRange[i];
emitter.emit(BotEvents.CHECKING, epoch);
- const updatedTransactions = await checkAndChallengeResolve(
+ const updatedTransactions = await checkAndChallengeResolve(...);
}
// ... rest of the loop
- await wait(1000 * 10);
+ await wait(POLL_INTERVAL_MS);
+ } catch (error) {
+ emitter.emit(BotEvents.ERROR, error);
+ await wait(POLL_INTERVAL_MS);
+ }
}
+ // Cleanup
+ Object.values(transactionHandlers).forEach(handler => handler.cleanup?.());Committable suggestion skipped: line range outside the PR's diff.
There was a problem hiding this comment.
Let the bot crash for now, and cleanup and restart mechanism will be added after consideration.
There was a problem hiding this comment.
@mani99brar Understood, I'll retract the error handling suggestion.
(｀□´)⊃ Noted for future reference!
✏️ Learnings added
Learnt from: mani99brar
PR: kleros/vea#388
File: validator-cli/src/watcher.ts:38-71
Timestamp: 2025-01-08T08:42:42.986Z
Learning: In the validator-cli watcher.ts, errors are intentionally left uncaught to allow the bot to crash, with cleanup and restart mechanism planned to be added after consideration.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
validator-cli/src/utils/errors.ts
Outdated
| class ContractNotSupportedError extends Error { | ||
| constructor(contract: string) { | ||
| super(); | ||
| this.name = "ContractNotSupportedError"; | ||
| this.message = `Unsupported contract type: ${contract}`; | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Remove unused error class.
Based on the provided learning, ContractNotSupportedError is not needed as invalid contracts are prevented by upper-layer logic.
-class ContractNotSupportedError extends Error {
- constructor(contract: string) {
- super();
- this.name = "ContractNotSupportedError";
- this.message = `Unsupported contract type: ${contract}`;
- }
-}There was a problem hiding this comment.
@mani99brar Thanks for confirming! Looking forward to the update.
| inboxRPC: process.env.RPC_ARB, | ||
| outboxRPC: process.env.RPC_ETH, | ||
| inboxAddress: process.env.VEAINBOX_ARB_TO_ETH_ADDRESS, | ||
| outboxAddress: process.env.VEAOUTBOX_ARB_TO_ETH_ADDRESS, | ||
| }, |
There was a problem hiding this comment.
💡 Codebase verification
Environment variable configuration needs improvement
The codebase has several issues with environment variable handling:
- No template file (.env.example) documenting required variables
- Direct usage of environment variables without validation
- Missing runtime checks for critical infrastructure values
Suggested improvements:
- Add an
.env.examplefile listing all required variables - Implement validation at startup to ensure all required variables are set
- Consider using a configuration validation library like
zodorenvalid
🔗 Analysis chain
Verify environment variables are properly configured.
The code relies on multiple environment variables for RPCs and addresses. Consider adding runtime validation to ensure these critical values are properly set.
Also applies to: 35-40
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that all required environment variables are defined in .env file
# Check for environment variables used in the bridges configuration
env_vars=(
"RPC_ARB"
"RPC_ETH"
"RPC_GNOSIS"
"VEAINBOX_ARB_TO_ETH_ADDRESS"
"VEAOUTBOX_ARB_TO_ETH_ADDRESS"
"VEAINBOX_ARB_TO_GNOSIS_ADDRESS"
"VEAOUTBOX_ARB_TO_GNOSIS_ADDRESS"
"VEA_ROUTER_ARB_TO_GNOSIS_ADDRESS"
)
# Check .env.dist file
echo "Checking .env.dist file..."
for var in "${env_vars[@]}"; do
if ! grep -q "^${var}=" .env.dist; then
echo "Warning: ${var} is missing in .env.dist"
fi
done
Length of output: 2187
Script:
#!/bin/bash
# Search for environment variable validation and template files
echo "=== Searching for environment template files ==="
fd -t f -e example -e dist -e template .env
echo -e "\n=== Checking for environment variable validation ==="
rg -l "process\.env\." --type ts
echo -e "\n=== Checking for validation logic in files that use env vars ==="
ast-grep --pattern 'process.env.$_'
Length of output: 11573
There was a problem hiding this comment.
there is a template file .env.dist and env validation will be added ion future, there's already an issue for it.
There was a problem hiding this comment.
@mani99brar I see the .env.dist template file is comprehensive. Since I couldn't find an existing issue for environment variable validation, would you like me to create one to track this enhancement? The issue would cover implementing runtime validation for the environment variables defined in .env.dist. Let me know if you'd like me to proceed with creating the issue.
🔗 Analysis chain
@mani99brar Thank you for clarifying. I see the .env.dist template file. Would you like me to create a GitHub issue to track the environment variable validation task, or is there an existing issue that you're referring to? I can help create one with the appropriate details if needed.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for existing issues about env validation
gh issue list --search "env validation in:title,body"
# Verify .env.dist file
echo -e "\n=== Contents of .env.dist ==="
cat validator-cli/.env.dist
Length of output: 1319
| let result = (await nodeInterface.functions | ||
| .findBatchContainingBlock(L2Block.number, { blockTag: "latest" }) | ||
| .catch((e) => { | ||
| // If the L2Block is the latest ArbBlock this will always throw an error | ||
| console.log("Error finding batch containing block, searching heuristically..."); | ||
| })) as any; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Replace any type with proper interface.
The result variable is typed as any. Consider creating a proper interface for the return type of findBatchContainingBlock.
interface BatchResult {
batch: { toNumber: () => number };
// Add other relevant fields
}
// Then use it:
let result = (await nodeInterface.functions
.findBatchContainingBlock(L2Block.number, { blockTag: "latest" })
.catch((e) => {
logger.warn("Error finding batch containing block, searching heuristically...");
})) as BatchResult;There was a problem hiding this comment.
I don't think its needed for this, its just a single call
There was a problem hiding this comment.
@mani99brar Understood, that's a fair point since it's an isolated usage. I'll retract the suggestion.
| } | ||
|
|
||
| if (trnxHash == null) { | ||
| return 0; |
There was a problem hiding this comment.
Can we provide an enum instead? I would have troubles trying to remember the meaning of a specific number, but the enum would give a tag to it.
| * @param trnxHash Transaction hash to check the status of. | ||
| * @param contract Contract type to check the transaction status in. | ||
| * | ||
| * @returns False if transaction is pending || not final || not made, else True. |
There was a problem hiding this comment.
This JSDoc no longer seems valid: now multiple numbers are returned
There was a problem hiding this comment.
Forgot to update it with the latest PR, will fix it.
| let provider: JsonRpcProvider; | ||
| if (contract === ContractType.INBOX) { | ||
| provider = this.veaInboxProvider; | ||
| } else if (contract === ContractType.OUTBOX) { |
There was a problem hiding this comment.
There are no alternative branch, so the else if can be an else or just use a ternary to make the code more concise (provider = contract === ContractType.INBOX ? this.veaInboxProvider : this.veaOutboxProvider)
There was a problem hiding this comment.
There's a third type Router, used for "hop" routes like arb-gnosis. It's not defined here though kept it to avoid confusion.
| const receipt = await provider.getTransactionReceipt(trnxHash); | ||
|
|
||
| if (!receipt) { | ||
| // TODO: Add transaction pending timeout- redo transaction. |
There was a problem hiding this comment.
Please consider opening a GH issue instead
There was a problem hiding this comment.
Done, removing comment.
| if (!this.claim) { | ||
| throw new ClaimNotSetError(); | ||
| } | ||
| if ((await this.checkTransactionStatus(this.transactions.challengeTxn, ContractType.OUTBOX)) > 0) { |
There was a problem hiding this comment.
(ignoreable) Consider extracting this kind of lengthy await statement to a named variable for readability
There was a problem hiding this comment.
Nice job on docs, custom errors and use of guards!
| // https://github.com/prysmaticlabs/prysm/blob/493905ee9e33a64293b66823e69704f012b39627/config/params/mainnet_config.go#L103 | ||
| const secondsPerSlotEth = 12; | ||
|
|
||
| export async function challengeAndResolveClaim( |
There was a problem hiding this comment.
When a function has so many parameters, consider using an object instead of positional arguments. That makes invoking the function way easier to read. It also allows you to mock one of the dependencies (for example, fetchClaim) without having to provide a value for a previous value (emitter)
There was a problem hiding this comment.
Yes will do,I was facing some issues due to this while mocking.
| it("should return null if no claim is made", async () => { | ||
| const transactionHandler = null; | ||
| mockGetClaim = jest.fn().mockReturnValue(null); | ||
| const result = await challengeAndResolveClaim( |
There was a problem hiding this comment.
If the function accepts an object instead of positional arguments, you can create a default object with all the mocked values: assertions will only require to spread the default object and place the few values that are relevant for the test case.
There was a problem hiding this comment.
Awesome great suggestion 👍 , will follow this in future tests.
| ArbProvider: JsonRpcProvider, | ||
| veaEpoch: number, | ||
| veaEpochPeriod: number | ||
| ): Promise<[Block, Block, boolean] | undefined> => { |
There was a problem hiding this comment.
(ignoreable) Usually prefer returning objects, are less error prone at destructuring
validator-cli/src/utils/botEvents.ts
Outdated
validator-cli/src/utils/claim.ts
Outdated
| if (hashClaim(claim) == claimHash) { | ||
| return claim; | ||
| } | ||
| claim.honest = 1; // Assuming claimer is honest |
There was a problem hiding this comment.
Would be nice to have an enum translating this raw values into legible values
validator-cli/src/utils/claim.ts
Outdated
| }; | ||
|
|
||
| if (sentSnapshotLogs.length === 0) return claimResolveState; | ||
| else { |
There was a problem hiding this comment.
else clause not required due to return finishing execution
|
|
||
| export const defaultEmitter = new EventEmitter(); | ||
|
|
||
| export class MockEmitter extends EventEmitter { |
There was a problem hiding this comment.
Have you considered using a new EventEmitter() instead of creating a custom class? Not sure if I am missing any context
There was a problem hiding this comment.
I used this mockEmitter to suppress logs while testing, I think you are referring to the defaultEmitter which is used for logging and defined above.
| now: number = Date.now(), | ||
| fetchBridgeConfig: typeof getBridgeConfig = getBridgeConfig | ||
| ): number => { | ||
| // NOTE: Add logic to check if claim was made here or in main function? |
There was a problem hiding this comment.
Consider using a GH issue instead of a TODO
There was a problem hiding this comment.
Forgot to remove the comment, its been done.
validator-cli/src/utils/logger.ts
Outdated
| */ | ||
|
|
||
| export const watch = async ( | ||
| shutDownSignal: ShutdownSignal = new ShutdownSignal(), |
There was a problem hiding this comment.
It's good but we have to be careful with this (we do not want the function to instantiate a different instance than the main function and not being able to detect shutdowns)
| if (confirmations >= this.requiredConfirmations) { | ||
| this.emitter.emit(BotEvents.TXN_FINAL, trnxHash, confirmations); | ||
| return 3; | ||
| } else { |
There was a problem hiding this comment.
else clause not needed (the if branch has a return)
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (16)
validator-cli/src/utils/epochHandler.ts (5)
3-13: Enhance function documentation.Consider adding:
@throwssection to document potential error scenarios@examplesection to demonstrate usage, similar to the one provided forcheckForNewEpoch
21-21: Consider externalizing the coldStartBacklog constant.The hardcoded value
7 * 24 * 60 * 60(7 days in seconds) could be moved to a configuration file or environment variable for better maintainability and flexibility.
26-27: Document the magic number in the calculation.The
-2adjustment in the calculation needs documentation explaining its purpose:const veaEpochOutboxWatchLowerBound = - Math.floor((currentTimestamp - L2SyncPeriod - coldStartBacklog) / epochPeriod) - 2; + // Subtract 2 epochs to account for <explanation here> + Math.floor((currentTimestamp - L2SyncPeriod - coldStartBacklog) / epochPeriod) - 2;
43-54: Update documentation example.The
@examplesection referencescheckForNewEpoch, but should referencegetLatestChallengeableEpochinstead:- * currentEpoch = checkForNewEpoch(currentEpoch, 7200); + * const latestEpoch = getLatestChallengeableEpoch(1); // For chainId 1 (Ethereum)
55-62: Add input validation and document the epoch adjustment.Two suggestions:
- Add validation for chainId to ensure it's a positive integer
- Document why we subtract 2 epochs in the calculation
const getLatestChallengeableEpoch = ( chainId: number, now: number = Date.now(), fetchBridgeConfig: typeof getBridgeConfig = getBridgeConfig ): number => { + if (!Number.isInteger(chainId) || chainId <= 0) { + throw new Error('chainId must be a positive integer'); + } const { epochPeriod } = fetchBridgeConfig(chainId); + // Subtract 2 epochs to account for <explanation here> return Math.floor(now / 1000 / epochPeriod) - 2; };validator-cli/src/utils/errors.ts (1)
12-12: Fix error name inconsistency.The error name "NoClaimSetError" doesn't match the class name "ClaimNotSetError". Consider updating for consistency:
- this.name = "NoClaimSetError"; + this.name = "ClaimNotSetError";validator-cli/src/utils/claim.ts (3)
7-13: Complete the function documentation.The JSDoc comment is incomplete. Consider adding:
- Return type documentation
- Description of all parameters
- Error cases documentation
/** - * * @param veaOutbox VeaOutbox contract instance + * @param veaOutboxProvider JsonRpcProvider instance * @param epoch epoch number of the claim to be fetched + * @param fromBlock starting block number for event filtering + * @param toBlock ending block number for event filtering - * @returns claim type of ClaimStruct + * @returns Promise<ClaimStruct | null> The claim data if found, null if no claim exists + * @throws {ClaimNotFoundError} If claim logs are not found or hash validation fails */
20-20: Improve type safety by replacinganywith a specific contract type.Consider using the correct contract type instead of
anyfor better type safety.- veaOutbox: any, + veaOutbox: Contract, // Import Contract from ethers
58-68: Optimize repeated hash checks.The code performs the same hash calculation multiple times. Consider optimizing:
- if (hashClaim(claim) == claimHash) { - return claim; - } - claim.honest = ClaimHonestState.CLAIMER; - if (hashClaim(claim) == claimHash) { - return claim; - } - claim.honest = ClaimHonestState.CHALLENGER; - if (hashClaim(claim) == claimHash) { - return claim; - } + const honestStates = [ + ClaimHonestState.NONE, + ClaimHonestState.CLAIMER, + ClaimHonestState.CHALLENGER + ]; + + for (const state of honestStates) { + claim.honest = state; + if (hashClaim(claim) === claimHash) { + return claim; + } + }validator-cli/src/watcher.ts (2)
34-36: Consider adding cleanup for transaction handlers.The
transactionHandlersobject might grow indefinitely as new epochs are processed. Consider implementing a cleanup mechanism for old epochs.const transactionHandlers: { [epoch: number]: InstanceType<typeof TransactionHandler> } = {}; +const MAX_STORED_EPOCHS = 100; // Adjust based on memory constraints + +function cleanupOldTransactionHandlers(handlers: typeof transactionHandlers) { + const epochs = Object.keys(handlers).map(Number).sort((a, b) => a - b); + while (epochs.length > MAX_STORED_EPOCHS) { + const oldestEpoch = epochs.shift(); + if (oldestEpoch !== undefined) { + delete handlers[oldestEpoch]; + } + } +}
71-71: Consider making the wait interval configurable.The wait interval is hardcoded to 10 seconds. Consider making it configurable through environment variables.
+const WAIT_INTERVAL_MS = Number(process.env.WAIT_INTERVAL_MS) || 10000; - await wait(1000 * 10); + await wait(WAIT_INTERVAL_MS);validator-cli/src/ArbToEth/validator.ts (2)
48-48: Add JSDoc comment for the heuristic calculation.The comment about block range query performance could be more descriptive.
- // to query event performantly, we limit the block range with the heuristic that. delta blocknumber <= delta timestamp / secondsPerSlot + /** + * Limit the block range for performant event queries using the heuristic: + * Δblock_number ≤ Δtimestamp / seconds_per_slot + * This approximation works because Ethereum targets a constant block time. + */
76-77: Consider using strict equality operator.The comparison with claim snapshot uses loose equality (
!=). Consider using strict equality (!==).- if (claimSnapshot != claim.stateRoot && claim.challenger == ethers.ZeroAddress) { + if (claimSnapshot !== claim.stateRoot && claim.challenger === ethers.ZeroAddress) {validator-cli/src/ArbToEth/transactionHandler.ts (3)
85-89: Simplify provider selection using ternary operator.The provider selection can be simplified using a ternary operator.
- if (contract === ContractType.INBOX) { - provider = this.veaInboxProvider; - } else if (contract === ContractType.OUTBOX) { - provider = this.veaOutboxProvider; - } + provider = contract === ContractType.INBOX ? this.veaInboxProvider : this.veaOutboxProvider;
130-138: Consider extracting gas fee calculations to a separate method.The gas fee calculation logic could be extracted to improve readability and reusability.
+ private calculateGasFees(deposit: bigint, gasEstimate: bigint): { maxFeePerGas: bigint, maxPriorityFeePerGas: bigint } { + const maxFeePerGasProfitable = deposit / (gasEstimate * BigInt(6)); + let maxPriorityFeePerGas = BigInt(6667000000000); // 6667 gwei + + if (maxPriorityFeePerGas > maxFeePerGasProfitable) { + maxPriorityFeePerGas = maxFeePerGasProfitable; + } + + return { + maxFeePerGas: maxFeePerGasProfitable, + maxPriorityFeePerGas + }; + }
193-193: Consider adding return type annotation.The
resolveChallengedClaimmethod is missing a return type annotation.- public async resolveChallengedClaim(sendSnapshotTxn: string, executeMsg: typeof messageExecutor = messageExecutor) { + public async resolveChallengedClaim(sendSnapshotTxn: string, executeMsg: typeof messageExecutor = messageExecutor): Promise<void> {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
validator-cli/src/ArbToEth/transactionHandler.test.ts(1 hunks)validator-cli/src/ArbToEth/transactionHandler.ts(1 hunks)validator-cli/src/ArbToEth/validator.test.ts(1 hunks)validator-cli/src/ArbToEth/validator.ts(1 hunks)validator-cli/src/utils/claim.ts(1 hunks)validator-cli/src/utils/epochHandler.ts(1 hunks)validator-cli/src/utils/errors.ts(1 hunks)validator-cli/src/watcher.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- validator-cli/src/ArbToEth/validator.test.ts
- validator-cli/src/ArbToEth/transactionHandler.test.ts
🧰 Additional context used
📓 Learnings (3)
validator-cli/src/utils/errors.ts (1)
Learnt from: mani99brar
PR: kleros/vea#388
File: validator-cli/src/utils/errors.ts:17-23
Timestamp: 2025-01-06T06:19:48.719Z
Learning: According to the user, an invalid contract will never be triggered due to the upper-layer logic, so the `ContractNotSupportedError` is not needed.
validator-cli/src/utils/epochHandler.ts (1)
Learnt from: mani99brar
PR: kleros/vea#370
File: bridger-cli/src/utils/epochHandler.ts:13-24
Timestamp: 2024-12-10T08:26:12.411Z
Learning: In `bridger-cli/src/utils/epochHandler.ts`, the `setEpochRange` function does not require validation checks for `startEpoch` being non-negative or the epoch range being too large because the upper layer ensures `startEpoch` is always valid and the epoch range is appropriately managed.
validator-cli/src/watcher.ts (1)
Learnt from: mani99brar
PR: kleros/vea#388
File: validator-cli/src/watcher.ts:38-71
Timestamp: 2025-01-08T08:42:43.153Z
Learning: In the validator-cli watcher.ts, errors are intentionally left uncaught to allow the bot to crash, with cleanup and restart mechanism planned to be added after consideration.
⏰ 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 (6)
validator-cli/src/utils/errors.ts (1)
1-25: LGTM! Well-structured error classes with clear messages.The error classes are well-designed, extending the base Error class properly and providing clear error messages.
validator-cli/src/utils/claim.ts (2)
72-81: LGTM! Well-structured type definition.The ClaimResolveState type is well-defined with clear status documentation.
110-131: LGTM! Well-documented and correctly implemented hash function.The hashClaim function is well-documented and correctly uses solidityPackedKeccak256 with proper parameter ordering.
validator-cli/src/watcher.ts (2)
19-22: LGTM! Well-structured function signature with default parameters.The function signature is well-designed with appropriate default parameters for
shutDownSignalandemitter.
25-28: Consider adding environment variable validation.Critical configuration values like
VEAOUTBOX_CHAIN_IDandPRIVATE_KEYare being used without validation.validator-cli/src/ArbToEth/transactionHandler.ts (1)
38-73: LGTM! Well-structured class with clear documentation.The class is well-documented with appropriate property initialization and type definitions.
| veaInbox: any; | ||
| veaInboxProvider: JsonRpcProvider; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Remove any type and use proper contract types.
The interface uses any type for veaInbox and veaOutbox. Consider using the imported contract types.
- veaInbox: any;
+ veaInbox: VeaInboxArbToEth;
- veaOutbox: any;
+ veaOutbox: VeaOutboxArbToEth;Also applies to: 19-20
PR-Codex overview
This PR refactors the bridge validation logic between Arbitrum and Ethereum, introduces new error handling, and enhances testing coverage. It also adds a shutdown mechanism and integrates Jest for testing.
Detailed summary
ShutdownSignalclass for graceful shutdown handling.BotEventsenum for event management.ArbToEth/transactionHandler.ts.watcher.tsto include new logic and shutdown support..env.dist.bridgeRoutes.ts.claim.ts.Summary by CodeRabbit
Release Notes
New Features
Improvements
Infrastructure
Chores