Conversation
|
|
The latest updates on your projects. Learn more about Vercel for GitHub. 3 Skipped Deployments
|
📝 WalkthroughWalkthroughThis pull request introduces Drizzle ORM migration infrastructure to the ensindexer application. It adds a database migration system with an initial migration creating an offchain schema and ensnode_metadata table, reorganizes the metadata schema from onchain to offchain, and integrates migration execution into the EnsDbClient and startup flow. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ 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 |
There was a problem hiding this comment.
Pull request overview
Splits ENSNode’s Drizzle/Ponder schema into on-chain (Ponder-managed) vs off-chain (ENSIndexer-managed) and introduces Drizzle migrations/config to manage the new off-chain tables (starting with ensnode_metadata).
Changes:
- Move
ensnode_metadatato anoffchainPostgres schema and expose it via a new@ensnode/ensnode-schema/offchainentrypoint. - Add Drizzle migration artifacts + drizzle-kit config in
apps/ensindexerfor generating/applying off-chain schema migrations. - Add migration execution to
EnsDbWriterWorkerstartup and addEnsDbClient.runMigrations().
Reviewed changes
Copilot reviewed 13 out of 14 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| pnpm-lock.yaml | Adds drizzle-kit and related dependency graph updates. |
| packages/ensnode-schema/src/schemas/ensnode-metadata.schema.ts | Defines ensnode_metadata under pgSchema("offchain") using Drizzle PG core. |
| packages/ensnode-schema/src/ponder.schema.ts | Removes ensnode_metadata from the on-chain/root ponder schema exports. |
| packages/ensnode-schema/src/offchain.schema.ts | New off-chain schema entrypoint exporting ensnode_metadata. |
| packages/ensnode-schema/package.json | Exposes new ./offchain export; adds drizzle-orm peer/dev dependency. |
| apps/ensindexer/src/lib/ensdb-writer-worker/ensdb-writer-worker.ts | Runs ENSDb migrations on worker start; adds migrations folder resolution. |
| apps/ensindexer/src/lib/ensdb-writer-worker/ensdb-writer-worker.test.ts | Updates mocks/assertions for the new migration call. |
| apps/ensindexer/src/lib/ensdb-client/ensdb-client.ts | Uses off-chain schema import, adds runMigrations(), and removes temp-table transaction logic for metadata upserts. |
| apps/ensindexer/package.json | Adds drizzle-kit dev dependency. |
| apps/ensindexer/drizzle/meta/_journal.json | Drizzle migration journal metadata. |
| apps/ensindexer/drizzle/meta/0000_snapshot.json | Drizzle snapshot describing the off-chain schema/table. |
| apps/ensindexer/drizzle/0000_orange_veda.sql | Initial migration creating offchain schema + ensnode_metadata table. |
| apps/ensindexer/drizzle.schema.ts | Drizzle-kit schema entrypoint exporting the off-chain schema tables. |
| apps/ensindexer/drizzle.config.ts | Drizzle-kit configuration for migration generation. |
Files not reviewed (1)
- pnpm-lock.yaml: Language not supported
💡 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.
| export default defineConfig({ | ||
| dialect: "postgresql", | ||
| schema: "./drizzle.schema.ts", | ||
| out: "./drizzle", | ||
| dbCredentials: { | ||
| url: "postgresql://dbuser:abcd1234@localhost:54320/ensnode_local_ponder_0_16", |
There was a problem hiding this comment.
drizzle.config.ts hardcodes a Postgres connection URL including credentials. This is easy to accidentally use against the wrong DB and is risky to commit (even if the password is “local”). Prefer reading the URL from env (e.g. process.env.DATABASE_URL) and/or using dotenv-style config, or omit dbCredentials and pass it via CLI when generating migrations.
| export default defineConfig({ | |
| dialect: "postgresql", | |
| schema: "./drizzle.schema.ts", | |
| out: "./drizzle", | |
| dbCredentials: { | |
| url: "postgresql://dbuser:abcd1234@localhost:54320/ensnode_local_ponder_0_16", | |
| const connectionString = process.env.DATABASE_URL; | |
| if (!connectionString) { | |
| throw new Error("DATABASE_URL environment variable is required for drizzle config"); | |
| } | |
| export default defineConfig({ | |
| dialect: "postgresql", | |
| schema: "./drizzle.schema.ts", | |
| out: "./drizzle", | |
| dbCredentials: { | |
| url: connectionString, |
| import { ensNodeMetadata } from "@ensnode/ensnode-schema/offchain"; | ||
| import { |
There was a problem hiding this comment.
ensNodeMetadata is now defined under the offchain schema, but makeDrizzle() still calls setDatabaseSchema(schema, databaseSchemaName) based on the constructor argument. If callers keep passing the on-chain DATABASE_SCHEMA (e.g. ensnode/public), the client will query DATABASE_SCHEMA.ensnode_metadata while migrations create offchain.ensnode_metadata, causing runtime failures. Consider removing the schema monkeypatch for this client (let the table’s own pgSchema("offchain") win) or hardcoding/deriving the offchain schema name separately from the on-chain schema config.
| */ | ||
|
|
||
| export * from "./schemas/ensnode-metadata.schema"; | ||
| export * from "./schemas/ensv2.schema"; |
There was a problem hiding this comment.
Removing the ensnode-metadata export from the main ponder schema breaks existing imports of ensNodeMetadata from @ensnode/ensnode-schema (e.g. apps/ensindexer/src/lib/ensdb-client/ensdb-client.test.ts). Either update all in-repo imports to use @ensnode/ensnode-schema/offchain, or re-export the metadata table from the root entrypoint for backward compatibility while consumers migrate.
| @@ -0,0 +1,6 @@ | |||
| CREATE SCHEMA "offchain"; | |||
There was a problem hiding this comment.
This migration uses CREATE SCHEMA "offchain"; without IF NOT EXISTS. This can fail on databases where the schema already exists (or under concurrent startups where two processes try to apply the first migration at once). Consider making schema creation idempotent (e.g. CREATE SCHEMA IF NOT EXISTS ...) or otherwise guarding it to avoid startup/migration crashes.
| CREATE SCHEMA "offchain"; | |
| CREATE SCHEMA IF NOT EXISTS "offchain"; |
| public async run(): Promise<void> { | ||
| // Do not allow multiple concurrent runs of the worker | ||
| if (this.isRunning) { | ||
| throw new Error("EnsDbWriterWorker is already running"); | ||
| } | ||
|
|
||
| const migrationsFolder = resolveMigrationsFolder(); | ||
| console.log(`[EnsDbWriterWorker]: Running ENSDb migrations from ${migrationsFolder}...`); | ||
| await this.ensDbClient.runMigrations(migrationsFolder); | ||
| console.log(`[EnsDbWriterWorker]: ENSDb migrations completed`); |
There was a problem hiding this comment.
The isRunning guard only becomes true after indexingStatusInterval is set near the end of run(). With the newly added potentially-slow migration step, there’s now a larger window where run() can be invoked twice concurrently and both calls will pass the isRunning check, leading to duplicated upserts and multiple intervals. Consider adding an explicit starting/running flag that’s set immediately on entry (and cleared on failure), or set indexingStatusInterval (or another sentinel) before awaiting migrations.
| async runMigrations(migrationsFolder: string): Promise<void> { | ||
| await migrate(this.db, { migrationsFolder }); | ||
| } |
There was a problem hiding this comment.
runMigrations() is a new public method on EnsDbClient and is now a required call path from EnsDbWriterWorker, but there’s no unit test asserting it wires through to Drizzle’s migrate() with the provided folder. Since this file already has dedicated unit tests, consider adding a small test that mocks drizzle-orm/node-postgres/migrator and verifies the correct call.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/ensnode-schema/src/schemas/ensnode-metadata.schema.ts (1)
7-17:⚠️ Potential issue | 🟠 MajorUpdate mock and test configurations to use
offchainschema consistently.The schema move to
offchainis correctly implemented in the schema definition and imported properly byensdb-client.ts. However, test configurations still reference the old schema:
apps/ensindexer/src/lib/ensdb-client/ensdb-client.mock.tsline 24:databaseSchemaName = "public"packages/ensnode-sdk/src/ensindexer/config/conversions.test.tsandzod-schemas.test.ts: hardcodeddatabaseSchemaName: "public"Update these test mocks and fixtures to use
"offchain"to match the schema move, or if these tests are meant to be schema-agnostic, ensure the configuration is properly sourced from a shared test constant.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/ensnode-schema/src/schemas/ensnode-metadata.schema.ts` around lines 7 - 17, Tests and mocks still reference the old schema name "public"; update the test fixtures so the database schema used matches the new offchain schema by changing the mock constant databaseSchemaName (in ensdb-client.mock.ts) and the hardcoded test configs in conversions.test and zod-schemas.test from "public" to "offchain", or better, import/consume a shared constant that exposes offchainSchema (the same symbol used by offchainSchema/ensNodeMetadata) so tests remain schema-agnostic.
🤖 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/drizzle.config.ts`:
- Around line 7-9: Remove the hardcoded DB URL in the dbCredentials.url setting
and replace it with a secure configuration source (e.g., read from environment
variable or a secrets/config service); update the code that sets dbCredentials
(the dbCredentials object and its url property) to use process.env (or your
project's config loader) and validate presence of the variable at startup,
failing fast with a clear error if it's missing.
In `@apps/ensindexer/package.json`:
- Line 50: The dependency entry for "drizzle-kit" in package.json is pinned to a
hardcoded version ("0.30.1"); update the "drizzle-kit" version specifier to use
your workspace catalog syntax (e.g., replace the literal "0.30.1" with the
catalog reference used elsewhere) and then add that version to the pnpm catalog
configuration so the workspace uses the centralized version management; locate
the "drizzle-kit" key in the dependencies section of package.json to make this
change.
In `@apps/ensindexer/src/lib/ensdb-client/ensdb-client.ts`:
- Line 3: Remove the unused `sql` named import from the import statement that
currently reads `import { eq, sql } from "drizzle-orm/sql";` in ensdb-client.ts;
leave only the used symbol(s) such as `eq` so the file no longer imports `sql`,
which will resolve the CI unused-import failure.
In `@apps/ensindexer/src/lib/ensdb-writer-worker/ensdb-writer-worker.test.ts`:
- Line 83: The test currently only asserts call count for
ensDbClient.runMigrations; update it to also assert the migration-folder
argument is correct by checking the actual call arguments (e.g., use
toHaveBeenCalledWith or toHaveBeenCalledTimes(1) plus
expect(ensDbClient.runMigrations).toHaveBeenCalledWith(expectedMigrationsPath,
...)) — locate the assertion in ensdb-writer-worker.test.ts around the
expect(ensDbClient.runMigrations).toHaveBeenCalledTimes(1) line and replace or
augment it to verify the passed migrations folder value (reference the expected
variable or hard-coded path used by the worker).
In `@packages/ensnode-schema/package.json`:
- Around line 19-20: The package exports include "./offchain" but tsup isn't
building src/offchain.schema.ts; update the tsup entry list in tsup.config.ts to
include "src/offchain.schema.ts" alongside "src/ponder.schema.ts" so that
dist/offchain.schema.js and dist/offchain.schema.d.ts are generated, and update
package.json's publishConfig.exports to add an "./offchain" export pointing to
the built files (e.g., the dist/offchain.schema.js and its types) so published
consumers can import the offchain entry.
---
Outside diff comments:
In `@packages/ensnode-schema/src/schemas/ensnode-metadata.schema.ts`:
- Around line 7-17: Tests and mocks still reference the old schema name
"public"; update the test fixtures so the database schema used matches the new
offchain schema by changing the mock constant databaseSchemaName (in
ensdb-client.mock.ts) and the hardcoded test configs in conversions.test and
zod-schemas.test from "public" to "offchain", or better, import/consume a shared
constant that exposes offchainSchema (the same symbol used by
offchainSchema/ensNodeMetadata) so tests remain schema-agnostic.
ℹ️ Review info
Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 80d50ed4-57ba-4611-8400-cb65872e61d5
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (13)
apps/ensindexer/drizzle.config.tsapps/ensindexer/drizzle.schema.tsapps/ensindexer/drizzle/0000_orange_veda.sqlapps/ensindexer/drizzle/meta/0000_snapshot.jsonapps/ensindexer/drizzle/meta/_journal.jsonapps/ensindexer/package.jsonapps/ensindexer/src/lib/ensdb-client/ensdb-client.tsapps/ensindexer/src/lib/ensdb-writer-worker/ensdb-writer-worker.test.tsapps/ensindexer/src/lib/ensdb-writer-worker/ensdb-writer-worker.tspackages/ensnode-schema/package.jsonpackages/ensnode-schema/src/offchain.schema.tspackages/ensnode-schema/src/ponder.schema.tspackages/ensnode-schema/src/schemas/ensnode-metadata.schema.ts
💤 Files with no reviewable changes (1)
- packages/ensnode-schema/src/ponder.schema.ts
| dbCredentials: { | ||
| url: "postgresql://dbuser:abcd1234@localhost:54320/ensnode_local_ponder_0_16", | ||
| }, |
There was a problem hiding this comment.
Remove hardcoded database credentials from source.
Line 8 commits a concrete DB user/password, which is a security and configuration-management risk.
Proposed secure config change
import { defineConfig } from "drizzle-kit";
+const databaseUrl = process.env.ENSDB_DATABASE_URL;
+if (!databaseUrl) {
+ throw new Error("ENSDB_DATABASE_URL is required");
+}
+
export default defineConfig({
dialect: "postgresql",
schema: "./drizzle.schema.ts",
out: "./drizzle",
dbCredentials: {
- url: "postgresql://dbuser:abcd1234@localhost:54320/ensnode_local_ponder_0_16",
+ url: databaseUrl,
},
});📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| dbCredentials: { | |
| url: "postgresql://dbuser:abcd1234@localhost:54320/ensnode_local_ponder_0_16", | |
| }, | |
| import { defineConfig } from "drizzle-kit"; | |
| const databaseUrl = process.env.ENSDB_DATABASE_URL; | |
| if (!databaseUrl) { | |
| throw new Error("ENSDB_DATABASE_URL is required"); | |
| } | |
| export default defineConfig({ | |
| dialect: "postgresql", | |
| schema: "./drizzle.schema.ts", | |
| out: "./drizzle", | |
| dbCredentials: { | |
| url: databaseUrl, | |
| }, | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/ensindexer/drizzle.config.ts` around lines 7 - 9, Remove the hardcoded
DB URL in the dbCredentials.url setting and replace it with a secure
configuration source (e.g., read from environment variable or a secrets/config
service); update the code that sets dbCredentials (the dbCredentials object and
its url property) to use process.env (or your project's config loader) and
validate presence of the variable at startup, failing fast with a clear error if
it's missing.
| "@types/dns-packet": "^5.6.5", | ||
| "@types/node": "catalog:", | ||
| "@types/pg": "8.16.0", | ||
| "drizzle-kit": "0.30.1", |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider using catalog: for drizzle-kit version.
Other dependencies in this file use catalog: for version management. Using a hardcoded version (0.30.1) for drizzle-kit breaks that consistency and may lead to version drift across workspaces.
♻️ Suggested change
- "drizzle-kit": "0.30.1",
+ "drizzle-kit": "catalog:",Then add the version to your pnpm catalog configuration.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| "drizzle-kit": "0.30.1", | |
| "drizzle-kit": "catalog:", |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/ensindexer/package.json` at line 50, The dependency entry for
"drizzle-kit" in package.json is pinned to a hardcoded version ("0.30.1");
update the "drizzle-kit" version specifier to use your workspace catalog syntax
(e.g., replace the literal "0.30.1" with the catalog reference used elsewhere)
and then add that version to the pnpm catalog configuration so the workspace
uses the centralized version management; locate the "drizzle-kit" key in the
dependencies section of package.json to make this change.
| @@ -1,7 +1,8 @@ | |||
| import type { NodePgDatabase } from "drizzle-orm/node-postgres"; | |||
| import { migrate } from "drizzle-orm/node-postgres/migrator"; | |||
| import { eq, sql } from "drizzle-orm/sql"; | |||
There was a problem hiding this comment.
Remove unused sql import to fix CI failure.
The pipeline reports unused imports. The sql import is no longer used after the upsert refactor.
🧹 Suggested fix
-import { eq, sql } from "drizzle-orm/sql";
+import { eq } from "drizzle-orm/sql";📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| import { eq, sql } from "drizzle-orm/sql"; | |
| import { eq } from "drizzle-orm/sql"; |
🧰 Tools
🪛 GitHub Actions: Test: CI
[warning] 3-14: Unused imports might be the result of an incomplete refactoring.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/ensindexer/src/lib/ensdb-client/ensdb-client.ts` at line 3, Remove the
unused `sql` named import from the import statement that currently reads `import
{ eq, sql } from "drizzle-orm/sql";` in ensdb-client.ts; leave only the used
symbol(s) such as `eq` so the file no longer imports `sql`, which will resolve
the CI unused-import failure.
| await worker.run(); | ||
|
|
||
| // assert - verify initial upserts happened | ||
| expect(ensDbClient.runMigrations).toHaveBeenCalledTimes(1); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Assert migration-folder argument, not only call count.
Line 83 only verifies invocation count; it won’t catch an incorrect folder being passed to runMigrations.
Suggested test hardening
- expect(ensDbClient.runMigrations).toHaveBeenCalledTimes(1);
+ expect(ensDbClient.runMigrations).toHaveBeenCalledTimes(1);
+ expect(ensDbClient.runMigrations).toHaveBeenCalledWith(
+ expect.stringMatching(/(?:\/|\\)drizzle$/),
+ );📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| expect(ensDbClient.runMigrations).toHaveBeenCalledTimes(1); | |
| expect(ensDbClient.runMigrations).toHaveBeenCalledTimes(1); | |
| expect(ensDbClient.runMigrations).toHaveBeenCalledWith( | |
| expect.stringMatching(/(?:\/|\\)drizzle$/), | |
| ); |
🤖 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` at
line 83, The test currently only asserts call count for
ensDbClient.runMigrations; update it to also assert the migration-folder
argument is correct by checking the actual call arguments (e.g., use
toHaveBeenCalledWith or toHaveBeenCalledTimes(1) plus
expect(ensDbClient.runMigrations).toHaveBeenCalledWith(expectedMigrationsPath,
...)) — locate the assertion in ensdb-writer-worker.test.ts around the
expect(ensDbClient.runMigrations).toHaveBeenCalledTimes(1) line and replace or
augment it to verify the passed migrations folder value (reference the expected
variable or hard-coded path used by the worker).
| ".": "./src/ponder.schema.ts", | ||
| "./offchain": "./src/offchain.schema.ts" |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's examine the full package.json structure for ensnode-schema
cat packages/ensnode-schema/package.jsonRepository: namehash/ensnode
Length of output: 1355
🏁 Script executed:
# Check for tsup configuration
fd -t f 'tsup' packages/ensnode-schema/ --exec cat {}Repository: namehash/ensnode
Length of output: 388
🏁 Script executed:
# Check if offchain.schema.ts exists
ls -la packages/ensnode-schema/src/ | grep -E '\.ts$'Repository: namehash/ensnode
Length of output: 191
🏁 Script executed:
# Look for any build configuration in the ensnode-schema directory
ls -la packages/ensnode-schema/Repository: namehash/ensnode
Length of output: 540
🏁 Script executed:
# Check the content of offchain.schema.ts to understand its purpose
cat packages/ensnode-schema/src/offchain.schema.tsRepository: namehash/ensnode
Length of output: 110
🏁 Script executed:
# Check if there are any references to offchain in the codebase to understand usage intent
rg "offchain.schema" --type ts --type jsonRepository: namehash/ensnode
Length of output: 141
Add offchain.schema.ts to the tsup build configuration and update publishConfig.exports.
The exports field declares ./offchain but tsup is not configured to build it. The tsup.config.ts entry array only includes src/ponder.schema.ts, so dist/offchain.schema.d.ts and dist/offchain.schema.js files will not be generated. Additionally, publishConfig.exports must be updated to declare the offchain entry point for published consumers.
🔧 Suggested fixes
Update tsup.config.ts to include the offchain entry:
export default defineConfig({
- entry: ["src/ponder.schema.ts"],
+ entry: ["src/ponder.schema.ts", "src/offchain.schema.ts"],
platform: "neutral",Update publishConfig in package.json to include the offchain export:
"publishConfig": {
"access": "public",
"exports": {
+ ".": {
"types": "./dist/ponder.schema.d.ts",
"default": "./dist/ponder.schema.js"
+ },
+ "./offchain": {
+ "types": "./dist/offchain.schema.d.ts",
+ "default": "./dist/offchain.schema.js"
+ }
},🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/ensnode-schema/package.json` around lines 19 - 20, The package
exports include "./offchain" but tsup isn't building src/offchain.schema.ts;
update the tsup entry list in tsup.config.ts to include "src/offchain.schema.ts"
alongside "src/ponder.schema.ts" so that dist/offchain.schema.js and
dist/offchain.schema.d.ts are generated, and update package.json's
publishConfig.exports to add an "./offchain" export pointing to the built files
(e.g., the dist/offchain.schema.js and its types) so published consumers can
import the offchain entry.
![vercel[bot]](https://avatars.githubusercontent.com/in/8329?s=60&v=4){kind=small}
Lite PR
Tip: Review docs on the ENSNode PR process
Summary
Why
Testing
pnpm -F ensindexer exec drizzle-kit generate --config drizzle.config.tsdatabaseSchemaNametooffchainbefore passing it as a consturctor param toEnsDbClient.pluginsconfig field).Notes for Reviewer (Optional)
Pre-Review Checklist (Blocking)