Improve stability of Block Handlers #738
Conversation
WalkthroughAdds a buffer-driven fetch model (targetBufferSize, bufferBlockNumber/bufferBlock helpers, latestFullyFetchedBlock/latestOnBlockBlockNumber) across FetchState, ChainFetcher, ChainManager, SourceManager and GlobalState; introduces hasUnorderedNextItem in Batch.res; removes db_write_timestamp from templates/schemas and tests; adds Array.at in Utils; updates tests for buffering and onBlock behavior. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor ChainManager
participant ChainFetcher
participant SourceManager
participant FetchState
participant Executor as executeQuery
ChainManager->>ChainFetcher: requestNext() (uses targetBufferSize)
ChainFetcher->>FetchState: getNextQuery(fetchState{targetBufferSize,...})
FetchState-->>ChainFetcher: Query | None (based on bufferBlockNumber)
alt Query available
ChainFetcher->>SourceManager: fetchNext(~fetchState,...)
SourceManager->>Executor: run(query)
Executor-->>SourceManager: result(newItems, latestFetchedBlock)
SourceManager->>FetchState: handleQueryResult(~query, ~latestFetchedBlock, ~newItems)
FetchState-->>SourceManager: updated state (mutItems buffered/sorted)
note right of FetchState: buffered items bounded by targetBufferSize
else No query
ChainFetcher-->>ChainManager: idle / await new block
end
note right of ChainManager: unordered readiness uses hasUnorderedNextItem(fetchStates)
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: 3
🧹 Nitpick comments (7)
codegenerator/cli/templates/static/codegen/src/Index.res (1)
186-192: Avoid misleading 0 for latestFetchedBlockNumber; bound by chain startBlockIf startBlock > 0 and buffer is still empty (-1), clamping to 0 under-reports. Use startBlock - 1 as the lower bound.
- let latestFetchedBlockNumber = Pervasives.max(fetchState->FetchState.bufferBlockNumber, 0) + let minDisplay = Pervasives.max(cf.chainConfig.startBlock - 1, 0) + let latestFetchedBlockNumber = + Pervasives.max(fetchState->FetchState.bufferBlockNumber, minDisplay)Please confirm TUI/console consumers expect startBlock - 1 semantics and not 0 in this field.
codegenerator/cli/templates/static/codegen/src/eventFetching/ChainFetcher.res (1)
17-17: Public field isFetchingAtHead addedLooks good; consider documenting semantics (head of chain vs confirmed head) where it’s updated.
codegenerator/cli/templates/static/codegen/src/eventFetching/ChainManager.res (1)
9-18: Consider extracting the magic number3as a named constant.The
targetBatchesInBuffervalue of 3 appears to be a crucial tuning parameter for buffer management. Extracting it as a module-level constant would improve maintainability and make it easier to adjust if needed.+let defaultTargetBatchesInBuffer = 3 + let calculateTargetBufferSize = activeChainsCount => { - let targetBatchesInBuffer = 3 switch Env.targetBufferSize { | Some(size) => size | None => Env.maxProcessBatchSize * ( - activeChainsCount > targetBatchesInBuffer ? 1 : targetBatchesInBuffer + activeChainsCount > defaultTargetBatchesInBuffer ? 1 : defaultTargetBatchesInBuffer ) } }scenarios/test_codegen/test/lib_tests/FetchState_test.res (1)
2119-2119: Remove or comment out debug logging statement.The
Js.log(updatedFetchState)debug statement should be removed from production test code.- Js.log(updatedFetchState) -codegenerator/cli/templates/static/codegen/src/globalState/GlobalState.res (1)
157-163: Simplify the isFetchingAtHead logic using max function.The conditional assignment can be simplified for better readability.
{ ...chainFetcher, - isFetchingAtHead: chainFetcher.isFetchingAtHead || - chainFetcher.fetchState->FetchState.bufferBlockNumber >= currentBlockHeight, + isFetchingAtHead: chainFetcher.isFetchingAtHead || + chainFetcher.fetchState->FetchState.bufferBlockNumber >= currentBlockHeight, currentBlockHeight, }codegenerator/cli/npm/envio/src/FetchState.res (2)
218-218: Consider documenting the blockItemLogIndex constant value.The value
16777216(2^24) is used to ensure block items have higher priority than events at the same block. This magic number should be documented to explain its purpose and why this specific value was chosen.-// Some big number which should be bigger than any log index +// Use 2^24 as the base logIndex for block items to ensure they're processed +// with higher priority than events at the same block (max logIndex in a block is typically much smaller) let blockItemLogIndex = 16777216
1287-1308: Improve hasFullBatch implementation clarity.The comment about not subtracting 1 is correct but the implementation could be clearer with a direct comparison.
let hasFullBatch = ({queue} as fetchState: t, ~maxBatchSize) => { - // 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 { + if queue->Array.length < maxBatchSize { false } else { - // Unsafe can fail when maxBatchSize is 0, - // but we ignore the case + // Check if we have enough eligible items for a full batch + // The item at index (length - maxBatchSize) must be eligible + let targetBlockIdx = queue->Array.length - maxBatchSize queue->Js.Array2.unsafe_get(targetBlockIdx)->Internal.getItemBlockNumber <= fetchState->bufferBlockNumber } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (20)
codegenerator/cli/npm/envio/src/Batch.res(1 hunks)codegenerator/cli/npm/envio/src/FetchState.res(17 hunks)codegenerator/cli/npm/envio/src/Utils.res(1 hunks)codegenerator/cli/npm/envio/src/db/EntityHistory.res(0 hunks)codegenerator/cli/npm/envio/src/db/InternalTable.res(0 hunks)codegenerator/cli/npm/envio/src/sources/SourceManager.res(0 hunks)codegenerator/cli/npm/envio/src/sources/SourceManager.resi(0 hunks)codegenerator/cli/templates/dynamic/codegen/src/db/Entities.res.hbs(0 hunks)codegenerator/cli/templates/static/codegen/src/Index.res(1 hunks)codegenerator/cli/templates/static/codegen/src/eventFetching/ChainFetcher.res(10 hunks)codegenerator/cli/templates/static/codegen/src/eventFetching/ChainManager.res(5 hunks)codegenerator/cli/templates/static/codegen/src/globalState/GlobalState.res(7 hunks)scenarios/test_codegen/schema.graphql(0 hunks)scenarios/test_codegen/test/ChainManager_test.res(3 hunks)scenarios/test_codegen/test/SerDe_Test.res(1 hunks)scenarios/test_codegen/test/entity-column-types-test.ts(1 hunks)scenarios/test_codegen/test/lib_tests/FetchState_onBlock_test.res(1 hunks)scenarios/test_codegen/test/lib_tests/FetchState_test.res(64 hunks)scenarios/test_codegen/test/lib_tests/PgStorage_test.res(6 hunks)scenarios/test_codegen/test/lib_tests/SourceManager_test.res(3 hunks)
💤 Files with no reviewable changes (6)
- scenarios/test_codegen/schema.graphql
- codegenerator/cli/npm/envio/src/sources/SourceManager.res
- codegenerator/cli/npm/envio/src/sources/SourceManager.resi
- codegenerator/cli/npm/envio/src/db/EntityHistory.res
- codegenerator/cli/templates/dynamic/codegen/src/db/Entities.res.hbs
- codegenerator/cli/npm/envio/src/db/InternalTable.res
🧰 Additional context used
📓 Path-based instructions (4)
scenarios/test_codegen/**/*.ts
📄 CodeRabbit inference engine (scenarios/test_codegen/.cursor/rules/hyperindex.mdc)
scenarios/test_codegen/**/*.ts: After changing any TypeScript files, run: pnpm tsc --noEmit to ensure successful compilation
When updating existing entities in handlers, always use the spread operator to create updated objects before persisting
For any external call (e.g., fetch), wrap it in an Effect via experimental_createEffect and consume via context.effect
Use !context.isPreload to skip logic that should not run during preload
In TypeScript, set relationship fields using *_id properties (e.g., token_id) rather than object references
Always cast timestamps from events to BigInt (e.g., BigInt(event.block.timestamp))
When matching addresses in configuration objects within code, ensure keys are lowercase and compare using address.toLowerCase()
Use string | undefined for optional string fields instead of string | null
Always normalize token amounts to a standard decimal (e.g., 18) before addition across tokens; use helpers like normalizeAmountToUSD()
Files:
scenarios/test_codegen/test/entity-column-types-test.ts
**/*.{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/templates/static/codegen/src/Index.resscenarios/test_codegen/test/SerDe_Test.rescodegenerator/cli/npm/envio/src/Batch.resscenarios/test_codegen/test/lib_tests/FetchState_onBlock_test.rescodegenerator/cli/npm/envio/src/Utils.rescodegenerator/cli/templates/static/codegen/src/eventFetching/ChainManager.resscenarios/test_codegen/test/lib_tests/PgStorage_test.resscenarios/test_codegen/test/ChainManager_test.resscenarios/test_codegen/test/lib_tests/FetchState_test.rescodegenerator/cli/templates/static/codegen/src/eventFetching/ChainFetcher.rescodegenerator/cli/templates/static/codegen/src/globalState/GlobalState.resscenarios/test_codegen/test/lib_tests/SourceManager_test.rescodegenerator/cli/npm/envio/src/FetchState.res
**/*.res
📄 CodeRabbit inference engine (.cursor/rules/navigation.mdc)
Prefer reading ReScript .res modules directly; compiled .js artifacts can be ignored
Files:
codegenerator/cli/templates/static/codegen/src/Index.resscenarios/test_codegen/test/SerDe_Test.rescodegenerator/cli/npm/envio/src/Batch.resscenarios/test_codegen/test/lib_tests/FetchState_onBlock_test.rescodegenerator/cli/npm/envio/src/Utils.rescodegenerator/cli/templates/static/codegen/src/eventFetching/ChainManager.resscenarios/test_codegen/test/lib_tests/PgStorage_test.resscenarios/test_codegen/test/ChainManager_test.resscenarios/test_codegen/test/lib_tests/FetchState_test.rescodegenerator/cli/templates/static/codegen/src/eventFetching/ChainFetcher.rescodegenerator/cli/templates/static/codegen/src/globalState/GlobalState.resscenarios/test_codegen/test/lib_tests/SourceManager_test.rescodegenerator/cli/npm/envio/src/FetchState.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/Index.rescodegenerator/cli/templates/static/codegen/src/eventFetching/ChainManager.rescodegenerator/cli/templates/static/codegen/src/eventFetching/ChainFetcher.rescodegenerator/cli/templates/static/codegen/src/globalState/GlobalState.res
🧠 Learnings (6)
📚 Learning: 2025-08-11T08:49:57.399Z
Learnt from: CR
PR: enviodev/hyperindex#0
File: scenarios/test_codegen/.cursor/rules/hyperindex.mdc:0-0
Timestamp: 2025-08-11T08:49:57.399Z
Learning: Applies to scenarios/test_codegen/**/*.ts : When updating existing entities in handlers, always use the spread operator to create updated objects before persisting
Applied to files:
scenarios/test_codegen/test/entity-column-types-test.ts
📚 Learning: 2025-08-11T08:49:57.399Z
Learnt from: CR
PR: enviodev/hyperindex#0
File: scenarios/test_codegen/.cursor/rules/hyperindex.mdc:0-0
Timestamp: 2025-08-11T08:49:57.399Z
Learning: Applies to scenarios/test_codegen/**/*.ts : In TypeScript, set relationship fields using *_id properties (e.g., token_id) rather than object references
Applied to files:
scenarios/test_codegen/test/entity-column-types-test.ts
📚 Learning: 2025-09-04T14:42:53.500Z
Learnt from: CR
PR: enviodev/hyperindex#0
File: .cursor/rules/subgraph-migration.mdc:0-0
Timestamp: 2025-09-04T14:42:53.500Z
Learning: Applies to src/**/*.ts : Match entity field types exactly with schema: use bigint for BigInt fields, BigDecimal for BigDecimal fields, and correct scalar types
Applied to files:
scenarios/test_codegen/test/entity-column-types-test.ts
📚 Learning: 2025-08-11T08:47:04.346Z
Learnt from: CR
PR: enviodev/hyperindex#0
File: scenarios/fuel_test/.cursor/rules/hyperindex.mdc:0-0
Timestamp: 2025-08-11T08:47:04.346Z
Learning: Applies to scenarios/fuel_test/**/*.ts : When updating existing entities in handlers, always use the spread operator to clone immutable returned objects before modifying and saving
Applied to files:
scenarios/test_codegen/test/entity-column-types-test.ts
📚 Learning: 2025-08-11T08:47:04.346Z
Learnt from: CR
PR: enviodev/hyperindex#0
File: scenarios/fuel_test/.cursor/rules/hyperindex.mdc:0-0
Timestamp: 2025-08-11T08:47:04.346Z
Learning: Applies to scenarios/fuel_test/**/*.ts : In TypeScript code, set relationship fields using the `_id` form expected by generated types (e.g., `{ token_id: tokenId }`, not `{ token: tokenObject }`)
Applied to files:
scenarios/test_codegen/test/entity-column-types-test.ts
📚 Learning: 2025-08-11T08:49:57.399Z
Learnt from: CR
PR: enviodev/hyperindex#0
File: scenarios/test_codegen/.cursor/rules/hyperindex.mdc:0-0
Timestamp: 2025-08-11T08:49:57.399Z
Learning: Applies to scenarios/test_codegen/**/*.ts : Always cast timestamps from events to BigInt (e.g., BigInt(event.block.timestamp))
Applied to files:
scenarios/test_codegen/test/entity-column-types-test.ts
⏰ 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). (5)
- GitHub Check: build_and_test
- GitHub Check: aarch64-apple-darwin (macos-latest)
- GitHub Check: x86_64-apple-darwin (macos-latest)
- GitHub Check: aarch64-unknown-linux-gnu (ubuntu-22.04)
- GitHub Check: x86_64-unknown-linux-musl (ubuntu-22.04)
🔇 Additional comments (22)
scenarios/test_codegen/test/entity-column-types-test.ts (1)
161-161: Exact column-count check aligns with removal of db_write_timestamp — LGTMStricter equality is correct now that no extra auto-generated column is present.
TypeScript compile verification required — run
pnpm -s tsc --noEmitand paste stdout/stderr (previous run produced no output).scenarios/test_codegen/test/SerDe_Test.res (1)
129-129: Updated CREATE TABLE expectation (no db_write_timestamp) is consistent — LGTMMatches the generator/template removals.
scenarios/test_codegen/test/lib_tests/PgStorage_test.res (6)
68-69: A table DDL expectation updated (timestamp column removed) — LGTM
78-80: B table DDL expectation updated — LGTM
88-93: Default-values DDL expectation for A updated — LGTM
150-154: Main DDL: raw_events/A/B definitions updated to drop db_write_timestamp — LGTMThe new raw_events layout placing serial immediately after params matches insert behavior.
235-236: Minimal config DDL: raw_events change reflected — LGTM
317-319: Public schema DDL: raw_events and A table updates — LGTMcodegenerator/cli/npm/envio/src/Utils.res (1)
306-308: Ensure Array.at is supported at runtime or replace with a pure ReScript fallbackRan verification: no engines.node found in package.json files scanned; no setup-node occurrences in .github workflows; jq failed parsing a template file — no repo evidence guarantees Array.prototype.at.
File: codegenerator/cli/npm/envio/src/Utils.res (lines 306–308)
Either declare/lock a Node engine (Node ≥ 16.6) and pin CI, or replace the external with this pure ReScript fallback (preserves negative-index semantics):
- @send external at: (array<'a>, int) => option<'a> = "at" + /** + * Option-safe index access with negative index support. + * Pure ReScript version to avoid relying on Array.prototype.at at runtime. + */ + let at = (arr: array<'a>, index: int): option<'a> => { + let len = arr->Array.length + let idx = if index >= 0 { index } else { len + index } + if idx >= 0 && idx < len { + Some(arr->Js.Array2.unsafe_get(idx)) + } else { + None + } + }codegenerator/cli/npm/envio/src/Batch.res (1)
61-71: LGTM: concise unordered-next indicatorClear, short-circuiting check gated by isActivelyIndexing.
scenarios/test_codegen/test/lib_tests/SourceManager_test.res (3)
254-256: Good default coverage for targetBufferSizeDefaulting to 5000 here mirrors production and keeps tests representative.
293-301: Fields aligned with new buffering APIlatestOnBlockBlockNumber and onBlockConfigs [] look correct with updateInternal.
799-800: Nice boundary test for queue saturationUsing targetBufferSize=4 exercises the “skip at max queue size” path well.
scenarios/test_codegen/test/ChainManager_test.res (3)
33-35: FetchState.make: targetBufferSize plumbed into testsKeeps test harness consistent with runtime behavior.
115-115: isFetchingAtHead added to mock fetcherMatches the new public field and keeps tests compiling.
271-273: Partition fields updated to buffering modellatestOnBlockBlockNumber and onBlockConfigs [] are appropriate; targetBufferSize included.
Also applies to: 282-282
codegenerator/cli/templates/static/codegen/src/eventFetching/ChainFetcher.res (3)
39-39: targetBufferSize is threaded through constructionEnd-to-end propagation into FetchState.make and initial state is correct.
Also applies to: 181-181, 201-201
470-475: Updated FetchState.handleQueryResult signatureRemoval of currentBlockHeight arg is correctly reflected.
212-230: Factory signature updated to accept targetBufferSizeNo issues spotted; callers must be updated accordingly.
Run to find any stale call sites:
#!/bin/bash rg -nP 'ChainFetcher\.makeFromConfig\([^)]*\)' -C2 rg -nP 'ChainFetcher\.make\([^)]*\)' -C2scenarios/test_codegen/test/lib_tests/FetchState_onBlock_test.res (1)
256-257: LGTM! Clear test structure with comprehensive onBlock functionality coverage.The test suite effectively validates the new buffering-based block generation logic, covering various edge cases including intervals, start/end block boundaries, and multiple onBlock configs. The use of logIndex values (16777216 + index) to distinguish block items from different configs is a clever approach for deterministic testing.
codegenerator/cli/npm/envio/src/FetchState.res (2)
1168-1168: Fix inconsistent comment about rollback behavior.The FIXME comment questions whether
latestFetchedBlockshould be rolled back, but the implementation doesn't rollback for wildcard partitions. This should either be fixed or the comment should be updated to explain why.Should wildcard partitions have their
latestFetchedBlockrolled back when a rollback occurs? The current implementation doesn't roll it back for partitions withdependsOnAddresses: false.
255-256: Verify supported runtimes before using Utils.Array.at(-index)Array.prototype.at with negative indices is natively supported in Chrome 92+, Edge 92+, Firefox 90+, Safari 15.4+, Node.js 16.6.0+, Deno 1.12+. If you must support older engines, replace or polyfill. Location: codegenerator/cli/npm/envio/src/FetchState.res (lines 255-256).
- }->Utils.Array.at(-fetchState.targetBufferSize) { + }->{ + let arr = _ + let idx = arr->Array.length - fetchState.targetBufferSize + if idx >= 0 { + arr->Array.get(idx) + } else { + None + } + } {
| ~targetBufferSize, | ||
| ~sql=Db.sql, | ||
| ) => { |
There was a problem hiding this comment.
Resume filter off-by-one and cleanup condition risk
restartBlockNumber is set to progressBlockNumber + 1, but the filter comment says “Start from the same block and filter out already processed.” As written, events in the next block (progressBlockNumber + 1) with logIndex ≤ progressNextBlockLogIndex would be incorrectly dropped, and cleanup uses bufferBlockNumber (likely the max), which can remove the filter while older-block items may still be queued. Suggest aligning to the last committed block and cleaning up based on the earliest queued block.
- let restartBlockNumber =
- // Can be -1 when not set
- resumedChainState.progressBlockNumber >= 0
- ? resumedChainState.progressBlockNumber + 1
- : resumedChainState.startBlock
+ // Resume from the last committed block; filter within it by logIndex.
+ let resumeBlockNumber =
+ resumedChainState.progressBlockNumber >= 0
+ ? resumedChainState.progressBlockNumber
+ : resumedChainState.startBlock
@@
- filter: qItem => {
+ filter: qItem => {
switch qItem {
| Internal.Event({blockNumber, logIndex})
| Internal.Block({blockNumber, logIndex}) =>
- //Only keep events greater than the last processed event
- (blockNumber, logIndex) > (restartBlockNumber, progressNextBlockLogIndex)
+ // Only keep events strictly after the last processed (block, logIndex)
+ (blockNumber, logIndex) > (resumeBlockNumber, progressNextBlockLogIndex)
}
},
- isValid: (~fetchState) => {
- //the filter can be cleaned up as soon as the fetch state block is ahead of the latestProcessedEvent blockNumber
- fetchState->FetchState.bufferBlockNumber <= restartBlockNumber
- },
+ isValid: (~fetchState) => {
+ // Keep the filter until the earliest queued block advances beyond resumeBlockNumber
+ switch fetchState->FetchState.getEarliestEvent {
+ | Item({item}) => item->Internal.getItemBlockNumber <= resumeBlockNumber
+ | NoItem({latestFetchedBlock: {blockNumber}}) => blockNumber <= resumeBlockNumber
+ }
+ },Please validate expected semantics with an end-to-end resume case (e.g., progressBlockNumber=100, progressNextBlockLogIndex=4) to ensure no events from block 101 are erroneously filtered and duplicates from block 100 are not reprocessed. You can grep for how progressBlockNumber/NextBlockLogIndex are persisted/consumed:
#!/bin/bash
rg -n 'progressBlockNumber|progressNextBlockLogIndex' -C3 --type=res --type=sql --type=tsAlso applies to: 251-256, 262-275
🤖 Prompt for AI Agents
In codegenerator/cli/templates/static/codegen/src/eventFetching/ChainFetcher.res
around lines 245-247 (also review 251-256 and 262-275), the resume logic sets
restartBlockNumber to progressBlockNumber + 1 and uses bufferBlockNumber for
cleaning up filters, which can incorrectly drop events in the next block and
prematurely remove filters; change restartBlockNumber to start from
progressBlockNumber (not +1) and make the filter comparison only drop events
with logIndex ≤ progressNextBlockLogIndex when blockNumber ==
progressBlockNumber, allow all events from blockNumber > progressBlockNumber,
and change cleanup to base removal on the earliest queued/block-minimum (not
buffer/max) so older queued items aren’t lost; after changes, validate with an
end-to-end resume case (e.g., progressBlockNumber=100,
progressNextBlockLogIndex=4) to confirm block 101 events are kept and duplicates
from block 100 are filtered.
codegenerator/cli/templates/static/codegen/src/eventFetching/ChainManager.res
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
codegenerator/cli/templates/static/codegen/src/eventFetching/ChainManager.res (1)
105-108: Resolved: unordered next-item logic now correctly negated.nextItemIsNone uses !hasUnorderedNextItem as expected. Thanks for addressing.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
codegenerator/cli/templates/static/codegen/src/eventFetching/ChainManager.res(5 hunks)
🧰 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/templates/static/codegen/src/eventFetching/ChainManager.res
**/*.res
📄 CodeRabbit inference engine (.cursor/rules/navigation.mdc)
Prefer reading ReScript .res modules directly; compiled .js artifacts can be ignored
Files:
codegenerator/cli/templates/static/codegen/src/eventFetching/ChainManager.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.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 (4)
codegenerator/cli/templates/static/codegen/src/eventFetching/ChainManager.res (4)
21-24: Propagate targetBufferSize through makeFromConfig — LGTM.The calculation and forwarding into ChainFetcher.makeFromConfig look correct.
48-52: Good instrumentation for visibility — LGTM.Prometheus gauges for max batch size, target buffer size, and reorg threshold are helpful for ops.
61-66: Forwarding targetBufferSize to ChainFetcher.makeFromDbState — LGTM.Consistent with constructor path; keeps buffering behavior uniform on resume.
213-214: Switched to cf.isFetchingAtHead property — verify remaining call sites.rg -nP '\bChainFetcher.isFetchingAtHead\b' returned no matches; not conclusive. Run a full repo search for "isFetchingAtHead" and function-call forms (e.g. "isFetchingAtHead(" or ".isFetchingAtHead()") and update any remaining call sites. Location: codegenerator/cli/templates/static/codegen/src/eventFetching/ChainManager.res lines 213–214
codegenerator/cli/templates/static/codegen/src/eventFetching/ChainManager.res
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
scenarios/erc20_multichain_factory/test/DynamicContractRecovery_test.res (1)
164-173: Hoist test targetBufferSize; callsites already include ~targetBufferSize.All ChainFetcher.makeFromDbState callsites include the ~targetBufferSize label; the test still hard-codes 5000 — hoist to a local constant and assert the wiring.
Apply this minimal change at the callsite:
- ~targetBufferSize=5000, + ~targetBufferSize=targetBufferSize,Add near the top of getFetchingDcAddressesFromDbState:
+ let targetBufferSize = 5_000Optionally assert the wiring after construction:
+ Assert.equal( + chainFetcher.fetchState.targetBufferSize, + targetBufferSize, + ~message="ChainFetcher should use configured targetBufferSize", + )
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
scenarios/erc20_multichain_factory/pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (1)
scenarios/erc20_multichain_factory/test/DynamicContractRecovery_test.res(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{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:
scenarios/erc20_multichain_factory/test/DynamicContractRecovery_test.res
**/*.res
📄 CodeRabbit inference engine (.cursor/rules/navigation.mdc)
Prefer reading ReScript .res modules directly; compiled .js artifacts can be ignored
Files:
scenarios/erc20_multichain_factory/test/DynamicContractRecovery_test.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
| let compareBufferItem = (a: Internal.item, b: Internal.item) => { | ||
| let blockDiff = b->Internal.getItemBlockNumber - a->Internal.getItemBlockNumber | ||
| if blockDiff === 0 { | ||
| b->Internal.getItemLogIndex - a->Internal.getItemLogIndex | ||
| } else { | ||
| blockDiff | ||
| } | ||
| } |
There was a problem hiding this comment.
This looks backwards for a cmp function.
Usually negative return would mean a comes first and positive would mean b comes first
There was a problem hiding this comment.
We still store buffer in desc order, so this is correct. I actually didn't change this part + it's covered with tests.
| mkField( | ||
| "db_write_timestamp", | ||
| TimestampWithoutTimezone, | ||
| ~default="CURRENT_TIMESTAMP", | ||
| ~fieldSchema=S.int, | ||
| ), |
There was a problem hiding this comment.
This is kind of a breaking change because this was public on hasura. I think that's why it was left in.
I don't like it there by default but it is unfortunately breaking.
There was a problem hiding this comment.
Yes, but it's not the first time we are breaking undocumented behaviour. I think it should be fine, unless someone complains.
Summary by CodeRabbit
New Features
Breaking Changes