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): add support for watching pending logs #3287

Merged
merged 14 commits into from
Oct 16, 2024
Merged

Conversation

alvrs
Copy link
Member

@alvrs alvrs commented Oct 14, 2024

No description provided.

Copy link

changeset-bot bot commented Oct 14, 2024

🦋 Changeset detected

Latest commit: 554f484

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

This PR includes changesets to release 24 packages
Name Type
@latticexyz/store-sync Patch
@latticexyz/dev-tools Patch
@latticexyz/explorer Patch
@latticexyz/store-indexer Patch
@latticexyz/abi-ts Patch
@latticexyz/block-logs-stream Patch
@latticexyz/cli Patch
@latticexyz/common Patch
@latticexyz/config Patch
create-mud Patch
@latticexyz/faucet Patch
@latticexyz/gas-report Patch
@latticexyz/protocol-parser Patch
@latticexyz/react Patch
@latticexyz/recs Patch
@latticexyz/schema-type Patch
solhint-config-mud Patch
solhint-plugin-mud Patch
@latticexyz/stash Patch
@latticexyz/store Patch
@latticexyz/utils Patch
@latticexyz/world-module-metadata Patch
@latticexyz/world-modules Patch
@latticexyz/world Patch

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 previously approved these changes Oct 15, 2024
Copy link
Member

@holic holic left a comment

Choose a reason for hiding this comment

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

I think we need to move around the onProgress handler so that we can communicate to the client when we're hydrated. But I don't think the current watchLogs approach communicates that super well, so unclear how to add this behavior.

// (https://github.com/wevm/viem/blob/f81d497f2afc11b9b81a79057d1f797694b69793/src/utils/rpc/socket.ts#L178)
client.socket.addEventListener("message", (message) => {
const response = JSON.parse(message.data);
if ("error" in response) {
Copy link
Member

@holic holic Oct 15, 2024

Choose a reason for hiding this comment

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

Suggested change
if ("error" in response) {
if ("error" in response) {
// if error isn't associated with a request ID, ignore
// TODO: keep track of our request IDs and ignore others?
if ("id" in response && response.id != null) return;

Copy link
Member

Choose a reason for hiding this comment

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

actually could maybe lift this early return outside the error handler?

Copy link
Member

Choose a reason for hiding this comment

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

I discovered that something sending other random RPC calls (net_version specifically, so I think it was Metamask) to the websocket will cause this to close

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 should currently open a separate websocket connection for this log stream, did you experience any other errors come through here?

Copy link
Member

@holic holic Oct 15, 2024

Choose a reason for hiding this comment

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

yes, I saw net_version occasionally sent through this (and not sure where from or how it got a reference to this open socket, but my only guess is metamask) and that would fail on our node, causing the stream to close, until it was recently added to the whitelist

Copy link
Member

@holic holic Oct 16, 2024

Choose a reason for hiding this comment

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

Turns out net_version request comes from viem's ping method used by keepAlive option. The right move was to enable this at our node rather than fixing anything here.

And it would be good if viem keeps net_version as the ping method (rather than e.g. moving to eth_chainId) because we cache eth_chainId requests to reduce end-to-end latency of the transaction request lifecycle.

@holic holic merged commit 84ae33b into main Oct 16, 2024
13 checks passed
@holic holic deleted the pending-logs branch October 16, 2024 09:25
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.

2 participants