Feat(validator-cli)/arb to gnosis #424
Conversation
WalkthroughThis update refactors transaction handler logic by introducing a new modular, interface-based architecture for managing cross-chain claims and snapshots. It removes legacy Arbitrum-to-Ethereum handler code and tests, adds new generic base classes and handlers for both Arbitrum-Ethereum and Arbitrum-Gnosis routes, updates helper and utility modules for chain ID awareness, and revises environment variable naming. Changes
Sequence Diagram(s)sequenceDiagram
participant Watcher
participant HandlerFactory
participant TransactionHandler
participant BridgeContracts
participant Emitter
Watcher->>HandlerFactory: getTransactionHandler(chainId, network)
HandlerFactory-->>Watcher: TransactionHandler instance
Watcher->>TransactionHandler: makeClaim(stateRoot)
TransactionHandler->>BridgeContracts: send claim transaction
BridgeContracts-->>TransactionHandler: transaction receipt
TransactionHandler->>Emitter: emit CLAIM_MADE event
Watcher->>TransactionHandler: challengeClaim()
TransactionHandler->>BridgeContracts: send challenge transaction
BridgeContracts-->>TransactionHandler: transaction receipt
TransactionHandler->>Emitter: emit CHALLENGE_MADE event
Watcher->>TransactionHandler: sendSnapshot()
TransactionHandler->>BridgeContracts: send snapshot transaction
BridgeContracts-->>TransactionHandler: transaction receipt
TransactionHandler->>Emitter: emit SNAPSHOT_SENT event
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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 project 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: 17
🔭 Outside diff range comments (3)
validator-cli/src/watcher.ts (1)
142-154:⚠️ Potential issueMissing chainId parameter in TransactionHandler constructor
The TransactionHandler is being instantiated without the
chainIdparameter, which appears to be required based on the changes in other files.Add the missing chainId parameter:
new TransactionHandler({ + chainId, network, epoch: currentEpoch, veaInbox, veaOutbox, veaInboxProvider, veaOutboxProvider, veaRouterProvider, emitter, });validator-cli/src/utils/graphQueries.ts (1)
191-205:⚠️ Potential issue
getLastMessageSavedmay returnundefinedbut advertisesstringThe explicit guard on line 203 can return
undefined; network errors are also unhandled.
Align the signature and add atry/catchfor parity with other helpers.-const getLastMessageSaved = async (veaInbox: string, chainId: number): Promise<string> => { +const getLastMessageSaved = async (veaInbox: string, chainId: number): Promise<string | undefined> => { try { ... } catch (e) { console.error(e); return undefined; } }validator-cli/src/helpers/claimer.ts (1)
45-48: 🛠️ Refactor suggestionMissing
try/catcharound RPC call may crash the bot
veaOutbox.stateRoot()is an external RPC request. Any transient node hiccup (timeout, rate-limit, network flap) will currently throw and bubble up, killing the wholecheckAndClaimloop.
Wrap the call in atry/catch(or let the caller inject a resilient wrapper) and emit aBotEvents.RPC_ERRORso the bot can continue operating.
🧹 Nitpick comments (5)
validator-cli/src/utils/botConfig.test.ts (1)
6-19: Consider testing thetoSaveSnapshotproperty.The tests currently only verify the
pathproperty but don't test thetoSaveSnapshotproperty that's also returned bygetBotPath. Consider adding test cases to verify thetoSaveSnapshotbehavior with and without the--saveSnapshotflag.Add test cases like:
+ it("should return toSaveSnapshot as false by default", () => { + const config = getBotPath({ cliCommand: defCommand }); + expect(config.toSaveSnapshot).toBe(false); + }); + it("should return toSaveSnapshot as true when flag is present", () => { + const command = ["yarn", "start", "--saveSnapshot"]; + const config = getBotPath({ cliCommand: command }); + expect(config.toSaveSnapshot).toBe(true); + });validator-cli/src/utils/transactionHandlers/arbToEthHandler.test.ts (1)
141-141: Consider implementing the TODO test case.The TODO comment indicates a missing test for verifying that
challengeTxnis marked as completed when the transaction is final. This is an important test case for ensuring proper transaction state management.Do you want me to help implement this test case to verify the transaction completion logic?
validator-cli/src/utils/transactionHandlers/arbToEthHandler.ts (1)
17-20: Useless constructor – remove to reduce noiseThe constructor only forwards
optstosuperwithout additional logic; Biome correctly flags this.-export class ArbToEthTransactionHandler extends BaseTransactionHandler<VeaInboxArbToEth, VeaOutboxArbToEth> { - constructor(opts: BaseTransactionHandlerConstructor) { - super(opts); - } +export class ArbToEthTransactionHandler extends BaseTransactionHandler<VeaInboxArbToEth, VeaOutboxArbToEth> {}🧰 Tools
🪛 Biome (1.9.4)
[error] 17-20: This constructor is unnecessary.
Unsafe fix: Remove the unnecessary constructor.
(lint/complexity/noUselessConstructor)
validator-cli/src/utils/transactionHandlers/arbToGnosisHandler.ts (1)
22-24: Useless constructor – same issue as Arb→Eth handlerDrop the empty constructor to satisfy
noUselessConstructorand declutter.🧰 Tools
🪛 Biome (1.9.4)
[error] 22-24: This constructor is unnecessary.
Unsafe fix: Remove the unnecessary constructor.
(lint/complexity/noUselessConstructor)
validator-cli/src/utils/transactionHandlers/baseTransactionHandler.ts (1)
84-87:emittertyping is too concrete
BaseTransactionHandlerConstructorfixes the type totypeof defaultEmitter, making it impossible to inject custom emitters in tests or alternate runtimes.
Prefer the looserEventEmitter(or a minimalEmitterinterface) to improve reusability.- emitter: typeof defaultEmitter; + emitter: EventEmitter;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (24)
validator-cli/.env.dist(1 hunks)validator-cli/src/ArbToEth/transactionHandler.test.ts(0 hunks)validator-cli/src/ArbToEth/transactionHandler.ts(0 hunks)validator-cli/src/ArbToEth/transactionHandlerDevnet.ts(0 hunks)validator-cli/src/ArbToGnosis/watcherArbToGnosis.ts(0 hunks)validator-cli/src/consts/bridgeRoutes.ts(3 hunks)validator-cli/src/helpers/claimer.ts(7 hunks)validator-cli/src/helpers/snapshot.test.ts(10 hunks)validator-cli/src/helpers/snapshot.ts(6 hunks)validator-cli/src/helpers/validator.test.ts(1 hunks)validator-cli/src/helpers/validator.ts(4 hunks)validator-cli/src/utils/botConfig.test.ts(1 hunks)validator-cli/src/utils/claim.test.ts(2 hunks)validator-cli/src/utils/claim.ts(5 hunks)validator-cli/src/utils/ethers.ts(2 hunks)validator-cli/src/utils/graphQueries.ts(7 hunks)validator-cli/src/utils/transactionHandlers/arbToEthHandler.test.ts(1 hunks)validator-cli/src/utils/transactionHandlers/arbToEthHandler.ts(1 hunks)validator-cli/src/utils/transactionHandlers/arbToGnosisHandler.test.ts(1 hunks)validator-cli/src/utils/transactionHandlers/arbToGnosisHandler.ts(1 hunks)validator-cli/src/utils/transactionHandlers/baseTransactionHandler.test.ts(1 hunks)validator-cli/src/utils/transactionHandlers/baseTransactionHandler.ts(1 hunks)validator-cli/src/utils/transactionHandlers/index.ts(1 hunks)validator-cli/src/watcher.ts(8 hunks)
💤 Files with no reviewable changes (4)
- validator-cli/src/ArbToEth/transactionHandlerDevnet.ts
- validator-cli/src/ArbToEth/transactionHandler.test.ts
- validator-cli/src/ArbToEth/transactionHandler.ts
- validator-cli/src/ArbToGnosis/watcherArbToGnosis.ts
🧰 Additional context used
🧠 Learnings (4)
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/ethers.ts (1)
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`.
validator-cli/src/utils/graphQueries.ts (1)
Learnt from: mani99brar
PR: kleros/vea#370
File: bridger-cli/src/utils/graphQueries.ts:12-34
Timestamp: 2024-12-09T09:03:31.474Z
Learning: In `bridger-cli/src/utils/graphQueries.ts`, within the `getClaimForEpoch` function, returning `result['claims'][0]` is sufficient because it will be `undefined` if `result['claims']` is an empty array, making additional checks unnecessary.
validator-cli/src/utils/transactionHandlers/baseTransactionHandler.ts (1)
Learnt from: mani99brar
PR: kleros/vea#370
File: bridger-cli/src/utils/transactionHandler.ts:13-13
Timestamp: 2024-12-09T09:40:28.400Z
Learning: In `bridger-cli/src/utils/transactionHandler.ts`, the `veaOutbox` implementations differ for each chain, but a common interface should be defined for type checks to enhance type safety.
🧬 Code Graph Analysis (9)
validator-cli/src/utils/botConfig.test.ts (1)
validator-cli/src/utils/botConfig.ts (1)
getBotPath(21-42)
validator-cli/src/helpers/snapshot.ts (2)
validator-cli/src/utils/graphQueries.ts (1)
getLastMessageSaved(213-213)validator-cli/src/utils/emitter.ts (1)
defaultEmitter(4-4)
validator-cli/src/utils/claim.ts (2)
validator-cli/src/utils/errors.ts (1)
ClaimNotFoundError(60-60)validator-cli/src/utils/graphQueries.ts (1)
getSnapshotSentForEpoch(212-212)
validator-cli/src/utils/transactionHandlers/arbToEthHandler.ts (4)
validator-cli/src/utils/transactionHandlers/index.ts (3)
ArbToEthTransactionHandler(7-7)BaseTransactionHandler(11-11)ArbToEthDevnetTransactionHandler(8-8)validator-cli/src/utils/transactionHandlers/baseTransactionHandler.ts (3)
BaseTransactionHandlerConstructor(76-87)Transactions(60-71)Transaction(55-58)validator-cli/src/utils/errors.ts (1)
ClaimNotSetError(61-61)validator-cli/src/utils/arbMsgExecutor.ts (1)
messageExecutor(82-82)
validator-cli/src/utils/transactionHandlers/arbToGnosisHandler.test.ts (6)
validator-cli/src/utils/emitter.ts (1)
MockEmitter(6-14)validator-cli/src/utils/transactionHandlers/arbToGnosisHandler.ts (1)
ArbToGnosisTransactionHandler(21-110)validator-cli/src/utils/transactionHandlers/baseTransactionHandler.ts (1)
BaseTransactionHandlerConstructor(76-87)validator-cli/src/utils/ethers.ts (2)
getWETH(115-115)getWallet(111-111)validator-cli/src/utils/errors.ts (1)
ClaimNotSetError(61-61)validator-cli/src/utils/arbMsgExecutor.ts (1)
messageExecutor(82-82)
validator-cli/src/utils/transactionHandlers/baseTransactionHandler.test.ts (3)
validator-cli/src/utils/transactionHandlers/baseTransactionHandler.ts (3)
BaseTransactionHandlerConstructor(76-87)MAX_PENDING_CONFIRMATIONS(74-74)Transaction(55-58)validator-cli/src/utils/emitter.ts (1)
MockEmitter(6-14)validator-cli/src/utils/errors.ts (1)
ClaimNotSetError(61-61)
validator-cli/src/utils/transactionHandlers/arbToEthHandler.test.ts (4)
validator-cli/src/utils/transactionHandlers/baseTransactionHandler.ts (1)
BaseTransactionHandlerConstructor(76-87)validator-cli/src/utils/emitter.ts (1)
MockEmitter(6-14)validator-cli/src/utils/transactionHandlers/arbToEthHandler.ts (1)
ArbToEthTransactionHandler(16-119)validator-cli/src/utils/errors.ts (1)
ClaimNotSetError(61-61)
validator-cli/src/helpers/claimer.ts (2)
validator-cli/src/utils/transactionHandlers/index.ts (1)
IDevnetTransactionHandler(15-17)validator-cli/src/utils/graphQueries.ts (1)
getLastClaimedEpoch(209-209)
validator-cli/src/helpers/validator.ts (6)
validator-cli/src/utils/transactionHandlers/baseTransactionHandler.ts (1)
ITransactionHandler(9-39)validator-cli/src/utils/emitter.ts (1)
defaultEmitter(4-4)validator-cli/src/utils/claim.ts (2)
getClaim(196-196)getClaimResolveState(196-196)validator-cli/src/utils/arbToEthState.ts (1)
getBlocksAndCheckFinality(243-243)validator-cli/src/utils/ethers.ts (1)
getTransactionHandler(117-117)relayer-cli/src/consts/bridgeRoutes.ts (1)
Network(88-88)
🪛 Biome (1.9.4)
validator-cli/src/utils/transactionHandlers/arbToEthHandler.ts
[error] 17-20: This constructor is unnecessary.
Unsafe fix: Remove the unnecessary constructor.
(lint/complexity/noUselessConstructor)
validator-cli/src/utils/transactionHandlers/arbToGnosisHandler.ts
[error] 22-24: This constructor is unnecessary.
Unsafe fix: Remove the unnecessary constructor.
(lint/complexity/noUselessConstructor)
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: Redirect rules - veascan
- GitHub Check: Header rules - veascan
- GitHub Check: Pages changed - veascan
- GitHub Check: dependency-review
- GitHub Check: Scorecard analysis
- GitHub Check: Analyze (javascript)
🔇 Additional comments (36)
validator-cli/src/utils/botConfig.test.ts (2)
7-8: LGTM: Correctly updated for new return type.The test properly adapts to the new
getBotPathfunction signature that returns an object with apathproperty.
12-13: LGTM: Consistent test pattern updated.These tests correctly follow the same pattern of accessing the
pathproperty from the returned configuration object.Also applies to: 17-18
validator-cli/.env.dist (1)
22-22: GNOSIS_WETH address appears valid.The new environment variable for Gnosis WETH token address follows the expected Ethereum address format and aligns with the addition of Gnosis chain support mentioned in the PR objectives.
validator-cli/src/helpers/validator.test.ts (1)
43-43: LGTM! ChainId parameter addition aligns with multi-chain refactoring.The addition of
chainId: 11155111(Ethereum Sepolia) to the mock dependencies is consistent with the broader refactoring to support chain-aware operations throughout the codebase.validator-cli/src/utils/claim.test.ts (1)
261-261: LGTM! ChainId parameter correctly added to function calls.The addition of the
chainIdparameter (11155111 for Ethereum Sepolia) togetClaimResolveStatefunction calls is consistent with the broader refactoring to support multi-chain operations. The parameter placement and value are appropriate for the test scenarios.Also applies to: 278-278
validator-cli/src/helpers/snapshot.test.ts (3)
3-3: Import path update reflects improved code organization.The change from
"./emitter"to"../utils/emitter"suggests the emitter module has been moved to a more appropriate location in the utils directory, which improves code organization.
8-8: ChainId constant appropriately defined for test consistency.The addition of a
chainIdconstant with value11155111(Ethereum Sepolia) provides consistency across all test cases and aligns with the multi-chain architecture requirements.
28-28: ChainId parameter systematically added to all function calls.The chainId parameter has been correctly and consistently added to all calls of
isSnapshotNeededandsaveSnapshotfunctions throughout the test file. This ensures the tests align with the updated function signatures that support multi-chain operations.Also applies to: 46-46, 63-63, 80-80, 97-97, 120-120, 147-147, 173-173, 200-200
validator-cli/src/utils/claim.ts (5)
20-20: LGTM: Chain awareness added to interface.The addition of
chainIdparameter toClaimParamsinterface correctly supports the multi-chain refactoring mentioned in the PR objectives.
38-38: LGTM: Function signature updated for chain awareness.The
chainIdparameter addition togetClaimfunction aligns with the interface changes and enables chain-specific claim retrieval.
75-80: LGTM: Chain-aware fallback queries implemented correctly.The
chainIdparameter is properly propagated to all graph query functions (fetchClaimForEpoch,fetchVerificationForClaim,fetchChallengerForClaim) in the catch block, ensuring consistent chain-aware querying when direct contract queries fail.
129-129: LGTM: Chain awareness added to resolve state function.The
chainIdparameter addition togetClaimResolveStatefunction is consistent with the overall chain-aware refactoring pattern.
158-158: LGTM: Chain-aware snapshot query in fallback.The
chainIdparameter is correctly passed togetSnapshotSentForEpochin the catch block, maintaining consistency with the chain-aware querying approach.validator-cli/src/consts/bridgeRoutes.ts (5)
22-22: LGTM: Interface refactored for better deposit handling.The change from
depositto optionaldepositTokenin the Bridge interface aligns with the route-level deposit configuration, enabling different tokens per chain.
30-30: LGTM: Route-level deposit configuration added.Moving the
depositproperty toRouteConfigstype makes sense as different networks may require different deposit amounts.
43-49: LGTM: Explicit deposit values added for Arbitrum to Ethereum routes.The deposit values (1 ETH for both devnet and testnet) are explicitly defined at the route level, providing clear configuration for each network.
57-58: Verify the epochPeriod and deposit changes are intentional.The changes include:
- DEVNET epochPeriod: 3600 → 1800 (halved)
- TESTNET epochPeriod: 7200 → 3600 (halved)
- Different deposit amounts (0.1 ETH for devnet, 0.2 ETH for testnet)
Please confirm these timing and deposit adjustments are intentional and align with the Gnosis bridge requirements.
Also applies to: 64-65
86-86: LGTM: Environment-based deposit token configuration.The
depositTokenproperty usingprocess.env.GNOSIS_WETHenables flexible token configuration for the Gnosis chain (chainId 10200).validator-cli/src/utils/transactionHandlers/index.ts (3)
1-4: LGTM: Clean import structure for transaction handlers.The imports are well-organized, bringing in all necessary transaction handler classes and the base interface from their respective modules.
6-13: LGTM: Comprehensive barrel export pattern.The re-exports provide a clean, centralized access point for all transaction handler classes and interfaces, improving modularity and ease of use.
15-17: LGTM: Well-defined devnet-specific interface extension.The
IDevnetTransactionHandlerinterface properly extendsITransactionHandlerwith thedevnetAdvanceStatemethod, enabling devnet-specific state advancement functionality while maintaining type safety.validator-cli/src/helpers/snapshot.ts (5)
2-4: LGTM: Import paths updated for reorganized structure.The import paths are correctly updated from relative "./" to "../utils/" reflecting the codebase reorganization mentioned in the PR.
7-7: LGTM: Chain awareness added to snapshot interfaces.The
chainIdparameter addition to bothSnapshotCheckParamsandSaveSnapshotParamsinterfaces aligns with the multi-chain support refactoring.Also applies to: 14-14
26-26: LGTM: Chain ID propagated throughout snapshot functions.The
chainIdparameter is consistently passed through all function calls (saveSnapshot,isSnapshotNeeded,fetchLastSavedMessage), maintaining chain-aware functionality throughout the snapshot workflow.Also applies to: 46-46, 56-56, 71-71
34-34: LGTM: Timestamp calculation standardized.The change to
Math.floor(Date.now() / 1000)provides a consistent Unix timestamp format across the codebase.
83-83: LGTM: Defensive guard clause added.The guard clause
if (id === undefined) return 0;prevents errors when processing undefined message IDs, improving the function's robustness.validator-cli/src/utils/ethers.ts (1)
97-104: LGTM! Chain ID support for Gnosis added successfully.The implementation correctly extends the
getTransactionHandlerfunction to support chain ID 10200 (Gnosis) with appropriate network-specific handlers, following the same pattern as the existing Ethereum handlers.validator-cli/src/utils/transactionHandlers/arbToGnosisHandler.test.ts (1)
109-118: LGTM! Comprehensive test coverage for WETH approval logic.The tests properly verify both scenarios - when WETH approval is needed (allowance < deposit) and when it's not needed (allowance >= deposit).
validator-cli/src/helpers/validator.ts (2)
53-53: LGTM! Good use of conditional provider selectionThe logic to use
veaRouterProviderwhen available for finality checks is a good design choice that provides flexibility for different network configurations.
109-109:✅ Verification successful
Verify the claim.honest value check is correct
The condition now checks for
claim.honest == 2before withdrawing the challenge deposit. This appears to be a new condition that wasn't present before.Please verify that
claim.honest == 2is the correct value to check for honest claims. Let me search for the enum or constant definitions:
🏁 Script executed:
#!/bin/bash # Search for definitions of claim.honest values or HonestClaim enum ast-grep --pattern 'enum $_ { $$$ }' # Also search for any constants or documentation about honest claim values rg -A 5 -B 5 "honest.*=.*2|HONEST.*=.*2|claim\.honest"Length of output: 96300
Confirmed:
claim.honest == 2correctly targets a Challenger-honest claimThe
ClaimHonestStateenum (validator-cli/src/utils/claim.ts) defines:
• NONE = 0
• CLAIMER = 1
• CHALLENGER = 2The Solidity
withdrawChallengeDepositimplementation also requireshonest == Party.Challenger (2). The check in validator-cli/src/helpers/validator.ts (around line 109) is therefore accurate.• File: validator-cli/src/helpers/validator.ts
Line:} else if (claimResolveState.execution.status == 2 && claim.honest == 2) {(Optional) For readability, you might replace the literal
2withClaimHonestState.CHALLENGER.validator-cli/src/utils/transactionHandlers/baseTransactionHandler.test.ts (1)
23-259: Comprehensive test coverage!The test suite provides excellent coverage of the BaseTransactionHandler abstract class, including:
- Transaction status checking with various states
- Proper error handling for missing claims
- Timing constraints for verification and snapshot operations
- Transaction lifecycle management
Well structured with good use of mocks and clear test descriptions.
validator-cli/src/watcher.ts (2)
56-56: LGTM! Clean integration of router RPC supportThe changes properly extract
routerRPCfrom the bridge configuration and create the optionalveaRouterProvideronly when available. This is then correctly passed through to all relevant functions.Also applies to: 87-87, 112-112, 126-126, 136-136
156-162: Good addition of chainId to saveSnapshot callThe chainId parameter is correctly added to the saveSnapshot function call, maintaining consistency with the broader refactoring.
validator-cli/src/utils/transactionHandlers/arbToGnosisHandler.ts (1)
141-144:value:deposit may be wrong asset on Gnosis devnetDeposits for Gnosis are WETH, yet
valuesends native xDai/ETH. If the outbox expects ERC-20 deposit (depositToken), this argument should be omitted—the allowance flow already covered payment. Verify the contract’s payable requirement.validator-cli/src/helpers/claimer.ts (1)
48-60: Constructor parameter mismatch foremitter
BaseTransactionHandlerConstructorexpectsemitter: typeof defaultEmitter, but here anEventEmitterinstance is passed.
Although structurally compatible, the nominal typings differ and cause TS errors oncestrictFunctionTypesis on.- emitter, + emitter: emitter as unknown as typeof defaultEmitter,Consider tightening the types (e.g. export an
Emitterinterface) to avoid the cast.validator-cli/src/utils/transactionHandlers/baseTransactionHandler.ts (1)
201-208:timeOverlogic off-by-one risk
timeOveris calculated ascurrentTimestamp - timestampClaimed - sequencerDelayLimit - epochPeriodIf it equals 0 exactly, verification will proceed, but the sequencer delay might just be expiring in the same block, causing a revert.
Safer check:-if (timeOver < 0) { // allow 0 +if (timeOver <= 0) { // require strictly past the window
validator-cli/src/utils/transactionHandlers/arbToGnosisHandler.test.ts
Outdated
Show resolved
Hide resolved
validator-cli/src/utils/transactionHandlers/arbToGnosisHandler.test.ts
Outdated
Show resolved
Hide resolved
validator-cli/src/utils/transactionHandlers/arbToEthHandler.test.ts
Outdated
Show resolved
Hide resolved
validator-cli/src/utils/transactionHandlers/arbToEthHandler.test.ts
Outdated
Show resolved
Hide resolved
validator-cli/src/utils/transactionHandlers/arbToGnosisHandler.ts
Outdated
Show resolved
Hide resolved
|
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (2)
validator-cli/src/utils/transactionHandlers/arbToEthHandler.ts (1)
61-70:⚠️ Potential issueFix BigNumber and bigint type mixing.
The code mixes
BigNumber(from ethers.js) with nativebiginttypes, which causes type compatibility issues.Apply this fix to ensure type consistency:
const gasEstimate = await this.veaOutbox[ "challenge(uint256,(bytes32,address,uint32,uint32,uint32,uint8,address))" ].estimateGas(this.epoch, this.claim, { value: deposit }); // Profit-driven fee calculation - const maxFeePerGas = deposit / (toBigInt(gasEstimate) * BigInt(6)); + const gasEstimateBigInt = toBigInt(gasEstimate); + const maxFeePerGas = deposit / (gasEstimateBigInt * 6n); let maxPriorityFeePerGas = BigInt(6_667_000_000_000); if (maxPriorityFeePerGas > maxFeePerGas) { maxPriorityFeePerGas = maxFeePerGas; }validator-cli/src/helpers/validator.ts (1)
60-66: Hard-codedNetwork.TESTNETacknowledgedThe challenger flow is intentionally limited to TESTNET (see previous discussion).
No change requested.
🧹 Nitpick comments (4)
validator-cli/src/utils/transactionHandlers/index.ts (1)
19-39: Consider using constants for chain IDs to improve maintainability.The factory function logic is correct, but hardcoded chain IDs could be improved for maintainability.
Consider extracting chain IDs to constants:
+const ARBITRUM_TO_ETHEREUM_CHAIN_ID = 11155111; // Sepolia +const ARBITRUM_TO_GNOSIS_CHAIN_ID = 10200; // Gnosis Chiado export const getTransactionHandler = (chainId: number, network: Network) => { - if (chainId === 11155111) { + if (chainId === ARBITRUM_TO_ETHEREUM_CHAIN_ID) { // ... - } else if (chainId === 10200) { + } else if (chainId === ARBITRUM_TO_GNOSIS_CHAIN_ID) { // ...This improves readability and makes chain ID values easier to maintain.
validator-cli/src/utils/transactionHandlers/arbToEthHandler.ts (1)
18-21: Remove unnecessary constructor.The constructor only calls
super(opts)without additional logic, making it redundant.- constructor(opts: BaseTransactionHandlerConstructor) { - super(opts); - }🧰 Tools
🪛 Biome (1.9.4)
[error] 18-21: This constructor is unnecessary.
Unsafe fix: Remove the unnecessary constructor.
(lint/complexity/noUselessConstructor)
validator-cli/src/utils/transactionHandlers/arbToGnosisHandler.ts (2)
22-24: Remove the no-op constructorThe subclass does not add any custom logic — it only forwards
optstosuper.
Keeping an empty constructor increases noise and was flagged by Biome.- constructor(opts: BaseTransactionHandlerConstructor) { - super(opts); - } + // No constructor needed – BaseTransactionHandler already handles the props🧰 Tools
🪛 Biome (1.9.4)
[error] 22-24: This constructor is unnecessary.
Unsafe fix: Remove the unnecessary constructor.
(lint/complexity/noUselessConstructor)
68-74:maxPriorityFeePerGasMEVcan be zero – guard against invalid 0-gwei txFor small
depositvalues,maxFeePerGasProfitablemay underflow to0n, causing the subsequent transaction to revert.Consider bounding the value:
const maxFeePerGasProfitable = deposit / (toBigInt(gasEstimate) * 6n); const minTip = 1n * 10n ** 9n; // 1 gwei const safeMaxFee = maxFeePerGasProfitable === 0n ? minTip : maxFeePerGasProfitable;and use
safeMaxFeein the transaction.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (15)
validator-cli/src/helpers/claimer.ts(7 hunks)validator-cli/src/helpers/validator.test.ts(5 hunks)validator-cli/src/helpers/validator.ts(1 hunks)validator-cli/src/utils/claim.test.ts(4 hunks)validator-cli/src/utils/claim.ts(6 hunks)validator-cli/src/utils/epochHandler.ts(1 hunks)validator-cli/src/utils/ethers.ts(0 hunks)validator-cli/src/utils/transactionHandlers/arbToEthHandler.test.ts(1 hunks)validator-cli/src/utils/transactionHandlers/arbToEthHandler.ts(1 hunks)validator-cli/src/utils/transactionHandlers/arbToGnosisHandler.test.ts(1 hunks)validator-cli/src/utils/transactionHandlers/arbToGnosisHandler.ts(1 hunks)validator-cli/src/utils/transactionHandlers/baseTransactionHandler.test.ts(1 hunks)validator-cli/src/utils/transactionHandlers/baseTransactionHandler.ts(1 hunks)validator-cli/src/utils/transactionHandlers/index.ts(1 hunks)validator-cli/src/watcher.ts(8 hunks)
💤 Files with no reviewable changes (1)
- validator-cli/src/utils/ethers.ts
🚧 Files skipped from review as they are similar to previous changes (7)
- validator-cli/src/utils/claim.test.ts
- validator-cli/src/utils/transactionHandlers/arbToGnosisHandler.test.ts
- validator-cli/src/helpers/validator.test.ts
- validator-cli/src/utils/transactionHandlers/arbToEthHandler.test.ts
- validator-cli/src/utils/claim.ts
- validator-cli/src/watcher.ts
- validator-cli/src/helpers/claimer.ts
🧰 Additional context used
🧠 Learnings (6)
validator-cli/src/utils/epochHandler.ts (3)
Learnt from: mani99brar
PR: kleros/vea#370
File: bridger-cli/src/utils/epochHandler.ts:38-50
Timestamp: 2024-12-11T08:52:23.697Z
Learning: The `getBlockNumberFromEpoch` function in `bridger-cli/src/utils/epochHandler.ts` is currently unused and will be addressed in a future issue.
Learnt from: mani99brar
PR: kleros/vea#370
File: bridger-cli/src/utils/epochHandler.ts:17-17
Timestamp: 2024-12-09T09:43:28.877Z
Learning: In `bridger-cli/src/utils/epochHandler.ts`, within the `getBlockNumberFromEpoch` function, the block difference used to calculate the average block time is always 1000. Therefore, dividing by 1000 directly is appropriate in this case.
Learnt from: mani99brar
PR: kleros/vea#396
File: validator-cli/src/utils/epochHandler.ts:67-74
Timestamp: 2025-03-31T09:03:06.510Z
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/utils/transactionHandlers/index.ts (2)
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#424
File: validator-cli/src/utils/transactionHandlers/arbToEthHandler.ts:128-131
Timestamp: 2025-06-05T12:17:22.893Z
Learning: In the BaseTransactionHandler class, all transaction properties are expected to be initialized to null. When subclasses like ArbToEthDevnetTransactionHandler use spread syntax to extend the transactions object (e.g., `{ ...this.transactions, devnetAdvanceStateTxn: null }`), there's no issue with state loss since the base transactions are null initially.
validator-cli/src/utils/transactionHandlers/arbToGnosisHandler.ts (1)
Learnt from: mani99brar
PR: kleros/vea#424
File: validator-cli/src/utils/transactionHandlers/arbToGnosisHandler.ts:26-39
Timestamp: 2025-06-05T12:17:19.221Z
Learning: In the Arbitrum to Gnosis bridge transaction handler, the WETH approval pattern approves tokens for multiple claims upfront (typically 10 claims) using `deposit * 10n` to avoid repeated approval transactions. The approval amount should be calculated as `deposit * numberOfClaims`, not `deposit * (deposit * numberOfClaims)`.
validator-cli/src/helpers/validator.ts (1)
Learnt from: mani99brar
PR: kleros/vea#424
File: validator-cli/src/helpers/validator.ts:71-74
Timestamp: 2025-06-05T12:17:17.603Z
Learning: The challenger functionality in validator-cli/src/helpers/validator.ts is currently only supported for Network.TESTNET. There is no MAINNET support yet, which is why the network is hardcoded to Network.TESTNET when fetching and creating transaction handlers.
validator-cli/src/utils/transactionHandlers/baseTransactionHandler.ts (2)
Learnt from: mani99brar
PR: kleros/vea#370
File: bridger-cli/src/utils/transactionHandler.ts:13-13
Timestamp: 2024-12-09T09:40:28.400Z
Learning: In `bridger-cli/src/utils/transactionHandler.ts`, the `veaOutbox` implementations differ for each chain, but a common interface should be defined for type checks to enhance type safety.
Learnt from: mani99brar
PR: kleros/vea#424
File: validator-cli/src/utils/transactionHandlers/baseTransactionHandler.ts:158-168
Timestamp: 2025-06-05T12:17:27.139Z
Learning: In `validator-cli/src/utils/transactionHandlers/baseTransactionHandler.ts`, when `ContractType.ROUTER` is used in `checkTransactionStatus`, the code is designed so that `veaRouterProvider` should always be defined. If it's undefined, the code should crash rather than being handled gracefully, following a fail-fast design pattern.
validator-cli/src/utils/transactionHandlers/arbToEthHandler.ts (1)
Learnt from: mani99brar
PR: kleros/vea#424
File: validator-cli/src/utils/transactionHandlers/arbToEthHandler.ts:128-131
Timestamp: 2025-06-05T12:17:22.893Z
Learning: In the BaseTransactionHandler class, all transaction properties are expected to be initialized to null. When subclasses like ArbToEthDevnetTransactionHandler use spread syntax to extend the transactions object (e.g., `{ ...this.transactions, devnetAdvanceStateTxn: null }`), there's no issue with state loss since the base transactions are null initially.
🧬 Code Graph Analysis (1)
validator-cli/src/utils/transactionHandlers/arbToGnosisHandler.ts (6)
validator-cli/src/utils/transactionHandlers/index.ts (3)
ArbToGnosisTransactionHandler(9-9)BaseTransactionHandler(11-11)ArbToGnosisDevnetTransactionHandler(10-10)validator-cli/src/utils/transactionHandlers/baseTransactionHandler.ts (3)
BaseTransactionHandlerConstructor(75-86)Transactions(59-70)Transaction(54-57)validator-cli/src/utils/ethers.ts (2)
getWallet(82-82)getWETH(86-86)relayer-cli/src/consts/bridgeRoutes.ts (1)
Network(88-88)validator-cli/src/utils/errors.ts (1)
ClaimNotSetError(61-61)validator-cli/src/utils/arbMsgExecutor.ts (1)
messageExecutor(82-82)
🪛 Biome (1.9.4)
validator-cli/src/utils/transactionHandlers/arbToGnosisHandler.ts
[error] 22-24: This constructor is unnecessary.
Unsafe fix: Remove the unnecessary constructor.
(lint/complexity/noUselessConstructor)
validator-cli/src/utils/transactionHandlers/arbToEthHandler.ts
[error] 18-21: This constructor is unnecessary.
Unsafe fix: Remove the unnecessary constructor.
(lint/complexity/noUselessConstructor)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: dependency-review
- GitHub Check: Scorecard analysis
- GitHub Check: Analyze (javascript)
- GitHub Check: test
🔇 Additional comments (10)
validator-cli/src/utils/epochHandler.ts (1)
67-67: Excellent improvement to use finalized blocks for epoch calculations.Changing from
"latest"to"final"provides better finality guarantees for epoch-to-block mapping calculations, which is crucial for bridge operations where transaction finality matters.validator-cli/src/utils/transactionHandlers/index.ts (2)
1-13: Clean module organization with proper exports.The imports and exports are well-structured, providing a clear entry point for transaction handler functionality.
15-17: Good interface extension for devnet functionality.The
IDevnetTransactionHandlerinterface properly extends the base interface to add devnet-specific state advancement functionality.validator-cli/src/utils/transactionHandlers/baseTransactionHandler.test.ts (3)
15-29: Excellent use of concrete subclass for testing abstract base class.The
DummyHandlerimplementation provides minimal stub implementations that allow testing the base class functionality without complex setup.
31-93: Comprehensive test setup with proper mocking.The test setup properly mocks all dependencies (providers, contracts, emitter) and provides realistic test data for thorough testing.
95-267: Thorough test coverage of transaction lifecycle methods.The test suite comprehensively covers all transaction status scenarios, timing constraints, and error conditions for the base handler functionality.
validator-cli/src/utils/transactionHandlers/arbToEthHandler.ts (4)
25-45: Well-implemented claim making with proper status checks and gas estimation.The method correctly handles transaction status verification, gas estimation, and event emission for the claim process.
87-103: Clean snapshot sending implementation.The method properly handles status checks and transaction recording for snapshot operations.
108-119: Good use of dependency injection for message executor.The
execFnparameter with default value allows for easy testing while maintaining the default behavior.
127-157: Proper devnet extension with state advancement functionality.The devnet handler correctly extends the base functionality and uses the spread operator appropriately (as confirmed by previous learning about base transactions being null initially).
PR-Codex overview
This PR primarily focuses on refactoring and enhancing the transaction handling logic for bridging between Ethereum and Arbitrum, introducing better structure, error handling, and new interfaces for transaction handlers.
Detailed summary
getBlockFromEpochto fetch final blocks instead of the latest.getBotPathfunction to return structured config objects.ArbToEthandArbToGnosis.Summary by CodeRabbit
New Features
Bug Fixes
Refactor
Tests
Chores