Merge pull request #2318 from MetaMask/network-controller-client

NetworkController refactor for new EthClient interface

app/scripts/controllers/network.js

@@ -1,11 +1,12 @@
 const assert = require('assert')
 const EventEmitter = require('events')
-const createMetamaskProvider = require('web3-provider-engine/zero.js')
 const ObservableStore = require('obs-store')
 const ComposedStore = require('obs-store/lib/composed')
 const extend = require('xtend')
 const EthQuery = require('eth-query')
+const createEthRpcClient = require('eth-rpc-client')
 const createEventEmitterProxy = require('../lib/events-proxy.js')
+const createObjectProxy = require('../lib/obj-proxy.js')
 const RPC_ADDRESS_LIST = require('../config.js').network
 const DEFAULT_RPC = RPC_ADDRESS_LIST['rinkeby']
 
@@ -17,7 +18,8 @@ module.exports = class NetworkController extends EventEmitter {
     this.networkStore = new ObservableStore('loading')
     this.providerStore = new ObservableStore(config.provider)
     this.store = new ComposedStore({ provider: this.providerStore, network: this.networkStore })
-    this._proxy = createEventEmitterProxy()
+    this.providerProxy = createObjectProxy()
+    this.blockTrackerProxy = createEventEmitterProxy()
 
     this.on('networkDidChange', this.lookupNetwork)
   }
@@ -25,12 +27,11 @@ module.exports = class NetworkController extends EventEmitter {
   initializeProvider (_providerParams) {
     this._baseProviderParams = _providerParams
     const rpcUrl = this.getCurrentRpcAddress()
-    this._configureStandardProvider({ rpcUrl })
-    this._proxy.on('block', this._logBlock.bind(this))
-    this._proxy.on('error', this.verifyNetwork.bind(this))
-    this.ethQuery = new EthQuery(this._proxy)
+    this._configureStandardClient({ rpcUrl })
+    this.blockTrackerProxy.on('block', this._logBlock.bind(this))
+    this.blockTrackerProxy.on('error', this.verifyNetwork.bind(this))
+    this.ethQuery = new EthQuery(this.providerProxy)
     this.lookupNetwork()
-    return this._proxy
   }
 
   verifyNetwork () {
@@ -76,8 +77,10 @@ module.exports = class NetworkController extends EventEmitter {
     assert(type !== 'rpc', `NetworkController.setProviderType - cannot connect by type "rpc"`)
     // skip if type already matches
     if (type === this.getProviderConfig().type) return
+    // lookup rpcTarget for typecreateMetamaskProvider
     const rpcTarget = this.getRpcAddressForType(type)
     assert(rpcTarget, `NetworkController - unknown rpc address for type "${type}"`)
+    // update connectioncreateMetamaskProvider
     this.providerStore.updateState({ type, rpcTarget })
     this._switchNetwork({ rpcUrl: rpcTarget })
   }
@@ -97,32 +100,29 @@ module.exports = class NetworkController extends EventEmitter {
 
   _switchNetwork (providerParams) {
     this.setNetworkState('loading')
-    this._configureStandardProvider(providerParams)
+    this._configureStandardClient(providerParams)
     this.emit('networkDidChange')
   }
 
-  _configureStandardProvider(_providerParams) {
+  _configureStandardClient(_providerParams) {
     const providerParams = extend(this._baseProviderParams, _providerParams)
-    const provider = createMetamaskProvider(providerParams)
-    this._setProvider(provider)
+    const client = createEthRpcClient(providerParams)
+    this._setClient(client)
   }
 
-  _setProvider (provider) {
-    // collect old block tracker events
-    const oldProvider = this._provider
-    let blockTrackerHandlers
-    if (oldProvider) {
-      // capture old block handlers
-      blockTrackerHandlers = oldProvider._blockTracker.proxyEventHandlers
-      // tear down
-      oldProvider.removeAllListeners()
-      oldProvider.stop()
+  _setClient (newClient) {
+    // teardown old client
+    const oldClient = this._currentClient
+    if (oldClient) {
+      oldClient.blockTracker.stop()
+      // asyncEventEmitter lacks a "removeAllListeners" method
+      // oldClient.blockTracker.removeAllListeners
+      oldClient.blockTracker._events = {}
     }
-    // override block tracler
-    provider._blockTracker = createEventEmitterProxy(provider._blockTracker, blockTrackerHandlers)
     // set as new provider
-    this._provider = provider
-    this._proxy.setTarget(provider)
+    this._currentClient = newClient
+    this.providerProxy.setTarget(newClient.provider)
+    this.blockTrackerProxy.setTarget(newClient.blockTracker)
   }
 
   _logBlock (block) {

app/scripts/controllers/transactions.js

@@ -46,6 +46,7 @@ module.exports = class TransactionController extends EventEmitter {
     this.txStateManager.on('tx:status-update', this.emit.bind(this, 'tx:status-update'))
     this.nonceTracker = new NonceTracker({
       provider: this.provider,
+      blockTracker: this.blockTracker,
       getPendingTransactions: this.txStateManager.getPendingTransactions.bind(this.txStateManager),
       getConfirmedTransactions: (address) => {
         return this.txStateManager.getFilteredTxList({

app/scripts/lib/events-proxy.js

@@ -1,6 +1,5 @@
-module.exports = function createEventEmitterProxy(eventEmitter, listeners) {
+module.exports = function createEventEmitterProxy(eventEmitter, eventHandlers = {}) {
   let target = eventEmitter
-  const eventHandlers = listeners || {}
   const proxy = new Proxy({}, {
     get: (obj, name) => {
       // intercept listeners
@@ -14,9 +13,12 @@ module.exports = function createEventEmitterProxy(eventEmitter, listeners) {
       return true
     },
   })
+  proxy.setTarget(eventEmitter)
+  return proxy
+
   function setTarget (eventEmitter) {
     target = eventEmitter
-    // migrate listeners
+    // migrate eventHandlers
     Object.keys(eventHandlers).forEach((name) => {
       eventHandlers[name].forEach((handler) => target.on(name, handler))
     })
@@ -26,6 +28,4 @@ module.exports = function createEventEmitterProxy(eventEmitter, listeners) {
     eventHandlers[name].push(handler)
     target.on(name, handler)
   }
-  if (listeners) proxy.setTarget(eventEmitter)
-  return proxy
 }

app/scripts/lib/nonce-tracker.js

@@ -4,8 +4,9 @@ const Mutex = require('await-semaphore').Mutex
 
 class NonceTracker {
 
-  constructor ({ provider, getPendingTransactions, getConfirmedTransactions }) {
+  constructor ({ provider, blockTracker, getPendingTransactions, getConfirmedTransactions }) {
     this.provider = provider
+    this.blockTracker = blockTracker
     this.ethQuery = new EthQuery(provider)
     this.getPendingTransactions = getPendingTransactions
     this.getConfirmedTransactions = getConfirmedTransactions
@@ -53,7 +54,7 @@ class NonceTracker {
   }
 
   async _getCurrentBlock () {
-    const blockTracker = this._getBlockTracker()
+    const blockTracker = this.blockTracker
     const currentBlock = blockTracker.getCurrentBlock()
     if (currentBlock) return currentBlock
     return await Promise((reject, resolve) => {
@@ -139,11 +140,6 @@ class NonceTracker {
     return { name: 'local', nonce: highest, details: { startPoint, highest } }
   }
 
-  // this is a hotfix for the fact that the blockTracker will
-  // change when the network changes
-  _getBlockTracker () {
-    return this.provider._blockTracker
-  }
 }
 
 module.exports = NonceTracker

app/scripts/lib/obj-proxy.js

@@ -0,0 +1,19 @@
+module.exports = function createObjectProxy(obj) {
+  let target = obj
+  const proxy = new Proxy({}, {
+    get: (obj, name) => {
+      // intercept setTarget
+      if (name === 'setTarget') return setTarget
+      return target[name]
+    },
+    set: (obj, name, value) => {
+      target[name] = value
+      return true
+    },
+  })
+  return proxy
+
+  function setTarget (obj) {
+    target = obj
+  }
+}

app/scripts/metamask-controller.js

@@ -81,9 +81,24 @@ module.exports = class MetamaskController extends EventEmitter {
     })
     this.blacklistController.scheduleUpdates()
 
-    // rpc provider
-    this.provider = this.initializeProvider()
-    this.blockTracker = this.provider._blockTracker
+    // rpc provider and block tracker
+    this.networkController.initializeProvider({
+      scaffold: {
+        eth_syncing: false,
+        web3_clientVersion: `MetaMask/v${version}`,
+      },
+      // account mgmt
+      getAccounts: nodeify(this.getAccounts, this),
+      // tx signing
+      processTransaction: nodeify(this.newTransaction, this),
+      // old style msg signing
+      processMessage: this.newUnsignedMessage.bind(this),
+      // personal_sign msg signing
+      processPersonalMessage: this.newUnsignedPersonalMessage.bind(this),
+      processTypedMessage: this.newUnsignedTypedMessage.bind(this),
+    })
+    this.provider = this.networkController.providerProxy
+    this.blockTracker = this.networkController.blockTrackerProxy
 
     // eth data query tools
     this.ethQuery = new EthQuery(this.provider)
@@ -218,36 +233,6 @@ module.exports = class MetamaskController extends EventEmitter {
   // Constructor helpers
   //
 
-  initializeProvider () {
-    const providerOpts = {
-      static: {
-        eth_syncing: false,
-        web3_clientVersion: `MetaMask/v${version}`,
-      },
-      // account mgmt
-      getAccounts: (cb) => {
-        const isUnlocked = this.keyringController.memStore.getState().isUnlocked
-        const result = []
-        const selectedAddress = this.preferencesController.getSelectedAddress()
-
-        // only show address if account is unlocked
-        if (isUnlocked && selectedAddress) {
-          result.push(selectedAddress)
-        }
-        cb(null, result)
-      },
-      // tx signing
-      processTransaction: nodeify(async (txParams) => await this.txController.newUnapprovedTransaction(txParams), this),
-      // old style msg signing
-      processMessage: this.newUnsignedMessage.bind(this),
-      // personal_sign msg signing
-      processPersonalMessage: this.newUnsignedPersonalMessage.bind(this),
-      processTypedMessage: this.newUnsignedTypedMessage.bind(this),
-    }
-    const providerProxy = this.networkController.initializeProvider(providerOpts)
-    return providerProxy
-  }
-
   initPublicConfigStore () {
     // get init state
     const publicConfigStore = new ObservableStore()
@@ -483,6 +468,18 @@ module.exports = class MetamaskController extends EventEmitter {
   // Opinionated Keyring Management
   //
 
+  async getAccounts () {
+    const isUnlocked = this.keyringController.memStore.getState().isUnlocked
+    const result = []
+    const selectedAddress = this.preferencesController.getSelectedAddress()
+
+    // only show address if account is unlocked
+    if (isUnlocked && selectedAddress) {
+      result.push(selectedAddress)
+    }
+    return result
+  }
+
   addNewAccount (cb) {
     const primaryKeyring = this.keyringController.getKeyringsByType('HD Key Tree')[0]
     if (!primaryKeyring) return cb(new Error('MetamaskController - No HD Key Tree found'))
@@ -529,6 +526,11 @@ module.exports = class MetamaskController extends EventEmitter {
   // Identity Management
   //
 
+  // this function wrappper lets us pass the fn reference before txController is instantiated
+  async newTransaction (txParams) {
+    return await this.txController.newUnapprovedTransaction(txParams)
+  }
+
   newUnsignedMessage (msgParams, cb) {
     const msgId = this.messageManager.addUnapprovedMessage(msgParams)
     this.sendUpdate()

package.json

@@ -71,9 +71,11 @@
     "eth-contract-metadata": "^1.1.4",
     "eth-hd-keyring": "^1.2.1",
     "eth-json-rpc-filters": "^1.2.2",
+    "eth-json-rpc-middleware": "^1.4.3",
     "eth-keyring-controller": "^2.1.0",
     "eth-phishing-detect": "^1.1.4",
     "eth-query": "^2.1.2",
+    "eth-rpc-client": "^1.1.3",
     "eth-sig-util": "^1.4.0",
     "eth-simple-keyring": "^1.1.1",
     "eth-token-tracker": "^1.1.4",

test/integration/lib/first-time.js

@@ -3,6 +3,9 @@ const PASSWORD = 'password123'
 QUnit.module('first time usage')
 
 QUnit.test('render init screen', (assert) => {
+  // intercept reload attempts
+  window.onbeforeunload = () => true
+
   const done = assert.async()
   runFirstTimeUsageTest(assert).then(done).catch((err) => {
     assert.notOk(err, `Error was thrown: ${err.stack}`)

test/unit/network-contoller-test.js

@@ -14,15 +14,15 @@ describe('# Network Controller', function () {
       },
     })
 
-    networkController.initializeProvider(networkControllerProviderInit, dummyProviderConstructor)
+    networkController.initializeProvider(networkControllerProviderInit)
   })
   describe('network', function () {
     describe('#provider', function () {
       it('provider should be updatable without reassignment', function () {
-        networkController.initializeProvider(networkControllerProviderInit, dummyProviderConstructor)
-        const proxy = networkController._proxy
-        proxy.setTarget({ test: true, on: () => {} })
-        assert.ok(proxy.test)
+        networkController.initializeProvider(networkControllerProviderInit)
+        const providerProxy = networkController.providerProxy
+        providerProxy.setTarget({ test: true })
+        assert.ok(providerProxy.test)
       })
     })
     describe('#getNetworkState', function () {
@@ -66,19 +66,4 @@ describe('# Network Controller', function () {
   })
 })
 
-function dummyProviderConstructor() {
-  return {
-    // provider
-    sendAsync: noop,
-    // block tracker
-    _blockTracker: {},
-    start: noop,
-    stop: noop,
-    on: noop,
-    addListener: noop,
-    once: noop,
-    removeAllListeners: noop,
-  }
-}
-
 function noop() {}

test/unit/nonce-tracker-test.js

@@ -190,12 +190,13 @@ function generateNonceTrackerWith (pending, confirmed, providerStub = '0x0') {
   providerResultStub.result = providerStub
   const provider = {
     sendAsync: (_, cb) => { cb(undefined, providerResultStub) },
-    _blockTracker: {
-      getCurrentBlock: () => '0x11b568',
-    },
+  }
+  const blockTracker = {
+    getCurrentBlock: () => '0x11b568',
   }
   return new NonceTracker({
     provider,
+    blockTracker,
     getPendingTransactions,
     getConfirmedTransactions,
   })

ui/app/app.js

@@ -319,7 +319,7 @@ App.prototype.renderNetworkDropdown = function () {
       [
         h('i.fa.fa-question-circle.fa-lg.menu-icon'),
         'Localhost 8545',
-        activeNetwork === 'http://localhost:8545' ? h('.check', '✓') : null,
+        providerType === 'localhost' ? h('.check', '✓') : null,
       ]
     ),