Reproduce duplicate history on rollback bug #745
Conversation
WalkthroughIntroduces a buffer-based fetching model across FetchState, ChainFetcher, ChainManager, and GlobalState; removes targetBufferSize from SourceManager.fetchNext’s API; adds Batch.hasUnorderedNextItem; removes db_write_timestamp from schemas/templates; updates templates and tests to align with bufferBlockNumber and new signatures; adds Array.at and Promise.makeAsync bindings. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor App
participant CM as ChainManager
participant CF as ChainFetcher
participant SM as SourceManager
participant FS as FetchState
participant Src as Source
App->>CM: init (compute targetBufferSize)
CM->>CF: makeFromConfig(..., ~targetBufferSize)
CF->>FS: make(..., ~targetBufferSize, onBlockConfigs=[])
loop Fetch cycle
CM->>SM: fetchNext(~fetchState, ~currentBlockHeight, ~stateId)
SM->>FS: getNextQuery(fetchState, ~concurrencyLimit, ~currentBlockHeight, ~stateId)
FS-->>SM: query plan (fromBlock/toBlock/partitions)
SM->>Src: execute query (getItemsOrThrow)
Src-->>SM: items, latestFetchedBlock
SM->>FS: handleQueryResult(~query, ~latestFetchedBlock, ~newItems)
FS-->>SM: updated buffer/queue (bufferBlockNumber, mutItems)
SM-->>CM: done
end
note over CM,CF: Unordered mode check
CM->>FS: get state
CM->>FS: Batch.hasUnorderedNextItem(fetchStates)
FS-->>CM: bool
CM-->>App: scheduling decision
sequenceDiagram
autonumber
participant GS as GlobalState
participant CF as ChainFetcher
participant FS as FetchState
GS->>CF: update current block height
CF->>FS: read bufferBlockNumber
FS-->>CF: bufferBlockNumber
CF-->>GS: isFetchingAtHead := bufferBlockNumber >= currentBlockHeight
GS->>GS: StartProcessingBatch
GS->>GS: processedBatches += 1 on completion/rollback
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: 6
🧹 Nitpick comments (22)
scenarios/test_codegen/test/helpers/Mock.res (2)
271-285: Unused gsManagerRef.Ref is written once and never read; remove to reduce noise.
- let gsManagerRef = ref(None) ... - gsManagerRef := Some(gsManager)
49-69: Expose setEffectCacheOrThrowCalls or remove it.Counter is created but not part of Storage.t, so it’s inaccessible for assertions.
type t = { isInitializedCalls: array<bool>, resolveIsInitialized: bool => unit, initializeCalls: array<{ "entities": array<Internal.entityConfig>, "chainConfigs": array<InternalConfig.chain>, "enums": array<Internal.enumConfig<Internal.enum>>, }>, resolveInitialize: Persistence.initialState => unit, resumeInitialStateCalls: array<bool>, resolveLoadInitialState: Persistence.initialState => unit, loadByIdsOrThrowCalls: array<{"ids": array<string>, "tableName": string}>, loadByFieldOrThrowCalls: array<{ "fieldName": string, "fieldValue": unknown, "tableName": string, "operator": Persistence.operator, }>, dumpEffectCacheCalls: ref<int>, + setEffectCacheOrThrowCalls: ref<int>, storage: Persistence.storage, }And include it in the returned record where other refs are exposed.
- let setEffectCacheOrThrowCalls = ref(0) + let setEffectCacheOrThrowCalls = ref(0) ... { isInitializedCalls, initializeCalls, loadByIdsOrThrowCalls, loadByFieldOrThrowCalls, dumpEffectCacheCalls, + setEffectCacheOrThrowCalls,Also applies to: 95-96
scenarios/test_codegen/test/E2E_test.res (2)
9-9: Fix typo in test name.- Async.it("Currectly starts indexing from a non-zero start block", async () => { + Async.it("Correctly starts indexing from a non-zero start block", async () => {
6-7: Remove unused undefined workaround in this file.It’s not referenced here. Keep it only where needed.
-let undefined = (%raw(`undefined`): option<'a>)scenarios/erc20_multichain_factory/test/DynamicContractRecovery_test.res (1)
171-172: Hard-coded targetBufferSize; consider centralizing.If 5000 is a convention, lift it to a shared constant/config to keep tests consistent and avoid drift.
Would you like me to scan tests for other hard-coded buffer sizes and propose a constants module?
codegenerator/cli/templates/static/codegen/src/Index.res (1)
187-187: Clamp latestFetchedBlockNumber to chain startBlock (and endBlock if present).Using 0 as a floor can mislead for chains with startBlock > 0. Also consider capping to endBlock for TUI correctness once reached.
Apply this diff:
- let latestFetchedBlockNumber = Pervasives.max(fetchState->FetchState.bufferBlockNumber, 0) + let bufferBlock = fetchState->FetchState.bufferBlockNumber + let startBlock = cf.chainConfig.startBlock + let latestFetchedBlockNumber = + switch cf.fetchState.endBlock { + | Some(endBlock) => bufferBlock->Belt.Int.max(startBlock)->Belt.Int.min(endBlock) + | None => bufferBlock->Belt.Int.max(startBlock) + }codegenerator/cli/npm/envio/src/Batch.res (1)
61-71: Helper looks good; add explicit return type and brief doc.Small polish for readability and API clarity.
Apply this diff:
-let hasUnorderedNextItem = (fetchStates: ChainMap.t<FetchState.t>) => { +/** Returns true if any actively indexing chain has an item available (unordered mode). */ +let hasUnorderedNextItem = (fetchStates: ChainMap.t<FetchState.t>): bool => { fetchStates ->ChainMap.values ->Js.Array2.some(fetchState => { fetchState->FetchState.isActivelyIndexing && switch fetchState->FetchState.getEarliestEvent { | Item(_) => true | NoItem(_) => false } }) }scenarios/test_codegen/test/rollback/Rollback_test.res (4)
4-61: Avoid module name shadowing: local Mock hides the imported Mock.You alias the imported Mock as M, then define a local module Mock. This is confusing and brittle. Prefer renaming the local one (e.g., MockData or LocalMock) to avoid accidental shadowing.
Would you like me to generate a scripted, safe rename across this file?
515-598: Fix assertion message (says “two” but asserts three entities).The expected list contains 3 SimpleEntity rows; the message claims two.
Apply this diff:
- ~message="Should have two entities in the db", + ~message="Should have three entities in the db",
971-976: Replace FIXME with tracked TODO or adjust expectation when fixing.Tests contain “// FIXME: This looks wrong” next to history.previous. Either document with an issue link or codify the intended behavior so the test won’t rot post-fix.
Also applies to: 1354-1360
427-441: Extract a microtask “flush” helper to reduce flakiness.Multiple Utils.delay(0) calls suggest timing sensitivity. Add a shared flush helper (e.g., with Promise.makeAsync twice) and use it across tests.
scenarios/test_codegen/test/lib_tests/FetchState_onBlock_test.res (1)
96-108: Avoid magic logIndex sentinel; bind to a constant.16777216 is opaque. If FetchState exposes a base logIndex constant, use it; otherwise define a local const with a comment.
codegenerator/cli/templates/static/codegen/src/eventFetching/ChainManager.res (2)
9-18: Target buffer sizing: confirm the branching logicThe heuristic reduces per-chain buffer (x1) when activeChainsCount > 3, else uses x3. That seems intentional to cap total memory as chains grow. Please confirm this is the intended direction; the AI summary states the opposite.
Optional tidy-up for readability:
-let calculateTargetBufferSize = activeChainsCount => { - let targetBatchesInBuffer = 3 - switch Env.targetBufferSize { - | Some(size) => size - | None => - Env.maxProcessBatchSize * ( - activeChainsCount > targetBatchesInBuffer ? 1 : targetBatchesInBuffer - ) - } -} +let calculateTargetBufferSize = activeChainsCount => { + let minBatches = 1 + let maxBatches = 3 + switch Env.targetBufferSize { + | Some(size) => size + | None => { + let batchesPerChain = activeChainsCount > maxBatches ? minBatches : maxBatches + Env.maxProcessBatchSize * batchesPerChain + } + } +}
48-52: Good metrics surface; consider setting at cold-start tooPrometheus metrics for max batch, target buffer, and reorg threshold are set on resume. Consider also setting IndexingTargetBufferSize in makeFromConfig for cold starts so dashboards don’t miss initial runs.
let makeFromConfig = (~config: Config.t): t => { let targetBufferSize = calculateTargetBufferSize(config.chainMap->ChainMap.size) + Prometheus.IndexingTargetBufferSize.set(~targetBufferSize) let chainFetchers = config.chainMap->ChainMap.map(ChainFetcher.makeFromConfig(_, ~config, ~targetBufferSize)) {scenarios/test_codegen/test/ChainManager_test.res (1)
50-55: Off‑by‑one in random event generation loopfor logIndex in 0 to numberOfEventsInBatch generates numberOfEventsInBatch+1 events. If that’s unintentional, iterate up to numberOfEventsInBatch - 1.
- for logIndex in 0 to numberOfEventsInBatch { + if numberOfMockEventsCreated.contents >= 0 { /* keep structure; guard for zero */ + for logIndex in 0 to (max(numberOfEventsInBatch - 1, -1)) { let batchItem = Internal.Event({ /* ... */ }) /* ... */ - } + } + }codegenerator/cli/templates/static/codegen/src/globalState/GlobalState.res (2)
153-166: Head-tracking latch may never demote after DC registrationisFetchingAtHead is latched true once bufferBlockNumber >= currentBlockHeight. If dynamic contracts register later (and buffers fall behind), this stays true, potentially overstating “at head” and prematurely setting timestampCaughtUpToHeadOrEndblock.
Consider explicitly demoting on DC registration for that chain if it falls behind:
let submitPartitionQueryResponse = ( state, ~newItems, ~dynamicContracts, ~currentBlockHeight, ~latestFetchedBlock, ~query, ~chain, ) => { let chainFetcher = state.chainManager.chainFetchers->ChainMap.get(chain) let updatedChainFetcher = chainFetcher ->ChainFetcher.handleQueryResult(~query, ~latestFetchedBlock, ~newItems, ~dynamicContracts) ->Utils.unwrapResultExn ->updateChainFetcherCurrentBlockHeight(~currentBlockHeight) + // Demote head status if newly-registered contracts push us behind the head + let updatedChainFetcher = + dynamicContracts->Js.Array2.length > 0 + && updatedChainFetcher.isFetchingAtHead + && updatedChainFetcher.fetchState->FetchState.bufferBlockNumber < currentBlockHeight + ? {...updatedChainFetcher, isFetchingAtHead: false} + : updatedChainFetcher
467-472: Add chain context to “All events have been fetched” logMinor usability: include chain id to ease debugging.
- if !wasFetchingAtHead && isCurrentlyFetchingAtHead { - updatedChainFetcher.logger->Logging.childInfo("All events have been fetched") - } + if !wasFetchingAtHead && isCurrentlyFetchingAtHead { + updatedChainFetcher.logger->Logging.childInfo({ + "msg": "All events have been fetched", + "chainId": updatedChainFetcher.chainConfig.id, + }) + }scenarios/test_codegen/test/lib_tests/SourceManager_test.res (1)
807-875: Time-based assertions may be flaky under loadTests rely on Utils.delay with very small intervals (0/1/2 ms). CI noise can cause intermittent failures.
- Increase pollingInterval/stalledPollingInterval/retry intervals by 1–2 orders of magnitude within tests.
- Prefer deterministic virtual timers or inject a clock where feasible.
Also applies to: 897-1039, 1041-1179, 1181-1237
codegenerator/cli/npm/envio/src/FetchState.res (4)
254-261: OOM guard: fix off‑by‑one and make pivot robust.
- Loop can add targetBufferSize+1 items due to <=.
- maxBlockNumber pivot is taken from potentially unsorted mutItems; this can under‑limit or over‑limit buffering.
Apply:
- }->Utils.Array.at(-fetchState.targetBufferSize) { + }->Utils.Array.at(-fetchState.targetBufferSize) { ... } ... - while ( + while ( latestOnBlockBlockNumber.contents < maxBlockNumber && - // Additional safeguard to prevent OOM - newItemsCounter.contents <= fetchState.targetBufferSize + // Additional safeguard to prevent OOM + newItemsCounter.contents < fetchState.targetBufferSize ) {Optionally, compute the pivot from the sorted source to avoid relying on unsorted mutItems:
- let maxBlockNumber = switch switch mutItemsRef.contents { - | Some(mutItems) => mutItems - | None => fetchState.queue - }->Utils.Array.at(-fetchState.targetBufferSize) { + let sourceForPivot = + switch mutItemsRef.contents { + | Some(mutItems) => { + let copy = mutItems->Array.copy + copy->Js.Array2.sortInPlaceWith(compareBufferItem) + copy + } + | None => fetchState.queue + } + let maxBlockNumber = switch sourceForPivot->Utils.Array.at(-fetchState.targetBufferSize) {Also applies to: 268-279, 332-336
331-336: Typos in comments.
- “asume” → “assume”
- “respose” → “response”
- “Comparitor” → “Comparator”
Also applies to: 903-907, 206-208
871-886: Validate targetBufferSize > 0.Zero or negative values break pivot math (e.g., at(-0) / length - 0) and gating. Enforce at construction.
Apply:
let make = ( ~startBlock, ~endBlock, ~eventConfigs: array<Internal.eventConfig>, ~contracts: array<indexingContract>, ~maxAddrInPartition, ~chainId, ~targetBufferSize, ~progressBlockNumber=startBlock - 1, ~onBlockConfigs=[], ~blockLag=0, ): t => { + if targetBufferSize <= 0 { + Js.Exn.raiseError("Invalid configuration: targetBufferSize must be > 0") + }Also applies to: 1007-1011
1287-1291: Guard hasFullBatch against maxBatchSize <= 0.Avoid unsafe_get and edge-case misclassification.
Apply:
- let hasFullBatch = ({queue} as fetchState: t, ~maxBatchSize) => { + let hasFullBatch = ({queue} as fetchState: t, ~maxBatchSize) => { + if maxBatchSize <= 0 { false } else { let targetBlockIdx = queue->Array.length - maxBatchSize if targetBlockIdx < 0 { false } else { queue->Js.Array2.unsafe_get(targetBlockIdx)->Internal.getItemBlockNumber <= fetchState->bufferBlockNumber } - } + } }Also applies to: 1294-1307
📜 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 (25)
codegenerator/cli/npm/envio/src/Batch.res(1 hunks)codegenerator/cli/npm/envio/src/FetchState.res(18 hunks)codegenerator/cli/npm/envio/src/Utils.res(1 hunks)codegenerator/cli/npm/envio/src/bindings/Promise.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(15 hunks)scenarios/erc20_multichain_factory/test/DynamicContractRecovery_test.res(1 hunks)scenarios/test_codegen/schema.graphql(1 hunks)scenarios/test_codegen/test/ChainManager_test.res(3 hunks)scenarios/test_codegen/test/E2E_test.res(1 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/helpers/Mock.res(2 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(17 hunks)scenarios/test_codegen/test/rollback/Rollback_test.res(2 hunks)
💤 Files with no reviewable changes (5)
- codegenerator/cli/templates/dynamic/codegen/src/db/Entities.res.hbs
- codegenerator/cli/npm/envio/src/sources/SourceManager.res
- codegenerator/cli/npm/envio/src/db/InternalTable.res
- codegenerator/cli/npm/envio/src/sources/SourceManager.resi
- codegenerator/cli/npm/envio/src/db/EntityHistory.res
🧰 Additional context used
📓 Path-based instructions (5)
**/*.{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/bindings/Promise.rescodegenerator/cli/npm/envio/src/Batch.rescodegenerator/cli/templates/static/codegen/src/Index.resscenarios/test_codegen/test/E2E_test.resscenarios/test_codegen/test/lib_tests/FetchState_onBlock_test.rescodegenerator/cli/npm/envio/src/Utils.resscenarios/erc20_multichain_factory/test/DynamicContractRecovery_test.resscenarios/test_codegen/test/lib_tests/PgStorage_test.resscenarios/test_codegen/test/rollback/Rollback_test.rescodegenerator/cli/templates/static/codegen/src/globalState/GlobalState.resscenarios/test_codegen/test/SerDe_Test.resscenarios/test_codegen/test/lib_tests/FetchState_test.rescodegenerator/cli/templates/static/codegen/src/eventFetching/ChainFetcher.resscenarios/test_codegen/test/ChainManager_test.resscenarios/test_codegen/test/lib_tests/SourceManager_test.rescodegenerator/cli/npm/envio/src/FetchState.rescodegenerator/cli/templates/static/codegen/src/eventFetching/ChainManager.resscenarios/test_codegen/test/helpers/Mock.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/bindings/Promise.rescodegenerator/cli/npm/envio/src/Batch.rescodegenerator/cli/templates/static/codegen/src/Index.resscenarios/test_codegen/test/E2E_test.resscenarios/test_codegen/test/lib_tests/FetchState_onBlock_test.rescodegenerator/cli/npm/envio/src/Utils.resscenarios/erc20_multichain_factory/test/DynamicContractRecovery_test.resscenarios/test_codegen/test/lib_tests/PgStorage_test.resscenarios/test_codegen/test/rollback/Rollback_test.rescodegenerator/cli/templates/static/codegen/src/globalState/GlobalState.resscenarios/test_codegen/test/SerDe_Test.resscenarios/test_codegen/test/lib_tests/FetchState_test.rescodegenerator/cli/templates/static/codegen/src/eventFetching/ChainFetcher.resscenarios/test_codegen/test/ChainManager_test.resscenarios/test_codegen/test/lib_tests/SourceManager_test.rescodegenerator/cli/npm/envio/src/FetchState.rescodegenerator/cli/templates/static/codegen/src/eventFetching/ChainManager.resscenarios/test_codegen/test/helpers/Mock.res
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
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/globalState/GlobalState.rescodegenerator/cli/templates/static/codegen/src/eventFetching/ChainFetcher.rescodegenerator/cli/templates/static/codegen/src/eventFetching/ChainManager.res
scenarios/test_codegen/**/schema.graphql
📄 CodeRabbit inference engine (scenarios/test_codegen/.cursor/rules/hyperindex.mdc)
scenarios/test_codegen/**/schema.graphql: After changing schema.graphql, run: pnpm codegen
Do not add the @entity decorator to GraphQL schema types
Avoid time-series aggregate fields (e.g., dailyVolume) in the GraphQL schema
Never use arrays of entities in the schema (e.g., [User!]!)
Model relationships using *_id fields in the schema (e.g., user_id: String!) instead of entity references
Files:
scenarios/test_codegen/schema.graphql
🧠 Learnings (14)
📚 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.tsscenarios/test_codegen/schema.graphql
📚 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: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
📚 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.tsscenarios/test_codegen/schema.graphql
📚 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/**/*.graphql : Use `entity_id` fields for relationships (e.g., `token_id: string`) rather than direct object references in the GraphQL schema
Applied to files:
scenarios/test_codegen/schema.graphql
📚 Learning: 2025-08-11T08:42:57.311Z
Learnt from: CR
PR: enviodev/hyperindex#0
File: codegenerator/cli/templates/static/shared/.cursor/rules/hyperindex.mdc:0-0
Timestamp: 2025-08-11T08:42:57.311Z
Learning: Applies to codegenerator/cli/templates/static/shared/**/*.graphql : In GraphQL schema, use `entity_id` fields (e.g., `token_id: String!`) for relationships instead of direct object references
Applied to files:
scenarios/test_codegen/schema.graphql
📚 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/**/schema.graphql : Model relationships using *_id fields in the schema (e.g., user_id: String!) instead of entity references
Applied to files:
scenarios/test_codegen/schema.graphql
📚 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/**/schema.graphql : Do not add the entity decorator to GraphQL schema types
Applied to files:
scenarios/test_codegen/schema.graphql
📚 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/**/*.graphql : Do not add the `entity` decorator to GraphQL schema types
Applied to files:
scenarios/test_codegen/schema.graphql
📚 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/**/schema.graphql : Avoid time-series aggregate fields (e.g., dailyVolume) in the GraphQL schema
Applied to files:
scenarios/test_codegen/schema.graphql
📚 Learning: 2025-08-11T08:42:57.311Z
Learnt from: CR
PR: enviodev/hyperindex#0
File: codegenerator/cli/templates/static/shared/.cursor/rules/hyperindex.mdc:0-0
Timestamp: 2025-08-11T08:42:57.311Z
Learning: Applies to codegenerator/cli/templates/static/shared/**/*.graphql : Use `entity_id` fields for relationships instead of entity arrays in the schema
Applied to files:
scenarios/test_codegen/schema.graphql
📚 Learning: 2025-05-27T17:07:12.878Z
Learnt from: JonoPrest
PR: enviodev/hyperindex#555
File: scenarios/test_codegen/test/lib_tests/EntityHistory_test.res:631-659
Timestamp: 2025-05-27T17:07:12.878Z
Learning: In isolated tests for EntityHistory rollbacks with controlled mock data setup, the ordering of getRollbackDiff results is deterministic and doesn't require sorting before comparison with deepStrictEqual.
Applied to files:
scenarios/test_codegen/test/rollback/Rollback_test.res
🔇 Additional comments (32)
scenarios/test_codegen/schema.graphql (1)
153-156: LGTM: Appropriate addition of SimpleEntity type.The new SimpleEntity type follows the coding guidelines correctly - it uses basic scalar fields (ID! and String!) without entity decorators or relationship arrays, which aligns with the established patterns for this schema.
scenarios/test_codegen/test/entity-column-types-test.ts (1)
161-161: Correct test expectation update for exact column count.The change from allowing an extra column (
actualColumns.length === expectedColumns.length + 1) to requiring exact match (actualColumns.length === expectedColumns.length) is correct. This aligns with the removal of thedb_write_timestampcolumn from generated entity tables.scenarios/test_codegen/test/lib_tests/PgStorage_test.res (1)
68-68: Consistent removal of db_write_timestamp from SQL generation tests.All the test expectations have been correctly updated to remove
db_write_timestampcolumns from:
- Entity table CREATE statements (A, B, raw_events)
- Entity history table structures
- The ENTITY_TYPE enum now includes SimpleEntity
These changes are consistent with the PR's objective to fix duplicate history on rollback bugs by removing db_write_timestamp infrastructure.
Let me verify the impact of removing db_write_timestamp on entity history and rollback operations:
Also applies to: 78-78, 88-88, 146-146, 150-151, 153-153, 235-235, 317-318
codegenerator/cli/npm/envio/src/Utils.res (1)
306-308: New Array.at binding looks good; verify runtime support.Binding shape matches other option-returning externals in this file. Please confirm all runtimes (Node/browsers) used by the CLI/tests support Array.prototype.at or are polyfilled.
scenarios/test_codegen/test/rollback/Rollback_test.res (1)
415-418: OK to unblock; remove once on ReScript v12.The undefined shim is fine for v11. Please gate it with a TODO referencing the upgrade and delete when you bump to v12.
scenarios/test_codegen/test/lib_tests/FetchState_onBlock_test.res (1)
239-263: Nice coverage of multiple onBlock configs.Good, deterministic ordering assertions across intervals and indices.
codegenerator/cli/templates/static/codegen/src/eventFetching/ChainFetcher.res (3)
271-276: Filter cleanup condition looks correct.Keeping the filter while bufferBlockNumber <= restartBlockNumber and dropping it afterwards matches the comment.
Consider adding a unit test that advances bufferBlockNumber past the threshold to prove cleanup occurs.
469-475: Good: registerDynamicContracts before handling query result.This ensures indexingContracts are up-to-date for queueing. No currentBlockHeight threading required here.
17-17: isFetchingAtHead field added — call sites migratedConfirmed: ChainFetcher.t defines isFetchingAtHead and all usages reference the new field or the ChainManager helper; no remaining calls to the old helper were found.
- codegenerator/cli/templates/static/codegen/src/eventFetching/ChainFetcher.res (field + default)
- codegenerator/cli/templates/static/codegen/src/eventFetching/ChainManager.res (manager helper + Js.Array2.every(cf => cf.isFetchingAtHead))
- codegenerator/cli/templates/static/codegen/src/globalState/GlobalState.res (multiple uses/assignments)
- scenarios/test_codegen/test/ChainManager_test.res (test fixture)
codegenerator/cli/templates/static/codegen/src/eventFetching/ChainManager.res (3)
21-23: Threading targetBufferSize through fetcher construction looks goodPassing ~targetBufferSize into ChainFetcher.makeFromConfig keeps initialization consistent.
105-108: nextItemIsNone branching is correctOrdered/unordered branches align with Batch API (getOrderedNextItem vs hasUnorderedNextItem).
213-214: Use of cf.isFetchingAtHead is consistent with new ChainFetcher.t shapeSwitching from a helper to the field makes sense given the refactor.
Please confirm all test/mocks initialize isFetchingAtHead (I see updates in ChainManager_test.res).
scenarios/test_codegen/test/ChainManager_test.res (1)
96-117: Mock fetcher shape aligns with ChainFetcher.tAdding isFetchingAtHead: false keeps tests compatible with the new field.
Also applies to: 114-116
scenarios/test_codegen/test/lib_tests/FetchState_test.res (3)
6-7: Consistent targetBufferSize usage in testsThreading targetBufferSize via FetchState.make and via state overrides matches the new API.
Also applies to: 80-86, 98-103
126-129: Use of %raw(undefined) is acceptable as a v11 workaroundAcknowledged in comment; safe to keep until bumping ReScript to v12.
Please confirm CI uses v11 where this is needed; remove once on v12.
2824-2851: Buffer overflow prevention test is solidClear, deterministic expectations for limiting to maxQueryBlockNumber based on targetBufferSize.
Also applies to: 2893-2915
codegenerator/cli/templates/static/codegen/src/globalState/GlobalState.res (4)
171-188: Metadata uses bufferBlockNumber now—confirm semanticslatestFetchedBlockNumber is set from fetchState.bufferBlockNumber. If dashboards previously relied on fully-fetched semantics, confirm this is the intended visible metric.
Also applies to: 183-184
286-317: Synced-at-head timestamp gating looks good; relies on accurate isFetchingAtHeadGiven the latch above, correctness depends on proper demotion when new work appears.
659-660: Batch processing state transitions are soundStartProcessingBatch is dispatched for both progress-only and normal batches; currentlyProcessingBatch is reset on EventBatchProcessed and on invalidation.
Also applies to: 928-941
1080-1093: Reorg filter lifetime condition: double-check boundaryFilter validity uses fetchState.bufferBlockNumber <= firstChangeEvent.blockNumber. Confirm inclusive boundary matches “remove filter once fetched past” intent.
scenarios/test_codegen/test/lib_tests/SourceManager_test.res (2)
1252-1260: executeQuery test: asserts include fromBlock in call records—nice coverageGood verification of new shape (fromBlock/toBlock/retry).
1319-1420: Retry rotation behavior is well coveredClear expectation of initial retries, immediate source switching, and rotation cadence.
codegenerator/cli/npm/envio/src/FetchState.res (10)
185-204: Buffer boundary helpers are correct (min frontier).Semantics of bufferBlockNumber/bufferBlock look good and align with the “ready-to-consume” invariant.
205-216: Comparator matches queue invariant (latest → earliest).Descending by (blockNumber, logIndex) is consistent with popping from the end.
217-219: Guard blockItemLogIndex against collision with real logIndex.Ensure runtime logIndex from sources can’t exceed 16,777,216. Add an assert or property test in Internal.item creation, or document the guarantee.
339-351: Metrics updates are sensible.Partition count, buffer size, and buffer block number are updated atomically with state.
695-701: Queue merge path is correct.Concatenate then sort at updateInternal keeps the invariant.
979-995: Earliest-item gating vs buffer boundary looks good.Pop-from-end is consistent with ordering; NoItem returns bufferBlock for consumers.
1129-1137: Initialize latestOnBlockBlockNumber from progress.Reasonable default. Please add a test covering non-zero progress with onBlockConfigs to ensure no onBlock gaps/duplication across startup.
1164-1171: Rollback sets latestFetchedBlock to previous block.Conservative choice to re-fetch the block. Confirm this doesn’t reintroduce duplicate onBlock items (given pruning) in mixed Event/Block queues.
1260-1271: Active indexing heuristic OK.Uses bufferBlockNumber and buffer size to stop after endBlock is passed.
1334-1341: Progress helpers align with buffer boundary.Correctly derive progress block and next log index only when within buffer.
Also applies to: 1343-1351
| @new | ||
| external makeAsync: ((@uncurry 'a => unit, 'e => unit) => promise<unit>) => t<'a> = "Promise" | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
Don’t use async Promise executors; replace external makeAsync with a safe wrapper.
JS ignores the executor’s returned promise, so async executors won’t propagate rejections to the created Promise, leading to silent failures. Remove the external constructor overload and implement a wrapper that wires rejections correctly.
Apply this diff to remove the unsafe external:
-@new
-external makeAsync: ((@uncurry 'a => unit, 'e => unit) => promise<unit>) => t<'a> = "Promise"Then add this helper (place it below the existing externals so then/catch are available):
let makeAsync = (executor: (@uncurry 'a => unit, 'e => unit) => promise<unit>): t<'a> =>
make((resolve, reject) => {
executor(resolve, reject)
->ignoreValue
->thenResolve(_ => ())
->catch(err => reject(Js.Exn.anyToExnInternal(err)))
->done
})Run to locate usages that rely on the removed external:
🏁 Script executed:
#!/bin/bash
rg -n --type=res '\bPromise\.makeAsync\b' -C2Length of output: 27
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
printf "pwd: %s\n" "$(pwd)"
printf "\n--- list .res files (first 200) ---\n"
if command -v fd >/dev/null 2>&1; then
fd -e res -H | sed -n '1,200p' || true
else
find . -type f -name '*.res' | sed -n '1,200p' || true
fi
target="codegenerator/cli/npm/envio/src/bindings/Promise.res"
printf "\n--- show %s if present ---\n" "$target"
if [ -f "$target" ]; then
sed -n '1,200p' "$target"
else
printf "%s not found\n" "$target"
if [ -d "codegenerator/cli/npm/envio/src/bindings" ]; then
printf "\n--- listing directory codegenerator/cli/npm/envio/src/bindings ---\n"
ls -la codegenerator/cli/npm/envio/src/bindings || true
fi
fi
printf "\n--- ripgrep searches for makeAsync/usages ---\n"
if command -v rg >/dev/null 2>&1; then
printf "Searching for 'Promise.makeAsync'...\n"
rg -n --hidden -S "Promise\\.makeAsync" -g '!*node_modules/**' -C2 || true
printf "\nSearching for word 'makeAsync'...\n"
rg -n --hidden -S "\\bmakeAsync\\b" -g '!*node_modules/**' -C2 || true
printf "\nSearching for 'external makeAsync' definitions...\n"
rg -n --hidden -S "external\\s+makeAsync" -g '!*node_modules/**' -C2 || true
else
printf "rg not found, falling back to grep (may be slower)...\n"
grep -RIn --exclude-dir=node_modules --exclude-dir=vendor --line-number -E "Promise\\.makeAsync|\\bmakeAsync\\b|external\\s+makeAsync" . || true
fiLength of output: 11484
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
printf "pwd: %s\n" "$(pwd)"
printf "\n--- show Promise.res (with line numbers) ---\n"
nl -ba codegenerator/cli/npm/envio/src/bindings/Promise.res | sed -n '1,240p' || true
printf "\n--- grep for 'makeAsync' or 'Promise.makeAsync' across repo ---\n"
grep -RIn --exclude-dir=node_modules --exclude-dir=.git --exclude-dir=dist -E '\bmakeAsync\b|Promise\.makeAsync' . || trueLength of output: 540
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
printf "\n--- Mock.res lines 280-320 ---\n"
sed -n '280,320p' scenarios/test_codegen/test/helpers/Mock.res || trueLength of output: 1499
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
printf "\n--- scenarios/test_codegen/test/helpers/Mock.res top lines 1-50 ---\n"
sed -n '1,50p' scenarios/test_codegen/test/helpers/Mock.res || trueLength of output: 1339
Replace unsafe async Promise executor with a safe wrapper.
JS ignores the executor’s returned promise, so async executors won’t propagate rejections. Remove the unsafe external and add a wrapper that wires rejections to reject.
Change (file: codegenerator/cli/npm/envio/src/bindings/Promise.res). Usages found: scenarios/test_codegen/test/helpers/Mock.res:295, 304.
Apply this diff to remove the unsafe external:
-@new
-external makeAsync: ((@uncurry 'a => unit, 'e => unit) => promise<unit>) => t<'a> = "Promise"Then add this helper (place it below the existing externals so then/catch are available):
let makeAsync = (executor: (@uncurry 'a => unit, 'e => unit) => promise<unit>): t<'a> =>
make((resolve, reject) => {
executor(resolve, reject)
->ignoreValue
->thenResolve(_ => ())
->catch(err => reject(Js.Exn.anyToExnInternal(err)))
->done
})🤖 Prompt for AI Agents
In codegenerator/cli/npm/envio/src/bindings/Promise.res around lines 6 to 8,
replace the unsafe external declaration of makeAsync with a safe wrapper: remove
the external binding that maps directly to JS Promise (which ignores any promise
returned by an async executor) and instead add a ReScript helper function
(placed below the other externals so then/catch are available) that calls
make((resolve, reject) => { executor(resolve, reject) |> ignoreValue |>
thenResolve(_ => ()) |> catch(err => reject(Js.Exn.anyToExnInternal(err))) |>
done }), ensuring rejections from an async executor are forwarded to reject;
update usages in scenarios/test_codegen/test/helpers/Mock.res if necessary.
| { | ||
| ...fetchState, | ||
| latestOnBlockBlockNumber: firstChangeEvent.blockNumber - 1, // TODO: This is not tested | ||
| }->updateInternal( | ||
| ~partitions, | ||
| ~indexingContracts, | ||
| ~queue=fetchState.queue->pruneQueueFromFirstChangeEvent(~firstChangeEvent), | ||
| ~mutItems=fetchState.queue->pruneQueueFromFirstChangeEvent(~firstChangeEvent), | ||
| ~dcsToStore=switch fetchState.dcsToStore { |
There was a problem hiding this comment.
🧩 Analysis chain
Bug: wrong optional-arg passing to updateInternal (~mutItems).
updateInternal expects option<array<Internal.item>>. Here an array is passed directly, which will not type-check and would skip pruning on rollback if coerced.
Apply:
- }->updateInternal(
+ }->updateInternal(
~partitions,
~indexingContracts,
- ~mutItems=fetchState.queue->pruneQueueFromFirstChangeEvent(~firstChangeEvent),
+ ~mutItems=?{
+ let pruned = fetchState.queue->pruneQueueFromFirstChangeEvent(~firstChangeEvent)
+ switch pruned {
+ | [] => None
+ | _ => Some(pruned)
+ }
+ },
~dcsToStore=switch fetchState.dcsToStore {Run to find other call sites that might have the same issue:
🏁 Script executed:
#!/bin/bash
rg -nP -C2 '\bupdateInternal\s*\([^)]*~mutItems\s*='Length of output: 532
Bug: pass an option for ~mutItems (wrap arrays in Some/None)
updateInternal expects option<array<Internal.item>>; currently an array is passed directly which can skip pruning on rollback.
Apply:
- }->updateInternal(
+ }->updateInternal(
~partitions,
~indexingContracts,
- ~mutItems=fetchState.queue->pruneQueueFromFirstChangeEvent(~firstChangeEvent),
+ ~mutItems=?{
+ let pruned = fetchState.queue->pruneQueueFromFirstChangeEvent(~firstChangeEvent)
+ switch pruned {
+ | [] => None
+ | _ => Some(pruned)
+ }
+ },
~dcsToStore=switch fetchState.dcsToStore {Also update test caller: scenarios/test_codegen/test/lib_tests/FetchState_test.res:2633 — replace
~mutItems=[mockEvent(~blockNumber=6)] with ~mutItems=Some([mockEvent(~blockNumber=6)]).
📝 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.
| { | |
| ...fetchState, | |
| latestOnBlockBlockNumber: firstChangeEvent.blockNumber - 1, // TODO: This is not tested | |
| }->updateInternal( | |
| ~partitions, | |
| ~indexingContracts, | |
| ~queue=fetchState.queue->pruneQueueFromFirstChangeEvent(~firstChangeEvent), | |
| ~mutItems=fetchState.queue->pruneQueueFromFirstChangeEvent(~firstChangeEvent), | |
| ~dcsToStore=switch fetchState.dcsToStore { | |
| { | |
| ...fetchState, | |
| latestOnBlockBlockNumber: firstChangeEvent.blockNumber - 1, // TODO: This is not tested | |
| }->updateInternal( | |
| ~partitions, | |
| ~indexingContracts, | |
| ~mutItems=?{ | |
| let pruned = fetchState.queue->pruneQueueFromFirstChangeEvent(~firstChangeEvent) | |
| switch pruned { | |
| | [] => None | |
| | _ => Some(pruned) | |
| } | |
| }, | |
| ~dcsToStore=switch fetchState.dcsToStore { |
🤖 Prompt for AI Agents
In codegenerator/cli/npm/envio/src/FetchState.res around lines 1236 to 1243,
update the call to updateInternal so that ~mutItems is passed as an option
(Some(array) or None) instead of passing the array directly; replace
~mutItems=fetchState.queue->pruneQueueFromFirstChangeEvent(~firstChangeEvent)
with wrapping the result in Some(...) or None as appropriate so updateInternal
receives option<array<Internal.item>> and pruning works correctly on rollback.
Also update the test caller in
scenarios/test_codegen/test/lib_tests/FetchState_test.res at the indicated
location (around line 2633) to change ~mutItems=[mockEvent(~blockNumber=6)] to
~mutItems=Some([mockEvent(~blockNumber=6)]).
| Promise.makeAsync(async (resolve, _reject) => { | ||
| let before = (gsManager->GlobalStateManager.getState).processedBatches | ||
| while before >= (gsManager->GlobalStateManager.getState).processedBatches { | ||
| await Utils.delay(50) | ||
| } | ||
| resolve() | ||
| }) | ||
| }, |
There was a problem hiding this comment.
Avoid async Promise executor in getBatchWritePromise.
Use Promise.make with chaining; don’t pass an async executor.
- Promise.makeAsync(async (resolve, _reject) => {
- let before = (gsManager->GlobalStateManager.getState).processedBatches
- while before >= (gsManager->GlobalStateManager.getState).processedBatches {
- await Utils.delay(50)
- }
- resolve()
- })
+ Promise.make((resolve, _reject) => {
+ let before = (gsManager->GlobalStateManager.getState).processedBatches
+ let rec poll = () => {
+ if before < (gsManager->GlobalStateManager.getState).processedBatches {
+ resolve()
+ } else {
+ Utils.delay(50)->Promise.thenResolve(_ => poll())->Promise.done
+ }
+ }
+ poll()
+ })📝 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.
| Promise.makeAsync(async (resolve, _reject) => { | |
| let before = (gsManager->GlobalStateManager.getState).processedBatches | |
| while before >= (gsManager->GlobalStateManager.getState).processedBatches { | |
| await Utils.delay(50) | |
| } | |
| resolve() | |
| }) | |
| }, | |
| Promise.make((resolve, _reject) => { | |
| let before = (gsManager->GlobalStateManager.getState).processedBatches | |
| let rec poll = () => { | |
| if before < (gsManager->GlobalStateManager.getState).processedBatches { | |
| resolve() | |
| } else { | |
| Utils.delay(50)->Promise.thenResolve(_ => poll())->Promise.done | |
| } | |
| } | |
| poll() | |
| }) | |
| }, |
🤖 Prompt for AI Agents
In scenarios/test_codegen/test/helpers/Mock.res around lines 295 to 302, the
Promise.makeAsync executor is async (an async function passed to the Promise
constructor) which should be avoided; change this to use Promise.make with
chaining instead of an async executor. Replace the async executor by creating a
Promise.make that kicks off a non-async function which uses a loop implemented
with then-chained calls (or a recursive function) that awaits Utils.delay via
Promise chaining until (gsManager->GlobalStateManager.getState).processedBatches
increments, then resolves; do not use an async executor or await inside the
Promise constructor.
| Promise.makeAsync(async (resolve, _reject) => { | ||
| while ( | ||
| switch (gsManager->GlobalStateManager.getState).rollbackState { | ||
| | RollbackInMemStore(_) => false | ||
| | RollingBack(_) | ||
| | NoRollback => true | ||
| } | ||
| ) { | ||
| await Utils.delay(50) | ||
| } | ||
| resolve() | ||
| }) | ||
| }, |
There was a problem hiding this comment.
Same issue for getRollbackReadyPromise; replace async executor.
- Promise.makeAsync(async (resolve, _reject) => {
- while (
- switch (gsManager->GlobalStateManager.getState).rollbackState {
- | RollbackInMemStore(_) => false
- | RollingBack(_)
- | NoRollback => true
- }
- ) {
- await Utils.delay(50)
- }
- resolve()
- })
+ Promise.make((resolve, _reject) => {
+ let rec poll = () => {
+ let waiting =
+ switch (gsManager->GlobalStateManager.getState).rollbackState {
+ | RollbackInMemStore(_) => false
+ | RollingBack(_)
+ | NoRollback => true
+ }
+ if waiting {
+ Utils.delay(50)->Promise.thenResolve(_ => poll())->Promise.done
+ } else {
+ resolve()
+ }
+ }
+ poll()
+ })📝 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.
| Promise.makeAsync(async (resolve, _reject) => { | |
| while ( | |
| switch (gsManager->GlobalStateManager.getState).rollbackState { | |
| | RollbackInMemStore(_) => false | |
| | RollingBack(_) | |
| | NoRollback => true | |
| } | |
| ) { | |
| await Utils.delay(50) | |
| } | |
| resolve() | |
| }) | |
| }, | |
| Promise.make((resolve, _reject) => { | |
| let rec poll = () => { | |
| let waiting = | |
| switch (gsManager->GlobalStateManager.getState).rollbackState { | |
| | RollbackInMemStore(_) => false | |
| | RollingBack(_) | |
| | NoRollback => true | |
| } | |
| if waiting { | |
| Utils.delay(50)->Promise.thenResolve(_ => poll())->Promise.done | |
| } else { | |
| resolve() | |
| } | |
| } | |
| poll() | |
| }) | |
| }, |
🤖 Prompt for AI Agents
In scenarios/test_codegen/test/helpers/Mock.res around lines 304 to 316,
getRollbackReadyPromise uses an async function as the Promise.makeAsync
executor; change the executor to a non-async function that invokes an async IIFE
so the executor itself is synchronous. Concretely, replace the async (resolve,
_reject) => { ... await ...; resolve() } with (resolve, reject) => { (async ()
=> { ... await ...; resolve() })().catch(reject) } so awaiting and error
handling happen inside the IIFE and the Promise executor is not async.
| resolve({ | ||
| Source.currentBlockHeight, | ||
| reorgGuard: { |
There was a problem hiding this comment.
Bug: using module name instead of param (currentBlockHeight).
You’re returning Source.currentBlockHeight, which is undefined here. Use the function parameter.
- Source.currentBlockHeight,
+ currentBlockHeight,📝 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.
| resolve({ | |
| Source.currentBlockHeight, | |
| reorgGuard: { | |
| resolve({ | |
| currentBlockHeight, | |
| reorgGuard: { |
🤖 Prompt for AI Agents
In scenarios/test_codegen/test/helpers/Mock.res around lines 474 to 476, the
resolve call returns Source.currentBlockHeight (a module name) instead of the
function parameter currentBlockHeight; replace Source.currentBlockHeight with
the local parameter currentBlockHeight so the resolved object uses the actual
value passed into the function.
| Assert.equal( | ||
| createQuery, | ||
| `CREATE TABLE IF NOT EXISTS "public"."EntityWithAllNonArrayTypes"("bigDecimal" NUMERIC NOT NULL, "bigDecimalWithConfig" NUMERIC(10, 8) NOT NULL, "bigInt" NUMERIC NOT NULL, "bool" BOOLEAN NOT NULL, "enumField" "public".AccountType NOT NULL, "float_" DOUBLE PRECISION NOT NULL, "id" TEXT NOT NULL, "int_" INTEGER NOT NULL, "optBigDecimal" NUMERIC, "optBigInt" NUMERIC, "optBool" BOOLEAN, "optEnumField" "public".AccountType, "optFloat" DOUBLE PRECISION, "optInt" INTEGER, "optString" TEXT, "string" TEXT NOT NULL, "db_write_timestamp" TIMESTAMP DEFAULT CURRENT_TIMESTAMP, PRIMARY KEY("id"));`, | ||
| `CREATE TABLE IF NOT EXISTS "public"."EntityWithAllNonArrayTypes"("bigDecimal" NUMERIC NOT NULL, "bigDecimalWithConfig" NUMERIC(10, 8) NOT NULL, "bigInt" NUMERIC NOT NULL, "bool" BOOLEAN NOT NULL, "enumField" "public".AccountType NOT NULL, "float_" DOUBLE PRECISION NOT NULL, "id" TEXT NOT NULL, "int_" INTEGER NOT NULL, "optBigDecimal" NUMERIC, "optBigInt" NUMERIC, "optBool" BOOLEAN, "optEnumField" "public".AccountType, "optFloat" DOUBLE PRECISION, "optInt" INTEGER, "optString" TEXT, "string" TEXT NOT NULL, PRIMARY KEY("id"));`, |
There was a problem hiding this comment.
🧩 Analysis chain
Verify removal of db_write_timestamp aligns with the PR's rollback bug fix.
The test correctly removes the db_write_timestamp column from the expected CREATE TABLE statement for EntityWithAllNonArrayTypes. This change aligns with the broader removal of db_write_timestamp columns across the codebase as part of fixing the "duplicate history on rollback bug" mentioned in the PR title.
However, please verify that removing db_write_timestamp doesn't break entity history tracking or rollback functionality:
🏁 Script executed:
#!/bin/bash
# Verify that entity history functionality still works without db_write_timestamp
rg -n "db_write_timestamp" --type=res -C3 -g '!**/test/**'Length of output: 27
🏁 Script executed:
#!/bin/bash
# Re-run search for db_write_timestamp references outside of tests (no file-type filter)
rg -n "db_write_timestamp" -C3 -g '!**/test/**'Length of output: 148
🏁 Script executed:
#!/bin/bash
set -uo pipefail
echo "PWD: $(pwd)"
git rev-parse --abbrev-ref HEAD 2>/dev/null || true
echo "---- rg version ----"
rg --version 2>/dev/null || echo "rg not found"
echo "---- Search (rg) across repo including hidden/unignored files ----"
rg -n --hidden -uu "db_write_timestamp" || true
echo "---- Search results excluding common test paths ----"
rg -n --hidden -uu "db_write_timestamp" 2>/dev/null | grep -v -E '/test/|/tests/|/spec/' || echo "no matches outside test paths"
echo "---- Fallback: git grep across tracked files ----"
git grep -n "db_write_timestamp" || true
git grep -n "db_write_timestamp" | grep -v -E '/test/|/tests/|/spec/' || echo "git grep: no matches outside test paths"
echo "---- Repo root listing (first 200 entries) ----"
find . -maxdepth 3 -type f | sed -n '1,200p'Length of output: 3552
Remove remaining db_write_timestamp references & verify rollback/history
- Found occurrences that still reference db_write_timestamp: codegenerator/cli/src/config_parsing/entity_parsing.rs:355 and codegenerator/integration_tests/tests/evm_Erc20.js:13 — remove or update these to match the CREATE TABLE change.
- After updating, run integration/e2e tests and explicitly verify entity history and rollback behavior end-to-end.
🤖 Prompt for AI Agents
In scenarios/test_codegen/test/SerDe_Test.res around line 129, and in the two
files called out by the review
(codegenerator/cli/src/config_parsing/entity_parsing.rs line ~355 and
codegenerator/integration_tests/tests/evm_Erc20.js line ~13), there are leftover
references to db_write_timestamp that no longer exist in the CREATE TABLE
(EntityWithAllNonArrayTypes) schema; remove or update those references to use
the current schema fields (or the new timestamp column name if renamed), adjust
any parsing/serialization logic to stop expecting db_write_timestamp, and update
tests to assert the new behavior; after making the changes run the full
integration/e2e test suite and explicitly verify entity history and rollback
behavior end-to-end to ensure no lingering dependencies on db_write_timestamp
remain.
…k-duplicate-history-bug
Summary by CodeRabbit