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

Update EIP 1193 #2577

Merged
merged 23 commits into from
Apr 3, 2020
Merged

Conversation

rekmarks
Copy link
Contributor

@rekmarks rekmarks commented Mar 30, 2020

Link to file: https://github.com/rekmarks/EIPs/blob/rekmarks-1193-update/EIPS/eip-1193.md

Makes the following updates to EIP 1193, in light of recent discussions in #2319:

  • Deprecates send and sendAsync
    • Providers may continue to support these methods
  • Specifies a new method, request, with the same Promise signature as the send currently at ethereum/EIPs#master
  • Changes the interface of the notification event, such that a generalized object is emitted
    • This was done to support other notifications than eth_subscription
  • Makes the spec transport- and RPC protocol-agnostic, per the contributions of @ryanio
  • Adds the message event for generalized messaging, and deprecates the notification event
    • This is so that existing uses of the notification event do not break
    • All functionality previously handled by the notification event is now handled by the message event
  • Various edits for clarity and stylistic consistency

@axic
Copy link
Member

axic commented Mar 30, 2020

cc @alcuadrado @ryanio

Copy link

@adrianmcli adrianmcli left a comment

Choose a reason for hiding this comment

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

At first read, only one thing stood out to me! Great work @rekmarks!

EIPS/eip-1193.md Outdated Show resolved Hide resolved
@rekmarks
Copy link
Contributor Author

cc: @ricmoo @MicahZoltu @danfinlay

I am in touch with @ryanio

@davidmurdoch
Copy link

Since this renames send to request do we not need to consider versioning this API now?

@danfinlay
Copy link
Contributor

Since this renames send to request do we not need to consider versioning this API now?

Since this is not in production anywhere yet, and is in DRAFT status, I don't think we need to version.

I sometimes fall on the other end of the spectrum: I somewhat wish proposals were hash-locked, because some specs evolve after they've gone to production (see EIP 712: eth_signTypedData).

I also don't think a version number always does justice to the evolution of a spec, since two versions could conflict and not build on each other.

But since this is both DRAFT and not in production anywhere, I think it's okay to eschew versioning for this iteration. Let me know if there's a reason to disagree.

@davidmurdoch
Copy link

@danfinlay, previously, a versioning mechanism was being discussed because the signature of send was changing. MetaMask already injects send onto window.ethereum, for example, so we were trying to figure out how dapp developers could differentiate. Now, with this function being renamed, it seems we don't have to. ♥️

@adrianmcli
Copy link

@rekmarks how come the chainId was changed from a number to a hex string? Do we envision trillions of chains? 😂Just curious.

@MicahZoltu
Copy link
Contributor

I think versioning or capability detection (or versioned capabilities!) is still an incredibly wise thing to do, but I agree that can and probably should be a separate EIP from this one. Ideally that separate EIP would be done first, and this one would follow it (adding a new capability/version) but I understand that beggars can't be choosers.

@rekmarks
Copy link
Contributor Author

rekmarks commented Mar 31, 2020

@adrianmcli I suppose we are! See: https://github.com/ethereum/EIPs/blob/master/EIPS/eip-695.md

Relevant excerpt:

Returns

QUANTITY - integer of the current chain ID.
...

References

Return value QUANTITY adheres to standard JSON RPC hex value encoding, as documented here: https://github.com/ethereum/wiki/wiki/JSON-RPC#hex-value-encoding.

Geth, Parity, and MetaMask are already returning hex strings for chainId. I see no benefit to making it a number; only costs. (Planting my flag here.)

@frozeman
Copy link
Contributor

I personally find the examples in JS better :) and a bit to much text.

But the ideas are great and giving a notification type makes a lot of sense!

Great job @rekmarks

@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):

EIPS/eip-1193.md Outdated Show resolved Hide resolved
EIPS/eip-1193.md Outdated Show resolved Hide resolved
EIPS/eip-1193.md Outdated Show resolved Hide resolved
EIPS/eip-1193.md Outdated Show resolved Hide resolved
EIPS/eip-1193.md Show resolved Hide resolved
EIPS/eip-1193.md Outdated Show resolved Hide resolved
@rekmarks
Copy link
Contributor Author

rekmarks commented Apr 1, 2020

My thanks to all of you for your feedback!

As far as I'm concerned, this is ready to be merged.

EIPS/eip-1193.md Outdated Show resolved Hide resolved
EIPS/eip-1193.md Outdated Show resolved Hide resolved
EIPS/eip-1193.md Outdated Show resolved Hide resolved
rekmarks and others added 6 commits April 1, 2020 16:32
Co-Authored-By: Whymarrh Whitby <whymarrh.whitby@gmail.com>
Co-Authored-By: Whymarrh Whitby <whymarrh.whitby@gmail.com>
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.

LGTM, thanks for all your work on this!

@eip-automerger eip-automerger merged commit e9a35bf into ethereum:master Apr 3, 2020
@rekmarks rekmarks deleted the rekmarks-1193-update branch April 3, 2020 20:18
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.

10 participants