Skip to content
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.

Sign up for GitHub

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

Jump to bottom

feat(store-indexer,store-sync): filter findAll by tableIds #1572

Merged
merged 6 commits into from
Sep 29, 2023
Merged

feat(store-indexer,store-sync): filter findAll by tableIds #1572

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
5d06ad2
feat(store-indexer,store-sync): support filtering indexer query by ta…
holic Sep 22, 2023
a5655ab
fix filter
holic Sep 22, 2023
4f4c7e7
include internal table IDs
holic Sep 29, 2023
004b0c4
sets
holic Sep 29, 2023
72033de
Merge remote-tracking branch 'origin/main' into holic/indexer-tableids
holic Sep 29, 2023
941c863
make indexer dumber
holic Sep 29, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 7 additions & 5 deletions packages/store-indexer/src/postgres/createQueryAdapter.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import { buildTable, buildInternalTables, getTables } from "@latticexyz/store-sy
import { QueryAdapter } from "@latticexyz/store-sync/trpc-indexer";
import { debug } from "../debug";
import { getAddress } from "viem";
import { internalTableIds } from "@latticexyz/store-sync";

/**
* Creates a query adapter for the tRPC server/client to query data from Postgres.
Expand All @@ -13,11 +14,11 @@ import { getAddress } from "viem";
*/
export async function createQueryAdapter(database: PgDatabase<any>): Promise<QueryAdapter> {
const adapter: QueryAdapter = {
async findAll(chainId, address) {
const internalTables = buildInternalTables();
const tables = (await getTables(database)).filter(
(table) => address != null && getAddress(address) === getAddress(table.address)
);
async findAll({ chainId, address, tableIds = [] }) {
const includedTableIds = new Set(tableIds.length ? [...internalTableIds, ...tableIds] : []);
const tables = (await getTables(database))
.filter((table) => address == null || getAddress(address) === getAddress(table.address))
.filter((table) => !includedTableIds.size || includedTableIds.has(table.tableId));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

general q, not related to this PR: should we combine this adapter with the one defined in sqlite?

On the same note, a couple lines below this we have const sqliteTable = builtTable(table), should that be postgresTable? Is it the same object type? (Can't comment directly on it bc it didn't change in this PR)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would like to, but there's some API differences between drizzle's postgres and sqlite adapters that makes it hard to combine these.

Also the sqlite patterns here are a bit behind - I cleaned things up in the postgres, but didn't go back to refactor sqlite to match.


const tablesWithRecords = await Promise.all(
tables.map(async (table) => {
Expand All @@ -33,6 +34,7 @@ export async function createQueryAdapter(database: PgDatabase<any>): Promise<Que
})
);

const internalTables = buildInternalTables();
const metadata = await database
.select()
.from(internalTables.chain)
Expand Down
9 changes: 7 additions & 2 deletions packages/store-indexer/src/sqlite/createQueryAdapter.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,8 @@ import { BaseSQLiteDatabase } from "drizzle-orm/sqlite-core";
import { buildTable, chainState, getTables } from "@latticexyz/store-sync/sqlite";
import { QueryAdapter } from "@latticexyz/store-sync/trpc-indexer";
import { debug } from "../debug";
import { getAddress } from "viem";
import { internalTableIds } from "@latticexyz/store-sync";

/**
* Creates a storage adapter for the tRPC server/client to query data from SQLite.
Expand All @@ -12,8 +14,11 @@ import { debug } from "../debug";
*/
export async function createQueryAdapter(database: BaseSQLiteDatabase<"sync", any>): Promise<QueryAdapter> {
const adapter: QueryAdapter = {
async findAll(chainId, address) {
const tables = getTables(database).filter((table) => table.address === address);
async findAll({ chainId, address, tableIds = [] }) {
const includedTableIds = new Set(tableIds.length ? [...internalTableIds, ...tableIds] : []);
const tables = getTables(database)
.filter((table) => address == null || getAddress(address) === getAddress(table.address))
.filter((table) => !includedTableIds.size || includedTableIds.has(table.tableId));

const tablesWithRecords = tables.map((table) => {
const sqliteTable = buildTable(table);
Expand Down
4 changes: 4 additions & 0 deletions packages/store-sync/src/common.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,10 @@ export type SyncOptions<TConfig extends StoreConfig = StoreConfig> = {
* MUD Store/World contract address
*/
address?: Address;
/**
* Optional table IDs to filter indexer state and RPC state.
*/
tableIds?: Hex[];
/**
* Optional block number to start indexing from. Useful for resuming the indexer from a particular point in time or starting after a particular contract deployment.
*/
Expand Down
7 changes: 5 additions & 2 deletions packages/store-sync/src/createStoreSync.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@ import { createIndexerClient } from "./trpc-indexer";
import { SyncStep } from "./SyncStep";
import { chunk, isDefined } from "@latticexyz/common/utils";
import { encodeKey, encodeValueArgs } from "@latticexyz/protocol-parser";
import { internalTableIds } from "./internalTableIds";

const debug = parentDebug.extend("createStoreSync");

Expand All @@ -49,13 +50,15 @@ type CreateStoreSyncOptions<TConfig extends StoreConfig = StoreConfig> = SyncOpt
export async function createStoreSync<TConfig extends StoreConfig = StoreConfig>({
storageAdapter,
onProgress,
address,
publicClient,
address,
tableIds = [],
startBlock: initialStartBlock = 0n,
maxBlockRange,
initialState,
indexerUrl,
}: CreateStoreSyncOptions<TConfig>): Promise<SyncResult> {
const includedTableIds = new Set(tableIds.length ? [...internalTableIds, ...tableIds] : []);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it looks like we actively pass internalTableIds here, but also always include it internally in the query adapter - doesn't really hurt bc it's a set, but feels like it could be confusing (e.g. someone removing internalTableIds here and then wondering why they still show up in the results). What are your thoughts on only putting it in one of the two places and making it an explicit assumption? (e.g. "findAll always returns internal tables", or "findAll returns only the tables passed in the query, it is recommended internal tables are always included in the query")

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think making the indexer dumber (uses whatever you pass in for table IDs) makes sense, and only the store-sync includes internal table IDs.

const initialState$ = defer(
async (): Promise<
| {
Expand All @@ -79,7 +82,7 @@ export async function createStoreSync<TConfig extends StoreConfig = StoreConfig>

const indexer = createIndexerClient({ url: indexerUrl });
const chainId = publicClient.chain?.id ?? (await publicClient.getChainId());
const result = await indexer.findAll.query({ chainId, address });
const result = await indexer.findAll.query({ chainId, address, tableIds: Array.from(includedTableIds) });

onProgress?.({
step: SyncStep.SNAPSHOT,
Expand Down
1 change: 1 addition & 0 deletions packages/store-sync/src/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
export * from "./common";
export * from "./createStoreSync";
export * from "./internalTableIds";
export * from "./SyncStep";
23 changes: 23 additions & 0 deletions packages/store-sync/src/internalTableIds.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
import { resourceIdToHex } from "@latticexyz/common";
import storeConfig from "@latticexyz/store/mud.config";
import worldConfig from "@latticexyz/world/mud.config";

// TODO: refactor config to include table IDs (https://github.com/latticexyz/mud/pull/1561)

export const storeTableIds = Object.keys(storeConfig.tables).map((name) =>
resourceIdToHex({
type: storeConfig.tables[name as keyof typeof storeConfig.tables].offchainOnly ? "offchainTable" : "table",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

as keyof typeof storeConfig.table sad that typescript can't infer that from Object.keys(storeConfig.tables) 😢

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

agreed! I think lodash has a thing for this, just waiting for them to finish their TS rewrite

namespace: storeConfig.namespace,
name,
})
);

const worldTableIds = Object.keys(worldConfig.tables).map((name) =>
resourceIdToHex({
type: worldConfig.tables[name as keyof typeof worldConfig.tables].offchainOnly ? "offchainTable" : "table",
namespace: worldConfig.namespace,
name,
})
);

export const internalTableIds = [...storeTableIds, ...worldTableIds];
5 changes: 1 addition & 4 deletions packages/store-sync/src/trpc-indexer/common.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,10 +2,7 @@ import { Hex } from "viem";
import { TableWithRecords } from "../common";

export type QueryAdapter = {
findAll: (
chainId: number,
address?: Hex
) => Promise<{
findAll: (opts: { chainId: number; address?: Hex; tableIds?: Hex[] }) => Promise<{
blockNumber: bigint | null;
tables: TableWithRecords[];
}>;
Expand Down
5 changes: 3 additions & 2 deletions packages/store-sync/src/trpc-indexer/createAppRouter.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,12 +16,13 @@ export function createAppRouter() {
z.object({
chainId: z.number(),
address: z.string().refine(isHex).optional(),
tableIds: z.array(z.string().refine(isHex)).optional(),
})
)
.query(async (opts): ReturnType<QueryAdapter["findAll"]> => {
const { queryAdapter } = opts.ctx;
const { chainId, address } = opts.input;
return queryAdapter.findAll(chainId, address);
const { chainId, address, tableIds } = opts.input;
return queryAdapter.findAll({ chainId, address, tableIds });
}),
});
}
Expand Down
Loading