Implement connectivity events correctly (MetaMask#120)

This PR correctly implements connectivity events per [EIP-1193](https://eips.ethereum.org/EIPS/eip-1193). Previously, `connect` was only emitted correctly by accident, while `disconnect` was always correct when emitted, but was only emitted for a subset of cases.
abu0306 · Dec 7, 2020 · 8f0c61b · 8f0c61b
1 parent 89ea071
commit 8f0c61b
Show file tree

Hide file tree

Showing 5 changed files with 144 additions and 75 deletions.
diff --git a/src/MetaMaskInpageProvider.js b/src/MetaMaskInpageProvider.js
@@ -4,7 +4,7 @@ const createJsonRpcStream = require('json-rpc-middleware-stream')
 const ObjectMultiplex = require('obj-multiplex')
 const SafeEventEmitter = require('safe-event-emitter')
 const dequal = require('fast-deep-equal')
-const { ethErrors } = require('eth-rpc-errors')
+const { ethErrors, EthereumRpcError } = require('eth-rpc-errors')
 const { duplex: isDuplex } = require('is-stream')
 
 const messages = require('./messages')
@@ -32,6 +32,8 @@ module.exports = class MetaMaskInpageProvider extends SafeEventEmitter {
   /**
    * @param {Object} connectionStream - A Node.js duplex stream
    * @param {Object} options - An options bag
+   * @param {string} [options.jsonRpcStreamName] - The name of the internal JSON-RPC stream.
+   * Default: metamask_provider
    * @param {ConsoleLike} [options.logger] - The logging API to use. Default: console
    * @param {number} [options.maxEventListeners] - The maximum number of event
    * listeners. Default: 100
@@ -41,6 +43,7 @@ module.exports = class MetaMaskInpageProvider extends SafeEventEmitter {
   constructor (
     connectionStream,
     {
+      jsonRpcStreamName = 'metamask-provider',
       logger = console,
       maxEventListeners = 100,
       shouldSendMetadata = true,
@@ -87,6 +90,7 @@ module.exports = class MetaMaskInpageProvider extends SafeEventEmitter {
       isConnected: false,
       isUnlocked: false,
       initialized: false,
+      isPermanentlyDisconnected: false,
     }
 
     this._metamask = this._getExperimentalApi()
@@ -98,9 +102,11 @@ module.exports = class MetaMaskInpageProvider extends SafeEventEmitter {
 
     // bind functions (to prevent consumers from making unbound calls)
     this._handleAccountsChanged = this._handleAccountsChanged.bind(this)
+    this._handleConnect = this._handleConnect.bind(this)
     this._handleChainChanged = this._handleChainChanged.bind(this)
-    this._handleUnlockStateChanged = this._handleUnlockStateChanged.bind(this)
     this._handleDisconnect = this._handleDisconnect.bind(this)
+    this._handleStreamDisconnect = this._handleStreamDisconnect.bind(this)
+    this._handleUnlockStateChanged = this._handleUnlockStateChanged.bind(this)
     this._sendSync = this._sendSync.bind(this)
     this._rpcRequest = this._rpcRequest.bind(this)
     this._warnOfDeprecation = this._warnOfDeprecation.bind(this)
@@ -115,7 +121,7 @@ module.exports = class MetaMaskInpageProvider extends SafeEventEmitter {
       connectionStream,
       mux,
       connectionStream,
-      this._handleDisconnect.bind(this, 'MetaMask'),
+      this._handleStreamDisconnect.bind(this, 'MetaMask'),
     )
 
     // ignore phishing warning message (handled elsewhere)
@@ -133,9 +139,9 @@ module.exports = class MetaMaskInpageProvider extends SafeEventEmitter {
     const jsonRpcConnection = createJsonRpcStream()
     pump(
       jsonRpcConnection.stream,
-      mux.createStream('provider'),
+      mux.createStream(jsonRpcStreamName),
       jsonRpcConnection.stream,
-      this._handleDisconnect.bind(this, 'MetaMask RpcProvider'),
+      this._handleStreamDisconnect.bind(this, 'MetaMask RpcProvider'),
     )
 
     // handle RPC requests via dapp-side rpc engine
@@ -159,14 +165,17 @@ module.exports = class MetaMaskInpageProvider extends SafeEventEmitter {
       } else if (method === 'metamask_chainChanged') {
         this._handleChainChanged(params)
       } else if (EMITTED_NOTIFICATIONS.includes(method)) {
-        this.emit('notification', payload) // deprecated
         this.emit('message', {
           type: method,
           data: params,
         })
 
         // deprecated
-        this.emit('notification', params.result)
+        this.emit('notification', payload.params.result)
+      } else if (method === 'METAMASK_STREAM_FAILURE') {
+        connectionStream.destroy(
+          new Error(messages.errors.permanentlyDisconnected()),
+        )
       }
     })
 
@@ -180,9 +189,6 @@ module.exports = class MetaMaskInpageProvider extends SafeEventEmitter {
       }
       window.addEventListener('DOMContentLoaded', domContentLoadedHandler)
     }
-
-    // indicate that we've connected, for EIP-1193 compliance
-    setTimeout(() => this.emit('connect', { chainId: this.chainId }))
   }
 
   //====================
@@ -371,25 +377,115 @@ module.exports = class MetaMaskInpageProvider extends SafeEventEmitter {
     this._rpcEngine.handle(payload, cb)
   }
 
+  /**
+   * When the provider becomes connected, updates internal state and emits
+   * required events. Idempotent.
+   *
+   * @param {string} chainId - The ID of the newly connected chain.
+   * @emits MetaMaskInpageProvider#connect
+   */
+  _handleConnect (chainId) {
+    if (!this._state.isConnected) {
+      this._state.isConnected = true
+      this.emit('connect', { chainId })
+      this._log.debug(messages.info.connected(chainId))
+    }
+  }
+
+  /**
+   * When the provider becomes disconnected, updates internal state and emits
+   * required events. Idempotent with respect to the isRecoverable parameter.
+   *
+   * Error codes per the CloseEvent status codes as required by EIP-1193:
+   * https://developer.mozilla.org/en-US/docs/Web/API/CloseEvent#Status_codes
+   *
+   * @param {boolean} isRecoverable - Whether the disconnection is recoverable.
+   * @param {string} [errorMessage] - A custom error message.
+   * @emits MetaMaskInpageProvider#disconnect
+   */
+  _handleDisconnect (isRecoverable, errorMessage) {
+    if (
+      this._state.isConnected ||
+      (!this._state.isPermanentlyDisconnected && !isRecoverable)
+    ) {
+      this._state.isConnected = false
+
+      let error
+      if (isRecoverable) {
+        error = new EthereumRpcError(
+          1013, // Try again later
+          errorMessage || messages.errors.disconnected(),
+        )
+        this._log.debug(error)
+      } else {
+        error = new EthereumRpcError(
+          1011, // Internal error
+          errorMessage || messages.errors.permanentlyDisconnected(),
+        )
+        this._log.error(error)
+        this.chainId = null
+        this.networkVersion = null
+        this._state.accounts = null
+        this.selectedAddress = null
+        this._state.isUnlocked = null
+        this._state.isPermanentlyDisconnected = true
+      }
+
+      this.emit('disconnect', error)
+      this.emit('close', error) // deprecated
+    }
+  }
+
   /**
    * Called when connection is lost to critical streams.
    *
    * @private
    * @emits MetamaskInpageProvider#disconnect
    */
-  _handleDisconnect (streamName, err) {
-    logStreamDisconnectWarning.bind(this)(this._log, streamName, err)
+  _handleStreamDisconnect (streamName, error) {
+    logStreamDisconnectWarning(this._log, streamName, error, this)
+    this._handleDisconnect(false, error ? error.message : undefined)
+  }
 
-    const disconnectError = {
-      code: 1011,
-      reason: messages.errors.disconnected(),
+  /**
+   * Upon receipt of a new chainId and networkVersion, emits corresponding
+   * events and sets relevant public state.
+   * Does nothing if neither the chainId nor the networkVersion are different
+   * from existing values.
+   *
+   * @private
+   * @emits MetamaskInpageProvider#chainChanged
+   * @param {Object} networkInfo - An object with network info.
+   * @param {string} networkInfo.chainId - The latest chain ID.
+   * @param {string} networkInfo.networkVersion - The latest network ID.
+   */
+  _handleChainChanged ({ chainId, networkVersion } = {}) {
+    if (
+      !chainId || typeof chainId !== 'string' || !chainId.startsWith('0x') ||
+      !networkVersion || typeof networkVersion !== 'string'
+    ) {
+      this._log.error(
+        'MetaMask: Received invalid network parameters. Please report this bug.',
+        { chainId, networkVersion },
+      )
+      return
     }
 
-    if (this._state.isConnected) {
-      this.emit('disconnect', disconnectError)
-      this.emit('close', disconnectError) // deprecated
+    if (networkVersion === 'loading') {
+      this._handleDisconnect(true)
+    } else {
+      this._handleConnect(chainId)
+
+      if (chainId !== this.chainId) {
+        this.chainId = chainId
+        this.emit('chainChanged', this.chainId)
+      }
+
+      if (networkVersion !== this.networkVersion) {
+        this.networkVersion = networkVersion
+        this.emit('networkChanged', this.networkVersion)
+      }
     }
-    this._state.isConnected = false
   }
 
   /**
@@ -437,41 +533,6 @@ module.exports = class MetaMaskInpageProvider extends SafeEventEmitter {
     }
   }
 
-  /**
-   * Upon receipt of a new chainId and networkVersion, emits corresponding
-   * events and sets relevant public state.
-   * Does nothing if neither the chainId nor the networkVersion are different
-   * from existing values.
-   *
-   * @private
-   * @emits MetamaskInpageProvider#chainChanged
-   * @param {Object} networkInfo - An object with network info.
-   * @param {string} networkInfo.chainId - The latest chain ID.
-   * @param {string} networkInfo.networkVersion - The latest network ID.
-   */
-  _handleChainChanged ({ chainId, networkVersion } = {}) {
-    if (
-      !chainId || typeof chainId !== 'string' || !chainId.startsWith('0x') ||
-      !networkVersion || typeof networkVersion !== 'string'
-    ) {
-      this._log.error(
-        'MetaMask: Received invalid network parameters. Please report this bug.',
-        { chainId, networkVersion },
-      )
-      return
-    }
-
-    if (chainId !== this.chainId) {
-      this.chainId = chainId
-      this.emit('chainChanged', this.chainId)
-    }
-
-    if (networkVersion !== this.networkVersion) {
-      this.networkVersion = networkVersion
-      this.emit('networkChanged', this.networkVersion)
-    }
-  }
-
   /**
    * Upon receipt of a new isUnlocked state, sets relevant public state.
    * Calls the accounts changed handler with the received accounts, or an empty

diff --git a/src/initializeProvider.js b/src/initializeProvider.js
@@ -6,14 +6,16 @@ const shimWeb3 = require('./shimWeb3')
  *
  * @param {Object} options - An options bag.
  * @param {Object} options.connectionStream - A Node.js stream.
- * @param {number} options.maxEventListeners - The maximum number of event listeners.
- * @param {boolean} options.shouldSendMetadata - Whether the provider should send page metadata.
- * @param {boolean} options.shouldSetOnWindow - Whether the provider should be set as window.ethereum.
- * @param {boolean} options.shouldShimWeb3 - Whether a window.web3 shim should be injected.
+ * @param {string} [options.jsonRpcStreamName] - The name of the internal JSON-RPC stream.
+ * @param {number} [options.maxEventListeners] - The maximum number of event listeners.
+ * @param {boolean} [options.shouldSendMetadata] - Whether the provider should send page metadata.
+ * @param {boolean} [options.shouldSetOnWindow] - Whether the provider should be set as window.ethereum.
+ * @param {boolean} [options.shouldShimWeb3] - Whether a window.web3 shim should be injected.
  * @returns {MetaMaskInpageProvider | Proxy} The initialized provider (whether set or not).
  */
 function initializeProvider ({
   connectionStream,
+  jsonRpcStreamName,
   logger = console,
   maxEventListeners = 100,
   shouldSendMetadata = true,
@@ -22,7 +24,13 @@ function initializeProvider ({
 } = {}) {
 
   let provider = new MetaMaskInpageProvider(
-    connectionStream, { logger, maxEventListeners, shouldSendMetadata },
+    connectionStream,
+    {
+      logger,
+      jsonRpcStreamName,
+      maxEventListeners,
+      shouldSendMetadata,
+    },
   )
 
   provider = new Proxy(provider, {

diff --git a/src/messages.js b/src/messages.js
@@ -1,6 +1,7 @@
 module.exports = {
   errors: {
-    disconnected: () => `MetaMask: Lost connection to MetaMask background process.`,
+    disconnected: () => 'MetaMask: Disconnected from chain. Attempting to connect.',
+    permanentlyDisconnected: () => 'MetaMask: Disconnected from MetaMask background. Page reload required.',
     sendSiteMetadata: () => `MetaMask: Failed to send site metadata. This is an internal error, please report this bug.`,
     unsupportedSync: (method) => `MetaMask: The MetaMask Ethereum provider does not support synchronous methods like ${method} without a callback parameter.`,
     invalidDuplexStream: () => 'Must provide a Node.js-style duplex stream.',
@@ -11,6 +12,9 @@ module.exports = {
     invalidLoggerObject: () => `'args.logger' must be an object if provided.`,
     invalidLoggerMethod: (method) => `'args.logger' must include required method '${method}'.`,
   },
+  info: {
+    connected: (chainId) => `MetaMask: Connected to chain with ID "${chainId}".`,
+  },
   warnings: {
     // deprecated methods
     enableDeprecation: `MetaMask: 'ethereum.enable()' is deprecated and may be removed in the future. Please use the 'eth_requestAccounts' RPC method instead.\nFor more information, see: https://eips.ethereum.org/EIPS/eip-1102`,

diff --git a/src/utils.js b/src/utils.js
@@ -1,6 +1,4 @@
-const EventEmitter = require('events')
 const { ethErrors } = require('eth-rpc-errors')
-const SafeEventEmitter = require('safe-event-emitter')
 
 // utility functions
 
@@ -44,28 +42,26 @@ const getRpcPromiseCallback = (resolve, reject, unwrapResult = true) => (error,
 }
 
 /**
- * Logs a stream disconnection error. Emits an 'error' if bound to an
+ * Logs a stream disconnection error. Emits an 'error' if given an
  * EventEmitter that has listeners for the 'error' event.
  *
- * @param {Object} log - The logging API to use.
+ * @param {typeof console} log - The logging API to use.
  * @param {string} remoteLabel - The label of the disconnected stream.
- * @param {Error} err - The associated error to log.
+ * @param {Error} [err] - The associated error to log.
+ * @param {import('safe-event-emitter')} [emitter] - The logging API to use.
  */
-function logStreamDisconnectWarning (log, remoteLabel, err) {
-  let warningMsg = `MetaMaskInpageProvider - lost connection to ${remoteLabel}`
-  if (err) {
+function logStreamDisconnectWarning (log, remoteLabel, err, emitter) {
+  let warningMsg = `MetaMask: Lost connection to "${remoteLabel}".`
+  if (err && err.stack) {
     warningMsg += `\n${err.stack}`
   }
   log.warn(warningMsg)
-  if (this instanceof EventEmitter || this instanceof SafeEventEmitter) {
-    if (this.listenerCount('error') > 0) {
-      this.emit('error', warningMsg)
-    }
+  if (emitter && emitter.listenerCount('error') > 0) {
+    emitter.emit('error', warningMsg)
   }
 }
 
-// eslint-disable-next-line no-empty-function
-const NOOP = () => {}
+const NOOP = () => undefined
 
 // constants
 

diff --git a/test/MetaMaskInpageProvider.misc.test.js b/test/MetaMaskInpageProvider.misc.test.js
@@ -61,7 +61,7 @@ describe('MetaMaskInpageProvider: Miscellanea', () => {
 
       expect(
         () => new MetaMaskInpageProvider(stream, null),
-      ).toThrow('Cannot destructure property `logger` of \'undefined\' or \'null\'')
+      ).toThrow('Cannot destructure property `jsonRpcStreamName` of \'undefined\' or \'null\'')
 
       expect(
         () => new MetaMaskInpageProvider(stream, {