Improve chain priority logic - #690
Conversation
WalkthroughAdds batch-fullness prioritization to unordered-batch sorting (new hasFullBatch + ~maxBatchSize), updates ChainManager call site, refactors and extends FetchState tests to use public APIs, changes Hyperliquid network discriminant, and appends a Testing note to navigation docs. Changes
Sequence Diagram(s)sequenceDiagram
participant CM as ChainManager
participant FS as FetchState
participant Q as Queues
CM->>Q: collect unordered fetch queues
CM->>FS: filterAndSortForUnorderedBatch(~maxBatchSize, queues)
FS->>FS: hasFullBatch(queue, latestFullyFetchedBlock, maxBatchSize)
FS-->>CM: sorted queues (full batches first, then timestamps)
CM->>Q: select queues for unordered batch
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
.cursor/rules/navigation.mdc (1)
35-39: Good addition: lightweight testing guidance.Clear and useful. One nit: consider warning contributors not to leave focused tests committed.
You could append a short note:
## Testing Prefer Public module API for testing. -Verify that tests pass by running a compiler `pnpm rescript` and tests `pnpm mocha`. Use `_only` to specify which tests to run. +Verify that tests pass by running a compiler `pnpm rescript` and tests `pnpm mocha`. Use `_only` to specify which tests to run. +Note: avoid committing focused tests (e.g. `describe_only` / `_only`) to main.scenarios/test_codegen/test/lib_tests/FetchState_test.res (1)
2660-2675: Public-API-based construction looks solid; add an “exactly full” case.Given the off-by-one risk, add a case where
queue.length === maxBatchSizeto pin expected behavior.You can extend this block with:
Assert.deepEqual( prepared->Array.map(fs => (fs.queue->Utils.Array.last->Option.getUnsafe).blockNumber), [1, 5], ) }, ) + + it("Treats exactly maxBatchSize items as a full batch (public API)", () => { + let mk = () => makeInitial() + let mkQuery = (fetchState: FetchState.t) => { + { + FetchState.partitionId: "0", + target: Head, + selection: fetchState.normalSelection, + addressesByContractName: Js.Dict.empty(), + fromBlock: 0, + indexingContracts: fetchState.indexingContracts, + } + } + let makeFsWith = (~latestBlock: int, ~queueBlocks: array<int>): FetchState.t => { + let fs0 = mk() + let query = mkQuery(fs0) + fs0 + ->FetchState.handleQueryResult( + ~query, + ~latestFetchedBlock={blockNumber: latestBlock, blockTimestamp: latestBlock}, + ~reversedNewItems=queueBlocks->Array.map(b => mockEvent(~blockNumber=b)), + ~currentBlockHeight=latestBlock, + ) + ->Result.getExn + } + let fsExactlyFull = makeFsWith(~latestBlock=10, ~queueBlocks=[3, 2]) + let fsHalf = makeFsWith(~latestBlock=10, ~queueBlocks=[1]) + let prepared = + [fsHalf, fsExactlyFull]->FetchState.filterAndSortForUnorderedBatch(~maxBatchSize=2) + Assert.deepEqual( + prepared->Array.map(fs => (fs.queue->Utils.Array.last->Option.getUnsafe).blockNumber), + [2, 1], + ) + })
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (4)
.cursor/rules/navigation.mdc(1 hunks)codegenerator/cli/npm/envio/src/FetchState.res(1 hunks)codegenerator/cli/templates/static/codegen/src/eventFetching/ChainManager.res(1 hunks)scenarios/test_codegen/test/lib_tests/FetchState_test.res(6 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
**/*.{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/FetchState.rescodegenerator/cli/templates/static/codegen/src/eventFetching/ChainManager.resscenarios/test_codegen/test/lib_tests/FetchState_test.res
codegenerator/cli/npm/envio/**
📄 CodeRabbit Inference Engine (.cursor/rules/navigation.mdc)
Library-fied runtime shared across indexers lives in codegenerator/cli/npm/envio.
Files:
codegenerator/cli/npm/envio/src/FetchState.res
codegenerator/cli/templates/{dynamic/**/*.hbs,static/**}
📄 CodeRabbit Inference Engine (.cursor/rules/navigation.mdc)
Templates live under codegenerator/cli/templates: dynamic/ for Handlebars (.hbs), static/ for raw Rescript files copied verbatim.
Files:
codegenerator/cli/templates/static/codegen/src/eventFetching/ChainManager.res
{**/generated/src/**/*.res,codegenerator/cli/templates/static/codegen/src/**/*.res,codegenerator/cli/templates/dynamic/codegen/src/**/*.res}
📄 CodeRabbit Inference Engine (.cursor/rules/navigation.mdc)
Runtime code lives in each project’s generated/src, but template versions (good for editing) are under codegenerator/cli/templates/static/codegen/src or codegenerator/cli/templates/dynamic/codegen/src.
Files:
codegenerator/cli/templates/static/codegen/src/eventFetching/ChainManager.res
🧠 Learnings (5)
📚 Learning: 2025-08-04T07:35:20.300Z
Learnt from: CR
PR: enviodev/hyperindex#0
File: .cursor/rules/navigation.mdc:0-0
Timestamp: 2025-08-04T07:35:20.300Z
Learning: Config parsing & codegen lives in Rust. When tracking how a value reaches templates, follow human_config.rs → system_config.rs → codegen_templates.rs.
Applied to files:
.cursor/rules/navigation.mdc
📚 Learning: 2025-08-04T07:35:20.300Z
Learnt from: CR
PR: enviodev/hyperindex#0
File: .cursor/rules/navigation.mdc:0-0
Timestamp: 2025-08-04T07:35:20.300Z
Learning: Applies to {human_config.rs,system_config.rs,hbs_templating/codegen_templates.rs} : Config parsing pipeline: human_config.rs reads user files & JSON schemas; system_config.rs converts to internal structs; hbs_templating/codegen_templates.rs feeds templates.
Applied to files:
.cursor/rules/navigation.mdc
📚 Learning: 2025-08-04T07:35:20.300Z
Learnt from: CR
PR: enviodev/hyperindex#0
File: .cursor/rules/navigation.mdc:0-0
Timestamp: 2025-08-04T07:35:20.300Z
Learning: Applies to **/generated/src/**/*.js : Prefer reading ReScript .res modules directly; compiled .js artifacts can be ignored.
Applied to files:
.cursor/rules/navigation.mdc
📚 Learning: 2025-08-04T07:35:20.300Z
Learnt from: CR
PR: enviodev/hyperindex#0
File: .cursor/rules/navigation.mdc:0-0
Timestamp: 2025-08-04T07:35:20.300Z
Learning: Applies to **/generated/{Index,Config,PgStorage,Hasura,Persistence,IO,ChainManager,ChainFetcher,FetchState,SourceManager,GlobalStateManager,EventProcessing,Prometheus}.res : Generated runtime inside each project’s generated/: entry module is Index.res, config is Config.res, persistence stack includes PgStorage.res, Hasura.res, Persistence.res, IO.res; fetch side includes ChainManager.res, ChainFetcher.res, FetchState.res, SourceManager.res; processing includes GlobalStateManager.res, EventProcessing.res, IO.res; metrics is Prometheus.res.
Applied to files:
.cursor/rules/navigation.mdc
📚 Learning: 2025-08-04T07:35:20.300Z
Learnt from: CR
PR: enviodev/hyperindex#0
File: .cursor/rules/navigation.mdc:0-0
Timestamp: 2025-08-04T07:35:20.300Z
Learning: Applies to codegenerator/cli/templates/{dynamic/**/*.hbs,static/**} : Templates live under codegenerator/cli/templates: dynamic/ for Handlebars (.hbs), static/ for raw Rescript files copied verbatim.
Applied to files:
.cursor/rules/navigation.mdc
⏰ 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 (6)
codegenerator/cli/templates/static/codegen/src/eventFetching/ChainManager.res (2)
185-189: Pass-through of ~maxBatchSize to FetchState is correct.The pipe into
FetchState.filterAndSortForUnorderedBatch(~maxBatchSize)matches the updated signature(fetchStates: array<t>, ~maxBatchSize: int)and keeps the pipeline readable.
185-189: Confirmation: AllfilterAndSortForUnorderedBatchcalls include~maxBatchSizeI ran the grep search and verified every invocation has been updated:
- scenarios/test_codegen/test/lib_tests/FetchState_test.res
• Line 2668:[fsLate, fsExcluded, fsEarly]->FetchState.filterAndSortForUnorderedBatch(~maxBatchSize=3)
• Line 2709:[fsHalfEarlier, fsFullLater]->FetchState.filterAndSortForUnorderedBatch(~maxBatchSize=2)- codegenerator/cli/templates/static/codegen/src/eventFetching/ChainManager.res
• Line 188:->FetchState.filterAndSortForUnorderedBatch(~maxBatchSize)No lingering invocations detected. Feel free to resolve this check.
codegenerator/cli/npm/envio/src/FetchState.res (1)
1251-1261: Comparator behavior looks consistent post-fix; keep timestamp fallback.Once
hasFullBatchis corrected, the prioritization (full over non-full, else earliest timestamp) aligns with the stated goal. The unsafe last access is safe due to prior filtering byhasBatchItem.scenarios/test_codegen/test/lib_tests/FetchState_test.res (3)
1821-1855: Nice refactor to use public API in setup.Switching to
makeInitial()+updateInternalimproves readability and reduces brittle test construction.
2123-2149: LGTM: consistent refactor to public API for earliest-event tests.The pattern mirrors the previous test; keeps tests focused on public surface.
2703-2715: Test intent is correct; confirm after fixinghasFullBatch.Prioritizing full batches over earlier half-batches is validated here. Once the off-by-one in
hasFullBatchis corrected, this test should pass and avoid edge-case regressions.Please run the test suite locally to verify in the project root (where
package.jsonlives):pnpm rescript && pnpm mocha --reporter dot
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
scenarios/test_codegen/test/lib_tests/FetchState_test.res (4)
2671-2673: Decouple assertion from queue ordering; use public API to derive earliest itemDeriving the “earliest” item via Utils.Array.last implicitly depends on queue ordering. Prefer using FetchState.getEarliestEvent to avoid coupling tests to internal ordering.
Apply this diff:
- Assert.deepEqual( - prepared->Array.map(fs => (fs.queue->Utils.Array.last->Option.getUnsafe).blockNumber), - [1, 5], - ) + Assert.deepEqual( + prepared + ->Array.map(fs => + switch fs->FetchState.getEarliestEvent->getItem { + | Some(item) => item.blockNumber + | None => Assert.fail("Expected earliest item") + } + ), + [1, 5], + )
2678-2689: Hoist shared helpers to the describe scope to avoid duplicationmk and mkQuery are re-declared in both tests in this block. Hoisting them (and makeFsWith) to the describe("FetchState.filterAndSortForUnorderedBatch") scope reduces duplication and keeps the tests consistent.
If you prefer, I can provide a small patch consolidating these helpers at the describe level and updating both tests accordingly.
2703-2707: Add boundary/tie-breaker coverage for fullnessYou test “>= maxBatchSize” by using 3 with a limit of 2. Consider:
- A case where size == maxBatchSize to ensure equality is treated as “full”.
- A tie-breaker where two “full” buffers exist; verify fallback ordering (e.g., earliest item wins).
I can add these test cases aligned to the current helper pattern if you’d like.
2711-2714: Mirror the earliest-item assertion refactor hereAs above, avoid relying on queue ordering; use the public API to compute earliest.
Apply this diff:
- Assert.deepEqual( - prepared->Array.map(fs => (fs.queue->Utils.Array.last->Option.getUnsafe).blockNumber), - [7, 1], - ) + Assert.deepEqual( + prepared + ->Array.map(fs => + switch fs->FetchState.getEarliestEvent->getItem { + | Some(item) => item.blockNumber + | None => Assert.fail("Expected earliest item") + } + ), + [7, 1], + )
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
scenarios/test_codegen/test/lib_tests/FetchState_test.res(6 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{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/test_codegen/test/lib_tests/FetchState_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
🔇 Additional comments (3)
scenarios/test_codegen/test/lib_tests/FetchState_test.res (3)
1821-1854: LGTM: clean test setup via updateInternalSolid use of makeInitial + updateInternal to craft the minimal state needed for merge semantics. The structure and labels are clear and consistent with the rest of the suite.
2123-2148: LGTM: straightforward earliest-event setupUsing base + updateInternal to stage two partitions at the same latestFetchedBlock with a small queue is clear and keeps the test focused on getEarliestEvent.
2630-2631: Thank you for removing the focused suiteReplacing describe_only with describe resolves the CI risk of skipping other tests.
JonoPrest
left a comment
There was a problem hiding this comment.
I'm just wondering if there's a case where one fetch state is always getting full batches and another one that will never have a "full" batch? Like is it possible to be stuck where one never progresses
|
Well, theoretically, it might happen. But this means that the blocking chains generate more events than we can process - which is a bigger problem 😅 |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
scenarios/test_codegen/test/lib_tests/FetchState_test.res (1)
2721-2761: Exactly-full batch case proves off-by-one is fixed.This test would have failed under the previous
length - maxBatchSize - 1bug; it now passes and protects against regressions.
🧹 Nitpick comments (1)
scenarios/test_codegen/test/lib_tests/FetchState_test.res (1)
2631-2677: Unordered-batch: inclusion filter and timestamp ordering covered.
- Excludes non-eligible queues as expected.
- Orders remaining by earliest timestamp when neither is full.
Consider adding a boundary test for
~maxBatchSize=0to assert graceful behavior (after guarding in code).If you want, I can draft the extra test.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (4)
codegenerator/cli/README.md(1 hunks)codegenerator/cli/npm/envio/src/FetchState.res(1 hunks)codegenerator/cli/src/config_parsing/chain_helpers.rs(1 hunks)scenarios/test_codegen/test/lib_tests/FetchState_test.res(6 hunks)
✅ Files skipped from review due to trivial changes (1)
- codegenerator/cli/README.md
🧰 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:
codegenerator/cli/npm/envio/src/FetchState.resscenarios/test_codegen/test/lib_tests/FetchState_test.res
codegenerator/cli/npm/envio/**
📄 CodeRabbit Inference Engine (.cursor/rules/navigation.mdc)
Library-fied runtime shared across indexers lives in codegenerator/cli/npm/envio.
Files:
codegenerator/cli/npm/envio/src/FetchState.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 (5)
codegenerator/cli/src/config_parsing/chain_helpers.rs (1)
197-197: Couldn’t find any calls to from_network_id in the repository. It looks like this conversion helper isn’t being used anywhere, so adding a fallback won’t have any effect.Please verify where and how persisted network IDs are being deserialized and ensure that from_network_id is actually invoked. If configs are parsed via a different path (e.g., Serde deserialization or another helper), you’ll need to add the backward-compat mapping there instead of—or in addition to—this function.
codegenerator/cli/npm/envio/src/FetchState.res (1)
1254-1268: All filterAndSortForUnorderedBatch call sites now include~maxBatchSizeVerified that:
scenarios/test_codegen/test/lib_tests/FetchState_test.resat lines 2667, 2710, and 2752 all pass~maxBatchSize.codegenerator/cli/templates/static/codegen/src/eventFetching/ChainManager.res(line 188) pipesfetchStatesintofilterAndSortForUnorderedBatch(~maxBatchSize).LGTM—approving these changes.
scenarios/test_codegen/test/lib_tests/FetchState_test.res (3)
1817-1854: Good move: use public updateInternal for deterministic, minimal test setup.This removes test-only knowledge of internals and keeps fixtures concise.
2123-2148: Solid use of updateInternal to craft multi-partition + queue scenarios.Keeps test readability high and reuses the same selection instance as prod.
2705-2719: Priority of full vs half-full batches verified.This validates the core behavior change; nice use of constructed states to isolate the signal.
Prioritize chains with a buffer containing a whole batch of events. This way, it'll hit the preload optimization more effectively.
Summary by CodeRabbit
New Features
Documentation
Tests
Chores