EIP-1193: standard provider API by bitpshr · Pull Request #6170 · MetaMask/metamask-extension

bitpshr · 2019-02-15T14:03:23Z

This pull request adds a new inpage adapter that exposes a provider API that conforms to EIP-1193. The exposure of this API is done so in a way that coexists with the legacy API without additional global namespace pollution.

The key difference between the legacy provider API and the standard provider API is the send method; formerly, this method was a synchronous method that was rarely used in production dapps. In the new API, send becomes asynchronous and replaces sendAsync, which goes away entirely. So to summarize, a single asynchronous send method and five emitted events are all that make up the standard provider API. So, per @danfinlay's suggestion, we can gracefully let these two APIs initially coexist:

We already emit most of the events on the legacy provider.
We can introspect the arguments passed to send to determine legacy vs. standard.
The new web3 1193-compatible version will work with this approach.
The legacy provider API works unchanged.

This means the following provider APIs all work with this PR:

// Legacy sync
ethereum.send({ method: 'net_version', params: [] });
// Legacy async
ethereum.sendAsync({ method: 'net_version', params: [] }, console.log);
// Standard async
await ethereum.send('net_version');

metamaskbot · 2019-02-15T17:49:14Z

Builds ready [5fc1d66]: mascara, chrome, firefox, edge, opera

danfinlay

Looks good, safe, and useful!

I'm happy to merge it before I continue my branch kinda refactoring how the provider communicates with the background, so I can just handle the conflicts with this PR fresh in my head.

Some small comments but nothing that can't be cleaned up or refined later imo, no blockers.

danfinlay · 2019-02-15T18:29:37Z

app/scripts/controllers/network/createBlockTracker.js

+module.exports = function createBlockTracker (args, platform) {
+  const blockTracker = new BlockTracker(args)
+  blockTracker.on('latest', () => {
+    platform && platform.sendMessage && platform.sendMessage({ action: 'ethereum-ping-success' })


Oh cool, so this means dapps can now receive push events on new blocks?

danfinlay · 2019-02-15T18:33:07Z

app/scripts/createStandardProvider.js

+        })
+      } catch (error) {
+        // Per EIP-1193, send should never throw, only reject its Promise. Here
+        // we swallow thrown errors, which is safe since we handle them above.


This still makes me nervous. resolve() is safe to call twice, so as a casual reader, I would gain confidence seeing a reject() here too in case it ever isn't handled correctly.

I too feel like this should reject the promise, esp. if it's part of the contract that sendAsync doesn't throw sync errors.

danfinlay · 2019-02-15T18:33:50Z

app/scripts/inpage.js

-'November 2nd, 2018. Dapps should now call provider.enable() in order to view and use ' +
-'accounts. Please see https://bit.ly/2QQHXvF for complete information and up-to-date ' +
-'example code.')
-


This will make certain twitter users happy ;)

whymarrh

I'm excited for this! I've left a few small comments.

whymarrh · 2019-02-15T20:31:39Z

app/scripts/controllers/network/createBlockTracker.js

+    platform && platform.sendMessage && platform.sendMessage({ action: 'ethereum-ping-success' })
+  })
+  blockTracker.on('error', () => {
+    platform && platform.sendMessage && platform.sendMessage({ action: 'ethereum-ping-error' })


Can we use an if block here? We might be getting away with too many of these single-line expressions. 😄

Similarly for on('latest', ...) above

But such terse syntax! Fixed :)

whymarrh · 2019-02-15T20:33:30Z

app/scripts/createStandardProvider.js

+  }
+
+  _onClose () {
+    (this._isConnected === undefined || this._isConnected) && this._provider.emit('close', {


Can we use an if block here as well?

whymarrh · 2019-02-15T20:35:43Z

app/scripts/createStandardProvider.js

+        })
+      } catch (error) {
+        // Per EIP-1193, send should never throw, only reject its Promise. Here
+        // we swallow thrown errors, which is safe since we handle them above.


I too feel like this should reject the promise, esp. if it's part of the contract that sendAsync doesn't throw sync errors.

whymarrh · 2019-02-15T20:36:12Z

app/scripts/createStandardProvider.js

+export default function createStandardProvider (provider) {
+  const standardProvider = new StandardProvider(provider)
+  const sendLegacy = provider.send
+  provider.send = (a, b) => {


Can we be a bit more specific with the argument names here?

Since these can be completely different types of variables depending on what API you're targeting (standard or legacy), good names were tough. I improved them as best I could though, I think we can do better than a and b, you're right :)

whymarrh · 2019-02-15T20:37:08Z

app/scripts/createStandardProvider.js

+  const standardProvider = new StandardProvider(provider)
+  const sendLegacy = provider.send
+  provider.send = (a, b) => {
+    if (typeof payload === 'string' && !b || Array.isArray(b)) {


Where does payload come from?

Great save, should be a, or what is now methodOrPayload.

metamaskbot · 2019-02-18T17:35:59Z

Builds ready [9f1d7ef]: mascara, chrome, firefox, edge, opera

danfinlay

This looks good. Thank you!

Requested changes were made.

tclairet · 2019-03-27T11:11:09Z

app/scripts/createStandardProvider.js

+
+    return new Promise((resolve, reject) => {
+      try {
+        this._provider.sendAsync({ method, params, beta: true }, (error, response) => {


If may, why use beta: true and not the classic jsonrpc: '2.0' ?
I think this the cause of this issue : MM 6.2.1 sends invalid JSONRPC requests to private network

kumavis · 2019-04-16T04:22:49Z

Historical note: this reintroduced always-on block tracker

EIP-1193: Implement new provider API

4e7f2aa

bitpshr requested review from brunobar79, danfinlay, estebanmino, frankiebee and whymarrh February 15, 2019 14:03

bitpshr force-pushed the eip-1193-new branch 2 times, most recently from bb46ee1 to 9d142bd Compare February 15, 2019 14:46

EIP-1193: Updated implementation

845eb8d

bitpshr force-pushed the eip-1193-new branch from 9d142bd to 845eb8d Compare February 15, 2019 15:00

bitpshr added 4 commits February 15, 2019 10:20

Remove test file

ac2ab6c

Fix tests

252f238

Update ping check

4a393e2

Update logic

5fc1d66

danfinlay previously approved these changes Feb 15, 2019

View reviewed changes

whymarrh previously requested changes Feb 15, 2019

View reviewed changes

PR feedback

9f1d7ef

bitpshr dismissed danfinlay’s stale review via 9f1d7ef February 18, 2019 13:09

danfinlay approved these changes Feb 20, 2019

View reviewed changes

danfinlay merged commit 2f7d449 into develop Feb 20, 2019

danfinlay deleted the eip-1193-new branch February 20, 2019 00:42

tclairet reviewed Mar 27, 2019

View reviewed changes

Conversation

bitpshr commented Feb 15, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

metamaskbot commented Feb 15, 2019

Uh oh!

danfinlay left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

whymarrh left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

metamaskbot commented Feb 18, 2019

Uh oh!

danfinlay left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kumavis commented Apr 16, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

bitpshr commented Feb 15, 2019 •

edited

Loading