Feat/snapshot period #431
Open
Feat/snapshot period #431
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
✅ Deploy Preview for veascan ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
WalkthroughPer-network snapshot timing was added (snapshotSavingPeriod) and DEVNET epochPeriod reduced; bridgeRoutes exports were reorganized. Snapshot decision logic and tests now use the network-specific threshold. Transaction handlers and BaseTransactionHandler now gate submissions via a new toSubmitTransaction rule (skip when PENDING or NOT_FINAL). Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Validator
participant SnapshotHelper as snapshot.ts
participant BridgeRoutes as bridgeRoutes.ts
participant Storage as SnapshotStore
Validator->>SnapshotHelper: saveSnapshotIfNeeded(params)
SnapshotHelper->>BridgeRoutes: read epochPeriod, snapshotSavingPeriod[network]
SnapshotHelper->>SnapshotHelper: compute timeLeftForEpoch
alt timeLeftForEpoch > snapshotSavingPeriod[network]
SnapshotHelper-->>Validator: SNAPSHOT_WAITING (early return)
else proceed
SnapshotHelper->>Storage: fetchLastSavedMessage()
SnapshotHelper->>SnapshotHelper: isSnapshotNeeded(count, ...)
alt snapshot needed
SnapshotHelper-->>Validator: toSaveSnapshot = true (proceed to save)
else
SnapshotHelper-->>Validator: toSaveSnapshot = false
end
end
note over SnapshotHelper: isSnapshotNeeded now receives `count`
sequenceDiagram
autonumber
actor Bot
participant BaseHandler
participant Chain as L1/L2 Chain
Bot->>BaseHandler: performAction(trnx, contract, currentTime)
BaseHandler->>BaseHandler: toSubmitTransaction(trnx, contract, currentTime)
alt toSubmitTransaction == false (PENDING / NOT_FINAL)
BaseHandler-->>Bot: return (skip submission)
else
BaseHandler->>Chain: estimate/call tx
Chain-->>BaseHandler: receipt/events
BaseHandler-->>Bot: emit status update
end
note over BaseHandler: Applied to ArbToEth and ArbToGnosis handlers (devnet paths included)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ Finishing Touches
🧪 Generate unit tests
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
validator-cli/src/utils/transactionHandlers/baseTransactionHandler.ts (1)
165-167: ROUTER provider can be undefined; add explicit guardIf veaRouterProvider isn’t configured, provider.getTransactionReceipt will throw with a vague error. Fail fast with a clear message.
Apply:
- case ContractType.ROUTER: - provider = this.veaRouterProvider; - break; + case ContractType.ROUTER: + if (!this.veaRouterProvider) { + throw new Error(`Router provider not configured for chainId=${this.chainId}`); + } + provider = this.veaRouterProvider; + break;validator-cli/src/consts/bridgeRoutes.ts (1)
14-23: Type export bug:Bridgeis a type, not a value — current export breaks compilation
export { …, Bridge, … }attempts to re-export a type as a value. Export the interface at declaration or useexport type.Apply either of these fixes (pick one):
Option A — export at declaration, drop from list:
-interface Bridge { +export interface Bridge { chain: string; minChallengePeriod: number; sequencerDelayLimit: number; inboxRPC: string; outboxRPC: string; routerRPC?: string; routeConfig: { [key in Network]: RouteConfigs }; depositToken?: string; } @@ -export { bridges, getBridgeConfig, Bridge, Network, snapshotSavingPeriod }; +export { bridges, getBridgeConfig, Network, snapshotSavingPeriod };Option B — keep declaration local, export as type:
-export { bridges, getBridgeConfig, Bridge, Network, snapshotSavingPeriod }; +export { bridges, getBridgeConfig, Network, snapshotSavingPeriod }; +export type { Bridge };Also applies to: 102-102
🧹 Nitpick comments (7)
validator-cli/src/utils/transactionHandlers/baseTransactionHandler.ts (1)
192-297: DRY the status gating to avoid 5 copy-pastesCentralize “should submit?” logic to keep handlers consistent and easy to tweak.
Example:
// inside BaseTransactionHandler private shouldSubmit(status: TransactionStatus) { return status === TransactionStatus.NOT_MADE || status === TransactionStatus.EXPIRED; }Then replace each:
if (status === TransactionStatus.PENDING || status === TransactionStatus.NOT_FINAL || status === TransactionStatus.FINAL) return;with:
if (!this.shouldSubmit(status)) return;validator-cli/src/utils/transactionHandlers/arbToEthHandler.ts (1)
108-119: Optional: make exec function injection consistent with the base/interfaceThis method adds an injectable execFn (good for tests) but the abstract signature doesn’t expose it. Either add an optional execFn to the abstract method or keep resolution logic in a helper.
Possible base change:
- public abstract resolveChallengedClaim(sendSnapshotTxn: string): Promise<void>; + public abstract resolveChallengedClaim( + sendSnapshotTxn: string, + execFn?: typeof messageExecutor + ): Promise<void>;validator-cli/src/consts/bridgeRoutes.ts (1)
90-95: AnnotatesnapshotSavingPeriodfor safer keyed accessAdd an explicit Record type to catch missing keys at compile time.
-const snapshotSavingPeriod = { +const snapshotSavingPeriod: Record<Network, number> = { [Network.DEVNET]: 90, // 1m 30s [Network.TESTNET]: 600, // 10 mins };validator-cli/src/helpers/snapshot.test.ts (1)
188-198: Update misleading comment to match dynamic savingPeriodThe comment says “60 seconds…” but savingPeriod is pulled from config (90s for devnet now).
- const now = epochPeriod + epochPeriod - savingPeriod; // 60 seconds before the second epoch ends + const now = epochPeriod + epochPeriod - savingPeriod; // savingPeriod seconds before the second epoch endsvalidator-cli/src/helpers/snapshot.ts (3)
5-5: Consolidate duplicate import from bridgeRoutesMinor cleanup: import both Network and snapshotSavingPeriod in one statement.
-import { Network } from "../consts/bridgeRoutes"; +import { Network, snapshotSavingPeriod } from "../consts/bridgeRoutes"; ... -import { snapshotSavingPeriod } from "../consts/bridgeRoutes";
36-41: Per-network gate: add safe default and clamp to epochPeriodGood move applying the time-gate across networks. Add a defensive default so unknown/missing keys don’t disable the gate unintentionally, and clamp to epochPeriod to avoid odd configs. Also consider including network in the WAITING payload for observability.
- const timeElapsed = now % epochPeriod; - const timeLeftForEpoch = epochPeriod - timeElapsed; - - if (timeLeftForEpoch > snapshotSavingPeriod[network]) { - emitter.emit(BotEvents.SNAPSHOT_WAITING, timeLeftForEpoch); + const timeElapsed = now % epochPeriod; + const savingPeriod = + Math.min(epochPeriod, snapshotSavingPeriod[network] ?? epochPeriod); // default: allow immediate snapshot + const timeLeftForEpoch = epochPeriod - timeElapsed; + + if (timeLeftForEpoch > savingPeriod) { + emitter.emit(BotEvents.SNAPSHOT_WAITING, { timeLeftForEpoch, savingPeriod, network }); return { transactionHandler, latestCount: count }; }Verification checklist:
- Ensure snapshotSavingPeriod is typed as Record<Network, number> and has entries for all live networks.
- Add a boundary test for timeLeftForEpoch === savingPeriod (should proceed to snapshot checks).
71-73: Stale “+1” comment; verify off-by-one semantics for Graph fallbackThe comment says “adding 1,” but the code no longer does. Clarify intent to avoid regressions (duplicate snapshots or missed saves).
Option A — fix the comment (current logic kept):
- // adding 1 to the message index to get the last saved count - lastSavedCount = messageIndex; + // Graph returns last included message index; use it as-is for lastSavedCount + lastSavedCount = messageIndex;Option B — if contract semantics require count === index + 1, restore +1:
- // adding 1 to the message index to get the last saved count - lastSavedCount = messageIndex; + // Convert last included message index to count + lastSavedCount = messageIndex + 1;Suggested tests:
- Mock queryFilter to throw, set fetchLastSavedMessage → "msg-12", and assert:
- When veaInbox.count() === 13, snapshotNeeded is false if using +1; true if using index (confirm desired behavior).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (6)
validator-cli/src/consts/bridgeRoutes.ts(5 hunks)validator-cli/src/helpers/snapshot.test.ts(2 hunks)validator-cli/src/helpers/snapshot.ts(3 hunks)validator-cli/src/utils/transactionHandlers/arbToEthHandler.ts(5 hunks)validator-cli/src/utils/transactionHandlers/arbToGnosisHandler.ts(5 hunks)validator-cli/src/utils/transactionHandlers/baseTransactionHandler.ts(5 hunks)
🧰 Additional context used
🧠 Learnings (20)
📚 Learning: 2024-12-02T10:16:56.825Z
Learnt from: madhurMongia
PR: kleros/vea#359
File: validator-cli/src/ArbToEth/watcherArbToGnosis.ts:829-840
Timestamp: 2024-12-02T10:16:56.825Z
Learning: In the `validator-cli/src/ArbToEth/watcherArbToGnosis.ts` file, avoid adding redundant error handling in functions like `reconstructChallengeProgress` when error handling is already adequately managed.
Applied to files:
validator-cli/src/utils/transactionHandlers/arbToGnosisHandler.tsvalidator-cli/src/utils/transactionHandlers/arbToEthHandler.ts
📚 Learning: 2024-12-02T10:08:28.998Z
Learnt from: madhurMongia
PR: kleros/vea#359
File: validator-cli/src/ArbToEth/watcherArbToGnosis.ts:625-648
Timestamp: 2024-12-02T10:08:28.998Z
Learning: In `validator-cli/src/ArbToEth/watcherArbToGnosis.ts`, the function `findLatestL2BatchAndBlock` already handles the case when `fromArbBlock > latestBlockNumber`, so additional error handling for this case is unnecessary.
Applied to files:
validator-cli/src/utils/transactionHandlers/arbToGnosisHandler.tsvalidator-cli/src/utils/transactionHandlers/arbToEthHandler.ts
📚 Learning: 2024-11-26T08:37:47.591Z
Learnt from: madhurMongia
PR: kleros/vea#359
File: validator-cli/src/ArbToEth/watcherArbToGnosis.ts:829-1112
Timestamp: 2024-11-26T08:37:47.591Z
Learning: Refactoring the `reconstructChallengeProgress` function in `validator-cli/src/ArbToEth/watcherArbToGnosis.ts` into smaller functions is considered unnecessary abstraction.
Applied to files:
validator-cli/src/utils/transactionHandlers/arbToGnosisHandler.tsvalidator-cli/src/utils/transactionHandlers/arbToEthHandler.ts
📚 Learning: 2025-06-05T12:17:22.931Z
Learnt from: mani99brar
PR: kleros/vea#424
File: validator-cli/src/utils/transactionHandlers/arbToEthHandler.ts:128-131
Timestamp: 2025-06-05T12:17:22.931Z
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.
Applied to files:
validator-cli/src/utils/transactionHandlers/arbToGnosisHandler.tsvalidator-cli/src/utils/transactionHandlers/arbToEthHandler.tsvalidator-cli/src/utils/transactionHandlers/baseTransactionHandler.ts
📚 Learning: 2024-12-09T09:42:34.067Z
Learnt from: mani99brar
PR: kleros/vea#370
File: bridger-cli/src/utils/transactionHandler.ts:64-77
Timestamp: 2024-12-09T09:42:34.067Z
Learning: In the `TransactionHandler` class (`bridger-cli/src/utils/transactionHandler.ts`), it's acceptable to let methods like `makeClaim` fail without additional error handling.
Applied to files:
validator-cli/src/utils/transactionHandlers/arbToGnosisHandler.tsvalidator-cli/src/utils/transactionHandlers/arbToEthHandler.tsvalidator-cli/src/utils/transactionHandlers/baseTransactionHandler.ts
📚 Learning: 2024-12-02T10:08:22.848Z
Learnt from: madhurMongia
PR: kleros/vea#359
File: validator-cli/src/ArbToEth/watcherArbToGnosis.ts:1128-1131
Timestamp: 2024-12-02T10:08:22.848Z
Learning: In `validator-cli/src/ArbToEth/watcherArbToGnosis.ts`, adding graceful shutdown handling is unnecessary because the bot only has TCP connections to be closed, and both client and server are already resilient regarding TCP connections.
Applied to files:
validator-cli/src/utils/transactionHandlers/arbToGnosisHandler.tsvalidator-cli/src/utils/transactionHandlers/arbToEthHandler.ts
📚 Learning: 2024-11-27T04:18:05.872Z
Learnt from: mani99brar
PR: kleros/vea#344
File: validator-cli/src/ArbToEth/watcherArbToEth.ts:732-745
Timestamp: 2024-11-27T04:18:05.872Z
Learning: In `validator-cli/src/ArbToEth/watcherArbToEth.ts`, input validation inside the `hashClaim` function is unnecessary because the upper layer ensures valid input before calling it.
Applied to files:
validator-cli/src/utils/transactionHandlers/arbToGnosisHandler.tsvalidator-cli/src/utils/transactionHandlers/arbToEthHandler.ts
📚 Learning: 2024-11-25T09:05:35.087Z
Learnt from: madhurMongia
PR: kleros/vea#359
File: validator-cli/src/ArbToEth/watcherArbToGnosis.ts:342-351
Timestamp: 2024-11-25T09:05:35.087Z
Learning: The `retryOperation` function in `watcherArbToGnosis.ts` acts as the retry mechanism for the challenge transaction, so additional retry logic is unnecessary.
Applied to files:
validator-cli/src/utils/transactionHandlers/arbToGnosisHandler.ts
📚 Learning: 2024-11-25T09:05:40.703Z
Learnt from: madhurMongia
PR: kleros/vea#359
File: validator-cli/src/ArbToEth/watcherArbToGnosis.ts:454-457
Timestamp: 2024-11-25T09:05:40.703Z
Learning: In `validator-cli/src/ArbToEth/watcherArbToGnosis.ts`, when calculating `arbAverageBlockTime`, it's acceptable not to add error handling for division by zero when `blockLatestArb.timestamp` equals `blockoldArb.timestamp` because if it occurs, it indicates a major issue that requires investigation.
Applied to files:
validator-cli/src/utils/transactionHandlers/arbToGnosisHandler.tsvalidator-cli/src/utils/transactionHandlers/arbToEthHandler.ts
📚 Learning: 2024-12-02T10:08:28.998Z
Learnt from: madhurMongia
PR: kleros/vea#359
File: validator-cli/src/ArbToEth/watcherArbToGnosis.ts:625-648
Timestamp: 2024-12-02T10:08:28.998Z
Learning: Avoid adding logging within loops in `validator-cli/src/ArbToEth/watcherArbToGnosis.ts` that could lead to excessive log output.
Applied to files:
validator-cli/src/utils/transactionHandlers/arbToGnosisHandler.tsvalidator-cli/src/utils/transactionHandlers/arbToEthHandler.ts
📚 Learning: 2025-06-05T12:17:21.782Z
Learnt from: mani99brar
PR: kleros/vea#424
File: validator-cli/src/helpers/claimer.ts:72-76
Timestamp: 2025-06-05T12:17:21.782Z
Learning: In validator-cli/src/helpers/claimer.ts, the outboxStateRoot captured at the beginning of the checkAndClaim function won't change for the epoch being claimed, so there's no race condition concern when reusing it in makeClaim/makeClaimDevnet functions.
Applied to files:
validator-cli/src/utils/transactionHandlers/arbToGnosisHandler.tsvalidator-cli/src/utils/transactionHandlers/arbToEthHandler.tsvalidator-cli/src/utils/transactionHandlers/baseTransactionHandler.ts
📚 Learning: 2024-11-20T11:50:15.304Z
Learnt from: madhurMongia
PR: kleros/vea#359
File: contracts/src/test/ArbToGnosis/VeaInboxArbToGnosisMock.sol:27-38
Timestamp: 2024-11-20T11:50:15.304Z
Learning: In `VeaInboxArbToGnosisMock.sendSnapshot`, `epochPeriod` is guaranteed to be non-zero, so a check for `epochPeriod > 0` is unnecessary.
Applied to files:
validator-cli/src/helpers/snapshot.test.ts
📚 Learning: 2024-11-19T10:59:44.618Z
Learnt from: fcanela
PR: kleros/vea#344
File: validator-cli/src/ArbToEth/watcherArbToEth.ts:411-414
Timestamp: 2024-11-19T10:59:44.618Z
Learning: In the TypeScript codebase, particularly in `validator-cli/src/ArbToEth/watcherArbToEth.ts`, when there is no error fallback operation, errors should not be caught; they should be allowed to bubble up to enable proper error propagation and handling at higher levels.
Applied to files:
validator-cli/src/utils/transactionHandlers/arbToEthHandler.ts
📚 Learning: 2024-12-09T09:42:51.590Z
Learnt from: mani99brar
PR: kleros/vea#370
File: bridger-cli/src/utils/transactionHandler.ts:79-101
Timestamp: 2024-12-09T09:42:51.590Z
Learning: In the `bridger-cli/src/utils/transactionHandler.ts` file, within the `TransactionHandler` class, it's acceptable to let methods like `startVerification` fail without additional error handling.
Applied to files:
validator-cli/src/utils/transactionHandlers/baseTransactionHandler.ts
📚 Learning: 2024-12-09T09:40:49.098Z
Learnt from: mani99brar
PR: kleros/vea#370
File: bridger-cli/src/utils/transactionHandler.ts:131-144
Timestamp: 2024-12-09T09:40:49.098Z
Learning: In the `TransactionHandler` class, it's acceptable to let methods like `withdrawClaimDeposit` fail without additional error handling.
Applied to files:
validator-cli/src/utils/transactionHandlers/baseTransactionHandler.ts
📚 Learning: 2024-12-09T09:40:28.400Z
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.
Applied to files:
validator-cli/src/utils/transactionHandlers/baseTransactionHandler.ts
📚 Learning: 2025-01-06T06:27:38.796Z
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`.
Applied to files:
validator-cli/src/utils/transactionHandlers/baseTransactionHandler.ts
📚 Learning: 2024-12-10T08:00:35.645Z
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.
Applied to files:
validator-cli/src/consts/bridgeRoutes.ts
📚 Learning: 2024-12-09T10:53:55.715Z
Learnt from: mani99brar
PR: kleros/vea#370
File: bridger-cli/src/bridger.ts:25-109
Timestamp: 2024-12-09T10:53:55.715Z
Learning: In the `bridger-cli/src/bridger.ts` file, refactoring the main loop and adding additional error handling are not needed unless deemed necessary by the developer.
Applied to files:
validator-cli/src/consts/bridgeRoutes.ts
📚 Learning: 2025-01-06T06:24:17.556Z
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.
Applied to files:
validator-cli/src/consts/bridgeRoutes.ts
🧬 Code graph analysis (2)
validator-cli/src/helpers/snapshot.test.ts (1)
validator-cli/src/consts/bridgeRoutes.ts (1)
snapshotSavingPeriod(102-102)
validator-cli/src/helpers/snapshot.ts (1)
validator-cli/src/consts/bridgeRoutes.ts (1)
snapshotSavingPeriod(102-102)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Analyze (javascript)
🔇 Additional comments (2)
validator-cli/src/utils/transactionHandlers/arbToGnosisHandler.ts (1)
97-105: Router provider dependency: ensure Base null-check covers this pathresolveChallengedClaim uses ContractType.ROUTER; the Base null-check I suggested will make failures explicit if routerRPC/env is missing.
If you want, I can add a small test harness to simulate missing router provider and assert the error message.
validator-cli/src/consts/bridgeRoutes.ts (1)
42-49: Epoch period changes vs PR summary — confirm intended TESTNET valuesPR summary says TESTNET epoch reduced to 300s; current code keeps TESTNET at 7200 (Arb→Eth) and 3600 (Arb→Gnosis). Clarify which is correct and update summary or code accordingly.
Would you like me to prepare a follow-up diff if TESTNET should also be 300s?
Also applies to: 57-65
validator-cli/src/utils/transactionHandlers/arbToGnosisHandler.ts
Outdated
Show resolved
Hide resolved
validator-cli/src/utils/transactionHandlers/baseTransactionHandler.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (14)
validator-cli/src/utils/transactionHandlers/baseTransactionHandler.ts (5)
207-209: Guard relies on toSubmitTransaction — fix there prevents duplicate StartVerification after FINALNo change needed here once the base guard is corrected to skip FINAL.
235-237: Same: avoid re-verifying after FINAL via base guardProceed safely once toSubmitTransaction blocks FINAL.
259-265: Same: prevent duplicate withdrawClaimDeposit after FINALCovered by the base fix.
279-285: Same: prevent duplicate withdrawChallengeDeposit after FINALCovered by the base fix.
192-201: Do not submit when a prior tx is FINAL — include FINAL in the early-returnAllowing submissions after FINAL risks revert loops, wasted gas, and noisy logs. Only submit when status is NOT_MADE or EXPIRED.
Apply:
public async toSubmitTransaction( trnx: Transaction | null, contract: ContractType, currentTime: number ): Promise<boolean> { const status = await this.checkTransactionStatus(trnx, contract, currentTime); - if (status === TransactionStatus.PENDING || status === TransactionStatus.NOT_FINAL) return false; - return true; + return status === TransactionStatus.NOT_MADE || status === TransactionStatus.EXPIRED; }validator-cli/src/utils/transactionHandlers/arbToEthHandler.ts (5)
29-31: Avoid re-claim after FINAL via base guardOnce toSubmitTransaction returns false for FINAL, this won’t resubmit.
53-55: Avoid re-challenge after FINAL via base guardCovered by the base fix.
92-94: Avoid re-sending snapshot after FINAL via base guardCovered by the base fix.
106-108: Avoid re-executing snapshot after FINAL via base guardCovered by the base fix.
138-140: Avoid duplicate devnetAdvanceState after FINAL via base guardCovered by the base fix.
validator-cli/src/utils/transactionHandlers/arbToGnosisHandler.ts (4)
44-46: Avoid re-claim after FINAL via base guardCovered by the base fix.
60-62: Avoid re-challenge after FINAL via base guardCovered by the base fix.
88-90: Avoid re-sending snapshot after FINAL via base guardCovered by the base fix.
137-139: Avoid duplicate devnetAdvanceState after FINAL via base guardCovered by the base fix.
🧹 Nitpick comments (2)
validator-cli/src/consts/bridgeRoutes.ts (1)
90-95: Strongly type snapshotSavingPeriod to avoid index errorsPrevents TS from complaining when indexing by a
Networkvariable and documents intent.-const snapshotSavingPeriod = { +const snapshotSavingPeriod: Record<Network, number> = { [Network.DEVNET]: 90, // 1m 30s [Network.TESTNET]: 600, // 10 mins };validator-cli/src/helpers/snapshot.test.ts (1)
187-197: Fix misleading comment and simplify expressionThe comment says “60 seconds” but DEVNET savingPeriod is 90s. Also, 2*epochPeriod reads clearer.
- const now = epochPeriod + epochPeriod - savingPeriod; // 60 seconds before the second epoch ends + const now = 2 * epochPeriod - savingPeriod; // savingPeriod seconds before the second epoch ends
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (6)
validator-cli/src/consts/bridgeRoutes.ts(6 hunks)validator-cli/src/helpers/snapshot.test.ts(2 hunks)validator-cli/src/helpers/snapshot.ts(3 hunks)validator-cli/src/utils/transactionHandlers/arbToEthHandler.ts(5 hunks)validator-cli/src/utils/transactionHandlers/arbToGnosisHandler.ts(5 hunks)validator-cli/src/utils/transactionHandlers/baseTransactionHandler.ts(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- validator-cli/src/helpers/snapshot.ts
🧰 Additional context used
🧠 Learnings (23)
📚 Learning: 2024-12-09T09:42:34.067Z
Learnt from: mani99brar
PR: kleros/vea#370
File: bridger-cli/src/utils/transactionHandler.ts:64-77
Timestamp: 2024-12-09T09:42:34.067Z
Learning: In the `TransactionHandler` class (`bridger-cli/src/utils/transactionHandler.ts`), it's acceptable to let methods like `makeClaim` fail without additional error handling.
Applied to files:
validator-cli/src/utils/transactionHandlers/baseTransactionHandler.tsvalidator-cli/src/utils/transactionHandlers/arbToGnosisHandler.tsvalidator-cli/src/utils/transactionHandlers/arbToEthHandler.ts
📚 Learning: 2024-12-09T09:42:51.590Z
Learnt from: mani99brar
PR: kleros/vea#370
File: bridger-cli/src/utils/transactionHandler.ts:79-101
Timestamp: 2024-12-09T09:42:51.590Z
Learning: In the `bridger-cli/src/utils/transactionHandler.ts` file, within the `TransactionHandler` class, it's acceptable to let methods like `startVerification` fail without additional error handling.
Applied to files:
validator-cli/src/utils/transactionHandlers/baseTransactionHandler.ts
📚 Learning: 2025-06-05T12:17:22.931Z
Learnt from: mani99brar
PR: kleros/vea#424
File: validator-cli/src/utils/transactionHandlers/arbToEthHandler.ts:128-131
Timestamp: 2025-06-05T12:17:22.931Z
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.
Applied to files:
validator-cli/src/utils/transactionHandlers/baseTransactionHandler.tsvalidator-cli/src/utils/transactionHandlers/arbToGnosisHandler.tsvalidator-cli/src/utils/transactionHandlers/arbToEthHandler.ts
📚 Learning: 2024-12-09T09:40:28.400Z
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.
Applied to files:
validator-cli/src/utils/transactionHandlers/baseTransactionHandler.tsvalidator-cli/src/consts/bridgeRoutes.tsvalidator-cli/src/utils/transactionHandlers/arbToGnosisHandler.tsvalidator-cli/src/utils/transactionHandlers/arbToEthHandler.ts
📚 Learning: 2025-06-05T12:17:21.782Z
Learnt from: mani99brar
PR: kleros/vea#424
File: validator-cli/src/helpers/claimer.ts:72-76
Timestamp: 2025-06-05T12:17:21.782Z
Learning: In validator-cli/src/helpers/claimer.ts, the outboxStateRoot captured at the beginning of the checkAndClaim function won't change for the epoch being claimed, so there's no race condition concern when reusing it in makeClaim/makeClaimDevnet functions.
Applied to files:
validator-cli/src/utils/transactionHandlers/baseTransactionHandler.tsvalidator-cli/src/utils/transactionHandlers/arbToGnosisHandler.tsvalidator-cli/src/utils/transactionHandlers/arbToEthHandler.ts
📚 Learning: 2024-11-20T11:50:15.304Z
Learnt from: madhurMongia
PR: kleros/vea#359
File: contracts/src/test/ArbToGnosis/VeaInboxArbToGnosisMock.sol:27-38
Timestamp: 2024-11-20T11:50:15.304Z
Learning: In `VeaInboxArbToGnosisMock.sendSnapshot`, `epochPeriod` is guaranteed to be non-zero, so a check for `epochPeriod > 0` is unnecessary.
Applied to files:
validator-cli/src/helpers/snapshot.test.ts
📚 Learning: 2024-12-10T08:00:35.645Z
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.
Applied to files:
validator-cli/src/consts/bridgeRoutes.ts
📚 Learning: 2024-12-09T10:53:55.715Z
Learnt from: mani99brar
PR: kleros/vea#370
File: bridger-cli/src/bridger.ts:25-109
Timestamp: 2024-12-09T10:53:55.715Z
Learning: In the `bridger-cli/src/bridger.ts` file, refactoring the main loop and adding additional error handling are not needed unless deemed necessary by the developer.
Applied to files:
validator-cli/src/consts/bridgeRoutes.ts
📚 Learning: 2024-12-09T09:04:04.819Z
Learnt from: mani99brar
PR: kleros/vea#370
File: bridger-cli/src/utils/graphQueries.ts:36-58
Timestamp: 2024-12-09T09:04:04.819Z
Learning: In `bridger-cli/src/utils/graphQueries.ts` (TypeScript), the `getVerificationStatus` function is no longer needed and has been removed.
Applied to files:
validator-cli/src/consts/bridgeRoutes.ts
📚 Learning: 2024-12-11T08:52:17.062Z
Learnt from: mani99brar
PR: kleros/vea#370
File: bridger-cli/src/utils/epochHandler.ts:13-13
Timestamp: 2024-12-11T08:52:17.062Z
Learning: In `bridger-cli/src/utils/epochHandler.ts`, the `veaOutbox` parameter is intentionally typed as `any` because the script is designed to be agnostic for multiple contracts.
Applied to files:
validator-cli/src/consts/bridgeRoutes.ts
📚 Learning: 2024-12-16T09:22:57.434Z
Learnt from: mani99brar
PR: kleros/vea#383
File: contracts/test/integration/ArbToGnosis.ts:114-114
Timestamp: 2024-12-16T09:22:57.434Z
Learning: In the local Hardhat deployment for testing, `VeaInboxArbToGnosisMock` is deployed with the identifier name `VeaInboxArbToGnosis`, so it's appropriate to retrieve it using `getContract("VeaInboxArbToGnosis")` and cast it to `VeaInboxArbToGnosisMock` in `ArbToGnosis.ts`.
Applied to files:
validator-cli/src/consts/bridgeRoutes.ts
📚 Learning: 2025-01-27T04:12:05.847Z
Learnt from: mani99brar
PR: kleros/vea#388
File: validator-cli/src/ArbToEth/validator.ts:16-17
Timestamp: 2025-01-27T04:12:05.847Z
Learning: In the validator-cli, `any` type is intentionally used for `veaInbox` and `veaOutbox` parameters to maintain chain agnosticism. This is because different chains have their own specific contract types (e.g., VeaInboxArbToEth, VeaOutboxArbToEth, VeaOutboxArbToGnosis) and using union types would make the code more complex and less maintainable.
Applied to files:
validator-cli/src/consts/bridgeRoutes.ts
📚 Learning: 2025-01-06T06:24:17.556Z
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.
Applied to files:
validator-cli/src/consts/bridgeRoutes.ts
📚 Learning: 2024-12-02T10:16:56.825Z
Learnt from: madhurMongia
PR: kleros/vea#359
File: validator-cli/src/ArbToEth/watcherArbToGnosis.ts:829-840
Timestamp: 2024-12-02T10:16:56.825Z
Learning: In the `validator-cli/src/ArbToEth/watcherArbToGnosis.ts` file, avoid adding redundant error handling in functions like `reconstructChallengeProgress` when error handling is already adequately managed.
Applied to files:
validator-cli/src/utils/transactionHandlers/arbToGnosisHandler.tsvalidator-cli/src/utils/transactionHandlers/arbToEthHandler.ts
📚 Learning: 2024-12-02T10:08:28.998Z
Learnt from: madhurMongia
PR: kleros/vea#359
File: validator-cli/src/ArbToEth/watcherArbToGnosis.ts:625-648
Timestamp: 2024-12-02T10:08:28.998Z
Learning: In `validator-cli/src/ArbToEth/watcherArbToGnosis.ts`, the function `findLatestL2BatchAndBlock` already handles the case when `fromArbBlock > latestBlockNumber`, so additional error handling for this case is unnecessary.
Applied to files:
validator-cli/src/utils/transactionHandlers/arbToGnosisHandler.tsvalidator-cli/src/utils/transactionHandlers/arbToEthHandler.ts
📚 Learning: 2024-12-02T10:08:22.848Z
Learnt from: madhurMongia
PR: kleros/vea#359
File: validator-cli/src/ArbToEth/watcherArbToGnosis.ts:1128-1131
Timestamp: 2024-12-02T10:08:22.848Z
Learning: In `validator-cli/src/ArbToEth/watcherArbToGnosis.ts`, adding graceful shutdown handling is unnecessary because the bot only has TCP connections to be closed, and both client and server are already resilient regarding TCP connections.
Applied to files:
validator-cli/src/utils/transactionHandlers/arbToGnosisHandler.tsvalidator-cli/src/utils/transactionHandlers/arbToEthHandler.ts
📚 Learning: 2024-11-25T09:05:40.703Z
Learnt from: madhurMongia
PR: kleros/vea#359
File: validator-cli/src/ArbToEth/watcherArbToGnosis.ts:454-457
Timestamp: 2024-11-25T09:05:40.703Z
Learning: In `validator-cli/src/ArbToEth/watcherArbToGnosis.ts`, when calculating `arbAverageBlockTime`, it's acceptable not to add error handling for division by zero when `blockLatestArb.timestamp` equals `blockoldArb.timestamp` because if it occurs, it indicates a major issue that requires investigation.
Applied to files:
validator-cli/src/utils/transactionHandlers/arbToGnosisHandler.ts
📚 Learning: 2024-11-25T09:05:35.087Z
Learnt from: madhurMongia
PR: kleros/vea#359
File: validator-cli/src/ArbToEth/watcherArbToGnosis.ts:342-351
Timestamp: 2024-11-25T09:05:35.087Z
Learning: The `retryOperation` function in `watcherArbToGnosis.ts` acts as the retry mechanism for the challenge transaction, so additional retry logic is unnecessary.
Applied to files:
validator-cli/src/utils/transactionHandlers/arbToGnosisHandler.ts
📚 Learning: 2024-12-02T10:08:28.998Z
Learnt from: madhurMongia
PR: kleros/vea#359
File: validator-cli/src/ArbToEth/watcherArbToGnosis.ts:625-648
Timestamp: 2024-12-02T10:08:28.998Z
Learning: Avoid adding logging within loops in `validator-cli/src/ArbToEth/watcherArbToGnosis.ts` that could lead to excessive log output.
Applied to files:
validator-cli/src/utils/transactionHandlers/arbToGnosisHandler.tsvalidator-cli/src/utils/transactionHandlers/arbToEthHandler.ts
📚 Learning: 2024-11-27T04:18:05.872Z
Learnt from: mani99brar
PR: kleros/vea#344
File: validator-cli/src/ArbToEth/watcherArbToEth.ts:732-745
Timestamp: 2024-11-27T04:18:05.872Z
Learning: In `validator-cli/src/ArbToEth/watcherArbToEth.ts`, input validation inside the `hashClaim` function is unnecessary because the upper layer ensures valid input before calling it.
Applied to files:
validator-cli/src/utils/transactionHandlers/arbToEthHandler.ts
📚 Learning: 2024-11-26T08:37:47.591Z
Learnt from: madhurMongia
PR: kleros/vea#359
File: validator-cli/src/ArbToEth/watcherArbToGnosis.ts:829-1112
Timestamp: 2024-11-26T08:37:47.591Z
Learning: Refactoring the `reconstructChallengeProgress` function in `validator-cli/src/ArbToEth/watcherArbToGnosis.ts` into smaller functions is considered unnecessary abstraction.
Applied to files:
validator-cli/src/utils/transactionHandlers/arbToEthHandler.ts
📚 Learning: 2025-01-06T06:27:38.796Z
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`.
Applied to files:
validator-cli/src/utils/transactionHandlers/arbToEthHandler.ts
📚 Learning: 2024-12-09T10:54:09.943Z
Learnt from: mani99brar
PR: kleros/vea#370
File: bridger-cli/src/utils/claim.ts:42-51
Timestamp: 2024-12-09T10:54:09.943Z
Learning: In the `bridger-cli/src/utils/claim.ts` file, within the `fetchClaim` function, explicit error handling for provider calls and event queries is not required; errors should be allowed to propagate naturally.
Applied to files:
validator-cli/src/utils/transactionHandlers/arbToEthHandler.ts
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Analyze (javascript)
- GitHub Check: test
- GitHub Check: dependency-review
🔇 Additional comments (8)
validator-cli/src/consts/bridgeRoutes.ts (5)
14-23: Public Bridge interface export: OKInterface shape is clear and minimal.
33-36: Enum re-export pattern is fineDeclaring locally and re-exporting below keeps the public surface explicit.
69-88: Bridges mapping looks correctChain IDs, RPC/env usage, and route selections align with our supported networks (11155111, 10200).
102-102: Export surface: OKNamed exports are consistent and minimal.
42-42: Align TESTNET epochPeriod settings with PR specificationThe current
epochPeriodvalues invalidator-cli/src/consts/bridgeRoutes.tsdo not match the PR description (which specifies a 300 s epoch for TESTNET). Please confirm the intended TESTNET settings and update either the code or the PR description/tests accordingly.• validator-cli/src/consts/bridgeRoutes.ts
– Arbitrum→Ethereum TESTNET (line 48):epochPeriod: 7200
– Arbitrum→Gnosis TESTNET (line 64):epochPeriod: 3600• DEVNET entries are correctly set to 300 s (lines 42 and 57).
Suggested diff to apply if TESTNET should indeed be 300 s:
- // Arbitrum→Ethereum TESTNET - epochPeriod: 7200, + // Arbitrum→Ethereum TESTNET (per PR: 300 s) + epochPeriod: 300, - // Arbitrum→Gnosis TESTNET - epochPeriod: 3600, + // Arbitrum→Gnosis TESTNET (per PR: 300 s) + epochPeriod: 300,validator-cli/src/helpers/snapshot.test.ts (1)
1-1: Importing snapshotSavingPeriod: OKKeeps tests aligned with per-network config.
validator-cli/src/utils/transactionHandlers/baseTransactionHandler.ts (1)
298-300: Same: prevent duplicate saveSnapshot after FINAL; also confirm snapshotSavingPeriod is applied upstreamBase fix handles FINAL. Verify that snapshotSavingPeriod-based timing is enforced by the caller, since this method doesn’t enforce it.
validator-cli/src/utils/transactionHandlers/arbToGnosisHandler.ts (1)
101-103: Router-based status check: ensure veaRouterProvider is definedresolveChallengedClaim passes ContractType.ROUTER into toSubmitTransaction; checkTransactionStatus will use veaRouterProvider — confirm it’s always set for this path.
|
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
PR-Codex overview
This PR primarily focuses on enhancing the snapshot saving mechanism for different networks, particularly
DEVNET, by introducing a time limit for saving snapshots and modifying the logic around epoch periods.Detailed summary
snapshotSavingPeriodconstant forDEVNETandTESTNET.epochPeriodfrom 1800 to 300 seconds forDEVNETandTESTNET.saveSnapshotto check againstsnapshotSavingPeriodfor saving snapshots.isSnapshotNeededto align with new snapshot logic.toSubmitTransactionfunction for better readability and maintainability.Bridgeinterface to be exported.toSubmitTransactionacross various transaction handlers for status checking.Summary by CodeRabbit
New Features
Improvements
Tests
Chores