Conversation
|
|
The latest updates on your projects. Learn more about Vercel for GitHub. 3 Skipped Deployments
|
📝 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 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->>ENSRainbow: wait for health (retry)
ENSRainbow-->>InitModule: healthy
InitModule->>ENSDB: fetch existing indexingMetadataContext
InitModule->>ENSDB: fetch omnichain snapshot & configs
InitModule->>InitModule: build initialized IndexingMetadataContext
InitModule->>ENSDB: upsert IndexingMetadataContext
ENSDB-->>InitModule: upsert ok
InitModule->>ENSRainbow: wait for ready
ENSRainbow-->>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 docstrings
🧪 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 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.
| // Init the indexing setup just once. There will be multiple | ||
| // setup events executed during Ponder startup, but they will | ||
| // run sequentially, so we can just check if we have already | ||
| // initialized the indexing setup or not. | ||
| indexingSetupPromise = initIndexingSetup(); |
There was a problem hiding this comment.
initIndexingSetup() is only executed for :setup handlers. If Ponder skips setup handlers on restarts when checkpoints already exist (as documented in init-indexing-setup.ts), ENSNode schema migrations may never run on subsequent startups, leaving the writer/indexer running against an outdated schema. Consider ensuring migrations run on every startup (e.g., keep them in the API entrypoint) or invoke initIndexingSetup() from the onchain precondition path as well.
| import { startEnsDbWriterWorker } from "@/lib/ensdb-writer-worker/singleton"; | ||
| import { logger } from "@/lib/logger"; | ||
|
|
||
| import ensNodeApi from "./handlers/ensnode-api"; | ||
|
|
There was a problem hiding this comment.
The API now starts startEnsDbWriterWorker() without first ensuring the ENSNode schema migrations have been applied. If migrateEnsNodeSchema() is only run from Ponder setup handlers, restarts where setup handlers are skipped can leave the worker using an outdated schema. Consider restoring a migration step here (or otherwise guaranteeing migrations execute before starting the worker).
| import { startEnsDbWriterWorker } from "@/lib/ensdb-writer-worker/singleton"; | |
| import { logger } from "@/lib/logger"; | |
| import ensNodeApi from "./handlers/ensnode-api"; | |
| import { migrateEnsNodeSchema, startEnsDbWriterWorker } from "@/lib/ensdb-writer-worker/singleton"; | |
| import { logger } from "@/lib/logger"; | |
| import ensNodeApi from "./handlers/ensnode-api"; | |
| // Ensure ENSNode schema migrations are applied before starting the writer worker. | |
| await migrateEnsNodeSchema(); |
| msg: "Failed to initialize ENSNode metadata", | ||
| error, | ||
| module: "ponder-api", |
There was a problem hiding this comment.
The log context here looks copied from the old ponder API initialization: the message is about "initialize ENSNode metadata" and module is set to "ponder-api", but this function is specifically running schema migrations as part of indexing setup. Updating the message/module to reflect the actual operation will make errors easier to triage.
| msg: "Failed to initialize ENSNode metadata", | |
| error, | |
| module: "ponder-api", | |
| msg: "Failed to migrate ENSNode schema during indexing setup", | |
| error, | |
| module: "indexing-setup", |
| */ | ||
| export const IndexingMetadataContextStatusCodes = { | ||
| /** | ||
| * Represents that the no indexing metadata context has been initialized |
There was a problem hiding this comment.
Grammar: "the no" reads like a typo in this doc comment.
| * Represents that the no indexing metadata context has been initialized | |
| * Represents that no indexing metadata context has been initialized |
| ): IndexingMetadataContextInitialized { | ||
| const result = makeIndexingMetadataContextInitializedSchema().safeParse( |
There was a problem hiding this comment.
This validator is exported but, unlike other validate* helpers in this repo, it doesn't accept an optional valueLabel to improve Zod error paths. Consider adding valueLabel?: string and passing it through to makeIndexingMetadataContextInitializedSchema(valueLabel) for consistency with e.g. validateEnsNodeStackInfo / validateCrossChainIndexingStatusSnapshot.
| ): IndexingMetadataContextInitialized { | |
| const result = makeIndexingMetadataContextInitializedSchema().safeParse( | |
| valueLabel?: string, | |
| ): IndexingMetadataContextInitialized { | |
| const result = makeIndexingMetadataContextInitializedSchema(valueLabel).safeParse( |
| * Builds an unvalidated {@link IndexingMetadataContextInitialized} object. | ||
| */ | ||
| function buildUnvalidatedIndexingMetadataContextInitializedSchema( | ||
| serializedIndexingMetadataContext: SerializedIndexingMetadataContextInitialized, | ||
| ): Unvalidated<IndexingMetadataContextInitialized> { | ||
| return { |
There was a problem hiding this comment.
These helpers build objects, not Zod schemas. The *Schema suffix is inconsistent with similar code (e.g. buildUnvalidatedCrossChainIndexingStatusSnapshot) and makes it harder to tell what's a schema vs a transformer.
| * Builds an unvalidated {@link IndexingMetadataContext} object to be | ||
| * validated with {@link makeIndexingMetadataContextSchema}. | ||
| * | ||
| * @param serializedIndexingMetadataContext - The serialized indexing metadata context to build from. | ||
| * @return An unvalidated {@link IndexingMetadataContextInitialized} object. | ||
| */ | ||
| function buildUnvalidatedIndexingMetadataContextSchema( | ||
| serializedIndexingMetadataContext: SerializedIndexingMetadataContext, | ||
| ): Unvalidated<IndexingMetadataContext> { |
There was a problem hiding this comment.
JSDoc mismatch: this function returns an unvalidated IndexingMetadataContext union, not specifically an IndexingMetadataContextInitialized. Update the @return text to match the actual return type.
| .safeParse(serializedIndexingMetadataContext); | ||
|
|
||
| if (parsed.error) { | ||
| throw new Error(`Cannot validate IndexingMetadataContext:\n${prettifyError(parsed.error)}\n`); |
There was a problem hiding this comment.
The thrown error says "Cannot validate IndexingMetadataContext" but this function is deserializing a serialized context. Consider aligning the message with other deserializers (e.g., "Cannot deserialize into IndexingMetadataContext") to make failures clearer.
| throw new Error(`Cannot validate IndexingMetadataContext:\n${prettifyError(parsed.error)}\n`); | |
| throw new Error( | |
| `Cannot deserialize into IndexingMetadataContext:\n${prettifyError(parsed.error)}\n`, | |
| ); |
| export type IndexingMetadataContext = | ||
| | IndexingMetadataContextUninitialized | ||
| | IndexingMetadataContextInitialized; | ||
|
|
||
| /** | ||
| * Build an {@link IndexingMetadataContextUninitialized} object. | ||
| */ | ||
| export function buildIndexingMetadataContextUninitialized(): IndexingMetadataContextUninitialized { | ||
| return { | ||
| statusCode: IndexingMetadataContextStatusCodes.Uninitialized, | ||
| }; | ||
| } | ||
|
|
||
| /** | ||
| * Build an {@link IndexingMetadataContextInitialized} object. | ||
| * | ||
| * @throws Error if the provided parameters do not satisfy the validation | ||
| * criteria for an {@link IndexingMetadataContextInitialized} object. | ||
| */ | ||
| export function buildIndexingMetadataContextInitialized( | ||
| indexingStatus: CrossChainIndexingStatusSnapshot, | ||
| stackInfo: EnsNodeStackInfo, | ||
| ): IndexingMetadataContextInitialized { | ||
| return validateIndexingMetadataContextInitialized({ | ||
| statusCode: IndexingMetadataContextStatusCodes.Initialized, | ||
| indexingStatus, | ||
| stackInfo, | ||
| }); | ||
| } |
There was a problem hiding this comment.
New indexing-metadata-context model introduces build/serialize/deserialize/validate logic but no unit tests. The SDK has extensive snapshot/serialization tests elsewhere (e.g. under src/indexing-status/*); adding a small test suite for both statusCode variants would help prevent regressions.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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
| export async function initIndexingSetup(): Promise<void> { | ||
| const { migrateEnsNodeSchema } = await import("@/lib/ensdb/migrate-ensnode-schema"); | ||
| // Ensure the ENSNode Schema in ENSDb is up to date by running any pending migrations. | ||
| await migrateEnsNodeSchema().catch((error) => { | ||
| logger.error({ | ||
| msg: "Failed to initialize ENSNode metadata", | ||
| error, | ||
| module: "ponder-api", | ||
| }); | ||
| process.exitCode = 1; | ||
| throw error; | ||
| }); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Does Ponder framework run ":setup" event handlers on restart when checkpoints are already initialized, or are setup handlers skipped?
💡 Result:
Ponder framework's ":setup" event handlers run on restart when checkpoints are already initialized. The setup handlers execute before indexing begins, as part of the instance lifecycle where user code (including indexing functions like :setup) is built early, followed by backfill resuming from checkpoints via crash recovery. Official docs confirm ":setup" runs "before indexing begins," and crash recovery rolls back unfinalized changes then resumes indexing from the finalized checkpoint block, implying setup re-runs at the start of this process to ensure initialization (e.g., singletons) before continuing event processing.
Citations:
- 1: https://ponder.sh/docs/api-reference/ponder/database
- 2: https://ponder.sh/docs/api-reference/ponder/indexing-functions
- 3: https://ponder.sh/docs/indexing/overview
🏁 Script executed:
cat -n apps/ensindexer/src/lib/indexing-engines/init-indexing-setup.ts | head -60Repository: namehash/ensnode
Length of output: 2400
Improve log message and add comment explaining dynamic import.
The error log message "Failed to initialize ENSNode metadata" is misleading—this function specifically runs schema migrations, not metadata initialization. Change the message to clarify the actual operation (e.g., "Failed to run ENSNode schema migrations"). Also remove or correct the module: "ponder-api" label; this code is in the indexing-engines layer, not the API layer.
The dynamic import on line 42 should include a comment explaining why migrateEnsNodeSchema is imported dynamically rather than statically (likely to defer singleton instantiation and database connection until after ENSDb initialization). This helps future readers understand the design rationale.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/ensindexer/src/lib/indexing-engines/init-indexing-setup.ts` around lines
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.
| * @throws Error if any precondition is not satisfied. | ||
| */ | ||
| export async function initIndexingSetup(): Promise<void> { | ||
| const { migrateEnsNodeSchema } = await import("@/lib/ensdb/migrate-ensnode-schema"); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider documenting why the import is dynamic.
migrate-ensnode-schema.ts transitively imports the ensDbClient singleton, which connects to ENSDb at module-load time. Deferring the import until initIndexingSetup() runs avoids that side effect at module load (e.g., during tests or when the function is never reached). A one-liner comment here would prevent someone from "fixing" this into a static import later.
🧹 Optional clarification
export async function initIndexingSetup(): Promise<void> {
+ // Dynamic import so the ENSDb client singleton (transitively imported by
+ // migrate-ensnode-schema) isn't instantiated at module-load time.
const { migrateEnsNodeSchema } = await import("@/lib/ensdb/migrate-ensnode-schema");🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/ensindexer/src/lib/indexing-engines/init-indexing-setup.ts` at 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.
| function buildUnvalidatedIndexingMetadataContextInitializedSchema( | ||
| serializedIndexingMetadataContext: SerializedIndexingMetadataContextInitialized, | ||
| ): Unvalidated<IndexingMetadataContextInitialized> { | ||
| return { | ||
| statusCode: serializedIndexingMetadataContext.statusCode, | ||
| indexingStatus: buildUnvalidatedCrossChainIndexingStatusSnapshot( | ||
| serializedIndexingMetadataContext.indexingStatus, | ||
| ), | ||
| stackInfo: buildUnvalidatedEnsNodeStackInfo(serializedIndexingMetadataContext.stackInfo), | ||
| }; | ||
| } | ||
|
|
||
| /** | ||
| * Builds an unvalidated {@link IndexingMetadataContext} object to be | ||
| * validated with {@link makeIndexingMetadataContextSchema}. | ||
| * | ||
| * @param serializedIndexingMetadataContext - The serialized indexing metadata context to build from. | ||
| * @return An unvalidated {@link IndexingMetadataContextInitialized} object. | ||
| */ | ||
| function buildUnvalidatedIndexingMetadataContextSchema( | ||
| serializedIndexingMetadataContext: SerializedIndexingMetadataContext, | ||
| ): Unvalidated<IndexingMetadataContext> { | ||
| switch (serializedIndexingMetadataContext.statusCode) { | ||
| case IndexingMetadataContextStatusCodes.Uninitialized: | ||
| return serializedIndexingMetadataContext; | ||
|
|
||
| case IndexingMetadataContextStatusCodes.Initialized: | ||
| return buildUnvalidatedIndexingMetadataContextInitializedSchema( | ||
| serializedIndexingMetadataContext, | ||
| ); | ||
| } | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Rename helpers: drop misleading Schema suffix.
buildUnvalidatedIndexingMetadataContextSchema and buildUnvalidatedIndexingMetadataContextInitializedSchema don't build Zod schemas — they build plain Unvalidated<...> objects. The Schema suffix is misleading and inconsistent with the existing convention in this SDK (e.g., buildUnvalidatedIndexingStatusResponse, buildUnvalidatedCrossChainIndexingStatusSnapshot, buildUnvalidatedEnsNodeStackInfo).
♻️ Proposed rename
-function buildUnvalidatedIndexingMetadataContextInitializedSchema(
+function buildUnvalidatedIndexingMetadataContextInitialized(
serializedIndexingMetadataContext: SerializedIndexingMetadataContextInitialized,
): Unvalidated<IndexingMetadataContextInitialized> {
...
}
-function buildUnvalidatedIndexingMetadataContextSchema(
+function buildUnvalidatedIndexingMetadataContext(
serializedIndexingMetadataContext: SerializedIndexingMetadataContext,
): Unvalidated<IndexingMetadataContext> {
switch (serializedIndexingMetadataContext.statusCode) {
case IndexingMetadataContextStatusCodes.Uninitialized:
return serializedIndexingMetadataContext;
case IndexingMetadataContextStatusCodes.Initialized:
- return buildUnvalidatedIndexingMetadataContextInitializedSchema(
+ return buildUnvalidatedIndexingMetadataContextInitialized(
serializedIndexingMetadataContext,
);
}
}Don't forget to update the reference in the {@link ...} JSDoc and in .transform(buildUnvalidatedIndexingMetadataContextSchema) on line 66.
🤖 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 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).
| /** | ||
| * Builds an unvalidated {@link IndexingMetadataContext} object to be | ||
| * validated with {@link makeIndexingMetadataContextSchema}. | ||
| * | ||
| * @param serializedIndexingMetadataContext - The serialized indexing metadata context to build from. | ||
| * @return An unvalidated {@link IndexingMetadataContextInitialized} object. | ||
| */ | ||
| function buildUnvalidatedIndexingMetadataContextSchema( | ||
| serializedIndexingMetadataContext: SerializedIndexingMetadataContext, | ||
| ): Unvalidated<IndexingMetadataContext> { |
There was a problem hiding this comment.
Fix incorrect @return and drop redundant tag.
The @return describes IndexingMetadataContextInitialized, but this function returns Unvalidated<IndexingMetadataContext> (the union, not only the Initialized variant). Additionally, the tag merely restates the summary and should be removed per coding guidelines.
✏️ Proposed fix
/**
* Builds an unvalidated {`@link` IndexingMetadataContext} object to be
* validated with {`@link` makeIndexingMetadataContextSchema}.
- *
- * `@param` serializedIndexingMetadataContext - The serialized indexing metadata context to build from.
- * `@return` An unvalidated {`@link` IndexingMetadataContextInitialized} object.
*/
function buildUnvalidatedIndexingMetadataContextSchema(As per coding guidelines: "Do not add JSDoc @returns tags that merely restate the method summary; remove such redundancy during PR review".
📝 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.
| /** | |
| * Builds an unvalidated {@link IndexingMetadataContext} object to be | |
| * validated with {@link makeIndexingMetadataContextSchema}. | |
| * | |
| * @param serializedIndexingMetadataContext - The serialized indexing metadata context to build from. | |
| * @return An unvalidated {@link IndexingMetadataContextInitialized} object. | |
| */ | |
| function buildUnvalidatedIndexingMetadataContextSchema( | |
| serializedIndexingMetadataContext: SerializedIndexingMetadataContext, | |
| ): Unvalidated<IndexingMetadataContext> { | |
| /** | |
| * Builds an unvalidated {`@link` IndexingMetadataContext} object to be | |
| * validated with {`@link` makeIndexingMetadataContextSchema}. | |
| * | |
| * `@param` serializedIndexingMetadataContext - The serialized indexing metadata context to build from. | |
| */ | |
| function buildUnvalidatedIndexingMetadataContextSchema( | |
| serializedIndexingMetadataContext: SerializedIndexingMetadataContext, | |
| ): Unvalidated<IndexingMetadataContext> { |
🤖 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 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.
![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.
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.
| /** | ||
| * Builds an unvalidated {@link IndexingMetadataContextInitialized} object. | ||
| */ | ||
| function buildUnvalidatedIndexingMetadataContextInitializedSchema( | ||
| serializedIndexingMetadataContext: SerializedIndexingMetadataContextInitialized, | ||
| ): Unvalidated<IndexingMetadataContextInitialized> { | ||
| return { | ||
| statusCode: serializedIndexingMetadataContext.statusCode, | ||
| indexingStatus: buildUnvalidatedCrossChainIndexingStatusSnapshot( | ||
| serializedIndexingMetadataContext.indexingStatus, | ||
| ), | ||
| stackInfo: buildUnvalidatedEnsIndexerStackInfo(serializedIndexingMetadataContext.stackInfo), | ||
| }; | ||
| } | ||
|
|
||
| /** | ||
| * Builds an unvalidated {@link IndexingMetadataContext} object to be | ||
| * validated with {@link makeIndexingMetadataContextSchema}. | ||
| * | ||
| * @param serializedIndexingMetadataContext - The serialized indexing metadata context to build from. | ||
| * @return An unvalidated {@link IndexingMetadataContextInitialized} object. | ||
| */ | ||
| function buildUnvalidatedIndexingMetadataContextSchema( | ||
| serializedIndexingMetadataContext: SerializedIndexingMetadataContext, | ||
| ): Unvalidated<IndexingMetadataContext> { | ||
| switch (serializedIndexingMetadataContext.statusCode) { | ||
| case IndexingMetadataContextStatusCodes.Uninitialized: | ||
| return serializedIndexingMetadataContext; | ||
|
|
||
| case IndexingMetadataContextStatusCodes.Initialized: | ||
| return buildUnvalidatedIndexingMetadataContextInitializedSchema( | ||
| serializedIndexingMetadataContext, | ||
| ); | ||
| } | ||
| } |
There was a problem hiding this comment.
These helper functions are named *Schema but they build unvalidated objects, not Zod schemas, and the JSDoc @return also claims it always returns an Initialized object even though it returns the full union. Renaming to buildUnvalidatedIndexingMetadataContext* (without "Schema") and fixing the doc will make the deserialization pipeline clearer to maintain.
| const indexingMetadataContext = await ensDbClient.getIndexingMetadataContext(); | ||
| console.log("Indexing Metadata Context:", indexingMetadataContext); | ||
| const indexingStatus = await indexingStatusBuilder.getOmnichainIndexingStatusSnapshot(); | ||
| const ensIndexerPublicConfig = await publicConfigBuilder.getPublicConfig(); | ||
| const ensDbPublicConfig = await ensDbClient.buildEnsDbPublicConfig(); | ||
|
|
||
| if (indexingMetadataContext.statusCode === IndexingMetadataContextStatusCodes.Uninitialized) { | ||
| // Invariant: indexing status must be "unstarted" | ||
| if (indexingStatus.omnichainStatus !== OmnichainIndexingStatusIds.Unstarted) { | ||
| throw new Error( | ||
| `Invariant violation: expected omnichain indexing status to be "unstarted" when initializing indexing of onchain events for the first time, but got "${indexingStatus.omnichainStatus}" instead`, | ||
| ); | ||
| } | ||
| } else { | ||
| // if (ensIndexerPublicConfig.ensIndexerBuildId !== indexingMetadataContext.stackInfo.ensIndexer.ensIndexerBuildId) { | ||
| // TODO: store the `ensIndexerPublicConfig` object in ENSDb so `indexingMetadataContext.stackInfo.ensIndexer` is updated | ||
| // } | ||
| } | ||
|
|
||
| await waitForEnsRainbowToBeReady(); | ||
|
|
||
| const ensRainbowPublicConfig = await ensRainbowClient.config(); | ||
| const now = getUnixTime(new Date()); | ||
| const updatedIndexingMetadataContext = buildIndexingMetadataContextInitialized( | ||
| buildCrossChainIndexingStatusSnapshotOmnichain(indexingStatus, now), | ||
| buildEnsIndexerStackInfo(ensDbPublicConfig, ensIndexerPublicConfig, ensRainbowPublicConfig), | ||
| ); | ||
|
|
||
| // TODO: check ENSRainbow compatibility | ||
| if ( | ||
| ensRainbowPublicConfig.serverLabelSet.labelSetId < | ||
| ensIndexerPublicConfig.clientLabelSet.labelSetId | ||
| ) { | ||
| throw new Error( | ||
| `ENSRainbow instance is not compatible with the current ENSIndexer instance: ENSRainbow serverLabelSetId (${ensRainbowPublicConfig.serverLabelSet.labelSetId}) is less than ENSIndexer clientLabelSetId (${ensIndexerPublicConfig.clientLabelSet.labelSetId})`, | ||
| ); | ||
| } | ||
|
|
||
| await ensDbClient.upsertIndexingMetadataContext(updatedIndexingMetadataContext); | ||
|
|
||
| // TODO: start Indexing Status Sync worker | ||
| // It will be responsible for keeping the indexing status stored within Indexing Metadata Context record in ENSDb up to date | ||
| // await indexingStatusSyncWorker.start(); | ||
| startEnsDbWriterWorker(); | ||
| } catch (error) { | ||
| // If any error happens during the execution of the preconditions for onchain events, | ||
| // 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. | ||
| console.error("Failed to execute preconditions for onchain events:", error); | ||
| process.exit(1); | ||
| } |
There was a problem hiding this comment.
This initialization path still uses console.log/console.error and hard process.exit(1). In this codebase other runtime failures are logged via the structured logger and typically set process.exitCode + throw (to allow cleanup and to keep unit tests from exiting the runner). Consider replacing console usage with logger and propagating the error rather than calling process.exit inside a library-style init function.
| const indexingMetadataContext = await ensDbClient.getIndexingMetadataContext(); | ||
| console.log("Indexing Metadata Context:", indexingMetadataContext); | ||
| const indexingStatus = await indexingStatusBuilder.getOmnichainIndexingStatusSnapshot(); | ||
| const ensIndexerPublicConfig = await publicConfigBuilder.getPublicConfig(); | ||
| const ensDbPublicConfig = await ensDbClient.buildEnsDbPublicConfig(); | ||
|
|
||
| if (indexingMetadataContext.statusCode === IndexingMetadataContextStatusCodes.Uninitialized) { | ||
| // Invariant: indexing status must be "unstarted" | ||
| if (indexingStatus.omnichainStatus !== OmnichainIndexingStatusIds.Unstarted) { | ||
| throw new Error( | ||
| `Invariant violation: expected omnichain indexing status to be "unstarted" when initializing indexing of onchain events for the first time, but got "${indexingStatus.omnichainStatus}" instead`, | ||
| ); | ||
| } | ||
| } else { | ||
| // if (ensIndexerPublicConfig.ensIndexerBuildId !== indexingMetadataContext.stackInfo.ensIndexer.ensIndexerBuildId) { | ||
| // TODO: store the `ensIndexerPublicConfig` object in ENSDb so `indexingMetadataContext.stackInfo.ensIndexer` is updated | ||
| // } | ||
| } | ||
|
|
||
| await waitForEnsRainbowToBeReady(); | ||
|
|
||
| const ensRainbowPublicConfig = await ensRainbowClient.config(); | ||
| const now = getUnixTime(new Date()); | ||
| const updatedIndexingMetadataContext = buildIndexingMetadataContextInitialized( | ||
| buildCrossChainIndexingStatusSnapshotOmnichain(indexingStatus, now), | ||
| buildEnsIndexerStackInfo(ensDbPublicConfig, ensIndexerPublicConfig, ensRainbowPublicConfig), | ||
| ); | ||
|
|
||
| // TODO: check ENSRainbow compatibility | ||
| if ( | ||
| ensRainbowPublicConfig.serverLabelSet.labelSetId < | ||
| ensIndexerPublicConfig.clientLabelSet.labelSetId | ||
| ) { | ||
| throw new Error( | ||
| `ENSRainbow instance is not compatible with the current ENSIndexer instance: ENSRainbow serverLabelSetId (${ensRainbowPublicConfig.serverLabelSet.labelSetId}) is less than ENSIndexer clientLabelSetId (${ensIndexerPublicConfig.clientLabelSet.labelSetId})`, | ||
| ); | ||
| } | ||
|
|
||
| await ensDbClient.upsertIndexingMetadataContext(updatedIndexingMetadataContext); | ||
|
|
||
| // TODO: start Indexing Status Sync worker | ||
| // It will be responsible for keeping the indexing status stored within Indexing Metadata Context record in ENSDb up to date | ||
| // await indexingStatusSyncWorker.start(); | ||
| startEnsDbWriterWorker(); |
There was a problem hiding this comment.
The init routine currently does not upsert EnsIndexerPublicConfig or the legacy indexing status snapshot record, but other parts of the app (ENSNode HTTP API handlers) still read those keys from ENSDb and assume they are present. If IndexingMetadataContext is intended to replace those records, the readers/endpoints need to be migrated in the same change (or the legacy records should continue to be written for compatibility).
| */ | ||
| export const IndexingMetadataContextStatusCodes = { | ||
| /** | ||
| * Represents that the no indexing metadata context has been initialized |
There was a problem hiding this comment.
Spelling/grammar: "Represents that the no indexing metadata context has been initialized" should be "Represents that no indexing metadata context has been initialized".
| * Represents that the no indexing metadata context has been initialized | |
| * Represents that no indexing metadata context has been initialized |
| const makeSerializedIndexingMetadataContextUninitializedSchema = (_valueLabel?: string) => { | ||
| return z.object({ | ||
| statusCode: z.literal(IndexingMetadataContextStatusCodes.Uninitialized), | ||
| }); | ||
| }; |
There was a problem hiding this comment.
makeSerializedIndexingMetadataContextUninitializedSchema accepts a label argument but doesn’t use it (the param is prefixed with _). Either remove the argument and stop passing label, or wire it into the schema creation (consistent with the other helpers) so error paths include the label context.
| const omnichainSnapshot = await this.getValidatedIndexingStatusSnapshot(); | ||
| if (indexingMetadataContext.statusCode === IndexingMetadataContextStatusCodes.Uninitialized) { | ||
| throw new Error( | ||
| `Cannot upsert Indexing Status Snapshot into ENSDb because Indexing Metadata Context should be be initialized first`, |
There was a problem hiding this comment.
The error message has a typo ("should be be") and refers to upserting an "Indexing Status Snapshot" even though this method now upserts Indexing Metadata Context. Please fix the wording so logs are accurate and searchable.
| `Cannot upsert Indexing Status Snapshot into ENSDb because Indexing Metadata Context should be be initialized first`, | |
| `Cannot upsert Indexing Metadata Context into ENSDb because Indexing Metadata Context should be initialized first`, |
| @@ -21,10 +40,15 @@ vi.mock("@/lib/ensrainbow/singleton", () => ({ | |||
| waitForEnsRainbowToBeReady: mockWaitForEnsRainbow, | |||
| })); | |||
|
|
|||
| vi.mock("@/lib/ensdb/migrate-ensnode-schema", () => ({ | |||
| migrateEnsNodeSchema: mockMigrateEnsNodeSchema, | |||
| })); | |||
|
|
|||
There was a problem hiding this comment.
These tests mock waitForEnsRainbowToBeReady/migrateEnsNodeSchema, but event preconditions now call initIndexingOnchainEvents(), which also touches ENSDb/ENSRainbow/indexingStatus/publicConfig singletons and can call process.exit(1) on failure. To keep this a unit test of ponder.ts, mock ./init-indexing-onchain-events (and ./init-indexing-setup if needed) or mock the additional singletons so the test doesn’t reach real runtime dependencies.
| export interface SerializedEnsNodeMetadataIndexingMetadataContext { | ||
| key: typeof EnsNodeMetadataKeys.IndexingMetadataContext; | ||
| value: SerializedIndexingMetadataContextInitialized; | ||
| } |
There was a problem hiding this comment.
SerializedEnsNodeMetadataIndexingMetadataContext currently restricts value to SerializedIndexingMetadataContextInitialized, but the new ENSNode metadata model is a discriminated union (Uninitialized | Initialized). Consider typing this as SerializedIndexingMetadataContext so ENSDb records can represent all variants and getIndexingMetadataContext() can round-trip whatever is stored.
| export interface EnsNodeMetadataIndexingMetadataContext { | ||
| key: typeof EnsNodeMetadataKeys.IndexingMetadataContext; | ||
| value: IndexingMetadataContextInitialized; | ||
| } |
There was a problem hiding this comment.
EnsNodeMetadataIndexingMetadataContext types value as IndexingMetadataContextInitialized, but the reader API returns IndexingMetadataContext (union) and the data model includes an Uninitialized variant. To avoid an inconsistent public API, consider changing this metadata record type to store the full IndexingMetadataContext union (and update writer/serializer types accordingly).
| * validated with {@link makeIndexingMetadataContextSchema}. | ||
| * | ||
| * @param serializedIndexingMetadataContext - The serialized indexing metadata context to build from. | ||
| * @return An unvalidated {@link IndexingMetadataContextInitialized} object. |
There was a problem hiding this comment.
JSDoc @return tag incorrectly states the return type is IndexingMetadataContextInitialized instead of the union type IndexingMetadataContext
| ): IndexingMetadataContextInitialized { | ||
| const result = makeIndexingMetadataContextInitializedSchema().safeParse( |
There was a problem hiding this comment.
The validateIndexingMetadataContextInitialized function should accept an optional valueLabel parameter for consistent error messaging
tk-o
force-pushed
the
feat/indexing-metadata-context
branch
from
2ddb70a to
7595a41
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 20 out of 20 changed files in this pull request and generated 5 comments.
Comments suppressed due to low confidence (2)
packages/ensdb-sdk/src/client/serialize/ensnode-metadata.ts:13
EnsNodeMetadataKeysis imported withimport type, but it’s used intypeof EnsNodeMetadataKeys.*type queries below. Type queries require the value symbol, so this will fail TypeScript compilation (“cannot be used as a value because it was imported using 'import type'”). ImportEnsNodeMetadataKeysas a value (non-type import), or replace thesetypeofqueries with string literal key types.
import type {
EnsNodeMetadata,
EnsNodeMetadataEnsDbVersion,
EnsNodeMetadataEnsIndexerIndexingStatus,
EnsNodeMetadataEnsIndexerPublicConfig,
EnsNodeMetadataKeys,
} from "../ensnode-metadata";
apps/ensindexer/src/lib/ensdb-writer-worker/ensdb-writer-worker.ts:67
- The
run()docstring still describes the old responsibilities (upserting ENSDb version + public config + indexing status snapshot). After this refactor,run()only starts the recurring upsert of indexing metadata context, so the doc/throws description should be updated to match current behavior.
/**
* Run the ENSDb Writer Worker
*
* The worker performs the following tasks:
* 1) A single attempt to upsert ENSDb version into ENSDb.
* 2) A single attempt to upsert serialized representation of
* {@link EnsIndexerPublicConfig} into ENSDb.
* 3) A recurring attempt to upsert serialized representation of
* {@link CrossChainIndexingStatusSnapshot} into ENSDb.
*
* @throws Error if the worker is already running, or
* if the in-memory ENSIndexer Public Config could not be fetched, or
* if the in-memory ENSIndexer Public Config is incompatible with the stored config in ENSDb.
*/
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| logger.info({ | ||
| msg: `Indexing Metadata Context is "uninitialized"`, | ||
| }); | ||
|
|
||
| // Invariant: indexing status must be "unstarted" when the indexing metadata context is uninitialized, | ||
| // since we haven't started processing any onchain events yet | ||
| if (inMemoryIndexingStatusSnapshot.omnichainStatus !== OmnichainIndexingStatusIds.Unstarted) { | ||
| throw new Error( | ||
| `Omnichain indexing status must be "unstarted" for "uninitialized" Indexing Metadata Context. Provided omnichain indexing status "${inMemoryIndexingStatusSnapshot.omnichainStatus}".`, | ||
| ); |
There was a problem hiding this comment.
The strict invariant here (Uninitialized => omnichainStatus must be Unstarted) can break upgrades: existing deployments may have no indexing_metadata_context row yet (so it deserializes as Uninitialized) while Ponder checkpoints indicate indexing already started (e.g. Following/Backfill). In that case this will throw during startup and prevent the indexer from running. Consider treating this as a recoverable state (log a warning and proceed to initialize/upsert the context from the current in-memory snapshot), or gate the invariant behind an explicit “fresh install” signal.
| logger.info({ | |
| msg: `Indexing Metadata Context is "uninitialized"`, | |
| }); | |
| // Invariant: indexing status must be "unstarted" when the indexing metadata context is uninitialized, | |
| // since we haven't started processing any onchain events yet | |
| if (inMemoryIndexingStatusSnapshot.omnichainStatus !== OmnichainIndexingStatusIds.Unstarted) { | |
| throw new Error( | |
| `Omnichain indexing status must be "unstarted" for "uninitialized" Indexing Metadata Context. Provided omnichain indexing status "${inMemoryIndexingStatusSnapshot.omnichainStatus}".`, | |
| ); | |
| if (inMemoryIndexingStatusSnapshot.omnichainStatus === OmnichainIndexingStatusIds.Unstarted) { | |
| logger.info({ | |
| msg: `Indexing Metadata Context is "uninitialized"`, | |
| }); | |
| } else { | |
| logger.warn({ | |
| msg: `Indexing Metadata Context is "uninitialized" but omnichain indexing has already started; proceeding with initialization so the stored context can be reconciled from the current in-memory indexing snapshot.`, | |
| omnichainStatus: inMemoryIndexingStatusSnapshot.omnichainStatus, | |
| }); |
| }); | ||
|
|
||
| // Before starting to process onchain events, we want to make sure that | ||
| // ENSRainbow is ready and ready to serve the "heal" requests. |
There was a problem hiding this comment.
This comment has a duplicated word: “ENSRainbow is ready and ready…”. It should be corrected to avoid confusion in docs.
| // ENSRainbow is ready and ready to serve the "heal" requests. | |
| // ENSRainbow is ready to serve the "heal" requests. |
| type CrossChainIndexingStatusSnapshot, | ||
| type EnsIndexerPublicConfig, |
There was a problem hiding this comment.
CrossChainIndexingStatusSnapshot and EnsIndexerPublicConfig are imported but not referenced in type positions (only in JSDoc), which is likely to be flagged as unused by the linter/CI. Either remove these imports or update the implementation/types so they are actually referenced.
| type CrossChainIndexingStatusSnapshot, | |
| type EnsIndexerPublicConfig, |
|
|
||
| if (indexingMetadataContext.statusCode === IndexingMetadataContextStatusCodes.Uninitialized) { | ||
| throw new Error( | ||
| `Cannot upsert Indexing Status Snapshot into ENSDb because Indexing Metadata Context should be be initialized first`, |
There was a problem hiding this comment.
This error message has a typo (“should be be”) and still mentions “Indexing Status Snapshot” even though this worker now upserts indexing metadata context. Updating it will make logs clearer when the interval hits this path.
| `Cannot upsert Indexing Status Snapshot into ENSDb because Indexing Metadata Context should be be initialized first`, | |
| `Cannot upsert indexing metadata context into ENSDb because indexing metadata context should be initialized first`, |
| // We can also reset the promises for indexing setup and onchain events to free up memory, | ||
| // since they will never be used again after the preconditions have been fully executed. |
There was a problem hiding this comment.
This comment refers to “promises for indexing setup and onchain events”, but the setup promise was removed in this refactor. Updating the comment to match the current state will avoid confusion when revisiting this code.
| // We can also reset the promises for indexing setup and onchain events to free up memory, | |
| // since they will never be used again after the preconditions have been fully executed. | |
| // We can also reset the promise for indexing onchain events to free up memory, | |
| // since it will never be used again after the preconditions have been fully executed. |
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
apps/ensindexer/src/lib/indexing-engines/ponder.test.ts (1)
305-348: 🧹 Nitpick | 🔵 TrivialConcurrent idempotency test — minor: assert handler invocation count after settle.
The test correctly exercises the in-flight de-duplication path (
indexingOnchainEventsPromise !== null) andvi.waitForis the right primitive given the dynamic import settles asynchronously. Consider also assertingexpect(mockInitIndexingOnchainEvents).toHaveBeenCalledTimes(1)afterPromise.all([promise1, promise2])resolves on Line 343 to guard against a future regression where a second invocation could be triggered post-resolution by the cleanup logic ineventHandlerPreconditions.🧪 Proposed extra assertion
// Wait for both handlers to complete await Promise.all([promise1, promise2]); // Both handlers should have executed after resolution expect(handler1).toHaveBeenCalledTimes(1); expect(handler2).toHaveBeenCalledTimes(1); + // Init must remain a singleton even after both promises resolve + expect(mockInitIndexingOnchainEvents).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 305 - 348, Add an assertion after awaiting Promise.all([promise1, promise2]) to re-check that mockInitIndexingOnchainEvents was only called once; specifically, after the handlers complete in the test "calls initIndexingOnchainEvents only once..." add expect(mockInitIndexingOnchainEvents).toHaveBeenCalledTimes(1) to guard against any late second invocation triggered by eventHandlerPreconditions or cleanup logic involved with getRegisteredCallback/addOnchainEventListener.apps/ensindexer/src/lib/indexing-engines/ponder.ts (1)
130-189: 🧹 Nitpick | 🔵 TrivialPrecondition gate correctly addresses prior ensv2 migration gap.
Moving the migration/init logic into
initIndexingOnchainEventsand gating it on the firstOnchainEvent(rather than:setup) resolves the previously-flagged bug where ensv2-only deployments would skip migrations because they register no:setuphandlers. The Ponder lifecycle guarantees all:setupevents fire before any onchain event, so this correctly runs migrations exactly once before any onchain handler executes, regardless of plugin composition.A couple of minor observations on the rejection path (worth confirming, no change required if intentional):
- If
initIndexingOnchainEvents()rejects,indexingOnchainEventsPromisestays as a permanently-rejected promise andeventHandlerPreconditionsFullyExecutedis never flipped. Every subsequent onchain event will re-await the same rejected promise and re-throw. This is consistent with theprocess.exitCode = 1fail-fast semantics insideinitIndexingOnchainEventsand Ponder's globalunhandledRejectionshutdown, so it's effectively a one-shot fail. Mention if this should be made explicit in the comment on Lines 140-143.- The cleanup
indexingOnchainEventsPromise = nullon Line 150 runs on every call after the first successful init. It's a no-op in steady state but slightly wasteful on the hot path; consider moving the nulling into the.then()callback that flips the flag so the early-return short-circuit becomes a single boolean check.♻️ Optional refactor to consolidate one-time cleanup
async function eventHandlerPreconditions(eventType: EventTypeId): Promise<void> { if (eventHandlerPreconditionsFullyExecuted) { - // Preconditions have already been fully executed, so we can skip executing them again. - // We can also reset the promises for indexing setup and onchain events to free up memory, - // since they will never be used again after the preconditions have been fully executed. - indexingOnchainEventsPromise = null; + // Preconditions have already been fully executed, so we can skip executing them again. return; } switch (eventType) { case EventTypeIds.Setup: { return; } case EventTypeIds.OnchainEvent: { if (indexingOnchainEventsPromise === null) { indexingOnchainEventsPromise = import("./init-indexing-onchain-events") .then(({ initIndexingOnchainEvents }) => initIndexingOnchainEvents()) .then(() => { eventHandlerPreconditionsFullyExecuted = true; + // Drop the reference to free memory; subsequent calls short-circuit + // via eventHandlerPreconditionsFullyExecuted. + indexingOnchainEventsPromise = null; }); } return await indexingOnchainEventsPromise; } } }🤖 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.ts` around lines 130 - 189, The current eventHandlerPreconditions logic leaves indexingOnchainEventsPromise nulled on every subsequent call after eventHandlerPreconditionsFullyExecuted is true and also leaves a permanently-rejected promise if initIndexingOnchainEvents() rejects; fix by moving the cleanup indexingOnchainEventsPromise = null into the .then() callback that sets eventHandlerPreconditionsFullyExecuted so the fast-path becomes a single boolean check, and update the comment near eventHandlerPreconditions / initIndexingOnchainEvents to explicitly note that a rejection will leave a permanently-rejected promise (intentional fail-fast) if you want that behavior; key symbols: eventHandlerPreconditions, eventHandlerPreconditionsFullyExecuted, indexingOnchainEventsPromise, initIndexingOnchainEvents.
🤖 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.mock.ts`:
- Around line 38-43: The mock baseEnsDbWriter currently resolves
getIndexingMetadataContext to undefined which violates the
Promise<IndexingMetadataContext> contract and causes TypeErrors; update
baseEnsDbWriter so getIndexingMetadataContext.mockResolvedValue returns a
minimal, uninitialized IndexingMetadataContext object (include required fields
such as statusCode and any other non-optional properties) so tests that forget
to override fail with a meaningful assertion, and keep
upsertIndexingMetadataContext.mockResolvedValue consistent (e.g., resolve to the
same minimal context or a sensible default).
- Around line 45-54: The factory createMockIndexingMetadataContextInitialized
currently returns the broad IndexingMetadataContext union but always constructs
the Initialized variant; change its return type to the discriminated variant
IndexingMetadataContextInitialized so callers no longer need to cast to access
stackInfo, and likewise consider narrowing
createMockIndexingMetadataContextUninitialized to
IndexingMetadataContextUninitialized for symmetry; update the function
signature(s) and any import/type references to use
IndexingMetadataContextInitialized (and IndexingMetadataContextUninitialized)
while keeping the same object shape and overrides parameter.
In `@apps/ensindexer/src/lib/ensdb-writer-worker/ensdb-writer-worker.test.ts`:
- Around line 5-7: The helper createMockIndexingMetadataContextInitialized
currently has a too-broad return type (IndexingMetadataContext) causing callers
to repeatedly cast to IndexingMetadataContextInitialized; change its declared
return type to IndexingMetadataContextInitialized and update its signature so
TypeScript infers the initialized variant directly, then remove the redundant
casts at the callsites that access stackInfo (uses of
createMockIndexingMetadataContextInitialized,
IndexingMetadataContextInitialized, and IndexingMetadataContext).
- Around line 195-199: The test currently calls
vi.mocked(buildIndexingMetadataContextInitialized).mockReturnValueOnce(indexingMetadataContext)
three times which redundantly queues the same value; replace the three
mockReturnValueOnce calls with a single mockReturnValue(indexingMetadataContext
as IndexingMetadataContextInitialized) (or set the mock once at the top of the
test) so buildIndexingMetadataContextInitialized always returns the same
indexingMetadataContext and avoids brittle per-call sequencing.
In `@apps/ensindexer/src/lib/indexing-engines/init-indexing-onchain-events.ts`:
- Around line 76-86: The calls to migrateEnsNodeSchema() and
waitForEnsRainbowToBeHealthy() run before the try/catch so their rejections
bypass the centralized error logger and exit handling; move both calls
(migrateEnsNodeSchema and waitForEnsRainbowToBeHealthy) inside the existing try
block that surrounds the rest of initIndexingOnchainEvents so any thrown errors
are caught by the catch which logs with logger.error({ msg: "Failed to
initialize the onchain events indexing", module: "init-indexing-onchain-events",
error }) and sets process.exitCode = 1, preserving consistent failure
observability and exit semantics.
In `@apps/ensindexer/src/lib/indexing-engines/ponder.test.ts`:
- Around line 13-27: Replace the loose vi.hoisted side-effect with a single
typed helper factory: create one vi.hoisted that sets globalThis.PONDER_COMMON
(use a narrow shape cast via unknown or reference an existing PONDER_COMMON type
instead of (globalThis as any)), and return/mock the related helpers
(mockPonderOn and mockInitIndexingOnchainEvents) from the same factory so test
bootstrap is colocated; locate the hoisted block using the symbol vi.hoisted and
the global name PONDER_COMMON and update tests to consume the returned mocks
from that single hoisted factory.
---
Outside diff comments:
In `@apps/ensindexer/src/lib/indexing-engines/ponder.test.ts`:
- Around line 305-348: Add an assertion after awaiting Promise.all([promise1,
promise2]) to re-check that mockInitIndexingOnchainEvents was only called once;
specifically, after the handlers complete in the test "calls
initIndexingOnchainEvents only once..." add
expect(mockInitIndexingOnchainEvents).toHaveBeenCalledTimes(1) to guard against
any late second invocation triggered by eventHandlerPreconditions or cleanup
logic involved with getRegisteredCallback/addOnchainEventListener.
In `@apps/ensindexer/src/lib/indexing-engines/ponder.ts`:
- Around line 130-189: The current eventHandlerPreconditions logic leaves
indexingOnchainEventsPromise nulled on every subsequent call after
eventHandlerPreconditionsFullyExecuted is true and also leaves a
permanently-rejected promise if initIndexingOnchainEvents() rejects; fix by
moving the cleanup indexingOnchainEventsPromise = null into the .then() callback
that sets eventHandlerPreconditionsFullyExecuted so the fast-path becomes a
single boolean check, and update the comment near eventHandlerPreconditions /
initIndexingOnchainEvents to explicitly note that a rejection will leave a
permanently-rejected promise (intentional fail-fast) if you want that behavior;
key symbols: eventHandlerPreconditions, eventHandlerPreconditionsFullyExecuted,
indexingOnchainEventsPromise, initIndexingOnchainEvents.
🪄 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: 9ad105fa-1fa5-48fd-8fd9-bd99fe0a553a
📒 Files selected for processing (5)
apps/ensindexer/src/lib/ensdb-writer-worker/ensdb-writer-worker.mock.tsapps/ensindexer/src/lib/ensdb-writer-worker/ensdb-writer-worker.test.tsapps/ensindexer/src/lib/indexing-engines/init-indexing-onchain-events.tsapps/ensindexer/src/lib/indexing-engines/ponder.test.tsapps/ensindexer/src/lib/indexing-engines/ponder.ts
| export function baseEnsDbWriter() { | ||
| return { | ||
| getEnsDbVersion: vi.fn().mockResolvedValue(undefined), | ||
| getEnsIndexerPublicConfig: vi.fn().mockResolvedValue(undefined), | ||
| getIndexingStatusSnapshot: vi.fn().mockResolvedValue(undefined), | ||
| upsertEnsDbVersion: vi.fn().mockResolvedValue(undefined), | ||
| upsertEnsIndexerPublicConfig: vi.fn().mockResolvedValue(undefined), | ||
| upsertIndexingStatusSnapshot: vi.fn().mockResolvedValue(undefined), | ||
| getIndexingMetadataContext: vi.fn().mockResolvedValue(undefined), | ||
| upsertIndexingMetadataContext: vi.fn().mockResolvedValue(undefined), | ||
| }; | ||
| } |
There was a problem hiding this comment.
baseEnsDbWriter defaults violate the typed contract.
getIndexingMetadataContext is typed to return Promise<IndexingMetadataContext>, but the default mock resolves with undefined. The worker (and any test that forgets to override) reads .statusCode on the resolved value and will throw TypeError: Cannot read properties of undefined (reading 'statusCode'). Default to an uninitialized context so tests fail with a meaningful assertion error rather than a TypeError.
🛠 Proposed default
export function baseEnsDbWriter() {
return {
- getIndexingMetadataContext: vi.fn().mockResolvedValue(undefined),
+ getIndexingMetadataContext: vi
+ .fn()
+ .mockResolvedValue(createMockIndexingMetadataContextUninitialized()),
upsertIndexingMetadataContext: vi.fn().mockResolvedValue(undefined),
};
}🤖 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.mock.ts`
around lines 38 - 43, The mock baseEnsDbWriter currently resolves
getIndexingMetadataContext to undefined which violates the
Promise<IndexingMetadataContext> contract and causes TypeErrors; update
baseEnsDbWriter so getIndexingMetadataContext.mockResolvedValue returns a
minimal, uninitialized IndexingMetadataContext object (include required fields
such as statusCode and any other non-optional properties) so tests that forget
to override fail with a meaningful assertion, and keep
upsertIndexingMetadataContext.mockResolvedValue consistent (e.g., resolve to the
same minimal context or a sensible default).
| export function createMockIndexingMetadataContextInitialized( | ||
| overrides: Partial<IndexingMetadataContext> = {}, | ||
| ): IndexingMetadataContext { | ||
| return { | ||
| getPublicConfig: vi.fn().mockResolvedValue(resolvedConfig), | ||
| } as unknown as PublicConfigBuilder; | ||
| statusCode: IndexingMetadataContextStatusCodes.Initialized, | ||
| indexingStatus: createMockCrossChainSnapshot(), | ||
| stackInfo: mockStackInfo, | ||
| ...overrides, | ||
| }; | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Narrow the return type to the discriminated Initialized variant.
createMockIndexingMetadataContextInitialized constructs the Initialized variant unconditionally but is typed as the broader IndexingMetadataContext union. Consumers across ensdb-writer-worker.test.ts repeatedly cast with as IndexingMetadataContextInitialized (Lines 52, 75, 80, 197–199, 271, 289, 292) just to access stackInfo. Fixing the return type here removes those casts at the root.
♻️ Proposed signature
+import type { IndexingMetadataContextInitialized } from "@ensnode/ensnode-sdk";
+
-export function createMockIndexingMetadataContextInitialized(
- overrides: Partial<IndexingMetadataContext> = {},
-): IndexingMetadataContext {
+export function createMockIndexingMetadataContextInitialized(
+ overrides: Partial<IndexingMetadataContextInitialized> = {},
+): IndexingMetadataContextInitialized {
return {
statusCode: IndexingMetadataContextStatusCodes.Initialized,
indexingStatus: createMockCrossChainSnapshot(),
stackInfo: mockStackInfo,
...overrides,
};
}Also consider similarly narrowing createMockIndexingMetadataContextUninitialized to IndexingMetadataContextUninitialized for symmetry.
🤖 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.mock.ts`
around lines 45 - 54, The factory createMockIndexingMetadataContextInitialized
currently returns the broad IndexingMetadataContext union but always constructs
the Initialized variant; change its return type to the discriminated variant
IndexingMetadataContextInitialized so callers no longer need to cast to access
stackInfo, and likewise consider narrowing
createMockIndexingMetadataContextUninitialized to
IndexingMetadataContextUninitialized for symmetry; update the function
signature(s) and any import/type references to use
IndexingMetadataContextInitialized (and IndexingMetadataContextUninitialized)
while keeping the same object shape and overrides parameter.
| buildIndexingMetadataContextInitialized, | ||
| type IndexingMetadataContextInitialized, | ||
| } from "@ensnode/ensnode-sdk"; |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Tighten the helper return type to drop the repeated as IndexingMetadataContextInitialized casts.
createMockIndexingMetadataContextInitialized is declared as returning the union IndexingMetadataContext, which forces every consumer to re-narrow with as IndexingMetadataContextInitialized to access stackInfo (Lines 77, 197–199, 289). Since the function literally constructs the initialized variant, returning IndexingMetadataContextInitialized would let TypeScript verify these usages and remove ~6 casts here.
♻️ Proposed signature change in the mock module
-export function createMockIndexingMetadataContextInitialized(
- overrides: Partial<IndexingMetadataContext> = {},
-): IndexingMetadataContext {
+export function createMockIndexingMetadataContextInitialized(
+ overrides: Partial<IndexingMetadataContextInitialized> = {},
+): IndexingMetadataContextInitialized {
return {
statusCode: IndexingMetadataContextStatusCodes.Initialized,
indexingStatus: createMockCrossChainSnapshot(),
stackInfo: mockStackInfo,
...overrides,
};
}Also applies to: 46-53, 75-80, 195-199, 267-292
🤖 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.test.ts`
around lines 5 - 7, The helper createMockIndexingMetadataContextInitialized
currently has a too-broad return type (IndexingMetadataContext) causing callers
to repeatedly cast to IndexingMetadataContextInitialized; change its declared
return type to IndexingMetadataContextInitialized and update its signature so
TypeScript infers the initialized variant directly, then remove the redundant
casts at the callsites that access stackInfo (uses of
createMockIndexingMetadataContextInitialized,
IndexingMetadataContextInitialized, and IndexingMetadataContext).
| const indexingMetadataContext = createMockIndexingMetadataContextInitialized(); | ||
| vi.mocked(buildIndexingMetadataContextInitialized) | ||
| .mockReturnValueOnce(indexingMetadataContext as IndexingMetadataContextInitialized) | ||
| .mockReturnValueOnce(indexingMetadataContext as IndexingMetadataContextInitialized) | ||
| .mockReturnValueOnce(indexingMetadataContext as IndexingMetadataContextInitialized); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Three identical mockReturnValueOnce calls — use mockReturnValue.
The same indexingMetadataContext is queued three times. mockReturnValue (or set once at the top of the test) would convey intent more directly and avoid drift if a fourth tick is ever added.
♻️ Proposed simplification
- const indexingMetadataContext = createMockIndexingMetadataContextInitialized();
- vi.mocked(buildIndexingMetadataContextInitialized)
- .mockReturnValueOnce(indexingMetadataContext as IndexingMetadataContextInitialized)
- .mockReturnValueOnce(indexingMetadataContext as IndexingMetadataContextInitialized)
- .mockReturnValueOnce(indexingMetadataContext as IndexingMetadataContextInitialized);
+ const indexingMetadataContext = createMockIndexingMetadataContextInitialized();
+ vi.mocked(buildIndexingMetadataContextInitialized).mockReturnValue(indexingMetadataContext);📝 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 indexingMetadataContext = createMockIndexingMetadataContextInitialized(); | |
| vi.mocked(buildIndexingMetadataContextInitialized) | |
| .mockReturnValueOnce(indexingMetadataContext as IndexingMetadataContextInitialized) | |
| .mockReturnValueOnce(indexingMetadataContext as IndexingMetadataContextInitialized) | |
| .mockReturnValueOnce(indexingMetadataContext as IndexingMetadataContextInitialized); | |
| const indexingMetadataContext = createMockIndexingMetadataContextInitialized(); | |
| vi.mocked(buildIndexingMetadataContextInitialized).mockReturnValue(indexingMetadataContext); |
🤖 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.test.ts`
around lines 195 - 199, The test currently calls
vi.mocked(buildIndexingMetadataContextInitialized).mockReturnValueOnce(indexingMetadataContext)
three times which redundantly queues the same value; replace the three
mockReturnValueOnce calls with a single mockReturnValue(indexingMetadataContext
as IndexingMetadataContextInitialized) (or set the mock once at the top of the
test) so buildIndexingMetadataContextInitialized always returns the same
indexingMetadataContext and avoids brittle per-call sequencing.
| export async function initIndexingOnchainEvents(): Promise<void> { | ||
| // TODO: wait for ENSDb instance to be healthy | ||
| // Ensure the ENSNode Schema in ENSDb is up to date by running any pending migrations. | ||
| await migrateEnsNodeSchema(); | ||
|
|
||
| // Before calling `ensRainbowClient.config()`, we want to make sure that | ||
| // the ENSRainbow instance is healthy and ready to serve requests. | ||
| // This is a quick check, as we expect the ENSRainbow instance to be healthy | ||
| // by the time ENSIndexer instance executes `initIndexingOnchainEvents`. | ||
| await waitForEnsRainbowToBeHealthy(); | ||
|
|
There was a problem hiding this comment.
Migrate/health calls escape the centralized error-logging path.
migrateEnsNodeSchema() (Line 79) and waitForEnsRainbowToBeHealthy() (Line 85) are called before the try/catch that begins on Line 87. If either rejects, the structured logger.error({ msg: "Failed to initialize the onchain events indexing", module: "init-indexing-onchain-events", error }) (Lines 185–189) does not run, and process.exitCode = 1 is not set explicitly here (relying solely on Ponder's unhandled-rejection handler).
Move both calls inside the try block for consistent failure observability and to keep the process.exitCode = 1 semantics aligned across all init failures.
🛡️ Proposed fix
export async function initIndexingOnchainEvents(): Promise<void> {
- // TODO: wait for ENSDb instance to be healthy
- // Ensure the ENSNode Schema in ENSDb is up to date by running any pending migrations.
- await migrateEnsNodeSchema();
-
- // Before calling `ensRainbowClient.config()`, we want to make sure that
- // the ENSRainbow instance is healthy and ready to serve requests.
- // This is a quick check, as we expect the ENSRainbow instance to be healthy
- // by the time ENSIndexer instance executes `initIndexingOnchainEvents`.
- await waitForEnsRainbowToBeHealthy();
-
try {
+ // TODO: wait for ENSDb instance to be healthy
+ // Ensure the ENSNode Schema in ENSDb is up to date by running any pending migrations.
+ await migrateEnsNodeSchema();
+
+ // Before calling `ensRainbowClient.config()`, we want to make sure that
+ // the ENSRainbow instance is healthy and ready to serve requests.
+ await waitForEnsRainbowToBeHealthy();
+
const [
inMemoryIndexingStatusSnapshot,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/ensindexer/src/lib/indexing-engines/init-indexing-onchain-events.ts`
around lines 76 - 86, The calls to migrateEnsNodeSchema() and
waitForEnsRainbowToBeHealthy() run before the try/catch so their rejections
bypass the centralized error logger and exit handling; move both calls
(migrateEnsNodeSchema and waitForEnsRainbowToBeHealthy) inside the existing try
block that surrounds the rest of initIndexingOnchainEvents so any thrown errors
are caught by the catch which logs with logger.error({ msg: "Failed to
initialize the onchain events indexing", module: "init-indexing-onchain-events",
error }) and sets process.exitCode = 1, preserving consistent failure
observability and exit semantics.
| vi.hoisted(() => { | ||
| (globalThis as any).PONDER_COMMON = { | ||
| options: { | ||
| command: "start", | ||
| port: 42069, | ||
| }, | ||
| logger: { | ||
| trace: vi.fn(), | ||
| debug: vi.fn(), | ||
| info: vi.fn(), | ||
| warn: vi.fn(), | ||
| error: vi.fn(), | ||
| }, | ||
| }; | ||
| }); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
vi.hoisted side-effect setup is correct here; consider extracting a typed helper.
Using vi.hoisted purely for the side effect of populating globalThis.PONDER_COMMON is the right pattern because the SUT is imported lazily inside each test (after vi.resetModules()), and the global must be present before any module that reads it executes. Two small ergonomic improvements:
- The
(globalThis as any)cast bypasses any future typing ofPONDER_COMMON. If a typed declaration exists in the codebase, prefer it; otherwise anunknowncast through a narrow shape is safer thanany. - This block, the
mockPonderOn, andmockInitIndexingOnchainEventshoisted values can be merged into a singlevi.hoistedfactory to keep test bootstrap colocated.
🤖 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 13 -
27, Replace the loose vi.hoisted side-effect with a single typed helper factory:
create one vi.hoisted that sets globalThis.PONDER_COMMON (use a narrow shape
cast via unknown or reference an existing PONDER_COMMON type instead of
(globalThis as any)), and return/mock the related helpers (mockPonderOn and
mockInitIndexingOnchainEvents) from the same factory so test bootstrap is
colocated; locate the hoisted block using the symbol vi.hoisted and the global
name PONDER_COMMON and update tests to consume the returned mocks from that
single hoisted factory.
1daecef to
c64d500
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 26 out of 26 changed files in this pull request and generated 6 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| vi.mock("@/lib/ensrainbow/singleton", () => ({ | ||
| waitForEnsRainbowToBeReady: mockWaitForEnsRainbow, | ||
| vi.mock("@/lib/indexing-engines/init-indexing-onchain-events", () => ({ |
There was a problem hiding this comment.
The module being mocked here doesn't match the specifier used by ponder.ts (import("./init-indexing-onchain-events")). As written, this mock won't apply and the test will likely execute the real initialization code. Update the mock path to match the actual import specifier (or change ponder.ts to import via the same alias) so tests are reliable.
| vi.mock("@/lib/indexing-engines/init-indexing-onchain-events", () => ({ | |
| vi.mock("./init-indexing-onchain-events", () => ({ |
| import type { | ||
| SerializedCrossChainIndexingStatusSnapshot, | ||
| SerializedEnsIndexerPublicConfig, | ||
| SerializedIndexingMetadataContextInitialized, | ||
| } from "@ensnode/ensnode-sdk"; | ||
|
|
||
| import type { | ||
| EnsNodeMetadata, | ||
| EnsNodeMetadataEnsDbVersion, | ||
| EnsNodeMetadataEnsIndexerIndexingStatus, | ||
| EnsNodeMetadataEnsIndexerPublicConfig, | ||
| EnsNodeMetadataKeys, | ||
| } from "../ensnode-metadata"; | ||
|
|
||
| /** | ||
| * Serialized representation of {@link EnsNodeMetadataEnsDbVersion}. | ||
| */ | ||
| export type SerializedEnsNodeMetadataEnsDbVersion = EnsNodeMetadataEnsDbVersion; | ||
|
|
||
| /** | ||
| * Serialized representation of {@link EnsNodeMetadataEnsIndexerPublicConfig}. | ||
| */ | ||
| export interface SerializedEnsNodeMetadataEnsIndexerPublicConfig { | ||
| key: typeof EnsNodeMetadataKeys.EnsIndexerPublicConfig; | ||
| value: SerializedEnsIndexerPublicConfig; | ||
| } | ||
| import type { EnsNodeMetadata, EnsNodeMetadataKeys } from "../ensnode-metadata"; |
There was a problem hiding this comment.
Unused imports here (e.g. SerializedCrossChainIndexingStatusSnapshot, SerializedEnsIndexerPublicConfig, and EnsNodeMetadata) will likely fail lint/CI (Biome organizes imports + flags unused). Remove the unused type imports to keep this file clean and avoid build failures.
| type CrossChainIndexingStatusSnapshot, | ||
| type EnsIndexerPublicConfig, | ||
| type IndexingMetadataContextInitialized, | ||
| serializeCrossChainIndexingStatusSnapshot, | ||
| serializeEnsIndexerPublicConfig, |
There was a problem hiding this comment.
CrossChainIndexingStatusSnapshot and EnsIndexerPublicConfig are imported but not used in this file anymore. These unused imports are likely to fail lint/CI; please remove them.
| type CrossChainIndexingStatusSnapshot, | |
| type EnsIndexerPublicConfig, | |
| type IndexingMetadataContextInitialized, | |
| serializeCrossChainIndexingStatusSnapshot, | |
| serializeEnsIndexerPublicConfig, | |
| type IndexingMetadataContextInitialized, |
| serializeCrossChainIndexingStatusSnapshot, | ||
| serializeEnsIndexerPublicConfig, |
There was a problem hiding this comment.
serializeCrossChainIndexingStatusSnapshot and serializeEnsIndexerPublicConfig are now unused after removing the old upsert methods. Please drop these unused imports to avoid lint/CI failures.
| serializeCrossChainIndexingStatusSnapshot, | |
| serializeEnsIndexerPublicConfig, |
| buildCrossChainIndexingStatusSnapshotOmnichain, | ||
| buildIndexingMetadataContextInitialized, | ||
| type CrossChainIndexingStatusSnapshot, | ||
| type EnsIndexerPublicConfig, | ||
| OmnichainIndexingStatusIds, | ||
| type OmnichainIndexingStatusSnapshot, | ||
| validateEnsIndexerPublicConfigCompatibility, | ||
| type IndexingMetadataContext, | ||
| IndexingMetadataContextStatusCodes, | ||
| } from "@ensnode/ensnode-sdk"; |
There was a problem hiding this comment.
The imports include several unused types (CrossChainIndexingStatusSnapshot, EnsIndexerPublicConfig, IndexingMetadataContext). Unused imports are likely to fail lint/CI; remove them or reference them in actual type positions (not only JSDoc).
| * Upsert the current Indexing Status Snapshot into ENSDb. | ||
| * | ||
| * This method is called by the scheduler at regular intervals. |
There was a problem hiding this comment.
The JSDoc for this method still says it upserts an "Indexing Status Snapshot", but the method now upserts the full indexing metadata context. Please update the comment to reflect what is actually written to ENSDb.
| * Upsert the current Indexing Status Snapshot into ENSDb. | |
| * | |
| * This method is called by the scheduler at regular intervals. | |
| * Upsert the current Indexing Metadata Context into ENSDb. | |
| * | |
| * This method is called by the scheduler at regular intervals to refresh | |
| * the indexing metadata context written to ENSDb. |
Drop all unused methods
tk-o
force-pushed
the
feat/indexing-metadata-context
branch
from
c64d500 to
4684fb4
Compare
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 (4)
packages/integration-test-env/src/orchestrator.ts (1)
205-217: 🧹 Nitpick | 🔵 TrivialAdd a log line for the non-Initialized branch to preserve poll observability.
Previously, every poll iteration emitted
Omnichain status: …, giving a heartbeat in CI logs. With the new gate, whenindexingMetadataContext.statusCode !== Initialized(e.g., the indexer is still bootstrapping), nothing is logged for up to the entiretimeoutMswindow, which makes diagnosing a stuck pre-init state harder. Consider logging the currentstatusCodeso the poll cadence remains visible.♻️ Suggested tweak
if (indexingMetadataContext.statusCode === IndexingMetadataContextStatusCodes.Initialized) { const { omnichainStatus } = indexingMetadataContext.indexingStatus.omnichainSnapshot; log(`Omnichain status: ${omnichainStatus}`); if ( omnichainStatus === OmnichainIndexingStatusIds.Following || omnichainStatus === OmnichainIndexingStatusIds.Completed ) { log("Indexing reached target status"); return; } + } else { + log(`Indexing metadata context status: ${indexingMetadataContext.statusCode}`); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/integration-test-env/src/orchestrator.ts` around lines 205 - 217, Add a log when indexingMetadataContext.statusCode is not Initialized so each poll emits a heartbeat; after calling ensDbClient.getIndexingMetadataContext() and before returning, log the statusCode (and optionally indexingMetadataContext.indexingStatus) when statusCode !== IndexingMetadataContextStatusCodes.Initialized to preserve observability of the poll cadence in the getIndexingMetadataContext / indexingMetadataContext branch where omnichainStatus is not yet available.apps/ensapi/src/cache/indexing-status.cache.ts (1)
41-53:⚠️ Potential issue | 🟡 MinorCatch handler assumes the failure reason; misleading for transport/DB errors.
The
.catchis reached not only whenIndexing Metadata Context was uninitialized(the explicitthrowabove) but also whengetIndexingMetadataContext()itself rejects (network/DB failures, deserialization errors, etc.). The inline comment on Line 42 and the log header both hard-code "uninitialized" as the reason, which will misdirect operators when the real failure is, e.g., an ENSDb connection error. Consider a neutral message and let the loggederrorcarry the specific reason.📝 Suggested wording
.catch((error) => { - // Indexing Metadata Context was uninitialized in ENSDb. - // Therefore, throw an error so that this current invocation of `readCache` will: + // Failed to load (or validate) the Indexing Metadata Context from ENSDb. + // Therefore, throw an error so that this current invocation of `readCache` will: // - Reject the newly fetched response (if any) such that it won't be cached. // - Return the most recently cached value from prior invocations, or `null` if no prior invocation successfully cached a value. logger.error( error, - `Error occurred while loading Indexing Metadata Context record from ENSNode Metadata table in ENSDb. ` + + `Failed to load Indexing Metadata Context record from ENSNode Metadata table in ENSDb. ` + `Where clause applied: ("ensIndexerSchemaName" = "${ensDbClient.ensIndexerSchemaName}", "key" = "${EnsNodeMetadataKeys.IndexingMetadataContext}"). ` + `The cached indexing status snapshot (if any) will not be updated.`, );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ensapi/src/cache/indexing-status.cache.ts` around lines 41 - 53, The catch handler for the promise returned by getIndexingMetadataContext() wrongly assumes the failure reason is "uninitialized"; update the comment and the logger.error call in the .catch((error) => { ... }) block so the log uses neutral wording (e.g., "Error loading Indexing Metadata Context from ENSDb") and includes the passed error object (do not hard-code "uninitialized"), and keep rethrowing the original error; reference getIndexingMetadataContext, EnsNodeMetadataKeys.IndexingMetadataContext, and the existing logger.error call to locate and update this catch block.apps/ensapi/src/config/config.schema.ts (1)
60-90:⚠️ Potential issue | 🟡 MinorStale log/JSDoc wording after the metadata model switch.
The fetch now retrieves the entire
IndexingMetadataContext, not just theEnsIndexerPublicConfig, but both the JSDoc onbuildConfigFromEnvironment("fetching the EnsIndexerPublicConfig") and theonFailedAttemptlog message ("ENSIndexer Public Config fetch attempt …") still describe the old shape. Update them so failure logs point operators at the right ENSDb record/key when triaging startup retries.📝 Suggested wording
-/** - * Builds the EnsApiConfig from an EnsApiEnvironment object, fetching the EnsIndexerPublicConfig. - * - * `@returns` A validated EnsApiConfig object - * `@throws` Error with formatted validation messages if environment parsing fails - */ +/** + * Builds the EnsApiConfig from an EnsApiEnvironment object, fetching the + * Indexing Metadata Context from ENSDb and deriving the EnsIndexerPublicConfig + * from its `stackInfo.ensIndexer`. + * + * `@throws` Error with formatted validation messages if environment parsing fails + */ @@ - logger.info( - `ENSIndexer Public Config fetch attempt ${attemptNumber} failed (${error.message}). ${retriesLeft} retries left.`, - ); + logger.info( + `Indexing Metadata Context fetch attempt ${attemptNumber} failed (${error.message}). ${retriesLeft} retries left.`, + );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ensapi/src/config/config.schema.ts` around lines 60 - 90, Update the stale JSDoc and log wording in buildConfigFromEnvironment to reflect that we now fetch the full IndexingMetadataContext (via ensDbClient.getIndexingMetadataContext) rather than an EnsIndexerPublicConfig: change the JSDoc summary/description to mention fetching the IndexingMetadataContext and include the relevant ENSDb record/key (IndexingMetadataContext) for operators, and update the onFailedAttempt logger message to say "IndexingMetadataContext fetch attempt X failed (…)" and optionally include a hint to check the ENSDb IndexingMetadataContext record/key and the IndexingMetadataContextStatusCodes for triage; keep references to ensDbClient.getIndexingMetadataContext and IndexingMetadataContextStatusCodes in the message.apps/ensindexer/ponder/src/api/handlers/ensnode-api.ts (1)
33-75: 🧹 Nitpick | 🔵 TrivialConsider 503 (Service Unavailable) instead of 500 for the uninitialized-context branch.
The comment explicitly notes that an uninitialized
IndexingMetadataContextis an expected transient state shortly after startup ("ENSDb has not yet been populated with the first snapshot"). Returning HTTP500conflates this expected readiness gap with a true server error and disrupts uptime metrics/alerting.503more accurately describes a not-yet-ready upstream and lets clients/load balancers retry sensibly. If both branches (transient uninitialized vs. unexpected throw) need to remain a single status today, at least split them so the readiness case maps to503.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ensindexer/ponder/src/api/handlers/ensnode-api.ts` around lines 33 - 75, The route handler for "/indexing-status" treats an uninitialized IndexingMetadataContext as an error and currently returns HTTP 500 for all failures; change it so the specific transient readiness case (when ensDbClient.getIndexingMetadataContext() returns a context whose statusCode !== IndexingMetadataContextStatusCodes.Initialized) responds with HTTP 503 and an EnsIndexerIndexingStatusResponse with responseCode EnsIndexerIndexingStatusResponseCodes.Error (or a new dedicated code if preferred), while keeping unexpected exceptions (caught in the catch block) returning HTTP 500; locate the logic around getIndexingMetadataContext, the Initialed check, and the catch/logger.error in the "/indexing-status" handler and adjust the response status codes 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/ensapi/src/config/config.schema.test.ts`:
- Around line 103-105: Replace the bare cast of
deserializeIndexingMetadataContext(INDEXING_METADATA_CONTEXT) to
IndexingMetadataContextInitialized with a runtime invariant check: call
deserializeIndexingMetadataContext(...) into a variable, verify its statusCode
equals IndexingMetadataContextStatusCodes.Initialized, and if not throw a clear
Error (mentioning INDEXING_METADATA_CONTEXT and the unexpected status) so the
test fails immediately; only after this check treat the value as an
IndexingMetadataContextInitialized before passing it to
buildConfigFromEnvironment or further assertions.
In `@packages/ensdb-sdk/src/client/ensdb-reader.test.ts`:
- Around line 123-148: The test currently computes expected by deserializing the
serialized context and then compares result to that deserialized value, which is
tautological; instead assert the reader's output equals the original in-memory
context you built. Change the assertion in the "returns the deserialized
initialized context when one record exists" test to compare result to the local
context variable (built via buildIndexingMetadataContextInitialized) rather than
to deserializeIndexingMetadataContext(serialized); keep the same setup using
serializeIndexingMetadataContext(serialized) and
createEnsDbReader().getIndexingMetadataContext() so the test becomes a true
serialize/deserialize roundtrip verification.
In `@packages/ensdb-sdk/src/client/ensnode-metadata.ts`:
- Around line 23-28: Update the stale JSDoc for the EnsNodeMetadata type: the
comment should no longer say "Union type gathering all variants" because
EnsNodeMetadata is currently a direct alias of
EnsNodeMetadataIndexingMetadataContext; either change the wording to indicate it
is a single-variant alias (kept for future extensibility) or otherwise describe
that it aliases EnsNodeMetadataIndexingMetadataContext, referencing the
EnsNodeMetadata and EnsNodeMetadataIndexingMetadataContext symbols so the
comment accurately reflects the current model.
---
Outside diff comments:
In `@apps/ensapi/src/cache/indexing-status.cache.ts`:
- Around line 41-53: The catch handler for the promise returned by
getIndexingMetadataContext() wrongly assumes the failure reason is
"uninitialized"; update the comment and the logger.error call in the
.catch((error) => { ... }) block so the log uses neutral wording (e.g., "Error
loading Indexing Metadata Context from ENSDb") and includes the passed error
object (do not hard-code "uninitialized"), and keep rethrowing the original
error; reference getIndexingMetadataContext,
EnsNodeMetadataKeys.IndexingMetadataContext, and the existing logger.error call
to locate and update this catch block.
In `@apps/ensapi/src/config/config.schema.ts`:
- Around line 60-90: Update the stale JSDoc and log wording in
buildConfigFromEnvironment to reflect that we now fetch the full
IndexingMetadataContext (via ensDbClient.getIndexingMetadataContext) rather than
an EnsIndexerPublicConfig: change the JSDoc summary/description to mention
fetching the IndexingMetadataContext and include the relevant ENSDb record/key
(IndexingMetadataContext) for operators, and update the onFailedAttempt logger
message to say "IndexingMetadataContext fetch attempt X failed (…)" and
optionally include a hint to check the ENSDb IndexingMetadataContext record/key
and the IndexingMetadataContextStatusCodes for triage; keep references to
ensDbClient.getIndexingMetadataContext and IndexingMetadataContextStatusCodes in
the message.
In `@apps/ensindexer/ponder/src/api/handlers/ensnode-api.ts`:
- Around line 33-75: The route handler for "/indexing-status" treats an
uninitialized IndexingMetadataContext as an error and currently returns HTTP 500
for all failures; change it so the specific transient readiness case (when
ensDbClient.getIndexingMetadataContext() returns a context whose statusCode !==
IndexingMetadataContextStatusCodes.Initialized) responds with HTTP 503 and an
EnsIndexerIndexingStatusResponse with responseCode
EnsIndexerIndexingStatusResponseCodes.Error (or a new dedicated code if
preferred), while keeping unexpected exceptions (caught in the catch block)
returning HTTP 500; locate the logic around getIndexingMetadataContext, the
Initialed check, and the catch/logger.error in the "/indexing-status" handler
and adjust the response status codes accordingly.
In `@packages/integration-test-env/src/orchestrator.ts`:
- Around line 205-217: Add a log when indexingMetadataContext.statusCode is not
Initialized so each poll emits a heartbeat; after calling
ensDbClient.getIndexingMetadataContext() and before returning, log the
statusCode (and optionally indexingMetadataContext.indexingStatus) when
statusCode !== IndexingMetadataContextStatusCodes.Initialized to preserve
observability of the poll cadence in the getIndexingMetadataContext /
indexingMetadataContext branch where omnichainStatus is not yet available.
🪄 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: ce4136c5-4748-4bf1-aaec-4d93a269738c
📒 Files selected for processing (12)
apps/ensapi/src/cache/indexing-status.cache.tsapps/ensapi/src/config/config.schema.test.tsapps/ensapi/src/config/config.schema.tsapps/ensindexer/ponder/src/api/handlers/ensnode-api.tsapps/ensindexer/src/lib/indexing-engines/ponder.test.tspackages/ensdb-sdk/src/client/ensdb-reader.test.tspackages/ensdb-sdk/src/client/ensdb-reader.tspackages/ensdb-sdk/src/client/ensdb-writer.test.tspackages/ensdb-sdk/src/client/ensdb-writer.tspackages/ensdb-sdk/src/client/ensnode-metadata.tspackages/ensdb-sdk/src/client/serialize/ensnode-metadata.tspackages/integration-test-env/src/orchestrator.ts
| const indexingMetadataContextInitialized = deserializeIndexingMetadataContext( | ||
| INDEXING_METADATA_CONTEXT, | ||
| ) as IndexingMetadataContextInitialized; |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Prefer a runtime assertion over the bare type cast.
deserializeIndexingMetadataContext(...) returns a union, and the as IndexingMetadataContextInitialized cast silently lies if the deserializer ever produces a non-initialized variant (e.g., due to a future shape change in the fixture). A small invariant check (e.g., if (deserialized.statusCode !== IndexingMetadataContextStatusCodes.Initialized) throw new Error(...)) makes the test fail loudly at the fixture rather than at an assertion deep inside buildConfigFromEnvironment.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/ensapi/src/config/config.schema.test.ts` around lines 103 - 105, Replace
the bare cast of deserializeIndexingMetadataContext(INDEXING_METADATA_CONTEXT)
to IndexingMetadataContextInitialized with a runtime invariant check: call
deserializeIndexingMetadataContext(...) into a variable, verify its statusCode
equals IndexingMetadataContextStatusCodes.Initialized, and if not throw a clear
Error (mentioning INDEXING_METADATA_CONTEXT and the unexpected status) so the
test fails immediately; only after this check treat the value as an
IndexingMetadataContextInitialized before passing it to
buildConfigFromEnvironment or further assertions.
| it("returns the deserialized initialized context when one record exists", async () => { | ||
| const indexingStatus = deserializeCrossChainIndexingStatusSnapshot( | ||
| ensDbClientMock.serializedSnapshot, | ||
| ); | ||
| const ensDbPublicConfig: EnsDbPublicConfig = { | ||
| versionInfo: { postgresql: "17.4" }, | ||
| }; | ||
| const ensRainbowPublicConfig = { | ||
| serverLabelSet: { labelSetId: "subgraph", highestLabelSetVersion: 0 }, | ||
| versionInfo: { ensRainbow: "1.9.0" }, | ||
| }; | ||
| const stackInfo = buildEnsIndexerStackInfo( | ||
| ensDbPublicConfig, | ||
| ensDbClientMock.publicConfig, | ||
| ensRainbowPublicConfig, | ||
| ); | ||
| const context = buildIndexingMetadataContextInitialized(indexingStatus, stackInfo); | ||
| const serialized = serializeIndexingMetadataContext(context); | ||
|
|
||
| selectResult.current = [{ value: serialized }]; | ||
|
|
||
| const result = await createEnsDbReader().getIndexingMetadataContext(); | ||
|
|
||
| const expected = deserializeIndexingMetadataContext(serialized); | ||
| expect(result).toStrictEqual(expected); | ||
| }); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Tautological roundtrip assertion — compare against the original context instead.
expected is computed by calling deserializeIndexingMetadataContext(serialized) — exactly the same call the production path makes after fetching the row. The assertion therefore only proves deserializeIndexingMetadataContext is deterministic and never catches a regression where serialize/deserialize is lossy or asymmetric. Comparing to the original context (built locally via buildIndexingMetadataContextInitialized) makes this a meaningful roundtrip test and would also fail loudly if the reader ever stopped deserializing.
♻️ Proposed assertion change
- const result = await createEnsDbReader().getIndexingMetadataContext();
-
- const expected = deserializeIndexingMetadataContext(serialized);
- expect(result).toStrictEqual(expected);
+ const result = await createEnsDbReader().getIndexingMetadataContext();
+
+ expect(result).toStrictEqual(context);📝 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.
| it("returns the deserialized initialized context when one record exists", async () => { | |
| const indexingStatus = deserializeCrossChainIndexingStatusSnapshot( | |
| ensDbClientMock.serializedSnapshot, | |
| ); | |
| const ensDbPublicConfig: EnsDbPublicConfig = { | |
| versionInfo: { postgresql: "17.4" }, | |
| }; | |
| const ensRainbowPublicConfig = { | |
| serverLabelSet: { labelSetId: "subgraph", highestLabelSetVersion: 0 }, | |
| versionInfo: { ensRainbow: "1.9.0" }, | |
| }; | |
| const stackInfo = buildEnsIndexerStackInfo( | |
| ensDbPublicConfig, | |
| ensDbClientMock.publicConfig, | |
| ensRainbowPublicConfig, | |
| ); | |
| const context = buildIndexingMetadataContextInitialized(indexingStatus, stackInfo); | |
| const serialized = serializeIndexingMetadataContext(context); | |
| selectResult.current = [{ value: serialized }]; | |
| const result = await createEnsDbReader().getIndexingMetadataContext(); | |
| const expected = deserializeIndexingMetadataContext(serialized); | |
| expect(result).toStrictEqual(expected); | |
| }); | |
| it("returns the deserialized initialized context when one record exists", async () => { | |
| const indexingStatus = deserializeCrossChainIndexingStatusSnapshot( | |
| ensDbClientMock.serializedSnapshot, | |
| ); | |
| const ensDbPublicConfig: EnsDbPublicConfig = { | |
| versionInfo: { postgresql: "17.4" }, | |
| }; | |
| const ensRainbowPublicConfig = { | |
| serverLabelSet: { labelSetId: "subgraph", highestLabelSetVersion: 0 }, | |
| versionInfo: { ensRainbow: "1.9.0" }, | |
| }; | |
| const stackInfo = buildEnsIndexerStackInfo( | |
| ensDbPublicConfig, | |
| ensDbClientMock.publicConfig, | |
| ensRainbowPublicConfig, | |
| ); | |
| const context = buildIndexingMetadataContextInitialized(indexingStatus, stackInfo); | |
| const serialized = serializeIndexingMetadataContext(context); | |
| selectResult.current = [{ value: serialized }]; | |
| const result = await createEnsDbReader().getIndexingMetadataContext(); | |
| expect(result).toStrictEqual(context); | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/ensdb-sdk/src/client/ensdb-reader.test.ts` around lines 123 - 148,
The test currently computes expected by deserializing the serialized context and
then compares result to that deserialized value, which is tautological; instead
assert the reader's output equals the original in-memory context you built.
Change the assertion in the "returns the deserialized initialized context when
one record exists" test to compare result to the local context variable (built
via buildIndexingMetadataContextInitialized) rather than to
deserializeIndexingMetadataContext(serialized); keep the same setup using
serializeIndexingMetadataContext(serialized) and
createEnsDbReader().getIndexingMetadataContext() so the test becomes a true
serialize/deserialize roundtrip verification.
| /** | ||
| * ENSNode Metadata | ||
| * | ||
| * Union type gathering all variants of ENSNode Metadata. | ||
| */ | ||
| export type EnsNodeMetadata = | ||
| | EnsNodeMetadataEnsDbVersion | ||
| | EnsNodeMetadataEnsIndexerPublicConfig | ||
| | EnsNodeMetadataEnsIndexerIndexingStatus; | ||
| export type EnsNodeMetadata = EnsNodeMetadataIndexingMetadataContext; |
There was a problem hiding this comment.
Stale JSDoc: EnsNodeMetadata is no longer a union.
The doc still claims "Union type gathering all variants" but the type alias now collapses to a single variant (EnsNodeMetadataIndexingMetadataContext). Either update the comment to reflect the current single-variant model, or rephrase to indicate this is intentionally a single-variant alias kept for future extensibility.
📝 Suggested wording
/**
* ENSNode Metadata
*
- * Union type gathering all variants of ENSNode Metadata.
+ * Type alias for ENSNode Metadata records. Currently has a single variant
+ * ({`@link` EnsNodeMetadataIndexingMetadataContext}); kept as an alias to
+ * accommodate future variants.
*/
export type EnsNodeMetadata = EnsNodeMetadataIndexingMetadataContext;📝 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.
| /** | |
| * ENSNode Metadata | |
| * | |
| * Union type gathering all variants of ENSNode Metadata. | |
| */ | |
| export type EnsNodeMetadata = | |
| | EnsNodeMetadataEnsDbVersion | |
| | EnsNodeMetadataEnsIndexerPublicConfig | |
| | EnsNodeMetadataEnsIndexerIndexingStatus; | |
| export type EnsNodeMetadata = EnsNodeMetadataIndexingMetadataContext; | |
| /** | |
| * ENSNode Metadata | |
| * | |
| * Type alias for ENSNode Metadata records. Currently has a single variant | |
| * ({`@link` EnsNodeMetadataIndexingMetadataContext}); kept as an alias to | |
| * accommodate future variants. | |
| */ | |
| export type EnsNodeMetadata = EnsNodeMetadataIndexingMetadataContext; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/ensdb-sdk/src/client/ensnode-metadata.ts` around lines 23 - 28,
Update the stale JSDoc for the EnsNodeMetadata type: the comment should no
longer say "Union type gathering all variants" because EnsNodeMetadata is
currently a direct alias of EnsNodeMetadataIndexingMetadataContext; either
change the wording to indicate it is a single-variant alias (kept for future
extensibility) or otherwise describe that it aliases
EnsNodeMetadataIndexingMetadataContext, referencing the EnsNodeMetadata and
EnsNodeMetadataIndexingMetadataContext symbols so the comment accurately
reflects the current model.
Lite PR
Tip: Review docs on the ENSNode PR process
Summary
Why
Testing
Notes for Reviewer (Optional)
Resolves #1884
Pre-Review Checklist (Blocking)