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.

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-indexer,store-sync): filter by table and key #1794

Merged
merged 15 commits into from
Oct 25, 2023
Merged

Conversation

holic
Copy link
Member

@holic holic commented Oct 19, 2023

closes #1639

@changeset-bot
Copy link

changeset-bot bot commented Oct 19, 2023

🦋 Changeset detected

Latest commit: f7a366a

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 30 packages
Name Type
@latticexyz/store-indexer Major
@latticexyz/store-sync Major
@latticexyz/dev-tools Major
@latticexyz/abi-ts Major
@latticexyz/block-logs-stream Major
@latticexyz/cli Major
@latticexyz/common Major
@latticexyz/config Major
create-mud Major
@latticexyz/ecs-browser Major
@latticexyz/faucet Major
@latticexyz/gas-report Major
@latticexyz/network Major
@latticexyz/noise Major
@latticexyz/phaserx Major
@latticexyz/protocol-parser Major
@latticexyz/react Major
@latticexyz/recs Major
@latticexyz/schema-type Major
@latticexyz/services Major
@latticexyz/solecs Major
solhint-config-mud Major
solhint-plugin-mud Major
@latticexyz/std-client Major
@latticexyz/std-contracts Major
@latticexyz/store-cache Major
@latticexyz/store Major
@latticexyz/utils Major
@latticexyz/world-modules Major
@latticexyz/world Major

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

@@ -26,6 +26,14 @@ export type TableWithRecords = Table & { records: TableRecord[] };
export type StoreEventsLog = Log<bigint, number, false, StoreEventsAbiItem, true, StoreEventsAbi>;
export type BlockLogs = { blockNumber: StoreEventsLog["blockNumber"]; logs: StoreEventsLog[] };

// TODO: should we add address here?
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 it's best if we keep address outside of this to align with eth_getLogs filtering capability (where address is separate from topics): https://ethereum.org/en/developers/docs/apis/json-rpc/#eth_getlogs

@holic holic marked this pull request as ready for review October 24, 2023 14:54
@holic holic requested a review from alvrs as a code owner October 24, 2023 14:54
@holic holic changed the title feat(store-indexer,store-sync): filter by table and/or key feat(store-indexer,store-sync): filter by table and key Oct 24, 2023
await waitForInitialSync(page);

const entities = await callPageFunction(page, "getKeys", ["Position"]);
expect(entities).toEqual([
Copy link
Member Author

Choose a reason for hiding this comment

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

we can't use toMatchInlineSnapshot here due to the describe.each above

```ts
syncToRecs({
...
filters: [{ tableId: '0x...', key0: '0x...' }],
Copy link
Member

@alvrs alvrs Oct 24, 2023

Choose a reason for hiding this comment

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

i assume key0 and key1 need to be the Store encoded hex values correct? bytes32 encoded version of the key correct?

Copy link
Member Author

Choose a reason for hiding this comment

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

correct, yep! can add to the typedoc

@@ -13,7 +14,10 @@ import { getAddress } from "viem";
*/
export async function createQueryAdapter(database: PgDatabase<any>): Promise<QueryAdapter> {
const adapter: QueryAdapter = {
async findAll({ chainId, address, tableIds = [] }) {
async findAll({ chainId, address, filters = [] }) {
// If _any_ filter has a table ID, this will filter down all data to just those tables. Which mean we can't yet mix table filters with key-only filters.
Copy link
Member

Choose a reason for hiding this comment

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

Should we add this to the changeset? Might make it easier to update in the docs later.

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 ended up making tableId required for now, so this comment doesn't really apply but I left it here so that we revisit this once we make tableId optional.

alvrs
alvrs previously approved these changes Oct 24, 2023
Copy link
Member

@alvrs alvrs left a comment

Choose a reason for hiding this comment

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

🥳

@holic holic merged commit f6d214e into main Oct 25, 2023
9 checks passed
@holic holic deleted the holic/sync-filters branch October 25, 2023 09:19
@holic
Copy link
Member Author

holic commented Oct 25, 2023

Just making a note to myself: I decided to do the breaking change at the indexer level but probably should have done the backwards-compatibility one there too, to make it easier to safely bump the indexer without breaking clients (clients may start getting errors when sending tableIds args)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

indexer: filter by key
2 participants