Persist only processed DCs #752
Conversation
WalkthroughRenamed Batch.t field fetchStates to updatedFetchStates. Converted FetchState.dcsToStore from option to array and updated associated logic. ChainManager reworked batch formation to compute per-chain batchDcs and leftover dcs, propagating updatedFetchStates. Removed DynamicContractRegistry cleanup query and its usages/tests. Added Array.clearInPlace. Updated tests accordingly and added a new rollback test. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor App
participant ChainManager
participant FetchStates as ChainMap<FetchState>
participant DC as DynamicContracts
participant Batch
App->>ChainManager: createBatch(sizePerChain)
ChainManager->>FetchStates: iterate chains
loop per chain
ChainManager->>DC: evaluate dcs vs nextProgress pointers
alt include in current batch
ChainManager->>FetchStates: set dcsToStore := batchDcs
else defer
ChainManager->>FetchStates: set dcsToStore := leftDcsToStore
end
end
ChainManager->>Batch: emit { updatedFetchStates, items, ... }
sequenceDiagram
autonumber
actor Runtime
participant GlobalState
participant ChainManager
participant UpdateQueues
Runtime->>ChainManager: getNextBatch()
ChainManager-->>Runtime: Batch { updatedFetchStates, items, ... }
Runtime->>GlobalState: ProcessEventBatch(Batch)
GlobalState->>GlobalState: updatedFetchStates := Batch.updatedFetchStates
GlobalState->>UpdateQueues: dispatch(UpdateQueues, updatedFetchStates)
Note over GlobalState,UpdateQueues: Downstream consumers now read updatedFetchStates
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches🧪 Generate unit tests
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Please see the documentation for more information. Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 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 (9)
codegenerator/cli/npm/envio/src/FetchState.res (4)
1244-1247: Good: rollback filters pending DCs by removed addresses. Consider updating metrics.We drop DCs from both indexingContracts and dcsToStore; update Prometheus.IndexingAddresses for accuracy after rollback.
1289-1303: Avoid unsafe_get when maxBatchSize is 0.Current comment admits the hazard; add a fast path to prevent undefined access.
Apply:
let hasFullBatch = ({queue} as fetchState: t, ~maxBatchSize) => { + if maxBatchSize <= 0 { + false + } else { // Queue is ordered from latest to earliest, so the earliest eligible // item for a full batch of size B is at index (length - B). // Do NOT subtract an extra 1 here; when length === B we should still // classify the queue as full and probe index 0. let targetBlockIdx = queue->Array.length - maxBatchSize if targetBlockIdx < 0 { false } else { - // Unsafe can fail when maxBatchSize is 0, - // but we ignore the case - queue->Js.Array2.unsafe_get(targetBlockIdx)->Internal.getItemBlockNumber <= - fetchState->bufferBlockNumber + queue->Js.Array2.unsafe_get(targetBlockIdx) + ->Internal.getItemBlockNumber <= fetchState->bufferBlockNumber } } }
86-105: Optional: copy dcsToStore in copy(fetchState) for symmetry.Queue is copied; dcsToStore shares the underlying array. Copying avoids accidental external mutation.
- dcsToStore: fetchState.dcsToStore, + dcsToStore: fetchState.dcsToStore->Array.copy,
590-601: Guard against duplicate DCs in dcsToStore — merge by address and keep earliest startBlockConcatenating existing dcsToStore with the new batch can reintroduce duplicates across repeated registerDynamicContracts calls; merge entries by address and choose the earliest startBlock so writes remain idempotent.
Apply:
- ~dcsToStore=switch fetchState.dcsToStore { - | [] => dcsToStore - | existingDcs => Array.concat(existingDcs, dcsToStore) - }, + ~dcsToStore={ + let byAddr = Js.Dict.empty() + /* seed with existing buffered DCs */ + fetchState.dcsToStore->Array.forEach(dc => + byAddr->Js.Dict.set(dc.address->Address.toString, dc) + ) + /* merge new batch, keeping the entry with the smallest startBlock */ + dcsToStore->Array.forEach(dc => { + let key = dc.address->Address.toString + switch byAddr->Js.Dict.get(key) { + | None => byAddr->Js.Dict.set(key, dc) + | Some(existing) => + if dc.startBlock < existing.startBlock { + byAddr->Js.Dict.set(key, dc) + } else { + () + } + } + }) + byAddr->Js.Dict.values + },scenarios/test_codegen/test/helpers/Mock.res (1)
505-528: Simplify optional handler wiring.Use Option.map to reduce branching; behavior unchanged.
- handler: switch item.handler { - | Some(handler) => - ( - ({context} as args) => { - // We don't want preload optimization for the tests - if context.isPreload { - Promise.resolve() - } else { - handler(args) - } - } - )->( - Utils.magic: Types.HandlerTypes.loader<unit, unit> => option< - Internal.handler, - > - ) - - | None => None - }, + handler: + item.handler->Option.map(handler => + ( + ({context} as args) => + context.isPreload ? Promise.resolve() : handler(args) + )->( + Utils.magic: Types.HandlerTypes.loader<unit, unit> => + Internal.handler + ) + ),codegenerator/cli/templates/static/codegen/src/globalState/GlobalState.res (1)
955-958: Avoid scheduling no-op DC writes when all per-chain arrays are empty.Utils.Dict.size counts keys, not values. If ChainManager inserts keys with empty arrays, this branch still fires unnecessarily.
Apply this guard:
- if dcsToStoreByChainId->Utils.Dict.size > 0 { - let shouldSaveHistory = state.config->Config.shouldSaveHistory(~isInReorgThreshold) - inMemoryStore->InMemoryStore.setDcsToStore(dcsToStoreByChainId, ~shouldSaveHistory) - } + let hasAnyDcs = + dcsToStoreByChainId + ->Js.Dict.entries + ->Array.some(((_, dcs)) => dcs->Array.length > 0) + if hasAnyDcs { + let shouldSaveHistory = state.config->Config.shouldSaveHistory(~isInReorgThreshold) + inMemoryStore->InMemoryStore.setDcsToStore(dcsToStoreByChainId, ~shouldSaveHistory) + }codegenerator/cli/templates/static/codegen/src/eventFetching/ChainManager.res (1)
130-173: Persist only processed DCs: logic looks solid; skip writing empty per-chain DC lists.The progress-based gating for DC persistence aligns with “processed only.” One nit: avoid adding entries with empty arrays to dcsToStoreByChainId to prevent downstream no-ops.
Apply:
- dcsToStoreByChainId->Js.Dict.set(fetchState.chainId->Int.toString, batchDcs) + if batchDcs->Array.length > 0 { + dcsToStoreByChainId->Js.Dict.set(fetchState.chainId->Int.toString, batchDcs) + }Also consider replacing dangerouslyGetNonOption with a safe getter to reduce footguns (non-blocking).
scenarios/test_codegen/test/lib_tests/FetchState_test.res (2)
708-709: Make dcsToStore order-insensitive in assertions (optional).Several asserts depend on array order that isn’t guaranteed by design. To reduce flakiness, compare on a sorted projection (e.g., by (address, startBlock)).
Example helper:
let sortDcs = dcs => dcs->Array.sortBy(dc => (Address.toString(dc.address), dc.startBlock))Then wrap both sides with sortDcs in the affected assertions.
Also applies to: 758-759, 1226-1228
2119-2119: Remove stray log from test output.- Js.log(updatedFetchState) + // (removed noisy log)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
codegenerator/cli/npm/envio/src/Batch.res(1 hunks)codegenerator/cli/npm/envio/src/FetchState.res(4 hunks)codegenerator/cli/npm/envio/src/PgStorage.res(0 hunks)codegenerator/cli/npm/envio/src/Utils.res(1 hunks)codegenerator/cli/npm/envio/src/db/InternalTable.res(0 hunks)codegenerator/cli/templates/static/codegen/src/eventFetching/ChainManager.res(3 hunks)codegenerator/cli/templates/static/codegen/src/globalState/GlobalState.res(2 hunks)scenarios/erc20_multichain_factory/test/DynamicContractRecovery_test.res(0 hunks)scenarios/test_codegen/test/ChainManager_test.res(3 hunks)scenarios/test_codegen/test/helpers/Mock.res(2 hunks)scenarios/test_codegen/test/lib_tests/FetchState_test.res(14 hunks)scenarios/test_codegen/test/lib_tests/PgStorage_test.res(0 hunks)scenarios/test_codegen/test/lib_tests/SourceManager_test.res(1 hunks)scenarios/test_codegen/test/rollback/Rollback_test.res(1 hunks)
💤 Files with no reviewable changes (4)
- scenarios/test_codegen/test/lib_tests/PgStorage_test.res
- codegenerator/cli/npm/envio/src/PgStorage.res
- scenarios/erc20_multichain_factory/test/DynamicContractRecovery_test.res
- codegenerator/cli/npm/envio/src/db/InternalTable.res
🧰 Additional context used
📓 Path-based instructions (3)
**/*.{res,resi}
📄 CodeRabbit inference engine (.cursor/rules/rescript.mdc)
**/*.{res,resi}: Never use[| item |]to create an array. Use[ item ]instead.
Must always use=for setting value to a field. Use:=only for ref values created usingreffunction.
ReScript has record types which require a type definition before hand. You can access record fields by dot likefoo.myField.
It's also possible to define an inline object, it'll have quoted fields in this case.
Use records when working with structured data, and objects to conveniently pass payload data between functions.
Never use %raw to access object fields if you know the type.
Files:
codegenerator/cli/npm/envio/src/Utils.rescodegenerator/cli/templates/static/codegen/src/eventFetching/ChainManager.resscenarios/test_codegen/test/helpers/Mock.resscenarios/test_codegen/test/lib_tests/SourceManager_test.resscenarios/test_codegen/test/rollback/Rollback_test.rescodegenerator/cli/npm/envio/src/Batch.resscenarios/test_codegen/test/ChainManager_test.rescodegenerator/cli/npm/envio/src/FetchState.resscenarios/test_codegen/test/lib_tests/FetchState_test.rescodegenerator/cli/templates/static/codegen/src/globalState/GlobalState.res
**/*.res
📄 CodeRabbit inference engine (.cursor/rules/navigation.mdc)
Prefer reading ReScript .res modules directly; compiled .js artifacts can be ignored
Files:
codegenerator/cli/npm/envio/src/Utils.rescodegenerator/cli/templates/static/codegen/src/eventFetching/ChainManager.resscenarios/test_codegen/test/helpers/Mock.resscenarios/test_codegen/test/lib_tests/SourceManager_test.resscenarios/test_codegen/test/rollback/Rollback_test.rescodegenerator/cli/npm/envio/src/Batch.resscenarios/test_codegen/test/ChainManager_test.rescodegenerator/cli/npm/envio/src/FetchState.resscenarios/test_codegen/test/lib_tests/FetchState_test.rescodegenerator/cli/templates/static/codegen/src/globalState/GlobalState.res
codegenerator/cli/templates/{static,dynamic}/codegen/src/**/*
📄 CodeRabbit inference engine (.cursor/rules/navigation.mdc)
Edit template versions under codegenerator/cli/templates/static/codegen/src or codegenerator/cli/templates/dynamic/codegen/src instead of editing generated/src
Files:
codegenerator/cli/templates/static/codegen/src/eventFetching/ChainManager.rescodegenerator/cli/templates/static/codegen/src/globalState/GlobalState.res
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build_and_test
🔇 Additional comments (13)
codegenerator/cli/npm/envio/src/FetchState.res (2)
63-66: dcsToStore now a concrete array — LGTM.The non-optional shape simplifies control flow and aligns with persistence semantics.
1131-1134: Initialize dcsToStore to [] — LGTM.Consistent with the new non-nullable contract.
scenarios/test_codegen/test/helpers/Mock.res (1)
357-362: itemMock optional fields — LGTM.Optional handler/contractRegister make the helper more flexible.
scenarios/test_codegen/test/lib_tests/SourceManager_test.res (1)
193-199: Mocks updated to dcsToStore: [] — LGTM.Matches the new non-optional state shape.
scenarios/test_codegen/test/ChainManager_test.res (3)
162-166: Switched to updatedFetchStates in pattern match — LGTM.Keeps tests aligned with Batch.t changes.
181-187: Use updatedFetchStates when propagating fetchState — LGTM.Correctly maps chain → updated fetchState.
277-283: Mocks updated to dcsToStore: [] — LGTM.Consistent with FetchState.t.
codegenerator/cli/npm/envio/src/Batch.res (1)
12-14: Rename acknowledged — no downstream destructuring foundRepo search shows no object destructuring of {items, fetchStates}; destructuring sites use updatedFetchStates (e.g., GlobalState.res, ChainManager_test.res). Remaining fetchStates references are local params/vars (Batch.res, ChainManager template) and need no change.
codegenerator/cli/templates/static/codegen/src/eventFetching/ChainManager.res (1)
179-181: updatedFetchStates propagation and progressedChains computation look correct.Using updatedFetchStates for buffers/metrics and returning it in Batch.t matches the new API.
Also applies to: 210-216, 239-244
scenarios/test_codegen/test/lib_tests/FetchState_test.res (2)
160-168: Tests updated for non-optional dcsToStore: LGTM.Expectations now consistently use [] instead of None, matching the new FetchState shape.
Also applies to: 236-241, 303-306, 401-404, 878-880, 918-920, 1224-1228, 1655-1670, 1698-1704, 1778-1784, 1868-1872
126-129: v11 undefined workaround acknowledged.Keep until v12 upgrade; add a TODO to remove afterwards.
Also applies to: 415-418
scenarios/test_codegen/test/rollback/Rollback_test.res (1)
777-1028: New “Rollback Dynamic Contract” E2E test validates “persist only processed DCs.”Good coverage: pre-persist gating, rollback depth discovery, and post-rollback DB state. Assertions on getItemsOrThrowCalls sequence are especially helpful.
codegenerator/cli/templates/static/codegen/src/globalState/GlobalState.res (1)
905-906: Switch to batch.updatedFetchStates is correct; UpdateQueues wiring looks consistent.Repository-wide search found no remaining references to "Batch.fetchStates". Batch.t declares updatedFetchStates (codegenerator/cli/npm/envio/src/Batch.res) and GlobalState uses batch.updatedFetchStates (codegenerator/cli/templates/static/codegen/src/globalState/GlobalState.res — lines ~905 and ~938–942); ChainManager still uses a local fetchStates variable (expected).
| let clearInPlace: array<'a> => unit = %raw(`(arr) => { | ||
| arr.length = 0 | ||
| }`) | ||
|
|
There was a problem hiding this comment.
Interesting. Is this the best api to achieve clearing?
| // Needed to: | ||
| // - Recalculate the computed queue sizes | ||
| // - Accumulate registered dynamic contracts to store in the db | ||
| // - Trigger onBlock pointer update |
There was a problem hiding this comment.
How about leaving this as a doc comment?
There was a problem hiding this comment.
nevermind I thought it was a function
| handler?: Types.HandlerTypes.loader<unit, unit>, | ||
| contractRegister?: Types.HandlerTypes.contractRegister<unit>, |
There was a problem hiding this comment.
Why is this now optional?
There was a problem hiding this comment.
This is for tests, we can have an event either with handler, or contract register, or both, or without.
This is only a refactoring PR, which shouldn't change the indexer behaviour.
It prevents storing Dynamic Contracts in the store until their registration event is processed.
This allows us:
Also, rewritten related test to use the new internal test framework.
Needed for new rollback on reorg solution - since it won't support writing history for blocks which are not processed.
Summary by CodeRabbit