Conversation
🦋 Changeset detectedLatest commit: d51b2ea 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:
📝 WalkthroughWalkthroughCentralized the ENSRainbow API client into a process-wide singleton, removed the factory, added a memoized readiness check with retries, and made onchain event handlers await ENSRainbow readiness (setup events bypass wait). Tests updated to mock and assert the new async precondition behavior. Changes
Sequence Diagram(s)sequenceDiagram
participant Ponder as Ponder Event System
participant Precond as Event Preconditions
participant ENS as ENSRainbow Client
participant Retry as p-retry
participant Handler as Registered Event Handler
Ponder->>Precond: eventHandlerPreconditions(eventName)
alt Setup Event (ends with :setup)
Precond->>Handler: invoke handler immediately
Handler-->>Precond: result
else Onchain Event
Precond->>ENS: waitForEnsRainbowToBeReady()
ENS->>Retry: perform health() with retries (up to 12)
loop retry attempts
Retry->>ENS: client.health()
ENS-->>Retry: success / error
Retry-->>Retry: log & decide retry
end
alt Ready
Precond->>Handler: invoke handler
Handler-->>Precond: result
else Exhausted
Retry-->>Precond: reject with Error (cause)
Precond-->>Ponder: promise rejects (handler not invoked)
end
end
Precond-->>Ponder: precondition resolved/rejected
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
@greptile review |
![vercel[bot]](https://avatars.githubusercontent.com/in/8329?s=60&v=4){kind=small}
There was a problem hiding this comment.
Pull request overview
Adds an ENSRainbow client singleton and introduces an indexing “precondition” layer so onchain Ponder handlers wait for ENSRainbow readiness before executing, improving resiliency when ENSRainbow is temporarily unavailable.
Changes:
- Introduced
ensRainbowClientsingleton +waitForEnsRainbowToBeReady()with shared retry/backoff. - Added event-handler preconditions in the Ponder indexing wrapper to block onchain handlers until ENSRainbow is ready (setup handlers bypass this).
- Updated ENSRainbow client call sites and expanded unit tests to cover precondition behavior.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| apps/ensindexer/src/lib/public-config-builder/singleton.ts | Switches to the shared ENSRainbow singleton for public config construction. |
| apps/ensindexer/src/lib/indexing-engines/ponder.ts | Adds event-type detection + precondition gating before handler execution. |
| apps/ensindexer/src/lib/indexing-engines/ponder.test.ts | Updates tests for async callback behavior and adds precondition coverage. |
| apps/ensindexer/src/lib/graphnode-helpers.ts | Migrates heal calls to the ENSRainbow singleton client. |
| apps/ensindexer/src/lib/ensrainbow/singleton.ts | New singleton module with readiness-wait helper and retry logging. |
| apps/ensindexer/src/lib/ensraibow-api-client.ts | Removes the old ENSRainbow client factory. |
| .changeset/sharp-moons-shave.md | Declares a minor version bump for the resiliency change. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Greptile SummaryThis PR introduces event handler preconditions for the ENSIndexer's Ponder-based indexing engine, specifically to block onchain event processing until ENSRainbow is ready to serve heal requests. It consolidates the ENSRainbow client into a proper singleton module ( Key changes:
Confidence Score: 5/5Safe to merge; the precondition logic is sound and well-tested, and the only finding is a defensive-coding suggestion All remaining feedback is P2 (style/best-practice). The core logic — singleton creation, retry strategy, setup-event bypass, and error propagation — is correct and thoroughly covered by the new test suite. No P0/P1 issues were found. apps/ensindexer/src/lib/ensrainbow/singleton.ts — minor health-response validation suggestion Important Files Changed
Sequence DiagramsequenceDiagram
participant Ponder
participant addOnchainEventListener
participant eventHandlerPreconditions
participant waitForEnsRainbowToBeReady
participant ensRainbowClient
participant eventHandler
Ponder->>addOnchainEventListener: onchain event fires
addOnchainEventListener->>eventHandlerPreconditions: eventHandlerPreconditions(eventName)
alt eventName ends with :setup
eventHandlerPreconditions-->>addOnchainEventListener: return (no-op)
else onchain event
eventHandlerPreconditions->>waitForEnsRainbowToBeReady: await
alt promise already cached
waitForEnsRainbowToBeReady-->>eventHandlerPreconditions: resolved immediately
else first call
waitForEnsRainbowToBeReady->>ensRainbowClient: health() [pRetry, up to 12x]
loop until ok or retries exhausted
ensRainbowClient-->>waitForEnsRainbowToBeReady: throws (network error) → retry
end
ensRainbowClient-->>waitForEnsRainbowToBeReady: { status: "ok" }
waitForEnsRainbowToBeReady-->>eventHandlerPreconditions: resolved
end
end
eventHandlerPreconditions-->>addOnchainEventListener: precondition met
addOnchainEventListener->>eventHandler: call with IndexingEngineContext
eventHandler-->>addOnchainEventListener: done
Reviews (3): Last reviewed commit: "Merge remote-tracking branch 'origin/mai..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/ensrainbow/singleton.ts`:
- Around line 50-58: The health check currently treats any fulfilled
ensRainbowClient.health() response as success; change the pRetry callback used
for waitForEnsRainbowToBeReadyPromise to parse and validate the response body
and throw unless it exactly matches { status: "ok" } (so pRetry will retry on
unexpected payloads); add import { z } from "zod" near the top or perform an
explicit equality check, validate the response inside the async function that
calls ensRainbowClient.health(), and only resolve/allow the .then() readiness
log when the validation passes (use the identifiers
waitForEnsRainbowToBeReadyPromise, pRetry, ensRainbowClient.health(), and
ensRainbowUrl to locate the code).
In `@apps/ensindexer/src/lib/indexing-engines/ponder.test.ts`:
- Around line 254-256: The test currently asserts await expect(callback({
context: mockContext, event: mockEvent })).resolves.not.toThrow(); but the
callback resolves to void; change the assertion to await expect(callback({
context: mockContext, event: mockEvent })).resolves.toBeUndefined() so the test
explicitly verifies the promise resolves to undefined; update the line using
mockPonderOn.mock.calls[0] and the extracted callback to use
resolves.toBeUndefined() when invoking callback with mockContext and mockEvent
(ENSRainbow-related behavior remains unchanged).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 17de5765-4835-4c7e-b972-b4df11823062
📒 Files selected for processing (7)
.changeset/sharp-moons-shave.mdapps/ensindexer/src/lib/ensraibow-api-client.tsapps/ensindexer/src/lib/ensrainbow/singleton.tsapps/ensindexer/src/lib/graphnode-helpers.tsapps/ensindexer/src/lib/indexing-engines/ponder.test.tsapps/ensindexer/src/lib/indexing-engines/ponder.tsapps/ensindexer/src/lib/public-config-builder/singleton.ts
💤 Files with no reviewable changes (1)
- apps/ensindexer/src/lib/ensraibow-api-client.ts
This function will enbale ENSIndexer modules to wait for when the ENSRainbow instance is ready to serve traffic.
Allows to wait with indexing onchain events until ENSRainbow instance is ready.
…ndler-preconditions
tk-o
force-pushed
the
feat/onchain-event-handler-preconditions
branch
from
9ff175e to
911035a
Compare
|
@greptile review |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 7 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/ensrainbow/singleton.ts`:
- Around line 51-58: The pRetry loop using waitForEnsRainbowToBeReadyPromise
currently awaits ensRainbowClient.health() which can hang indefinitely; wrap
each attempt in a per-attempt timeout (e.g., implement a withTimeout(operation,
timeoutMs) helper) and call pRetry with async () => withTimeout(() =>
ensRainbowClient.health(), 30_000) so that a stalled fetch rejects after the
timeout and allows retries to proceed; ensure the timeout error propagates into
the existing onFailedAttempt logging.
In `@apps/ensindexer/src/lib/indexing-engines/ponder.test.ts`:
- Around line 261-279: Replace the current mockWaitForEnsRainbow implementation
with a deferred promise pattern so you can control when the precondition
resolves; call addOnchainEventListener("Resolver:AddrChanged", testHandler) as
before, grab the callback from mockPonderOn.mock.calls[0], invoke callback({
context: mockContext, event: mockEvent }) but DO NOT resolve the deferred
precondition yet, then assert testHandler has not been called (ensuring handler
did not run before precondition), next resolve the deferred promise and await
the callback completion, and finally assert testHandler was called and
preconditionResolved is true; reference mockWaitForEnsRainbow,
addOnchainEventListener, mockPonderOn, callback, and testHandler when making
these changes.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: c2179a85-b889-4c0d-b23f-1b249266e66c
📒 Files selected for processing (4)
.changeset/sharp-moons-shave.mdapps/ensindexer/src/lib/ensrainbow/singleton.tsapps/ensindexer/src/lib/indexing-engines/ponder.test.tsapps/ensindexer/src/lib/indexing-engines/ponder.ts
lightwalker-eth
left a comment
lightwalker-eth
left a comment
There was a problem hiding this comment.
@tk-o Thanks shared some suggestions.
|
|
||
| console.log(`Waiting for ENSRainbow instance to be ready at '${ensRainbowUrl}'...`); | ||
|
|
||
| waitForEnsRainbowToBeReadyPromise = pRetry(async () => ensRainbowClient.health(), { |
There was a problem hiding this comment.
A few questions:
- Can you confirm that ENSRainbow's healthcheck fails if it's not ready yet? In other words I'm asking about the distinction between health vs ready.
- Can you please confirm if you've rehearsed this operation from a cold start and that all proceeds as expected?
There was a problem hiding this comment.
ENSRainbow health check is also the readiness check.
I have rehearsed this operation in simulated cold start scenario. The relevant ENSIndexer logs were added to this PR description, but I'll paste it here as well. Please note how we:
- Print the log to acknowledge the wait for ENSRainbow instance to be ready:
Waiting for ENSRainbow instance to be ready at 'http://localhost:3223/'.... - Print the log to acknowledge retries for the health check, i.e.:
ENSRainbow health check attempt 1 failed (fetch failed). 12 retries left.. - Print the log to acknowledge that ENSRainbow instance is ready when the health check is successful:
ENSRainbow instance is ready at 'http://localhost:3223/'..
ENSIndexer logs
Running database migrations for ENSNode Schema in ENSDb.
09:41:39.839 INFO Created HTTP server port=42069 (13ms)
09:41:39.839 INFO Started returning 200 responses endpoint=/health
Database migrations for ENSNode Schema in ENSDb completed successfully.
[EnsDbWriterWorker]: Upserting ENSDb version into ENSDb...
[EnsDbWriterWorker]: ENSDb version upserted successfully: 1.9.0
[EnsDbWriterWorker]: Upserting ENSIndexer Public Config into ENSDb...
[EnsDbWriterWorker]: ENSIndexer Public Config upserted successfully
09:41:40.079 INFO Started backfill indexing chain=421614 block_range=[123142726,254959863]
09:41:40.079 INFO Started backfill indexing chain=11155420 block_range=[23770766,41526949]
09:41:40.079 INFO Started backfill indexing chain=11155111 block_range=[3702721,10552128]
09:41:40.079 INFO Started backfill indexing chain=534351 block_range=[8175276,17482817]
09:41:40.079 INFO Started backfill indexing chain=84532 block_range=[13012458,39544075]
09:41:40.079 INFO Started backfill indexing chain=59141 block_range=[2395094,27904262]
09:41:40.080 INFO Started fetching backfill JSON-RPC data chain=421614 cached_block=254959811 cache_rate=100%
09:41:40.080 INFO Started fetching backfill JSON-RPC data chain=11155420 cached_block=41526942 cache_rate=100%
09:41:40.080 INFO Started fetching backfill JSON-RPC data chain=11155111 cached_block=10552126 cache_rate=100%
09:41:40.080 INFO Started fetching backfill JSON-RPC data chain=534351 cached_block=17482815 cache_rate=100%
09:41:40.080 INFO Started fetching backfill JSON-RPC data chain=84532 cached_block=39544015 cache_rate=100%
09:41:40.080 INFO Skipped fetching backfill JSON-RPC data (cache contains all required data) chain=59141 cached_block=27904262 cache_rate=100%
09:41:40.274 INFO Finished fetching backfill JSON-RPC data chain=11155420 (195ms)
09:41:40.295 INFO Finished fetching backfill JSON-RPC data chain=534351 (215ms)
Waiting for ENSRainbow instance to be ready at 'http://localhost:3223/'...
ENSRainbow health check attempt 1 failed (fetch failed). 12 retries left.
09:41:40.368 INFO Finished fetching backfill JSON-RPC data chain=421614 (289ms)
09:41:40.369 INFO Finished fetching backfill JSON-RPC data chain=11155111 (290ms)
09:41:41.100 INFO Finished fetching backfill JSON-RPC data chain=84532 (1s)
ENSRainbow health check attempt 2 failed (fetch failed). 11 retries left.
09:41:45.079 INFO Updated backfill indexing progress progress=0.0%
ENSRainbow health check attempt 3 failed (fetch failed). 10 retries left.
ENSRainbow health check attempt 4 failed (fetch failed). 9 retries left.
09:41:50.078 INFO Updated backfill indexing progress progress=0.0%
09:41:55.078 INFO Updated backfill indexing progress progress=0.0%
ENSRainbow instance is ready at 'http://localhost:3223/'.
09:41:58.671 INFO Indexed block range chain=11155111 event_count=2872 block_range=[3702721,4095260] (18s)
09:41:59.200 INFO Indexed block range chain=11155111 event_count=2892 block_range=[4095261,4212765] (526ms)
09:41:59.836 INFO Indexed block range chain=11155111 event_count=2758 block_range=[4212766,4282224] (633ms)
09:42:00.079 INFO Updated backfill indexing progress progress=9.3%
| console.log(`Waiting for ENSRainbow instance to be ready at '${ensRainbowUrl}'...`); | ||
|
|
||
| waitForEnsRainbowToBeReadyPromise = pRetry(async () => ensRainbowClient.health(), { | ||
| retries: 12, // This allows for a total of over 1 hour of retries with the exponential backoff strategy |
There was a problem hiding this comment.
Please document why 12 retries translates into over 1 hour of retries?
| retries: 12, // This allows for a total of over 1 hour of retries with the exponential backoff strategy | ||
| onFailedAttempt: ({ error, attemptNumber, retriesLeft }) => { | ||
| console.log( | ||
| `ENSRainbow health check attempt ${attemptNumber} failed (${error.message}). ${retriesLeft} retries left.`, |
There was a problem hiding this comment.
Perhaps include the ensRainbowUrl in these error messages also.
| waitForEnsRainbowToBeReadyPromise = pRetry(async () => ensRainbowClient.health(), { | ||
| retries: 12, // This allows for a total of over 1 hour of retries with the exponential backoff strategy | ||
| onFailedAttempt: ({ error, attemptNumber, retriesLeft }) => { | ||
| console.log( |
There was a problem hiding this comment.
Maybe error or warn instead of log?
| * store the current Indexing Status in ENSDb. | ||
| */ | ||
| return; | ||
| case EventTypeIds.Onchain: { |
There was a problem hiding this comment.
Suggest to document how we can support blocking preconditions here.
There was a problem hiding this comment.
I've added JSDoc examples for prepareIndexingActivation function above 👍
| context: buildIndexingEngineContext(context), | ||
| event, | ||
| }), | ||
| return ponder.on(eventName, async ({ context, event }) => |
There was a problem hiding this comment.
We should optimize the logic here to consume as little CPU as possible as it's a very hot code path.
Ex: We shouldn't be parsing the eventName into an eventType more than once for each call to addOnchainEventListener.
There was a problem hiding this comment.
I've created a perf test and here are the results:
Performance benchmark:
- Cached: 2.58ms for 100k iterations
- Uncached: 1.03ms for 100k iterations
- Result: Uncached is ~2.5x faster, confirming the cache adds unnecessary overhead for this simple operation. A single endsWith(":setup") call is so fast that the Map lookup overhead (hash computation, bucket traversal) makes caching counterproductive.
Based on that, I suggest we don't change the buildEventTypeId function.
Cached option
/**
* Cache for event type IDs to avoid recomputing them for every event.
* The cache is a simple in-memory Map that stores the mapping from event names
* to their corresponding event type IDs.
*/
const buildEventTypeIdCache = new Map<EventNames, EventTypeId>();
export function buildEventTypeId(eventName: EventNames): EventTypeId {
if (buildEventTypeIdCache.has(eventName)) {
return buildEventTypeIdCache.get(eventName) as EventTypeId;
}
const result = eventName.endsWith(":setup") ? EventTypeIds.Setup : EventTypeIds.Onchain;
buildEventTypeIdCache.set(eventName, result);
return result;
}Uncached option
function buildEventTypeId(eventName: EventNames): EventTypeId {
if (eventName.endsWith(":setup")) {
return EventTypeIds.Setup;
} else {
return EventTypeIds.Onchain;
}
}| */ | ||
| return; | ||
| case EventTypeIds.Onchain: { | ||
| return await waitForEnsRainbowToBeReady(); |
There was a problem hiding this comment.
Hmm. I'm not so clear on the strategy here of calling return await ....
I assumed we would instead do something like:
- Define simple booleans for
preparedIndexingSetupandpreparedIndexedActivationthat would both be initialized to false. - Define a function for
prepareIndexingSetup. It would be called oncase EventTypeIds.Setup. For now it would be an empty function. Once it returns the first time it would setpreparedIndexingSetupto true. - Define a function for
prepareIndexingActivation. It would be called oncase EventTypeIds. Onchain. For now it would just callwaitForEnsRainbowToBeReady. Once it returns the first time it would setpreparedIndexingActivationto true. - The "hot path" of extra overhead we are adding to the event indexing adds minimal CPU use.
Please feel welcome to attack. Appreciate your advice.
There was a problem hiding this comment.
I like how we would create an abstraction layer to ensure single execution of preconditions logic for each event type 👍 But also, this will have zero effect on the current runtime complexity as waitForEnsRainbowToBeReady() returns a cached promise to be awaited by the caller.
Co-authored-by: lightwalker.eth <126201998+lightwalker-eth@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 7 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
♻️ Duplicate comments (2)
apps/ensindexer/src/lib/ensrainbow/singleton.ts (2)
54-56: 🧹 Nitpick | 🔵 TrivialConsider using
console.warnand includingensRainbowUrlin failure messages.Failed health check attempts are warnings, not informational logs. Additionally, including the
ensRainbowUrlin the message aids debugging when multiple ENSRainbow instances or environments are involved.♻️ Proposed fix
onFailedAttempt: ({ error, attemptNumber, retriesLeft }) => { - console.log( - `ENSRainbow health check attempt ${attemptNumber} failed (${error.message}). ${retriesLeft} retries left.`, + console.warn( + `ENSRainbow health check attempt ${attemptNumber} to '${ensRainbowUrl}' failed (${error.message}). ${retriesLeft} retries left.`, ); },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ensindexer/src/lib/ensrainbow/singleton.ts` around lines 54 - 56, Replace the informational console.log in the ENSRainbow health check (where attemptNumber and retriesLeft are used) with console.warn and include the ensRainbowUrl variable in the message so failures are emitted as warnings and show which ENSRainbow instance failed; update the log call inside the health-check/retry logic in singleton.ts to use console.warn and interpolate ensRainbowUrl along with attemptNumber, error.message, and retriesLeft.
63-68: 🧹 Nitpick | 🔵 TrivialInclude
ensRainbowUrlin the final error message.Including the URL in both the log and the thrown error will help operators identify which ENSRainbow instance failed during debugging.
♻️ Proposed fix
- console.error(`ENSRainbow health check failed after multiple attempts: ${errorMessage}`); + console.error(`ENSRainbow health check to '${ensRainbowUrl}' failed after multiple attempts: ${errorMessage}`); // Throw the error to terminate the ENSIndexer process due to the failed health check of a critical dependency - throw new Error(errorMessage, { + throw new Error(`ENSRainbow health check to '${ensRainbowUrl}' failed: ${errorMessage}`, { cause: error instanceof Error ? error : undefined, });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ensindexer/src/lib/ensrainbow/singleton.ts` around lines 63 - 68, The thrown error and the console.error currently only contain errorMessage; update both to include the ensRainbowUrl so operators can see which ENSRainbow instance failed: when building the message used in console.error and new Error (the throw in the block that constructs Error with cause), append or interpolate the ensRainbowUrl (e.g., `${errorMessage} (url: ${ensRainbowUrl})`) and use that combined string for both console.error and the Error constructor while keeping the existing cause handling.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@apps/ensindexer/src/lib/ensrainbow/singleton.ts`:
- Around line 54-56: Replace the informational console.log in the ENSRainbow
health check (where attemptNumber and retriesLeft are used) with console.warn
and include the ensRainbowUrl variable in the message so failures are emitted as
warnings and show which ENSRainbow instance failed; update the log call inside
the health-check/retry logic in singleton.ts to use console.warn and interpolate
ensRainbowUrl along with attemptNumber, error.message, and retriesLeft.
- Around line 63-68: The thrown error and the console.error currently only
contain errorMessage; update both to include the ensRainbowUrl so operators can
see which ENSRainbow instance failed: when building the message used in
console.error and new Error (the throw in the block that constructs Error with
cause), append or interpolate the ensRainbowUrl (e.g., `${errorMessage} (url:
${ensRainbowUrl})`) and use that combined string for both console.error and the
Error constructor while keeping the existing cause handling.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 9a53df0a-7c19-423a-8e8f-21e023cd4202
📒 Files selected for processing (3)
.changeset/sharp-moons-shave.mdapps/ensindexer/src/lib/ensrainbow/singleton.tsapps/ensindexer/src/lib/indexing-engines/ponder.ts
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/ensindexer/src/lib/indexing-engines/ponder.ts`:
- Around line 157-188: The boolean preparedIndexingActivation creates a race by
being set before waitForEnsRainbowToBeReady() settles; instead replace it with
an in-flight promise (e.g., preparedIndexingActivationPromise) in
prepareIndexingActivation so concurrent callers await the same promise, and on
rejection clear the cached promise so future calls can retry; update
prepareIndexingActivation to assign the promise before awaiting and wrap await
in try/catch to clear the cache on failure. Also add a unit test that mocks
waitForEnsRainbowToBeReady to return a controllable promise, fires two onchain
callbacks before resolving that promise, asserts both awaited the readiness (did
not proceed early), and then resolves the readiness promise to complete both
callbacks.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 2d7e182b-b4dd-429b-a2b4-5089c3649390
📒 Files selected for processing (3)
apps/ensindexer/src/lib/ensrainbow/singleton.tsapps/ensindexer/src/lib/indexing-engines/ponder.test.tsapps/ensindexer/src/lib/indexing-engines/ponder.ts
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 7 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| async function prepareIndexingActivation() { | ||
| if (preparedIndexingActivation) { | ||
| return; | ||
| } | ||
|
|
||
| preparedIndexingActivation = true; | ||
|
|
||
| await waitForEnsRainbowToBeReady(); | ||
| } |
There was a problem hiding this comment.
prepareIndexingActivation() sets preparedIndexingActivation = true before awaiting waitForEnsRainbowToBeReady(). If a second onchain callback fires while the first is still waiting, it will hit the early return and skip waiting, allowing onchain handlers to run before ENSRainbow is ready. To preserve the precondition guarantee under concurrency, cache and return a shared promise (or only flip the flag after the await resolves) so every onchain handler awaits readiness until it’s actually satisfied.
| it("calls waitForEnsRainbowToBeReady only once when two onchain callbacks fire concurrently before the readiness promise resolves", async () => { | ||
| const { addOnchainEventListener } = await getPonderModule(); | ||
| const handler1 = vi.fn().mockResolvedValue(undefined); | ||
| const handler2 = vi.fn().mockResolvedValue(undefined); | ||
| let resolveReadiness: (() => void) | undefined; | ||
|
|
||
| // Create a promise that won't resolve until we manually trigger it | ||
| mockWaitForEnsRainbow.mockImplementation(() => { | ||
| return new Promise<void>((resolve) => { | ||
| resolveReadiness = resolve; | ||
| }); | ||
| }); | ||
|
|
||
| // Register two different onchain event listeners | ||
| addOnchainEventListener("Resolver:AddrChanged" as EventNames, handler1); | ||
| addOnchainEventListener("Registry:Transfer" as EventNames, handler2); | ||
|
|
||
| // Fire both handlers concurrently - neither should complete yet | ||
| const promise1 = getRegisteredCallback(0)({ | ||
| context: { db: vi.fn() } as unknown as Context<EventNames>, | ||
| event: { args: { a: "1" } } as unknown as IndexingEngineEvent<EventNames>, | ||
| }); | ||
| const promise2 = getRegisteredCallback(1)({ | ||
| context: { db: vi.fn() } as unknown as Context<EventNames>, | ||
| event: { args: { a: "2" } } as unknown as IndexingEngineEvent<EventNames>, | ||
| }); | ||
|
|
||
| // Should only have been called once despite concurrent execution | ||
| expect(mockWaitForEnsRainbow).toHaveBeenCalledTimes(1); | ||
|
|
||
| // Neither handler should have executed yet | ||
| expect(handler1).not.toHaveBeenCalled(); | ||
| expect(handler2).not.toHaveBeenCalled(); | ||
|
|
There was a problem hiding this comment.
This concurrency test can pass even if onchain handlers execute before ENSRainbow is ready, because promise callbacks may not run until the first await, and resolveReadiness() is invoked before yielding. Consider explicitly flushing the microtask queue before resolving readiness (or asserting the handler promises are still pending) so the test will fail if a handler runs without actually awaiting waitForEnsRainbowToBeReady().
| return ponder.on(eventName, async ({ context, event }) => | ||
| eventHandlerPreconditions(eventName).then(() => | ||
| eventHandler({ | ||
| context: buildIndexingEngineContext(context), | ||
| event, | ||
| }), | ||
| ), | ||
| ); |
There was a problem hiding this comment.
In an async callback, using .then(() => ...) is redundant and makes control flow harder to follow/step through. Prefer await eventHandlerPreconditions(eventName); return eventHandler(...) so sequencing and error propagation are explicit.
| return ponder.on(eventName, async ({ context, event }) => | |
| eventHandlerPreconditions(eventName).then(() => | |
| eventHandler({ | |
| context: buildIndexingEngineContext(context), | |
| event, | |
| }), | |
| ), | |
| ); | |
| return ponder.on(eventName, async ({ context, event }) => { | |
| await eventHandlerPreconditions(eventName); | |
| return eventHandler({ | |
| context: buildIndexingEngineContext(context), | |
| event, | |
| }); | |
| }); |
Lite PR
Tip: Review docs on the ENSNode PR process
Summary
ensRainbowClientsingleton for modules in the ENSIndexer app to use.apps/ensindexer/src/lib/ensrainbow/singleton.tswaitForEnsRainbowToBeReadyfunction to enable waiting with certain code execution untli the ENSRainbow instance is ready to serve traffic.apps/ensindexer/src/lib/ensrainbow/singleton.tseventHandlerPrecondtionsfunction to prevent indexing onchain events beforewaitForEnsRainbowToBeReadyresolves. Please note how processing "setup" events is unaffected.apps/ensindexer/src/lib/indexing-engines/ponder.tsWhy
Testing
ensRainbowPublicConfigobject available.Test logs from ENSIndexer
Notes for Reviewer (Optional)
Pre-Review Checklist (Blocking)