feat(ensindexer): leverage SWR cache to achieve goals related to LocalPonderClient#1652
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub. 3 Skipped Deployments
|
🦋 Changeset detectedLatest commit: 11c2c23 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 |
There was a problem hiding this comment.
Pull request overview
This PR refactors ENSIndexer’s LocalPonderClient initialization to rely on SWR caches (with proactive revalidation) and extends the shared SWRCache utility to support a context object for cache loaders.
Changes:
- Extend
SWRCacheto accept an optionalcontextparameter infn()and add asetContext()API. - Refactor
LocalPonderClientto use new SWR-backed caches for Ponder indexing metrics/status (dynamic) and derived immutable chain metadata. - Update the Ponder API handler to use a singleton
LocalPonderClientinstance (no per-request initialization).
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/ensnode-sdk/src/shared/cache/swr-cache.ts | Adds context support and proactive revalidation stop API to the shared SWR cache implementation. |
| packages/ensnode-sdk/src/shared/cache/swr-cache.test.ts | Updates tests for the new (cachedResult, context) callback signature and adds context coverage. |
| apps/ensindexer/src/lib/ponder-api-client.ts | Switches singleton getter to synchronous construction and injects SWR caches into LocalPonderClient. |
| apps/ensindexer/ponder/src/api/lib/local-ponder-client.ts | Reworks LocalPonderClient to extend PonderClient and source metadata via SWR caches. |
| apps/ensindexer/ponder/src/api/lib/chains-indexing-metadata-immutable.ts | Introduces builder for immutable chain indexing metadata. |
| apps/ensindexer/ponder/src/api/lib/chains-indexing-metadata-dynamic.ts | Introduces builder for dynamic chain indexing metadata from metrics/status. |
| apps/ensindexer/ponder/src/api/lib/cache/ponder-client.cache.ts | Adds SWR cache for Ponder metrics/status with proactive revalidation. |
| apps/ensindexer/ponder/src/api/lib/cache/chains-indexing-metadata-immutable.cache.ts | Adds SWR cache for immutable chain metadata and stops revalidation after success. |
| apps/ensindexer/ponder/src/api/handlers/ensnode-api.ts | Uses a module-level singleton LocalPonderClient for request handling. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![vercel[bot]](https://avatars.githubusercontent.com/in/8329?s=60&v=4){kind=small}
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ 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 |
tk-o
force-pushed
the
feat/indexing-status-builder-3
branch
from
34db972 to
b8b2f19
Compare
tk-o
force-pushed
the
feat/indexing-status-builder-3-with-cache
branch
from
0999394 to
d314b5d
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 10 out of 10 changed files in this pull request and generated 6 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Move logic from `LocalPonderClient` into a separate file.
Move logic from `LocalPonderClient` into a separate file.
tk-o
force-pushed
the
feat/indexing-status-builder-3-with-cache
branch
from
9f9ca10 to
dc578eb
Compare
lightwalker-eth
left a comment
lightwalker-eth
left a comment
There was a problem hiding this comment.
@tk-o Thanks for these updates 👍 Reviewed and shared some feedback with suggestions. Please feel welcome to merge when ready.
| // Initialize the singleton LocalPonderClient instance if it hasn't been | ||
| // initialized yet. | ||
| if (localPonderClient === undefined) { | ||
| localPonderClient = new LocalPonderClient(config.ensIndexerUrl, config.indexedChainIds); |
There was a problem hiding this comment.
Why create it here? Why not define localPonderClient as a const and move this logic next to the definition of localPonderClient?
And likewise, why have a getLocalPonderClient function at all? Why not just have people import localPonderClient and make direct use of it?
There was a problem hiding this comment.
I'm not a big fan of eager evaluation, as it makes it harder to test code, and also harder to run code in other contexts than server rutime. Referring here to our latest issue with creating OpenAPI spec where config object is eagerly evaluated, forcing us to find workarounds.
That's why I prefer lazy evaluation, and one way to achieve it is through factory functions.
There was a problem hiding this comment.
Answering your question directly, we can define localPonderClient as a const and move it next to definition of LocalPonderClient in the following file:
apps/ensindexer/ponder/src/api/lib/local-ponder-client.ts
| * connecting to the local Ponder app and fetching the necessary data to | ||
| * build the client's state. The initialized client is cached and returned | ||
| * on subsequent calls. | ||
| * This function relies on SWR caches with proactive revalidation to load |
There was a problem hiding this comment.
Why is there any references to SWR caches here? It doesn't seem relevant at this layer of responsibility.
| indexedChainIds, | ||
| chainIndexingMetadataImmutable, | ||
| ); | ||
| const ponderClientCacheResult = await ponderClientCache.read(); |
There was a problem hiding this comment.
I'm not clear what we're trying to do here?
The variable names are confusing. A few lines down it says "indexing metrics" and then later "Ponder client data ..."
What is it exactly? So many names that aren't aligned.
| get indexedChainIds() { | ||
| return this.#indexedChainIds; | ||
| } | ||
| // Build and cache immutable metadata for indexed chains if not already cached. |
There was a problem hiding this comment.
It's difficult for me to wrap my head around what's going on here.
Is this a possible asynchronous fetch operation? Or is it just in memory data manipulation?
Maybe this operation better belongs wherever we process receiving a new ponderIndexingMetrics?
There was a problem hiding this comment.
I'll update the SWR cache to store, both types of chain indexing metadata. Then, LocalPonderClient will simply read these values from cache.
| publicClient(chainId: ChainId): PublicClient { | ||
| const publicClient = this.#publicClients.get(chainId); | ||
| try { | ||
| chainsIndexingMetadataImmutable = await this.#chainsIndexingMetadataImmutable; |
There was a problem hiding this comment.
Why try? Why await? I'm confused. I thought this should just be reading a simple in-memory value?
There was a problem hiding this comment.
I'll update the SWR cache to store, both types of chain indexing metadata. Then, LocalPonderClient will simply read these values from cache.
| */ | ||
| export async function buildChainsIndexingMetadataImmutable( | ||
| indexedChainIds: Set<ChainId>, | ||
| chainsConfigBlockrange: Map<ChainId, BlockrangeWithStartBlock>, |
There was a problem hiding this comment.
Why does this function need to be passed the indexedChainIds param?
It seems redundant with chainsConfigBlockrange and publicClients -- are these not going to have exactly the same set of chainIds?
There was a problem hiding this comment.
We can't always trust that Ponder metrics will come complete for every indexed chain. There were cases in the past where Ponder metrics generation had a bug and made the metrics incomplete. That's why we need to use indexedChainIds value coming from ENSIndexer config. Invariants included in the inside the for loop are ultimate test checking ponderIndexingMetrics are complete in the context of indexedChainIds.
| /** | ||
| * Result of the Ponder Client cache. | ||
| */ | ||
| export interface PonderClientCacheResult { | ||
| ponderIndexingMetrics: PonderIndexingMetrics; | ||
| ponderIndexingStatus: PonderIndexingStatus; | ||
| } | ||
|
|
||
| /** | ||
| * SWR Cache for Ponder Client data | ||
| */ | ||
| export type PonderClientCache = SWRCache<PonderClientCacheResult>; | ||
|
|
||
| const ponderClient = new PonderClient(config.ensIndexerUrl); | ||
|
|
||
| /** | ||
| * Cache for Ponder Client data |
There was a problem hiding this comment.
| /** | |
| * Result of the Ponder Client cache. | |
| */ | |
| export interface PonderClientCacheResult { | |
| ponderIndexingMetrics: PonderIndexingMetrics; | |
| ponderIndexingStatus: PonderIndexingStatus; | |
| } | |
| /** | |
| * SWR Cache for Ponder Client data | |
| */ | |
| export type PonderClientCache = SWRCache<PonderClientCacheResult>; | |
| const ponderClient = new PonderClient(config.ensIndexerUrl); | |
| /** | |
| * Cache for Ponder Client data | |
| /** | |
| * Metadata from a Ponder app. | |
| */ | |
| export interface PonderAppMetadata { | |
| ponderIndexingMetrics: PonderIndexingMetrics; | |
| ponderIndexingStatus: PonderIndexingStatus; | |
| } | |
| /** | |
| * SWR Cache for Ponder App Metadata | |
| */ | |
| export type PonderAppMetadataCache = SWRCache<PonderAppMetadata>; | |
| const ponderClient = new PonderClient(config.ensIndexerUrl); | |
| /** | |
| * Cache for Ponder App Metadata |
Goal: Improve terminology so it's more clear whats going on here.
We aren't caching things from the ponder client. The client isn't the origin of this data. We are caching things from the ponder app.
| * consistency to call all endpoints at once. This cache loads both | ||
| * Ponder Indexing Metrics and Ponder Indexing Status together, and provides | ||
| * them as a single cached result. This way, we ensure that the metrics and | ||
| * status data are always from the same point in time, and avoid potential |
There was a problem hiding this comment.
| * status data are always from the same point in time, and avoid potential | |
| * status data are always from (approximately) the same point in time, and minimize potential |
| } | ||
| }, | ||
| ttl: Number.POSITIVE_INFINITY, | ||
| proactiveRevalidationInterval: 10 satisfies Duration, |
There was a problem hiding this comment.
I assume this should be more frequent: Ex: Every 1 second? We want the indexing metrics to be more frequently updating.
| // This ensures that the client is ready to use when handling requests, | ||
| // and allows us to catch initialization errors early. | ||
| getLocalPonderClient(); | ||
| // Get the local Ponder Client instance |
There was a problem hiding this comment.
These ideas don't belong at this layer of responsibility.
Keep the singleton instance next to the `LocalPonderClient` definition.
This will support direct imports inside the Ponder App Metadata cache.
…a for each indexed chain This will allow the cache consumers to simply await the cache read and use the ChainIndexingMetadata object with no need for further refinements.
The client reads data from Ponder App Metadata cache.
tk-o
changed the title
LocalPonderClientLocalPonderClient
|
@greptile review |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Lite PR
Tip: Review docs on the ENSNode PR process
Summary
PonderAppMetadatavalueLocalPonderClientto read data from aforementioned SWR cache to simplify how possible data loading failures are handled.localPonderClientsingletonWhy
LocalPonderClientshould leverage it.Testing
/api/indexing-statusendpoint, including test logsNotes for Reviewer (Optional)
LocalPonderClientidea in a meaningful way.LocalPonderClient"knows" about Ponder APIs and configs, and allows caching data that can be later used to handle Indexing Status API requests.Pre-Review Checklist (Blocking)