Fan out eth_getLogs requests for event filter OR-sets - #1368
Conversation
eth_getLogs expresses only one topic selection per request, so the RPC source previously rejected any selection that compressed to more than one topic selection (multiple wildcard events with filters, a normal event mixed with a filtered one, or a single event whose `where` is an OR of param groups). Fan out to one eth_getLogs per selection and merge the responses. getSelectionConfig now returns an array of log selections — filter-less events are still compressed into a single topic0 OR-set per address bucket (wildcard vs address-bound), so the common case stays at one request. getNextPage issues the requests in parallel and dedups the merged logs by (blockNumber, logIndex), since a log can satisfy more than one selection when a single event's filter is an OR of param groups. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01YHkGCPKogsoJjSGo4oKbpF
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 46 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughRpcSource now fans out ChangesRPC Multi-Selection Log Fan-out
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
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: 1
🧹 Nitpick comments (3)
packages/envio/src/sources/RpcSource.res (1)
288-304: 🩺 Stability & Availability | 🔵 Trivial | 🏗️ Heavy liftBound the
eth_getLogsfan-out concurrency.
Promise.allsends every generated selection at once. Large OR-sets can turn one page fetch into many simultaneous RPC calls, and retries will repeat the same burst. Consider chunking or applying a small concurrency limit before merging/deduping.🤖 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/envio/src/sources/RpcSource.res` around lines 288 - 304, The RpcSource.getLogs fan-out currently uses Promise.all over every selection, which can spike concurrent eth_getLogs calls and amplify retries. Update the logSelections handling in RpcSource.res to process selections in small chunks or with a bounded concurrency limit before merging the results, while keeping the existing Prometheus.SourceRequestCount increment and getLogs request shape intact.scenarios/test_codegen/test/helpers/MockIndexer.res (1)
954-954: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueNote:
dependsOnAddressesfield not synced with expliciteventFilters.When
eventFiltersis explicitly supplied, the eventConfig's owndependsOnAddressesfield (line 966) is still computed independently from the~dependsOnAddressesparam/isWildcard, regardless of whether the supplied filters areStaticorDynamic. Based on thegetLogSelectionsOrThrowlogic reviewed in RpcSource.res (routes byisWildcard, notdependsOnAddresses), this appears harmless today, but could confuse future test authors relying on this flag being accurate.Also applies to: 973-1004
🤖 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 `@scenarios/test_codegen/test/helpers/MockIndexer.res` at line 954, The eventConfig built in MockIndexer.res is computing dependsOnAddresses independently from the explicit eventFilters input, so the flag can drift from the actual filter mode. Update the eventConfig construction to derive dependsOnAddresses from the supplied eventFilters whenever it is provided, while preserving the existing fallback behavior for the ~dependsOnAddresses param and isWildcard path. Make sure the logic around the MockIndexer helper that assembles eventFilters and eventConfig stays consistent for both Static and Dynamic filters.scenarios/test_codegen/test/RpcSource_test.res (1)
1261-1377: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winStrengthen fan-out test to verify per-selection topic distinctness.
The test asserts
eth_getLogsis called twice and the result is deduped to one item, but never checks that the two requests actually carry different topic filters corresponding to the twowhereOR-groups. If a fan-out bug caused two identical requests to be issued (rather than one per distinct selection), this test would still pass since dedup would still collapse to one item.Consider asserting on
mock.requestsbodies to confirm the twoeth_getLogscalls used the expected distincttopics(e.g., one containing topic1, the other topic2), which would catch a regression where selections aren't actually fanned out correctly.🤖 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 `@scenarios/test_codegen/test/RpcSource_test.res` around lines 1261 - 1377, The fan-out test in RpcSource.getItemsOrThrow only checks request count and deduped output, so it can miss identical duplicate requests. Update the test to inspect mock.requests for the two eth_getLogs bodies and assert they carry distinct topic filters matching the two eventFilters OR-groups in eventConfig (one request with the topic1 selection, one with the topic2 selection). Keep the existing count and dedup assertions, but add body-level checks so the test verifies true per-selection fan-out.
🤖 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/envio/src/sources/RpcSource.res`:
- Around line 361-367: The topic selection logic in RpcSource.res is pooling
non-wildcard filters in a way that can detach a contract’s address from its own
filter set; update the selection flow around entries/getEventFiltersOrThrow so
each address-bound contract keeps its own topic filters coupled with its
contract address instead of reusing a shared query across contracts. In the code
that maps selections to addressesByContractNameGetAll and downstream routing,
ensure the event signature query and any restrictive where criteria stay bound
to the originating contract so a broad match from one contract cannot route logs
for another. Add a regression test with two address-bound contracts sharing the
same topic0 where only one has a restrictive where, and verify the filtered
contract does not emit the other contract’s logs.
---
Nitpick comments:
In `@packages/envio/src/sources/RpcSource.res`:
- Around line 288-304: The RpcSource.getLogs fan-out currently uses Promise.all
over every selection, which can spike concurrent eth_getLogs calls and amplify
retries. Update the logSelections handling in RpcSource.res to process
selections in small chunks or with a bounded concurrency limit before merging
the results, while keeping the existing Prometheus.SourceRequestCount increment
and getLogs request shape intact.
In `@scenarios/test_codegen/test/helpers/MockIndexer.res`:
- Line 954: The eventConfig built in MockIndexer.res is computing
dependsOnAddresses independently from the explicit eventFilters input, so the
flag can drift from the actual filter mode. Update the eventConfig construction
to derive dependsOnAddresses from the supplied eventFilters whenever it is
provided, while preserving the existing fallback behavior for the
~dependsOnAddresses param and isWildcard path. Make sure the logic around the
MockIndexer helper that assembles eventFilters and eventConfig stays consistent
for both Static and Dynamic filters.
In `@scenarios/test_codegen/test/RpcSource_test.res`:
- Around line 1261-1377: The fan-out test in RpcSource.getItemsOrThrow only
checks request count and deduped output, so it can miss identical duplicate
requests. Update the test to inspect mock.requests for the two eth_getLogs
bodies and assert they carry distinct topic filters matching the two
eventFilters OR-groups in eventConfig (one request with the topic1 selection,
one with the topic2 selection). Keep the existing count and dedup assertions,
but add body-level checks so the test verifies true per-selection fan-out.
🪄 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: 4fefffe9-2061-4d14-900e-05c56d7de1f8
📒 Files selected for processing (3)
packages/envio/src/sources/RpcSource.resscenarios/test_codegen/test/RpcSource_test.resscenarios/test_codegen/test/helpers/MockIndexer.res
A `where: false` event filter resolves to an empty selection set. The fan-out change makes getItemsOrThrow advance the block cursor without issuing an eth_getLogs (the pre-fan-out code threw instead). Pin that behavior end-to-end: no eth_getLogs request, no items, cursor advanced. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01YHkGCPKogsoJjSGo4oKbpF
The two near-identical push loops differed only in the address scope (None vs Some(addresses)). Fold them into a single pass over both buckets, skipping the address-bound bucket when the partition has no addresses. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01YHkGCPKogsoJjSGo4oKbpF
getLogSelectionsOrThrow recompressed the wildcard bucket on every batch even though, for a selection whose events don't depend on addresses (the wildcard partition), the result is identical every time. Gate on that and build the selections once, mirroring how HyperSyncSource precomputes its no-address selection. The address-dependent path is unchanged. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01YHkGCPKogsoJjSGo4oKbpF
The skip-all test asserted latestFetchedBlockNumber == 100, but the mock returned block number 100 for every eth_getBlockByNumber, so the assertion held regardless of which block the source loaded. Add MockRpcServer.makeWithParams and echo the requested block number so the assertion actually pins that the cursor advanced to the queried block. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01YHkGCPKogsoJjSGo4oKbpF
There was a problem hiding this comment.
🧹 Nitpick comments (1)
scenarios/test_codegen/test/helpers/MockRpcServer.res (1)
75-76: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDrop comments that just restate function behavior.
Both added comments describe exactly what the code already shows (JSON-RPC envelope construction, routing by method/params, echoing id back) without capturing any non-obvious constraint or invariant.
As per coding guidelines,
**/*.resfiles should "Default to writing no comments. A comment earns its place only when it explains something the code itself cannot show," and should not restate "what a function does."✏️ Suggested removal
-// Reply 200 with a JSON-RPC envelope whose `result` is routed by the request's -// `method` and `params`, echoing the request's `id` back. let makeWithParams = (~getResult: (~method: string, ~params: JSON.t) => JSON.t) =>-// Reply 200 with a JSON-RPC envelope whose `result` is routed by the request's -// `method`, echoing the request's `id` back. let make = (~getResult: string => JSON.t) => makeWithParams(~getResult=(~method, ~params as _) => getResult(method))Also applies to: 101-103
🤖 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 `@scenarios/test_codegen/test/helpers/MockRpcServer.res` around lines 75 - 76, Remove the redundant comments in MockRpcServer.res that only restate what the response-building logic already makes obvious; keep the code in the relevant reply helper untouched, and delete the descriptive note about the JSON-RPC envelope/result routing and echoed id since it adds no non-obvious invariant or constraint.Source: Coding guidelines
🤖 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.
Nitpick comments:
In `@scenarios/test_codegen/test/helpers/MockRpcServer.res`:
- Around line 75-76: Remove the redundant comments in MockRpcServer.res that
only restate what the response-building logic already makes obvious; keep the
code in the relevant reply helper untouched, and delete the descriptive note
about the JSON-RPC envelope/result routing and echoed id since it adds no
non-obvious invariant or constraint.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 22acc76e-de3d-486d-b762-f9cee0081e6c
📒 Files selected for processing (3)
packages/envio/src/sources/RpcSource.resscenarios/test_codegen/test/RpcSource_test.resscenarios/test_codegen/test/helpers/MockRpcServer.res
🚧 Files skipped from review as they are similar to previous changes (2)
- scenarios/test_codegen/test/RpcSource_test.res
- packages/envio/src/sources/RpcSource.res
The common case is one log selection; route it straight through a single eth_getLogs without the Array.map/Promise.all/dedup machinery the fan-out needs. Extract the request into a local queryLogs shared by both paths, and drop the now-redundant single-element guard in mergeAndDedupItems since the caller no longer reaches it with one selection. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01YHkGCPKogsoJjSGo4oKbpF
2 participants
Summary
Refactor RPC data source to issue multiple
eth_getLogsrequests when an event'swhereclause is an OR of parameter groups, enabling support for complex event filters. Previously, the code rejected such configurations; now it fans out to one request per filter group and deduplicates results by (blockNumber, logIndex).Key Changes
getSelectionConfignow returnsarray<logSelection>instead of a singlelogSelection, allowing oneeth_getLogsrequest per topic selectionmergeAndDedupItemsto handle logs matched by multiple selections (e.g., when a single event's filter is an OR of groups)addresses: Noneand addressed selections scoped to partition addressescompressTopicSelectionsstill folds filter-less events into a single topic0 OR-set per bucket, keeping the common case at one requestImplementation Details
getNextPagenow acceptslogSelections: array<logSelection>and issuesPromise.allover multipleeth_getLogscallsblockNumber-logIndex, which is unique per chainhttps://claude.ai/code/session_01YHkGCPKogsoJjSGo4oKbpF
Summary by CodeRabbit