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-sync,store-indexer): sync from getLogs indexer endpoint #1973

Merged
merged 21 commits into from Dec 1, 2023

Conversation

holic
Copy link
Member

@holic holic commented Nov 29, 2023

No description provided.

Copy link

changeset-bot bot commented Nov 29, 2023

🦋 Changeset detected

Latest commit: 74921c7

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-sync Major
@latticexyz/common Major
@latticexyz/store-indexer Major
@latticexyz/dev-tools Major
@latticexyz/block-logs-stream Major
@latticexyz/cli Major
@latticexyz/config Major
@latticexyz/protocol-parser Major
@latticexyz/store Major
@latticexyz/world-modules Major
@latticexyz/world Major
@latticexyz/react Major
@latticexyz/abi-ts 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/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/utils 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

@holic holic changed the title WIP sync from indexer logs feat(store-sync,store-indexer): sync from getLogs indexer endpoint Nov 30, 2023
@holic holic marked this pull request as ready for review November 30, 2023 14:43
@holic holic requested a review from alvrs as a code owner November 30, 2023 14:43
alvrs
alvrs previously approved these changes Nov 30, 2023
Comment on lines 57 to 62
.orderBy(
asc(tables.recordsTable.lastUpdatedBlockNumber),
// TODO: add logIndex and use that to sort instead of address/tableId? (https://github.com/latticexyz/mud/issues/1979)
asc(tables.recordsTable.address),
asc(tables.recordsTable.tableId)
);
Copy link
Member

Choose a reason for hiding this comment

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

for context, why do we have to order the result 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.

it was an attempt to fix the e2e tests having inconsistent results between rpc and indexer as far how data is populated, but ended up not being enough

I fixed the e2e test here and need to bring this commit over: 74611df

ultimately we lose some fidelity/ordering when hydrating from the indexer because everything is done at the block level, so records pulled from the DB within the same block may not be exactly the same order as records created from a list of logs from the RPC (mostly due to a lack of logIndex)

this isn't really an issue practically speaking, but felt like it would be nice to have some consistency between rpc and indexer in terms of data returned and its ordering

Copy link
Member

Choose a reason for hiding this comment

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

do we still need the sorting now? unclear if it affects performance at all

Copy link
Member Author

@holic holic Dec 1, 2023

Choose a reason for hiding this comment

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

we should still prob order by block number, but maybe not others

I think these will still take advantage of the index, but will remove for now

alvrs
alvrs previously approved these changes Dec 1, 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.

lgtm, only question i have is if we can skip the overhead of sorting if it doesn't solve a problem

@holic holic merged commit 5df1f31 into main Dec 1, 2023
11 checks passed
@holic holic deleted the holic/sync-indexer-logs branch December 1, 2023 13:37
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.

None yet

2 participants