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

Add re-initialization logic for event filters #954

Merged
merged 5 commits into from
Aug 2, 2023
Merged

Conversation

jaxernst
Copy link
Contributor

@jaxernst jaxernst commented Jul 30, 2023

Here's my proposal to address #836. Filters can timeout or get into a bad state where reinitialization is necessary to continue getting filter changes on the polling interval. This request adds fallback reinitialization, so if 'getFilterChanges' fails repeatedly, it will attempt to recreate the filter.


PR-Codex overview

Focus of the PR:

This PR focuses on adding filter reinitialization logic for watchContractEvent and watchEvent when a filter has been uninstalled.

Detailed summary:

  • Added filter reinitialization logic for watchContractEvent and watchEvent when a filter has been uninstalled.
  • Imported InvalidInputRpcError in watchEvent.ts and watchContractEvent.ts.
  • Added error handling and filter reinitialization logic in watchEvent.ts and watchContractEvent.ts if a filter has been set and gets uninstalled.
  • Added a test case to verify the filter reinitialization logic in watchEvent.test.ts and watchContractEvent.test.ts.

✨ Ask PR-Codex anything about this PR by commenting with /codex {your question}

@vercel
Copy link

vercel bot commented Jul 30, 2023

@jaxernst is attempting to deploy a commit to the wagmi Team on Vercel.

A member of the Team first needs to authorize it.

@changeset-bot
Copy link

changeset-bot bot commented Jul 30, 2023

🦋 Changeset detected

Latest commit: 732a56d

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

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

@jxom
Copy link
Member

jxom commented Jul 30, 2023

Instead of checking if the filter has errored n times, maybe we can check the error for “filter not found”.

Most nodes support this, examples:
https://github.com/ethereum/go-ethereum/blob/master/eth/filters/api.go#L38
https://github.com/paradigmxyz/reth/blob/f41386d28e89dd436feea872178452e5302314a5/crates/rpc/rpc/src/eth/filter.rs#L447

@jaxernst
Copy link
Contributor Author

Instead of checking if the filter has errored n times, maybe we can check the error for “filter not found”.

Most nodes support this, examples: https://github.com/ethereum/go-ethereum/blob/master/eth/filters/api.go#L38 https://github.com/paradigmxyz/reth/blob/f41386d28e89dd436feea872178452e5302314a5/crates/rpc/rpc/src/eth/filter.rs#L447

This does make more sense. I was hesitant to do this because I wasn't sure if "filter not found" was standardized, so I looked a bit further. Every client I've checked returns this error except for Erigon:

Besu: https://github.com/hyperledger/besu/blob/main/ethereum/api/src/main/java/org/hyperledger/besu/ethereum/api/jsonrpc/internal/response/RpcErrorType.java#L45

Nethermind:
https://github.com/NethermindEth/nethermind/blob/a099f0fe2158b969ca6b81861c5097ecc5f6c0b1/src/Nethermind/Nethermind.JsonRpc/Modules/Eth/EthRpcModule.cs#L553

Erigon:
https://github.com/ledgerwatch/erigon/blob/56934395210c48585ae8d7f77404d7c9b9ba3a30/turbo/jsonrpc/eth_filters.go#L78

Should we still check for this even if not all clients return it?

@jxom
Copy link
Member

jxom commented Jul 30, 2023

Thought so. Maybe we can keep the current implementation as a fallback, and check the error first?

@jaxernst
Copy link
Contributor Author

jaxernst commented Aug 1, 2023

Although Erigon doesn't throw a 'filter not found', they do all appear to return a -32000 (Invalid input) error code when the filter isn't found. It looks like Viem already catches these as "InvalidInputRpcError", so might we just reinitialize the filter if we catch an InvalidInputRpcError?

@jaxernst
Copy link
Contributor Author

jaxernst commented Aug 1, 2023

Implemented this with tests for watchContractEvent and watchEvent

@jxom jxom self-requested a review August 1, 2023 07:10
Copy link
Member

@jxom jxom left a comment

Choose a reason for hiding this comment

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

Looks good! Will merge it in soon.

@jxom jxom merged commit e98e265 into wevm:main Aug 2, 2023
@github-actions github-actions bot mentioned this pull request Aug 2, 2023
aaronmgdr pushed a commit to celo-org/viem that referenced this pull request Aug 18, 2023
* Add reinitialization logic for event filters

* Add changeset

* fix: auto-reinitialize filters

* Update changeset

* Update brown-horses-design.md

---------

Co-authored-by: jxom <jakemoxey@gmail.com>
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