From 8da0d0b28a52d476da3623774159e8d6a595da2d Mon Sep 17 00:00:00 2001 From: kumavis Date: Wed, 18 Oct 2017 15:09:32 -0700 Subject: [PATCH] Revert "NetworkController refactor for new EthClient interface" --- app/scripts/controllers/network.js | 54 ++++++++++---------- app/scripts/controllers/transactions.js | 1 - app/scripts/lib/events-proxy.js | 10 ++-- app/scripts/lib/nonce-tracker.js | 10 ++-- app/scripts/lib/obj-proxy.js | 19 ------- app/scripts/metamask-controller.js | 68 ++++++++++++------------- package.json | 2 - test/integration/lib/first-time.js | 3 -- test/unit/network-contoller-test.js | 25 +++++++-- test/unit/nonce-tracker-test.js | 7 ++- ui/app/app.js | 2 +- 11 files changed, 96 insertions(+), 105 deletions(-) delete mode 100644 app/scripts/lib/obj-proxy.js diff --git a/app/scripts/controllers/network.js b/app/scripts/controllers/network.js index 64ed4b7c2..0f9db4d53 100644 --- a/app/scripts/controllers/network.js +++ b/app/scripts/controllers/network.js @@ -1,12 +1,11 @@ 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'] @@ -18,8 +17,7 @@ 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.providerProxy = createObjectProxy() - this.blockTrackerProxy = createEventEmitterProxy() + this._proxy = createEventEmitterProxy() this.on('networkDidChange', this.lookupNetwork) } @@ -27,11 +25,12 @@ module.exports = class NetworkController extends EventEmitter { initializeProvider (_providerParams) { this._baseProviderParams = _providerParams const rpcUrl = this.getCurrentRpcAddress() - 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._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.lookupNetwork() + return this._proxy } verifyNetwork () { @@ -77,10 +76,8 @@ 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 }) } @@ -100,29 +97,32 @@ module.exports = class NetworkController extends EventEmitter { _switchNetwork (providerParams) { this.setNetworkState('loading') - this._configureStandardClient(providerParams) + this._configureStandardProvider(providerParams) this.emit('networkDidChange') } - _configureStandardClient(_providerParams) { + _configureStandardProvider(_providerParams) { const providerParams = extend(this._baseProviderParams, _providerParams) - const client = createEthRpcClient(providerParams) - this._setClient(client) - } - - _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 = {} + const provider = createMetamaskProvider(providerParams) + this._setProvider(provider) + } + + _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() } + // override block tracler + provider._blockTracker = createEventEmitterProxy(provider._blockTracker, blockTrackerHandlers) // set as new provider - this._currentClient = newClient - this.providerProxy.setTarget(newClient.provider) - this.blockTrackerProxy.setTarget(newClient.blockTracker) + this._provider = provider + this._proxy.setTarget(provider) } _logBlock (block) { diff --git a/app/scripts/controllers/transactions.js b/app/scripts/controllers/transactions.js index d46dee230..ef659a300 100644 --- a/app/scripts/controllers/transactions.js +++ b/app/scripts/controllers/transactions.js @@ -46,7 +46,6 @@ 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({ diff --git a/app/scripts/lib/events-proxy.js b/app/scripts/lib/events-proxy.js index 840b06b1a..d1199a278 100644 --- a/app/scripts/lib/events-proxy.js +++ b/app/scripts/lib/events-proxy.js @@ -1,5 +1,6 @@ -module.exports = function createEventEmitterProxy(eventEmitter, eventHandlers = {}) { +module.exports = function createEventEmitterProxy(eventEmitter, listeners) { let target = eventEmitter + const eventHandlers = listeners || {} const proxy = new Proxy({}, { get: (obj, name) => { // intercept listeners @@ -13,12 +14,9 @@ module.exports = function createEventEmitterProxy(eventEmitter, eventHandlers = return true }, }) - proxy.setTarget(eventEmitter) - return proxy - function setTarget (eventEmitter) { target = eventEmitter - // migrate eventHandlers + // migrate listeners Object.keys(eventHandlers).forEach((name) => { eventHandlers[name].forEach((handler) => target.on(name, handler)) }) @@ -28,4 +26,6 @@ module.exports = function createEventEmitterProxy(eventEmitter, eventHandlers = eventHandlers[name].push(handler) target.on(name, handler) } + if (listeners) proxy.setTarget(eventEmitter) + return proxy } \ No newline at end of file diff --git a/app/scripts/lib/nonce-tracker.js b/app/scripts/lib/nonce-tracker.js index 2af40a27f..0029ac953 100644 --- a/app/scripts/lib/nonce-tracker.js +++ b/app/scripts/lib/nonce-tracker.js @@ -4,9 +4,8 @@ const Mutex = require('await-semaphore').Mutex class NonceTracker { - constructor ({ provider, blockTracker, getPendingTransactions, getConfirmedTransactions }) { + constructor ({ provider, getPendingTransactions, getConfirmedTransactions }) { this.provider = provider - this.blockTracker = blockTracker this.ethQuery = new EthQuery(provider) this.getPendingTransactions = getPendingTransactions this.getConfirmedTransactions = getConfirmedTransactions @@ -54,7 +53,7 @@ class NonceTracker { } async _getCurrentBlock () { - const blockTracker = this.blockTracker + const blockTracker = this._getBlockTracker() const currentBlock = blockTracker.getCurrentBlock() if (currentBlock) return currentBlock return await Promise((reject, resolve) => { @@ -140,6 +139,11 @@ 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 diff --git a/app/scripts/lib/obj-proxy.js b/app/scripts/lib/obj-proxy.js deleted file mode 100644 index 29ca1269f..000000000 --- a/app/scripts/lib/obj-proxy.js +++ /dev/null @@ -1,19 +0,0 @@ -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 - } -} \ No newline at end of file diff --git a/app/scripts/metamask-controller.js b/app/scripts/metamask-controller.js index a742f3cba..727f48f1c 100644 --- a/app/scripts/metamask-controller.js +++ b/app/scripts/metamask-controller.js @@ -81,24 +81,9 @@ module.exports = class MetamaskController extends EventEmitter { }) this.blacklistController.scheduleUpdates() - // 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 + // rpc provider + this.provider = this.initializeProvider() + this.blockTracker = this.provider._blockTracker // eth data query tools this.ethQuery = new EthQuery(this.provider) @@ -233,6 +218,36 @@ 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() @@ -468,18 +483,6 @@ 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')) @@ -526,11 +529,6 @@ 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() diff --git a/package.json b/package.json index d82766fb5..3843a8a41 100644 --- a/package.json +++ b/package.json @@ -71,11 +71,9 @@ "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.2.0", "eth-token-tracker": "^1.1.4", diff --git a/test/integration/lib/first-time.js b/test/integration/lib/first-time.js index ee49d0901..cedb14f6e 100644 --- a/test/integration/lib/first-time.js +++ b/test/integration/lib/first-time.js @@ -3,9 +3,6 @@ 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}`) diff --git a/test/unit/network-contoller-test.js b/test/unit/network-contoller-test.js index 42ca40c56..0b3b5adeb 100644 --- a/test/unit/network-contoller-test.js +++ b/test/unit/network-contoller-test.js @@ -14,15 +14,15 @@ describe('# Network Controller', function () { }, }) - networkController.initializeProvider(networkControllerProviderInit) + networkController.initializeProvider(networkControllerProviderInit, dummyProviderConstructor) }) describe('network', function () { describe('#provider', function () { it('provider should be updatable without reassignment', function () { - networkController.initializeProvider(networkControllerProviderInit) - const providerProxy = networkController.providerProxy - providerProxy.setTarget({ test: true }) - assert.ok(providerProxy.test) + networkController.initializeProvider(networkControllerProviderInit, dummyProviderConstructor) + const proxy = networkController._proxy + proxy.setTarget({ test: true, on: () => {} }) + assert.ok(proxy.test) }) }) describe('#getNetworkState', function () { @@ -66,4 +66,19 @@ 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() {} \ No newline at end of file diff --git a/test/unit/nonce-tracker-test.js b/test/unit/nonce-tracker-test.js index 77af2a21c..8970cf84d 100644 --- a/test/unit/nonce-tracker-test.js +++ b/test/unit/nonce-tracker-test.js @@ -190,13 +190,12 @@ function generateNonceTrackerWith (pending, confirmed, providerStub = '0x0') { providerResultStub.result = providerStub const provider = { sendAsync: (_, cb) => { cb(undefined, providerResultStub) }, - } - const blockTracker = { - getCurrentBlock: () => '0x11b568', + _blockTracker: { + getCurrentBlock: () => '0x11b568', + }, } return new NonceTracker({ provider, - blockTracker, getPendingTransactions, getConfirmedTransactions, }) diff --git a/ui/app/app.js b/ui/app/app.js index 30d3766ab..613577913 100644 --- a/ui/app/app.js +++ b/ui/app/app.js @@ -319,7 +319,7 @@ App.prototype.renderNetworkDropdown = function () { [ h('i.fa.fa-question-circle.fa-lg.menu-icon'), 'Localhost 8545', - providerType === 'localhost' ? h('.check', '✓') : null, + activeNetwork === 'http://localhost:8545' ? h('.check', '✓') : null, ] ),