Fix/handle sent snapshots #435
Conversation
✅ Deploy Preview for veascan ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
WalkthroughThis pull request introduces support for VEA outbox channels in snapshot and claim resolution logic, adjusts epoch period configurations, adds snapshot saving period constraints, refactors transaction submission checks, and includes snapshot hash verification against stored onchain data. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
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/watcher.ts (1)
144-170: Handler key mismatch causes repeated re-instantiation (state loss).You read with
transactionHandlers[currentEpoch](line 146) but store withtransactionHandlers[txnHandlerKey](line 168). This drops state between iterations and can trigger duplicate submissions.const txnHandlerKey = `${routeConfig[network].veaInbox.address}_${currentEpoch}`; const transactionHandler = - transactionHandlers[currentEpoch] || + transactionHandlers[txnHandlerKey] || new TransactionHandler({ network, epoch: currentEpoch, veaInbox, veaOutbox, veaInboxProvider, veaOutboxProvider, veaRouterProvider, emitter, });validator-cli/src/helpers/snapshot.test.ts (1)
103-116: fetchLastSavedMessage mock shape likely incorrect vs production.isSnapshotNeeded expects an object with
{ id, stateRoot }(used to derive message index and last saved snapshot). Returning a string like "message-0" can mask issues.Please update test mocks to the expected shape.
- fetchLastSavedMessage = jest.fn().mockResolvedValue("message-0"); + fetchLastSavedMessage = jest.fn().mockResolvedValue({ + id: "message-0", + stateRoot: "0xdeadbeef", +});Based on snapshot.ts snippet.
🧹 Nitpick comments (15)
validator-cli/src/utils/graphQueries.ts (2)
164-171: Guard against empty results and limit nested results to 1 item.Accessing
result.snapshots[0].fallback[0]will throw when no snapshots/fallbacks exist. Also, you can ask The Graph for only the latest fallback.Apply:
- snapshots(where: {epoch: ${epoch}, inbox_: { id: "${veaInbox}" }}) { - fallback(orderBy: timestamp, orderDirection: desc){ + snapshots(where: {epoch: ${epoch}, inbox_: { id: "${veaInbox}" }}) { + fallback(first: 1, orderBy: timestamp, orderDirection: desc){ txHash } }And add a null-safe return:
- return result.snapshots[0].fallback[0]; + const [firstSnapshot] = result.snapshots ?? []; + const [firstFallback] = firstSnapshot?.fallback ?? []; + return firstFallback;
178-185: Consider backward-compatibility forstateRoot.If older snapshots lack
stateRoot, the static type should reflect that to avoid accidentalundefinedaccess at runtime.type SnapshotSavedResponse = { snapshots: { - stateRoot: string; + stateRoot?: string; messages: { id: string; }[]; }[]; };validator-cli/src/consts/bridgeRoutes.ts (3)
90-95: TypesnapshotSavingPeriodexplicitly and document units.Improves clarity and prevents accidental key drift.
-// For the remaining time in an epoch the bot should save snapshots -const snapshotSavingPeriod = { +// For the remaining time (seconds) in an epoch the bot should save snapshots +const snapshotSavingPeriod: Record<Network, number> = { [Network.DEVNET]: 90, // 1m 30s [Network.TESTNET]: 600, // 10 mins };
96-100: Improve error message and return the local variable.Tiny polish; aids debugging without adding new validation.
const getBridgeConfig = (chainId: number): Bridge => { const bridge = bridges[chainId]; - if (!bridge) throw new Error(`Bridge not found for chain`); - return bridges[chainId]; + if (!bridge) throw new Error(`Bridge not found for chainId=${chainId}`); + return bridge; };Based on learnings.
14-23: Optional: tightenBridgeshapes (env-var fields).If
strictNullChecksis on,process.env.*arestring | undefined. Consider asserting at load time or widening types here tostring | undefinedand validate upstream. Leaving as-is is acceptable if upper layers already validate.Based on learnings.
validator-cli/src/watcher.ts (2)
17-17: Make RPC block range configurable (and possibly per-chain).100 may be too conservative/too aggressive depending on provider. Suggest env override and per-network defaults.
-const RPC_BLOCK_LIMIT = 100; // RPC_BLOCK_LIMIT is the limit of blocks that can be queried at once +const RPC_BLOCK_LIMIT = Number(process.env.RPC_BLOCK_LIMIT ?? 100); // blocks per getLogs/queryOptionally, add a per-chain map if providers differ.
139-141: Optional: derivecurrentEpochfrom chain time, not local clock.Local clock skew can misalign epoch. You already have outbox timestamps available.
-const currentEpoch = Math.floor(Date.now() / (1000 * routeConfig[network].epochPeriod)); +const latestOutboxBlock = await veaOutboxProvider.getBlock("latest"); +const currentEpoch = Math.floor(latestOutboxBlock.timestamp / routeConfig[network].epochPeriod);validator-cli/README.md (2)
32-33: Example command should include the script path for consistency.Use the same invocation style in both examples to avoid confusion.
-`pm2 start -- --saveSnapshot` +`pm2 start dist/watcher.js -- --saveSnapshot`
18-27: Clarify defaults and flag interactions.Document the default for --path and how --saveSnapshot behaves in challenger-only mode (ignored or error).
- Add: “Default: --path=both.”
- Add: “--saveSnapshot is only used when the bridger path is active.”
validator-cli/src/utils/transactionHandlers/arbToEthHandler.ts (1)
63-67: Guard against zero/too‑low fee caps from profitability heuristic.If deposit/(gas*6) underflows to 0, tx may never land. Provide a sane floor or fallback to provider fee data.
-const maxFeePerGas = deposit / (toBigInt(gasEstimate) * BigInt(6)); -let maxPriorityFeePerGas = BigInt(6_667_000_000_000); +const profitabilityCap = deposit / (toBigInt(gasEstimate) * BigInt(6)); +const minFee = BigInt(1_000_000_000); // 1 gwei floor; tune per network +const maxFeePerGas = profitabilityCap > 0n ? profitabilityCap : minFee; +let maxPriorityFeePerGas = 6_667_000_000_000n; if (maxPriorityFeePerGas > maxFeePerGas) { maxPriorityFeePerGas = maxFeePerGas; }Confirm deposit is in wei (bigint) for all routes so the division stays in consistent units.
validator-cli/src/utils/claim.test.ts (1)
97-99: Typo in comment.There’s a stray “ß” in the VerificationStarted comment; remove to avoid noise.
- .mockImplementationOnce(() => Promise.resolve([])); // For VerificationStartedß + .mockImplementationOnce(() => Promise.resolve([])); // For VerificationStartedvalidator-cli/src/utils/transactionHandlers/arbToGnosisHandler.ts (1)
68-75: Fee cap heuristic: add floor/ceil to avoid non‑includable txs.Same concern as Arb→Eth: profitability division can yield 0; set a minimum and optionally cap priority fee.
-const maxFeePerGasProfitable = deposit / (toBigInt(gasEstimate) * BigInt(6)); -let maxPriorityFeePerGasMEV = BigInt(6667000000000); +const cap = deposit / (toBigInt(gasEstimate) * 6n); +const minFee = 1_000_000_000n; // 1 gwei floor +const maxFeePerGasProfitable = cap > 0n ? cap : minFee; +let maxPriorityFeePerGasMEV = 6_667_000_000_000n; if (maxPriorityFeePerGasMEV > maxFeePerGasProfitable) { maxPriorityFeePerGasMEV = maxFeePerGasProfitable; }Verify deposit unit is wei across networks to ensure fee math correctness.
validator-cli/src/helpers/snapshot.test.ts (2)
124-125: Prefer ethers.ZeroHash over "0x0" to match contract zeros.Literal "0x0" may not equal ZeroHash. Use the canonical constant to prevent false negatives.
- veaInbox.snapshots.mockResolvedValue("0x0"); + const { ZeroHash } = await import("ethers"); + veaInbox.snapshots.mockResolvedValue(ZeroHash);
193-201: Test name reads opposite of its assertion.It asserts that no snapshot is saved when snapshotNeeded is false. Rename for clarity.
-it("should not save snapshot if snapshot is needed", async () => { +it("should not save snapshot if snapshot is NOT needed", async () => {validator-cli/src/helpers/validator.ts (1)
77-85: Early-exit is fine; add stale-claim guard before withdrawing.
claim.honest !== 0short‑circuits correctly, butclaimmay be stale. Before callingwithdrawChallengeDeposit(Line 81), re‑fetchhonestor rely on a quick resolve-state check to avoid unnecessary reverts if state flipped post-fetch.Would you prefer a lightweight re-query of
claimHashes/claimhere, or to let the handler’s idempotency handle no-ops?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
validator-cli/README.md(2 hunks)validator-cli/src/consts/bridgeRoutes.ts(6 hunks)validator-cli/src/helpers/snapshot.test.ts(11 hunks)validator-cli/src/helpers/snapshot.ts(4 hunks)validator-cli/src/helpers/validator.ts(5 hunks)validator-cli/src/utils/botEvents.ts(1 hunks)validator-cli/src/utils/claim.test.ts(5 hunks)validator-cli/src/utils/claim.ts(5 hunks)validator-cli/src/utils/graphQueries.ts(3 hunks)validator-cli/src/utils/logger.ts(1 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)validator-cli/src/watcher.ts(2 hunks)
🧰 Additional context used
🧠 Learnings (11)
📚 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/arbToEthHandler.tsvalidator-cli/src/utils/transactionHandlers/arbToGnosisHandler.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/arbToEthHandler.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/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: 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/arbToEthHandler.tsvalidator-cli/src/helpers/snapshot.test.tsvalidator-cli/src/watcher.tsvalidator-cli/src/utils/claim.tsvalidator-cli/src/utils/transactionHandlers/arbToGnosisHandler.tsvalidator-cli/src/utils/claim.test.tsvalidator-cli/src/helpers/validator.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.tsvalidator-cli/src/helpers/snapshot.test.ts
📚 Learning: 2024-12-09T10:54:57.068Z
Learnt from: mani99brar
PR: kleros/vea#370
File: bridger-cli/src/utils/claim.ts:56-69
Timestamp: 2024-12-09T10:54:57.068Z
Learning: In the TypeScript file 'bridger-cli/src/utils/claim.ts', the upper layers handle input validation for the 'hashClaim' function. Therefore, explicit input validation within 'hashClaim' is not necessary.
Applied to files:
validator-cli/src/utils/claim.ts
📚 Learning: 2024-12-10T04:59:10.224Z
Learnt from: mani99brar
PR: kleros/vea#370
File: bridger-cli/src/utils/claim.ts:23-32
Timestamp: 2024-12-10T04:59:10.224Z
Learning: In `bridger-cli/src/utils/claim.ts`, within the `fetchClaim` function, when `claimData` is undefined, avoid initializing it with default values, as this can result in incorrect claim values and make issues harder to identify.
Applied to files:
validator-cli/src/utils/claim.ts
📚 Learning: 2024-11-27T10:48:48.433Z
Learnt from: madhurMongia
PR: kleros/vea#359
File: validator-cli/src/ArbToEth/watcherArbToGnosis.ts:24-24
Timestamp: 2024-11-27T10:48:48.433Z
Learning: In the `validator-cli` codebase, both `arbitrumToEth` and `arbitrumToGnosis` modules use the same `ClaimStruct`, so importing `ClaimStruct` from either module is acceptable.
Applied to files:
validator-cli/src/utils/claim.tsvalidator-cli/src/utils/claim.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-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/consts/bridgeRoutes.ts
🧬 Code graph analysis (2)
validator-cli/src/utils/transactionHandlers/baseTransactionHandler.ts (1)
validator-cli/src/utils/errors.ts (1)
ClaimNotSetError(70-70)
validator-cli/src/helpers/snapshot.test.ts (2)
validator-cli/src/helpers/snapshot.ts (2)
isSnapshotNeeded(59-98)saveSnapshot(28-57)validator-cli/src/consts/bridgeRoutes.ts (2)
snapshotSavingPeriod(102-102)Network(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). (3)
- GitHub Check: Analyze (javascript)
- GitHub Check: dependency-review
- GitHub Check: test
🔇 Additional comments (13)
validator-cli/src/utils/logger.ts (1)
144-146: LGTM: added log for already-resolved claims.Event wiring and message are clear and consistent.
validator-cli/src/utils/botEvents.ts (1)
35-35: LGTM: new BotEvents member.
CLAIM_ALREADY_RESOLVEDis well-named and used by logger/validator.validator-cli/src/watcher.ts (1)
160-165: LGTM: passveaOutboxinto saveSnapshot.Matches the broader PR changes adding outbox-aware snapshot logic.
validator-cli/src/utils/claim.test.ts (2)
235-279: Good: pass veaOutbox and fetchSentSnapshotData through resolve-state paths.This aligns tests with production signatures and covers the cross‑channel check.
299-307: Nice negative test for incorrect snapshot hash.Covers the regression where a wrong snapshot was sent but execution remains pending.
validator-cli/src/utils/transactionHandlers/arbToGnosisHandler.ts (1)
44-46: Unified gating applied correctly.Early returns keep methods idempotent and avoid duplicate submissions.
Also applies to: 60-62, 88-90, 101-103, 137-139
validator-cli/src/helpers/snapshot.test.ts (2)
7-7: Good: include veaOutbox in tests.State root checks across inbox/outbox are now exercised.
Also applies to: 21-23
220-247: Devnet time-window logic LGTM.Dynamic calculation using snapshotSavingPeriod improves robustness across config changes.
validator-cli/src/utils/transactionHandlers/arbToEthHandler.ts (1)
29-31: Centralized gating pattern verified—no legacy status checks found outside base class.The script output confirms that direct status comparisons are properly encapsulated within
baseTransactionHandler.ts, and no orphanedTransactionStatus.NOT_MADE|EXPIREDor directcheckTransactionStatus()calls exist in handler implementations. The early-return pattern viatoSubmitTransaction()is consistently applied as intended.validator-cli/src/helpers/validator.ts (1)
106-112: veaOutbox propagation through resolve flow looks good.Signature and call-site updates are consistent, enabling cross‑channel checks during resolution.
Also applies to: 154-160, 168-174
validator-cli/src/helpers/snapshot.ts (2)
40-46: Time gating reads well.Epoch boundary wait using
snapshotSavingPeriod[network]is sensible to avoid early saves.
47-53: Good: include epochPeriod in decision; cross‑check with current outbox stateRoot.Passing
epochPeriodand checkinginbox.snapshots(epochNow)vsoutbox.stateRoot()is a solid additional guard.Also applies to: 89-96
validator-cli/src/utils/claim.ts (1)
66-74: Reject the review comment - it conflicts with the established codebase pattern.The current implementation is correct and aligns with the documented learning for this codebase. Here's why:
Contract Type:
veaOutboxis typed asany(line 24), not a properly typed contract interface.Current Implementation is Correct: The direct log property access correctly extracts event data:
claimLogs[0].data→_stateRoot(not indexed, stored in data field) ✓claimLogs[0].topics[1]→_claimer(indexed, topics[1]) ✓challengeLogs[0].topics[2]→_challenger(indexed, topics[2]) ✓Learning Applies: Per the retrieved learning from mani99brar (PR #396), "direct access to log properties is preferred when the contract variable is typed as
any, rather than using interface parsing methods like parseLog()." This is exactly the case here.Why parseLog() Isn't Recommended Here:
- The contract is
anytyped, so callingveaOutbox.interface.parseLog()would fail or be unreliable- Direct property access is the practical solution for
anytyped variables- The current implementation correctly handles the indexed/non-indexed field distinction
The code does not need changes. The review comment misses the specific context that this codebase intentionally uses direct log access for
anytyped contracts.Likely an incorrect or invalid review comment.
jaybuidl
added
Type: Feature🗿
Package: Bots
Added logic to handle failed resolution scenario and fallback for missed claims in
validator-cliFiles updated:
snapshot.ts: updated to save snapshot for now new messages if the inbox stateroot is not claimed on outbox.claim.ts: previously sent snapshot's claim is verified againt the outbox claim.validator.ts: outscoped challenge withdrawal transaction before resolving to prevent sending snapshots after a claim is resolved due to claimHash changes.graphQueries.ts: added stateRoot field for lastSavedSnapshot queryPR-Codex overview
This PR introduces enhancements to the
validator-cliproject, focusing on improving claim resolution handling, snapshot management, and event emissions for better bot functionality.Detailed summary
logger.ts.CLAIM_ALREADY_RESOLVEDevent inbotEvents.ts.RPC_BLOCK_LIMITfrom 1000 to 100 inwatcher.ts.claim.tsto verify claim hashes against sent snapshots.veaOutboxsupport in various functions and interfaces.snapshotSavingPeriodconstants.Summary by CodeRabbit
New Features
--saveSnapshotand--pathflags supporting challenger, bridger, or both modes.Bug Fixes
Documentation
Optimization