Skip to content

Update EIP 1193#2577

Merged
eip-automerger merged 23 commits intoethereum:masterfrom
rekmarks:rekmarks-1193-update
Apr 3, 2020
Merged

Update EIP 1193#2577
eip-automerger merged 23 commits intoethereum:masterfrom
rekmarks:rekmarks-1193-update

Conversation

@rekmarks
Copy link
Contributor

@rekmarks rekmarks commented Mar 30, 2020 •
edited
Loading

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

adrianmcli
adrianmcli reviewed Mar 30, 2020
View reviewed changes
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!

@rekmarks
Copy link
Contributor Author

rekmarks commented Mar 30, 2020

cc: @ricmoo @MicahZoltu @danfinlay

I am in touch with @ryanio

@davidmurdoch
Copy link

davidmurdoch commented Mar 30, 2020

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

@danfinlay
Copy link
Contributor

danfinlay commented Mar 30, 2020

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

davidmurdoch commented Mar 31, 2020

@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

adrianmcli commented Mar 31, 2020

@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

MicahZoltu commented Mar 31, 2020

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 •
edited
Loading

@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

frozeman commented Mar 31, 2020

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

eip-automerger commented Mar 31, 2020

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

@ryanio ryanio mentioned this pull request Mar 31, 2020
Closed
whymarrh
whymarrh reviewed Apr 1, 2020
View reviewed changes
@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.

whymarrh
whymarrh reviewed Apr 1, 2020
View reviewed changes
ryanio
ryanio approved these changes Apr 3, 2020
View reviewed changes
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
This was referenced Apr 7, 2020
Merged
Closed
@pedrouid pedrouid mentioned this pull request Apr 27, 2020
Closed
pizzarob pushed a commit to pizzarob/EIPs that referenced this pull request Jun 12, 2020
@rekmarks @pizzarob
Automatically merged updates to draft EIP(s) 1193 (ethereum#2577)
24fed01 
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
@rekmarks @tkstanczak
Automatically merged updates to draft EIP(s) 1193 (ethereum#2577)
3623258 
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
@rekmarks @Arachnid
Automatically merged updates to draft EIP(s) 1193 (ethereum#2577)
62874f7 
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

Reviewers

3 more reviewers

@adrianmcli adrianmcli adrianmcli left review comments

@whymarrh whymarrh whymarrh left review comments

@ryanio ryanio ryanio approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

10 participants

@rekmarks @axic @davidmurdoch @danfinlay @adrianmcli @MicahZoltu @frozeman @eip-automerger @ryanio @whymarrh