-
Notifications
You must be signed in to change notification settings - Fork 17
Update ENSDb schema definitions #1752
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 9 commits
6acbea0
eed4800
16c5584
92afddf
b5bd5ca
3f7f0e8
c1964a2
b4fb135
6b6f805
9c05883
f13b6af
1791217
03fecc1
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,5 @@ | ||
| --- | ||
| "@ensnode/ensnode-schema": minor | ||
| --- | ||
|
|
||
| Split database schemas into Ponder Schema, ENSIndexer Schema, and ENSNode Schema. |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,5 @@ | ||
| --- | ||
| "ensapi": minor | ||
| --- | ||
|
|
||
| Updated ENSDb connections to be always read-only. | ||
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -2,9 +2,12 @@ import config from "@/config"; | |||||
|
|
||||||
| import * as schema from "@ensnode/ensnode-schema"; | ||||||
|
|
||||||
| import { makeDrizzle } from "@/lib/handlers/drizzle"; | ||||||
| import { makeReadOnlyDrizzle } from "@/lib/handlers/drizzle"; | ||||||
|
|
||||||
| export const db = makeDrizzle({ | ||||||
| /** | ||||||
| * Read-only Drizzle instance for ENSDb queries to ENSIndexer Schema | ||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. How should we think about which db schemas ENSApi reads from? My understanding is it should read from both the ENSNode and ENSIndexer schemas. But here it's only reading from ENSIndexer. Why is that? Assuming ENSApi really does read from multiple schemas than that would suggest it's wrong to call this variable something generic such as Please invest more effort into naming and communicating ideas. |
||||||
| */ | ||||||
| export const db = makeReadOnlyDrizzle({ | ||||||
| databaseUrl: config.databaseUrl, | ||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Please see my other related comment about renaming things in a follow-up issue. I'd also like to see Eager to make all our terminology use be 100% precise, consistent, and aligned everywhere.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Issue created here: |
||||||
| databaseSchema: config.databaseSchemaName, | ||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Please create a follow-up issue for us to rename all the environment variable names / other variable names to be more precise and consistent in how they relate to our improved architecture. For example: We should stop using the generic Suggest to create a follow-up issue for this and action it in a separate PR.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Issue created here: |
||||||
| schema, | ||||||
|
|
||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -23,6 +23,10 @@ export const databaseUrl = "postgres://user:pass@localhost:5432/ensdb"; | |
|
|
||
| export const databaseSchemaName = "public"; | ||
|
|
||
| // This is the same as the default value of config.databaseSchemaName, | ||
| // which is used as the ensIndexerRef for multi-tenancy in ENSDb. | ||
| export const ensIndexerRef = databaseSchemaName; | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fix the bad naming. |
||
|
|
||
| export const publicConfig = { | ||
| databaseSchemaName, | ||
| ensRainbowPublicConfig: { | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,7 +1,7 @@ | ||
| import type { NodePgDatabase } from "drizzle-orm/node-postgres"; | ||
| import { eq, sql } from "drizzle-orm/sql"; | ||
| import { and, eq, sql } from "drizzle-orm/sql"; | ||
|
tk-o marked this conversation as resolved.
Dismissed
tk-o marked this conversation as resolved.
tk-o marked this conversation as resolved.
|
||
|
|
||
| import { ensNodeMetadata } from "@ensnode/ensnode-schema"; | ||
| import * as ensNodeSchema from "@ensnode/ensnode-schema/ensnode"; | ||
| import { | ||
| type CrossChainIndexingStatusSnapshot, | ||
| deserializeCrossChainIndexingStatusSnapshot, | ||
|
|
@@ -20,21 +20,12 @@ import { | |
|
|
||
| import { makeDrizzle } from "./drizzle"; | ||
|
|
||
| /** | ||
| * ENSDb Client Schema | ||
| * | ||
| * Includes schema definitions for {@link EnsDbClient} queries and mutations. | ||
| */ | ||
| const schema = { | ||
| ensNodeMetadata, | ||
| }; | ||
|
|
||
| /** | ||
| * Drizzle database | ||
| * | ||
| * Allows interacting with Postgres database for ENSDb, using Drizzle ORM. | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I find it confusing exactly what this class has responsibility for. Is it for everything in an ENSDb, or is it only for the ENSNode schema in an ENSDb? Please make all the naming and use of language 100% precise and clear. |
||
| */ | ||
| interface DrizzleDb extends NodePgDatabase<typeof schema> {} | ||
| interface DrizzleDb extends NodePgDatabase<typeof ensNodeSchema> {} | ||
|
|
||
| /** | ||
| * ENSDb Client | ||
|
|
@@ -53,16 +44,23 @@ export class EnsDbClient implements EnsDbClientQuery, EnsDbClientMutation { | |
| */ | ||
| private db: DrizzleDb; | ||
|
|
||
| /** | ||
| * ENSIndexer reference string for multi-tenancy in ENSDb. | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Be more precise. Multi-tenancy of what exactly? Always write for an assumed audience that isn't familiar with all the context yet. |
||
| */ | ||
| private ensIndexerRef: string; | ||
|
|
||
| /** | ||
| * @param databaseUrl connection string for ENSDb Postgres database | ||
| * @param databaseSchemaName Postgres schema name for ENSDb tables | ||
| * @param ensIndexerRef reference string for ENSIndexer instance (used for multi-tenancy in ENSDb) | ||
| */ | ||
| constructor(databaseUrl: string, databaseSchemaName: string) { | ||
| constructor(databaseUrl: string, ensIndexerRef: string) { | ||
| this.db = makeDrizzle({ | ||
| databaseSchema: databaseSchemaName, | ||
| databaseSchema: ensNodeSchema.ENSNODE_SCHEMA_NAME, | ||
|
tk-o marked this conversation as resolved.
Outdated
|
||
| databaseUrl, | ||
| schema, | ||
| schema: ensNodeSchema, | ||
| }); | ||
|
tk-o marked this conversation as resolved.
Comment on lines
+56
to
60
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ship the This client now hard-depends on the Also applies to: 182-190 🤖 Prompt for AI Agents
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
🧠 Learnings used |
||
|
|
||
| this.ensIndexerRef = ensIndexerRef; | ||
|
tk-o marked this conversation as resolved.
vercel[bot] marked this conversation as resolved.
|
||
| } | ||
|
|
||
| /** | ||
|
|
@@ -154,8 +152,13 @@ export class EnsDbClient implements EnsDbClientQuery, EnsDbClientMutation { | |
| ): Promise<EnsNodeMetadataType["value"] | undefined> { | ||
| const result = await this.db | ||
| .select() | ||
| .from(ensNodeMetadata) | ||
| .where(eq(ensNodeMetadata.key, metadata.key)); | ||
| .from(ensNodeSchema.ensNodeMetadata) | ||
| .where( | ||
|
tk-o marked this conversation as resolved.
|
||
| and( | ||
| eq(ensNodeSchema.ensNodeMetadata.ensIndexerRef, this.ensIndexerRef), | ||
| eq(ensNodeSchema.ensNodeMetadata.key, metadata.key), | ||
| ), | ||
| ); | ||
|
|
||
| if (result.length === 0) { | ||
| return undefined; | ||
|
|
@@ -186,13 +189,14 @@ export class EnsDbClient implements EnsDbClientQuery, EnsDbClientMutation { | |
| ); | ||
|
|
||
| await tx | ||
| .insert(ensNodeMetadata) | ||
| .insert(ensNodeSchema.ensNodeMetadata) | ||
| .values({ | ||
| ensIndexerRef: this.ensIndexerRef, | ||
| key: metadata.key, | ||
| value: metadata.value, | ||
| }) | ||
| .onConflictDoUpdate({ | ||
| target: ensNodeMetadata.key, | ||
| target: [ensNodeSchema.ensNodeMetadata.ensIndexerRef, ensNodeSchema.ensNodeMetadata.key], | ||
| set: { value: metadata.value }, | ||
| }); | ||
| }); | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -2,7 +2,11 @@ import config from "@/config"; | |
|
|
||
| import { EnsDbClient } from "./ensdb-client"; | ||
|
|
||
| // config.databaseSchemaName is unique per ENSIndexer instance and is used as the ensIndexerRef | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There's big naming problems here. I should not have the responsibility to catch and fix these. You should notice how there's 3 different terms being used for exactly the same idea here and fix it before sending to me for review.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. After reading feedback you shared, I see more clearly how this could just be |
||
| // tenant key in the shared ENSNode schema (ensnode.*). | ||
| const ensIndexerRef = config.databaseSchemaName; | ||
|
|
||
| /** | ||
| * Singleton instance of {@link EnsDbClient} for use in ENSIndexer. | ||
| */ | ||
| export const ensDbClient = new EnsDbClient(config.databaseUrl, config.databaseSchemaName); | ||
| export const ensDbClient = new EnsDbClient(config.databaseUrl, ensIndexerRef); | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -16,34 +16,60 @@ | |
| "Ponder" | ||
| ], | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We should update the |
||
| "exports": { | ||
| ".": "./src/ponder.schema.ts" | ||
| ".": "./src/index.ts", | ||
| "./ponder": "./src/ponder-schema/index.ts", | ||
| "./ensindexer": "./src/ensindexer-schema/index.ts", | ||
| "./ensnode": "./src/ensnode-schema/index.ts" | ||
| }, | ||
|
coderabbitai[bot] marked this conversation as resolved.
|
||
| "files": [ | ||
| "dist" | ||
| ], | ||
| "publishConfig": { | ||
| "access": "public", | ||
| "exports": { | ||
| "types": "./dist/ponder.schema.d.ts", | ||
| "default": "./dist/ponder.schema.js" | ||
| }, | ||
| "main": "./dist/ponder.schema.js", | ||
| "module": "./dist/ponder.schema.mjs", | ||
| "types": "./dist/ponder.schema.d.ts" | ||
| ".": { | ||
| "import": { | ||
| "types": "./dist/index.d.ts", | ||
| "default": "./dist/index.js" | ||
| } | ||
| }, | ||
| "./ponder": { | ||
| "import": { | ||
| "types": "./dist/ponder-schema/index.d.ts", | ||
| "default": "./dist/ponder-schema/index.js" | ||
| } | ||
| }, | ||
| "./ensindexer": { | ||
| "import": { | ||
| "types": "./dist/ensindexer-schema/index.d.ts", | ||
| "default": "./dist/ensindexer-schema/index.js" | ||
| } | ||
| }, | ||
| "./ensnode": { | ||
| "import": { | ||
| "types": "./dist/ensnode-schema/index.d.ts", | ||
| "default": "./dist/ensnode-schema/index.js" | ||
| } | ||
| } | ||
| } | ||
| }, | ||
| "scripts": { | ||
| "prepublish": "tsup", | ||
| "lint": "biome check --write .", | ||
| "lint:ci": "biome ci" | ||
| }, | ||
| "dependencies": { | ||
| "peerDependencies": { | ||
| "drizzle-orm": "catalog:", | ||
| "ponder": "catalog:", | ||
| "viem": "catalog:" | ||
| }, | ||
| "devDependencies": { | ||
| "@ensnode/ensnode-sdk": "workspace:", | ||
| "@ensnode/shared-configs": "workspace:*", | ||
| "drizzle-orm": "catalog:", | ||
| "ponder": "catalog:", | ||
| "tsup": "catalog:", | ||
| "typescript": "catalog:" | ||
| "typescript": "catalog:", | ||
| "viem": "catalog:" | ||
| } | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.