Extract indexing addresses into IndexingAddresses module - #1359
Merged
Conversation
…napshot
Routing previously looked a log's srcAddress up in a chain-wide
indexingAddresses dict that was snapshotted onto every query and
shallow-copied ({...dict}) on every dynamic-contract registration. For
factory indexers with millions of addresses this is O(N)-per-registration
churn plus multi-version retention (each in-flight query pins a full copy)
— a major driver of unbounded RSS growth.
Split routing into two concerns:
- Ownership (which contract a log belongs to) is now resolved from the
fetching partition's own address set via a reverse index
(contractNameByAddress), derived once in OptimizedPartitions.make and
referenced (not copied) by each query. EventRouter.get is ownership-only.
The wildcard partition has an empty index, so it can never claim an
address-bound contract's logs — fixing a latent dup/miss for events whose
signature is wildcard on one contract and address-bound on another.
- The effectiveStartBlock temporal gate moves into the codegen'd
clientAddressFilter (extended to check srcAddress for non-wildcard
events), run in handleQueryResult against the live global. The global
indexingAddresses is now a single mutable dict, mutated in place on
registration — no more snapshots, no more per-registration copies.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Uoh4bSbpWDtec3nxSKmaTH
17000.hypersync.xyz no longer resolves (HyperSync stopped serving Holesky), which fails the hypersync-health-check. Remove it from the HypersyncChain subenum so it's no longer probed or offered as a HyperSync data-source; it remains in the Network enum (still resolvable via explorer). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Uoh4bSbpWDtec3nxSKmaTH
FetchState.make derives contractNameByAddress for every partition and getNextQuery carries it on each query, so the expected partition and query literals in the lib tests must match the derived map instead of the empty placeholder. Derive it from each record's own addressesByContractName. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Uoh4bSbpWDtec3nxSKmaTH
The in-place mutation broke the immutable-version invariant that chained registrations and rollback rely on (reusing a prior fetchState's partitions / indexingAddresses), regressing the dynamic-contract partition tests and the TestIndexer process flow. Restore the per-registration shallowCopy so registration behaves exactly as before. The memory win that matters is preserved: queries no longer carry a contractNameByAddress derived from a chain-wide indexingAddresses snapshot, so in-flight queries don't pin full copies of the address index. The transient per-registration copy returns (same as main, not a regression); optimizing it further needs a structurally-shared map and can be done separately. Also fix one missed expectation: a partition spread that overrides addressesByContractName must override contractNameByAddress too. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Uoh4bSbpWDtec3nxSKmaTH
The non-wildcard clientAddressFilter now gates on srcAddress being a known indexing address. Simulated events for a contract not configured on the target chain synthesized a dummy srcAddress that wasn't registered, so the filter dropped them and handlers never ran. Derive the synthetic srcAddress from the provided contract's first configured address (dummy for wildcard events), and register the non-wildcard ones on the simulated chain so injected events pass the ownership check. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Uoh4bSbpWDtec3nxSKmaTH
A contract's address on another chain has no meaning for an event simulated on this chain, so drop the cross-chain fallback. When the contract isn't configured on the simulated chain the synthetic srcAddress is the dummy address, which is still registered for non-wildcard events so the event passes the ownership check. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Uoh4bSbpWDtec3nxSKmaTH
Move the chain-wide address index out of FetchState (where it was an immutable field copied on every registration) into a dedicated IndexingAddresses module with a typed interface and domain operations (make/get/size/register/rollback). ChainState now owns a single non-mutable dict mutated in place, so registration no longer allocates a fresh chain-wide dict and in-flight queries don't pin a snapshot. Address-count metric becomes pull-based: a new Metrics module hand-rolls the envio_indexing_addresses gauge from live indexer state at scrape time and merges it with the prom-client registry output, replacing the imperative Prometheus.IndexingAddresses gauge. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01E69LmmNeUcMeZKZNNpWHZJ
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
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 (3)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds ChangesIndexingAddresses ownership and state threading
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Comment |
Contributor
|
Caution Failed to replace (edit) comment. This is likely due to insufficient permissions or the comment being deleted. Error details |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (4)
packages/envio/src/sources/EventRouter.res (1)
31-35: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winKeep this comment to the invariant, not the refactor history.
Lines 31-33 narrate where ownership and the temporal gate moved. The valuable bit is the non-obvious wildcard/empty-index behavior; keep that and drop the refactor wording.
Suggested rewrite
- // Ownership only: resolve the owning contract from the partition's reverse - // index (the partition that fetched the log), not a chain-wide snapshot. The - // `effectiveStartBlock` temporal gate now lives in `clientAddressFilter`. The - // wildcard partition has an empty index → every log falls back to `wildcard`, - // so it can never claim an address-bound contract's logs. + // Wildcard partitions intentionally carry an empty reverse index: matching + // logs fall through to `wildcard` instead of being attributed to an + // address-bound contract.As per coding guidelines,
**/*.res: “Never narrate the refactor itself ... That belongs in the commit message, not the code.”🤖 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/EventRouter.res` around lines 31 - 35, The comment in EventRouter should describe only the invariant, not the refactor history. Update the block near the ownership resolution logic to keep the non-obvious wildcard/empty-index behavior, and remove narration about where ownership or the temporal gate moved; reference the EventRouter route/ownership handling so future readers understand that the wildcard partition’s empty index means it cannot claim address-bound logs.Source: Coding guidelines
packages/envio/src/ChainState.res (1)
422-422: 🗄️ Data Integrity & Integration | 🔵 Trivial | 🏗️ Heavy liftAvoid exposing the mutable address index directly.
IndexingAddresses.tis now the owner, but this accessor returns its mutabledict, so callers can bypassregister/rollbackand corrupt routing state. Prefer a snapshot/read-only view for external consumers.🤖 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/ChainState.res` at line 422, The ChainState.indexingAddresses accessor is exposing the mutable IndexingAddresses.dict directly, which lets callers mutate routing state outside register/rollback. Update the indexingAddresses function in ChainState.res to return a snapshot or read-only view from IndexingAddresses.t instead of the underlying dict, and keep direct mutation confined to the IndexingAddresses module’s register/rollback flow.scenarios/test_codegen/test/lib_tests/Metrics_test.res (1)
27-33: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd one positive
Metrics.collectcase forstate=Some(_).This file currently covers
renderGaugeand thestate=Nonepassthrough, but not the new behavior this PR introduces: merging liveenvio_indexing_addressessamples with the prom-client registry output. A regression in the stateful branch would still pass here.🤖 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/lib_tests/Metrics_test.res` around lines 27 - 33, Add a positive test for the stateful branch of Metrics.collect by covering state=Some(_): verify it merges live envio_indexing_addresses samples with the prom-client registry output instead of returning the base registry unchanged. Reuse the existing Metrics.collect and PromClient.defaultRegister setup in Metrics_test.res, and assert the collected result includes both the registry metrics and the gauge samples introduced by the new behavior.scenarios/test_codegen/test/lib_tests/CrossChainState_test.res (1)
100-106: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winBuild
IndexingAddresses.tthrough the module API instead ofUtils.magic.This fixture now hard-codes the abstract type’s current backing shape, so it can drift from the real constructor semantics without the test noticing. Using
IndexingAddresses.makehere keeps the test aligned with the production path.♻️ Proposed fix
- let indexingAddresses = - Dict.fromArray([ - ( - address->Address.toString, - ({contractName: "MockContract", address, registrationBlock: -1, effectiveStartBlock: 0}: Internal.indexingContract), - ), - ])->(Utils.magic: dict<Internal.indexingContract> => IndexingAddresses.t) + let indexingAddresses = IndexingAddresses.make( + ~contractConfigs=IndexingAddresses.makeContractConfigs( + ~eventConfigs=[(MockIndexer.evmEventConfig(~contractName="MockContract") :> Internal.eventConfig)], + ), + ~addresses=[ + { + Internal.address, + contractName: "MockContract", + registrationBlock: -1, + }, + ], + )As per coding guidelines,
**/*_test.res: Prefer Public module API for testing.🤖 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/lib_tests/CrossChainState_test.res` around lines 100 - 106, The test fixture is bypassing the public constructor for IndexingAddresses.t by using Utils.magic on the underlying dict shape. Update the CrossChainState_test.res setup to build the value via IndexingAddresses.make instead, using the existing Dict.fromArray data as input so the test stays aligned with the module’s API and constructor semantics.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.
Inline comments:
In `@packages/envio/src/FetchState.res`:
- Around line 1279-1283: The event filtering in FetchState.res is mixing
snapshot-based routing with the live shared indexingAddresses dict, which can
make a single response nondeterministically filter events. Update the query flow
so the address snapshot needed by Internal.Event.clientAddressFilter is captured
on query and passed through alongside query.contractNameByAddress, then have the
filter branch use that query-scoped snapshot instead of
indexingAddresses->IndexingAddresses.dict.
- Around line 1827-1830: The rollback path in FetchState.rollback mutates the
shared IndexingAddresses index before it has verified rollback can complete, so
preflight the rollback conditions first by checking that no partition still has
a fetchedBlock == None query or by validating rollbackPendingQueries behavior
before calling IndexingAddresses.rollback. If you keep the same flow, compute
the address rollback on a copy and only commit it after the fetch-state
partition/buffer rebuild succeeds, using the rollback and rollbackPendingQueries
symbols to locate the change.
In `@scenarios/test_codegen/test/lib_tests/FetchState_test.res`:
- Around line 1836-1838: The hand-built FetchState fixtures have
contractNameByAddress derived from the wrong address set, so it no longer
matches each partition’s addressesByContractName. Update the affected test
fixtures in FetchState_test.res so the contractNameByAddress field is derived
from the same partition-specific addressesByContractName used in that fixture,
including the recreated rollback partition and the partition "2" case, using
FetchState.deriveContractNameByAddress consistently.
---
Nitpick comments:
In `@packages/envio/src/ChainState.res`:
- Line 422: The ChainState.indexingAddresses accessor is exposing the mutable
IndexingAddresses.dict directly, which lets callers mutate routing state outside
register/rollback. Update the indexingAddresses function in ChainState.res to
return a snapshot or read-only view from IndexingAddresses.t instead of the
underlying dict, and keep direct mutation confined to the IndexingAddresses
module’s register/rollback flow.
In `@packages/envio/src/sources/EventRouter.res`:
- Around line 31-35: The comment in EventRouter should describe only the
invariant, not the refactor history. Update the block near the ownership
resolution logic to keep the non-obvious wildcard/empty-index behavior, and
remove narration about where ownership or the temporal gate moved; reference the
EventRouter route/ownership handling so future readers understand that the
wildcard partition’s empty index means it cannot claim address-bound logs.
In `@scenarios/test_codegen/test/lib_tests/CrossChainState_test.res`:
- Around line 100-106: The test fixture is bypassing the public constructor for
IndexingAddresses.t by using Utils.magic on the underlying dict shape. Update
the CrossChainState_test.res setup to build the value via IndexingAddresses.make
instead, using the existing Dict.fromArray data as input so the test stays
aligned with the module’s API and constructor semantics.
In `@scenarios/test_codegen/test/lib_tests/Metrics_test.res`:
- Around line 27-33: Add a positive test for the stateful branch of
Metrics.collect by covering state=Some(_): verify it merges live
envio_indexing_addresses samples with the prom-client registry output instead of
returning the base registry unchanged. Reuse the existing Metrics.collect and
PromClient.defaultRegister setup in Metrics_test.res, and assert the collected
result includes both the registry metrics and the gauge samples introduced by
the new 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: fe4c2540-7a0a-4594-9e0a-e2789e7d1974
📒 Files selected for processing (37)
packages/cli/src/config_parsing/chain_helpers.rspackages/envio/src/ChainState.respackages/envio/src/ChainState.resipackages/envio/src/EventConfigBuilder.respackages/envio/src/FetchState.respackages/envio/src/HandlerLoader.respackages/envio/src/IndexingAddresses.respackages/envio/src/IndexingAddresses.resipackages/envio/src/Main.respackages/envio/src/Metrics.respackages/envio/src/Prometheus.respackages/envio/src/SimulateItems.respackages/envio/src/TestIndexer.respackages/envio/src/sources/EventRouter.respackages/envio/src/sources/HyperFuelSource.respackages/envio/src/sources/HyperSyncSource.respackages/envio/src/sources/RpcSource.respackages/envio/src/sources/SimulateSource.respackages/envio/src/sources/Source.respackages/envio/src/sources/SourceManager.respackages/envio/src/sources/Svm.respackages/envio/src/sources/SvmHyperSyncSource.resscenarios/test_codegen/test/ClientAddressFilter_test.resscenarios/test_codegen/test/EventBlockFilter_test.resscenarios/test_codegen/test/IndexerState_test.resscenarios/test_codegen/test/RateLimit_test.resscenarios/test_codegen/test/RpcSource_test.resscenarios/test_codegen/test/SourceBlockHashes_test.resscenarios/test_codegen/test/SvmHyperSyncSource_test.resscenarios/test_codegen/test/helpers/MockIndexer.resscenarios/test_codegen/test/lib_tests/CrossChainState_test.resscenarios/test_codegen/test/lib_tests/EventRouter_test.resscenarios/test_codegen/test/lib_tests/FetchState_onBlock_test.resscenarios/test_codegen/test/lib_tests/FetchState_test.resscenarios/test_codegen/test/lib_tests/IndexerLoop_test.resscenarios/test_codegen/test/lib_tests/Metrics_test.resscenarios/test_codegen/test/lib_tests/SourceManager_test.res
Comment on lines
+1827
to
+1830
| let rollback = (fetchState: t, ~indexingAddresses: IndexingAddresses.t, ~targetBlockNumber) => { | ||
| // Step 1: Prune addresses registered after the target block; `addressesToRemove` | ||
| // drives partition pruning to match the surviving index. | ||
| let addressesToRemove = indexingAddresses->IndexingAddresses.rollback(~targetBlockNumber) |
Contributor
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Validate rollback preconditions before pruning the shared index.
Line 1830 mutates indexingAddresses in place before rollback has proved it can finish. If any partition still has an in-flight query, rollbackPendingQueries throws on Line 1813 and exits with the chain-wide address index already pruned but the fetch-state partitions/buffer still untouched. Please either preflight that no fetchedBlock == None queries remain before Line 1830, or compute the index rollback on a copy and commit it only after the fetch-state rebuild succeeds.
Suggested guard
let rollback = (fetchState: t, ~indexingAddresses: IndexingAddresses.t, ~targetBlockNumber) => {
+ let hasInflightQuery =
+ fetchState.optimizedPartitions.entities
+ ->Dict.valuesToArray
+ ->Array.some(p => p.mutPendingQueries->Array.some(pq => pq.fetchedBlock === None))
+
+ if hasInflightQuery {
+ JsError.throwWithMessage("Internal error: Must not have a fetching query during rollback")
+ }
+
// Step 1: Prune addresses registered after the target block; `addressesToRemove`
// drives partition pruning to match the surviving index.
let addressesToRemove = indexingAddresses->IndexingAddresses.rollback(~targetBlockNumber)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| let rollback = (fetchState: t, ~indexingAddresses: IndexingAddresses.t, ~targetBlockNumber) => { | |
| // Step 1: Prune addresses registered after the target block; `addressesToRemove` | |
| // drives partition pruning to match the surviving index. | |
| let addressesToRemove = indexingAddresses->IndexingAddresses.rollback(~targetBlockNumber) | |
| let rollback = (fetchState: t, ~indexingAddresses: IndexingAddresses.t, ~targetBlockNumber) => { | |
| let hasInflightQuery = | |
| fetchState.optimizedPartitions.entities | |
| ->Dict.valuesToArray | |
| ->Array.some(p => p.mutPendingQueries->Array.some(pq => pq.fetchedBlock === None)) | |
| if hasInflightQuery { | |
| JsError.throwWithMessage("Internal error: Must not have a fetching query during rollback") | |
| } | |
| // Step 1: Prune addresses registered after the target block; `addressesToRemove` | |
| // drives partition pruning to match the surviving index. | |
| let addressesToRemove = indexingAddresses->IndexingAddresses.rollback(~targetBlockNumber) |
🤖 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/FetchState.res` around lines 1827 - 1830, The rollback
path in FetchState.rollback mutates the shared IndexingAddresses index before it
has verified rollback can complete, so preflight the rollback conditions first
by checking that no partition still has a fetchedBlock == None query or by
validating rollbackPendingQueries behavior before calling
IndexingAddresses.rollback. If you keep the same flow, compute the address
rollback on a copy and only commit it after the fetch-state partition/buffer
rebuild succeeds, using the rollback and rollbackPendingQueries symbols to
locate the change.
…imedes-5lqych # Conflicts: # packages/envio/src/ChainState.res # packages/envio/src/EventConfigBuilder.res # packages/envio/src/FetchState.res # packages/envio/src/SimulateItems.res # packages/envio/src/sources/SourceManager.res # scenarios/test_codegen/test/ClientAddressFilter_test.res # scenarios/test_codegen/test/IndexerState_test.res # scenarios/test_codegen/test/lib_tests/FetchState_onBlock_test.res # scenarios/test_codegen/test/lib_tests/FetchState_test.res # scenarios/test_codegen/test/lib_tests/SourceManager_test.res
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/envio/src/FetchState.res (1)
1038-1073: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftKeep the earliest registration when the same address reappears for the same contract.
The live-index branch skips a newly-arrived earlier
effectiveStartBlock, and the intra-batch branch keeps the first-seen item even though the inline FIXME says source order has been wrong before. That can persist a later start block and permanently miss historical events for that address.🤖 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/FetchState.res` around lines 1038 - 1073, The duplicate-registration handling in FetchState.res is keeping the later seen start block instead of the earliest one for the same contract address. Update the logic around IndexingAddresses.get and the registeringAddresses switch so that when the same address and contractName reappear, the registration with the smaller effectiveStartBlock wins and replaces the existing entry rather than being skipped. Make the behavior consistent in both the live-index path and the intra-batch path, using the existing existingContract, registeringContract, and dcWithStartBlock checks to preserve the earliest block number.
🤖 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.
Outside diff comments:
In `@packages/envio/src/FetchState.res`:
- Around line 1038-1073: The duplicate-registration handling in FetchState.res
is keeping the later seen start block instead of the earliest one for the same
contract address. Update the logic around IndexingAddresses.get and the
registeringAddresses switch so that when the same address and contractName
reappear, the registration with the smaller effectiveStartBlock wins and
replaces the existing entry rather than being skipped. Make the behavior
consistent in both the live-index path and the intra-batch path, using the
existing existingContract, registeringContract, and dcWithStartBlock checks to
preserve the earliest block number.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 4401103d-05b4-4d8b-b16a-3be9d58ca8af
📒 Files selected for processing (9)
packages/envio/src/ChainState.respackages/envio/src/ChainState.resipackages/envio/src/FetchState.resscenarios/test_codegen/test/ClientAddressFilter_test.resscenarios/test_codegen/test/IndexerState_test.resscenarios/test_codegen/test/lib_tests/CrossChainState_test.resscenarios/test_codegen/test/lib_tests/FetchState_onBlock_test.resscenarios/test_codegen/test/lib_tests/FetchState_test.resscenarios/test_codegen/test/lib_tests/SourceManager_test.res
💤 Files with no reviewable changes (1)
- scenarios/test_codegen/test/lib_tests/SourceManager_test.res
🚧 Files skipped from review as they are similar to previous changes (4)
- packages/envio/src/ChainState.resi
- scenarios/test_codegen/test/lib_tests/CrossChainState_test.res
- scenarios/test_codegen/test/ClientAddressFilter_test.res
- packages/envio/src/ChainState.res
Drop the public dict escape hatch in favor of domain-specific functions (has/toArray/getContractAddresses; get/size already existed). ChainState exposes contractAddresses/numIndexingAddresses instead of the raw dict; Main and Metrics use those. The opaque type is now fully enforced — the only raw-dict handoff left is the precompiled clientAddressFilter, which does raw obj[srcAddress] access in generated JS and can't take the opaque type (Internal would cycle on IndexingAddresses); it uses a narrowly-named rawForFilter accessor. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01E69LmmNeUcMeZKZNNpWHZJ
Replace the lines-array + joinWith rendering with a %raw single-pass loop: indexed for, string += (V8 grows these as ConsStrings, no O(n^2) copy), direct tuple-slot access, hoisted line prefix, and int coercion via + instead of an Int.toString round-trip. Output is byte-identical (Metrics_test green). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01E69LmmNeUcMeZKZNNpWHZJ
Same single-pass shape without raw JS: a ref accumulator with `++` (compiles to string `+=`, grown as a ConsString — no lines array or join), an indexed `for ... getUnsafe` loop with the bound hoisted, and a hoisted line prefix. ReScript collapses the ref into a plain mutable local, so the emitted JS matches the previous %raw version (only an explicit .toString per chain differs). Output byte-identical; Metrics_test green. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01E69LmmNeUcMeZKZNNpWHZJ
IndexingAddresses.rollback collected pruned keys into a throwaway array and deleted them in a second pass. forEachWithKey is a `for..in`, so deleting the key currently being visited is safe (PgStorage already mutates a dict mid- iteration the same way), letting us drop the array allocation and the second pass. Behavior unchanged; FetchState_test rollback cases green. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01E69LmmNeUcMeZKZNNpWHZJ
has/toArray had no production callers — only tests used them, widening the public API for no benefit. Replace `has(k)` with `get(k)->Option.isSome`, and the one whole-index assertion with a size + targeted-get tuple compare. Every remaining IndexingAddresses export now has a real caller. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01E69LmmNeUcMeZKZNNpWHZJ
Contributor
|
Caution Failed to replace (edit) comment. This is likely due to insufficient permissions or the comment being deleted. Error details |
make resolved each registering address by looking it up in the prebuilt index, which is keyed by address alone — so when the same address is configured under two contract names, both registering entries collapsed to the last-written contract's ia (wrong contractName/effectiveStartBlock for the earlier one). Build the ia per addresses-entry from contractConfigs instead (as before the extract), which also makes the prebuilt ~indexingAddresses argument unnecessary — make now takes only ~contractConfigs + ~addresses, and ChainState keeps the index for its own ownership and the register/rollback/filter paths. Also trim the renderGauge comment to the one non-obvious bit (ConsString growth). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01E69LmmNeUcMeZKZNNpWHZJ
A pre-extraction commit added a contractNameByAddress field to the partition type, recomputed in OptimizedPartitions.make and seeded empty in every literal. main solved the same routing differently — SourceManager derives it lazily from query.addressesByContractName via the memoized deriveContractNameByAddress and never stores it on the partition — so the field is written everywhere and read nowhere. Remove the field, its recompute, and all literal seeds (source + test partition expectations); keep deriveContractNameByAddress, still used by SourceManager. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01E69LmmNeUcMeZKZNNpWHZJ
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
scenarios/test_codegen/test/lib_tests/FetchState_test.res (1)
3537-3605: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winThese tests never exercise contract-config
startBlock.
makeInitial()gives"Gravatar"astartBlock=None, andmakeDynContractRegistration(~blockNumber=...)only changesregistrationBlock. So both tests currently pass even ifderiveEffectiveStartBlockignores the configured contract start block entirely. The second case also uses the same contract name twice, so there is still only one contract-level start-block setting in play.Please build these fixtures with event configs that actually set
~startBlock=Some(...)and, if you want distinct configured starts, use different contract names.🤖 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/lib_tests/FetchState_test.res` around lines 3537 - 3605, The new FetchState tests are not actually exercising contract-config startBlock handling because makeInitial() leaves Gravatar with startBlock=None and makeDynContractRegistration() only changes registrationBlock. Update the fixtures used in FetchState.registerDynamicContracts and deriveEffectiveStartBlock tests so the event configs include real configured startBlock values (Some(...)), and use different contract names if you need separate configured starts; then assert against the effectiveStartBlock produced from those configured values.
♻️ Duplicate comments (2)
packages/envio/src/FetchState.res (2)
1269-1270: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftUse a query-scoped filter snapshot here.
Line 1270 still evaluates
clientAddressFilteragainst the live shared registry. If registration or rollback mutatesindexingAddresseswhile this response is in flight, the query’s address routing snapshot and param-address filtering can diverge. Capture the filter input onquerywhen the query is created and read that snapshot here.🤖 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/FetchState.res` around lines 1269 - 1270, Use a query-scoped snapshot for the address filter in FetchState.getFilterForQuery (the branch that calls clientAddressFilter), since it currently reads indexingAddresses->IndexingAddresses.rawForFilter from the live shared registry. Capture the raw filter input when the query is created in query, store it on the query-scoped state, and have this filter path read that stored snapshot instead of the mutable registry so routing and param filtering stay consistent.
1823-1826: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winPreflight rollback before mutating the shared address registry.
Line 1826 prunes
indexingAddressesbeforerollbackPendingQueriescan throw on in-flight queries. That can leave the chain-wide registry rolled back while partitions and buffer remain unchanged.Suggested guard
let rollback = (fetchState: t, ~indexingAddresses: IndexingAddresses.t, ~targetBlockNumber) => { + let hasInflightQuery = + fetchState.optimizedPartitions.entities + ->Dict.valuesToArray + ->Array.some(p => p.mutPendingQueries->Array.some(pq => pq.fetchedBlock === None)) + + if hasInflightQuery { + JsError.throwWithMessage("Internal error: Must not have a fetching query during rollback") + } + // Step 1: Prune addresses registered after the target block; `addressesToRemove` // drives partition pruning to match the surviving index. let addressesToRemove = indexingAddresses->IndexingAddresses.rollback(~targetBlockNumber)🤖 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/FetchState.res` around lines 1823 - 1826, The rollback flow in FetchState.rollback mutates the shared indexingAddresses too early, before rollbackPendingQueries and the rest of the partition/buffer cleanup are guaranteed to succeed. Rework the sequence so rollbackPendingQueries (and any other fallible preflight checks) runs first on the current state, then apply IndexingAddresses.rollback and the partition/buffer updates only after the preflight succeeds; use the rollback, rollbackPendingQueries, and IndexingAddresses.rollback symbols to keep the mutation order consistent.
🧹 Nitpick comments (2)
scenarios/test_codegen/test/lib_tests/FetchState_test.res (1)
1198-1206: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCollapse this into one expectation.
This is one logical outcome, but the test splits it into two asserts. That makes failures noisier and drifts from the repo test style.
As per coding guidelines,
**/*_test.res: Always use single assert to check the whole value instead of multiple asserts for every field.Suggested rewrite
- let hasAddress1 = - indexingAddresses->IndexingAddresses.get(mockAddress1->Address.toString)->Option.isSome - let hasAddress2 = - indexingAddresses->IndexingAddresses.get(mockAddress2->Address.toString)->Option.isSome - - t.expect(hasAddress1, ~message="Address1 should be registered").toBe(true) - t.expect( - hasAddress2, - ~message="Address2 should be registered even though Address1 (which came before it) was skipped", - ).toBe(true) + t.expect( + ( + indexingAddresses->IndexingAddresses.get(mockAddress1->Address.toString)->Option.isSome, + indexingAddresses->IndexingAddresses.get(mockAddress2->Address.toString)->Option.isSome, + ), + ~message="Both addresses should be registered even if an earlier DC in the item was skipped", + ).toEqual((true, true))🤖 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/lib_tests/FetchState_test.res` around lines 1198 - 1206, Collapse the two separate assertions in the FetchState test into a single expectation that checks the combined registration outcome. In the test around IndexingAddresses.get and the hasAddress1/hasAddress2 checks, replace the multiple t.expect calls with one assert over the whole value so the test matches the repo style for *_test.res files and keeps the logical outcome in one place.Source: Coding guidelines
packages/envio/src/FetchState.res (1)
1085-1088: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRewrite the stale ownership comment.
Line 1086 still says the address is tracked on
fetchState, but this PR moved ownership toIndexingAddresses. Keep the invariant, but update the owner to avoid misleading future changes. As per coding guidelines, “When refactoring, keep comments that still explain non-obvious behavior; drop or rewrite comments that described the old shape.”Suggested rewrite
- // already tracked on fetchState, either from the db on startup or + // already tracked on indexingAddresses, either from the db on startup or🤖 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/FetchState.res` around lines 1085 - 1088, The ownership comment above the `IndexingAddresses.get` lookup is stale: it still says the address is tracked on fetchState even though ownership moved to `IndexingAddresses`. Rewrite the comment near the `switch indexingAddresses->IndexingAddresses.get(...)` block to describe the current invariant and duplicate-prevention behavior using the new owner, and remove any wording that implies the old fetchState shape.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.
Outside diff comments:
In `@scenarios/test_codegen/test/lib_tests/FetchState_test.res`:
- Around line 3537-3605: The new FetchState tests are not actually exercising
contract-config startBlock handling because makeInitial() leaves Gravatar with
startBlock=None and makeDynContractRegistration() only changes
registrationBlock. Update the fixtures used in
FetchState.registerDynamicContracts and deriveEffectiveStartBlock tests so the
event configs include real configured startBlock values (Some(...)), and use
different contract names if you need separate configured starts; then assert
against the effectiveStartBlock produced from those configured values.
---
Duplicate comments:
In `@packages/envio/src/FetchState.res`:
- Around line 1269-1270: Use a query-scoped snapshot for the address filter in
FetchState.getFilterForQuery (the branch that calls clientAddressFilter), since
it currently reads indexingAddresses->IndexingAddresses.rawForFilter from the
live shared registry. Capture the raw filter input when the query is created in
query, store it on the query-scoped state, and have this filter path read that
stored snapshot instead of the mutable registry so routing and param filtering
stay consistent.
- Around line 1823-1826: The rollback flow in FetchState.rollback mutates the
shared indexingAddresses too early, before rollbackPendingQueries and the rest
of the partition/buffer cleanup are guaranteed to succeed. Rework the sequence
so rollbackPendingQueries (and any other fallible preflight checks) runs first
on the current state, then apply IndexingAddresses.rollback and the
partition/buffer updates only after the preflight succeeds; use the rollback,
rollbackPendingQueries, and IndexingAddresses.rollback symbols to keep the
mutation order consistent.
---
Nitpick comments:
In `@packages/envio/src/FetchState.res`:
- Around line 1085-1088: The ownership comment above the `IndexingAddresses.get`
lookup is stale: it still says the address is tracked on fetchState even though
ownership moved to `IndexingAddresses`. Rewrite the comment near the `switch
indexingAddresses->IndexingAddresses.get(...)` block to describe the current
invariant and duplicate-prevention behavior using the new owner, and remove any
wording that implies the old fetchState shape.
In `@scenarios/test_codegen/test/lib_tests/FetchState_test.res`:
- Around line 1198-1206: Collapse the two separate assertions in the FetchState
test into a single expectation that checks the combined registration outcome. In
the test around IndexingAddresses.get and the hasAddress1/hasAddress2 checks,
replace the multiple t.expect calls with one assert over the whole value so the
test matches the repo style for *_test.res files and keeps the logical outcome
in one place.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 55053f33-f5c3-434d-a0b4-bcdee994b308
📒 Files selected for processing (15)
packages/envio/src/ChainState.respackages/envio/src/ChainState.resipackages/envio/src/FetchState.respackages/envio/src/IndexingAddresses.respackages/envio/src/IndexingAddresses.resipackages/envio/src/Main.respackages/envio/src/Metrics.resscenarios/test_codegen/test/ClientAddressFilter_test.resscenarios/test_codegen/test/EventBlockFilter_test.resscenarios/test_codegen/test/IndexerState_test.resscenarios/test_codegen/test/lib_tests/CrossChainState_test.resscenarios/test_codegen/test/lib_tests/FetchState_onBlock_test.resscenarios/test_codegen/test/lib_tests/FetchState_test.resscenarios/test_codegen/test/lib_tests/IndexerLoop_test.resscenarios/test_codegen/test/lib_tests/SourceManager_test.res
💤 Files with no reviewable changes (7)
- scenarios/test_codegen/test/lib_tests/IndexerLoop_test.res
- scenarios/test_codegen/test/EventBlockFilter_test.res
- scenarios/test_codegen/test/lib_tests/SourceManager_test.res
- scenarios/test_codegen/test/IndexerState_test.res
- scenarios/test_codegen/test/lib_tests/CrossChainState_test.res
- scenarios/test_codegen/test/lib_tests/FetchState_onBlock_test.res
- scenarios/test_codegen/test/ClientAddressFilter_test.res
🚧 Files skipped from review as they are similar to previous changes (5)
- packages/envio/src/ChainState.resi
- packages/envio/src/IndexingAddresses.resi
- packages/envio/src/IndexingAddresses.res
- packages/envio/src/Metrics.res
- packages/envio/src/ChainState.res
numIndexingAddresses had a single caller (Metrics.collect) and duplicated the count already computed in toChainData. Drop it and read numAddresses off toChainData so there's one place that derives a chain's address count. Also remove an unused index build left in EventBlockFilter_test after make stopped taking ~indexingAddresses. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01E69LmmNeUcMeZKZNNpWHZJ
…lace Metrics.renderGauge now takes the per-chain dict plus a value getter and builds the exposition string in one pass, dropping the intermediate samples tuple array collect used to materialise. Rename IndexingAddresses.rollback to rollbackInPlace to make the mutation explicit at the call site (it deletes from the index in place). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01E69LmmNeUcMeZKZNNpWHZJ
rollbackInPlace already deletes rolled-back addresses, so "was this address removed?" is equivalent to "is it absent from the index?". Use IndexingAddresses.get for both the deleted-partition recreation and the kept-partition filter instead of threading a separate removed-set, and let rollbackInPlace return unit (no set allocation or unsafeFromString round-trips). One source of truth; behavior unchanged (FetchState_test rollback cases green). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01E69LmmNeUcMeZKZNNpWHZJ
FetchState.make and IndexingAddresses.make both built each address's ia with the same contractStartBlock lookup + deriveEffectiveStartBlock. Pull that into IndexingAddresses.makeIndexingAddress so the partition seed and the index can't drift on a future start-block change. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01E69LmmNeUcMeZKZNNpWHZJ
This was referenced Jul 24, 2026
Closed
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
What
Moves the chain-wide indexing-address index out of
FetchStateinto a dedicatedIndexingAddressesmodule, and makes the address-count metric pull-based.Why
registerDynamicContractspreviously didindexingAddresses->shallowCopy+mergeInPlaceon every registration, allocating a fresh chain-wide dict each time. The index lived as an immutable field onFetchState.t, so it was shared by reference with batch snapshots — which is why the earlier in-place-mutation attempt was reverted (it corrupted older versions that rollback/snapshots rely on).Pulling the index out removes that incidental coupling: it's now a single per-chain dict owned by
ChainState, mutated in place, so registration is O(added) instead of O(N) and in-flight queries don't pin a snapshot.Changes
IndexingAddressesmodule (.res/.resi): opaque type over a plain dict with domain operations —makeContractConfigs,make,get,size,dict,register,rollback,deriveEffectiveStartBlock. ThecontractConfig/indexingAddresstypes andderiveEffectiveStartBlockmoved here (one-way depFetchState → IndexingAddresses).FetchState: dropped theindexingAddressesfield andnumAddresses;make/registerDynamicContracts/handleQueryResult/rollbacktake~indexingAddressesand mutate via the module's domain methods. The index is built beforemakeand never mutated by it.ChainState: owns a non-mutableindexingAddresses: IndexingAddresses.t(the dict is mutated in place, so the reference is stable acrossfetchStateversions). Threaded into the mutators; accessor +toChainDataupdated.Metricsmodule (new):collecthand-rolls theenvio_indexing_addressesgauge from liveIndexerStatechain states at scrape time and merges it with the prom-client registry output. Removed the imperativePrometheus.IndexingAddressesgauge; rewiredMain's/metricshandler. Metric name/labels are preserved, so dashboards are unaffected.Why it's correct (no aliasing problem)
FetchState.rollbackderives surviving addresses byregistrationBlockand runs on the currentcs.fetchState; it never restores a snapshotted dict. AndhandleQueryResultalready registers into committedcsbefore reorg validation. So a single per-chain mutable dict matches the existing lifecycle — the field-on-record sharing with the batch snapshot was the only thing the reverted attempt broke.Testing
packages/envioandscenarios/test_codegenboth compile clean (0 warnings/errors).Metrics_testcovering the hand-rolled gauge format and the no-state path.One test (
Should split dcs into multiple partitions) did two independent registrations from the same base, relying on the old immutable-copy behaviour; it now resets the base + index for the second scenario, matching production semantics (the chain threads a single index forward).🤖 Generated with Claude Code
https://claude.ai/code/session_01E69LmmNeUcMeZKZNNpWHZJ
Generated by Claude Code
Summary by CodeRabbit
contractAddresses) andnumIndexingAddresses;chainData.numAddressesnow reflects the centralized index.envio_indexing_addressesmetric via/metricscollection.