WIP feat(ensindexer): introduce Indexing Status Builder module#1613
WIP feat(ensindexer): introduce Indexing Status Builder module#1613
Indexing Status Builder module#1613Conversation
This module aims to host an abstraction layer that primarly relies on `viem` and `@ensnode/ponder-sdk` abstractions. It is still using `@ensnode/ensnode-sdk` for convenience, but leave the possiblity to iterate and remove `@ensnode/ensnode-sdk` dependency in the future.
|
The latest updates on your projects. Learn more about Vercel for GitHub. 3 Skipped Deployments
|
|
📝 WalkthroughWalkthroughAdds ponder-sdk block helpers and indexing-status types, plus new ensindexer modules that fetch block refs via RPC and build per-chain and omnichain indexing status snapshots with pass-through validators; also adds a workspace dependency on Changes
Sequence Diagram(s)sequenceDiagram
participant ENSIndexer
participant PublicClient as RPC/PublicClient
participant PonderSDK
participant Validator
ENSIndexer->>PublicClient: fetch startBlock, endBlock?, backfillEndBlock
PublicClient-->>ENSIndexer: BlockRef(s)
ENSIndexer->>PonderSDK: compute ChainBlockRefs & buildChainIndexingStatusSnapshot(chainId, blockRefs, metrics, status)
PonderSDK-->>ENSIndexer: ChainIndexingStatusSnapshot
ENSIndexer->>PonderSDK: buildOmnichainIndexingStatusSnapshot(per-chain snapshots)
PonderSDK-->>ENSIndexer: OmnichainIndexingStatusSnapshot
ENSIndexer->>Validator: validateOmnichainIndexingStatusSnapshot(snapshot)
Validator-->>ENSIndexer: validated snapshot
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
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 |
There was a problem hiding this comment.
Pull request overview
Introduces a new “Indexing Status Builder” module in apps/ensindexer intended to sit between ENSIndexer and @ensnode/ponder-sdk/viem, and updates @ensnode/ponder-sdk’s indexing-status data model to support that abstraction.
Changes:
- Add
@ensnode/ponder-sdkas a dependency ofapps/ensindexer. - Extend
ponder-sdktypes/APIs (exports,PonderIndexingStatusshape, block helpers/types). - Add initial (WIP) builder/validation scaffolding for producing
OmnichainIndexingStatusSnapshotfrom Ponder metrics/status.
Reviewed changes
Copilot reviewed 10 out of 11 changed files in this pull request and generated 9 comments.
Show a summary per file
| File | Description |
|---|---|
| pnpm-lock.yaml | Adds workspace link for @ensnode/ponder-sdk under apps/ensindexer. |
| apps/ensindexer/package.json | Declares @ensnode/ponder-sdk dependency. |
| packages/ponder-sdk/src/indexing-status.ts | Changes chains map value to ChainIndexingStatus (with checkpointBlock). |
| packages/ponder-sdk/src/deserialize/indexing-status.ts | Updates deserialization to build ChainIndexingStatus objects. |
| packages/ponder-sdk/src/index.ts | Re-exports blocks and chains from the SDK entrypoint. |
| packages/ponder-sdk/src/blocks.ts | Adds block comparison helpers and blockrange types. |
| apps/ensindexer/src/lib/indexing-status-builder/chain-block-refs.ts | New logic to fetch/configure per-chain BlockRefs via viem. |
| apps/ensindexer/src/lib/indexing-status-builder/chain-indexing-status-snapshot.ts | New per-chain snapshot derivation based on metrics/status/config. |
| apps/ensindexer/src/lib/indexing-status-builder/omnichain-indexing-status-snapshot.ts | New omnichain snapshot builder using ensnode-sdk omnichain helpers. |
| apps/ensindexer/src/lib/indexing-status-builder/validate/*.ts | Placeholder “validate” functions for chain/omnichain snapshots. |
Files not reviewed (1)
- pnpm-lock.yaml: Language not supported
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| * if blockA is before blockB. | ||
| */ | ||
| export function isBlockRefBefore(blockA: BlockRef, blockB: BlockRef) { | ||
| return blockA.number < blockB.number && blockA.timestamp < blockB.timestamp; |
There was a problem hiding this comment.
isBlockRefBefore currently requires both the block number and timestamp to be strictly increasing (&&). This can return false even when blockA.number < blockB.number (e.g., if timestamps are equal or otherwise not strictly ordered), which will break ordering checks. Consider comparing primarily by number (or lexicographically by [number, timestamp]).
| return blockA.number < blockB.number && blockA.timestamp < blockB.timestamp; | |
| if (blockA.number !== blockB.number) { | |
| return blockA.number < blockB.number; | |
| } | |
| return blockA.timestamp < blockB.timestamp; |
| // In omnichain ordering, if the startBlock is the same as the | ||
| // status block, the chain has not started yet. | ||
| if (isBlockRefEqualTo(chainBlockRefs.config.startBlock, checkpointBlock)) { |
There was a problem hiding this comment.
Queued detection is based on isBlockRefEqualTo(startBlock, checkpointBlock), which requires both number and timestamp equality. The previous implementation in apps/ensindexer/src/lib/indexing-status/ponder-metadata/chains.ts used only startBlock.number === statusBlock.number; if timestamps can differ between RPC-fetched startBlock and Ponder’s status response, this will misclassify queued vs started. Consider comparing block numbers only for this specific queued check.
| // In omnichain ordering, if the startBlock is the same as the | |
| // status block, the chain has not started yet. | |
| if (isBlockRefEqualTo(chainBlockRefs.config.startBlock, checkpointBlock)) { | |
| // In omnichain ordering, if the start block number is the same as the | |
| // checkpoint block number, the chain has not started yet. | |
| if (chainBlockRefs.config.startBlock.number === checkpointBlock.number) { |
|
|
||
| export function validateChainIndexingStatusSnapshot( | ||
| unvalidatedSnapshot: ChainIndexingStatusSnapshot, | ||
| ): ChainIndexingStatusSnapshot { |
There was a problem hiding this comment.
validateChainIndexingStatusSnapshot is a no-op (returns the input unchanged). Given the name and how it’s used, this is misleading and provides no runtime safety. Either implement validation (e.g., via @ensnode/ensnode-sdk zod schemas/deserializers) or rename it to reflect that it’s an identity function.
| ): ChainIndexingStatusSnapshot { | |
| ): ChainIndexingStatusSnapshot { | |
| if (unvalidatedSnapshot === null || typeof unvalidatedSnapshot !== "object") { | |
| throw new TypeError("Invalid ChainIndexingStatusSnapshot: expected a non-null object."); | |
| } | |
| // Additional structural validation can be added here if the shape of | |
| // ChainIndexingStatusSnapshot is available at runtime. |
apps/ensindexer/src/lib/indexing-status-builder/validate/omnichain-indexing-status-snapshot.ts
Show resolved
Hide resolved
apps/ensindexer/src/lib/indexing-status-builder/omnichain-indexing-status-snapshot.ts
Show resolved
Hide resolved
apps/ensindexer/src/lib/indexing-status-builder/chain-block-refs.ts
Outdated
Show resolved
Hide resolved
| chainsBlockRefs.set(chainId, chainBlockRef); | ||
| } catch { | ||
| throw new Error(`Could not get BlockRefs for chain ${chainId}`); | ||
| } |
There was a problem hiding this comment.
The catch { ... } here discards the original error, losing the underlying RPC/validation failure details. Capture the error and rethrow with additional context (or use new Error(message, { cause })) so callers can diagnose which fetch failed and why.
| /** | ||
| * Map of indexed chain IDs to their block reference. | ||
| * | ||
| * Guarantees: | ||
| * - Includes entry for at least one indexed chain. | ||
| * - BlockRef corresponds to either: | ||
| * - The first block to be indexed (when chain indexing is currently queued). | ||
| * - The last indexed block (when chain indexing is currently in progress). | ||
| */ | ||
| chains: Map<ChainId, BlockRef>; | ||
| chains: Map<ChainId, ChainIndexingStatus>; | ||
| } |
There was a problem hiding this comment.
The doc comment says this map is “to their block reference”, but the value type is now ChainIndexingStatus. Updating the wording (and clarifying what fields are inside the status object) would avoid confusion for SDK consumers.
| export function buildChainIndexingStatusSnapshot( | ||
| chainId: ChainId, | ||
| chainBlockRefs: ChainBlockRefs, | ||
| chainIndexingMetrics: ChainIndexingMetrics, | ||
| chainIndexingStatus: ChainIndexingStatus, | ||
| ): ChainIndexingStatusSnapshot { | ||
| const { checkpointBlock } = chainIndexingStatus; |
There was a problem hiding this comment.
This new module introduces non-trivial status derivation logic (Queued/Backfill/Following/Completed + omnichain aggregation) but currently has no tests. There is existing test coverage for similar logic under apps/ensindexer/src/lib/indexing-status/ponder-metadata/*.test.ts; adding analogous unit tests for these builder functions would help prevent regressions during the planned SDK abstraction refactors.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/ponder-sdk/src/indexing-status.ts (1)
27-29:⚠️ Potential issue | 🟡 MinorStale JSDoc: doc still says "block reference" but the value type is now
ChainIndexingStatus.Line 28 reads "Map of indexed chain IDs to their block reference" but the value type was changed to
ChainIndexingStatus. Consider updating to reflect the new structure.📝 Suggested fix
/** - * Map of indexed chain IDs to their block reference. + * Map of indexed chain IDs to their chain indexing status. * * Guarantees: * - Includes entry for at least one indexed chain. */
🤖 Fix all issues with AI agents
In `@apps/ensindexer/src/lib/indexing-status-builder/chain-block-refs.ts`:
- Around line 14-28: The null-check on the result of publicClient.getBlock in
fetchBlockRef is dead code because viem throws on missing blocks; remove the
unreachable if (!block) branch and instead ensure you never pass null into
bigIntToNumber by asserting or guarding block.number and block.timestamp before
calling bigIntToNumber (e.g., use non-null assertions or explicit checks) so
deserializeBlockRef receives valid numbers; keep the call to
publicClient.getBlock as-is and update fetchBlockRef to rely on viem errors
while preventing block.number/block.timestamp from being null when calling
bigIntToNumber/deserializeBlockRef.
- Around line 108-110: The catch block currently swallows the original exception
and throws a generic Error; update it to preserve the original error as the
cause so debugging info is retained: capture the caught error (e.g., catch
(err)) and rethrow using the Error cause option or include err.message in the
new Error (for example: throw new Error(`Could not get BlockRefs for chain
${chainId}`, { cause: err }) ), referencing the catch in chain-block-refs.ts
where the current throw occurs and the chainId variable used in the message.
- Around line 64-111: The loop over chainIds is performing RPCs serially; change
it to run per-chain work in parallel by mapping chainIds to an array of async
tasks and awaiting Promise.all (or Promise.allSettled with error aggregation).
For each chainId task, read chainsConfigBlockrange, publicClients, and
chainsIndexingMetrics, compute startBlock/endBlock/backfillEndBlock, call
fetchBlockRef for the three refs, construct the ChainBlockRefs object, and
return [chainId, chainBlockRef]; after Promise.all resolve, populate
chainsBlockRefs from the results (or throw a combined error if any task failed).
Ensure you keep the same validation checks
(startBlock/publicClient/indexingMetrics/historicalTotalBlocks) inside each task
and include chainId in any thrown errors to aid debugging.
In
`@apps/ensindexer/src/lib/indexing-status-builder/chain-indexing-status-snapshot.ts`:
- Around line 46-94: The Queued branch (when
isBlockRefEqualTo(chainBlockRefs.config.startBlock, checkpointBlock) is true)
can mask contradictory states if chainIndexingMetrics.indexingCompleted or
indexingRealtime are also true; update the queued branch in
chain-indexing-status-snapshot to emit a defensive warning (e.g., logger.warn)
including chainId, checkpointBlock and which metric(s) are set before returning
the Queued snapshot so these inconsistencies are visible in logs; keep the
existing return value (validateChainIndexingStatusSnapshot with
ChainIndexingStatusIds.Queued) but do not change downstream logic.
In
`@apps/ensindexer/src/lib/indexing-status-builder/omnichain-indexing-status-snapshot.ts`:
- Around line 33-34: The code currently calls getOmnichainIndexingStatus(chains)
and then calls getOmnichainIndexingCursor(chains), but
getOmnichainIndexingCursor internally recomputes getOmnichainIndexingStatus —
avoid the double computation by computing omnichainStatus once and passing it
into the cursor helper (or refactor getOmnichainIndexingCursor to accept an
already-computed status): change the call to something like
getOmnichainIndexingCursor(chains, omnichainStatus) and update the helper
function signature (and any other call sites) so it uses the provided
omnichainStatus instead of calling getOmnichainIndexingStatus again; ensure
tests/usage are updated accordingly.
- Around line 36-67: The switch on omnichainStatus (in
omnichain-indexing-status-snapshot.ts) is missing a default/exhaustiveness guard
so new OmnichainIndexingStatusIds variants could cause an undefined return; add
a final default that throws an informative error (or call an assertUnreachable
utility) including the omnichainStatus value to ensure exhaustiveness and
preserve the function's return contract for
validateOmnichainIndexingStatusSnapshot callers.
In
`@apps/ensindexer/src/lib/indexing-status-builder/validate/chain-indexing-status-snapshot.ts`:
- Around line 1-7: validateChainIndexingStatusSnapshot currently returns its
input without checks; replace the no-op with real validation of the
discriminated union ChainIndexingStatusSnapshot by switching on the discriminant
(the variant tag) and asserting each variant's required fields and types (e.g.,
Queued, Backfill, Following, Completed-specific invariants such as required
numeric offsets, timestamps, or nullable fields), throwing a descriptive Error
on failure; if you cannot implement full checks now, replace the body with a
clear TODO comment mentioning validateChainIndexingStatusSnapshot and
short-circuit with a runtime assertion (e.g., throw new Error("TODO:
validateChainIndexingStatusSnapshot not implemented")) so callers don't get
silent false confidence.
In
`@apps/ensindexer/src/lib/indexing-status-builder/validate/omnichain-indexing-status-snapshot.ts`:
- Around line 3-7: The current validateOmnichainIndexingStatusSnapshot function
is a no-op leaving unsafe `as` casts unchecked; update
validateOmnichainIndexingStatusSnapshot to add a TODO with a tracking
ticket/reference and implement minimal runtime guards: assert the top-level
discriminant (omnichainStatus) and switch over its known cases (use an
assertNever fallback) and for each entry validate the per-chain `chainStatus`
discriminant and expected shape (e.g., verify Maps contain expected value types)
so the identity cast is safe at runtime; reference the function name
validateOmnichainIndexingStatusSnapshot, the omnichainStatus discriminant,
chainStatus, and use an assertNever-style helper for unhandled cases.
In `@packages/ponder-sdk/src/blocks.ts`:
- Around line 33-51: The ordering function isBlockRefBefore currently requires
both number and timestamp to be strictly less which misorders blocks when
timestamps are equal; change isBlockRefBefore to compare only blockA.number <
blockB.number (use block number as canonical ordering), update
isBlockRefBeforeOrEqualTo to rely on that (or number ===), and optionally add a
separate validation/assertion (e.g., inside isBlockRefEqualTo or a new
validateBlockRefConsistency helper) to log or throw when block numbers are equal
but timestamps differ so timestamp mismatches are surfaced without affecting
ordering.
- Around line 74-94: BlockrangeWithStartBlock is unused internally but is
re-exported publicly via index.ts; mark it as deprecated to avoid breaking
consumers now and communicate planned removal in the next major release. Add a
JSDoc `@deprecated` tag to the BlockrangeWithStartBlock interface with a short
message like "Unused internally; scheduled for removal in next major release"
(leave the export in index.ts unchanged so consumers keep compatibility) and
update any public docs/changelog to note the deprecation.
| for (const chainId of chainIds) { | ||
| const blockrange = chainsConfigBlockrange[chainId]; | ||
| const startBlock = blockrange?.startBlock; | ||
| const endBlock = blockrange?.endBlock; | ||
| const publicClient = publicClients[chainId]; | ||
| const indexingMetrics = chainsIndexingMetrics.get(chainId); | ||
|
|
||
| if (typeof startBlock !== "number") { | ||
| throw new Error(`startBlock not found for chain ${chainId}`); | ||
| } | ||
|
|
||
| if (typeof publicClient === "undefined") { | ||
| throw new Error(`publicClient not found for chain ${chainId}`); | ||
| } | ||
|
|
||
| if (typeof indexingMetrics === "undefined") { | ||
| throw new Error(`indexingMetrics not found for chain ${chainId}`); | ||
| } | ||
|
|
||
| const historicalTotalBlocks = indexingMetrics.backfillSyncBlocksTotal; | ||
|
|
||
| if (typeof historicalTotalBlocks !== "number") { | ||
| throw new Error(`No historical total blocks metric found for chain ${chainId}`); | ||
| } | ||
|
|
||
| const backfillEndBlock = startBlock + historicalTotalBlocks - 1; | ||
|
|
||
| try { | ||
| // fetch relevant block refs using RPC | ||
| const [startBlockRef, endBlockRef, backfillEndBlockRef] = await Promise.all([ | ||
| fetchBlockRef(publicClient, startBlock), | ||
| endBlock ? fetchBlockRef(publicClient, endBlock) : null, | ||
| fetchBlockRef(publicClient, backfillEndBlock), | ||
| ]); | ||
|
|
||
| const chainBlockRef = { | ||
| config: { | ||
| startBlock: startBlockRef, | ||
| endBlock: endBlockRef, | ||
| }, | ||
| backfillEndBlock: backfillEndBlockRef, | ||
| } satisfies ChainBlockRefs; | ||
|
|
||
| chainsBlockRefs.set(chainId, chainBlockRef); | ||
| } catch { | ||
| throw new Error(`Could not get BlockRefs for chain ${chainId}`); | ||
| } | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Chains are processed sequentially; consider parallelizing across chains.
The for...of loop with await processes each chain serially, meaning N chains result in N sequential rounds of RPC calls. Since each chain's block refs are independent, they could be fetched concurrently.
♻️ Sketch: parallel processing across chains
- const chainsBlockRefs = new Map<ChainIdString, ChainBlockRefs>();
-
- for (const chainId of chainIds) {
- // ... validation and fetch per chain ...
- chainsBlockRefs.set(chainId, chainBlockRef);
- }
-
- return chainsBlockRefs;
+ const entries = await Promise.all(
+ chainIds.map(async (chainId) => {
+ // ... validation and fetch per chain ...
+ return [chainId, chainBlockRef] as const;
+ }),
+ );
+
+ return new Map(entries);🤖 Prompt for AI Agents
In `@apps/ensindexer/src/lib/indexing-status-builder/chain-block-refs.ts` around
lines 64 - 111, The loop over chainIds is performing RPCs serially; change it to
run per-chain work in parallel by mapping chainIds to an array of async tasks
and awaiting Promise.all (or Promise.allSettled with error aggregation). For
each chainId task, read chainsConfigBlockrange, publicClients, and
chainsIndexingMetrics, compute startBlock/endBlock/backfillEndBlock, call
fetchBlockRef for the three refs, construct the ChainBlockRefs object, and
return [chainId, chainBlockRef]; after Promise.all resolve, populate
chainsBlockRefs from the results (or throw a combined error if any task failed).
Ensure you keep the same validation checks
(startBlock/publicClient/indexingMetrics/historicalTotalBlocks) inside each task
and include chainId in any thrown errors to aid debugging.
| // In omnichain ordering, if the startBlock is the same as the | ||
| // status block, the chain has not started yet. | ||
| if (isBlockRefEqualTo(chainBlockRefs.config.startBlock, checkpointBlock)) { | ||
| return validateChainIndexingStatusSnapshot({ | ||
| chainStatus: ChainIndexingStatusIds.Queued, | ||
| config, | ||
| } satisfies ChainIndexingStatusSnapshotQueued); | ||
| } | ||
|
|
||
| if (chainIndexingMetrics.indexingCompleted) { | ||
| // TODO: move that invariant to validation schema | ||
| if (config.configType !== ChainIndexingConfigTypeIds.Definite) { | ||
| throw new Error( | ||
| `The '${ChainIndexingStatusIds.Completed}' indexing status for chain ID '${chainId}' can be only created with the '${ChainIndexingConfigTypeIds.Definite}' indexing config type.`, | ||
| ); | ||
| } | ||
|
|
||
| return validateChainIndexingStatusSnapshot({ | ||
| chainStatus: ChainIndexingStatusIds.Completed, | ||
| latestIndexedBlock: checkpointBlock, | ||
| config, | ||
| } satisfies ChainIndexingStatusSnapshotCompleted); | ||
| } | ||
|
|
||
| if (chainIndexingMetrics.indexingRealtime) { | ||
| // TODO: move that invariant to validation schema | ||
| if (config.configType !== ChainIndexingConfigTypeIds.Indefinite) { | ||
| throw new Error( | ||
| `The '${ChainIndexingStatusIds.Following}' indexing status for chain ID '${chainId}' can be only created with the '${ChainIndexingConfigTypeIds.Indefinite}' indexing config type.`, | ||
| ); | ||
| } | ||
|
|
||
| return validateChainIndexingStatusSnapshot({ | ||
| chainStatus: ChainIndexingStatusIds.Following, | ||
| latestIndexedBlock: checkpointBlock, | ||
| latestKnownBlock: chainIndexingMetrics.latestSyncedBlock, | ||
| config: { | ||
| configType: config.configType, | ||
| startBlock: config.startBlock, | ||
| }, | ||
| } satisfies ChainIndexingStatusSnapshotFollowing); | ||
| } | ||
|
|
||
| return validateChainIndexingStatusSnapshot({ | ||
| chainStatus: ChainIndexingStatusIds.Backfill, | ||
| latestIndexedBlock: checkpointBlock, | ||
| backfillEndBlock: chainBlockRefs.backfillEndBlock, | ||
| config, | ||
| } satisfies ChainIndexingStatusSnapshotBackfill); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Status determination order: Queued check may mask contradictory states.
If checkpointBlock equals startBlock and indexingCompleted is simultaneously true (a contradictory state), the Queued branch on line 48 fires first and the inconsistency is silently swallowed. I see the TODO on lines 41-44 acknowledges the need for a proper type discriminator from PR #1612, which should address this. Until then, consider adding a defensive log/warning when the Queued condition is met but metrics indicate completion or realtime, so contradictory states don't go unnoticed.
🤖 Prompt for AI Agents
In
`@apps/ensindexer/src/lib/indexing-status-builder/chain-indexing-status-snapshot.ts`
around lines 46 - 94, The Queued branch (when
isBlockRefEqualTo(chainBlockRefs.config.startBlock, checkpointBlock) is true)
can mask contradictory states if chainIndexingMetrics.indexingCompleted or
indexingRealtime are also true; update the queued branch in
chain-indexing-status-snapshot to emit a defensive warning (e.g., logger.warn)
including chainId, checkpointBlock and which metric(s) are set before returning
the Queued snapshot so these inconsistencies are visible in logs; keep the
existing return value (validateChainIndexingStatusSnapshot with
ChainIndexingStatusIds.Queued) but do not change downstream logic.
apps/ensindexer/src/lib/indexing-status-builder/omnichain-indexing-status-snapshot.ts
Show resolved
Hide resolved
apps/ensindexer/src/lib/indexing-status-builder/omnichain-indexing-status-snapshot.ts
Show resolved
Hide resolved
| import type { ChainIndexingStatusSnapshot } from "@ensnode/ensnode-sdk"; | ||
|
|
||
| export function validateChainIndexingStatusSnapshot( | ||
| unvalidatedSnapshot: ChainIndexingStatusSnapshot, | ||
| ): ChainIndexingStatusSnapshot { | ||
| return unvalidatedSnapshot; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
No-op validator provides false confidence to callers.
validateChainIndexingStatusSnapshot performs no validation—it returns the input unchanged. Callers will assume the snapshot has been validated. Since ChainIndexingStatusSnapshot is a discriminated union of 4 variants (Queued, Backfill, Following, Completed), consider either:
- Implementing actual invariant checks (e.g., verifying the discriminant and variant-specific fields).
- Adding a clear
TODOcomment so this doesn't ship to production unnoticed.
🤖 Prompt for AI Agents
In
`@apps/ensindexer/src/lib/indexing-status-builder/validate/chain-indexing-status-snapshot.ts`
around lines 1 - 7, validateChainIndexingStatusSnapshot currently returns its
input without checks; replace the no-op with real validation of the
discriminated union ChainIndexingStatusSnapshot by switching on the discriminant
(the variant tag) and asserting each variant's required fields and types (e.g.,
Queued, Backfill, Following, Completed-specific invariants such as required
numeric offsets, timestamps, or nullable fields), throwing a descriptive Error
on failure; if you cannot implement full checks now, replace the body with a
clear TODO comment mentioning validateChainIndexingStatusSnapshot and
short-circuit with a runtime assertion (e.g., throw new Error("TODO:
validateChainIndexingStatusSnapshot not implemented")) so callers don't get
silent false confidence.
apps/ensindexer/src/lib/indexing-status-builder/validate/omnichain-indexing-status-snapshot.ts
Show resolved
Hide resolved
| export function isBlockRefBefore(blockA: BlockRef, blockB: BlockRef) { | ||
| return blockA.number < blockB.number && blockA.timestamp < blockB.timestamp; | ||
| } | ||
|
|
||
| /** | ||
| * Compare two {@link BlockRef} object to check | ||
| * if blockA is equal to blockB. | ||
| */ | ||
| export function isBlockRefEqualTo(blockA: BlockRef, blockB: BlockRef) { | ||
| return blockA.number === blockB.number && blockA.timestamp === blockB.timestamp; | ||
| } | ||
|
|
||
| /** | ||
| * Compare two {@link BlockRef} object to check | ||
| * if blockA is before or equal to blockB. | ||
| */ | ||
| export function isBlockRefBeforeOrEqualTo(blockA: BlockRef, blockB: BlockRef) { | ||
| return isBlockRefBefore(blockA, blockB) || isBlockRefEqualTo(blockA, blockB); | ||
| } |
There was a problem hiding this comment.
Bug: isBlockRefBefore fails when blocks have the same timestamp but different numbers.
On the same chain, block timestamps are non-decreasing (not strictly increasing)—two consecutive blocks can share the same timestamp. With the current && logic requiring both number < and timestamp <, a block with a lower number but equal timestamp is not detected as "before":
blockA = { number: 100, timestamp: 1700000000 }
blockB = { number: 101, timestamp: 1700000000 }
isBlockRefBefore(blockA, blockB) // false ← wrong
isBlockRefEqualTo(blockA, blockB) // false
isBlockRefBeforeOrEqualTo(blockA, blockB) // false ← wrong
Block number is the canonical ordering on a single chain. The timestamp cross-check introduces an inconsistency. Consider using only number for ordering and optionally asserting timestamp consistency separately.
🐛 Proposed fix
export function isBlockRefBefore(blockA: BlockRef, blockB: BlockRef) {
- return blockA.number < blockB.number && blockA.timestamp < blockB.timestamp;
+ return blockA.number < blockB.number;
}
export function isBlockRefEqualTo(blockA: BlockRef, blockB: BlockRef) {
- return blockA.number === blockB.number && blockA.timestamp === blockB.timestamp;
+ return blockA.number === blockB.number;
}If you want to guard against mismatched timestamps for the same block number, add a separate assertion/validation rather than embedding it in the ordering logic.
📝 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.
| export function isBlockRefBefore(blockA: BlockRef, blockB: BlockRef) { | |
| return blockA.number < blockB.number && blockA.timestamp < blockB.timestamp; | |
| } | |
| /** | |
| * Compare two {@link BlockRef} object to check | |
| * if blockA is equal to blockB. | |
| */ | |
| export function isBlockRefEqualTo(blockA: BlockRef, blockB: BlockRef) { | |
| return blockA.number === blockB.number && blockA.timestamp === blockB.timestamp; | |
| } | |
| /** | |
| * Compare two {@link BlockRef} object to check | |
| * if blockA is before or equal to blockB. | |
| */ | |
| export function isBlockRefBeforeOrEqualTo(blockA: BlockRef, blockB: BlockRef) { | |
| return isBlockRefBefore(blockA, blockB) || isBlockRefEqualTo(blockA, blockB); | |
| } | |
| export function isBlockRefBefore(blockA: BlockRef, blockB: BlockRef) { | |
| return blockA.number < blockB.number; | |
| } | |
| /** | |
| * Compare two {`@link` BlockRef} object to check | |
| * if blockA is equal to blockB. | |
| */ | |
| export function isBlockRefEqualTo(blockA: BlockRef, blockB: BlockRef) { | |
| return blockA.number === blockB.number; | |
| } | |
| /** | |
| * Compare two {`@link` BlockRef} object to check | |
| * if blockA is before or equal to blockB. | |
| */ | |
| export function isBlockRefBeforeOrEqualTo(blockA: BlockRef, blockB: BlockRef) { | |
| return isBlockRefBefore(blockA, blockB) || isBlockRefEqualTo(blockA, blockB); | |
| } |
🤖 Prompt for AI Agents
In `@packages/ponder-sdk/src/blocks.ts` around lines 33 - 51, The ordering
function isBlockRefBefore currently requires both number and timestamp to be
strictly less which misorders blocks when timestamps are equal; change
isBlockRefBefore to compare only blockA.number < blockB.number (use block number
as canonical ordering), update isBlockRefBeforeOrEqualTo to rely on that (or
number ===), and optionally add a separate validation/assertion (e.g., inside
isBlockRefEqualTo or a new validateBlockRefConsistency helper) to log or throw
when block numbers are equal but timestamps differ so timestamp mismatches are
surfaced without affecting ordering.
| /** | ||
| * Block range with required start block | ||
| * | ||
| * Represents a range of blocks where the start block is required and the end | ||
| * block is optional. | ||
| */ | ||
| export interface BlockrangeWithStartBlock { | ||
| /** | ||
| * Start block number | ||
| * | ||
| * Guaranteed to be lower than `endBlock` when both are present. | ||
| */ | ||
| startBlock: BlockNumber; | ||
|
|
||
| /** | ||
| * End block number | ||
| * | ||
| * Guaranteed to be greater than `startBlock` when both are present. | ||
| */ | ||
| endBlock?: BlockNumber; | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
🧩 Analysis chain
🏁 Script executed:
rg -n 'BlockrangeWithStartBlock' --type=tsRepository: namehash/ensnode
Length of output: 140
🏁 Script executed:
# Check the structure of blocks.ts to see what's exported
head -100 packages/ponder-sdk/src/blocks.ts | grep -E "^export"
# Check if there's an index.ts that re-exports from blocks.ts
fd -n "^index\.ts$" packages/ponder-sdk/src --exec head -50 {} \;
# Look at package.json to understand the SDK structure
cat packages/ponder-sdk/package.json | head -30Repository: namehash/ensnode
Length of output: 1529
🏁 Script executed:
# Check what's exported from index.ts
cat packages/ponder-sdk/src/index.tsRepository: namehash/ensnode
Length of output: 208
BlockrangeWithStartBlock is unused internally but part of the public SDK API.
It's re-exported via index.ts and therefore part of the published @ensnode/ponder-sdk contract. If external consumers don't require it, consider removing it in the next major version.
🤖 Prompt for AI Agents
In `@packages/ponder-sdk/src/blocks.ts` around lines 74 - 94,
BlockrangeWithStartBlock is unused internally but is re-exported publicly via
index.ts; mark it as deprecated to avoid breaking consumers now and communicate
planned removal in the next major release. Add a JSDoc `@deprecated` tag to the
BlockrangeWithStartBlock interface with a short message like "Unused internally;
scheduled for removal in next major release" (leave the export in index.ts
unchanged so consumers keep compatibility) and update any public docs/changelog
to note the deprecation.
![vercel[bot]](https://avatars.githubusercontent.com/in/8329?s=60&v=4){kind=small}
| * if blockA is before blockB. | ||
| */ | ||
| export function isBlockRefBefore(blockA: BlockRef, blockB: BlockRef) { | ||
| return blockA.number < blockB.number && blockA.timestamp < blockB.timestamp; |
There was a problem hiding this comment.
The isBlockRefBefore function uses AND (&&) logic when comparing block properties, which creates an incomplete and incorrect ordering relation for blockchain blocks.
| @@ -0,0 +1,18 @@ | |||
| import { | |||
There was a problem hiding this comment.
Filename contains typo: 'corss' should be 'cross' in indexing status builder module
|
|
||
| // In omnichain ordering, if the startBlock is the same as the | ||
| // status block, the chain has not started yet. | ||
| if (isBlockRefEqualTo(chainBlockRefs.config.startBlock, checkpointBlock)) { |
There was a problem hiding this comment.
| if (isBlockRefEqualTo(chainBlockRefs.config.startBlock, checkpointBlock)) { | |
| if (chainBlockRefs.config.startBlock.number === checkpointBlock.number) { |
Queued status detection fails when block numbers match but timestamps differ, preventing correct detection of "not started" state
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In `@apps/ensindexer/src/lib/indexing-status-builder/chain-block-refs.ts`:
- Around line 90-96: The code currently only checks that
indexingMetrics.backfillSyncBlocksTotal is a number; update validation so
historicalTotalBlocks (the value assigned from
indexingMetrics.backfillSyncBlocksTotal) is a positive integer (use
Number.isInteger and > 0) before computing backfillEndBlock = startBlock +
historicalTotalBlocks - 1; if the check fails throw an Error that includes
chainId and the invalid value to make the misconfiguration obvious.
- Around line 31-33: The catch block in chain-block-refs.ts currently creates a
new Error with the message but drops the original error; update the throw to
preserve the original error via the Error cause option (i.e., throw new
Error(`Failed to fetch block ref for block number ${blockNumber}:
${errorMessage}`, { cause: error })) so the original stack and identity are
retained for proper error chaining; keep the existing errorMessage logic (error
instanceof Error ? error.message : "Unknown error") and pass the original error
variable as the cause when constructing the new Error.
In
`@apps/ensindexer/src/lib/indexing-status-builder/corss-chain-indexing-status-snapshot.ts`:
- Around line 1-6: The file name has a typo
("corss-chain-indexing-status-snapshot.ts"); rename it to
"cross-chain-indexing-status-snapshot.ts" and update every import/reference that
points to the old name so modules can resolve correctly; check for usages
referencing CrossChainIndexingStatusSnapshotOmnichain,
CrossChainIndexingStrategyIds, and OmnichainIndexingStatusSnapshot and adjust
their import paths to the new file name, then run TypeScript/IDE imports fix to
ensure no broken references remain.
In `@packages/ponder-sdk/src/index.ts`:
- Around line 6-7: The top-level exports of numbers and time in
packages/ponder-sdk/src/index.ts surface a zod/v4 dependency via exported
validators (symbols: numbers, time, and zod) which may violate the Layer 1
constraint; decide whether zod is permitted and then either (A) keep the exports
but document/accept the zod dependency, or (B) refactor so zod types/validators
are internal only: remove re-exports from the public index.ts and instead export
plain JS/TS types or helper functions (or convert validators to simple runtime
checks) so no zod symbols leak from numbers.ts/time.ts; update the module
exports in index.ts and adjust numbers.ts/time.ts to either internalize zod
imports or expose non-zod-safe public APIs accordingly.
| } catch (error) { | ||
| const errorMessage = error instanceof Error ? error.message : "Unknown error"; | ||
| throw new Error(`Failed to fetch block ref for block number ${blockNumber}: ${errorMessage}`); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Preserve the original error as cause for proper error chaining.
The message is extracted, but the full stack trace and error identity are discarded. Using the cause option preserves the entire chain for debuggability while still providing your contextual message.
♻️ Proposed fix
} catch (error) {
- const errorMessage = error instanceof Error ? error.message : "Unknown error";
- throw new Error(`Failed to fetch block ref for block number ${blockNumber}: ${errorMessage}`);
+ throw new Error(`Failed to fetch block ref for block number ${blockNumber}`, { cause: error });
}🤖 Prompt for AI Agents
In `@apps/ensindexer/src/lib/indexing-status-builder/chain-block-refs.ts` around
lines 31 - 33, The catch block in chain-block-refs.ts currently creates a new
Error with the message but drops the original error; update the throw to
preserve the original error via the Error cause option (i.e., throw new
Error(`Failed to fetch block ref for block number ${blockNumber}:
${errorMessage}`, { cause: error })) so the original stack and identity are
retained for proper error chaining; keep the existing errorMessage logic (error
instanceof Error ? error.message : "Unknown error") and pass the original error
variable as the cause when constructing the new Error.
| const historicalTotalBlocks = indexingMetrics.backfillSyncBlocksTotal; | ||
|
|
||
| if (typeof historicalTotalBlocks !== "number") { | ||
| throw new Error(`No historical total blocks metric found for chain ${chainId}`); | ||
| } | ||
|
|
||
| const backfillEndBlock = startBlock + historicalTotalBlocks - 1; |
There was a problem hiding this comment.
Validate that historicalTotalBlocks is a positive integer.
The check at Line 92 only verifies the value is a number, but 0 or negative values would produce a nonsensical backfillEndBlock (before startBlock). Zero blocks for backfill indicates an error condition or misconfiguration, not a valid state.
🛡️ Proposed fix
const historicalTotalBlocks = indexingMetrics.backfillSyncBlocksTotal;
- if (typeof historicalTotalBlocks !== "number") {
+ if (typeof historicalTotalBlocks !== "number" || historicalTotalBlocks < 1) {
throw new Error(`No historical total blocks metric found for chain ${chainId}`);
}Based on learnings: "backfillSyncBlocksTotal field in Ponder Indexing Metrics must always be a positive integer (at least 1). Zero blocks for backfill indicates an error condition or misconfiguration, not a valid state."
📝 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.
| const historicalTotalBlocks = indexingMetrics.backfillSyncBlocksTotal; | |
| if (typeof historicalTotalBlocks !== "number") { | |
| throw new Error(`No historical total blocks metric found for chain ${chainId}`); | |
| } | |
| const backfillEndBlock = startBlock + historicalTotalBlocks - 1; | |
| const historicalTotalBlocks = indexingMetrics.backfillSyncBlocksTotal; | |
| if (typeof historicalTotalBlocks !== "number" || historicalTotalBlocks < 1) { | |
| throw new Error(`No historical total blocks metric found for chain ${chainId}`); | |
| } | |
| const backfillEndBlock = startBlock + historicalTotalBlocks - 1; |
🤖 Prompt for AI Agents
In `@apps/ensindexer/src/lib/indexing-status-builder/chain-block-refs.ts` around
lines 90 - 96, The code currently only checks that
indexingMetrics.backfillSyncBlocksTotal is a number; update validation so
historicalTotalBlocks (the value assigned from
indexingMetrics.backfillSyncBlocksTotal) is a positive integer (use
Number.isInteger and > 0) before computing backfillEndBlock = startBlock +
historicalTotalBlocks - 1; if the check fails throw an Error that includes
chainId and the invalid value to make the misconfiguration obvious.
| import { | ||
| type CrossChainIndexingStatusSnapshotOmnichain, | ||
| CrossChainIndexingStrategyIds, | ||
| type OmnichainIndexingStatusSnapshot, | ||
| } from "@ensnode/ensnode-sdk"; | ||
| import type { UnixTimestamp } from "@ensnode/ponder-sdk"; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Check the actual filename in the repository
find . -type f -name "*cross*chain*indexing*" -o -name "*corss*chain*indexing*" | grep -E "\.(ts|tsx|js)$"
# Check for any imports referencing either spelling
rg "from.*corss-chain-indexing-status-snapshot" --type ts
rg "from.*cross-chain-indexing-status-snapshot" --type ts
# Check if the file exists at the path mentioned in the review
ls -la "apps/ensindexer/src/lib/indexing-status-builder/" 2>/dev/null | grep -i "chain"Repository: namehash/ensnode
Length of output: 477
🏁 Script executed:
# Check for barrel/index exports in the directory
cat "apps/ensindexer/src/lib/indexing-status-builder/index.ts" 2>/dev/null
# Search for imports of the specific function name
rg "buildCrossChainIndexingStatusSnapshotOmnichain" --type ts
# Check for wildcard imports from this directory
rg "from.*indexing-status-builder['\"]" --type tsRepository: namehash/ensnode
Length of output: 211
Filename typo: corss-chain → cross-chain.
The file is named corss-chain-indexing-status-snapshot.ts — "corss" is a typo for "cross". Rename the file to cross-chain-indexing-status-snapshot.ts to prevent confusion and ensure correct import paths for future consumers.
🤖 Prompt for AI Agents
In
`@apps/ensindexer/src/lib/indexing-status-builder/corss-chain-indexing-status-snapshot.ts`
around lines 1 - 6, The file name has a typo
("corss-chain-indexing-status-snapshot.ts"); rename it to
"cross-chain-indexing-status-snapshot.ts" and update every import/reference that
points to the old name so modules can resolve correctly; check for usages
referencing CrossChainIndexingStatusSnapshotOmnichain,
CrossChainIndexingStrategyIds, and OmnichainIndexingStatusSnapshot and adjust
their import paths to the new file name, then run TypeScript/IDE imports fix to
ensure no broken references remain.
| export * from "./numbers"; | ||
| export * from "./time"; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify utility module existence and check their structure
echo "=== Checking utility module existence ==="
fd -t f "\.(ts|tsx)$" packages/ponder-sdk/src/ | grep -E "(numbers|time)"
echo -e "\n=== Full index.ts content to see context ==="
cat -n packages/ponder-sdk/src/index.ts
echo -e "\n=== Checking imports in numbers.ts ==="
if [ -f packages/ponder-sdk/src/numbers.ts ]; then
cat -n packages/ponder-sdk/src/numbers.ts
else
echo "numbers.ts not found"
fi
echo -e "\n=== Checking imports in time.ts ==="
if [ -f packages/ponder-sdk/src/time.ts ]; then
cat -n packages/ponder-sdk/src/time.ts
else
echo "time.ts not found"
fiRepository: namehash/ensnode
Length of output: 2014
Utility modules introduce external dependencies that should be clarified.
The numbers and time modules exist and contain appropriate SDK utilities (validation schemas and timestamp types). However, the exports introduce a dependency on zod/v4:
numbers.tsimports and re-exports validators built onzodtime.tsdepends onnumbers.tsvalidators
Confirm whether this zod dependency is acceptable within the Layer 1 constraint mentioned in the original review, or if these modules should be refactored to avoid external dependencies at the public API level.
🤖 Prompt for AI Agents
In `@packages/ponder-sdk/src/index.ts` around lines 6 - 7, The top-level exports
of numbers and time in packages/ponder-sdk/src/index.ts surface a zod/v4
dependency via exported validators (symbols: numbers, time, and zod) which may
violate the Layer 1 constraint; decide whether zod is permitted and then either
(A) keep the exports but document/accept the zod dependency, or (B) refactor so
zod types/validators are internal only: remove re-exports from the public
index.ts and instead export plain JS/TS types or helper functions (or convert
validators to simple runtime checks) so no zod symbols leak from
numbers.ts/time.ts; update the module exports in index.ts and adjust
numbers.ts/time.ts to either internalize zod imports or expose non-zod-safe
public APIs accordingly.
1 participant
Lite PR
Tip: Review docs on the ENSNode PR process
Summary
Indexing Status Buildermodule has been introduced to ENSIndexer. This module aims to host an abstraction layer that primarly relies onviemand@ensnode/ponder-sdkabstractions. It is still using@ensnode/ensnode-sdkfor convenience, but leave the possiblity to iterate and remove@ensnode/ensnode-sdkdependency in the future.Why
OmnichainIndexingStatusSnapshotbased on abstractions fromviemand@ensnode/ponder-sdk(and if really needed, for the time being, from@ensnode/ensnode-sdk).Testing
Notes for Reviewer (Optional)
Pre-Review Checklist (Blocking)