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.

Sign up for GitHub

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

Jump to bottom

Replace legacy websocket provider with V2 #3225

Merged
merged 7 commits into from
Feb 27, 2024
Merged

Replace legacy websocket provider with V2 #3225

merged 7 commits into from
Feb 27, 2024

Conversation

fselmo
Copy link
Collaborator

@fselmo fselmo commented Feb 6, 2024 •
edited

What was wrong?

  • Replace the legacy WebSocket provider with V2.

We wanted to get these changes in before the first release of v7 beta.

How was it fixed?

  • WebsocketProvider -> LegacyWebSocketProvider
  • WebsocketProviderV2 -> WebSocketProvider
  • Update documentation to reflect the above changes

Todo:

Cute Animal Picture

20240206_142634

fselmo added a commit to fselmo/web3.py that referenced this pull request Feb 6, 2024
@fselmo
newsfragment for ethereum#3225
f974a71
@fselmo fselmo force-pushed the rename-websocket-provider-v2 branch from bdf91d2 to f974a71 Compare February 6, 2024 21:32
@fselmo fselmo marked this pull request as ready for review February 6, 2024 21:43
@fselmo fselmo mentioned this pull request Feb 6, 2024
Merged
1 task
kclowes
kclowes approved these changes Feb 7, 2024
View reviewed changes
Copy link
Collaborator

@kclowes kclowes left a comment

Choose a reason for hiding this comment

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

Just putting this here so we don't forget... we need to add a deprecation warning (or some type of warning) in v6 on the WebsocketProvider to let people know about the change from WebsocketProvider -> LegacyWebsocketProvider, and provide the legacy alias. I'm also happy to add an issue, just let me know!

Looks good! I think there are a couple spots in the Quickstart docs where the new WebsocketProvider API doesn't work quite like the docs say it does, but that may just be user error :)

docs/quickstart.rst Outdated Show resolved Hide resolved
@pacrob
Copy link
Contributor

pacrob commented Feb 7, 2024

The organization of providers feels unintuitive:

image

Maybe org the HTTP Providers together? Do we have future PersistentConnectionProviders planned that would merit it staying as an umbrella term? If not, I feel like it's good to have the names of the actual providers users will be implementing show up first in docs. Can then include deets about PersistentConnectionProvider in more detailed description.

Just thinking out loud. What'chu think?

@kclowes
Copy link
Collaborator

kclowes commented Feb 7, 2024

The organization of providers feels unintuitive: ...

Yeah, we don't neatly split between Sync and Async providers any more. We really have Sync Providers, Async Providers, and Persistent Connection Providers, IMHO. Might be good to reflect that in the docs

reedsa
reedsa approved these changes Feb 8, 2024
View reviewed changes
ethpm/backends/registry.py Outdated Show resolved Hide resolved
@fselmo fselmo force-pushed the rename-websocket-provider-v2 branch 3 times, most recently from 28bae12 to 4894a7c Compare February 26, 2024 22:29
@fselmo
Establish WebsocketProviderV2 as the principal WebSocket provider:
3f9856a 
- ``WebsocketProvider`` -> ``LegacyWebsocketProvider``
- ``WebsocketProviderV2`` -> ``WebsocketProvider``
- Update documentation and code base to reflect these changes

Additionally:

- Remove the need for ``persistent_connection()`` on the ``AsyncWeb3`` class
  by implementing the magic methods on ``AsyncWeb3`` itself.
- Use a method wrapper for methods only allowed when ``AsyncWeb3`` is
  instantiated with a ``PersistentConnectionProvider``.

... fix lint
@fselmo
Factor out commonalities back to PersistentConnectionProvider
a065268
@fselmo fselmo force-pushed the rename-websocket-provider-v2 branch from 4894a7c to d480151 Compare February 26, 2024 22:36
@fselmo fselmo force-pushed the rename-websocket-provider-v2 branch from d480151 to e16f83d Compare February 26, 2024 22:49
reedsa
reedsa approved these changes Feb 26, 2024
View reviewed changes
Copy link
Contributor

@reedsa reedsa left a comment

Choose a reason for hiding this comment

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

lgtm!

pacrob
pacrob approved these changes Feb 27, 2024
View reviewed changes
Copy link
Contributor

@pacrob pacrob left a comment

Choose a reason for hiding this comment

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

More of a nit, but it's still bugging me that everything we get from the websockets lib uses capital S WebSocket and we use mostly Websocket. But as long as ya'll are good with it, lgtm :)

@fselmo fselmo force-pushed the rename-websocket-provider-v2 branch from 737d7a0 to b7fae4e Compare February 27, 2024 17:43
@fselmo
Copy link
Collaborator Author

fselmo commented Feb 27, 2024

More of a nit, but it's still bugging me that everything we get from the websockets lib uses capital S WebSocket and we use mostly Websocket. But as long as ya'll are good with it, lgtm :)

Being that the protocol is WebSocket, let's make it happen since we're already breaking everything. Updated in the last commit.

fselmo added a commit to fselmo/web3.py that referenced this pull request Feb 27, 2024
@fselmo
newsfragment for ethereum#3225
c2377c7
@fselmo fselmo force-pushed the rename-websocket-provider-v2 branch from c2377c7 to e2c44f9 Compare February 27, 2024 18:35
@fselmo fselmo merged commit 0d3b745 into ethereum:main Feb 27, 2024
85 checks passed
@fselmo fselmo deleted the rename-websocket-provider-v2 branch February 27, 2024 19:02
@fselmo fselmo mentioned this pull request Feb 27, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@reedsa reedsa reedsa approved these changes

@pacrob pacrob pacrob approved these changes

@kclowes kclowes Awaiting requested review from kclowes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@fselmo @pacrob @kclowes @reedsa