Draft
Conversation
|
Contributor
|
The latest updates on your projects. Learn more about Vercel for GitHub. 3 Skipped Deployments
|
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughConsolidates ENSNode metadata into a single IndexingMetadataContext record and moves ENS DB writer startup out of the API entrypoint into an idempotent onchain-event initialization flow; many reader/writer, serialization, worker, and tests updated to use the new context shape. Changes
Sequence Diagram(s)sequenceDiagram
participant Ponder
participant OnchainHandler as Onchain Event Handler
participant InitModule as initIndexingOnchainEvents
participant ENSRAIN as ENSRainbow
participant ENSDB as ENS DB
participant Worker as ENS DB Writer Worker
Ponder->>OnchainHandler: receive OnchainEvent
OnchainHandler->>InitModule: run preconditions (first call)
InitModule->>ENSDB: migrate ENSNode schema
InitModule->>ENSRAIN: wait for health (retry)
ENSRAIN-->>InitModule: healthy
InitModule->>ENSDB: fetch existing indexingMetadataContext & metadata
InitModule->>ENSDB: fetch omnichain snapshot & configs
InitModule->>InitModule: build IndexingMetadataContextInitialized
InitModule->>ENSDB: upsert IndexingMetadataContext
ENSDB-->>InitModule: upsert ok
InitModule->>ENSRAIN: wait for ready
ENSRAIN-->>InitModule: ready
InitModule->>Worker: start ENS DB writer worker
Worker->>ENSDB: periodically upsert updated IndexingMetadataContext
OnchainHandler-->>Ponder: continue handling event
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 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 |
Contributor
There was a problem hiding this comment.
Pull request overview
Introduces a new IndexingMetadataContext data model in @ensnode/ensnode-sdk to standardize how indexing metadata context is represented, validated, and (de)serialized, and refactors ENSIndexer’s Ponder initialization/preconditions into dedicated modules.
Changes:
- Added
IndexingMetadataContext(Uninitialized/Initialized) with Zod schemas plus serialize/deserialize/validate helpers in@ensnode/ensnode-sdk. - Refactored Ponder event-handler preconditions into
init-indexing-setupandinit-indexing-onchain-events, and adjusted event type IDs. - Removed ENSNode schema migration step from the Ponder API entrypoint (now triggered via indexing setup preconditions).
Reviewed changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 9 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/ensnode-sdk/src/ensnode/metadata/zod-schemas/indexing-metadata-context.ts | Zod schemas for serialized + runtime metadata context variants |
| packages/ensnode-sdk/src/ensnode/metadata/validate/indexing-metadata-context.ts | Validator helper for initialized context |
| packages/ensnode-sdk/src/ensnode/metadata/serialize/indexing-metadata-context.ts | Serialization helpers/types for metadata context |
| packages/ensnode-sdk/src/ensnode/metadata/deserialize/indexing-metadata-context.ts | Deserialization pipeline from serialized context into validated runtime context |
| packages/ensnode-sdk/src/ensnode/metadata/indexing-metadata-context.ts | Core types and builders for metadata context |
| packages/ensnode-sdk/src/ensnode/metadata/index.ts | Barrel export for metadata module |
| packages/ensnode-sdk/src/ensnode/index.ts | Re-exports the new metadata module from ensnode SDK |
| apps/ensindexer/src/lib/indexing-engines/ponder.ts | Refactors preconditions logic; splits setup/onchain initialization |
| apps/ensindexer/src/lib/indexing-engines/init-indexing-setup.ts | New setup initialization module (runs ENSNode schema migrations) |
| apps/ensindexer/src/lib/indexing-engines/init-indexing-onchain-events.ts | New onchain initialization module (waits for ENSRainbow readiness) |
| apps/ensindexer/src/lib/indexing-engines/ponder.test.ts | Updates tests to account for new precondition/migration mocking and PONDER_COMMON setup |
| apps/ensindexer/ponder/src/api/index.ts | Removes ENSNode schema migration before starting writer worker |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/ensindexer/src/lib/indexing-engines/ponder.test.ts (1)
10-51: 🧹 Nitpick | 🔵 TrivialAdd a test for the migration-failure precondition path.
mockMigrateEnsNodeSchemais wired up, but no test asserts what happens wheninitIndexingSetup()fails. Given this precondition runs before every setup handler and previously lived elsewhere, a regression test covering the rejection flow (setup handler is not invoked, error propagates from the registered callback) would close the coverage gap and catch any future changes to the setup branch ofeventHandlerPreconditions.♻️ Suggested test skeleton
describe("ENSNode schema migration preconditions (setup events)", () => { it("propagates migration failure and prevents setup handler execution", async () => { const { addOnchainEventListener } = await getPonderModule(); const handler = vi.fn().mockResolvedValue(undefined); mockMigrateEnsNodeSchema.mockRejectedValue(new Error("migration failed")); addOnchainEventListener("Registry:setup" as EventNames, handler); await expect( getRegisteredCallback()({ context: { db: vi.fn() } as unknown as Context<EventNames>, event: {} as IndexingEngineEvent<EventNames>, }), ).rejects.toThrow("migration failed"); expect(handler).not.toHaveBeenCalled(); }); it("runs migration only once across multiple setup events", async () => { const { addOnchainEventListener } = await getPonderModule(); const handler = vi.fn().mockResolvedValue(undefined); addOnchainEventListener("Registry:setup" as EventNames, handler); addOnchainEventListener("PublicResolver:setup" as EventNames, handler); await getRegisteredCallback(0)({ context: { db: vi.fn() } as unknown as Context<EventNames>, event: {} as IndexingEngineEvent<EventNames>, }); await getRegisteredCallback(1)({ context: { db: vi.fn() } as unknown as Context<EventNames>, event: {} as IndexingEngineEvent<EventNames>, }); expect(mockMigrateEnsNodeSchema).toHaveBeenCalledTimes(1); }); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ensindexer/src/lib/indexing-engines/ponder.test.ts` around lines 10 - 51, Add tests exercising the migration-failure precondition in apps/ensindexer by wiring mockMigrateEnsNodeSchema to reject and asserting initIndexingSetup's failure prevents the setup handler from running and that the error propagates; specifically, in the test suite using getPonderModule(), call addOnchainEventListener("Registry:setup", handler) with handler = vi.fn(), make mockMigrateEnsNodeSchema.mockRejectedValue(new Error("migration failed")), invoke the registered callback via getRegisteredCallback() and expect it to reject with that error and that handler was not called; also add a test that registers two setup listeners (e.g., "Registry:setup" and "PublicResolver:setup"), run both registered callbacks via getRegisteredCallback(0) and getRegisteredCallback(1), and assert mockMigrateEnsNodeSchema was called exactly once to ensure migration runs only once.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/ensindexer/src/lib/indexing-engines/init-indexing-setup.ts`:
- Line 42: Add a one-line comment above the dynamic import of
migrateEnsNodeSchema in initIndexingSetup explaining that the import is deferred
to avoid triggering the ensDbClient singleton's connection at module load time;
reference migrateEnsNodeSchema and ensDbClient in the comment so future
maintainers understand the side-effect and don't convert it to a static import.
- Around line 44-49: The log emitted in the migrateEnsNodeSchema() catch block
is using the wrong module string and an inaccurate message; update the
logger.error call in the migrateEnsNodeSchema().catch(...) handler to use the
correct module identifier (e.g., "ensindexer" or another appropriate module
name) and change the msg to accurately reflect the operation (e.g., "Failed to
migrate ENSNode schema" or similar), keeping the error payload intact so the
error details are preserved.
- Around line 41-53: Update the log message inside initIndexingSetup's
migrateEnsNodeSchema() catch to accurately describe the operation (e.g., "Failed
to run ENSNode schema migrations") and change the logger metadata to the correct
module label for the indexing layer (remove or replace module: "ponder-api" with
something like module: "indexing-engines"); retain logging of the error object.
Also add a brief comment above the dynamic import of migrateEnsNodeSchema
explaining why it is imported dynamically (to defer singleton/DB connection
initialization until after ENSDb is ready) so future readers understand the
rationale.
In
`@packages/ensnode-sdk/src/ensnode/metadata/deserialize/indexing-metadata-context.ts`:
- Around line 23-54: Rename the two helpers to drop the misleading "Schema"
suffix: change buildUnvalidatedIndexingMetadataContextSchema ->
buildUnvalidatedIndexingMetadataContext and
buildUnvalidatedIndexingMetadataContextInitializedSchema ->
buildUnvalidatedIndexingMetadataContextInitialized; update all internal
references/usages (including the .transform(...) call that currently passes
buildUnvalidatedIndexingMetadataContext and any imports/exports) and update the
JSDoc {`@link` ...} that references the old name to point to
buildUnvalidatedIndexingMetadataContext so names match the SDK convention (e.g.,
buildUnvalidatedCrossChainIndexingStatusSnapshot,
buildUnvalidatedEnsNodeStackInfo).
- Around line 35-44: Update the JSDoc for
buildUnvalidatedIndexingMetadataContextSchema: remove the redundant `@return` tag
that restates the summary and, if present, correct any incorrect type mention
(remove "IndexingMetadataContextInitialized") so the docs reflect the actual
return type Unvalidated<IndexingMetadataContext>; keep the summary and param
tags but drop the misleading/duplicative `@return` line to comply with the coding
guideline.
---
Outside diff comments:
In `@apps/ensindexer/src/lib/indexing-engines/ponder.test.ts`:
- Around line 10-51: Add tests exercising the migration-failure precondition in
apps/ensindexer by wiring mockMigrateEnsNodeSchema to reject and asserting
initIndexingSetup's failure prevents the setup handler from running and that the
error propagates; specifically, in the test suite using getPonderModule(), call
addOnchainEventListener("Registry:setup", handler) with handler = vi.fn(), make
mockMigrateEnsNodeSchema.mockRejectedValue(new Error("migration failed")),
invoke the registered callback via getRegisteredCallback() and expect it to
reject with that error and that handler was not called; also add a test that
registers two setup listeners (e.g., "Registry:setup" and
"PublicResolver:setup"), run both registered callbacks via
getRegisteredCallback(0) and getRegisteredCallback(1), and assert
mockMigrateEnsNodeSchema was called exactly once to ensure migration runs only
once.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 2d30f2e4-8a4d-4371-a2b0-cbc81e5b5876
📒 Files selected for processing (12)
apps/ensindexer/ponder/src/api/index.tsapps/ensindexer/src/lib/indexing-engines/init-indexing-onchain-events.tsapps/ensindexer/src/lib/indexing-engines/init-indexing-setup.tsapps/ensindexer/src/lib/indexing-engines/ponder.test.tsapps/ensindexer/src/lib/indexing-engines/ponder.tspackages/ensnode-sdk/src/ensnode/index.tspackages/ensnode-sdk/src/ensnode/metadata/deserialize/indexing-metadata-context.tspackages/ensnode-sdk/src/ensnode/metadata/index.tspackages/ensnode-sdk/src/ensnode/metadata/indexing-metadata-context.tspackages/ensnode-sdk/src/ensnode/metadata/serialize/indexing-metadata-context.tspackages/ensnode-sdk/src/ensnode/metadata/validate/indexing-metadata-context.tspackages/ensnode-sdk/src/ensnode/metadata/zod-schemas/indexing-metadata-context.ts
💤 Files with no reviewable changes (1)
- apps/ensindexer/ponder/src/api/index.ts
![vercel[bot]](https://avatars.githubusercontent.com/in/8329?s=60&v=4){kind=small}
It will just have a single recurring task to keep the stored `IndexingMetadataContext` up to date
This one was complaining about `ponder:api` imports being made from outside of `apps/ensindexer/ponder/src/api` dir. The dynamic imports solve that problem.
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 17 out of 17 changed files in this pull request and generated 9 comments.
Comments suppressed due to low confidence (1)
apps/ensindexer/src/lib/ensdb-writer-worker/ensdb-writer-worker.ts:75
- This file still imports and carries fields/docs for the previous worker responsibilities (pRetry/config validation, upserting ENSDb version/public config, etc.), but the implementation now only schedules IndexingMetadataContext upserts. Please remove now-unused imports/types/class fields and update the class JSDoc to match current behavior; otherwise this is likely to trip linting/TS unused checks and leaves misleading documentation.
import { getUnixTime, secondsToMilliseconds } from "date-fns";
import type { Duration } from "enssdk";
import pRetry from "p-retry";
import type { EnsDbWriter } from "@ensnode/ensdb-sdk";
import {
buildCrossChainIndexingStatusSnapshotOmnichain,
buildIndexingMetadataContextInitialized,
type CrossChainIndexingStatusSnapshot,
type EnsIndexerPublicConfig,
IndexingMetadataContextStatusCodes,
OmnichainIndexingStatusIds,
type OmnichainIndexingStatusSnapshot,
validateEnsIndexerPublicConfigCompatibility,
} from "@ensnode/ensnode-sdk";
import type { LocalPonderClient } from "@ensnode/ponder-sdk";
import type { IndexingStatusBuilder } from "@/lib/indexing-status-builder/indexing-status-builder";
import { logger } from "@/lib/logger";
import type { PublicConfigBuilder } from "@/lib/public-config-builder/public-config-builder";
/**
* Interval in seconds between two consecutive attempts to upsert
* the Indexing Status Snapshot record into ENSDb.
*/
const INDEXING_STATUS_RECORD_UPDATE_INTERVAL: Duration = 1;
/**
* ENSDb Writer Worker
*
* A worker responsible for writing ENSIndexer-related metadata into ENSDb, including:
* - ENSDb version
* - ENSIndexer Public Config
* - ENSIndexer Indexing Status Snapshots
*/
export class EnsDbWriterWorker {
/**
* Interval for recurring upserts of Indexing Status Snapshots into ENSDb.
*/
private indexingStatusInterval: ReturnType<typeof setInterval> | null = null;
/**
* ENSDb Client instance used by the worker to interact with ENSDb.
*/
private ensDbClient: EnsDbWriter;
/**
* Indexing Status Builder instance used by the worker to read ENSIndexer Indexing Status.
*/
private indexingStatusBuilder: IndexingStatusBuilder;
/**
* ENSIndexer Public Config Builder instance used by the worker to read ENSIndexer Public Config.
*/
private publicConfigBuilder: PublicConfigBuilder;
/**
* Local Ponder Client instance
*
* Used to get local Ponder app command.
*/
private localPonderClient: LocalPonderClient;
/**
* @param ensDbClient ENSDb Writer instance used by the worker to interact with ENSDb.
* @param publicConfigBuilder ENSIndexer Public Config Builder instance used by the worker to read ENSIndexer Public Config.
* @param indexingStatusBuilder Indexing Status Builder instance used by the worker to read ENSIndexer Indexing Status.
* @param localPonderClient Local Ponder Client instance, used to get local Ponder app command.
*/
constructor(
ensDbClient: EnsDbWriter,
publicConfigBuilder: PublicConfigBuilder,
indexingStatusBuilder: IndexingStatusBuilder,
localPonderClient: LocalPonderClient,
) {
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/ensindexer/src/lib/ensdb-writer-worker/ensdb-writer-worker.ts (1)
17-31: 🧹 Nitpick | 🔵 TrivialRename stale
INDEXING_STATUS_RECORD_*/indexingStatusIntervalidentifiers.After the migration to a single
IndexingMetadataContextrecord, these identifiers (constantINDEXING_STATUS_RECORD_UPDATE_INTERVALwith its JSDoc, and fieldindexingStatusIntervalwith its JSDoc on line 29) still describe an "Indexing Status Snapshot record". Renaming them to reference theIndexingMetadataContext(e.g.,INDEXING_METADATA_CONTEXT_UPDATE_INTERVAL,indexingMetadataContextInterval) keeps the worker internals consistent with the new data model and avoids confusion when reading the file.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ensindexer/src/lib/ensdb-writer-worker/ensdb-writer-worker.ts` around lines 17 - 31, Rename the stale identifiers that reference "Indexing Status Snapshot" to reflect the new single IndexingMetadataContext model: change the constant INDEXING_STATUS_RECORD_UPDATE_INTERVAL (and its JSDoc) to INDEXING_METADATA_CONTEXT_UPDATE_INTERVAL and update its JSDoc text accordingly, and rename the class field indexingStatusInterval (and its JSDoc) to indexingMetadataContextInterval; update all usages of these symbols within EnsDbWriterWorker (and related comments) so names consistently reference IndexingMetadataContext.
♻️ Duplicate comments (2)
packages/ensnode-sdk/src/ensnode/metadata/deserialize/indexing-metadata-context.ts (1)
67-69:⚠️ Potential issue | 🟡 MinorError message says "Cannot validate" in a deserialize function, and ignores
valueLabel.This was previously flagged but the current code still says
Cannot validate IndexingMetadataContext. For adeserialize*function the prefix should beCannot deserialize ..., and usinglabel(instead of the hardcoded type name) keeps the outer header consistent with the schema's inner field labels when a customvalueLabelis provided.✏️ Proposed fix
if (parsed.error) { - throw new Error(`Cannot validate IndexingMetadataContext:\n${prettifyError(parsed.error)}\n`); + throw new Error(`Cannot deserialize ${label}:\n${prettifyError(parsed.error)}\n`); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/ensnode-sdk/src/ensnode/metadata/deserialize/indexing-metadata-context.ts` around lines 67 - 69, The error thrown in the deserialize path currently says "Cannot validate IndexingMetadataContext" and hardcodes the type name; in the deserialize function handling parsed.error (where parsed.error and prettifyError are used) change the message to use the deserialize prefix and the dynamic label (use the function's valueLabel/label variable) so it reads like "Cannot deserialize {label}:\n{prettifyError(parsed.error)}"; update the throw in the deserialize* function that references IndexingMetadataContext to build the message from label/valueLabel instead of the hardcoded type name.apps/ensindexer/src/lib/ensdb-writer-worker/ensdb-writer-worker.ts (1)
92-108:⚠️ Potential issue | 🟡 MinorStale JSDoc, typo, and stale identifier names — previously flagged.
These were called out on a prior commit but are still present:
- Line 93 JSDoc: still says
Upsert the current Indexing Status Snapshot into ENSDb.— should reference theIndexingMetadataContext.- Line 106 error message: typo
should be be initialized first(duplicated "be").- Lines 125–126 comment still talks about "failure to retrieve the Indexing Status" — should reference the metadata context upsert flow.
📝 Proposed fix
/** - * Upsert the current Indexing Status Snapshot into ENSDb. + * Upsert the current Indexing Metadata Context into ENSDb. * * This method is called by the scheduler at regular intervals. * Errors are logged but not thrown, to keep the worker running. */ private async upsertIndexingMetadataContext(): Promise<void> { try { // get system timestamp for the current iteration const snapshotTime = getUnixTime(new Date()); const indexingMetadataContext = await this.ensDbClient.getIndexingMetadataContext(); if (indexingMetadataContext.statusCode === IndexingMetadataContextStatusCodes.Uninitialized) { throw new Error( - `Cannot upsert Indexing Status Snapshot into ENSDb because Indexing Metadata Context should be be initialized first`, + `Cannot upsert Indexing Metadata Context into ENSDb because it must be initialized first`, ); } @@ } catch (error) { logger.error({ msg: "Failed to upsert indexing metadata context", error, module: "EnsDbWriterWorker", }); - // Do not throw the error, as failure to retrieve the Indexing Status - // should not cause the ENSDb Writer Worker to stop functioning. + // Do not throw the error, as failure to upsert the Indexing Metadata + // Context should not cause the ENSDb Writer Worker to stop functioning. } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ensindexer/src/lib/ensdb-writer-worker/ensdb-writer-worker.ts` around lines 92 - 108, The JSDoc and inline messages in upsertIndexingMetadataContext() are stale/typoed: update the method JSDoc to reference "IndexingMetadataContext" (not "Indexing Status Snapshot"), fix the duplicated word in the thrown Error message inside upsertIndexingMetadataContext() (remove the extra "be"), and revise any comments that mention "failure to retrieve the Indexing Status" to instead reference the metadata context upsert flow; locate these texts around the method upsertIndexingMetadataContext, the thrown Error that uses IndexingMetadataContextStatusCodes.Uninitialized, and any adjacent comments inside the same method to update wording accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/ensindexer/src/lib/ensdb-writer-worker/ensdb-writer-worker.ts`:
- Around line 100-118: Parallelize the two independent async calls by kicking
them off together with Promise.all: call getIndexingMetadataContext() and
getOmnichainIndexingStatusSnapshot() concurrently, await Promise.all to receive
[indexingMetadataContext, omnichainSnapshot], then perform the existing check on
indexingMetadataContext.statusCode and build the updated context with
buildCrossChainIndexingStatusSnapshotOmnichain(snapshotTime) and
buildIndexingMetadataContextInitialized before calling
ensDbClient.upsertIndexingMetadataContext; this replaces the sequential awaits
of getIndexingMetadataContext and getOmnichainIndexingStatusSnapshot while
preserving the existing validation and upsert flow.
In `@packages/ensnode-sdk/src/ensnode/metadata/indexing-metadata-context.ts`:
- Around line 63-72: The builder buildIndexingMetadataContextInitialized should
accept an optional valueLabel parameter and pass it through to
validateIndexingMetadataContextInitialized; update the function signature to add
valueLabel?: string and forward it in the call
(validateIndexingMetadataContextInitialized({...}, valueLabel)) so the builder
API matches the validator/deserializer behavior and preserves error-labeling
consistency for IndexingMetadataContextInitialized.
In
`@packages/ensnode-sdk/src/ensnode/metadata/validate/indexing-metadata-context.ts`:
- Around line 18-22: The thrown error message currently hardcodes
"IndexingMetadataContextInitialized", which can mismatch a custom valueLabel;
update the throw in the validation branch to use the valueLabel (falling back to
the existing default) instead of the hardcoded string so the error header
matches the schema's inner field labels — modify the throw in the function that
checks result.error in validate/indexing-metadata-context.ts to interpolate
valueLabel (or its default) into the error prefix (ensure valueLabel is in scope
or passed through where the validation occurs).
---
Outside diff comments:
In `@apps/ensindexer/src/lib/ensdb-writer-worker/ensdb-writer-worker.ts`:
- Around line 17-31: Rename the stale identifiers that reference "Indexing
Status Snapshot" to reflect the new single IndexingMetadataContext model: change
the constant INDEXING_STATUS_RECORD_UPDATE_INTERVAL (and its JSDoc) to
INDEXING_METADATA_CONTEXT_UPDATE_INTERVAL and update its JSDoc text accordingly,
and rename the class field indexingStatusInterval (and its JSDoc) to
indexingMetadataContextInterval; update all usages of these symbols within
EnsDbWriterWorker (and related comments) so names consistently reference
IndexingMetadataContext.
---
Duplicate comments:
In `@apps/ensindexer/src/lib/ensdb-writer-worker/ensdb-writer-worker.ts`:
- Around line 92-108: The JSDoc and inline messages in
upsertIndexingMetadataContext() are stale/typoed: update the method JSDoc to
reference "IndexingMetadataContext" (not "Indexing Status Snapshot"), fix the
duplicated word in the thrown Error message inside
upsertIndexingMetadataContext() (remove the extra "be"), and revise any comments
that mention "failure to retrieve the Indexing Status" to instead reference the
metadata context upsert flow; locate these texts around the method
upsertIndexingMetadataContext, the thrown Error that uses
IndexingMetadataContextStatusCodes.Uninitialized, and any adjacent comments
inside the same method to update wording accordingly.
In
`@packages/ensnode-sdk/src/ensnode/metadata/deserialize/indexing-metadata-context.ts`:
- Around line 67-69: The error thrown in the deserialize path currently says
"Cannot validate IndexingMetadataContext" and hardcodes the type name; in the
deserialize function handling parsed.error (where parsed.error and prettifyError
are used) change the message to use the deserialize prefix and the dynamic label
(use the function's valueLabel/label variable) so it reads like "Cannot
deserialize {label}:\n{prettifyError(parsed.error)}"; update the throw in the
deserialize* function that references IndexingMetadataContext to build the
message from label/valueLabel instead of the hardcoded type name.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: fbf20fd0-a69b-4b70-b016-e8446280a039
📒 Files selected for processing (4)
apps/ensindexer/src/lib/ensdb-writer-worker/ensdb-writer-worker.tspackages/ensnode-sdk/src/ensnode/metadata/deserialize/indexing-metadata-context.tspackages/ensnode-sdk/src/ensnode/metadata/indexing-metadata-context.tspackages/ensnode-sdk/src/ensnode/metadata/validate/indexing-metadata-context.ts
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 27 out of 27 changed files in this pull request and generated 5 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Refactor logic into `IndexingMetadataContextBuilder` class and `StackInfoBuilder` class
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 33 out of 33 changed files in this pull request and generated 5 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| * Update the current Indexing Status Snapshot into ENSDb. | ||
| * | ||
| * This method is called by the scheduler at regular intervals. | ||
| * Errors are logged but not thrown, to keep the worker running. |
There was a problem hiding this comment.
The doc comment says errors are “not thrown, to keep the worker running”, but updateIndexingMetadataContext() sets process.exitCode and rethrows. Please align documentation and behavior (either swallow/log errors, or intentionally stop the worker/exit in a controlled way).
Suggested change
| * Errors are logged but not thrown, to keep the worker running. | |
| * Errors are logged, the process exit code is set to a non-zero value, | |
| * and the error is rethrown because this is treated as a critical failure. |
Comment on lines
+14
to
+18
| const result = makeIndexingMetadataContextInitializedSchema(valueLabel).safeParse( | ||
| maybeIndexingMetadataContext, | ||
| ); | ||
|
|
||
| if (result.error) { |
There was a problem hiding this comment.
Repo’s validator helpers typically branch on safeParse’s success flag (e.g. validateEnsIndexerPublicConfig in packages/ensnode-sdk/src/ensindexer/config/validate/ensindexer-public-config.ts). For consistency and clarity, prefer if (!result.success) here instead of checking result.error directly.
Comment on lines
+3
to
4
| import type { EnsNodeMetadata, EnsNodeMetadataKeys } from "../ensnode-metadata"; | ||
|
|
There was a problem hiding this comment.
EnsNodeMetadata is imported but only referenced in a JSDoc {@link ...}; this will be treated as an unused import by Biome/TS and can fail lint/CI. Remove EnsNodeMetadata from the import, or reference it via an import()-style JSDoc link that doesn’t require a TS import.
Comment on lines
23
to
27
| // Invariant: the public config is guaranteed to be available in ENSDb after | ||
| // application startup. | ||
| if (typeof publicConfig === "undefined") { | ||
| if (indexingMetadataContext.statusCode !== IndexingMetadataContextStatusCodes.Initialized) { | ||
| throw new Error("Unreachable: ENSIndexer Public Config is not available in ENSDb"); | ||
| } |
There was a problem hiding this comment.
/config can observe an uninitialized indexing metadata context shortly after startup with the new init flow. Throwing an “Unreachable” error here turns that expected transient state into a 500 without a structured response. Prefer returning a non-500 response (e.g. 503) or an explicit error payload while the context is still uninitialized.
| // Recurring update of the Indexing Metadata Context record in ENSDb. | ||
| this.indexingStatusInterval = setInterval( | ||
| () => this.upsertIndexingStatusSnapshot(), | ||
| () => this.updateIndexingMetadataContext(), |
There was a problem hiding this comment.
setInterval(() => this.updateIndexingMetadataContext()) invokes an async function without handling the returned promise. If updateIndexingMetadataContext() rejects (and it currently rethrows), this becomes an unhandled rejection and can crash the process depending on Node settings. Either swallow/log errors inside the callback (to keep the worker running) or explicitly stop the interval / exit the process, but avoid leaving a floating rejected promise.
Suggested change
| () => this.updateIndexingMetadataContext(), | |
| () => { | |
| void this.updateIndexingMetadataContext().catch(() => { | |
| // The update method already logs the error and sets a non-zero exit code. | |
| // Stop the interval so the process can terminate cleanly without leaving | |
| // a floating rejected promise behind. | |
| this.stop(); | |
| }); | |
| }, |
tk-o
force-pushed
the
feat/indexing-metadata-context
branch
from
0cb1767 to
98e6c45
Compare
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 33 out of 33 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
89
to
113
| /** | ||
| * Get validated ENSIndexer Public Config object for the ENSDb Writer Worker. | ||
| * | ||
| * The function retrieves the ENSIndexer Public Config object from both: | ||
| * - stored config in ENSDb, if available, and | ||
| * - in-memory config from ENSIndexer Client. | ||
| * | ||
| * If a stored config exists **and** the local Ponder app is **not** in dev | ||
| * mode, the in-memory config is validated for compatibility against the | ||
| * stored one. Validation is skipped if the local Ponder app is in dev mode, | ||
| * allowing to override the stored config in ENSDb with the current in-memory | ||
| * config, without having to keep them compatible. | ||
| * | ||
| * @returns The in-memory config when validation passes or no stored config | ||
| * exists. | ||
| * @throws Error if either fetch fails, or if the in-memory config is | ||
| * incompatible with the stored config. | ||
| */ | ||
| private async getValidatedEnsIndexerPublicConfig(): Promise<EnsIndexerPublicConfig> { | ||
| /** | ||
| * Fetch the in-memory config with retries, to handle potential transient errors | ||
| * in the ENSIndexer Public Config Builder (e.g. due to network issues). | ||
| * If the fetch fails after the defined number of retries, the error | ||
| * will be thrown and the worker will not start, as the ENSIndexer Public Config | ||
| * is a critical dependency for the worker's tasks. | ||
| */ | ||
| const configFetchRetries = 3; | ||
|
|
||
| logger.debug({ | ||
| msg: "Fetching ENSIndexer public config", | ||
| retries: configFetchRetries, | ||
| module: "EnsDbWriterWorker", | ||
| }); | ||
|
|
||
| const inMemoryConfigPromise = pRetry(() => this.publicConfigBuilder.getPublicConfig(), { | ||
| retries: configFetchRetries, | ||
| onFailedAttempt: ({ attemptNumber, retriesLeft }) => { | ||
| logger.warn({ | ||
| msg: "Config fetch attempt failed", | ||
| attempt: attemptNumber, | ||
| retriesLeft, | ||
| module: "EnsDbWriterWorker", | ||
| }); | ||
| }, | ||
| }); | ||
|
|
||
| let storedConfig: EnsIndexerPublicConfig | undefined; | ||
| let inMemoryConfig: EnsIndexerPublicConfig; | ||
|
|
||
| try { | ||
| [storedConfig, inMemoryConfig] = await Promise.all([ | ||
| this.ensDbClient.getEnsIndexerPublicConfig(), | ||
| inMemoryConfigPromise, | ||
| ]); | ||
| logger.info({ | ||
| msg: "Fetched ENSIndexer public config", | ||
| module: "EnsDbWriterWorker", | ||
| config: inMemoryConfig, | ||
| }); | ||
| } catch (error) { | ||
| const errorMessage = error instanceof Error ? error.message : "Unknown error"; | ||
|
|
||
| logger.error({ | ||
| msg: "Failed to fetch ENSIndexer public config", | ||
| error, | ||
| module: "EnsDbWriterWorker", | ||
| }); | ||
|
|
||
| // Throw the error to terminate the ENSIndexer process due to failed fetch of critical dependency | ||
| throw new Error(errorMessage, { | ||
| cause: error, | ||
| }); | ||
| } | ||
|
|
||
| // Validate in-memory config object compatibility with the stored one, | ||
| // if the stored one is available. | ||
| // The validation is skipped if the local Ponder app is running in dev mode. | ||
| // This is to improve the development experience during ENSIndexer | ||
| // development, by allowing to override the stored config in ENSDb with | ||
| // the current in-memory config, without having to keep them compatible. | ||
| if (storedConfig && !this.localPonderClient.isInDevMode) { | ||
| try { | ||
| validateEnsIndexerPublicConfigCompatibility(storedConfig, inMemoryConfig); | ||
| } catch (error) { | ||
| const errorMessage = error instanceof Error ? error.message : "Unknown error"; | ||
|
|
||
| logger.error({ | ||
| msg: "In-memory config incompatible with stored config", | ||
| error, | ||
| module: "EnsDbWriterWorker", | ||
| }); | ||
|
|
||
| // Throw the error to terminate the ENSIndexer process due to | ||
| // found config incompatibility | ||
| throw new Error(errorMessage, { | ||
| cause: error, | ||
| }); | ||
| } | ||
| } | ||
|
|
||
| return inMemoryConfig; | ||
| } | ||
|
|
||
| /** | ||
| * Upsert the current Indexing Status Snapshot into ENSDb. | ||
| * Update the current Indexing Status Snapshot into ENSDb. | ||
| * | ||
| * This method is called by the scheduler at regular intervals. | ||
| * Errors are logged but not thrown, to keep the worker running. | ||
| */ | ||
| private async upsertIndexingStatusSnapshot(): Promise<void> { | ||
| private async updateIndexingMetadataContext(): Promise<void> { | ||
| try { | ||
| // get system timestamp for the current iteration | ||
| const snapshotTime = getUnixTime(new Date()); | ||
|
|
||
| const omnichainSnapshot = await this.getValidatedIndexingStatusSnapshot(); | ||
|
|
||
| const crossChainSnapshot = buildCrossChainIndexingStatusSnapshotOmnichain( | ||
| omnichainSnapshot, | ||
| snapshotTime, | ||
| ); | ||
| const indexingMetadataContext = | ||
| await this.indexingMetadataContextBuilder.getIndexingMetadataContext(); | ||
|
|
||
| await this.ensDbClient.upsertIndexingStatusSnapshot(crossChainSnapshot); | ||
| await this.ensDbClient.upsertIndexingMetadataContext(indexingMetadataContext); | ||
| } catch (error) { | ||
| // If any error happens during the update of indexing metadata context record in ENSDb, | ||
| // we want to log the error and exit the process with a non-zero exit code, | ||
| // since this is a critical failure that prevents the ENSIndexer instance from functioning properly. | ||
| logger.error({ | ||
| msg: "Failed to upsert indexing status snapshot", | ||
| error, | ||
| msg: "Failed to update indexing metadata context record in ENSDb", | ||
| module: "EnsDbWriterWorker", | ||
| error, | ||
| }); | ||
| // Do not throw the error, as failure to retrieve the Indexing Status | ||
| // should not cause the ENSDb Writer Worker to stop functioning. | ||
| } | ||
| } | ||
|
|
||
| /** | ||
| * Get validated Omnichain Indexing Status Snapshot | ||
| * | ||
| * @returns Validated Omnichain Indexing Status Snapshot. | ||
| * @throws Error if the Omnichain Indexing Status is not in expected status yet. | ||
| */ | ||
| private async getValidatedIndexingStatusSnapshot(): Promise<OmnichainIndexingStatusSnapshot> { | ||
| const omnichainSnapshot = await this.indexingStatusBuilder.getOmnichainIndexingStatusSnapshot(); | ||
|
|
||
| // It only makes sense to write Indexing Status Snapshots into ENSDb once | ||
| // the indexing process has started, as before that there is no meaningful | ||
| // status to record. | ||
| // Invariant: the Omnichain Status must indicate that indexing has started already. | ||
| if (omnichainSnapshot.omnichainStatus === OmnichainIndexingStatusIds.Unstarted) { | ||
| throw new Error("Omnichain Status must not be 'Unstarted'."); | ||
| process.exitCode = 1; | ||
| throw error; | ||
| } |
There was a problem hiding this comment.
The docstring says errors are “logged but not thrown, to keep the worker running”, but the catch block sets process.exitCode and rethrows. Rethrowing here will surface as an unhandled promise rejection from the setInterval callback, and the interval will continue keeping the event loop alive (so exitCode alone won’t reliably terminate the process). Consider either (a) not throwing and implementing retry/backoff to keep the worker running, or (b) explicitly stopping the interval and terminating the process (or bubbling the error to a supervisor) so the worker fails fast without repeated unhandled rejections.
Comment on lines
+63
to
67
| // Recurring update of the Indexing Metadata Context record in ENSDb. | ||
| this.indexingStatusInterval = setInterval( | ||
| () => this.upsertIndexingStatusSnapshot(), | ||
| () => this.updateIndexingMetadataContext(), | ||
| secondsToMilliseconds(INDEXING_STATUS_RECORD_UPDATE_INTERVAL), | ||
| ); |
There was a problem hiding this comment.
setInterval(() => this.updateIndexingMetadataContext(), …) schedules an async task without awaiting it. If an iteration takes longer than the interval, multiple updates can overlap concurrently (and errors become unhandled rejections unless caught at the call site). Consider adding a re-entrancy guard / in-flight flag, or replacing setInterval with an async loop that awaits each update before sleeping for the next tick.
Comment on lines
+88
to
+90
| invariant_indexingStatusIsUnstartedForIndexingMetadataContextUninitialized( | ||
| inMemoryIndexingStatusSnapshot, | ||
| ); |
There was a problem hiding this comment.
Treating a missing record as Uninitialized and then enforcing omnichainStatus === Unstarted will break upgrades: existing deployments that don’t yet have an indexing_metadata_context row (but do have checkpoints / are already indexing) will restart with storedIndexingMetadataContext = Uninitialized and an in-memory status like Backfill/Following, causing a hard throw and preventing startup. Consider allowing initialization even when the in-memory status is not Unstarted (e.g., when the DB row is missing because of an upgrade), or add a data migration/backfill path from the legacy metadata records so restarts don’t fail.
Suggested change
| invariant_indexingStatusIsUnstartedForIndexingMetadataContextUninitialized( | |
| inMemoryIndexingStatusSnapshot, | |
| ); | |
| if ( | |
| inMemoryIndexingStatusSnapshot.omnichainStatus === | |
| OmnichainIndexingStatusIds.Unstarted | |
| ) { | |
| invariant_indexingStatusIsUnstartedForIndexingMetadataContextUninitialized( | |
| inMemoryIndexingStatusSnapshot, | |
| ); | |
| } else { | |
| logger.warn({ | |
| msg: `Indexing Metadata Context is "uninitialized" but the in-memory indexing status is already started; treating this as an upgrade/backfill scenario and rebuilding the stored context from in-memory state`, | |
| omnichainStatus: inMemoryIndexingStatusSnapshot.omnichainStatus, | |
| }); | |
| } |
Comment on lines
+31
to
+41
| async getStackInfo(): Promise<EnsIndexerStackInfo> { | ||
| if (typeof this.immutableStackInfo === "undefined") { | ||
| const ensDbPublicConfig = await this.ensDbClient.buildEnsDbPublicConfig(); | ||
| const ensIndexerPublicConfig = await this.publicConfigBuilder.getPublicConfig(); | ||
| const ensRainbowPublicConfig = await this.ensRainbowClient.config(); | ||
|
|
||
| this.immutableStackInfo = buildEnsIndexerStackInfo( | ||
| ensDbPublicConfig, | ||
| ensIndexerPublicConfig, | ||
| ensRainbowPublicConfig, | ||
| ); |
There was a problem hiding this comment.
The cache is value-based (immutableStackInfo) rather than promise-based. If getStackInfo() is called concurrently before the first call resolves, it will trigger duplicate work (multiple ENSDb version queries / config fetches / ENSRainbow requests) and then race to assign immutableStackInfo. Consider caching an in-flight promise (e.g., private stackInfoPromise?: Promise<EnsIndexerStackInfo>) so concurrent callers share the same build.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Lite PR
Tip: Review docs on the ENSNode PR process
Summary
Why
Testing
Notes for Reviewer (Optional)
Resolves #1884
Pre-Review Checklist (Blocking)