-
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 5 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 |
|---|---|---|
|
|
@@ -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. 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 |
|---|---|---|
| @@ -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 |
|---|---|---|
|
|
@@ -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.d.ts", | ||
| "default": "./dist/ponder-schema.js" | ||
| } | ||
| }, | ||
| "./ensindexer": { | ||
| "import": { | ||
| "types": "./dist/ensindexer-schema.d.ts", | ||
| "default": "./dist/ensindexer-schema.js" | ||
| } | ||
| }, | ||
| "./ensnode": { | ||
| "import": { | ||
| "types": "./dist/ensnode-schema.d.ts", | ||
| "default": "./dist/ensnode-schema.js" | ||
|
tk-o marked this conversation as resolved.
Outdated
|
||
| } | ||
| } | ||
| } | ||
| }, | ||
| "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:" | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,9 @@ | ||
| /** | ||
| * Merge the various sub-schemas into ENSIndexer Schema. | ||
| */ | ||
|
|
||
| export * from "./ensv2.subschema"; | ||
| export * from "./protocol-acceleration.subschema"; | ||
| export * from "./registrars.subschema"; | ||
| export * from "./subgraph.subschema"; | ||
| export * from "./tokenscope.subschema"; |
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
| @@ -0,0 +1,66 @@ | ||||||
| /** | ||||||
| * ENSNode Schema | ||||||
| * | ||||||
| * Defines the database objects describing the ENSNode services state. | ||||||
|
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
|
||||||
| */ | ||||||
|
|
||||||
| import { primaryKey } from "drizzle-orm/pg-core"; | ||||||
|
|
||||||
| import { ENSNODE_SCHEMA } from "./schema"; | ||||||
|
|
||||||
| export { ENSNODE_SCHEMA_NAME } from "./schema"; | ||||||
|
|
||||||
| /** | ||||||
| * ENSNode Metadata | ||||||
| * | ||||||
| * Possible key value pairs are defined by 'EnsNodeMetadata' type: | ||||||
| * - `EnsNodeMetadataEnsDbVersion` | ||||||
| * - `EnsNodeMetadataEnsIndexerPublicConfig` | ||||||
| * - `EnsNodeMetadataEnsIndexerIndexingStatus` | ||||||
| */ | ||||||
| export const ensNodeMetadata = ENSNODE_SCHEMA.table( | ||||||
| "ensnode_metadata", | ||||||
| (t) => ({ | ||||||
| /** | ||||||
| * ENSIndexer Reference | ||||||
|
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. Why is this introducing another terminology? That sounds unnecessary and creates space for confusion. Why not call this the "ensIndexerSchemaName"? Why not reuse our existing terminology?
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. @lightwalker-eth Thanks for providing alternative option, this really makes feedback item actionable. I appreciate that you saw many opportunities for naming improvements, while sharing less clear feedback, for example, "bad naming". It's great for me to know what requires improvements, but it's even better to learn examples of acceptable altenatives. |
||||||
| * | ||||||
| * References the ENSIndexer instance by a unique ENSIndexer schema name | ||||||
| * that a metadata record is associated with. This allows us to support | ||||||
| * multiple ENSIndexer instances using the same database, while ensuring | ||||||
|
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
|
||||||
| * that their metadata records do not conflict with each other. | ||||||
| */ | ||||||
| ensIndexerRef: t.text().notNull(), | ||||||
|
|
||||||
| /** | ||||||
| * Key | ||||||
| * | ||||||
| * Allowed keys: | ||||||
| * - `EnsNodeMetadataEnsDbVersion['key']` | ||||||
| * - `EnsNodeMetadataEnsIndexerPublicConfig['key']` | ||||||
| * - `EnsNodeMetadataEnsIndexerIndexingStatus['key']` | ||||||
| */ | ||||||
| key: t.text().notNull(), | ||||||
|
|
||||||
| /** | ||||||
| * Value | ||||||
| * | ||||||
| * Allowed values: | ||||||
| * - `EnsNodeMetadataEnsDbVersion['value']` | ||||||
| * - `EnsNodeMetadataEnsIndexerPublicConfig['value']` | ||||||
| * - `EnsNodeMetadataEnsIndexerIndexingStatus['value']` | ||||||
| * | ||||||
| * Guaranteed to be a serialized representation of JSON object. | ||||||
| */ | ||||||
| value: t.jsonb().notNull(), | ||||||
| }), | ||||||
| (table) => [ | ||||||
| /** | ||||||
| * Primary key constraint on 'ensIndexerRef' and 'key' columns, | ||||||
| * to ensure that there is only one record for each key per ENSIndexer instance. | ||||||
| */ | ||||||
| primaryKey({ | ||||||
| name: "ensnode_metadata_pkey", | ||||||
| columns: [table.ensIndexerRef, table.key], | ||||||
| }), | ||||||
| ], | ||||||
| ); | ||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,5 @@ | ||
| import { pgSchema } from "drizzle-orm/pg-core"; | ||
|
|
||
| export const ENSNODE_SCHEMA_NAME = "ensnode"; | ||
|
|
||
| export const ENSNODE_SCHEMA = pgSchema(ENSNODE_SCHEMA_NAME); |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,2 @@ | ||
| // Re-export relevant schema definitions for backward compatibility. | ||
|
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. Which backwards compatibility are we working to preserve? My understanding is no one is using |
||
| export * from "./ensindexer-schema"; | ||
|
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. This file is modified in a later commit to only include reference to Ponder code repository. Feel free to skip reviewing it. |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,8 @@ | ||
| /** | ||
| * Ponder Schema | ||
| * | ||
| * Definition of the Ponder Schema can be found in the Ponder repository. | ||
| * @see https://github.com/ponder-sh/ponder/blob/ponder%400.16.3/packages/core/src/sync-store/schema.ts | ||
| */ | ||
|
|
||
| export const PONDER_SCHEMA_NAME = "ponder_sync"; | ||
|
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. Keeping this one to signal the name of Ponder Schema in database. |
||
This file was deleted.
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.