Local Ponder Client & Indexing Status Builder to power ENSIndexer#1675
Open
Local Ponder Client & Indexing Status Builder to power ENSIndexer#1675
Conversation
The local client extends PonderClient, including override for the `metrics()` method to ensure the backfillEndBlock field is available in indexing metrics for each indexed chain.
…e SDK This function uses well known types from ENSNode SDK, so it as well might live in ENSNode SDK module for Indexing Status
…ckrange map from ENSIndexer config object.
The builder can produce `OmnichainIndexingStatusSnapshot` with just `LocalPonderClient` instance and some cache RPC calls.
🦋 Changeset detectedLatest commit: 2fc10c2 The changes in this PR will be included in the next version bump. This PR includes changesets to release 19 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
Contributor
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis PR introduces Changes
Sequence Diagram(s)sequenceDiagram
participant Handler as ENS API Handler
participant Builder as IndexingStatusBuilder
participant LocalClient as LocalPonderClient
participant PonderClient as PonderClient
participant PublicClient as PublicClient(s)
Handler->>Builder: getOmnichainIndexingStatusSnapshot()
Builder->>LocalClient: metrics()
LocalClient->>PonderClient: metrics()
PonderClient-->>LocalClient: PonderIndexingMetrics
LocalClient->>LocalClient: enrichMetrics (add backfillEndBlock)
LocalClient-->>Builder: LocalPonderIndexingMetrics
Builder->>Builder: fetchChainsIndexingBlockRefs()
Builder->>PublicClient: getBlock(startBlock)
PublicClient-->>Builder: BlockRef
Builder->>PublicClient: getBlock(backfillEndBlock)
PublicClient-->>Builder: BlockRef
Builder->>Builder: cache block refs
Builder->>Builder: buildChainIndexingStatusSnapshots()
Builder->>Builder: For each chain: buildChainIndexingStatusSnapshot()
Builder->>Builder: Determine state: Queued/Backfill/Completed/Following
Builder->>Builder: buildOmnichainIndexingStatusSnapshot(chainSnapshots)
Builder->>Builder: Validate & construct omnichain snapshot
Builder-->>Handler: OmnichainIndexingStatusSnapshot
Estimated Code Review Effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly Related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Pull request overview
This PR introduces a new “local Ponder client” + “indexing status builder” layer and wires it into the ENSIndexer /indexing-status API, aiming to simplify/standardize how omnichain indexing snapshots are built using cached RPC lookups.
Changes:
- Add
@ensnode/ponder-sdkLocalPonderClient+ “local” indexing metrics/blocks types, and export them from the SDK. - Add
IndexingStatusBuilderin ENSIndexer to buildOmnichainIndexingStatusSnapshot, including cached block ref fetching viaviemPublicClient. - Wire the builder into the Ponder API handler; add config helpers/types for chain block ranges.
Reviewed changes
Copilot reviewed 12 out of 13 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| pnpm-lock.yaml | Locks viem addition for the workspace. |
| packages/ponder-sdk/package.json | Adds viem as dev + peer dependency for the new client integration. |
| packages/ponder-sdk/src/index.ts | Exports new local client/metrics/blocks APIs. |
| packages/ponder-sdk/src/indexing-blocks.ts | Defines ChainIndexingBlocks input for block ref fetching. |
| packages/ponder-sdk/src/local-indexing-metrics.ts | Defines “local” metrics types (notably backfill end block enrichment). |
| packages/ponder-sdk/src/local-ponder-client.ts | Implements LocalPonderClient that enriches/filters Ponder metrics and holds cached PublicClients and block ranges. |
| packages/ensnode-sdk/src/shared/types.ts | Adds BlockrangeWithStartBlock in ensnode-sdk shared types. |
| packages/ensnode-sdk/src/indexing-status/omnichain-indexing-status-snapshot.ts | Adds buildOmnichainIndexingStatusSnapshot(Map<...>) helper with validation. |
| apps/ensindexer/src/config/types.ts | Renames ENSIndexerConfig interface to EnsIndexerConfig + provides deprecated alias. |
| apps/ensindexer/src/config/chains-blockrange.ts | Builds per-chain block ranges derived from plugins/datasources. |
| apps/ensindexer/src/lib/indexing-status-builder/indexing-status-builder.ts | New builder that fetches block refs (cached) and builds chain + omnichain snapshots. |
| apps/ensindexer/ponder/src/api/local-ponder-client.ts | Constructs singleton LocalPonderClient from Ponder publicClients + derived block ranges. |
| apps/ensindexer/ponder/src/api/handlers/ensnode-api.ts | Switches /indexing-status to use IndexingStatusBuilder. |
Files not reviewed (1)
- pnpm-lock.yaml: Language not supported
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
apps/ensindexer/src/lib/indexing-status-builder/indexing-status-builder.ts
Outdated
Show resolved
Hide resolved
apps/ensindexer/src/lib/indexing-status-builder/indexing-status-builder.ts
Show resolved
Hide resolved
packages/ensnode-sdk/src/indexing-status/omnichain-indexing-status-snapshot.ts
Show resolved
Hide resolved
apps/ensindexer/src/lib/indexing-status-builder/indexing-status-builder.ts
Show resolved
Hide resolved
![vercel[bot]](https://avatars.githubusercontent.com/in/8329?s=60&v=4){kind=small}
apps/ensindexer/src/lib/indexing-status-builder/indexing-status-builder.ts
Outdated
Show resolved
Hide resolved
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 7
🤖 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/config/chains-blockrange.ts`:
- Around line 11-18: Remove the redundant `@returns` JSDoc tags that merely
restate the summary in the JSDoc for the function that "Build[s] a map of
indexed chains to their corresponding blockranges" (the comment block above the
blockrange builder) and the similar JSDoc at lines 48-55; keep the summary and
`@param` tags but delete the duplicate `@returns` entries so the doc follows the
guideline against restating the method summary.
- Around line 64-69: The current endBlock calculation can drop a previously
computed currentBlockrange.endBlock when the new contract lacks an endBlock;
change the logic so that if contract.endBlock is present use
Math.max(currentBlockrange.endBlock, contract.endBlock) (when
currentBlockrange.endBlock exists) but if contract.endBlock is absent preserve
currentBlockrange.endBlock (and only fall back to contract.endBlock when
currentBlockrange.endBlock is undefined). Update the expression that assigns
endBlock (referencing currentBlockrange, contract.endBlock, and endBlock) to
prefer currentBlockrange.endBlock when contract.endBlock is missing, ensuring
the chain’s blockrange never widens due to a missing contract endBlock.
In `@apps/ensindexer/src/lib/indexing-status-builder/indexing-status-builder.ts`:
- Around line 41-47: Remove the redundant `@returns` JSDoc tags that simply
restate each method summary in the new JSDoc blocks; for example, edit the JSDoc
for getOmnichainIndexingStatusSnapshot() to delete the redundant `@returns` line
(or replace it with a more informative return description if needed). Apply the
same change to the other JSDoc blocks flagged in this review (the ones at the
other ranges in this file) so JSDoc only contains meaningful `@returns` content or
none at all.
- Around line 181-194: The current loop throws when a chain's metric isn't
Historical; instead, remove the throw and handle non‑historical states by only
using backfillEndBlock when present: for each entry in
localChainsIndexingMetrics check chainIndexingMetric.state ===
ChainIndexingStates.Historical and set a local backfillEndBlock variable (or
leave it undefined) accordingly, then call
this.fetchChainIndexingBlockRefs(chainId, { startBlock, endBlock,
backfillEndBlock }) using that variable; ensure you still call
this.localPonderClient.getChainBlockrange(chainId) and process results for all
chains rather than aborting on non‑historical states.
In
`@packages/ensnode-sdk/src/indexing-status/omnichain-indexing-status-snapshot.ts`:
- Around line 344-349: Remove the redundant `@returns` JSDoc in the comment block
above the function that "Build[s] an Omnichain Indexing Status Snapshot" (the
JSDoc immediately preceding the omnichain snapshot builder function); either
delete the `@returns` line or replace it with a more specific semantic description
if additional return detail is needed, leaving only the summary and any
non-redundant tags.
In `@packages/ensnode-sdk/src/shared/types.ts`:
- Around line 122-141: Replace the duplicated invariant comments by documenting
the invariant once on the type alias instead of on each field: convert or
replace the current interface BlockrangeWithStartBlock with a type alias
(keeping the same name) that has a single doc comment stating the invariant that
startBlock is required and, when endBlock is present, it is greater than
startBlock; then simplify the field docs for startBlock and endBlock to minimal
single-line descriptions (or remove them) and remove the duplicate invariant
text from startBlock and endBlock; reference BlockNumber for types as before.
In `@packages/ponder-sdk/src/local-ponder-client.ts`:
- Around line 40-45: Remove the redundant JSDoc `@returns` lines that merely
restate the summary: edit the JSDoc block that begins "Get the block range for a
specific chain ID" (the getter for the block range) and the other similar JSDoc
blocks flagged in the comment; delete the duplicate `@returns` tags and leave
either no `@returns` or a single concise `@returns` only if it adds new information
(e.g., describes the shape/type), ensuring the remaining comments still describe
thrown errors and parameters (retain `@param` and `@throws` as appropriate).
ℹ️ Review info
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (12)
apps/ensindexer/ponder/src/api/handlers/ensnode-api.tsapps/ensindexer/ponder/src/api/local-ponder-client.tsapps/ensindexer/src/config/chains-blockrange.tsapps/ensindexer/src/config/types.tsapps/ensindexer/src/lib/indexing-status-builder/indexing-status-builder.tspackages/ensnode-sdk/src/indexing-status/omnichain-indexing-status-snapshot.tspackages/ensnode-sdk/src/shared/types.tspackages/ponder-sdk/package.jsonpackages/ponder-sdk/src/index.tspackages/ponder-sdk/src/indexing-blocks.tspackages/ponder-sdk/src/local-indexing-metrics.tspackages/ponder-sdk/src/local-ponder-client.ts
apps/ensindexer/src/lib/indexing-status-builder/indexing-status-builder.ts
Show resolved
Hide resolved
apps/ensindexer/src/lib/indexing-status-builder/indexing-status-builder.ts
Show resolved
Hide resolved
packages/ensnode-sdk/src/indexing-status/omnichain-indexing-status-snapshot.ts
Show resolved
Hide resolved
Contributor
Author
|
@greptile review |
Contributor
Greptile SummaryIntroduces a new Ponder SDK architecture to simplify ENSIndexer's interaction with local Ponder instances. The PR adds Key changes:
Minor issues:
Confidence Score: 4/5
Important Files Changed
Sequence DiagramsequenceDiagram
participant Client as HTTP Client
participant API as ensnode-api handler
participant Builder as IndexingStatusBuilder
participant LPC as LocalPonderClient
participant Ponder as Ponder App
Client->>API: GET /indexing-status
API->>Builder: getOmnichainIndexingStatusSnapshot()
par Fetch Ponder Data
Builder->>LPC: metrics()
LPC->>Ponder: /metrics endpoint
Ponder-->>LPC: PonderIndexingMetrics
LPC-->>Builder: LocalPonderIndexingMetrics (enriched)
and
Builder->>LPC: status()
LPC->>Ponder: /status endpoint
Ponder-->>LPC: PonderIndexingStatus
LPC-->>Builder: PonderIndexingStatus
end
alt First call (cache miss)
loop For each chain
Builder->>LPC: getIndexedBlockrange(chainId)
LPC-->>Builder: BlockrangeWithStartBlock
Builder->>LPC: getCachedPublicClient(chainId)
LPC-->>Builder: CachedPublicClient
Builder->>Ponder: getBlock() [via client]
Ponder-->>Builder: Block data (startBlock, endBlock, backfillEndBlock)
end
Builder->>Builder: Cache block refs
end
Builder->>Builder: buildChainIndexingStatusSnapshots()
Builder->>Builder: buildOmnichainIndexingStatusSnapshot()
Builder-->>API: OmnichainIndexingStatusSnapshot
API->>API: createCrossChainIndexingStatusSnapshot()
API->>API: createRealtimeIndexingStatusProjection()
API-->>Client: IndexingStatusResponse (JSON)
Last reviewed commit: 2fc10c2 |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
apps/ensindexer/src/lib/indexing-status-builder/indexing-status-builder.ts
Show resolved
Hide resolved
apps/ensindexer/src/lib/indexing-status-builder/indexing-status-builder.ts
Show resolved
Hide resolved
Contributor
Author
|
@greptile review |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (6)
packages/ponder-sdk/src/local-ponder-client.ts (1)
40-47: Remove redundant@returnstags across multiple methods.The
@returnstags ongetChainBlockrange,getCachedPublicClient,metrics,buildLocalPonderIndexingMetrics, andselectEntriesForIndexedChainsOnlyrestate their respective summaries. As per coding guidelines, "Do not add JSDoc@returnstags that merely restate the method summary; remove redundancy during PR review."Also applies to lines 58-63, 75-80, 96-105, 167-174.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/ponder-sdk/src/local-ponder-client.ts` around lines 40 - 47, Remove the redundant JSDoc `@returns` tags that merely restate the method summary for the listed methods: getChainBlockrange, getCachedPublicClient, metrics, buildLocalPonderIndexingMetrics, and selectEntriesForIndexedChainsOnly; edit each function's JSDoc to delete the unnecessary `@returns` entry (but retain the summary and any `@returns` content that provides additional, non-redundant detail such as error conditions or value shape).packages/ponder-sdk/src/local-indexing-metrics.ts (2)
10-10: Remove unused importLocalPonderClient.
LocalPonderClientis only referenced in a JSDoc{@link}comment (line 42), which doesn't require a runtime or type-level import. The{@link}will still render as text without the import.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/ponder-sdk/src/local-indexing-metrics.ts` at line 10, Remove the unused type import LocalPonderClient from the top of the file; it’s only referenced in a JSDoc {`@link`} and doesn’t require a runtime or type import, so delete the line importing LocalPonderClient to eliminate the unused-import lint/error and keep the JSDoc link as-is.
4-4: Remove unused importChainIndexingMetrics.
ChainIndexingMetricsis imported but not referenced in any type definition or code in this file.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/ponder-sdk/src/local-indexing-metrics.ts` at line 4, Remove the unused import ChainIndexingMetrics from the import list in this module; locate the import statement that includes "ChainIndexingMetrics" (alongside other imports) and delete just that symbol so the file no longer references an unused identifier, then run the linter or TypeScript build to ensure no remaining references need cleanup.apps/ensindexer/src/config/chains-blockrange.ts (1)
54-54: Remove redundant@returnstag.The
@returns The blockrange for the contract.merely restates the method summary. As per coding guidelines, "Do not add JSDoc@returnstags that merely restate the method summary; remove redundancy during PR review."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ensindexer/src/config/chains-blockrange.ts` at line 54, Remove the redundant JSDoc `@returns` tag that restates the summary: delete the line "@returns The blockrange for the contract." from the JSDoc block above the function that returns the contract block range (the JSDoc describing the blockrange for the contract), leaving the descriptive summary without the duplicate `@returns` entry.apps/ensindexer/src/lib/indexing-status-builder/indexing-status-builder.ts (2)
45-67: First-call Historical-only gate: be aware of the restart-after-transition edge case.The caching logic at Lines 52–58 requires all chains to be in
Historicalstate on the very first invocation (viaassertChainsIndexingMetricsHistorical). If the process restarts after any chain has already transitioned toCompletedorRealtime, the first call will throw and block refs will never be cached, making the endpoint permanently broken until a full re-index.This was flagged in a previous review cycle. If this is intentional (i.e., the builder is always constructed early in the lifecycle before any chain transitions), please add an inline comment documenting that assumption.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ensindexer/src/lib/indexing-status-builder/indexing-status-builder.ts` around lines 45 - 67, The current getOmnichainIndexingStatusSnapshot() uses assertChainsIndexingMetricsHistorical() before caching _chainsIndexingBlockRefs, which throws on first call if any chain has already transitioned and thus breaks restarts; either remove/relax that strict assertion and instead proceed to call fetchChainsIndexingBlockRefs() unconditionally (or only validate per-chain without throwing) so the cache is populated even when some chains are Completed/Realtime, or if the strict historical-only requirement is intentional, add a clear inline comment on the constructor/builder and above the assertChainsIndexingMetricsHistorical() call documenting the lifecycle assumption; reference getOmnichainIndexingStatusSnapshot, _chainsIndexingBlockRefs, assertChainsIndexingMetricsHistorical, and fetchChainsIndexingBlockRefs when making the change.
42-44: Remove redundant@returnstags that merely restate the method summary.Lines 76, 119–120, 231, and 257 contain
@returnstags that restate the method summary without adding new information. As per coding guidelines, Do not add JSDoc@returnstags that merely restate the method summary; remove redundancy during PR review.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ensindexer/src/lib/indexing-status-builder/indexing-status-builder.ts` around lines 42 - 44, Remove redundant JSDoc `@returns` tags that simply restate the method summary in the IndexingStatusBuilder file: specifically remove the duplicate `@returns` entries from the JSDoc for getOmnichainIndexingStatusSnapshot (the "Get Omnichain Indexing Status Snapshot" comment) and the other functions in the same file whose `@returns` lines merely repeat the summary; keep meaningful `@returns` only when they add details about the return shape or semantics, otherwise delete the `@returns` tag.
🤖 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-status-builder/indexing-status-builder.test.ts`:
- Around line 249-261: The test's Realtime mock includes Historical-only fields;
remove `historicalTotalBlocks` and `backfillEndBlock` from the
`localMetricsRealtime` data passed to buildLocalChainsIndexingMetrics so the
Realtime variant only contains Realtime-relevant properties (e.g., `state` and
`latestSyncedBlock`); locate the object keyed by `chainId` in the test where
`ChainIndexingStates.Realtime` is used and drop those two fields to avoid
misleading excess properties.
- Around line 407-447: Add a test in the Error handling suite that simulates an
RPC failure by making the public client throw when fetching a block and asserts
the builder wraps that error with chain and block context: use
buildPublicClientMock() and set publicClientMock.getBlock to
mockRejectedValue(new Error("RPC failure")), construct localMetrics and
localStatus so the chain exists, create localPonderClientMock with
getCachedPublicClient returning the mocked public client and status/metrics as
before, instantiate IndexingStatusBuilder and assert await
expect(builder.getOmnichainIndexingStatusSnapshot()).rejects.toThrowError(/chain
ID 1.*block.*\d+|block.*\d+.*chain ID 1/) (or a regex matching the chain ID and
block number in the wrapped message) to verify the fetchBlockRef catch path
wraps RPC errors with chain ID and block number context.
In `@apps/ensindexer/src/lib/indexing-status-builder/indexing-status-builder.ts`:
- Around line 201-220: The current fetchChainsIndexingBlockRefs iterates
localChainsIndexingMetrics and awaits fetchChainIndexingBlockRefs inside a
for-loop, causing sequential RPCs; change it to build an array of promises by
mapping over localChainsIndexingMetrics (use
this.localPonderClient.getChainBlockrange(chainId) and call
this.fetchChainIndexingBlockRefs(chainId, {...}) for each entry), await
Promise.all on that array, then populate and return chainsIndexingBlockRefs Map
from the resolved results so per-chain fetches run in parallel; optionally limit
concurrency if needed.
In `@packages/ponder-sdk/src/local-ponder-client.test.ts`:
- Around line 89-102: The test uses a placeholder {} as PublicClient but only
asserts toBeDefined(); change the assertion to check referential equality so we
validate the exact cached instance is returned: in the test that calls
createLocalPonderClientMock(...) with cachedPublicClients and then
client.getCachedPublicClient(chainIds.Optimism), replace the
expect(...).toBeDefined() with an assertion like expect(clientRef).toBe(the same
map entry value) to confirm getCachedPublicClient returns the exact object
placed into cachedPublicClients.
In `@packages/ponder-sdk/src/local-ponder-client.ts`:
- Around line 17-19: The three private fields indexedChainIds, chainsBlockrange,
and cachedPublicClients in the LocalPonderClient class are only assigned in the
constructor and should be marked readonly to express immutability and prevent
reassignment; update their declarations to "private readonly indexedChainIds:
Set<ChainId>", "private readonly chainsBlockrange: Map<ChainId,
BlockrangeWithStartBlock>", and "private readonly cachedPublicClients:
Map<ChainId, PublicClient>" (constructor assignments remain valid for readonly
fields).
---
Duplicate comments:
In `@apps/ensindexer/src/config/chains-blockrange.ts`:
- Line 54: Remove the redundant JSDoc `@returns` tag that restates the summary:
delete the line "@returns The blockrange for the contract." from the JSDoc block
above the function that returns the contract block range (the JSDoc describing
the blockrange for the contract), leaving the descriptive summary without the
duplicate `@returns` entry.
In `@apps/ensindexer/src/lib/indexing-status-builder/indexing-status-builder.ts`:
- Around line 45-67: The current getOmnichainIndexingStatusSnapshot() uses
assertChainsIndexingMetricsHistorical() before caching _chainsIndexingBlockRefs,
which throws on first call if any chain has already transitioned and thus breaks
restarts; either remove/relax that strict assertion and instead proceed to call
fetchChainsIndexingBlockRefs() unconditionally (or only validate per-chain
without throwing) so the cache is populated even when some chains are
Completed/Realtime, or if the strict historical-only requirement is intentional,
add a clear inline comment on the constructor/builder and above the
assertChainsIndexingMetricsHistorical() call documenting the lifecycle
assumption; reference getOmnichainIndexingStatusSnapshot,
_chainsIndexingBlockRefs, assertChainsIndexingMetricsHistorical, and
fetchChainsIndexingBlockRefs when making the change.
- Around line 42-44: Remove redundant JSDoc `@returns` tags that simply restate
the method summary in the IndexingStatusBuilder file: specifically remove the
duplicate `@returns` entries from the JSDoc for getOmnichainIndexingStatusSnapshot
(the "Get Omnichain Indexing Status Snapshot" comment) and the other functions
in the same file whose `@returns` lines merely repeat the summary; keep meaningful
`@returns` only when they add details about the return shape or semantics,
otherwise delete the `@returns` tag.
In `@packages/ponder-sdk/src/local-indexing-metrics.ts`:
- Line 10: Remove the unused type import LocalPonderClient from the top of the
file; it’s only referenced in a JSDoc {`@link`} and doesn’t require a runtime or
type import, so delete the line importing LocalPonderClient to eliminate the
unused-import lint/error and keep the JSDoc link as-is.
- Line 4: Remove the unused import ChainIndexingMetrics from the import list in
this module; locate the import statement that includes "ChainIndexingMetrics"
(alongside other imports) and delete just that symbol so the file no longer
references an unused identifier, then run the linter or TypeScript build to
ensure no remaining references need cleanup.
In `@packages/ponder-sdk/src/local-ponder-client.ts`:
- Around line 40-47: Remove the redundant JSDoc `@returns` tags that merely
restate the method summary for the listed methods: getChainBlockrange,
getCachedPublicClient, metrics, buildLocalPonderIndexingMetrics, and
selectEntriesForIndexedChainsOnly; edit each function's JSDoc to delete the
unnecessary `@returns` entry (but retain the summary and any `@returns` content that
provides additional, non-redundant detail such as error conditions or value
shape).
ℹ️ Review info
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (11)
apps/ensindexer/ponder/src/api/local-ponder-client.tsapps/ensindexer/src/config/chains-blockrange.test.tsapps/ensindexer/src/config/chains-blockrange.tsapps/ensindexer/src/lib/indexing-status-builder/indexing-status-builder.mock.tsapps/ensindexer/src/lib/indexing-status-builder/indexing-status-builder.test.tsapps/ensindexer/src/lib/indexing-status-builder/indexing-status-builder.tspackages/ponder-sdk/src/block-refs.mock.tspackages/ponder-sdk/src/local-indexing-metrics.tspackages/ponder-sdk/src/local-ponder-client.mock.tspackages/ponder-sdk/src/local-ponder-client.test.tspackages/ponder-sdk/src/local-ponder-client.ts
apps/ensindexer/src/lib/indexing-status-builder/indexing-status-builder.test.ts
Show resolved
Hide resolved
apps/ensindexer/src/lib/indexing-status-builder/indexing-status-builder.test.ts
Show resolved
Hide resolved
apps/ensindexer/src/lib/indexing-status-builder/indexing-status-builder.ts
Show resolved
Hide resolved
apps/ensindexer/src/lib/indexing-status-builder/indexing-status-builder.ts
Show resolved
Hide resolved
tk-o
commented
Feb 25, 2026
Contributor
Author
tk-o
left a comment
tk-o
left a comment
There was a problem hiding this comment.
Self-review completed
Contributor
Author
There was a problem hiding this comment.
We need a place to create the singleton instance of LocalPonderClient for modules acorss ENSIndexer app. I decided to create this file here, which also includes loading objects required for LocalPonderClient constructor args.
Member
There was a problem hiding this comment.
Thanks. I'm surprised to see this file inside a subdirectory of /api though. Is there a special reason for this?
Contributor
Author
There was a problem hiding this comment.
Yes, the special reason is podner:api import:
import { publicClients } from "ponder:api";
Ponder only allows this import to be done from a file which sits inside the src/api driectory. In our case, it's apps/ensindexer/ponder/src/api directory.
Contributor
Author
There was a problem hiding this comment.
@lightwalker-eth I've just tested my assumption and it turns out it's possible to move the file out of the api directory!
I'll rename the file from:
apps/ensindexer/ponder/src/api/local-ponder-client.ts
to
apps/ensindexer/src/lib/clients.ts
so that we have a single file in ENSIndexer app to gather all signleton instances for clients to be used 👍
| // Fetch the chains indexing block refs if not already cached. | ||
| if (!this._chainsIndexingBlockRefs) { | ||
| const chainsIndexingMetrics = localPonderIndexingMetrics.chains; | ||
| this.assertChainsIndexingMetricsHistorical(chainsIndexingMetrics); |
Contributor
Author
There was a problem hiding this comment.
We can only fetch chain indexing block refs for "historical" chain indexing metics. All chain indexing metrics should be "historical" at ENSIndexer startup. We will soon be calling the getOmnichainIndexingStatusSnapshot() method from ENSDb Writer Worker, right at the ENSIndexer startup, so it's safe to assume that this._chainsIndexingBlockRefs will be initialized while all chains indexing metrics were "historical".
Member
There was a problem hiding this comment.
But I thought we only needed a chain to be in "historical" status if we needed to calculate backfill state for that chain? Therefore, if when we attempt to build this data model the chain isn't in "historical" status anymore for some reason, wouldn't that all be fine because wouldn't that also mean the chain is guaranteed not to be in a backfill state anymore for the lifecycle of the ponder app instance?
Contributor
Author
There was a problem hiding this comment.
Well, if we made the backfillEndBlock optional, then yes, when chain isn't "historical" status anymore, it should not require fetching a block ref for the backfill end block 👍 No other chain indexing status than "backfill" requires that information.
Comment on lines
30
to
37
| this.chainsBlockrange = this.selectEntriesForIndexedChainsOnly( | ||
| chainsBlockrange, | ||
| "Chains Blockrange", | ||
| ); | ||
| this.cachedPublicClients = this.selectEntriesForIndexedChainsOnly( | ||
| cachedPublicClients, | ||
| "Cached Public Clients", | ||
| ); |
Contributor
Author
There was a problem hiding this comment.
We don't want to use all chains' records that Ponder may give use. We just need the records for indexed chains (as determined by indexedChainIds.
Member
There was a problem hiding this comment.
This is a good PR comment, but it's better to put it as a comment in the code.
Comment on lines
83
to
86
| const chainsIndexingMetrics = this.selectEntriesForIndexedChainsOnly( | ||
| metrics.chains, | ||
| "Chains Indexing Metrics", | ||
| ); |
Contributor
Author
There was a problem hiding this comment.
We don't want to use all chains' records that Ponder may give use. We just need the records for indexed chains (as determined by indexedChainIds.
Member
There was a problem hiding this comment.
Thanks for adding this comment in the PR, but these ideas should be transferred directly into an inline comment in the code explaining why we are calling selectEntriesForIndexedChainsOnly here.
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 23 out of 24 changed files in this pull request and generated 2 comments.
Files not reviewed (1)
- pnpm-lock.yaml: Language not supported
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
apps/ensindexer/src/lib/indexing-status-builder/indexing-status-builder.ts
Show resolved
Hide resolved
tk-o
changed the title
tk-o
commented
Feb 25, 2026
| * @param chainStatusSnapshots - A map of chain IDs to their chain indexing status snapshots. | ||
| * @returns The omnichain indexing status snapshot. | ||
| */ | ||
| export function buildOmnichainIndexingStatusSnapshot( |
Contributor
Author
There was a problem hiding this comment.
This function body was copy-pasted (with modified input type) from this file:
tk-o
commented
Feb 25, 2026
Comment on lines
128
to
161
| const indexingConfig = createIndexingConfig(startBlock, endBlock); | ||
|
|
||
| // In omnichain ordering, if the startBlock is the same as the | ||
| // status block, the chain has not started yet. | ||
| if (isBlockRefEqualTo(startBlock, checkpointBlock)) { | ||
| return validateChainIndexingStatusSnapshot({ | ||
| chainStatus: ChainIndexingStatusIds.Queued, | ||
| config: indexingConfig, | ||
| } satisfies Unvalidated<ChainIndexingStatusSnapshotQueued>); | ||
| } | ||
|
|
||
| if (chainIndexingMetrics.state === ChainIndexingStates.Completed) { | ||
| return validateChainIndexingStatusSnapshot({ | ||
| chainStatus: ChainIndexingStatusIds.Completed, | ||
| latestIndexedBlock: checkpointBlock, | ||
| config: indexingConfig as Unvalidated<ChainIndexingConfigDefinite>, | ||
| } satisfies Unvalidated<ChainIndexingStatusSnapshotCompleted>); | ||
| } | ||
|
|
||
| if (chainIndexingMetrics.state === ChainIndexingStates.Realtime) { | ||
| return validateChainIndexingStatusSnapshot({ | ||
| chainStatus: ChainIndexingStatusIds.Following, | ||
| latestIndexedBlock: checkpointBlock, | ||
| latestKnownBlock: chainIndexingMetrics.latestSyncedBlock, | ||
| config: indexingConfig as Unvalidated<ChainIndexingConfigIndefinite>, | ||
| } satisfies Unvalidated<ChainIndexingStatusSnapshotFollowing>); | ||
| } | ||
|
|
||
| return validateChainIndexingStatusSnapshot({ | ||
| chainStatus: ChainIndexingStatusIds.Backfill, | ||
| latestIndexedBlock: checkpointBlock, | ||
| backfillEndBlock, | ||
| config: indexingConfig, | ||
| } satisfies Unvalidated<ChainIndexingStatusSnapshotBackfill>); |
Contributor
Author
There was a problem hiding this comment.
Potentially, we could extract this as a function and move into ENSNode SDK:
export function buildChainIndexingStatusSnapshot(
startBlock: BlockRef,
endBlock: BlockRef | null,
backfillEndBlock: BlockRef,
checkpointBlock: BlockRef,
): ChainIndexingStatusSnapshotand then call the SDK function from buildChainIndexingStatusSnapshots method
const { checkpointBlock } = chainIndexingStatus;
const { startBlock, endBlock, backfillEndBlock } = chainIndexingBlockRefs;
const chainStatusSnapshot = buildChainIndexingStatusSnapshot(
startBlock,
endBlock,
backfillEndBlock,
checkpointBlock
);Edit
I gave it a go, but ENSNode SDK does not know about ChainIndexingStatusIds (it's from Ponder SDK), so if we wanted to execute this refactoring, we'd need to extend ENSNode SDK with ChainIndexingStatusIds definition. Feedback appreciated.
Member
There was a problem hiding this comment.
I don't understand. Aren't ChainIndexingStatusIds already defined inside packages/ensnode-sdk/src/indexing-status/chain-indexing-status-snapshot.ts?
Contributor
Author
There was a problem hiding this comment.
My bad, sorry, I meant ChainIndexingStates, which are defined in Ponder SDK in order to describe variants of ChainIndexingMetrics.
Member
lightwalker-eth
left a comment
lightwalker-eth
left a comment
There was a problem hiding this comment.
@tk-o Thanks. This is a much improved architecture but a number of key points still require refinement.
Member
There was a problem hiding this comment.
Thanks. I'm surprised to see this file inside a subdirectory of /api though. Is there a special reason for this?
| * | ||
| * @throws Error if any of the above invariants are violated. | ||
| */ | ||
| function buildPonderCachedPublicClients(): Map<ChainId, PublicClient> { |
Member
There was a problem hiding this comment.
This responsibility should live inside the implementation of LocalPonderClient.
The constructor of LocalPonderClient should be either be passed ponder:api or be passed publicClients as imported from ponder:api.
| LocalPonderIndexingMetrics, | ||
| } from "./local-indexing-metrics"; | ||
|
|
||
| export class LocalPonderClient extends PonderClient { |
Member
There was a problem hiding this comment.
Please add JSDoc here that explains the big idea that motivated us to create this class. What role / responsibility does it have?
| private chainsBlockrange: Map<ChainId, BlockrangeWithStartBlock>; | ||
| private cachedPublicClients: Map<ChainId, PublicClient>; | ||
|
|
||
| constructor( |
Member
There was a problem hiding this comment.
The constructor needs JSDoc.
It's especially important to describe where some of these params need to come from. Ex: some of these need to come from imports from the local Ponder app!
| ponderAppUrl: URL, | ||
| indexedChainIds: Set<ChainId>, | ||
| chainsBlockrange: Map<ChainId, BlockrangeWithStartBlock>, | ||
| cachedPublicClients: Map<ChainId, PublicClient>, |
Member
There was a problem hiding this comment.
Suggest to make a type alias for CachedPublicClient and to make use of it wherever relevant to help document how these public clients are special.
| } | ||
|
|
||
| export class IndexingStatusBuilder { | ||
| private _chainsIndexingBlockRefs: Map<ChainId, ChainIndexingBlockRefs> | undefined; |
Member
There was a problem hiding this comment.
Suggested change
| private _chainsIndexingBlockRefs: Map<ChainId, ChainIndexingBlockRefs> | undefined; | |
| private _immutableIndexingConfig: Map<ChainId, ChainIndexingBlockRefs> | undefined; |
| * Assert Chains Indexing Metrics Historical | ||
| * | ||
| * This method asserts that all chains indexing metrics are historical, | ||
| * which is a necessary condition for fetching the block refs for |
Member
There was a problem hiding this comment.
Please document why this is the case.
Contributor
Author
There was a problem hiding this comment.
We'll be dropping this assertion altogether 👍
Comment on lines
128
to
161
| const indexingConfig = createIndexingConfig(startBlock, endBlock); | ||
|
|
||
| // In omnichain ordering, if the startBlock is the same as the | ||
| // status block, the chain has not started yet. | ||
| if (isBlockRefEqualTo(startBlock, checkpointBlock)) { | ||
| return validateChainIndexingStatusSnapshot({ | ||
| chainStatus: ChainIndexingStatusIds.Queued, | ||
| config: indexingConfig, | ||
| } satisfies Unvalidated<ChainIndexingStatusSnapshotQueued>); | ||
| } | ||
|
|
||
| if (chainIndexingMetrics.state === ChainIndexingStates.Completed) { | ||
| return validateChainIndexingStatusSnapshot({ | ||
| chainStatus: ChainIndexingStatusIds.Completed, | ||
| latestIndexedBlock: checkpointBlock, | ||
| config: indexingConfig as Unvalidated<ChainIndexingConfigDefinite>, | ||
| } satisfies Unvalidated<ChainIndexingStatusSnapshotCompleted>); | ||
| } | ||
|
|
||
| if (chainIndexingMetrics.state === ChainIndexingStates.Realtime) { | ||
| return validateChainIndexingStatusSnapshot({ | ||
| chainStatus: ChainIndexingStatusIds.Following, | ||
| latestIndexedBlock: checkpointBlock, | ||
| latestKnownBlock: chainIndexingMetrics.latestSyncedBlock, | ||
| config: indexingConfig as Unvalidated<ChainIndexingConfigIndefinite>, | ||
| } satisfies Unvalidated<ChainIndexingStatusSnapshotFollowing>); | ||
| } | ||
|
|
||
| return validateChainIndexingStatusSnapshot({ | ||
| chainStatus: ChainIndexingStatusIds.Backfill, | ||
| latestIndexedBlock: checkpointBlock, | ||
| backfillEndBlock, | ||
| config: indexingConfig, | ||
| } satisfies Unvalidated<ChainIndexingStatusSnapshotBackfill>); |
Member
There was a problem hiding this comment.
I don't understand. Aren't ChainIndexingStatusIds already defined inside packages/ensnode-sdk/src/indexing-status/chain-indexing-status-snapshot.ts?
apps/ensindexer/src/lib/indexing-status-builder/indexing-status-builder.ts
Show resolved
Hide resolved
| // Fetch the chains indexing block refs if not already cached. | ||
| if (!this._chainsIndexingBlockRefs) { | ||
| const chainsIndexingMetrics = localPonderIndexingMetrics.chains; | ||
| this.assertChainsIndexingMetricsHistorical(chainsIndexingMetrics); |
Member
There was a problem hiding this comment.
But I thought we only needed a chain to be in "historical" status if we needed to calculate backfill state for that chain? Therefore, if when we attempt to build this data model the chain isn't in "historical" status anymore for some reason, wouldn't that all be fine because wouldn't that also mean the chain is guaranteed not to be in a backfill state anymore for the lifecycle of the ponder app instance?
…ics state (not only for "historical" state)
Will be useful while converting local Ponder API data into LocalPonderClient data model.
Building a map of cached public clients is a responsibility of the `LocalPonderClient` class.
Create `indexedBlockranges` object with combination of `getPluginsRequiredDatasourceNames()` and `buildIndexedBlockranges()`.
…e indexed blockrange aggregation, for example, across multiple contract definitions.
Contributor
Author
|
@greptile review |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (5)
apps/ensindexer/src/lib/indexing-status-builder/indexing-status-builder.test.ts (1)
252-262: 🧹 Nitpick | 🔵 TrivialExcess properties on Realtime mock are misleading.
The
localMetricsRealtimeobject includeshistoricalTotalBlocksandbackfillEndBlockwhich are specific to theHistoricalstate, notRealtime. While TypeScript's structural typing allows this, it makes the test data confusing.♻️ Clean up Realtime mock data
const localMetricsRealtime = buildLocalChainsIndexingMetrics( new Map([ [ chainId, { state: ChainIndexingStates.Realtime, latestSyncedBlock: laterBlockRef, - } satisfies ChainIndexingMetricsRealtime, + }, ], ]), );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ensindexer/src/lib/indexing-status-builder/indexing-status-builder.test.ts` around lines 252 - 262, The test's Realtime mock (localMetricsRealtime) includes historical-only properties which is misleading; update the map entry used with buildLocalChainsIndexingMetrics so the value for chainId uses only the Realtime shape (state: ChainIndexingStates.Realtime and latestSyncedBlock) and remove historicalTotalBlocks and backfillEndBlock so it truly satisfies ChainIndexingMetricsRealtime (refer to the existing chainId, ChainIndexingStates.Realtime, ChainIndexingMetricsRealtime and buildLocalChainsIndexingMetrics identifiers to locate the mock).packages/ponder-sdk/src/local-ponder-client.ts (2)
43-67: 🧹 Nitpick | 🔵 TrivialConsider marking private fields as
readonly.The fields
indexedChainIds,indexedBlockranges, andcachedPublicClientsare assigned once in the constructor and never reassigned. Marking themreadonlycommunicates immutability intent and prevents accidental reassignment.♻️ Suggested change
- private indexedChainIds: Set<ChainId>; + private readonly indexedChainIds: Set<ChainId>; ... - private indexedBlockranges: Map<ChainId, BlockrangeWithStartBlock>; + private readonly indexedBlockranges: Map<ChainId, BlockrangeWithStartBlock>; ... - private cachedPublicClients: Map<ChainId, CachedPublicClient>; + private readonly cachedPublicClients: Map<ChainId, CachedPublicClient>;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/ponder-sdk/src/local-ponder-client.ts` around lines 43 - 67, Mark the three private fields that are only assigned in the constructor as readonly to express immutability and prevent future reassignment: change the declarations of indexedChainIds, indexedBlockranges, and cachedPublicClients in the LocalPonderClient class to private readonly indexedChainIds: Set<ChainId>, private readonly indexedBlockranges: Map<ChainId, BlockrangeWithStartBlock>, and private readonly cachedPublicClients: Map<ChainId, CachedPublicClient>; keep the constructor assignments as-is (no other logic changes required).
145-150: 🧹 Nitpick | 🔵 TrivialRemove redundant
@returnstags in JSDoc.Several
@returnstags repeat information already in the method summary. Per coding guidelines, these should be removed:
- Line 148: "Local Ponder Indexing Metrics" (restates "Get Local Ponder Indexing Metrics")
- Line 200: "The enriched Local Ponder Indexing Metrics" (restates "Build Local Ponder Indexing Metrics")
- Line 233: "The enriched local chain indexing metrics" (restates summary)
As per coding guidelines, Do not add JSDoc
@returnstags that merely restate the method summary; remove redundancy during PR review.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/ponder-sdk/src/local-ponder-client.ts` around lines 145 - 150, The JSDoc for methods in packages/ponder-sdk/src/local-ponder-client.ts contains redundant `@returns` lines that repeat the summary; remove the redundant `@returns` tags for the methods (e.g., getLocalPonderIndexingMetrics, buildLocalPonderIndexingMetrics, and the method returning local chain indexing metrics) so the summary remains as the primary description and the `@returns` tag is omitted when it only restates that summary; keep any `@returns` only if it adds distinct return-type/details beyond the method summary.apps/ensindexer/src/lib/indexing-status-builder/indexing-status-builder.ts (2)
189-219: 🧹 Nitpick | 🔵 TrivialConsider parallelizing per-chain block-ref fetches.
The current implementation processes chains sequentially with
awaitinside aforloop. For multi-chain setups, this serializes independent RPC calls unnecessarily. UsingPromise.allwould improve performance.♻️ Parallel fetch with Promise.all
private async fetchChainsIndexingBlockRefs( localChainsIndexingMetrics: Map<ChainId, LocalChainIndexingMetrics>, ): Promise<Map<ChainId, ChainIndexingBlockRefs>> { - const chainsIndexingBlockRefs = new Map<ChainId, ChainIndexingBlockRefs>(); - - for (const [chainId, chainIndexingMetric] of localChainsIndexingMetrics.entries()) { - let backfillEndBlock: BlockNumber | null = null; - - if (chainIndexingMetric.state === ChainIndexingStates.Historical) { - backfillEndBlock = chainIndexingMetric.backfillEndBlock; - } - - const indexedBlockrange = this.localPonderClient.getIndexedBlockrange(chainId); - - if (!indexedBlockrange) { - throw new Error(`Indexed blockrange not found for chain ID ${chainId}`); - } - - const { startBlock, endBlock = null } = indexedBlockrange; - - const chainIndexingBlockRefs = await this.fetchChainIndexingBlockRefs(chainId, { - startBlock, - endBlock, - backfillEndBlock, - }); - - chainsIndexingBlockRefs.set(chainId, chainIndexingBlockRefs); - } - - return chainsIndexingBlockRefs; + const entries = Array.from(localChainsIndexingMetrics.entries()); + + const results = await Promise.all( + entries.map(async ([chainId, chainIndexingMetric]) => { + const backfillEndBlock = + chainIndexingMetric.state === ChainIndexingStates.Historical + ? chainIndexingMetric.backfillEndBlock + : null; + + const indexedBlockrange = this.localPonderClient.getIndexedBlockrange(chainId); + + if (!indexedBlockrange) { + throw new Error(`Indexed blockrange not found for chain ID ${chainId}`); + } + + const { startBlock, endBlock = null } = indexedBlockrange; + + const chainIndexingBlockRefs = await this.fetchChainIndexingBlockRefs(chainId, { + startBlock, + endBlock, + backfillEndBlock, + }); + + return [chainId, chainIndexingBlockRefs] as const; + }), + ); + + return new Map(results); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ensindexer/src/lib/indexing-status-builder/indexing-status-builder.ts` around lines 189 - 219, fetchChainsIndexingBlockRefs is currently awaiting fetchChainIndexingBlockRefs inside a for-loop, causing sequential RPCs; convert this to parallel by mapping localChainsIndexingMetrics.entries() to an array of promises that call fetchChainIndexingBlockRefs for each chain (compute per-entry backfillEndBlock using ChainIndexingStates.Historical and localPonderClient.getIndexedBlockrange to get startBlock/endBlock), then await Promise.all on that array and populate chainsIndexingBlockRefs from the resolved results before returning. Ensure errors from missing indexedBlockrange still throw as before and preserve ChainId-to-ChainIndexingBlockRefs mapping.
115-124: 🧹 Nitpick | 🔵 TrivialRemove redundant
@returnstags that merely restate the method summary.Per coding guidelines, several
@returnstags duplicate information already in the method summary:
- Line 122: "The indexing status snapshot for the chain"
- Line 230: "The block references for the specified chain"
- Line 258: "The block reference for the specified block number..."
🔧 Suggested fix for line 122
* `@param` chainIndexingStatus - The Ponder indexing status for the chain. * `@param` chainIndexingBlockRefs - The block references for the chain. * - * `@returns` The indexing status snapshot for the chain. * `@throws` Error if validation of the built snapshot fails. */As per coding guidelines, Do not add JSDoc
@returnstags that merely restate the method summary; remove redundancy during PR review.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ensindexer/src/lib/indexing-status-builder/indexing-status-builder.ts` around lines 115 - 124, Remove the redundant JSDoc `@returns` tags that simply restate each method summary: delete the `@returns` line in the JSDoc for the "Build Chain Indexing Status Snapshot for a single indexed chain" method (the buildChainIndexingStatusSnapshot doc block), and also remove the duplicate `@returns` lines in the doc blocks that describe "The block references for the specified chain" and "The block reference for the specified block number" (the block references / block reference methods in indexing-status-builder.ts); keep the concise summary and type annotations but eliminate the repeated descriptive `@returns` sentences so the docs aren’t duplicative.
🤖 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-status-builder/indexing-status-builder.ts`:
- Around line 160-172: The code in the Historical branch of the switch in
indexing-status-builder.ts validates that
chainIndexingBlockRefs.backfillEndBlock is present and throws otherwise, so
remove the redundant null coalescing (the "?? null") when constructing the
payload for validateChainIndexingStatusSnapshot; pass
chainIndexingBlockRefs.backfillEndBlock directly into the backfillEndBlock field
of the object passed to validateChainIndexingStatusSnapshot (the case handling
ChainIndexingStates.Historical and the validateChainIndexingStatusSnapshot call
should be updated accordingly).
In `@apps/ensindexer/src/lib/plugin-helpers.ts`:
- Around line 143-154: Replace the manual loop in
getPluginsRequiredDatasourceNames with a concise Map construction: map
pluginNames via getPlugin to an array of [plugin.name,
plugin.requiredDatasourceNames] entries and pass that array into new Map(...) to
produce the Map<PluginName, DatasourceName[]>; keep the same types and use the
existing getPlugin, PluginName, DatasourceName, plugin.name and
plugin.requiredDatasourceNames symbols.
In `@packages/ensnode-sdk/src/shared/blockrange.test.ts`:
- Around line 33-40: Add a unit test for mergeBlockranges that covers the case
where both input Blockrange objects have undefined startBlock values: create two
ranges like rangeA/endBlock and rangeB/endBlock (no startBlock), call
mergeBlockranges(rangeA, rangeB) and assert the result has startBlock: undefined
and endBlock equal to the later endBlock (e.g., rangeB.endBlock); place this
alongside the existing tests referencing mergeBlockranges to ensure the
open-start + open-start merge behavior is validated.
In `@packages/ensnode-sdk/src/shared/blockrange.ts`:
- Around line 3-10: The JSDoc for the merge function contains a redundant
`@returns` tag; edit the comment block above the mergeBlockRanges (or similarly
named) function in blockrange.ts to remove the "@returns The merged blockrange."
line while keeping the summary and `@param` entries intact so the description and
parameter docs remain but the redundant return sentence is gone.
In `@packages/ponder-sdk/src/local-ponder-client.mock.ts`:
- Around line 17-32: The defaults for indexedChainIds, chainsBlockrange, and
cachedPublicClients intentionally differ (indexedChainIds excludes Base while
chainsBlockrange/cachedPublicClients include it) to exercise LocalPonderClient's
filtering; add a concise inline comment near the indexedChainIds declaration
explaining this intentional mismatch and its purpose (to ensure tests cover
filtering behavior), and optionally mirror that note near
chainsBlockrange/cachedPublicClients so future readers understand the test
intent rather than assuming a mistake.
---
Duplicate comments:
In
`@apps/ensindexer/src/lib/indexing-status-builder/indexing-status-builder.test.ts`:
- Around line 252-262: The test's Realtime mock (localMetricsRealtime) includes
historical-only properties which is misleading; update the map entry used with
buildLocalChainsIndexingMetrics so the value for chainId uses only the Realtime
shape (state: ChainIndexingStates.Realtime and latestSyncedBlock) and remove
historicalTotalBlocks and backfillEndBlock so it truly satisfies
ChainIndexingMetricsRealtime (refer to the existing chainId,
ChainIndexingStates.Realtime, ChainIndexingMetricsRealtime and
buildLocalChainsIndexingMetrics identifiers to locate the mock).
In `@apps/ensindexer/src/lib/indexing-status-builder/indexing-status-builder.ts`:
- Around line 189-219: fetchChainsIndexingBlockRefs is currently awaiting
fetchChainIndexingBlockRefs inside a for-loop, causing sequential RPCs; convert
this to parallel by mapping localChainsIndexingMetrics.entries() to an array of
promises that call fetchChainIndexingBlockRefs for each chain (compute per-entry
backfillEndBlock using ChainIndexingStates.Historical and
localPonderClient.getIndexedBlockrange to get startBlock/endBlock), then await
Promise.all on that array and populate chainsIndexingBlockRefs from the resolved
results before returning. Ensure errors from missing indexedBlockrange still
throw as before and preserve ChainId-to-ChainIndexingBlockRefs mapping.
- Around line 115-124: Remove the redundant JSDoc `@returns` tags that simply
restate each method summary: delete the `@returns` line in the JSDoc for the
"Build Chain Indexing Status Snapshot for a single indexed chain" method (the
buildChainIndexingStatusSnapshot doc block), and also remove the duplicate
`@returns` lines in the doc blocks that describe "The block references for the
specified chain" and "The block reference for the specified block number" (the
block references / block reference methods in indexing-status-builder.ts); keep
the concise summary and type annotations but eliminate the repeated descriptive
`@returns` sentences so the docs aren’t duplicative.
In `@packages/ponder-sdk/src/local-ponder-client.ts`:
- Around line 43-67: Mark the three private fields that are only assigned in the
constructor as readonly to express immutability and prevent future reassignment:
change the declarations of indexedChainIds, indexedBlockranges, and
cachedPublicClients in the LocalPonderClient class to private readonly
indexedChainIds: Set<ChainId>, private readonly indexedBlockranges: Map<ChainId,
BlockrangeWithStartBlock>, and private readonly cachedPublicClients:
Map<ChainId, CachedPublicClient>; keep the constructor assignments as-is (no
other logic changes required).
- Around line 145-150: The JSDoc for methods in
packages/ponder-sdk/src/local-ponder-client.ts contains redundant `@returns` lines
that repeat the summary; remove the redundant `@returns` tags for the methods
(e.g., getLocalPonderIndexingMetrics, buildLocalPonderIndexingMetrics, and the
method returning local chain indexing metrics) so the summary remains as the
primary description and the `@returns` tag is omitted when it only restates that
summary; keep any `@returns` only if it adds distinct return-type/details beyond
the method summary.
ℹ️ Review info
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (20)
.changeset/fifty-dingos-send.md.changeset/orange-windows-say.mdapps/ensindexer/ponder/src/api/handlers/ensnode-api.tsapps/ensindexer/src/lib/clients.tsapps/ensindexer/src/lib/indexing-status-builder/indexing-status-builder.mock.tsapps/ensindexer/src/lib/indexing-status-builder/indexing-status-builder.test.tsapps/ensindexer/src/lib/indexing-status-builder/indexing-status-builder.tsapps/ensindexer/src/lib/plugin-helpers.tspackages/ensnode-sdk/src/index.tspackages/ensnode-sdk/src/shared/blockrange.test.tspackages/ensnode-sdk/src/shared/blockrange.tspackages/ensnode-sdk/src/shared/config/indexed-blockranges.test.tspackages/ensnode-sdk/src/shared/config/indexed-blockranges.tspackages/ponder-sdk/src/cached-public-client.tspackages/ponder-sdk/src/deserialize/chains.tspackages/ponder-sdk/src/indexing-blocks.tspackages/ponder-sdk/src/local-indexing-metrics.tspackages/ponder-sdk/src/local-ponder-client.mock.tspackages/ponder-sdk/src/local-ponder-client.test.tspackages/ponder-sdk/src/local-ponder-client.ts
apps/ensindexer/src/lib/indexing-status-builder/indexing-status-builder.ts
Show resolved
Hide resolved
apps/ensindexer/src/lib/indexing-status-builder/indexing-status-builder.ts
Show resolved
Hide resolved
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Lite PR
Tip: Review docs on the ENSNode PR process
Summary
LocalPonderClientclass definitionIndexingStatusBuilderclass, wrappingLocalPonderClientinstance and making RPC calls (only once, cached afterwards).IndexingStatusBuilderinto Indexing Status API for ENSIndexer.Why
Testing
Notes for Reviewer (Optional)
Indexing Status Buildermodule #1613.Pre-Review Checklist (Blocking)
This PR does not introduce significant changes and is low-risk to review quickly.