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

Implement new 1193 spec, fixup #30

Merged
merged 9 commits into from
Apr 22, 2020
Merged

Implement new 1193 spec, fixup #30

merged 9 commits into from
Apr 22, 2020

Conversation

rekmarks
Copy link
Member

@rekmarks rekmarks commented Apr 13, 2020 •
edited by Gudahtt
Loading

Implements the new EIP 1193 specification.

  • Adds
    • Methods
      • request(args), the only future-proof way of making RPC requests
    • Events
      • message, a replacement for notification
      • disconnect, a replacement for close
  • Deprecates (in addition to existing deprecations, with warning messages)
    • Events
      • chainIdChanged
      • close
      • networkChanged
      • notification
  • Changes
    • Experimental methods:
      • sendBatch -> requestBatch

@rekmarks rekmarks marked this pull request as ready for review April 17, 2020 22:34
@rekmarks rekmarks requested a review from Gudahtt April 17, 2020 22:34
@rekmarks rekmarks mentioned this pull request Apr 18, 2020
Merged
@rekmarks
implement new EIP 1193 API
9b345e2 
send -> request
sendBatch -> batchRequest
add warnings for deprecated events
convert getExperimentalApi to class method
add log files to gitignore
bind all private methods
Gudahtt
Gudahtt previously requested changes Apr 21, 2020
View reviewed changes
Copy link
Member

@Gudahtt Gudahtt left a comment

Choose a reason for hiding this comment

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

Could we hold off on deprecating sendAsync until after request is stabilized? We should probably mark request as "experimental" until the EIP is finalized as well.

@rekmarks rekmarks requested a review from Gudahtt April 21, 2020 14:58
@rekmarks
Copy link
Member Author

@Gudahtt I undeprecated sendAsync, but I'd rather not mark request experimental, as it would only discourage its use, and I believe it is very unlikely to change. How we communicate about this new API in docs and announcements is a different story.

@Gudahtt
Copy link
Member

Gudahtt commented Apr 21, 2020

As long as we should communicate in some way that we might break request without warning. Otherwise we'd be in a very uncomfortable position if the spec did change, however unlikely.

Gudahtt
Gudahtt reviewed Apr 21, 2020
View reviewed changes
src/MetamaskInpageProvider.js Outdated Show resolved Hide resolved
src/MetamaskInpageProvider.js Outdated Show resolved Hide resolved
src/MetamaskInpageProvider.js Show resolved Hide resolved
@Gudahtt Gudahtt dismissed their stale review April 21, 2020 21:34

This has been addressed

Gudahtt
Gudahtt reviewed Apr 21, 2020
View reviewed changes
src/MetamaskInpageProvider.js Show resolved Hide resolved
src/MetamaskInpageProvider.js Show resolved Hide resolved
src/messages.js Outdated Show resolved Hide resolved
src/messages.js Outdated Show resolved Hide resolved
@rekmarks rekmarks requested a review from Gudahtt April 21, 2020 22:49
Gudahtt
Gudahtt approved these changes Apr 22, 2020
View reviewed changes
Copy link
Member

@Gudahtt Gudahtt left a comment

Choose a reason for hiding this comment

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

LGTM!

@rekmarks rekmarks merged commit df3ba89 into master Apr 22, 2020
@rekmarks rekmarks deleted the 1193-new branch April 22, 2020 02:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Gudahtt Gudahtt Gudahtt approved these changes

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

Successfully merging this pull request may close these issues.
2 participants
@rekmarks @Gudahtt