Ensure valid ENSRainbow connection before indexing starts#1850
Ensure valid ENSRainbow connection before indexing starts#1850
Conversation
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
Co-authored-by: lightwalker.eth <126201998+lightwalker-eth@users.noreply.github.com>
…` object This enable storing the public config of the connected ENSRainbow instance in ENSDb. The ENSDb record will be used to read the public config for ENSRainbow instance from other ENSNode apps.
🦋 Changeset detectedLatest commit: 4a80427 The changes in this PR will be included in the next version bump. This PR includes changesets to release 23 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 |
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 14 minutes and 10 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (5)
📝 WalkthroughWalkthroughExtended ENSDb SDK with ENSRainbow Public Config storage and retrieval methods. Updated ENSIndexer to validate ENSRainbow configuration compatibility before indexing initialization, including stored config comparison, omnichain status checks, and label set version validation. Changes
Sequence DiagramsequenceDiagram
actor Indexer as ENSIndexer<br/>(Activation)
participant ENSDb as ENSDb Client
participant Retry as Retry Engine<br/>(p-retry)
participant Status as Indexing Status<br/>Builder
participant RBow as ENSRainbow<br/>Service
Indexer->>ENSDb: getEnsRainbowPublicConfig()
alt Stored Config Exists
ENSDb-->>Indexer: return stored config
Indexer->>RBow: waitForEnsRainbowToBeReady()
RBow-->>Indexer: ready
Indexer->>RBow: config()
RBow-->>Indexer: return live config
Indexer->>Indexer: validate labelSetId match
Indexer->>Indexer: validate highestLabelSetVersion ≥ stored
Indexer->>ENSDb: upsertEnsRainbowPublicConfig(validated)
ENSDb-->>Indexer: ✓ upserted
else No Stored Config
ENSDb-->>Indexer: undefined
Indexer->>Retry: retry getIndexingStatusSnapshot()
Retry->>Status: getIndexingStatusSnapshot()
Status-->>Retry: snapshot
Retry-->>Indexer: snapshot
Indexer->>Indexer: verify status === Unstarted
Indexer->>RBow: waitForEnsRainbowToBeReady()
RBow-->>Indexer: ready
Indexer->>RBow: config()
RBow-->>Indexer: return config
Indexer->>ENSDb: upsertEnsRainbowPublicConfig(config)
ENSDb-->>Indexer: ✓ upserted
end
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 |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
There was a problem hiding this comment.
Pull request overview
This PR strengthens ENSIndexer startup/indexing preconditions around ENSRainbow by persisting ENSRainbow’s public config in ENSDb metadata and enforcing a “valid ENSRainbow connection + compatible config” gate before onchain event handlers run.
Changes:
- ENSDb SDK: add ENSNode metadata key/type + reader/writer helpers for
EnsRainbowPublicConfig. - ENSIndexer: introduce
ensureValidEnsRainbowConnection()precondition that loads/validates/upserts ENSRainbow public config and enforces “Unstarted” status on first-ever config write. - ENSDb writer worker: allow persisting
Unstartedindexing status snapshots.
Reviewed changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/ensdb-sdk/src/client/serialize/ensnode-metadata.ts | Adds serialized union support for ENSRainbow public config metadata. |
| packages/ensdb-sdk/src/client/ensnode-metadata.ts | Introduces ensrainbow_public_config metadata key + typed variant. |
| packages/ensdb-sdk/src/client/ensdb-writer.ts | Adds upsertEnsRainbowPublicConfig() writer API. |
| packages/ensdb-sdk/src/client/ensdb-writer.test.ts | Tests ENSRainbow public config upsert writes correct metadata. |
| packages/ensdb-sdk/src/client/ensdb-reader.ts | Adds getEnsRainbowPublicConfig() reader API. |
| packages/ensdb-sdk/src/client/ensdb-reader.test.ts | Tests ENSRainbow public config read behavior (undefined vs stored). |
| apps/ensindexer/src/lib/indexing-engines/preconditions/valid-ensrainbow-connection.ts | New precondition implementing ENSRainbow readiness + config validation/upsert logic. |
| apps/ensindexer/src/lib/indexing-engines/ponder.ts | Switches onchain precondition from ENSRainbow readiness to full ensureValidEnsRainbowConnection(). |
| apps/ensindexer/src/lib/indexing-engines/ponder.test.ts | Expands tests to cover new ENSRainbow config gating behaviors. |
| apps/ensindexer/src/lib/ensdb-writer-worker/ensdb-writer-worker.ts | Removes prior guard preventing Unstarted snapshot upserts. |
| apps/ensindexer/src/lib/ensdb-writer-worker/ensdb-writer-worker.test.ts | Updates snapshot upsert test to expect writes across Unstarted and started statuses. |
| .changeset/whole-lines-smoke.md | Publishes a minor bump for @ensnode/ensdb-sdk due to new metadata support. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
apps/ensindexer/src/lib/indexing-engines/preconditions/valid-ensrainbow-connection.ts
Outdated
Show resolved
Hide resolved
apps/ensindexer/src/lib/indexing-engines/preconditions/valid-ensrainbow-connection.ts
Outdated
Show resolved
Hide resolved
Greptile SummaryThis PR introduces a startup precondition gate ( Key changes:
Confidence Score: 5/5
Important Files Changed
Sequence DiagramsequenceDiagram
participant P as Ponder (onchain event)
participant IAP as indexingActivationPromise
participant VERC as ensureValidEnsRainbowConnection
participant ENSDb as ENSDb (ensDbClient)
participant ERW as ENSRainbow (singleton)
P->>IAP: await eventHandlerPreconditions(Onchain)
note over IAP: First call only — promise cached thereafter
IAP->>VERC: initializeIndexingActivation()
VERC->>ENSDb: getEnsRainbowPublicConfig()
ENSDb-->>VERC: storedConfig (or undefined)
alt Cold start (no storedConfig)
loop pRetry (max 3)
VERC->>ENSDb: getIndexingStatusSnapshot()
ENSDb-->>VERC: snapshot (or undefined → retry)
end
VERC->>VERC: invariant_indexingStatusUnstarted(snapshot)
end
VERC->>ERW: waitForEnsRainbowToBeReady()
note over ERW: pRetry health check, up to ~1 hour
ERW-->>VERC: ready
VERC->>ERW: ensRainbowClient.config()
ERW-->>VERC: fetchedConfig
alt Warm start (storedConfig exists)
VERC->>VERC: invariant_labelSetIdCompatibility
VERC->>VERC: invariant_highestLabelSetVersionCompatibility
end
VERC->>ENSDb: upsertEnsRainbowPublicConfig(fetchedConfig)
ENSDb-->>VERC: ok
VERC-->>IAP: resolved
IAP-->>P: preconditions met — handler executes
Reviews (2): Last reviewed commit: "Apply PR feedback" | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
apps/ensindexer/src/lib/indexing-engines/preconditions/valid-ensrainbow-connection.ts
Outdated
Show resolved
Hide resolved
![vercel[bot]](https://avatars.githubusercontent.com/in/8329?s=60&v=4){kind=small}
apps/ensindexer/src/lib/indexing-engines/preconditions/valid-ensrainbow-connection.ts
Outdated
Show resolved
Hide resolved
Also, allow for async preconditions for the indexing setup events. This will be handy for ensuring certain pg extensions were enabled before indexing starts.
tk-o
force-pushed
the
feat/ensure-valid-ensrainbow-connection
branch
from
d498f8d to
4e2454b
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 12 out of 12 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
lightwalker-eth
left a comment
lightwalker-eth
left a comment
There was a problem hiding this comment.
@tk-o Thanks for updates. Shared some small feedback 👍
| * @returns Validated Omnichain Indexing Status Snapshot. | ||
| * @throws Error if the Omnichain Indexing Status is not in expected status yet. | ||
| */ | ||
| private async getValidatedIndexingStatusSnapshot(): Promise<OmnichainIndexingStatusSnapshot> { |
There was a problem hiding this comment.
I'm not confident it's right to completely remove this logic.
My worry is: how do we tell the difference between the following situations:
- Omnichain status is Unstarted because no indexing has started in ENSDb yet.
- Indexing has started in ENSDb with an earlier instance of ENSIndexer, but now ENSIndexer is being restarted and it's still working to discover it's true omnichain indexing status from the state in ENSDb. During this case I'm worried that ENSIndexer also thinks it's "Unstarted"?
There was a problem hiding this comment.
Omnichain status is "unstarted" if an only if no indexing has started. This is related to the fact that the omnichain status can only be "unstarted" if and only if all chains are queued. And a chain is queued if and only if its config.startBlock is equal to its checkpointBlock (checkpointBlock is sourced from Ponder Indexing Status, which is sourced from _ponder_checkpoint table in the ENSIndexer Schema).
If indexing has started and ENSIndexer has written any indexed data into the ENSIndexer Schema, the checkpointBlock for some indexed chain has also been stored in the _ponder_checkpoint table in the ENSIndexer Schema in ENSDb. Therefore, if ENSIndexer instance restarts, some of the indexed chains will not be "queued" anyomore as checkpointBlock will be ahead of config.startBlock for that chain. That leads us to the fact that the omnichain status cannot be "unstarted" anymore, and goes straight to "backfill".
| } | ||
|
|
||
| /** | ||
| * Ensure that we have a valid connection to ENSRainbow and |
There was a problem hiding this comment.
We need to refine our terminology here.
Why are we using the word "valid"? I assume the goal is to describe something about "compatible" instead? And assuming the key idea here is "compatible", can you please help to make it more explicit exactly what "compatible" means?
There was a problem hiding this comment.
I was thinking that the connection is valid after it was validated that the connected ENSRainbow instance can be used to serve the ENSIndexer instance. The "compatibility" term was suggested to reference a relation between two EnsRainbowPublicConfig objects, where two of such objects are "compatbile if, and only if, both conditions are true:
- The label set ID is the same between these two objects.
- The highest label set version of the
EnsRainbowPublicConfigobject fetched from ENSRainbow instance is greater than or equal to the highest label set version of theEnsRainbowPublicConfigobject stored in ENSDb.
The compatibility of EnsRainbowPublicConfig objects is enforced in the form of invariants. And when both invariants are OK, we can say that the ENSIndexer has a valid connection to ENSRainbow instance. In other words, there's a valid connection to ENSRainbow.
Does that make sense?
shrugs
left a comment
shrugs
left a comment
There was a problem hiding this comment.
I can't comment on the indexing status terminology or logic, but otherwise looks good to me
|
@greptile review |
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/ensindexer/src/lib/ensdb-writer-worker/ensdb-writer-worker.test.ts (1)
283-315:⚠️ Potential issue | 🟡 MinorAssert the builder inputs, not only the mocked outputs.
Because
buildCrossChainIndexingStatusSnapshotOmnichain(...)is preprogrammed with sequential return values, this test still passes if the worker calls the builder with the wrong omnichain snapshot on either tick. AddtoHaveBeenNthCalledWith(...)assertions on the builder so the regression coverage is tied to the newUnstartedflow, not just to mocked return order.🧪 Suggested assertion tightening
vi.mocked(buildCrossChainIndexingStatusSnapshotOmnichain) .mockReturnValueOnce(unstartedCrossChainSnapshot) .mockReturnValueOnce(validCrossChainSnapshot); @@ // assert expect(indexingStatusBuilder.getOmnichainIndexingStatusSnapshot).toHaveBeenCalledTimes(2); + expect(buildCrossChainIndexingStatusSnapshotOmnichain).toHaveBeenNthCalledWith( + 1, + unstartedSnapshot, + expect.any(Number), + ); + expect(buildCrossChainIndexingStatusSnapshotOmnichain).toHaveBeenNthCalledWith( + 2, + validSnapshot, + expect.any(Number), + ); expect(ensDbClient.upsertIndexingStatusSnapshot).toHaveBeenCalledTimes(2); expect(ensDbClient.upsertIndexingStatusSnapshot).toHaveBeenNthCalledWith( 1, unstartedCrossChainSnapshot, );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/ensindexer/src/lib/ensdb-writer-worker/ensdb-writer-worker.test.ts` around lines 283 - 315, The test currently only asserts the mocked outputs from buildCrossChainIndexingStatusSnapshotOmnichain and ensDbClient.upsertIndexingStatusSnapshot, so add assertions that verify the worker actually called the builder with the expected omnichain snapshots on each tick: use expect(buildCrossChainIndexingStatusSnapshotOmnichain).toHaveBeenNthCalledWith(1, unstartedSnapshot) and .toHaveBeenNthCalledWith(2, validSnapshot) (or, if the builder method under test is indexingStatusBuilder.getOmnichainIndexingStatusSnapshot, assert that one instead) to ensure the worker passed the correct input snapshots to buildCrossChainIndexingStatusSnapshotOmnichain/indexingStatusBuilder.getOmnichainIndexingStatusSnapshot on the first and second interval ticks.
🤖 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.test.ts`:
- Around line 321-336: The test currently accepts any error (/.*/), which is too
broad; update the assertion in the it("throws when indexing status snapshot is
missing") case to match the specific error thrown when
mockGetIndexingStatusSnapshot resolves to undefined—capture the actual error
message produced by the indexing code and replace the generic regex with a
targeted one (for example matching "indexing status snapshot" or the exact
phrase thrown) in the expectHandlerThrows call that wraps
getRegisteredCallback(); ensure you still set
mockGetIndexingStatusSnapshot.mockResolvedValue(undefined) and call
addOnchainEventListener/getRegisteredCallback as before so the test validates
the correct failure path.
In
`@apps/ensindexer/src/lib/indexing-engines/preconditions/valid-ensrainbow-connection.ts`:
- Around line 146-166: The pRetry call that loads the indexing status snapshot
(wrapping ensDbClient.getIndexingStatusSnapshot) only sets retries and relies on
default exponential backoff; update that pRetry invocation to include explicit
timing options (e.g., minTimeout and maxTimeout or a timeout and factor) to make
retry timing deterministic, matching the backoff configuration used by
waitForEnsRainbowToBeReady(); adjust the options object passed to pRetry for the
indexingStatusSnapshot block so it includes the explicit timing fields alongside
retries and onFailedAttempt.
---
Outside diff comments:
In `@apps/ensindexer/src/lib/ensdb-writer-worker/ensdb-writer-worker.test.ts`:
- Around line 283-315: The test currently only asserts the mocked outputs from
buildCrossChainIndexingStatusSnapshotOmnichain and
ensDbClient.upsertIndexingStatusSnapshot, so add assertions that verify the
worker actually called the builder with the expected omnichain snapshots on each
tick: use
expect(buildCrossChainIndexingStatusSnapshotOmnichain).toHaveBeenNthCalledWith(1,
unstartedSnapshot) and .toHaveBeenNthCalledWith(2, validSnapshot) (or, if the
builder method under test is
indexingStatusBuilder.getOmnichainIndexingStatusSnapshot, assert that one
instead) to ensure the worker passed the correct input snapshots to
buildCrossChainIndexingStatusSnapshotOmnichain/indexingStatusBuilder.getOmnichainIndexingStatusSnapshot
on the first and second interval ticks.
🪄 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: 3b2e6925-5d95-4fec-9821-1feaa55211c3
📒 Files selected for processing (13)
.changeset/whole-lines-smoke.mdapps/ensindexer/src/lib/ensdb-writer-worker/ensdb-writer-worker.test.tsapps/ensindexer/src/lib/ensdb-writer-worker/ensdb-writer-worker.tsapps/ensindexer/src/lib/ensrainbow/singleton.tsapps/ensindexer/src/lib/indexing-engines/ponder.test.tsapps/ensindexer/src/lib/indexing-engines/ponder.tsapps/ensindexer/src/lib/indexing-engines/preconditions/valid-ensrainbow-connection.tspackages/ensdb-sdk/src/client/ensdb-reader.test.tspackages/ensdb-sdk/src/client/ensdb-reader.tspackages/ensdb-sdk/src/client/ensdb-writer.test.tspackages/ensdb-sdk/src/client/ensdb-writer.tspackages/ensdb-sdk/src/client/ensnode-metadata.tspackages/ensdb-sdk/src/client/serialize/ensnode-metadata.ts
apps/ensindexer/src/lib/indexing-engines/preconditions/valid-ensrainbow-connection.ts
Show resolved
Hide resolved
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 13 out of 13 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if (!storedConfig) { | ||
| console.log( | ||
| "No stored ENSRainbow Public Config found in ENSDb. Validating the omnichain indexing status is 'Unstarted'...", | ||
| ); |
There was a problem hiding this comment.
This precondition module uses console.log/console.warn for operational logging. In ENSIndexer, other runtime components use the shared structured logger (e.g. apps/ensindexer/src/lib/ensrainbow/singleton.ts) so logs carry level/module context and are routed consistently. Please switch these console calls to logger.info/logger.warn/logger.error (including a module field) to keep startup/indexing diagnostics consistent and avoid losing logs in production environments that don’t capture stdout/stderr the same way.
There was a problem hiding this comment.
@tk-o I understand you've been doing some optimizations for how logging works in ENSIndexer? Please take the lead to decide how to follow-up on this feedback from Copilot.
lightwalker-eth
left a comment
lightwalker-eth
left a comment
There was a problem hiding this comment.
@tk-o Hey thanks for the updates here. I shared a number of suggestions and don't currently feel comfortable that we've built all the state machines here in a bulletproof way for all possible state interleavings.
| if (!storedConfig) { | ||
| console.log( | ||
| "No stored ENSRainbow Public Config found in ENSDb. Validating the omnichain indexing status is 'Unstarted'...", | ||
| ); |
There was a problem hiding this comment.
@tk-o I understand you've been doing some optimizations for how logging works in ENSIndexer? Please take the lead to decide how to follow-up on this feedback from Copilot.
| `Using default public ENSRainbow server which may cause increased network latency. For production, use your own ENSRainbow server that runs on the same network as the ENSIndexer server.`, | ||
| ); | ||
| logger.warn({ | ||
| msg: `Using default public ENSRainbow server which may cause increased network latency`, |
There was a problem hiding this comment.
| msg: `Using default public ENSRainbow server which may cause increased network latency`, | |
| msg: `Using default public ENSRainbow server. This adds network latency that will significantly reduce indexing performance.`, |
| ); | ||
| logger.warn({ | ||
| msg: `Using default public ENSRainbow server which may cause increased network latency`, | ||
| advice: `For production, use your own ENSRainbow server that runs on the same network as the ENSIndexer server.`, |
There was a problem hiding this comment.
| advice: `For production, use your own ENSRainbow server that runs on the same network as the ENSIndexer server.`, | |
| advice: `Use your own ENSRainbow service that runs on the same network as your ENSIndexer service.`, |
| if (waitForEnsRainbowToBeReadyPromise) return waitForEnsRainbowToBeReadyPromise; | ||
|
|
||
| logger.info({ | ||
| msg: `Waiting for ENSRainbow instance to be ready`, |
There was a problem hiding this comment.
| msg: `Waiting for ENSRainbow instance to be ready`, | |
| msg: `Waiting for your ENSRainbow instance to be healthy`, |
|
|
||
| logger.info({ | ||
| msg: `Waiting for ENSRainbow instance to be ready`, | ||
| ensRainbowInstance: ensRainbowUrl.href, |
There was a problem hiding this comment.
| ensRainbowInstance: ensRainbowUrl.href, | |
| ensRainbowUrl: ensRainbowUrl.href, |
| } | ||
|
|
||
| let indexingSetupPromise: Promise<void> | null = null; | ||
| let indexingActivationPromise: Promise<void> | null = null; |
There was a problem hiding this comment.
| let indexingActivationPromise: Promise<void> | null = null; | |
| let initIndexingOnchainEventsPromise: Promise<void> | null = null; |
| EnsIndexerPublicConfig, | ||
| EnsRainbowPublicConfig, | ||
| } from "@ensnode/ensnode-sdk"; | ||
|
|
There was a problem hiding this comment.
It's very important that whenever we make updates to our database schemas that we also update the related docs inside the Drizzle schema definitions.
Based on the changes being introduced in this PR, I would expect to see updates to the docs in packages/ensdb-sdk/src/ensnode/index.ts
These docs needs to be strongly written to communicate the invariants of the state model of which permutations of keys can exist or not exist at the same time in relation to each other.
For example, it's my understanding that each of the following cases exist at a given instant in time when looking up multiple values in the ENSNode Metadata Table at the same time given a particular ENSIndexer Schema Name:
- No values found at all (suggests that that ENSIndexer Schema Name doesn't exist).
- You will find a value for the
EnsDbVersion,EnsIndexerPublicConfig, andEnsIndexerIndexingStatusbut NOT theEnsRainbowPublicConfigyet. - You will find a value for the
EnsDbVersion,EnsIndexerPublicConfig,EnsIndexerIndexingStatusandEnsRainbowPublicConfig.
Are any other state permutations possible? We need to think very defensively about what the possible state permutations are based on the precise way data is initialized and updated in the ENSNode metadata table.
From my memory, the way the write operations related to these keys is implemented creates problems because it's making the inserts one at a time. Do you see how this is a big problem? Instead, the write operations should be implemented such that there's only 3 ways our code performs a write operation:
- Insert (not update)
EnsDbVersion,EnsIndexerPublicConfig, andEnsIndexerIndexingStatusall at the same time atomically. - Insert (not update)
EnsRainbowPublicConfigfor an ENSIndexer Schema Name that already has anEnsDbVersion,EnsIndexerPublicConfig, andEnsIndexerIndexingStatus. - Update (not insert) an
EnsIndexerIndexingStatusfor an ENSIndexer Schema Name that already has all the keys inserted.
Do you see the mental model and rigorous approach I'm suggesting here? I expect us to implement all of our ENSDb logic with such a rigorous state-machine like approach that considers all possible interleavings.
I would also note that the current way the JSDocs are written in packages/ensdb-sdk/src/ensnode/index.ts is not good and really needs improvement.
For example, I see docs there written like:
/**
* Key
*
* Allowed keys:
* - `EnsNodeMetadataEnsDbVersion['key']`
* - `EnsNodeMetadataEnsIndexerPublicConfig['key']`
* - `EnsNodeMetadataEnsIndexerIndexingStatus['key']`
*/
key: t.text().notNull(),
This is way too abstract and will be a pain in the ass for someone reading the docs. This puts burden on them to figure out what the heck each of those keys actually represent as constants. Don't make it difficult for devs building on ENSDb as an integration point. Note also how these devs should not be expected to be building their app in JS / TS. We need to make it easy for them to read the docs here and get their integration right.
Same idea here:
/**
* Value
*
* Allowed values:
* - `EnsNodeMetadataEnsDbVersion['value']`
* - `EnsNodeMetadataEnsIndexerPublicConfig['value']`
* - `EnsNodeMetadataEnsIndexerIndexingStatus['value']`
*
* Guaranteed to be a serialized representation of JSON object.
*/
value: t.jsonb().notNull(),
We really need to improve the docs here for integrators!
| @@ -173,7 +174,7 @@ async function initializeIndexingSetup(): Promise<void> { | |||
| * ``` | |||
There was a problem hiding this comment.
@tk-o Important: I see a key issue with the current way initIndexingSetup and initIndexingOnchainEvents are written.
They both seem to make the incorrect assumption that ENSIndexer never restarts itself. But what happens if ENSIndexer is restarted at particular moments in time?
For example:
- What if
initIndexingSetuphas logic to do something like install a postgres extension, but the extension has already been installed by a previous run of ENSIndexer? - What if
initIndexingOnchainEventshas logic to do something like initialize a bunch of state inside the ENSNode Metadata Table, but that logic has already been executed and the related state has already been initialized by a previous run of ENSIndexer with the same ENSIndexer Schema Name?
We really need you to think more strictly and systematically about all the possible state interleavings and ensure that for all possible state interleavings we have a mature solution.
I assume that one possible strategy for making this implementation more mature would be to query the state of the ENSNode Metadatadata table for the configured ENSIndexer Schema Name, and then based on the state reflected there determine if it's a completely new instance or not? This needs careful planning as even the way I've described the idea here would not be reliable enough as some additional state would likely need to be added into the ENSNode Metadata table (an additional key?) to identify the exact state of executing these init functions in an idempotent way.
|
|
||
| if (omnichainStatus !== OmnichainIndexingStatusIds.Unstarted) { | ||
| throw new Error( | ||
| `The omnichain indexing status must be '${OmnichainIndexingStatusIds.Unstarted}' to upsert the ENSRainbow public config into ENSDb. Provided status: '${omnichainStatus}'.`, |
There was a problem hiding this comment.
| `The omnichain indexing status must be '${OmnichainIndexingStatusIds.Unstarted}' to upsert the ENSRainbow public config into ENSDb. Provided status: '${omnichainStatus}'.`, | |
| `The omnichain indexing status must be '${OmnichainIndexingStatusIds.Unstarted}' to insert the ENSRainbow public config into ENSDb. Provided status: '${omnichainStatus}'.`, |
Note: Shouldn't this be a more strict insert operation, not an upsert?
| @@ -173,7 +174,7 @@ async function initializeIndexingSetup(): Promise<void> { | |||
| * ``` | |||
There was a problem hiding this comment.
Is there a special reason why initIndexingSetup doesn't include logic inside of it to initialize state in the ENSNode Metadata Tables for the ENSIndexer Schema Name? For example, wouldn't we want it to initialize everything except for the ENSRainbow Metadata?
Lite PR
Tip: Review docs on the ENSNode PR process
Summary
EnsRainbowPublicConfigobject to/from ENSNode Metadata table.packages/ensdb-sdk/src/client/*apps/ensindexer/src/lib/ensdb-writer-worker/ensdb-writer-worker.tsensureValidEnsRainbowConnectionfunction to cover all business logic required as precondition for starting indexing.apps/ensindexer/src/lib/indexing-engines/preconditions/valid-ensrainbow-connection.tsandapps/ensindexer/src/lib/indexing-engines/ponder.tsWhy
Testing
Scenario 1: cold start
Simulated the scenarion when ENSIndexer was running for the first time and ENSRainbow instance was temporarily unavailable.
Result: ENSIndexer was able to:
Details
Scenario 2: warm start
Simulated the scenario when ENSIndexer was restarted and ENSRainbow instance was temporarily unavailable.
Result: ENSIndexer was able to
Details
Notes for Reviewer (Optional)
Pre-Review Checklist (Blocking)
This PR does not introduce significant changes and is low-risk to review quickly.