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: websockets rpc provider #125

Merged
merged 2 commits into from
Dec 27, 2023
Merged

feat: websockets rpc provider #125

merged 2 commits into from
Dec 27, 2023

Conversation

mfw78
Copy link
Contributor

@mfw78 mfw78 commented Dec 27, 2023

Description

This PR changes to use a websockets RPC provider

Changes

  • Use websockets

How to test

  1. Run in staging!

@mfw78 mfw78 self-assigned this Dec 27, 2023
@mfw78 mfw78 merged commit 52833c9 into main Dec 27, 2023
4 checks passed
@mfw78 mfw78 deleted the websockets branch December 27, 2023 11:50
@github-actions github-actions bot locked and limited conversation to collaborators Dec 27, 2023
Copy link
Contributor

@anxolin anxolin left a comment

Choose a reason for hiding this comment

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

I'm not very happy with these changes. I feel a change such as this should be discussed.

It removes features, and doesn't have any reviews. I fear we are passing too many uncontrolled changes, without enough eyes or agreement.

In particular, given the requirements of the watch tower, a Http RPC provider is good. It's the responsibility of the one using it, to decide on how to solve RPC issues, maybe they want to use a local node which doesn't have load balancing and don't expose Websockets.
If you want Websockets, you can recommend it in the README when you talk about config, but not force people.

Besides kills the authentication logic, and makes the code less generic as, before we were getting the provider from a method, and now we instanciate an specific provider.

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

Successfully merging this pull request may close these issues.

2 participants