Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
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 ⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan for PR comments
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 |
🦋 Changeset detectedLatest commit: 38f30d8 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 |
Greptile SummaryThis PR decouples ENSApi from ENSIndexer by replacing the HTTP-based ENSIndexer config fetch (using Key changes:
Confidence Score: 3/5
Important Files Changed
Sequence DiagramsequenceDiagram
participant Env as Process Env
participant BSC as buildConfigFromEnvironment
participant EnsDbClient
participant PgPool as node-postgres Pool
participant ENSDb as ENSDb (Postgres)
BSC->>Env: read DATABASE_URL + DATABASE_SCHEMA
BSC->>EnsDbClient: new EnsDbClient(databaseUrl, ensIndexerRef)
EnsDbClient->>PgPool: create read-only pool (never closed ⚠️)
BSC->>EnsDbClient: getEnsIndexerPublicConfig()
EnsDbClient->>ENSDb: SELECT * FROM ensnode.ens_node_metadata WHERE ens_indexer_ref = ? AND key = ?
ENSDb-->>EnsDbClient: row or empty
EnsDbClient-->>BSC: EnsIndexerPublicConfig | undefined
Note over BSC: if undefined → throw Error → process.exit(1)
BSC->>BSC: EnsApiConfigSchema.parse(...)
BSC-->>BSC: EnsApiConfig (EnsDbClient discarded, pool leaks)
|
There was a problem hiding this comment.
Pull request overview
Integrates ENSDb into ENSApi by introducing a read-only ENSDb client and switching ENSApi’s startup config loading to read ENSIndexer public config from ENSDb instead of fetching it from an ENSIndexer HTTP endpoint.
Changes:
- Added
EnsDbClient(read-only) in ENSApi for querying ENSNode metadata stored in ENSDb. - Updated ENSApi config bootstrapping to load
EnsIndexerPublicConfigfrom ENSDb and madeDATABASE_SCHEMArequired for ENSApi. - Removed the old
fetchENSIndexerConfig()HTTP-based implementation and updated examples/tests accordingly.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| terraform/modules/ensindexer/main.tf | Moves DATABASE_SCHEMA into shared env var locals for the Render service config. |
| apps/ensapi/src/lib/fetch-ensindexer-config.ts | Removes the old HTTP fetch helper for ENSIndexer config. |
| apps/ensapi/src/lib/ensdb-client/ensdb-client.ts | Adds a read-only ENSDb client to query ENSNode metadata (public config, version, indexing status). |
| apps/ensapi/src/config/environment.ts | Makes DATABASE_SCHEMA part of EnsApiEnvironment. |
| apps/ensapi/src/config/config.schema.ts | Switches config bootstrapping from HTTP fetch (with retry) to ENSDb reads via EnsDbClient. |
| apps/ensapi/src/config/config.schema.test.ts | Updates tests/mocks for the new ENSDb-based config loading. |
| apps/ensapi/.env.local.example | Documents new required DATABASE_SCHEMA for ENSApi. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
| const ensIndexerSchemaName = DatabaseSchemaNameSchema.parse(env.DATABASE_SCHEMA); | ||
|
|
||
| return new EnsDbClient(databaseUrl, ensIndexerSchemaName); |
There was a problem hiding this comment.
DATABASE_SCHEMA is parsed into ensIndexerSchemaName and passed as the ensIndexerRef argument to EnsDbClient. Since ensIndexerRef represents the ENSIndexer instance identifier (often the schema name), consider renaming ensIndexerSchemaName to ensIndexerRef (and/or clarifying via comments) to avoid confusing it with ensIndexerPublicConfig.databaseSchemaName used later for queries.
| const ensIndexerSchemaName = DatabaseSchemaNameSchema.parse(env.DATABASE_SCHEMA); | |
| return new EnsDbClient(databaseUrl, ensIndexerSchemaName); | |
| // NOTE: This is the ENSIndexer instance identifier (often the schema name), used as `ensIndexerRef` in EnsDbClient. | |
| const ensIndexerRef = DatabaseSchemaNameSchema.parse(env.DATABASE_SCHEMA); | |
| return new EnsDbClient(databaseUrl, ensIndexerRef); |
| const ensDbClient = buildEnsDbClientFromEnvironment(env); | ||
|
|
||
| const ensIndexerPublicConfig = await ensDbClient.getEnsIndexerPublicConfig(); | ||
|
|
||
| if (!ensIndexerPublicConfig) { | ||
| throw new Error("Failed to load EnsIndexerPublicConfig from ENSDb."); | ||
| } |
There was a problem hiding this comment.
Previously the ENSIndexer public config load had retry/backoff; the ENSDb read now happens once and any transient DB startup error will immediately exit the process. Consider reintroducing a small retry (or a clearer classification of “missing row” vs “DB connectivity/permission error”) around getEnsIndexerPublicConfig() to avoid flakiness during deploy/startup ordering.
| export async function buildConfigFromEnvironment(env: EnsApiEnvironment): Promise<EnsApiConfig> { | ||
| try { | ||
| const ensIndexerUrl = EnsIndexerUrlSchema.parse(env.ENSINDEXER_URL); | ||
|
|
||
| const ensIndexerPublicConfig = await pRetry(() => fetchENSIndexerConfig(ensIndexerUrl), { | ||
| retries: 3, | ||
| onFailedAttempt: ({ error, attemptNumber, retriesLeft }) => { | ||
| logger.info( | ||
| `ENSIndexer Config fetch attempt ${attemptNumber} failed (${error.message}). ${retriesLeft} retries left.`, | ||
| ); | ||
| }, | ||
| }); | ||
| const ensDbClient = buildEnsDbClientFromEnvironment(env); | ||
|
|
||
| const ensIndexerPublicConfig = await ensDbClient.getEnsIndexerPublicConfig(); | ||
|
|
||
| if (!ensIndexerPublicConfig) { | ||
| throw new Error("Failed to load EnsIndexerPublicConfig from ENSDb."); | ||
| } |
There was a problem hiding this comment.
PR description implies ENSApi won’t reference an ENSIndexer instance directly, but this function still builds a config that includes ensIndexerUrl (and ENSApi still has codepaths that use it). If full decoupling isn’t part of this PR, consider tightening the PR description/scope so it matches what’s actually changed here (public config source only).
| const mockGetVersion = vi.fn().mockResolvedValue("1.0.0"); | ||
| const mockGetEnsIndexerPublicConfig = vi.fn().mockResolvedValue(ENSINDEXER_PUBLIC_CONFIG); | ||
| const mockGetIndexingStatusSnapshot = vi.fn().mockResolvedValue(null); | ||
|
|
||
| vi.mock("@/lib/ensdb-client/ensdb-client", () => ({ | ||
| EnsDbClient: class MockEnsDbClient { | ||
| getVersion = mockGetVersion; |
There was a problem hiding this comment.
The EnsDbClient mock doesn’t match the real EnsDbClientQuery interface: it defines getVersion instead of getEnsDbVersion, and getIndexingStatusSnapshot resolves to null instead of undefined. Aligning the mock with the interface will prevent future tests from silently diverging when those methods start being used.
| const mockGetVersion = vi.fn().mockResolvedValue("1.0.0"); | |
| const mockGetEnsIndexerPublicConfig = vi.fn().mockResolvedValue(ENSINDEXER_PUBLIC_CONFIG); | |
| const mockGetIndexingStatusSnapshot = vi.fn().mockResolvedValue(null); | |
| vi.mock("@/lib/ensdb-client/ensdb-client", () => ({ | |
| EnsDbClient: class MockEnsDbClient { | |
| getVersion = mockGetVersion; | |
| const mockGetEnsDbVersion = vi.fn().mockResolvedValue("1.0.0"); | |
| const mockGetEnsIndexerPublicConfig = vi.fn().mockResolvedValue(ENSINDEXER_PUBLIC_CONFIG); | |
| const mockGetIndexingStatusSnapshot = vi.fn().mockResolvedValue(undefined); | |
| vi.mock("@/lib/ensdb-client/ensdb-client", () => ({ | |
| EnsDbClient: class MockEnsDbClient { | |
| getEnsDbVersion = mockGetEnsDbVersion; |
| const mockFetch = vi.fn(); | ||
| vi.stubGlobal("fetch", mockFetch); | ||
|
|
There was a problem hiding this comment.
fetch is still stubbed and configured in these tests, but buildConfigFromEnvironment no longer calls fetch after the switch to EnsDbClient. This makes the test setup misleading and reduces confidence that the new ENSDb path is being exercised; consider removing the fetch mocking and instead asserting EnsDbClient construction + getEnsIndexerPublicConfig() calls.
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| constructor(databaseUrl: string, ensIndexerRef: string) { | ||
| this.db = makeReadOnlyDrizzle({ | ||
| databaseUrl, | ||
| schema: ensNodeSchema, | ||
| }); | ||
|
|
||
| this.ensIndexerRef = ensIndexerRef; | ||
| } |
There was a problem hiding this comment.
Database connection pool is never closed
makeReadOnlyDrizzle creates a node-postgres connection pool under the hood. When buildConfigFromEnvironment creates an EnsDbClient to fetch the startup config, it makes a single query and then discards the client — but the underlying pg.Pool is never explicitly ended. In Node.js, a pg.Pool holds open TCP connections to Postgres until it is told to end(), so the pool will stay alive for the rest of the process lifetime after the config load.
Since buildConfigFromEnvironment runs once at startup (and the EnsDbClient is not reused), the simplest fix is to expose a close() method and call it after the query:
// in EnsDbClient
async close(): Promise<void> {
await (this.db as any).$client?.end();
}// in buildConfigFromEnvironment
const ensDbClient = buildEnsDbClientFromEnvironment(env);
try {
const ensIndexerPublicConfig = await ensDbClient.getEnsIndexerPublicConfig();
...
} finally {
await ensDbClient.close();
}Alternatively, consider sharing the EnsDbClient instance across the application lifetime (instead of creating a disposable one just for config loading) so the pool is reused rather than leaked.
There was a problem hiding this comment.
The connection must remain open for the process lifetime. It will be needed for loading current indexing status object while handling HTTP traffic.
| try { | ||
| const ensIndexerUrl = EnsIndexerUrlSchema.parse(env.ENSINDEXER_URL); | ||
|
|
||
| const ensIndexerPublicConfig = await pRetry(() => fetchENSIndexerConfig(ensIndexerUrl), { | ||
| retries: 3, | ||
| onFailedAttempt: ({ error, attemptNumber, retriesLeft }) => { | ||
| logger.info( | ||
| `ENSIndexer Config fetch attempt ${attemptNumber} failed (${error.message}). ${retriesLeft} retries left.`, | ||
| ); | ||
| }, | ||
| }); | ||
| const ensDbClient = buildEnsDbClientFromEnvironment(env); | ||
|
|
||
| const ensIndexerPublicConfig = await ensDbClient.getEnsIndexerPublicConfig(); | ||
|
|
||
| if (!ensIndexerPublicConfig) { | ||
| throw new Error("Failed to load EnsIndexerPublicConfig from ENSDb."); | ||
| } |
There was a problem hiding this comment.
No retry logic for DB query at startup
The previous implementation used pRetry with 3 retries (and informative log messages) when fetching the ENSIndexer config from the ENSIndexer HTTP endpoint. The new code performs a single database query with no retry: if the database is temporarily unavailable at startup (e.g., during a rolling deploy), ENSApi will immediately log an error and call process.exit(1), requiring an operator to manually restart the service.
Consider wrapping the ensDbClient.getEnsIndexerPublicConfig() call in similar retry logic, for example:
import pRetry from "p-retry";
const ensIndexerPublicConfig = await pRetry(
() => ensDbClient.getEnsIndexerPublicConfig(),
{
retries: 3,
onFailedAttempt: ({ error, attemptNumber, retriesLeft }) => {
logger.info(
`ENSIndexer Public Config DB read attempt ${attemptNumber} failed (${error.message}). ${retriesLeft} retries left.`,
);
},
},
);| const mockGetVersion = vi.fn().mockResolvedValue("1.0.0"); | ||
| const mockGetEnsIndexerPublicConfig = vi.fn().mockResolvedValue(ENSINDEXER_PUBLIC_CONFIG); | ||
| const mockGetIndexingStatusSnapshot = vi.fn().mockResolvedValue(null); | ||
|
|
||
| vi.mock("@/lib/ensdb-client/ensdb-client", () => ({ | ||
| EnsDbClient: class MockEnsDbClient { | ||
| getVersion = mockGetVersion; | ||
| getEnsIndexerPublicConfig = mockGetEnsIndexerPublicConfig; | ||
| getIndexingStatusSnapshot = mockGetIndexingStatusSnapshot; | ||
| }, | ||
| })); |
There was a problem hiding this comment.
Mock method name getVersion doesn't match actual implementation
The mock defines getVersion (line 69) but the real EnsDbClient exposes getEnsDbVersion(). The EnsDbClientQuery interface is the source of truth here, but the discrepancy can be misleading and won't surface as a type error because the mock class is untyped.
| const mockGetVersion = vi.fn().mockResolvedValue("1.0.0"); | |
| const mockGetEnsIndexerPublicConfig = vi.fn().mockResolvedValue(ENSINDEXER_PUBLIC_CONFIG); | |
| const mockGetIndexingStatusSnapshot = vi.fn().mockResolvedValue(null); | |
| vi.mock("@/lib/ensdb-client/ensdb-client", () => ({ | |
| EnsDbClient: class MockEnsDbClient { | |
| getVersion = mockGetVersion; | |
| getEnsIndexerPublicConfig = mockGetEnsIndexerPublicConfig; | |
| getIndexingStatusSnapshot = mockGetIndexingStatusSnapshot; | |
| }, | |
| })); | |
| vi.mock("@/lib/ensdb-client/ensdb-client", () => ({ | |
| EnsDbClient: class MockEnsDbClient { | |
| getEnsDbVersion = mockGetVersion; | |
| getEnsIndexerPublicConfig = mockGetEnsIndexerPublicConfig; | |
| getIndexingStatusSnapshot = mockGetIndexingStatusSnapshot; | |
| }, | |
| })); |
![vercel[bot]](https://avatars.githubusercontent.com/in/8329?s=60&v=4){kind=small}
| * @param ensIndexerRef reference string for ENSIndexer instance (used for multi-tenancy in ENSDb) | ||
| */ | ||
| constructor(databaseUrl: string, ensIndexerRef: string) { | ||
| this.db = makeReadOnlyDrizzle({ |
There was a problem hiding this comment.
EnsDbClient constructor doesn't pass databaseSchema to makeReadOnlyDrizzle(), causing queries to execute against the wrong schema
There was a problem hiding this comment.
Passing the name of target database schema is only required while working with ENSIndexer schema, as the schema name is dynamic in this case. Otherwise, we don't have to pass the schema name, as it's bound to the database object definition.
| const ensDbClient = buildEnsDbClientFromEnvironment(env); | ||
|
|
||
| const ensIndexerPublicConfig = await ensDbClient.getEnsIndexerPublicConfig(); | ||
|
|
There was a problem hiding this comment.
Database connection pool created in buildConfigFromEnvironment() is never closed, causing resource leak
…ma definitions. Handle multi-tenancy in ENSNode Metadata table.
Replaces ENSIndexer Client fetch with ENSDb Client read.
Includes `DATABASE_SCHEMA` env var
tk-o
force-pushed
the
feat/integrate-ensdb-into-ensapi
branch
from
0a54b8a to
38f30d8
Compare
lightwalker-eth
left a comment
lightwalker-eth
left a comment
There was a problem hiding this comment.
@tk-o Lots of issues to fix here.
| "ensapi": minor | ||
| --- | ||
|
|
||
| Replaced ENSIndexer Public Config source, from ENSIndexer to ENSDb. |
There was a problem hiding this comment.
Is it fair to also say that this change fully decouples ENSApi and ENSIndexer?
| # ENSDb: Database Schema name for ENSIndexer Schema | ||
| # Required. Should match the DATABASE_SCHEMA used by the connected ENSIndexer. | ||
| DATABASE_SCHEMA=public |
There was a problem hiding this comment.
| # ENSDb: Database Schema name for ENSIndexer Schema | |
| # Required. Should match the DATABASE_SCHEMA used by the connected ENSIndexer. | |
| DATABASE_SCHEMA=public | |
| # Schema name for the "ENSIndexer Schema" in ENSDb that ENSApi should read indexed data from. | |
| # Required. Should match the ENSINDEXER_SCHEMA environment variable in ENSIndexer that ENSApi should read indexed data from. | |
| ENSINDEXER_SCHEMA=public |
| # ENSDb: Database Schema name for ENSIndexer Schema | ||
| # Required. Should match the DATABASE_SCHEMA used by the connected ENSIndexer. | ||
| DATABASE_SCHEMA=public |
There was a problem hiding this comment.
Why are we promoting the use of public as the default value for the ENSINDEXER_SCHEMA? This will create more pain for devs.
Better to name the default something like ensindexer_0. That both makes it more intuitive how it's from ENSIndexer and it also plants a seed in people's minds how they can just increment the number if they want to build a new ENSIndexer schema.
Please think more about the mental model of devs who are new to ENSNode: they have a big learning curve and we should be doing everything we can to help them.
Please apply this idea everywhere across our monorepo.
|
|
||
| const BASE_ENV = { | ||
| DATABASE_URL: "postgresql://user:password@localhost:5432/mydb", | ||
| DATABASE_SCHEMA: "public", |
There was a problem hiding this comment.
Please see related comment, even for test files.
| @@ -116,6 +138,7 @@ describe("buildConfigFromEnvironment", () => { | |||
|
|
|||
| const TEST_ENV: EnsApiEnvironment = { | |||
| DATABASE_URL: BASE_ENV.DATABASE_URL, | |||
There was a problem hiding this comment.
Please create a follow-up issue to rename DATABASE_URL to ENSDB_URL everywhere.
We should be maximally aligning our terminology everywhere.
There was a problem hiding this comment.
Issue created here:
| */ | ||
| export type EnsApiEnvironment = Omit<DatabaseEnvironment, "DATABASE_SCHEMA"> & | ||
| export type EnsApiEnvironment = DatabaseEnvironment & | ||
| EnsIndexerUrlEnvironment & |
There was a problem hiding this comment.
Not yet, but will be removed in PR #1716.
| @@ -85,16 +97,13 @@ export type EnsApiConfig = z.infer<typeof EnsApiConfigSchema>; | |||
| */ | |||
| export async function buildConfigFromEnvironment(env: EnsApiEnvironment): Promise<EnsApiConfig> { | |||
There was a problem hiding this comment.
This needs refactoring / renaming.
It's not appropriate to call this function buildConfigFromEnvironment.
There was a problem hiding this comment.
@lightwalker-eth this week, we discussed how managing configuration objects in ENSApi has to be changed.
I suggest we address the refactoring buildConfigFromEnvironment function in PR #1716. The PR 1716 needs PRs 1752, 1753, and 1754 to be merged first.
| ); | ||
| }, | ||
| }); | ||
| const ensDbClient = buildEnsDbClientFromEnvironment(env); |
There was a problem hiding this comment.
In my mind, there should only ever be a single ensDbClient ever created in ENSApi.
There was a problem hiding this comment.
PR #1716 wil address required refactoring of how ENSApi configuration is applied. For now, we need to create ensDbClient to fetch EnsIndexerPublicConfig. The target state that PR 1716 will introduce is that there will be no need to call ensDbClient when building configuration from environment.
| * @returns instance of {@link EnsDbClient} | ||
| * @throws Error with formatted validation messages if environment parsing fails | ||
| */ | ||
| function buildEnsDbClientFromEnvironment(env: EnsApiEnvironment): EnsDbClient { |
There was a problem hiding this comment.
Why is this function in this file? This seems like bad code organization.
There was a problem hiding this comment.
I can move it to a separate file, but it's also a temporary measure that will be resolved with PR #1716, when there will be no need to call ensDbClient when building configuration from environment.
There was a problem hiding this comment.
Here, I'm just updating this bit, where ENSIndexer client configuration was parsed from env, and then ENSIndexer client object would be created from it.
If you like, I can replace buildEnsDbClientFromEnvironment function with inline code, just as the original.
ensnode/apps/ensapi/src/config/config.schema.ts
Lines 88 to 97 in 9429530
@lightwalker-eth advice appreciated.
| * - ENSIndexer Public Config, | ||
| * - Indexing Status Snapshot. | ||
| */ | ||
| export class EnsDbClient implements EnsDbClientQuery { |
There was a problem hiding this comment.
We're getting so many ideas badly mixed up. This is taking way too much of my time to identify all the problems and suggest solutions. It's very inappropriate to push all of these problems on me.
Issues here include how you're mixing up the idea of ENSDb with the ENSNode Schema in ENSDb.
Lite PR
Tip: Review docs on the ENSNode PR process
Summary
EnsDbClientfor ENSApibuildConfigFromEnvironmentto replace method for getting ENSIndexer Public ConfigWhy
Testing
ensnodeschema in local ENSDb.Notes for Reviewer (Optional)
Pre-Review Checklist (Blocking)