Local Ponder Client & Indexing Status Builder to power ENSIndexer#1675
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: 7e6d97a 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 |
|
The latest updates on your projects. Learn more about Vercel for GitHub. 3 Skipped Deployments
|
|
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}
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
Outdated
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}
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
Outdated
Show resolved
Hide resolved
packages/ensnode-sdk/src/indexing-status/omnichain-indexing-status-snapshot.ts
Show resolved
Hide resolved
|
@greptile review |
Greptile SummaryThis PR introduces a well-architected Ponder SDK with Key Changes:
Architecture: Code Quality: Confidence Score: 4/5
Important Files Changed
Last reviewed commit: 6c47611 |
![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
|
@greptile review |
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
Outdated
Show resolved
Hide resolved
apps/ensindexer/src/lib/indexing-status-builder/indexing-status-builder.ts
Outdated
Show resolved
Hide resolved
tk-o
force-pushed
the
local-ponder-client-2.0
branch
from
a4c5c86 to
2b55ec5
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 33 out of 34 changed files in this pull request and generated 4 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
|
@greptile review |
tk-o
left a comment
tk-o
left a comment
There was a problem hiding this comment.
Self-review completed.
|
|
||
| try { | ||
| omnichainSnapshot = await buildOmnichainIndexingStatusSnapshot(publicClients); | ||
| omnichainSnapshot = await indexingStatusBuilder.getOmnichainIndexingStatusSnapshot(); |
There was a problem hiding this comment.
Soon, only the ENSDb Writer Worker will call the IndexingStatusBuilder instance.
| * | ||
| * @param pluginNames - Names of the plugins to retrieve required datasource names for. | ||
| */ | ||
| export function getPluginsRequiredDatasourceNames( |
There was a problem hiding this comment.
Useful for creating constructor params for LocalPonderClient class.
| export function buildIndexedBlockranges( | ||
| namespace: ENSNamespaceId, | ||
| pluginsRequiredDatasourceNames: Map<PluginName, DatasourceName[]>, | ||
| ): Map<ChainId, BlockNumberRangeWithStartBlock> { |
There was a problem hiding this comment.
Uses only ENSNode SDK and datasources abstractions now.
| /** | ||
| * Deserialize an unvalidated string representation of a chain ID. | ||
| */ | ||
| export function deserializeChainId(unvalidatedData: Unvalidated<ChainIdString>): ChainId { |
There was a problem hiding this comment.
Used when parsing raw input returned from Ponder API.
| * | ||
| * References config applied when indexing a given chain. | ||
| */ | ||
| export interface ChainIndexingConfig { |
There was a problem hiding this comment.
This interface includes indexed block reg range for the chain, and also, if applicable, the block ref for the backfill end block.
lightwalker-eth
left a comment
lightwalker-eth
left a comment
There was a problem hiding this comment.
@tk-o Looks good 👍 Happy with the improved execution. Shared some small comments. Please take the lead to merge when ready
| pluginsRequiredDatasourceNames, | ||
| ); | ||
|
|
||
| export const localPonderClient = new LocalPonderClient( |
There was a problem hiding this comment.
Should this file be renamed from clients.ts to something like local-ponder-client.ts?
| /** | ||
| * Local Ponder Client | ||
| * | ||
| * It is a specialized client for interacting with a local Ponder app instance. |
There was a problem hiding this comment.
The big idea isn't being communicated here correctly yet. What makes a Local Ponder Client "local" is because it has access to state through in-memory Ponder library imports in addition to Ponder's external APIs.
There was a problem hiding this comment.
Thanks for sharing that example 👍
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 33 out of 34 changed files in this pull request and generated 5 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.
| export const schemaChainIdString = z | ||
| .string({ error: `Value must be a string representing a chain ID.` }) | ||
| .check(invariant_chainIdStringRepresentsValidChainId) | ||
| .pipe(z.preprocess((v) => Number(v), schemaChainId)); | ||
| .check(invariant_chainIdStringRepresentsValidChainId); |
There was a problem hiding this comment.
schemaChainIdString no longer validates that the string represents a valid ChainId per schemaChainId (positive integer). As written, values like "0", "-1", "1.5", or "NaN" will pass the .check(...), which weakens validation in other call sites (e.g. metrics label validation). Consider adding an additional check/refinement that Number(value) successfully parses and passes schemaChainId, while still keeping deserializeChainId() as the conversion helper.
| export function mergeBlockNumberRanges(...ranges: BlockNumberRange[]): BlockNumberRange { | ||
| if (ranges.length === 0) { | ||
| return buildBlockNumberRange(undefined, undefined); | ||
| } | ||
|
|
||
| let minStartBlock: BlockNumber | undefined; | ||
| let maxEndBlock: BlockNumber | undefined; | ||
|
|
||
| for (const range of ranges) { | ||
| // Update min start block (lower values win, undefined is ignored) | ||
| if (range.startBlock !== undefined) { | ||
| if (minStartBlock === undefined || range.startBlock < minStartBlock) { | ||
| minStartBlock = range.startBlock; | ||
| } | ||
| } | ||
|
|
||
| // Update max end block (higher values win, undefined is ignored) | ||
| if (range.endBlock !== undefined) { | ||
| if (maxEndBlock === undefined || range.endBlock > maxEndBlock) { | ||
| maxEndBlock = range.endBlock; | ||
| } | ||
| } | ||
| } | ||
|
|
||
| return buildBlockNumberRange(minStartBlock, maxEndBlock); | ||
| } |
There was a problem hiding this comment.
mergeBlockNumberRanges can throw via buildBlockNumberRange(minStartBlock, maxEndBlock) when combining a left-bounded range with a right-bounded range (or any mix that yields minStartBlock > maxEndBlock), despite the docstring saying it returns a merged range that covers all inputs. Consider treating an undefined start/end as unbounded in that direction (e.g., if any input has startBlock === undefined, keep merged startBlock undefined; similarly for endBlock) or explicitly handling the minStartBlock > maxEndBlock case with a well-defined outcome (return unbounded or throw with a clearer error + docs update).
| export function mergeBlockNumberRanges(...ranges: BlockNumberRange[]): BlockNumberRange { | ||
| if (ranges.length === 0) { | ||
| return buildBlockNumberRange(undefined, undefined); | ||
| } | ||
|
|
||
| let minStartBlock: BlockNumber | undefined; | ||
| let maxEndBlock: BlockNumber | undefined; | ||
|
|
||
| for (const range of ranges) { | ||
| // Update min start block (lower values win, undefined is ignored) | ||
| if (range.startBlock !== undefined) { | ||
| if (minStartBlock === undefined || range.startBlock < minStartBlock) { | ||
| minStartBlock = range.startBlock; | ||
| } | ||
| } | ||
|
|
||
| // Update max end block (higher values win, undefined is ignored) | ||
| if (range.endBlock !== undefined) { | ||
| if (maxEndBlock === undefined || range.endBlock > maxEndBlock) { | ||
| maxEndBlock = range.endBlock; | ||
| } | ||
| } | ||
| } | ||
|
|
||
| return buildBlockNumberRange(minStartBlock, maxEndBlock); | ||
| } |
There was a problem hiding this comment.
Same issue as in ponder-sdk: mergeBlockNumberRanges can throw if called with a left-bounded range and a right-bounded range (or any mix producing minStartBlock > maxEndBlock), which contradicts the function docs that imply it always returns a covering merged range. Consider treating undefined bounds as unbounded (if any input is unbounded on a side, the merged range must be too) or explicitly handling the minStartBlock > maxEndBlock case with a defined behavior and matching docs/tests.
| @@ -1,3 +1,4 @@ | |||
| import type { RangeTypeIds } from "../shared/blockrange"; | |||
There was a problem hiding this comment.
RangeTypeIds is imported but only referenced in JSDoc. With Biome's recommended rules (including unused import checks), this is likely to be reported as an unused import. Consider removing the import, or (if you need the symbol for docs) adding a targeted Biome ignore comment for unused imports on this line.
| expect(publicClientMock.getBlock).toHaveBeenCalledTimes(2); // RPC calls for startBlock, and backfillEndBlock | ||
| }); | ||
|
|
||
| it("retries fetching block refs when RPC call fails", async () => { |
There was a problem hiding this comment.
This test name suggests the builder performs an internal retry, but the behavior under test appears to be: first call fails, second call succeeds because _immutableIndexingConfig wasn't cached and the whole fetch is attempted again. Consider renaming the test to reflect the actual behavior (e.g. "does not cache partial results and allows a subsequent call to retry").
| it("retries fetching block refs when RPC call fails", async () => { | |
| it("does not cache partial results and allows a subsequent call to retry", async () => { |
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.