-
Notifications
You must be signed in to change notification settings - Fork 196
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
feat(store-sync,store-indexer): add postgres support #1338
Conversation
|
packages/store-indexer/package.json
Outdated
"start": "tsx bin/postgres-indexer", | ||
"start:local": "DATABASE_URL=http://127.0.0.1/postgres CHAIN_ID=31337 pnpm start", | ||
"start:testnet": "DATABASE_URL=http://127.0.0.1/postgres CHAIN_ID=4242 START_BLOCK=19037160 pnpm start", | ||
"start:testnet2": "DATABASE_URL=http://127.0.0.1/postgres CHAIN_ID=4243 pnpm start", |
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.
any opinions on whether we should keep this defaulting to sqlite and have separate scripts for postgres? sqlite feels a bit better for a quick-to-spin-up local thing (less overhead than e.g. ensuring postgres is running)
7de9c6b
to
83abbdf
Compare
This reverts commit 2f5761c.
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.
looking great! just a couple non-blocking nits/questions
@@ -54,14 +65,21 @@ describe("Sync from indexer", async () => { | |||
expect(asyncErrorHandler.getErrors()[0]).toContain("error fetching initial state from indexer"); | |||
}); | |||
|
|||
describe("indexer online", () => { | |||
describe.each([["sqlite"], ["postgres"]] as const)("%s indexer", (indexerType) => { |
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.
very cool 👍
e2e/packages/sync-test/package.json
Outdated
@@ -6,6 +6,9 @@ | |||
"scripts": { | |||
"test": "vitest --run" | |||
}, | |||
"dependencies": { | |||
"zod": "^3.22.2" |
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.
nit: i think this could be a dev dependency?
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.
oh sure, I guess the whole package is ~dev so I wasn't paying too close attention to it
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.
yeah it's not important, just OCD inducing that one dependency is not marked as dev
}, | ||
}); | ||
|
||
await server.listen({ port: env.PORT }); |
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.
should we make the host configurable here or do you want to update that for both sqlite and postgres indexer in a followup PR? (like e70bd42)
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.
oh yep, can add that here
operations: StorageOperation<TConfig>[]; | ||
}) => Promise<void>; | ||
}; | ||
export type BlockLogsToStorageOptions<TConfig extends StoreConfig = StoreConfig> = StorageAdapter<TConfig>; |
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.
Why do we need BlockLogsToStorageOptions
as separate type? Could we just use StorageAdapter
everywhere?
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.
I think this is just leftover from a refactor! Will clean up.
I am starting to prefer these explicit params+return types even if it's just a simple alias like this. But I don't feel that strongly.
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.
removing this revealed a lot more clean up work, so good call! feels much better now without it
// TODO: change schema version to varchar/text? | ||
schemaVersion: integer("schema_version").notNull().primaryKey(), |
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.
is this already in preparation for a protocol type?
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.
it's a reflection of the schemaVersion.ts
file (we have this in the sqlite indexer already) to help detect when the indexer meta tables change (the shape of the indexer DB as opposed to the shape of the MUD tables) so we can recreate them
I opened an issue for a better approach: #1362
fixes #1193
Mostly adapted from (i.e. copy pasted) sqlite -> postgres. The main changes are in handling schemas/namespaces (which sqlite doesn't have). I did a bit of refactoring too.
I left it as mostly duplicate code for now until we can figure out good abstractions. Drizzle makes it tricky because there aren't really shared types between types like
SqliteTable
andPgTable
. And executing queries is different for each flavor/dialect (sync vs async,run
vsexecute
).