Move RPC paging logic from ReScript to Rust - #1399
Conversation
Port getNextPage, mergeAndDedupItems, the query-timeout race, and the mutSuggestedBlockIntervals AIMD state (backoff-multiplicative shrink, additive acceleration, per-partition state with a source-wide max ceiling) from RpcSource.res into EvmRpcClient (packages/cli). The provider error-message classification (getSuggestedBlockIntervalFromExn, isResponseTooLargeError) moves with it since it drives the same retry decision and now runs directly against the error messages Rust already has. EvmRpcClient's constructor takes the resolved sourceSync knobs (passed from EvmChain.res's getSyncConfig via RpcSource.make) and owns the per-partition interval state internally. The napi surface adds one method, getNextPage: JS asks for a range and partition id, Rust decides the actual toBlock from its AIMD state, fans out one eth_getLogs per selection, dedups by (blockNumber, logIndex), races the whole thing against queryTimeoutMillis, and returns the toBlock it queried. On failure it throws a structured retry decision (JSON-encoded in the napi error) that RpcSource.res turns back into a Source.getItemsRetry. getLogs (single-shot, stateless) stays as-is for the existing unit tests and as the primitive getNextPage fans out over.
…tics - Remove the now-unused getLogs napi method/binding in favor of getNextPage everywhere (production and tests); EvmRpcClient_test.res and NativeDecoder.res now exercise decoding/error-propagation through getNextPage against a mock RPC server. - Make the sync-tuning knobs (initialBlockInterval, backoffMultiplicative, etc.) required on EvmRpcClientConfig/EvmRpcClient.make instead of optional-with-Rust-side-defaults — EvmChain.getSyncConfig is already the single source of truth for these defaults, so Rust just validates and uses what it's given. - Replace classify.rs's RangeRegexes struct + single LazyLock with individual named LazyLock<Regex> statics. - Trim comments that narrated the refactor or pointed at other files instead of documenting current behavior. - Give NativeDecoder's synthetic test logs distinct logIndex values, since the client now always dedups a page's items by (blockNumber, logIndex).
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThe EVM RPC client now supports adaptive paginated log fetching with provider-specific error classification, interval state, concurrent selection requests, deduplication, structured retry decisions, and updated Rescript integration and tests. ChangesEVM RPC adaptive paging
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant RpcSource
participant EvmRpcClient
participant fetch_page
participant eth_getLogs
RpcSource->>EvmRpcClient: getNextPage(params)
EvmRpcClient->>fetch_page: fetch log selections
fetch_page->>eth_getLogs: concurrent requests
eth_getLogs-->>fetch_page: logs or error
fetch_page-->>EvmRpcClient: items and request stats
EvmRpcClient-->>RpcSource: page response or retry payload
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
Comment |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
packages/cli/src/evm_rpc_source/classify.rs (2)
76-119: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRepeated provider-check blocks could be table-driven.
The nine near-identical
if let Some(n) = extract_positive_u64(&X, message) { return Some((n, true)); }blocks (plus the two fixed-value cases) duplicate the same pattern. Adding a new provider currently means copy-pasting another block; a small static table of(&Regex, Option<u64>)(fixed value orNoneto extract from a capture) iterated in order would reduce duplication and make it obvious the order matters.♻️ Sketch of a table-driven approach
enum RangeCase { Extract(&'static LazyLock<Regex>), Fixed(&'static LazyLock<Regex>, u64), } // Order matters: more specific patterns first. static STRUCTURAL_CASES: &[RangeCase] = &[ RangeCase::Extract(&BLOCK_RANGE_LIMIT), RangeCase::Extract(&ALCHEMY_RANGE), // ... RangeCase::Fixed(&BASE_RANGE, 2000), RangeCase::Fixed(&CHAINSTACK, 10000), ];🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/cli/src/evm_rpc_source/classify.rs` around lines 76 - 119, Refactor the repeated provider checks in suggested_block_interval_from_message into an ordered static table of regex cases, supporting both captured values and fixed intervals. Preserve the existing precedence exactly, including BASE_RANGE and CHAINSTACK placement, and iterate the table to return the first matching (interval, true) result.
29-68: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueCase-sensitivity is inconsistent with
TOO_LARGE_PATTERNS.
TOO_LARGE_PATTERNS(lines 13-21) all use the(?i)flag, but none of the range-suggestion regexes here do. If a provider varies casing (e.g. "Limited to a 1000 Blocks range" vs "limited to a 1000 blocks range"), the match silently fails and falls through to the generic backoff path instead of applying the structural cap. Worth double-checking whether these were captured verbatim from real provider responses and are stable, or whether(?i)should be added defensively.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/cli/src/evm_rpc_source/classify.rs` around lines 29 - 68, The range-suggestion patterns are case-sensitive unlike TOO_LARGE_PATTERNS, so provider message casing changes can bypass structural caps. Update every regex in the range-detection statics, including SUGGESTED_RANGE, BLOCK_RANGE_LIMIT, ALCHEMY_RANGE, CLOUDFLARE_RANGE, THIRDWEB_RANGE, BLOCKPI_RANGE, BASE_RANGE, MAX_ALLOWED_BLOCKS, BLAST_PAID, CHAINSTACK, COINBASE, PUBLIC_NODE, and HYPERLIQUID, to use the case-insensitive `(?i)` flag while preserving their existing matching behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/cli/src/evm_rpc_source/mod.rs`:
- Around line 205-221: Validate that params.from_block is less than or equal to
params.to_block_ceiling in the existing block-bounds validation before
converting them to u64; return the same mapped error with both values when the
ordering is invalid. Keep the to_block calculation in the surrounding RPC range
logic, including its defensive lower-bound behavior for valid inputs.
- Around line 161-178: Validate cfg.backoff_multiplicative while constructing
SyncConfig, requiring it to be finite and strictly between 0.0 and 1.0; return a
clear configuration error through the existing map_err path when invalid, and
only assign the validated value to backoff_multiplicative.
---
Nitpick comments:
In `@packages/cli/src/evm_rpc_source/classify.rs`:
- Around line 76-119: Refactor the repeated provider checks in
suggested_block_interval_from_message into an ordered static table of regex
cases, supporting both captured values and fixed intervals. Preserve the
existing precedence exactly, including BASE_RANGE and CHAINSTACK placement, and
iterate the table to return the first matching (interval, true) result.
- Around line 29-68: The range-suggestion patterns are case-sensitive unlike
TOO_LARGE_PATTERNS, so provider message casing changes can bypass structural
caps. Update every regex in the range-detection statics, including
SUGGESTED_RANGE, BLOCK_RANGE_LIMIT, ALCHEMY_RANGE, CLOUDFLARE_RANGE,
THIRDWEB_RANGE, BLOCKPI_RANGE, BASE_RANGE, MAX_ALLOWED_BLOCKS, BLAST_PAID,
CHAINSTACK, COINBASE, PUBLIC_NODE, and HYPERLIQUID, to use the case-insensitive
`(?i)` flag while preserving their existing matching behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 6fb9fc57-e975-4c17-b115-b370b35b4141
📒 Files selected for processing (8)
packages/cli/src/evm_rpc_source/classify.rspackages/cli/src/evm_rpc_source/interval.rspackages/cli/src/evm_rpc_source/mod.rspackages/envio/src/sources/EvmRpcClient.respackages/envio/src/sources/RpcSource.resscenarios/test_codegen/test/RpcSource_test.resscenarios/test_codegen/test/helpers/NativeDecoder.resscenarios/test_codegen/test/lib_tests/EvmRpcClient_test.res
💤 Files with no reviewable changes (1)
- scenarios/test_codegen/test/RpcSource_test.res
Add a table-driven describe block in RpcSource_test.res that feeds each real eth_getLogs error message (Alchemy, Cloudflare, Thirdweb, BlockPI, Base, Blast, Chainstack, Coinbase, PublicNode, Hyperliquid, 1RPC, evm-rpc.sei-apis.com, and a generic "retry with the range" hint) through a mock JSON-RPC server and asserts the resulting WithSuggestedToBlock decision — exercising the real napi boundary instead of hand-built exception objects.
- Reject backoffMultiplicative outside (0.0, 1.0) at client construction: a value >= 1.0 would silently defeat the AIMD shrink-on-failure contract (interval::shrink never actually shrinks). - Reject to_block_ceiling < from_block in get_next_page instead of silently clamping to_block back up via .max(from_block), which masked a caller bug by producing a range above the given ceiling.
…zk8u' into claude/paging-dedup-aimd-rust-2xzk8u
- Reject backoffMultiplicative of exactly 0.0 (message already claimed the open interval); the check also rejects NaN. - Require initialBlockInterval and intervalCeiling to be >= 1: a zero interval made fromBlock + interval - 1 underflow. - Surface a 'Query took longer than Nms' message on query timeout instead of a null errorMessage, restoring the old QueryTimout observability. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Kn4k8r4M4heiLxMFdJtU8L
Summary
Migrates the adaptive block interval (AIMD) paging logic and error classification from ReScript (
RpcSource.res) to Rust (EvmRpcClient), enabling the Rust CLI to manage RPC query pagination independently. The ReScript runtime now delegates paging decisions to the Rust client via a structured retry protocol.Key Changes
Rust-side paging: Implemented
get_next_pageinEvmRpcClientto decide the actualtoBlockfrom per-partition AIMD state, fan out concurrenteth_getLogsqueries, deduplicate results by(blockNumber, logIndex), and race against a query timeout. On failure, returns a structured retry decision (suggested block range or backoff parameters).Error classification: Extracted provider error message parsing and block-range suggestion logic into new
classify.rsmodule. Handles 15+ provider-specific error patterns (Alchemy, QuickNode, Chainstack, etc.) and deterministic "too many logs" responses.AIMD state management: New
interval.rsmodule tracks per-partition block intervals and source-wide structural caps. Implements additive increase (grow on success) and multiplicative decrease (shrink on failure) with proper clamping.Retry protocol:
EvmRpcClientthrows a napi error whose message is a JSON payload describing the retry decision:{ "kind": "Retry", "attemptedToBlock": int, "errorMessage": string | null, "requestStats": [{"method": string, "seconds": float}], "retry": { "tag": "WithSuggestedToBlock", "toBlock": int } | { "tag": "WithBackoff", "message": string, "backoffMillis": int } }ReScript integration: Updated
RpcSource.resto parse the retry protocol viaparseGetNextPageRetryError, removing ~400 lines of paging logic. UpdatedEvmRpcClient.resto acceptsyncConfigand callgetNextPageinstead ofgetLogs.Config plumbing:
EvmRpcClientConfignow includes sync-tuning knobs (initialBlockInterval,backoffMultiplicative,accelerationAdditive,intervalCeiling,backoffMillis,queryTimeoutMillis) resolved by ReScript'sEvmChain.getSyncConfig.Notable Implementation Details
tokio::time::timeout, which cancels in-flight requests on expiry.HashSetkeyed byblockNumber-logIndexto handle selections that overlap (e.g., OR'd param groups in a single event'swhereclause).getSuggestedBlockIntervalFromExnandisResponseTooLargeErrorare removed (logic now Rust-side).https://claude.ai/code/session_019FA1CWtGQEg7KRMx4NvVn5
Summary by CodeRabbit
getNextPage, replacing direct log fetching, with configurable sync/backoff/interval/timeout tuning.