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

1193: disconnect event, connectivity errors #2586

Merged
merged 6 commits into from
Apr 7, 2020

Conversation

rekmarks
Copy link
Contributor

@rekmarks rekmarks commented Apr 4, 2020

File: https://github.com/rekmarks/EIPs/blob/1193-open/EIPS/eip-1193.md

  • Adds the disconnect event, intended to:
    • supersede the close event
    • make it clear that it's the counterpart of the connect event
  • Deprecates the close event

The above follows discussions with @ryanio

In addition:

  • Adds the following Provider errors:
    • 4900: Not Connected
    • 4901: Chain Not Connected
  • Adds the section Connectivity for defining "connected" and "disconnected"

This allows consumers to tell whether their requests are failing because the Provider is disconnected.

@rekmarks rekmarks changed the title 1193: open event 1193: open event Apr 4, 2020
@rekmarks rekmarks marked this pull request as ready for review April 4, 2020 20:27
@adrianmcli
Copy link

adrianmcli commented Apr 5, 2020

As much as I like the fact that open and close are symmetric. I think connect and disconnect makes a lot more intuitive sense and reflects what's actually happening. When I hear provider.open, my next question is "open what?".

@rekmarks
Copy link
Contributor Author

rekmarks commented Apr 5, 2020

The only reason I picked open was because the close event uses the Web API CloseEvent status codes, but that's not really striking me as that relevant, on consideration. I'll change it.

@rekmarks rekmarks changed the title 1193: open event 1193: disconnect event Apr 5, 2020
@eip-automerger
Copy link

Hi! I'm a bot, and I wanted to automerge your PR, but couldn't because of the following issue(s):

@rekmarks rekmarks changed the title 1193: disconnect event 1193: disconnect event, isConnected method Apr 6, 2020
@rekmarks
Copy link
Contributor Author

rekmarks commented Apr 6, 2020

@ryanio, note that I added the isConnected method; please see description for justification.

Happy to roll it back if undesired, but, I obviously think it's justified 😛

@ryanio
Copy link
Contributor

ryanio commented Apr 6, 2020

I’d prefer not to introduce another method. Wouldn’t you be able to just send a json rpc request like eth_chainId to verify connection?

@rekmarks
Copy link
Contributor Author

rekmarks commented Apr 6, 2020

I’m very sympathetic to that goal. eth_chainId is unfortunately of no help, because EIP 695 has nothing to say about the case where no chain ID is available.

On reflection, even though a method enabling consumers to tell if “the wire is hot” would be nice, we could just standardize this by means of an RPC error.

Since this will be specific to the Provider, that error should IMO be introduced here. What do you think?

@rekmarks rekmarks changed the title 1193: disconnect event, isConnected method 1193: disconnect event, connectivity errors Apr 6, 2020
@rekmarks
Copy link
Contributor Author

rekmarks commented Apr 6, 2020

e0e52a3 Removed isConnected, added connectivity errors.

Copy link
Contributor

@ryanio ryanio left a comment

Choose a reason for hiding this comment

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

Really nice contributions, thanks very much 🎉

@eip-automerger eip-automerger merged commit 3eead84 into ethereum:master Apr 7, 2020
@adrianmcli adrianmcli mentioned this pull request Apr 7, 2020
pizzarob pushed a commit to pizzarob/EIPs that referenced this pull request Jun 12, 2020
Hi, I'm a bot! This change was automatically merged because:

 - It only modifies existing Draft or Last Call EIP(s)
 - The PR was approved or written by at least one author of each modified EIP
 - The build is passing
tkstanczak pushed a commit to tkstanczak/EIPs that referenced this pull request Nov 7, 2020
Hi, I'm a bot! This change was automatically merged because:

 - It only modifies existing Draft or Last Call EIP(s)
 - The PR was approved or written by at least one author of each modified EIP
 - The build is passing
Arachnid pushed a commit to Arachnid/EIPs that referenced this pull request Mar 6, 2021
Hi, I'm a bot! This change was automatically merged because:

 - It only modifies existing Draft or Last Call EIP(s)
 - The PR was approved or written by at least one author of each modified EIP
 - The build is passing
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.

4 participants