From 9c6c277b8b5d33568d81a81c3e13fc06b0f7241a Mon Sep 17 00:00:00 2001 From: Dan Finlay Date: Mon, 2 Jan 2017 14:38:04 -0800 Subject: [PATCH 01/18] Add replay protection to Transaction Manager Fixes #897 Needs tests. --- app/scripts/metamask-controller.js | 6 ++-- app/scripts/transaction-manager.js | 47 +++++++++++++++++------------- 2 files changed, 30 insertions(+), 23 deletions(-) diff --git a/app/scripts/metamask-controller.js b/app/scripts/metamask-controller.js index 5df10672a..31bb4a9c2 100644 --- a/app/scripts/metamask-controller.js +++ b/app/scripts/metamask-controller.js @@ -177,8 +177,8 @@ module.exports = class MetamaskController { // tx signing approveTransaction: this.newUnsignedTransaction.bind(this), signTransaction: (...args) => { - this.setupSigningListners(...args) - this.txManager.formatTxForSigining(...args) + this.setupSigningListeners(...args) + this.txManager.formatTxForSigning(...args) this.sendUpdate() }, @@ -257,7 +257,7 @@ module.exports = class MetamaskController { }) } - setupSigningListners (txParams) { + setupSigningListeners (txParams) { var txId = txParams.metamaskId // apply event listeners for signing and formating events this.txManager.once(`${txId}:formatted`, this.keyringController.signTransaction.bind(this.keyringController)) diff --git a/app/scripts/transaction-manager.js b/app/scripts/transaction-manager.js index fd136a51b..3cf12b016 100644 --- a/app/scripts/transaction-manager.js +++ b/app/scripts/transaction-manager.js @@ -137,26 +137,33 @@ module.exports = class TransactionManager extends EventEmitter { } // formats txParams so the keyringController can sign it - formatTxForSigining (txParams, cb) { - var address = txParams.from - var metaTx = this.getTx(txParams.metamaskId) - var gasMultiplier = metaTx.gasMultiplier - var gasPrice = new BN(ethUtil.stripHexPrefix(txParams.gasPrice), 16) - gasPrice = gasPrice.mul(new BN(gasMultiplier * 100, 10)).div(new BN(100, 10)) - txParams.gasPrice = ethUtil.intToHex(gasPrice.toNumber()) - - // normalize values - txParams.to = normalize(txParams.to) - txParams.from = normalize(txParams.from) - txParams.value = normalize(txParams.value) - txParams.data = normalize(txParams.data) - txParams.gasLimit = normalize(txParams.gasLimit || txParams.gas) - txParams.nonce = normalize(txParams.nonce) - const ethTx = new Transaction(txParams) - - // listener is assigned in metamaskController - this.emit(`${txParams.metamaskId}:formatted`, ethTx, address, txParams.metamaskId, cb) - } + formatTxForSigning (txParams, cb) { + this.getNetwork((err, networkId) => { + if (err) { + return cb(err) + } + + var address = txParams.from + var metaTx = this.getTx(txParams.metamaskId) + var gasMultiplier = metaTx.gasMultiplier + var gasPrice = new BN(ethUtil.stripHexPrefix(txParams.gasPrice), 16) + gasPrice = gasPrice.mul(new BN(gasMultiplier * 100, 10)).div(new BN(100, 10)) + txParams.gasPrice = ethUtil.intToHex(gasPrice.toNumber()) + + // normalize values + txParams.to = normalize(txParams.to) + txParams.from = normalize(txParams.from) + txParams.value = normalize(txParams.value) + txParams.data = normalize(txParams.data) + txParams.gasLimit = normalize(txParams.gasLimit || txParams.gas) + txParams.nonce = normalize(txParams.nonce) + + const ethTx = new Transaction(txParams, parseInt(networkId)) + + // listener is assigned in metamaskController + this.emit(`${txParams.metamaskId}:formatted`, ethTx, address, txParams.metamaskId, cb) + }) + } // receives a signed tx object and updates the tx hash // and pass it to the cb to be sent off From d2fb48f8ba1c717d3360d48551b568b511b7d84d Mon Sep 17 00:00:00 2001 From: Dan Finlay Date: Mon, 2 Jan 2017 14:41:04 -0800 Subject: [PATCH 02/18] Bump changelog --- CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 239203553..0a06314e0 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -3,6 +3,7 @@ ## Current Master - Add a check for when a tx is included in a block. +- Implement replay attack protections allowed by EIP 155. ## 2.14.1 2016-12-20 From 7320095079717b296776ac8e38571020a5d558cf Mon Sep 17 00:00:00 2001 From: Dan Finlay Date: Mon, 2 Jan 2017 14:54:31 -0800 Subject: [PATCH 03/18] Linted --- app/scripts/transaction-manager.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/app/scripts/transaction-manager.js b/app/scripts/transaction-manager.js index 3cf12b016..7fcdbe7ca 100644 --- a/app/scripts/transaction-manager.js +++ b/app/scripts/transaction-manager.js @@ -162,8 +162,8 @@ module.exports = class TransactionManager extends EventEmitter { // listener is assigned in metamaskController this.emit(`${txParams.metamaskId}:formatted`, ethTx, address, txParams.metamaskId, cb) - }) - } + }) + } // receives a signed tx object and updates the tx hash // and pass it to the cb to be sent off From 305cda4265e84d520d21c473a796e8f71febc57f Mon Sep 17 00:00:00 2001 From: Dan Finlay Date: Mon, 2 Jan 2017 15:31:05 -0800 Subject: [PATCH 04/18] Use chainId parameter instead of second argument --- app/scripts/transaction-manager.js | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/app/scripts/transaction-manager.js b/app/scripts/transaction-manager.js index 7fcdbe7ca..d426993a4 100644 --- a/app/scripts/transaction-manager.js +++ b/app/scripts/transaction-manager.js @@ -157,8 +157,9 @@ module.exports = class TransactionManager extends EventEmitter { txParams.data = normalize(txParams.data) txParams.gasLimit = normalize(txParams.gasLimit || txParams.gas) txParams.nonce = normalize(txParams.nonce) + txParams.chainId = parseInt(networkId) - const ethTx = new Transaction(txParams, parseInt(networkId)) + const ethTx = new Transaction(txParams) // listener is assigned in metamaskController this.emit(`${txParams.metamaskId}:formatted`, ethTx, address, txParams.metamaskId, cb) From e6da8e2762cd54975c334314357f1cd27cc980c8 Mon Sep 17 00:00:00 2001 From: Frankie Date: Wed, 4 Jan 2017 13:04:33 -0800 Subject: [PATCH 05/18] Fix signing of transactions --- app/scripts/keyring-controller.js | 28 +++++++++-------- app/scripts/metamask-controller.js | 30 +++++++++++------- app/scripts/transaction-manager.js | 50 +++++++++++++++++------------- 3 files changed, 62 insertions(+), 46 deletions(-) diff --git a/app/scripts/keyring-controller.js b/app/scripts/keyring-controller.js index 016740d88..acec8feb1 100644 --- a/app/scripts/keyring-controller.js +++ b/app/scripts/keyring-controller.js @@ -310,20 +310,22 @@ module.exports = class KeyringController extends EventEmitter { // // This method signs tx and returns a promise for // TX Manager to update the state after signing - signTransaction (ethTx, selectedAddress, txId, cb) { - try { - const address = normalize(selectedAddress) - return this.getKeyringForAccount(address) - .then((keyring) => { - return keyring.signTransaction(address, ethTx) - }).then((tx) => { - this.emit(`${txId}:signed`, {tx, txId, cb}) - }) - } catch (e) { - cb(e) - } - } + signTransaction (ethTx, selectedAddress, txId) { + return new Promise((resolve, reject) => { + try { + const address = normalize(selectedAddress) + return this.getKeyringForAccount(address) + .then((keyring) => { + return keyring.signTransaction(address, ethTx) + }).then((tx) => { + resolve({tx, txId}) + }) + } catch (e) { + reject(e) + } + }) + } // Add Unconfirmed Message // @object msgParams // @function cb diff --git a/app/scripts/metamask-controller.js b/app/scripts/metamask-controller.js index 5df10672a..32e9faab2 100644 --- a/app/scripts/metamask-controller.js +++ b/app/scripts/metamask-controller.js @@ -176,10 +176,25 @@ module.exports = class MetamaskController { }, // tx signing approveTransaction: this.newUnsignedTransaction.bind(this), - signTransaction: (...args) => { - this.setupSigningListners(...args) - this.txManager.formatTxForSigining(...args) - this.sendUpdate() + signTransaction: (txParams, cb) => { + this.txManager.formatTxForSigining(txParams) + .then(({ethTx, address, txId}) => { + return this.keyringController.signTransaction(ethTx, address, txId) + }) + .then(({tx, txId}) => { + return this.txManager.resolveSignedTransaction({tx, txId}) + }) + .then((rawTx) => { + cb(null, rawTx) + }) + .catch((err) => { + console.error(err) + cb(err) + }) + .then(() => { + this.sendUpdate() + this.txManager.emit(`${txParams.metamaskId}:signingComplete`) + }) }, // msg signing @@ -257,13 +272,6 @@ module.exports = class MetamaskController { }) } - setupSigningListners (txParams) { - var txId = txParams.metamaskId - // apply event listeners for signing and formating events - this.txManager.once(`${txId}:formatted`, this.keyringController.signTransaction.bind(this.keyringController)) - this.keyringController.once(`${txId}:signed`, this.txManager.resolveSignedTransaction.bind(this.txManager)) - } - enforceTxValidations (txParams) { if (('value' in txParams) && txParams.value.indexOf('-') === 0) { const msg = `Invalid transaction value of ${txParams.value} not a positive number.` diff --git a/app/scripts/transaction-manager.js b/app/scripts/transaction-manager.js index fd136a51b..40078d644 100644 --- a/app/scripts/transaction-manager.js +++ b/app/scripts/transaction-manager.js @@ -17,7 +17,7 @@ module.exports = class TransactionManager extends EventEmitter { this.provider = opts.provider this.blockTracker = opts.blockTracker this.txProviderUtils = new TxProviderUtil(this.provider) - this.blockTracker.on('block', this.checkForTxInBlock.bind(this)) + // this.blockTracker.on('block', this.checkForTxInBlock.bind(this)) this.getGasMultiplier = opts.getGasMultiplier this.getNetwork = opts.getNetwork } @@ -128,7 +128,7 @@ module.exports = class TransactionManager extends EventEmitter { approveTransaction (txId, cb = warn) { this.setTxStatusSigned(txId) - cb() + this.once(`${txId}:signingComplete`, cb) } cancelTransaction (txId, cb = warn) { @@ -137,25 +137,30 @@ module.exports = class TransactionManager extends EventEmitter { } // formats txParams so the keyringController can sign it - formatTxForSigining (txParams, cb) { - var address = txParams.from - var metaTx = this.getTx(txParams.metamaskId) - var gasMultiplier = metaTx.gasMultiplier - var gasPrice = new BN(ethUtil.stripHexPrefix(txParams.gasPrice), 16) - gasPrice = gasPrice.mul(new BN(gasMultiplier * 100, 10)).div(new BN(100, 10)) - txParams.gasPrice = ethUtil.intToHex(gasPrice.toNumber()) - - // normalize values - txParams.to = normalize(txParams.to) - txParams.from = normalize(txParams.from) - txParams.value = normalize(txParams.value) - txParams.data = normalize(txParams.data) - txParams.gasLimit = normalize(txParams.gasLimit || txParams.gas) - txParams.nonce = normalize(txParams.nonce) - const ethTx = new Transaction(txParams) - - // listener is assigned in metamaskController - this.emit(`${txParams.metamaskId}:formatted`, ethTx, address, txParams.metamaskId, cb) + formatTxForSigining (txParams) { + return new Promise((resolve, reject) => { + try { + var address = txParams.from + var metaTx = this.getTx(txParams.metamaskId) + var gasMultiplier = metaTx.gasMultiplier + var gasPrice = new BN(ethUtil.stripHexPrefix(txParams.gasPrice), 16) + gasPrice = gasPrice.mul(new BN(gasMultiplier * 100, 10)).div(new BN(100, 10)) + txParams.gasPrice = ethUtil.intToHex(gasPrice.toNumber()) + + // normalize values + txParams.to = normalize(txParams.to) + txParams.from = normalize(txParams.from) + txParams.value = normalize(txParams.value) + txParams.data = normalize(txParams.data) + txParams.gasLimit = normalize(txParams.gasLimit || txParams.gas) + txParams.nonce = normalize(txParams.nonce) + const ethTx = new Transaction(txParams) + var txId = txParams.metamaskId + resolve({ethTx, address, txId}) + } catch (err) { + reject(err) + } + }) } // receives a signed tx object and updates the tx hash @@ -167,7 +172,8 @@ module.exports = class TransactionManager extends EventEmitter { metaTx.hash = txHash this.updateTx(metaTx) var rawTx = ethUtil.bufferToHex(tx.serialize()) - cb(null, rawTx) + return Promise.resolve(rawTx) + } /* From 32e8063a778740cddbda67008bd68aa7500b9191 Mon Sep 17 00:00:00 2001 From: Frankie Date: Wed, 4 Jan 2017 13:13:34 -0800 Subject: [PATCH 06/18] clean up --- app/scripts/transaction-manager.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/scripts/transaction-manager.js b/app/scripts/transaction-manager.js index 40078d644..a2dba69b5 100644 --- a/app/scripts/transaction-manager.js +++ b/app/scripts/transaction-manager.js @@ -17,7 +17,7 @@ module.exports = class TransactionManager extends EventEmitter { this.provider = opts.provider this.blockTracker = opts.blockTracker this.txProviderUtils = new TxProviderUtil(this.provider) - // this.blockTracker.on('block', this.checkForTxInBlock.bind(this)) + this.blockTracker.on('block', this.checkForTxInBlock.bind(this)) this.getGasMultiplier = opts.getGasMultiplier this.getNetwork = opts.getNetwork } From 08351f801abd1bfd2a8c7071c6a3b0bd40570a4e Mon Sep 17 00:00:00 2001 From: Dan Finlay Date: Wed, 4 Jan 2017 13:56:21 -0800 Subject: [PATCH 07/18] Move eth-store@1.1.0 into local repo. --- app/scripts/lib/eth-store.js | 132 +++++++++++++++++++++++++++++++++++ 1 file changed, 132 insertions(+) create mode 100644 app/scripts/lib/eth-store.js diff --git a/app/scripts/lib/eth-store.js b/app/scripts/lib/eth-store.js new file mode 100644 index 000000000..aeb5667e5 --- /dev/null +++ b/app/scripts/lib/eth-store.js @@ -0,0 +1,132 @@ +const EventEmitter = require('events').EventEmitter +const inherits = require('util').inherits +const async = require('async') +const clone = require('clone') +const EthQuery = require('eth-query') + +module.exports = EthereumStore + + +inherits(EthereumStore, EventEmitter) +function EthereumStore(engine) { + const self = this + EventEmitter.call(self) + self._currentState = { + accounts: {}, + transactions: {}, + } + self._query = new EthQuery(engine) + + engine.on('block', self._updateForBlock.bind(self)) +} + +// +// public +// + +EthereumStore.prototype.getState = function(){ + const self = this + return clone(self._currentState) +} + +EthereumStore.prototype.addAccount = function(address){ + const self = this + self._currentState.accounts[address] = {} + self._didUpdate() + if (!self.currentBlockNumber) return + self._updateAccountForBlock(self.currentBlockNumber, address, noop) +} + +EthereumStore.prototype.removeAccount = function(address){ + const self = this + delete self._currentState.accounts[address] + self._didUpdate() +} + +EthereumStore.prototype.addTransaction = function(txHash){ + const self = this + self._currentState.transactions[txHash] = {} + self._didUpdate() + if (!self.currentBlockNumber) return + self._updateTransaction(self.currentBlockNumber, txHash, noop) +} + +EthereumStore.prototype.removeTransaction = function(address){ + const self = this + delete self._currentState.transactions[address] + self._didUpdate() +} + + +// +// private +// + +EthereumStore.prototype._didUpdate = function() { + const self = this + var state = self.getState() + self.emit('update', state) +} + +EthereumStore.prototype._updateForBlock = function(block) { + const self = this + var blockNumber = '0x'+block.number.toString('hex') + self.currentBlockNumber = blockNumber + async.parallel([ + self._updateAccountsForBlock.bind(self, blockNumber), + self._updateTransactions.bind(self, blockNumber), + ], function(err){ + if (err) return console.error(err) + self.emit('block', self.getState()) + }) +} + +EthereumStore.prototype._updateAccountsForBlock = function(block, cb) { + const self = this + var accountsState = self._currentState.accounts + var addresses = Object.keys(accountsState) + async.each(addresses, self._updateAccountForBlock.bind(self, block), cb) +} + +EthereumStore.prototype._updateAccountForBlock = function(block, address, cb) { + const self = this + var accountsState = self._currentState.accounts + self._query.getAccount(address, block, function(err, result){ + if (err) return cb(err) + result.address = address + // only populate if the entry is still present + if (accountsState[address]) { + accountsState[address] = result + self._didUpdate() + } + cb(null, result) + }) +} + +EthereumStore.prototype._updateTransactions = function(block, cb) { + const self = this + var transactionsState = self._currentState.transactions + var txHashes = Object.keys(transactionsState) + async.each(txHashes, self._updateTransaction.bind(self, block), cb) +} + +EthereumStore.prototype._updateTransaction = function(block, txHash, cb) { + const self = this + // would use the block here to determine how many confirmations the tx has + var transactionsState = self._currentState.transactions + self._query.getTransaction(txHash, function(err, result){ + if (err) return cb(err) + // only populate if the entry is still present + if (transactionsState[txHash]) { + transactionsState[txHash] = result + self._didUpdate() + } + cb(null, result) + }) +} + +function valuesFor(obj){ + return Object.keys(obj).map(function(key){ return obj[key] }) +} + +function noop(){} From 3588aabdf2639f65a5896f2f249c1277f3c95b84 Mon Sep 17 00:00:00 2001 From: Dan Finlay Date: Wed, 4 Jan 2017 14:01:32 -0800 Subject: [PATCH 08/18] Removed reliance on eth-store internal custom eth-query --- app/scripts/lib/eth-store.js | 21 +++++++++++++++------ 1 file changed, 15 insertions(+), 6 deletions(-) diff --git a/app/scripts/lib/eth-store.js b/app/scripts/lib/eth-store.js index aeb5667e5..e2451408c 100644 --- a/app/scripts/lib/eth-store.js +++ b/app/scripts/lib/eth-store.js @@ -34,7 +34,7 @@ EthereumStore.prototype.addAccount = function(address){ self._currentState.accounts[address] = {} self._didUpdate() if (!self.currentBlockNumber) return - self._updateAccountForBlock(self.currentBlockNumber, address, noop) + self._updateAccount(self.currentBlockNumber, address, noop) } EthereumStore.prototype.removeAccount = function(address){ @@ -73,7 +73,7 @@ EthereumStore.prototype._updateForBlock = function(block) { var blockNumber = '0x'+block.number.toString('hex') self.currentBlockNumber = blockNumber async.parallel([ - self._updateAccountsForBlock.bind(self, blockNumber), + self._updateAccounts.bind(self), self._updateTransactions.bind(self, blockNumber), ], function(err){ if (err) return console.error(err) @@ -81,17 +81,17 @@ EthereumStore.prototype._updateForBlock = function(block) { }) } -EthereumStore.prototype._updateAccountsForBlock = function(block, cb) { +EthereumStore.prototype._updateAccounts = function(cb) { const self = this var accountsState = self._currentState.accounts var addresses = Object.keys(accountsState) - async.each(addresses, self._updateAccountForBlock.bind(self, block), cb) + async.each(addresses, self._updateAccount.bind(self), cb) } -EthereumStore.prototype._updateAccountForBlock = function(block, address, cb) { +EthereumStore.prototype._updateAccount = function(address, cb) { const self = this var accountsState = self._currentState.accounts - self._query.getAccount(address, block, function(err, result){ + self._query.getAccount(address, function(err, result){ if (err) return cb(err) result.address = address // only populate if the entry is still present @@ -103,6 +103,15 @@ EthereumStore.prototype._updateAccountForBlock = function(block, address, cb) { }) } +EthereumStore.prototype.getAccount = function(address, cb){ + const block = 'latest' + async.parallel({ + balance: this._query.getBalance.bind(this, address, block), + nonce: this._query.getNonce.bind(this, address, block), + code: this._query.getCode.bind(this, address, block), + }, cb) +} + EthereumStore.prototype._updateTransactions = function(block, cb) { const self = this var transactionsState = self._currentState.transactions From d4958b7ffd11e73c01ea637f1acf61f4f297a02d Mon Sep 17 00:00:00 2001 From: Dan Finlay Date: Wed, 4 Jan 2017 14:01:43 -0800 Subject: [PATCH 09/18] Remove old eth-store dependency --- package.json | 1 - 1 file changed, 1 deletion(-) diff --git a/package.json b/package.json index 4c33ad9ab..6f0fe00c7 100644 --- a/package.json +++ b/package.json @@ -49,7 +49,6 @@ "eth-bin-to-ops": "^1.0.1", "eth-lightwallet": "^2.3.3", "eth-query": "^1.0.3", - "eth-store": "^1.1.0", "ethereumjs-tx": "^1.0.0", "ethereumjs-util": "^4.4.0", "ethereumjs-wallet": "^0.6.0", From 0fab22082a4583f2ca8b39c8a73ee338302d6561 Mon Sep 17 00:00:00 2001 From: Dan Finlay Date: Wed, 4 Jan 2017 14:02:13 -0800 Subject: [PATCH 10/18] Bump changelog --- CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index b10ff010f..a3b97d477 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,6 +4,7 @@ - Remove certain non-essential permissions from certain builds. - Add a check for when a tx is included in a block. +- Fix bug where sometimes loading account data would fail by querying a future block. ## 2.14.1 2016-12-20 From 381a60695d218d2ae965df9a9a2566029302d133 Mon Sep 17 00:00:00 2001 From: Dan Finlay Date: Wed, 4 Jan 2017 14:05:00 -0800 Subject: [PATCH 11/18] Linted --- app/scripts/lib/eth-store.js | 39 ++++++++++++++++-------------------- 1 file changed, 17 insertions(+), 22 deletions(-) diff --git a/app/scripts/lib/eth-store.js b/app/scripts/lib/eth-store.js index e2451408c..0ed7ef545 100644 --- a/app/scripts/lib/eth-store.js +++ b/app/scripts/lib/eth-store.js @@ -24,12 +24,12 @@ function EthereumStore(engine) { // public // -EthereumStore.prototype.getState = function(){ +EthereumStore.prototype.getState = function () { const self = this return clone(self._currentState) } -EthereumStore.prototype.addAccount = function(address){ +EthereumStore.prototype.addAccount = function (address) { const self = this self._currentState.accounts[address] = {} self._didUpdate() @@ -37,13 +37,13 @@ EthereumStore.prototype.addAccount = function(address){ self._updateAccount(self.currentBlockNumber, address, noop) } -EthereumStore.prototype.removeAccount = function(address){ +EthereumStore.prototype.removeAccount = function (address) { const self = this delete self._currentState.accounts[address] self._didUpdate() } -EthereumStore.prototype.addTransaction = function(txHash){ +EthereumStore.prototype.addTransaction = function (txHash) { const self = this self._currentState.transactions[txHash] = {} self._didUpdate() @@ -51,7 +51,7 @@ EthereumStore.prototype.addTransaction = function(txHash){ self._updateTransaction(self.currentBlockNumber, txHash, noop) } -EthereumStore.prototype.removeTransaction = function(address){ +EthereumStore.prototype.removeTransaction = function (address) { const self = this delete self._currentState.transactions[address] self._didUpdate() @@ -62,36 +62,36 @@ EthereumStore.prototype.removeTransaction = function(address){ // private // -EthereumStore.prototype._didUpdate = function() { +EthereumStore.prototype._didUpdate = function () { const self = this var state = self.getState() self.emit('update', state) } -EthereumStore.prototype._updateForBlock = function(block) { +EthereumStore.prototype._updateForBlock = function (block) { const self = this var blockNumber = '0x'+block.number.toString('hex') self.currentBlockNumber = blockNumber async.parallel([ self._updateAccounts.bind(self), self._updateTransactions.bind(self, blockNumber), - ], function(err){ + ], function (err) { if (err) return console.error(err) self.emit('block', self.getState()) }) } -EthereumStore.prototype._updateAccounts = function(cb) { +EthereumStore.prototype._updateAccounts = function (cb) { const self = this var accountsState = self._currentState.accounts var addresses = Object.keys(accountsState) async.each(addresses, self._updateAccount.bind(self), cb) } -EthereumStore.prototype._updateAccount = function(address, cb) { +EthereumStore.prototype._updateAccount = function (address, cb) { const self = this var accountsState = self._currentState.accounts - self._query.getAccount(address, function(err, result){ + self._query.getAccount(address, function (err, result) { if (err) return cb(err) result.address = address // only populate if the entry is still present @@ -103,7 +103,7 @@ EthereumStore.prototype._updateAccount = function(address, cb) { }) } -EthereumStore.prototype.getAccount = function(address, cb){ +EthereumStore.prototype.getAccount = function (address, cb) { const block = 'latest' async.parallel({ balance: this._query.getBalance.bind(this, address, block), @@ -112,21 +112,20 @@ EthereumStore.prototype.getAccount = function(address, cb){ }, cb) } -EthereumStore.prototype._updateTransactions = function(block, cb) { +EthereumStore.prototype._updateTransactions = function (block, cb) { const self = this var transactionsState = self._currentState.transactions var txHashes = Object.keys(transactionsState) async.each(txHashes, self._updateTransaction.bind(self, block), cb) } -EthereumStore.prototype._updateTransaction = function(block, txHash, cb) { +EthereumStore.prototype._updateTransaction = function (block, txHash, cb) { const self = this // would use the block here to determine how many confirmations the tx has var transactionsState = self._currentState.transactions - self._query.getTransaction(txHash, function(err, result){ + self._query.getTransaction(txHash, function (err, result) { if (err) return cb(err) - // only populate if the entry is still present - if (transactionsState[txHash]) { + // only populate if the entry is still present if (transactionsState[txHash]) { transactionsState[txHash] = result self._didUpdate() } @@ -134,8 +133,4 @@ EthereumStore.prototype._updateTransaction = function(block, txHash, cb) { }) } -function valuesFor(obj){ - return Object.keys(obj).map(function(key){ return obj[key] }) -} - -function noop(){} +function noop() {} From 18e6d266c1ac31bb17a5f2816167ea2d4b01ad64 Mon Sep 17 00:00:00 2001 From: Dan Finlay Date: Wed, 4 Jan 2017 14:07:08 -0800 Subject: [PATCH 12/18] Add descriptive comment --- app/scripts/lib/eth-store.js | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/app/scripts/lib/eth-store.js b/app/scripts/lib/eth-store.js index 0ed7ef545..cca6f7d5d 100644 --- a/app/scripts/lib/eth-store.js +++ b/app/scripts/lib/eth-store.js @@ -1,3 +1,12 @@ +/* Ethereum Store + * + * This module is responsible for tracking any number of accounts + * and caching their current balances & transaction counts. + * + * It also tracks transaction hashes, and checks their inclusion status + * on each new block. + */ + const EventEmitter = require('events').EventEmitter const inherits = require('util').inherits const async = require('async') From a107b66854a736a2809fba2a490039b9920b92d5 Mon Sep 17 00:00:00 2001 From: Dan Finlay Date: Wed, 4 Jan 2017 14:14:45 -0800 Subject: [PATCH 13/18] Caught typo --- app/scripts/lib/eth-store.js | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/app/scripts/lib/eth-store.js b/app/scripts/lib/eth-store.js index cca6f7d5d..3b5501095 100644 --- a/app/scripts/lib/eth-store.js +++ b/app/scripts/lib/eth-store.js @@ -79,7 +79,7 @@ EthereumStore.prototype._didUpdate = function () { EthereumStore.prototype._updateForBlock = function (block) { const self = this - var blockNumber = '0x'+block.number.toString('hex') + var blockNumber = '0x' + block.number.toString('hex') self.currentBlockNumber = blockNumber async.parallel([ self._updateAccounts.bind(self), @@ -134,7 +134,8 @@ EthereumStore.prototype._updateTransaction = function (block, txHash, cb) { var transactionsState = self._currentState.transactions self._query.getTransaction(txHash, function (err, result) { if (err) return cb(err) - // only populate if the entry is still present if (transactionsState[txHash]) { + // only populate if the entry is still present + if (transactionsState[txHash]) { transactionsState[txHash] = result self._didUpdate() } From ead8a05034e22da36d2a0d02109f3de35f1785f1 Mon Sep 17 00:00:00 2001 From: Dan Finlay Date: Wed, 4 Jan 2017 14:21:36 -0800 Subject: [PATCH 14/18] Fix dependency reference --- app/scripts/metamask-controller.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/scripts/metamask-controller.js b/app/scripts/metamask-controller.js index c0d2f3b4c..8bfb85370 100644 --- a/app/scripts/metamask-controller.js +++ b/app/scripts/metamask-controller.js @@ -1,5 +1,5 @@ const extend = require('xtend') -const EthStore = require('eth-store') +const EthStore = require('./lib/eth-store') const MetaMaskProvider = require('web3-provider-engine/zero.js') const KeyringController = require('./keyring-controller') const NoticeController = require('./notice-controller') From 6c99d09404d4b0a43d49c7a98351018f61229542 Mon Sep 17 00:00:00 2001 From: Dan Finlay Date: Wed, 4 Jan 2017 14:54:40 -0800 Subject: [PATCH 15/18] Fixed bugs with sanity-checking - Was incorrectly calling some eth-query methods (left over from old local eth-query) - Was still passing block to getAccount in addAccount - Now emitting update only after all account balances are loaded, reducing UI update traffic. --- app/scripts/lib/eth-store.js | 22 ++++++++++------------ 1 file changed, 10 insertions(+), 12 deletions(-) diff --git a/app/scripts/lib/eth-store.js b/app/scripts/lib/eth-store.js index 3b5501095..a42b2417f 100644 --- a/app/scripts/lib/eth-store.js +++ b/app/scripts/lib/eth-store.js @@ -43,7 +43,7 @@ EthereumStore.prototype.addAccount = function (address) { self._currentState.accounts[address] = {} self._didUpdate() if (!self.currentBlockNumber) return - self._updateAccount(self.currentBlockNumber, address, noop) + self._updateAccount(address, noop) } EthereumStore.prototype.removeAccount = function (address) { @@ -87,37 +87,35 @@ EthereumStore.prototype._updateForBlock = function (block) { ], function (err) { if (err) return console.error(err) self.emit('block', self.getState()) + self._didUpdate() }) } EthereumStore.prototype._updateAccounts = function (cb) { - const self = this - var accountsState = self._currentState.accounts + var accountsState = this._currentState.accounts var addresses = Object.keys(accountsState) - async.each(addresses, self._updateAccount.bind(self), cb) + async.each(addresses, this._updateAccount.bind(this), cb) } EthereumStore.prototype._updateAccount = function (address, cb) { - const self = this - var accountsState = self._currentState.accounts - self._query.getAccount(address, function (err, result) { + var accountsState = this._currentState.accounts + this.getAccount(address, function (err, result) { if (err) return cb(err) result.address = address // only populate if the entry is still present if (accountsState[address]) { accountsState[address] = result - self._didUpdate() } cb(null, result) }) } EthereumStore.prototype.getAccount = function (address, cb) { - const block = 'latest' + const query = this._query async.parallel({ - balance: this._query.getBalance.bind(this, address, block), - nonce: this._query.getNonce.bind(this, address, block), - code: this._query.getCode.bind(this, address, block), + balance: query.getBalance.bind(query, address), + nonce: query.getTransactionCount.bind(query, address), + code: query.getCode.bind(query, address), }, cb) } From cf6817092b92930ab96e1c9e610669b485b384ff Mon Sep 17 00:00:00 2001 From: Frankie Date: Wed, 4 Jan 2017 15:03:45 -0800 Subject: [PATCH 16/18] remove unnecessary try statments --- app/scripts/keyring-controller.js | 18 +++++--------- app/scripts/metamask-controller.js | 3 +-- app/scripts/transaction-manager.js | 40 +++++++++++++----------------- 3 files changed, 24 insertions(+), 37 deletions(-) diff --git a/app/scripts/keyring-controller.js b/app/scripts/keyring-controller.js index 1a26a887f..c58be0aae 100644 --- a/app/scripts/keyring-controller.js +++ b/app/scripts/keyring-controller.js @@ -318,18 +318,12 @@ module.exports = class KeyringController extends EventEmitter { // TX Manager to update the state after signing signTransaction (ethTx, selectedAddress, txId) { - return new Promise((resolve, reject) => { - try { - const address = normalize(selectedAddress) - return this.getKeyringForAccount(address) - .then((keyring) => { - return keyring.signTransaction(address, ethTx) - }).then((tx) => { - resolve({tx, txId}) - }) - } catch (e) { - reject(e) - } + const address = normalize(selectedAddress) + return this.getKeyringForAccount(address) + .then((keyring) => { + return keyring.signTransaction(address, ethTx) + }).then((tx) => { + return {tx, txId} }) } // Add Unconfirmed Message diff --git a/app/scripts/metamask-controller.js b/app/scripts/metamask-controller.js index 4caad46e1..d520a59ad 100644 --- a/app/scripts/metamask-controller.js +++ b/app/scripts/metamask-controller.js @@ -165,7 +165,6 @@ module.exports = class MetamaskController { sendUpdate () { this.getState() .then((state) => { - this.listeners.forEach((remote) => { remote.sendUpdate(state) }) @@ -461,7 +460,7 @@ module.exports = class MetamaskController { return this.state.network } - markAccountsFound(cb) { + markAccountsFound (cb) { this.configManager.setLostAccounts([]) this.sendUpdate() cb(null, this.getState()) diff --git a/app/scripts/transaction-manager.js b/app/scripts/transaction-manager.js index a2dba69b5..6becfa6d1 100644 --- a/app/scripts/transaction-manager.js +++ b/app/scripts/transaction-manager.js @@ -138,29 +138,23 @@ module.exports = class TransactionManager extends EventEmitter { // formats txParams so the keyringController can sign it formatTxForSigining (txParams) { - return new Promise((resolve, reject) => { - try { - var address = txParams.from - var metaTx = this.getTx(txParams.metamaskId) - var gasMultiplier = metaTx.gasMultiplier - var gasPrice = new BN(ethUtil.stripHexPrefix(txParams.gasPrice), 16) - gasPrice = gasPrice.mul(new BN(gasMultiplier * 100, 10)).div(new BN(100, 10)) - txParams.gasPrice = ethUtil.intToHex(gasPrice.toNumber()) - - // normalize values - txParams.to = normalize(txParams.to) - txParams.from = normalize(txParams.from) - txParams.value = normalize(txParams.value) - txParams.data = normalize(txParams.data) - txParams.gasLimit = normalize(txParams.gasLimit || txParams.gas) - txParams.nonce = normalize(txParams.nonce) - const ethTx = new Transaction(txParams) - var txId = txParams.metamaskId - resolve({ethTx, address, txId}) - } catch (err) { - reject(err) - } - }) + var address = txParams.from + var metaTx = this.getTx(txParams.metamaskId) + var gasMultiplier = metaTx.gasMultiplier + var gasPrice = new BN(ethUtil.stripHexPrefix(txParams.gasPrice), 16) + gasPrice = gasPrice.mul(new BN(gasMultiplier * 100, 10)).div(new BN(100, 10)) + txParams.gasPrice = ethUtil.intToHex(gasPrice.toNumber()) + + // normalize values + txParams.to = normalize(txParams.to) + txParams.from = normalize(txParams.from) + txParams.value = normalize(txParams.value) + txParams.data = normalize(txParams.data) + txParams.gasLimit = normalize(txParams.gasLimit || txParams.gas) + txParams.nonce = normalize(txParams.nonce) + const ethTx = new Transaction(txParams) + var txId = txParams.metamaskId + return Promise.resolve({ethTx, address, txId}) } // receives a signed tx object and updates the tx hash From 7659f5894a76bb905529d7e5ea88a156d06373d2 Mon Sep 17 00:00:00 2001 From: Dan Finlay Date: Thu, 5 Jan 2017 11:03:30 -0800 Subject: [PATCH 17/18] Combine two identical then blocks --- app/scripts/metamask-controller.js | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/app/scripts/metamask-controller.js b/app/scripts/metamask-controller.js index d520a59ad..02e65bf10 100644 --- a/app/scripts/metamask-controller.js +++ b/app/scripts/metamask-controller.js @@ -194,15 +194,13 @@ module.exports = class MetamaskController { }) .then((rawTx) => { cb(null, rawTx) + this.sendUpdate() + this.txManager.emit(`${txParams.metamaskId}:signingComplete`) }) .catch((err) => { console.error(err) cb(err) }) - .then(() => { - this.sendUpdate() - this.txManager.emit(`${txParams.metamaskId}:signingComplete`) - }) }, // msg signing From e62f70660d922b0bf98e7595939f84821eff814a Mon Sep 17 00:00:00 2001 From: Frankie Date: Thu, 5 Jan 2017 11:06:18 -0800 Subject: [PATCH 18/18] MetaMask Controller - Convert to EventEmitter --- app/scripts/background.js | 9 +++------ app/scripts/metamask-controller.js | 11 +++++------ 2 files changed, 8 insertions(+), 12 deletions(-) diff --git a/app/scripts/background.js b/app/scripts/background.js index ca2efc114..6b7926526 100644 --- a/app/scripts/background.js +++ b/app/scripts/background.js @@ -22,7 +22,6 @@ const controller = new MetamaskController({ setData, loadData, }) -const keyringController = controller.keyringController const txManager = controller.txManager function triggerUi () { if (!popupIsOpen) notification.show() @@ -81,13 +80,11 @@ function setupControllerConnection (stream) { stream.pipe(dnode).pipe(stream) dnode.on('remote', (remote) => { // push updates to popup - controller.ethStore.on('update', controller.sendUpdate.bind(controller)) - controller.listeners.push(remote) - keyringController.on('update', controller.sendUpdate.bind(controller)) - + var sendUpdate = remote.sendUpdate.bind(remote) + controller.on('update', sendUpdate) // teardown on disconnect eos(stream, () => { - controller.ethStore.removeListener('update', controller.sendUpdate.bind(controller)) + controller.removeListener('update', sendUpdate) popupIsOpen = false }) }) diff --git a/app/scripts/metamask-controller.js b/app/scripts/metamask-controller.js index c0d2f3b4c..2e5b02a7b 100644 --- a/app/scripts/metamask-controller.js +++ b/app/scripts/metamask-controller.js @@ -1,3 +1,4 @@ +const EventEmitter = require('events') const extend = require('xtend') const EthStore = require('eth-store') const MetaMaskProvider = require('web3-provider-engine/zero.js') @@ -13,12 +14,12 @@ const autoFaucet = require('./lib/auto-faucet') const nodeify = require('./lib/nodeify') const IdStoreMigrator = require('./lib/idStore-migrator') -module.exports = class MetamaskController { +module.exports = class MetamaskController extends EventEmitter { constructor (opts) { + super() this.state = { network: 'loading' } this.opts = opts - this.listeners = [] this.configManager = new ConfigManager(opts) this.keyringController = new KeyringController({ configManager: this.configManager, @@ -62,6 +63,7 @@ module.exports = class MetamaskController { }) this.ethStore.on('update', this.sendUpdate.bind(this)) + this.keyringController.on('update', this.sendUpdate.bind(this)) } getState () { @@ -165,10 +167,7 @@ module.exports = class MetamaskController { sendUpdate () { this.getState() .then((state) => { - - this.listeners.forEach((remote) => { - remote.sendUpdate(state) - }) + this.emit('update', state) }) }