-
Notifications
You must be signed in to change notification settings - Fork 197
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): sync to sqlite #1185
Conversation
🦋 Changeset detectedLatest commit: 3502df6 The changes in this PR will be included in the next version bump. This PR includes changesets to release 26 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 |
so they can be used outside of block logs stream if needed
b4abc6b
to
fd29902
Compare
tx.insert(chainState) | ||
.values({ | ||
schemaVersion, | ||
chainId: publicClient.chain.id, |
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 seems like the only reason we need a viem publicClient
in here is for the chain id - would it be sufficient to just pass in the chain id?
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.
there's a TODO above to ask the RPC for missing tables: https://github.com/latticexyz/mud/pull/1185/files#diff-57e424c9e734e8f90ffab2a6b59de8a4b9134d0f646207b502a555da87c3402fR62
I was just deferring this for now because it'll be soon refactored with the table registration changes.
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.
gotcha, makes sense
export function sqliteTableToSql(table: SQLiteTableWithColumns<any>): string { | ||
const tableName = getTableName(table); | ||
|
||
// db.schema.dropTable(tableName).ifExists().compile(); |
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.
do we need this or should we remove the comment?
col = col.notNull(); | ||
} | ||
if (column.hasDefault && typeof column.default !== "undefined") { | ||
// col = col.defaultTo(column.mapToDriverValue(column.default)); |
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.
to we need this or should we remove the comment?
|
||
await blockLogsToStorage(storageAdapter)({ | ||
blockNumber: 5448n, | ||
logs: [ |
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.
This kind of tests both blockLogsToStorage
and sqliteStorage
in one because it used to be combined (we'd wrap blockLogsToStorage
).
I feel like "integration" tests like this tend to be more accurate/useful so I kept it, but happy to relocate and save this file for more unit tests.
pulled out of #1113