From dab2fccc78ad76095cc12ce0a1056d9b7a9d6001 Mon Sep 17 00:00:00 2001 From: kumavis Date: Wed, 14 Jun 2017 22:16:14 -0700 Subject: [PATCH 01/80] introduce nonce-tracker --- app/scripts/lib/nonce-tracker.js | 49 ++++++++++++++++++++++++++++++++ 1 file changed, 49 insertions(+) create mode 100644 app/scripts/lib/nonce-tracker.js diff --git a/app/scripts/lib/nonce-tracker.js b/app/scripts/lib/nonce-tracker.js new file mode 100644 index 000000000..6e9d094bc --- /dev/null +++ b/app/scripts/lib/nonce-tracker.js @@ -0,0 +1,49 @@ +const EthQuery = require('ethjs-query') + +class NonceTracker { + + constructor({ blockTracker, provider, getPendingTransactions }) { + this.blockTracker = blockTracker + this.ethQuery = new EthQuery(provider) + this.getPendingTransactions = getPendingTransactions + this.lockMap = {} + } + + // releaseLock must be called + // releaseLock must be called after adding signed tx to pending transactions (or discarding) + async getNonceLock(address) { + // await lock free + await this.lockMap[address] + // take lock + const releaseLock = this._takeLock(address) + // calculate next nonce + const currentBlock = await this._getCurrentBlock() + const blockNumber = currentBlock.number + const pendingTransactions = this.getPendingTransactions(address) + const baseCount = await this.ethQuery.getTransactionCount(address, blockNumber) + const nextNonce = baseCount + pendingTransactions + // return next nonce and release cb + return { nextNonce, releaseLock } + } + + async _getCurrentBlock() { + const currentBlock = this.blockTracker.getCurrentBlock() + if (currentBlock) return currentBlock + return await Promise((reject, resolve) => { + this.blockTracker.once('latest', resolve) + }) + } + + _takeLock(lockId) { + let releaseLock = null + // create and store lock + const lock = new Promise((reject, resolve) => { releaseLock = resolve }) + this.lockMap[lockId] = lock + // setup lock teardown + lock.then(() => delete this.lockMap[lockId]) + return releaseLock + } + +} + +module.exports = NonceTracker From b3492d9c17e62332c17bb082c23db30512e2b881 Mon Sep 17 00:00:00 2001 From: kumavis Date: Wed, 14 Jun 2017 23:44:02 -0700 Subject: [PATCH 02/80] transaction controller - use nonce-tracker --- app/scripts/controllers/transactions.js | 90 ++++++++++++++++--------- app/scripts/metamask-controller.js | 2 +- test/unit/tx-controller-test.js | 7 +- 3 files changed, 63 insertions(+), 36 deletions(-) diff --git a/app/scripts/controllers/transactions.js b/app/scripts/controllers/transactions.js index 2db8041eb..e7fe9927e 100644 --- a/app/scripts/controllers/transactions.js +++ b/app/scripts/controllers/transactions.js @@ -4,9 +4,10 @@ const extend = require('xtend') const Semaphore = require('semaphore') const ObservableStore = require('obs-store') const ethUtil = require('ethereumjs-util') +const denodeify = require('denodeify') const TxProviderUtil = require('../lib/tx-utils') const createId = require('../lib/random-id') -const denodeify = require('denodeify') +const NonceTracker = require('../lib/nonce-tracker') const RETRY_LIMIT = 200 @@ -22,6 +23,11 @@ module.exports = class TransactionController extends EventEmitter { this.txHistoryLimit = opts.txHistoryLimit this.provider = opts.provider this.blockTracker = opts.blockTracker + this.nonceTracker = new NonceTracker({ + provider: this.provider, + blockTracker: this.blockTracker, + getPendingTransactions: (address) => this.getFilteredTxList({ from: address, status: 'submitted' }), + }) this.query = opts.ethQuery this.txProviderUtils = new TxProviderUtil(this.query) this.blockTracker.on('block', this.checkForTxInBlock.bind(this)) @@ -169,29 +175,58 @@ module.exports = class TransactionController extends EventEmitter { }, {}) } - approveTransaction (txId, cb = warn) { - const self = this - // approve - self.setTxStatusApproved(txId) - // only allow one tx at a time for atomic nonce usage - self.nonceLock.take(() => { - // begin signature process - async.waterfall([ - (cb) => self.fillInTxParams(txId, cb), - (cb) => self.signTransaction(txId, cb), - (rawTx, cb) => self.publishTransaction(txId, rawTx, cb), - ], (err) => { - self.nonceLock.leave() - if (err) { - this.setTxStatusFailed(txId, { - errCode: err.errCode || err, - message: err.message || 'Transaction failed during approval', - }) - return cb(err) - } - cb() + // approveTransaction (txId, cb = warn) { + // promiseToCallback((async () => { + // // approve + // self.setTxStatusApproved(txId) + // // get next nonce + // const txMeta = this.getTx(txId) + // const fromAddress = txMeta.txParams.from + // const { nextNonce, releaseLock } = await this.nonceTracker.getNonceLock(fromAddress) + // txMeta.txParams.nonce = nonce + // this.updateTx(txMeta) + // // sign transaction + // const rawTx = await denodeify(self.signTransaction.bind(self))(txId) + // await denodeify(self.publishTransaction.bind(self))(txId, rawTx) + // })())((err) => { + // if (err) { + // this.setTxStatusFailed(txId, { + // errCode: err.errCode || err, + // message: err.message || 'Transaction failed during approval', + // }) + // } + // // must set transaction to submitted/failed before releasing lock + // releaseLock() + // cb(err) + // }) + // } + + async approveTransaction (txId) { + let nonceLock + try { + // approve + this.setTxStatusApproved(txId) + // get next nonce + const txMeta = this.getTx(txId) + const fromAddress = txMeta.txParams.from + nonceLock = await this.nonceTracker.getNonceLock(fromAddress) + txMeta.txParams.nonce = nonceLock.nextNonce + this.updateTx(txMeta) + // sign transaction + const rawTx = await denodeify(this.signTransaction.bind(this))(txId) + await denodeify(this.publishTransaction.bind(this))(txId, rawTx) + // must set transaction to submitted/failed before releasing lock + nonceLock.releaseLock() + } catch (err) { + this.setTxStatusFailed(txId, { + errCode: err.errCode || err, + message: err.message || 'Transaction failed during approval', }) - }) + // must set transaction to submitted/failed before releasing lock + if (nonceLock) nonceLock.releaseLock() + // continue with error chain + throw err + } } cancelTransaction (txId, cb = warn) { @@ -199,15 +234,6 @@ module.exports = class TransactionController extends EventEmitter { cb() } - fillInTxParams (txId, cb) { - const txMeta = this.getTx(txId) - this.txProviderUtils.fillInTxParams(txMeta.txParams, (err) => { - if (err) return cb(err) - this.updateTx(txMeta) - cb() - }) - } - getChainId () { const networkState = this.networkStore.getState() const getChainId = parseInt(networkState) diff --git a/app/scripts/metamask-controller.js b/app/scripts/metamask-controller.js index a7eb3d056..006a32eac 100644 --- a/app/scripts/metamask-controller.js +++ b/app/scripts/metamask-controller.js @@ -290,7 +290,7 @@ module.exports = class MetamaskController extends EventEmitter { exportAccount: nodeify(keyringController.exportAccount).bind(keyringController), // txController - approveTransaction: txController.approveTransaction.bind(txController), + approveTransaction: nodeify(txController.approveTransaction).bind(txController), cancelTransaction: txController.cancelTransaction.bind(txController), updateAndApproveTransaction: this.updateAndApproveTx.bind(this), diff --git a/test/unit/tx-controller-test.js b/test/unit/tx-controller-test.js index f0d8a706e..7c8d1761d 100644 --- a/test/unit/tx-controller-test.js +++ b/test/unit/tx-controller-test.js @@ -1,5 +1,4 @@ const assert = require('assert') -const EventEmitter = require('events') const ethUtil = require('ethereumjs-util') const EthTx = require('ethereumjs-tx') const EthQuery = require('eth-query') @@ -19,13 +18,15 @@ describe('Transaction Controller', function () { txController = new TransactionController({ networkStore: new ObservableStore(currentNetworkId), txHistoryLimit: 10, - blockTracker: new EventEmitter(), - ethQuery: new EthQuery(new EventEmitter()), + blockTracker: { getCurrentBlock: noop, on: noop }, + provider: { sendAsync: noop }, + ethQuery: new EthQuery({ sendAsync: noop }), signTransaction: (ethTx) => new Promise((resolve) => { ethTx.sign(privKey) resolve() }), }) + txController.nonceTracker.getNonceLock = () => Promise.resolve({ nextNonce: 0, releaseLock: noop }) }) describe('#validateTxParams', function () { From b67bc7043ee231bb9ed4781aa4ac29d4e3107481 Mon Sep 17 00:00:00 2001 From: frankiebee Date: Thu, 15 Jun 2017 15:25:22 -0700 Subject: [PATCH 03/80] Fix test to call done --- test/unit/tx-controller-test.js | 15 ++++++++------- 1 file changed, 8 insertions(+), 7 deletions(-) diff --git a/test/unit/tx-controller-test.js b/test/unit/tx-controller-test.js index 7c8d1761d..908b060d4 100644 --- a/test/unit/tx-controller-test.js +++ b/test/unit/tx-controller-test.js @@ -270,26 +270,28 @@ describe('Transaction Controller', function () { it('does not overwrite set values', function (done) { + this.timeout(15000) const wrongValue = '0x05' txController.addTx(txMeta) const estimateStub = sinon.stub(txController.txProviderUtils.query, 'estimateGas') - .callsArgWith(1, null, wrongValue) + .callsArgWithAsync(1, null, wrongValue) const priceStub = sinon.stub(txController.txProviderUtils.query, 'gasPrice') - .callsArgWith(0, null, wrongValue) + .callsArgWithAsync(0, null, wrongValue) const nonceStub = sinon.stub(txController.txProviderUtils.query, 'getTransactionCount') - .callsArgWith(2, null, wrongValue) + .callsArgWithAsync(2, null, wrongValue) const signStub = sinon.stub(txController, 'signTransaction') - .callsArgWith(1, null, noop) + .callsArgWithAsync(1, null, noop) const pubStub = sinon.stub(txController.txProviderUtils, 'publishTransaction') - .callsArgWith(1, null, originalValue) + .callsArgWithAsync(1, null, originalValue) + console.log('HERE !!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!') - txController.approveTransaction(txMeta.id, (err) => { + txController.approveTransaction(txMeta.id).then((err) => { assert.ifError(err, 'should not error') const result = txController.getTx(txMeta.id) @@ -305,7 +307,6 @@ describe('Transaction Controller', function () { signStub.restore() nonceStub.restore() pubStub.restore() - done() }) }) From e672f2da0d74bc1e001acb35be0345e49663463e Mon Sep 17 00:00:00 2001 From: frankiebee Date: Fri, 16 Jun 2017 17:04:56 -0700 Subject: [PATCH 04/80] remove irrelevant test --- test/unit/tx-controller-test.js | 5 ----- 1 file changed, 5 deletions(-) diff --git a/test/unit/tx-controller-test.js b/test/unit/tx-controller-test.js index 908b060d4..8ce6a5a65 100644 --- a/test/unit/tx-controller-test.js +++ b/test/unit/tx-controller-test.js @@ -281,15 +281,12 @@ describe('Transaction Controller', function () { const priceStub = sinon.stub(txController.txProviderUtils.query, 'gasPrice') .callsArgWithAsync(0, null, wrongValue) - const nonceStub = sinon.stub(txController.txProviderUtils.query, 'getTransactionCount') - .callsArgWithAsync(2, null, wrongValue) const signStub = sinon.stub(txController, 'signTransaction') .callsArgWithAsync(1, null, noop) const pubStub = sinon.stub(txController.txProviderUtils, 'publishTransaction') .callsArgWithAsync(1, null, originalValue) - console.log('HERE !!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!') txController.approveTransaction(txMeta.id).then((err) => { assert.ifError(err, 'should not error') @@ -299,13 +296,11 @@ describe('Transaction Controller', function () { assert.equal(params.gas, originalValue, 'gas unmodified') assert.equal(params.gasPrice, originalValue, 'gas price unmodified') - assert.equal(params.nonce, originalValue, 'nonce unmodified') assert.equal(result.hash, originalValue, 'hash was set') estimateStub.restore() priceStub.restore() signStub.restore() - nonceStub.restore() pubStub.restore() done() }) From fa8c74fe9b19229580224815cc131611ee29027c Mon Sep 17 00:00:00 2001 From: frankiebee Date: Wed, 21 Jun 2017 17:28:19 -0700 Subject: [PATCH 05/80] add a test for #getNonceLock --- app/scripts/lib/nonce-tracker.js | 22 ++++++++++----- package.json | 1 + test/unit/nonce-tracker-test.js | 46 ++++++++++++++++++++++++++++++++ 3 files changed, 63 insertions(+), 6 deletions(-) create mode 100644 test/unit/nonce-tracker-test.js diff --git a/app/scripts/lib/nonce-tracker.js b/app/scripts/lib/nonce-tracker.js index 6e9d094bc..ff2317a91 100644 --- a/app/scripts/lib/nonce-tracker.js +++ b/app/scripts/lib/nonce-tracker.js @@ -1,4 +1,4 @@ -const EthQuery = require('ethjs-query') +const EthQuery = require('eth-query') class NonceTracker { @@ -20,10 +20,10 @@ class NonceTracker { const currentBlock = await this._getCurrentBlock() const blockNumber = currentBlock.number const pendingTransactions = this.getPendingTransactions(address) - const baseCount = await this.ethQuery.getTransactionCount(address, blockNumber) - const nextNonce = baseCount + pendingTransactions + const baseCount = await this._getTxCount(address, blockNumber) + const nextNonce = parseInt(baseCount) + pendingTransactions.length + 1 // return next nonce and release cb - return { nextNonce, releaseLock } + return { nextNonce: nextNonce.toString(16), releaseLock } } async _getCurrentBlock() { @@ -37,13 +37,23 @@ class NonceTracker { _takeLock(lockId) { let releaseLock = null // create and store lock - const lock = new Promise((reject, resolve) => { releaseLock = resolve }) + const lock = new Promise((resolve, reject) => { releaseLock = resolve }) this.lockMap[lockId] = lock // setup lock teardown - lock.then(() => delete this.lockMap[lockId]) + lock.then(() => { + delete this.lockMap[lockId] + }) return releaseLock } + _getTxCount (address, blockNumber) { + return new Promise((resolve, reject) => { + this.ethQuery.getTransactionCount(address, blockNumber, (err, result) => { + err ? reject(err) : resolve(result) + }) + }) + } + } module.exports = NonceTracker diff --git a/package.json b/package.json index 9ed2e7ae0..9085e4565 100644 --- a/package.json +++ b/package.json @@ -10,6 +10,7 @@ "dist": "npm install && gulp dist --disableLiveReload", "test": "npm run lint && npm run test-unit && npm run test-integration", "test-unit": "METAMASK_ENV=test mocha --require test/helper.js --recursive \"test/unit/**/*.js\"", + "single-test": "METAMASK_ENV=test mocha --require test/helper.js", "test-integration": "npm run buildMock && npm run buildCiUnits && testem ci -P 2", "lint": "gulp lint", "buildCiUnits": "node test/integration/index.js", diff --git a/test/unit/nonce-tracker-test.js b/test/unit/nonce-tracker-test.js new file mode 100644 index 000000000..5a05d6170 --- /dev/null +++ b/test/unit/nonce-tracker-test.js @@ -0,0 +1,46 @@ +const assert = require('assert') +const NonceTracker = require('../../app/scripts/lib/nonce-tracker') + +describe('Nonce Tracker', function () { + let nonceTracker, provider, getPendingTransactions, pendingTxs + const noop = () => {} + + + beforeEach(function () { + pendingTxs =[{ + 'status': 'submitted', + 'txParams': { + 'from': '0x7d3517b0d011698406d6e0aed8453f0be2697926', + 'gas': '0x30d40', + 'value': '0x0', + 'nonce': '0x1', + }, + }] + + + getPendingTransactions = () => pendingTxs + provider = { sendAsync: (_, cb) => { cb(undefined , {result: '0x0'}) }, } + nonceTracker = new NonceTracker({ + blockTracker: { + getCurrentBlock: () => '0x11b568', + once: (...args) => { + setTimeout(() => { + args.pop()() + }, 5000) + } + }, + provider, + getPendingTransactions, + }) + }) + + describe('#getNonceLock', function () { + it('should work', async function (done) { + this.timeout(15000) + const nonceLock = await nonceTracker.getNonceLock('0x7d3517b0d011698406d6e0aed8453f0be2697926') + assert.equal(nonceLock.nextNonce, '2', 'nonce should be 2') + nonceLock.releaseLock() + done() + }) + }) +}) From 92df9965ebd4a833817c32fd32f7e4533ec7fe19 Mon Sep 17 00:00:00 2001 From: frankiebee Date: Wed, 21 Jun 2017 19:51:00 -0700 Subject: [PATCH 06/80] fix nonceTracker --- app/scripts/controllers/transactions.js | 51 +++++++------------------ app/scripts/lib/nonce-tracker.js | 14 +++---- test/unit/nonce-tracker-test.js | 18 +++------ 3 files changed, 25 insertions(+), 58 deletions(-) diff --git a/app/scripts/controllers/transactions.js b/app/scripts/controllers/transactions.js index c2f98e66a..59c8be8b7 100644 --- a/app/scripts/controllers/transactions.js +++ b/app/scripts/controllers/transactions.js @@ -25,7 +25,7 @@ module.exports = class TransactionController extends EventEmitter { this.blockTracker = opts.blockTracker this.nonceTracker = new NonceTracker({ provider: this.provider, - blockTracker: this.blockTracker, + blockTracker: this.provider._blockTracker, getPendingTransactions: (address) => this.getFilteredTxList({ from: address, status: 'submitted' }), }) this.query = opts.ethQuery @@ -176,33 +176,7 @@ module.exports = class TransactionController extends EventEmitter { }, {}) } - // approveTransaction (txId, cb = warn) { - // promiseToCallback((async () => { - // // approve - // self.setTxStatusApproved(txId) - // // get next nonce - // const txMeta = this.getTx(txId) - // const fromAddress = txMeta.txParams.from - // const { nextNonce, releaseLock } = await this.nonceTracker.getNonceLock(fromAddress) - // txMeta.txParams.nonce = nonce - // this.updateTx(txMeta) - // // sign transaction - // const rawTx = await denodeify(self.signTransaction.bind(self))(txId) - // await denodeify(self.publishTransaction.bind(self))(txId, rawTx) - // })())((err) => { - // if (err) { - // this.setTxStatusFailed(txId, { - // errCode: err.errCode || err, - // message: err.message || 'Transaction failed during approval', - // }) - // } - // // must set transaction to submitted/failed before releasing lock - // releaseLock() - // cb(err) - // }) - // } - - async approveTransaction (txId) { + async approveTransaction (txId, cb = warn) { let nonceLock try { // approve @@ -215,9 +189,10 @@ module.exports = class TransactionController extends EventEmitter { this.updateTx(txMeta) // sign transaction const rawTx = await denodeify(this.signTransaction.bind(this))(txId) - await denodeify(this.publishTransaction.bind(this))(txId, rawTx) + await this.publishTransaction(txId, rawTx) // must set transaction to submitted/failed before releasing lock nonceLock.releaseLock() + cb() } catch (err) { this.setTxStatusFailed(txId, { errCode: err.errCode || err, @@ -226,7 +201,7 @@ module.exports = class TransactionController extends EventEmitter { // must set transaction to submitted/failed before releasing lock if (nonceLock) nonceLock.releaseLock() // continue with error chain - throw err + cb(err) } } @@ -260,16 +235,17 @@ module.exports = class TransactionController extends EventEmitter { }) } - publishTransaction (txId, rawTx, cb = warn) { + publishTransaction (txId, rawTx) { const txMeta = this.getTx(txId) txMeta.rawTx = rawTx this.updateTx(txMeta) - - this.txProviderUtils.publishTransaction(rawTx, (err, txHash) => { - if (err) return cb(err) - this.setTxHash(txId, txHash) - this.setTxStatusSubmitted(txId) - cb() + return new Promise((resolve, reject) => { + this.txProviderUtils.publishTransaction(rawTx, (err, txHash) => { + if (err) reject(err) + this.setTxHash(txId, txHash) + this.setTxStatusSubmitted(txId) + resolve() + }) }) } @@ -414,7 +390,6 @@ module.exports = class TransactionController extends EventEmitter { this.emit(`${txMeta.id}:${status}`, txId) if (status === 'submitted' || status === 'rejected') { this.emit(`${txMeta.id}:finished`, txMeta) - } this.updateTx(txMeta) this.emit('updateBadge') diff --git a/app/scripts/lib/nonce-tracker.js b/app/scripts/lib/nonce-tracker.js index ff2317a91..4ea511dec 100644 --- a/app/scripts/lib/nonce-tracker.js +++ b/app/scripts/lib/nonce-tracker.js @@ -2,7 +2,7 @@ const EthQuery = require('eth-query') class NonceTracker { - constructor({ blockTracker, provider, getPendingTransactions }) { + constructor ({ blockTracker, provider, getPendingTransactions }) { this.blockTracker = blockTracker this.ethQuery = new EthQuery(provider) this.getPendingTransactions = getPendingTransactions @@ -11,7 +11,7 @@ class NonceTracker { // releaseLock must be called // releaseLock must be called after adding signed tx to pending transactions (or discarding) - async getNonceLock(address) { + async getNonceLock (address) { // await lock free await this.lockMap[address] // take lock @@ -21,12 +21,12 @@ class NonceTracker { const blockNumber = currentBlock.number const pendingTransactions = this.getPendingTransactions(address) const baseCount = await this._getTxCount(address, blockNumber) - const nextNonce = parseInt(baseCount) + pendingTransactions.length + 1 + const nextNonce = parseInt(baseCount) + pendingTransactions.length // return next nonce and release cb return { nextNonce: nextNonce.toString(16), releaseLock } } - async _getCurrentBlock() { + async _getCurrentBlock () { const currentBlock = this.blockTracker.getCurrentBlock() if (currentBlock) return currentBlock return await Promise((reject, resolve) => { @@ -34,15 +34,13 @@ class NonceTracker { }) } - _takeLock(lockId) { + _takeLock (lockId) { let releaseLock = null // create and store lock const lock = new Promise((resolve, reject) => { releaseLock = resolve }) this.lockMap[lockId] = lock // setup lock teardown - lock.then(() => { - delete this.lockMap[lockId] - }) + lock.then(() => delete this.lockMap[lockId]) return releaseLock } diff --git a/test/unit/nonce-tracker-test.js b/test/unit/nonce-tracker-test.js index 5a05d6170..16cd6d008 100644 --- a/test/unit/nonce-tracker-test.js +++ b/test/unit/nonce-tracker-test.js @@ -3,31 +3,25 @@ const NonceTracker = require('../../app/scripts/lib/nonce-tracker') describe('Nonce Tracker', function () { let nonceTracker, provider, getPendingTransactions, pendingTxs - const noop = () => {} beforeEach(function () { - pendingTxs =[{ + pendingTxs = [{ 'status': 'submitted', 'txParams': { 'from': '0x7d3517b0d011698406d6e0aed8453f0be2697926', 'gas': '0x30d40', 'value': '0x0', - 'nonce': '0x1', + 'nonce': '0x0', }, }] getPendingTransactions = () => pendingTxs - provider = { sendAsync: (_, cb) => { cb(undefined , {result: '0x0'}) }, } + provider = { sendAsync: (_, cb) => { cb(undefined, {result: '0x0'}) } } nonceTracker = new NonceTracker({ blockTracker: { - getCurrentBlock: () => '0x11b568', - once: (...args) => { - setTimeout(() => { - args.pop()() - }, 5000) - } + getCurrentBlock: () => '0x11b568', }, provider, getPendingTransactions, @@ -38,8 +32,8 @@ describe('Nonce Tracker', function () { it('should work', async function (done) { this.timeout(15000) const nonceLock = await nonceTracker.getNonceLock('0x7d3517b0d011698406d6e0aed8453f0be2697926') - assert.equal(nonceLock.nextNonce, '2', 'nonce should be 2') - nonceLock.releaseLock() + assert.equal(nonceLock.nextNonce, '1', 'nonce should be 1') + await nonceLock.releaseLock() done() }) }) From 0ee4502d716ebe28fa426a05c454a75c7f82d965 Mon Sep 17 00:00:00 2001 From: frankiebee Date: Tue, 27 Jun 2017 15:26:04 -0700 Subject: [PATCH 07/80] calculate nonce based on local pending txs w/o error state. --- app/scripts/controllers/transactions.js | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-) diff --git a/app/scripts/controllers/transactions.js b/app/scripts/controllers/transactions.js index 651565519..460280578 100644 --- a/app/scripts/controllers/transactions.js +++ b/app/scripts/controllers/transactions.js @@ -26,7 +26,7 @@ module.exports = class TransactionController extends EventEmitter { this.nonceTracker = new NonceTracker({ provider: this.provider, blockTracker: this.provider._blockTracker, - getPendingTransactions: (address) => this.getFilteredTxList({ from: address, status: 'submitted' }), + getPendingTransactions: (address) => this.getFilteredTxList({ from: address, status: 'submitted', err: undefined }), }) this.query = opts.ethQuery this.txProviderUtils = new TxProviderUtil(this.query) @@ -263,10 +263,19 @@ module.exports = class TransactionController extends EventEmitter { to: '0x0..', from: '0x0..', status: 'signed', + err: undefined, } and returns a list of tx with all options matching + ****************HINT**************** + | `err: undefined` is like looking | + | for a tx with no err | + | so you can also search txs that | + | dont have something as well by | + | setting the value as undefined | + ************************************ + this is for things like filtering a the tx list for only tx's from 1 account or for filltering for all txs from one account From 690685d20de310b4c4589e92a5053afd9c56e85a Mon Sep 17 00:00:00 2001 From: frankiebee Date: Tue, 27 Jun 2017 16:46:14 -0700 Subject: [PATCH 08/80] nonce-tracker: only check transactions that are not supposed to be ignored --- app/scripts/controllers/transactions.js | 9 ++++++++- app/scripts/lib/nonce-tracker.js | 13 +++++++------ 2 files changed, 15 insertions(+), 7 deletions(-) diff --git a/app/scripts/controllers/transactions.js b/app/scripts/controllers/transactions.js index 460280578..c74427cd5 100644 --- a/app/scripts/controllers/transactions.js +++ b/app/scripts/controllers/transactions.js @@ -26,7 +26,14 @@ module.exports = class TransactionController extends EventEmitter { this.nonceTracker = new NonceTracker({ provider: this.provider, blockTracker: this.provider._blockTracker, - getPendingTransactions: (address) => this.getFilteredTxList({ from: address, status: 'submitted', err: undefined }), + getPendingTransactions: (address) => { + return this.getFilteredTxList({ + from: address, + status: 'submitted', + err: undefined, + ignore: undefined, + }) + }, }) this.query = opts.ethQuery this.txProviderUtils = new TxProviderUtil(this.query) diff --git a/app/scripts/lib/nonce-tracker.js b/app/scripts/lib/nonce-tracker.js index 4ea511dec..9ef7706f9 100644 --- a/app/scripts/lib/nonce-tracker.js +++ b/app/scripts/lib/nonce-tracker.js @@ -12,15 +12,14 @@ class NonceTracker { // releaseLock must be called // releaseLock must be called after adding signed tx to pending transactions (or discarding) async getNonceLock (address) { + const pendingTransactions = this.getPendingTransactions(address) // await lock free - await this.lockMap[address] + if (pendingTransactions.length) await this.lockMap[address] + else if (this.lockMap[address]) await this.lockMap[address]() // take lock const releaseLock = this._takeLock(address) // calculate next nonce - const currentBlock = await this._getCurrentBlock() - const blockNumber = currentBlock.number - const pendingTransactions = this.getPendingTransactions(address) - const baseCount = await this._getTxCount(address, blockNumber) + const baseCount = await this._getTxCount(address) const nextNonce = parseInt(baseCount) + pendingTransactions.length // return next nonce and release cb return { nextNonce: nextNonce.toString(16), releaseLock } @@ -44,7 +43,9 @@ class NonceTracker { return releaseLock } - _getTxCount (address, blockNumber) { + async _getTxCount (address) { + const currentBlock = await this._getCurrentBlock() + const blockNumber = currentBlock.number return new Promise((resolve, reject) => { this.ethQuery.getTransactionCount(address, blockNumber, (err, result) => { err ? reject(err) : resolve(result) From 6d2cddaac9348b0e9c8a6cc4a6621927765e7c17 Mon Sep 17 00:00:00 2001 From: frankiebee Date: Wed, 5 Jul 2017 12:00:42 -0700 Subject: [PATCH 09/80] fix nonce calculation order --- app/scripts/lib/nonce-tracker.js | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-) diff --git a/app/scripts/lib/nonce-tracker.js b/app/scripts/lib/nonce-tracker.js index 9ef7706f9..ab2893b10 100644 --- a/app/scripts/lib/nonce-tracker.js +++ b/app/scripts/lib/nonce-tracker.js @@ -12,14 +12,16 @@ class NonceTracker { // releaseLock must be called // releaseLock must be called after adding signed tx to pending transactions (or discarding) async getNonceLock (address) { - const pendingTransactions = this.getPendingTransactions(address) // await lock free - if (pendingTransactions.length) await this.lockMap[address] - else if (this.lockMap[address]) await this.lockMap[address]() + await this.lockMap[address] // take lock const releaseLock = this._takeLock(address) // calculate next nonce - const baseCount = await this._getTxCount(address) + // we need to make sure our base count + // and pending count are from the same block + const currentBlock = await this._getCurrentBlock() + const pendingTransactions = this.getPendingTransactions(address) + const baseCount = await this._getTxCount(address, currentBlock) const nextNonce = parseInt(baseCount) + pendingTransactions.length // return next nonce and release cb return { nextNonce: nextNonce.toString(16), releaseLock } @@ -43,8 +45,7 @@ class NonceTracker { return releaseLock } - async _getTxCount (address) { - const currentBlock = await this._getCurrentBlock() + async _getTxCount (address, currentBlock) { const blockNumber = currentBlock.number return new Promise((resolve, reject) => { this.ethQuery.getTransactionCount(address, blockNumber, (err, result) => { From 51ff6d74e884b599f78b5454b33f2bc1a046f0b2 Mon Sep 17 00:00:00 2001 From: frankiebee Date: Wed, 5 Jul 2017 12:07:34 -0700 Subject: [PATCH 10/80] clean up unused code from old noncelock --- app/scripts/controllers/transactions.js | 2 -- 1 file changed, 2 deletions(-) diff --git a/app/scripts/controllers/transactions.js b/app/scripts/controllers/transactions.js index c74427cd5..2b40f9456 100644 --- a/app/scripts/controllers/transactions.js +++ b/app/scripts/controllers/transactions.js @@ -1,7 +1,6 @@ const EventEmitter = require('events') const async = require('async') const extend = require('xtend') -const Semaphore = require('semaphore') const ObservableStore = require('obs-store') const ethUtil = require('ethereumjs-util') const denodeify = require('denodeify') @@ -41,7 +40,6 @@ module.exports = class TransactionController extends EventEmitter { this.blockTracker.on('latest', this.resubmitPendingTxs.bind(this)) this.blockTracker.on('sync', this.queryPendingTxs.bind(this)) this.signEthTx = opts.signTransaction - this.nonceLock = Semaphore(1) this.ethStore = opts.ethStore // memstore is computed from a few different stores this._updateMemstore() From 5c1ccc657c171e65a8cf8e2bc10fcb6267c44d4a Mon Sep 17 00:00:00 2001 From: tmashuang Date: Wed, 5 Jul 2017 13:42:15 -0700 Subject: [PATCH 11/80] Fix spelling --- ui/app/add-token.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/ui/app/add-token.js b/ui/app/add-token.js index b303b5c0d..15ef7a852 100644 --- a/ui/app/add-token.js +++ b/ui/app/add-token.js @@ -86,7 +86,7 @@ AddTokenScreen.prototype.render = function () { h('div', [ h('span', { style: { fontWeight: 'bold', paddingRight: '10px'}, - }, 'Token Sybmol'), + }, 'Token Symbol'), ]), h('div', { style: {display: 'flex'} }, [ From a915dfdeaa51c80c599b15f4e1ec14c90ac00fbf Mon Sep 17 00:00:00 2001 From: Dan Finlay Date: Wed, 5 Jul 2017 22:36:52 -0700 Subject: [PATCH 12/80] Add failing test for retrying an over-spending tx --- test/unit/tx-controller-test.js | 37 +++++++++++++++++++++++++++++++++ 1 file changed, 37 insertions(+) diff --git a/test/unit/tx-controller-test.js b/test/unit/tx-controller-test.js index 0d35cd62c..b5df7f970 100644 --- a/test/unit/tx-controller-test.js +++ b/test/unit/tx-controller-test.js @@ -322,4 +322,41 @@ describe('Transaction Controller', function () { }) }) }) + + describe('#_resubmitTx with a too-low balance', function () { + const from = '0xda0da0' + const txMeta = { + id: 1, + status: 'submitted' + txParams: { + from, + nonce: '0x1' + }, + } + + const lowBalance = '0x0' + const fakeStoreState = {} + fakeStoreState[from] = { + balance: lowBalance, + nonce: '0x0', + } + + // Stubbing out current account state: + const getStateStub = sinon.stub(txController.ethStore, 'getState') + .returns(fakeStoreState) + + // Adding the fake tx: + txController.addTx(txMeta, noop) + + it('should fail the transaction', function (done) { + txController._resubmitTx(txMeta, function (err) { + assert.ifError('should not throw an error') + const updatedMeta = txController.getTx(txMeta.id) + assert.notEqual(updatedMeta.status, txMeta.status, 'status changed.') + assert.notEqual(updatedMeta.status, 'failed', 'tx set to failed.') + }) + }) + }) + }) + From 3abceac55d16e41b37116a8dda565644ed0a9f52 Mon Sep 17 00:00:00 2001 From: Dan Finlay Date: Wed, 5 Jul 2017 22:06:39 -0700 Subject: [PATCH 13/80] Fail pending txs with low balance or invalid nonce --- app/scripts/controllers/transactions.js | 26 +++++++++++++++++++++---- 1 file changed, 22 insertions(+), 4 deletions(-) diff --git a/app/scripts/controllers/transactions.js b/app/scripts/controllers/transactions.js index 52251d66e..3f5834756 100644 --- a/app/scripts/controllers/transactions.js +++ b/app/scripts/controllers/transactions.js @@ -428,10 +428,28 @@ module.exports = class TransactionController extends EventEmitter { const gtBalance = Number.parseInt(txMeta.txParams.value) > Number.parseInt(balance) if (!('retryCount' in txMeta)) txMeta.retryCount = 0 - // if the value of the transaction is greater then the balance - // or the nonce of the transaction is lower then the accounts nonce - // dont resubmit the tx - if (gtBalance || txNonce < nonce) return cb() + // if the value of the transaction is greater then the balance, fail. + if (gtBalance) { + txMeta.err = { + isWarning: true, + message: 'Insufficient balance.', + } + this.updateTx(txMeta) + cb() + return log.error(txMeta.err.message) + } + + // if the nonce of the transaction is lower then the accounts nonce, fail. + if (txNonce < nonce) { + txMeta.err = { + isWarning: true, + message: 'Invalid nonce.', + } + this.updateTx(txMeta) + cb() + return log.error(txMeta.err.message) + } + // Only auto-submit already-signed txs: if (!('rawTx' in txMeta)) return cb() From ef1282b55648ad5e787b170cc06e5f8b292f5983 Mon Sep 17 00:00:00 2001 From: Dan Finlay Date: Wed, 5 Jul 2017 22:48:11 -0700 Subject: [PATCH 14/80] Typo fix --- test/unit/tx-controller-test.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/unit/tx-controller-test.js b/test/unit/tx-controller-test.js index b5df7f970..074e6c954 100644 --- a/test/unit/tx-controller-test.js +++ b/test/unit/tx-controller-test.js @@ -327,7 +327,7 @@ describe('Transaction Controller', function () { const from = '0xda0da0' const txMeta = { id: 1, - status: 'submitted' + status: 'submitted', txParams: { from, nonce: '0x1' From 96df7ad8d36b68e521e670d28e3efda38e41972f Mon Sep 17 00:00:00 2001 From: Dan Finlay Date: Wed, 5 Jul 2017 23:04:51 -0700 Subject: [PATCH 15/80] Add missing done --- test/unit/tx-controller-test.js | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/test/unit/tx-controller-test.js b/test/unit/tx-controller-test.js index 074e6c954..7b0ad66bd 100644 --- a/test/unit/tx-controller-test.js +++ b/test/unit/tx-controller-test.js @@ -342,6 +342,7 @@ describe('Transaction Controller', function () { } // Stubbing out current account state: + txController.ethStore = { getState: noop } const getStateStub = sinon.stub(txController.ethStore, 'getState') .returns(fakeStoreState) @@ -354,9 +355,9 @@ describe('Transaction Controller', function () { const updatedMeta = txController.getTx(txMeta.id) assert.notEqual(updatedMeta.status, txMeta.status, 'status changed.') assert.notEqual(updatedMeta.status, 'failed', 'tx set to failed.') + done() }) }) }) - }) From 07d4e4fe6f31d99a9f15c3862671c5c07831ff2a Mon Sep 17 00:00:00 2001 From: Dan Finlay Date: Wed, 5 Jul 2017 23:23:57 -0700 Subject: [PATCH 16/80] Fix failing test --- app/scripts/controllers/transactions.js | 18 +++----- test/unit/tx-controller-test.js | 56 +++++++++++++------------ 2 files changed, 35 insertions(+), 39 deletions(-) diff --git a/app/scripts/controllers/transactions.js b/app/scripts/controllers/transactions.js index 3f5834756..7946d10d1 100644 --- a/app/scripts/controllers/transactions.js +++ b/app/scripts/controllers/transactions.js @@ -430,24 +430,18 @@ module.exports = class TransactionController extends EventEmitter { // if the value of the transaction is greater then the balance, fail. if (gtBalance) { - txMeta.err = { - isWarning: true, - message: 'Insufficient balance.', - } - this.updateTx(txMeta) + const message = 'Insufficient balance.' + this.setTxStatusFailed(txMeta.id, message) cb() - return log.error(txMeta.err.message) + return log.error(message) } // if the nonce of the transaction is lower then the accounts nonce, fail. if (txNonce < nonce) { - txMeta.err = { - isWarning: true, - message: 'Invalid nonce.', - } - this.updateTx(txMeta) + const message = 'Invalid nonce.' + this.setTxStatusFailed(txMeta.id, message) cb() - return log.error(txMeta.err.message) + return log.error(message) } // Only auto-submit already-signed txs: diff --git a/test/unit/tx-controller-test.js b/test/unit/tx-controller-test.js index 7b0ad66bd..01a498820 100644 --- a/test/unit/tx-controller-test.js +++ b/test/unit/tx-controller-test.js @@ -19,6 +19,7 @@ describe('Transaction Controller', function () { txController = new TransactionController({ networkStore: new ObservableStore(currentNetworkId), txHistoryLimit: 10, + ethStore: { getState: noop }, provider: { _blockTracker: new EventEmitter()}, blockTracker: new EventEmitter(), ethQuery: new EthQuery(new EventEmitter()), @@ -324,37 +325,38 @@ describe('Transaction Controller', function () { }) describe('#_resubmitTx with a too-low balance', function () { - const from = '0xda0da0' - const txMeta = { - id: 1, - status: 'submitted', - txParams: { - from, - nonce: '0x1' - }, - } - - const lowBalance = '0x0' - const fakeStoreState = {} - fakeStoreState[from] = { - balance: lowBalance, - nonce: '0x0', - } - - // Stubbing out current account state: - txController.ethStore = { getState: noop } - const getStateStub = sinon.stub(txController.ethStore, 'getState') - .returns(fakeStoreState) - - // Adding the fake tx: - txController.addTx(txMeta, noop) - it('should fail the transaction', function (done) { + const from = '0xda0da0' + const txMeta = { + id: 1, + status: 'submitted', + metamaskNetworkId: currentNetworkId, + txParams: { + from, + nonce: '0x1', + value: '0xfffff', + }, + } + + const lowBalance = '0x0' + const fakeStoreState = { accounts: {} } + fakeStoreState.accounts[from] = { + balance: lowBalance, + nonce: '0x0', + } + + // Stubbing out current account state: + const getStateStub = sinon.stub(txController.ethStore, 'getState') + .returns(fakeStoreState) + + // Adding the fake tx: + txController.addTx(clone(txMeta)) + txController._resubmitTx(txMeta, function (err) { - assert.ifError('should not throw an error') + assert.ifError(err, 'should not throw an error') const updatedMeta = txController.getTx(txMeta.id) assert.notEqual(updatedMeta.status, txMeta.status, 'status changed.') - assert.notEqual(updatedMeta.status, 'failed', 'tx set to failed.') + assert.equal(updatedMeta.status, 'failed', 'tx set to failed.') done() }) }) From b87d10ab1dff539c4cb32ab32ccc1069598fb11b Mon Sep 17 00:00:00 2001 From: Dan Finlay Date: Wed, 5 Jul 2017 23:26:58 -0700 Subject: [PATCH 17/80] Bump changelog --- CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index e7934dc77..3a471108c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,6 +4,7 @@ - Re-enable default token list. - Add origin header to dapp-bound requests to allow providers to throttle sites. +- Fix bug that could sometimes resubmit a transaction that had been stalled due to low balance after balance was restored. ## 3.8.2 2017-7-3 From 289fdfb7015d2e09306246b7a6871cdd40063118 Mon Sep 17 00:00:00 2001 From: Dan Finlay Date: Thu, 6 Jul 2017 10:05:51 -0700 Subject: [PATCH 18/80] Version 3.8.3 --- CHANGELOG.md | 2 ++ app/manifest.json | 2 +- 2 files changed, 3 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 3a471108c..3966ea1bb 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,6 +2,8 @@ ## Current Master +## 3.8.3 2017-7-6 + - Re-enable default token list. - Add origin header to dapp-bound requests to allow providers to throttle sites. - Fix bug that could sometimes resubmit a transaction that had been stalled due to low balance after balance was restored. diff --git a/app/manifest.json b/app/manifest.json index 12ff6c2ea..aafc33e66 100644 --- a/app/manifest.json +++ b/app/manifest.json @@ -1,7 +1,7 @@ { "name": "MetaMask", "short_name": "Metamask", - "version": "3.8.2", + "version": "3.8.3", "manifest_version": 2, "author": "https://metamask.io", "description": "Ethereum Browser Extension", From 11b744bb87b4858dd2ef982c7d27e9751d8a09a1 Mon Sep 17 00:00:00 2001 From: frankiebee Date: Thu, 6 Jul 2017 22:30:25 -0700 Subject: [PATCH 19/80] if an error happens during a tx publication set tx status to fail --- app/scripts/controllers/transactions.js | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-) diff --git a/app/scripts/controllers/transactions.js b/app/scripts/controllers/transactions.js index 7946d10d1..8d3445c6f 100644 --- a/app/scripts/controllers/transactions.js +++ b/app/scripts/controllers/transactions.js @@ -240,7 +240,16 @@ module.exports = class TransactionController extends EventEmitter { this.updateTx(txMeta) this.txProviderUtils.publishTransaction(rawTx, (err, txHash) => { - if (err) return cb(err) + if (err) { + const errorMessage = err.message.toLowerCase() + if (errorMessage !== 'replacement transaction underpriced' + && errorMessage !== 'gas price too low to replace' + && !errorMessage.startsWith('known transaction') + ) { + this.setTxStatusFailed(txId) + } + return cb(err) + } this.setTxHash(txId, txHash) this.setTxStatusSubmitted(txId) cb() From 99556684096ed788ef01c909ff4cb4b0e61d3a05 Mon Sep 17 00:00:00 2001 From: frankiebee Date: Thu, 6 Jul 2017 22:34:54 -0700 Subject: [PATCH 20/80] add comment --- app/scripts/controllers/transactions.js | 16 +++++++++++----- 1 file changed, 11 insertions(+), 5 deletions(-) diff --git a/app/scripts/controllers/transactions.js b/app/scripts/controllers/transactions.js index 8d3445c6f..14de786b7 100644 --- a/app/scripts/controllers/transactions.js +++ b/app/scripts/controllers/transactions.js @@ -241,11 +241,17 @@ module.exports = class TransactionController extends EventEmitter { this.txProviderUtils.publishTransaction(rawTx, (err, txHash) => { if (err) { - const errorMessage = err.message.toLowerCase() - if (errorMessage !== 'replacement transaction underpriced' - && errorMessage !== 'gas price too low to replace' - && !errorMessage.startsWith('known transaction') - ) { + const errorMessage = err.message.toLowerCase() + /* + Dont marked as failed if the error is because + it's a "known" transaction + "there is already a transaction with the same sender-nonce + but higher/same gas price" + */ + + if (errorMessage !== 'replacement transaction underpriced' // geth + && errorMessage !== 'gas price too low to replace' // parity + && !errorMessage.startsWith('known transaction')) { // geth this.setTxStatusFailed(txId) } return cb(err) From 8661989f516ae4455117e5158a97b4a6912a1980 Mon Sep 17 00:00:00 2001 From: kumavis Date: Fri, 7 Jul 2017 01:37:45 -0700 Subject: [PATCH 21/80] tx controller - move comments --- app/scripts/controllers/transactions.js | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/app/scripts/controllers/transactions.js b/app/scripts/controllers/transactions.js index 14de786b7..42baaaadc 100644 --- a/app/scripts/controllers/transactions.js +++ b/app/scripts/controllers/transactions.js @@ -249,9 +249,13 @@ module.exports = class TransactionController extends EventEmitter { but higher/same gas price" */ - if (errorMessage !== 'replacement transaction underpriced' // geth - && errorMessage !== 'gas price too low to replace' // parity - && !errorMessage.startsWith('known transaction')) { // geth + // geth + if (errorMessage !== 'replacement transaction underpriced' + // geth + && !errorMessage.startsWith('known transaction') + // parity + && errorMessage !== 'gas price too low to replace' + ) { this.setTxStatusFailed(txId) } return cb(err) From 34e2f6650d0db42b9f820d56a7acf9b72ca14da2 Mon Sep 17 00:00:00 2001 From: kumavis Date: Fri, 7 Jul 2017 01:50:48 -0700 Subject: [PATCH 22/80] tx controller - clean code --- app/scripts/controllers/transactions.js | 27 +++++++++++++------------ 1 file changed, 14 insertions(+), 13 deletions(-) diff --git a/app/scripts/controllers/transactions.js b/app/scripts/controllers/transactions.js index 42baaaadc..b855f910c 100644 --- a/app/scripts/controllers/transactions.js +++ b/app/scripts/controllers/transactions.js @@ -241,23 +241,24 @@ module.exports = class TransactionController extends EventEmitter { this.txProviderUtils.publishTransaction(rawTx, (err, txHash) => { if (err) { - const errorMessage = err.message.toLowerCase() /* - Dont marked as failed if the error is because - it's a "known" transaction + Dont marked as failed if the error is a "known" transaction warning "there is already a transaction with the same sender-nonce but higher/same gas price" */ - - // geth - if (errorMessage !== 'replacement transaction underpriced' - // geth - && !errorMessage.startsWith('known transaction') - // parity - && errorMessage !== 'gas price too low to replace' - ) { - this.setTxStatusFailed(txId) - } + const errorMessage = err.message.toLowerCase() + const isKnownTx = ( + // geth + errorMessage === 'replacement transaction underpriced' + || errorMessage.startsWith('known transaction') + // parity + || errorMessage === 'gas price too low to replace' + ) + // ignore resubmit warnings, return early + if (isKnownTx) return cb() + + // encountered unknown error, set status to failed + this.setTxStatusFailed(txId, err.message) return cb(err) } this.setTxHash(txId, txHash) From 092a9c9defd4d9bd2db7f969a8076c8b624d30bb Mon Sep 17 00:00:00 2001 From: frankiebee Date: Fri, 7 Jul 2017 03:05:39 -0700 Subject: [PATCH 23/80] fail transactions that fail in resubmit --- app/scripts/controllers/transactions.js | 47 ++++++++++++------------- 1 file changed, 22 insertions(+), 25 deletions(-) diff --git a/app/scripts/controllers/transactions.js b/app/scripts/controllers/transactions.js index b855f910c..41d70194e 100644 --- a/app/scripts/controllers/transactions.js +++ b/app/scripts/controllers/transactions.js @@ -23,7 +23,10 @@ module.exports = class TransactionController extends EventEmitter { this.query = opts.ethQuery this.txProviderUtils = new TxProviderUtil(this.query) this.blockTracker.on('rawBlock', this.checkForTxInBlock.bind(this)) - this.blockTracker.on('latest', this.resubmitPendingTxs.bind(this)) + // this is a little messy but until ethstore has been either + // removed or redone this is to guard against the race condition + // where ethStore hasent been populated by the results yet + this.blockTracker.once('latest', () => this.blockTracker.on('latest', this.resubmitPendingTxs.bind(this))) this.blockTracker.on('sync', this.queryPendingTxs.bind(this)) this.signEthTx = opts.signTransaction this.nonceLock = Semaphore(1) @@ -240,27 +243,7 @@ module.exports = class TransactionController extends EventEmitter { this.updateTx(txMeta) this.txProviderUtils.publishTransaction(rawTx, (err, txHash) => { - if (err) { - /* - Dont marked as failed if the error is a "known" transaction warning - "there is already a transaction with the same sender-nonce - but higher/same gas price" - */ - const errorMessage = err.message.toLowerCase() - const isKnownTx = ( - // geth - errorMessage === 'replacement transaction underpriced' - || errorMessage.startsWith('known transaction') - // parity - || errorMessage === 'gas price too low to replace' - ) - // ignore resubmit warnings, return early - if (isKnownTx) return cb() - - // encountered unknown error, set status to failed - this.setTxStatusFailed(txId, err.message) - return cb(err) - } + if (err) return cb(err) this.setTxHash(txId, txHash) this.setTxStatusSubmitted(txId) cb() @@ -434,10 +417,24 @@ module.exports = class TransactionController extends EventEmitter { // only try resubmitting if their are transactions to resubmit if (!pending.length) return const resubmit = denodeify(this._resubmitTx.bind(this)) - Promise.all(pending.map(txMeta => resubmit(txMeta))) + pending.forEach((txMeta) => resubmit(txMeta) .catch((reason) => { - log.info('Problem resubmitting tx', reason) - }) + /* + Dont marked as failed if the error is a "known" transaction warning + "there is already a transaction with the same sender-nonce + but higher/same gas price" + */ + const errorMessage = reason.message.toLowerCase() + const isKnownTx = ( + // geth + errorMessage === 'replacement transaction underpriced' + || errorMessage.startsWith('known transaction') + // parity + || errorMessage === 'gas price too low to replace' + ) + // ignore resubmit warnings, return early + if (!isKnownTx) this.setTxStatusFailed(txMeta.id, reason.message) + })) } _resubmitTx (txMeta, cb) { From 04a0b949a2ce5b0335625236d9cac1dc7153dbf1 Mon Sep 17 00:00:00 2001 From: Dan Finlay Date: Fri, 7 Jul 2017 11:24:33 -0700 Subject: [PATCH 24/80] Version 3.8.4 --- CHANGELOG.md | 4 ++++ app/manifest.json | 2 +- 2 files changed, 5 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 3966ea1bb..696d68345 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,6 +2,10 @@ ## Current Master +## 3.8.4 2017-7-7 + +- Improve transaction resubmit logic to fail more eagerly when a user would expect it to. + ## 3.8.3 2017-7-6 - Re-enable default token list. diff --git a/app/manifest.json b/app/manifest.json index aafc33e66..d386e43aa 100644 --- a/app/manifest.json +++ b/app/manifest.json @@ -1,7 +1,7 @@ { "name": "MetaMask", "short_name": "Metamask", - "version": "3.8.3", + "version": "3.8.4", "manifest_version": 2, "author": "https://metamask.io", "description": "Ethereum Browser Extension", From ab8bae421e6b172f811694a597536c7d222043cd Mon Sep 17 00:00:00 2001 From: kumavis Date: Fri, 7 Jul 2017 14:26:52 -0700 Subject: [PATCH 25/80] test - tx-controller - stub block-tracker method --- test/unit/tx-controller-test.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/unit/tx-controller-test.js b/test/unit/tx-controller-test.js index c9eff5d01..9dfe0b982 100644 --- a/test/unit/tx-controller-test.js +++ b/test/unit/tx-controller-test.js @@ -18,7 +18,7 @@ describe('Transaction Controller', function () { txController = new TransactionController({ networkStore: new ObservableStore(currentNetworkId), txHistoryLimit: 10, - blockTracker: { getCurrentBlock: noop, on: noop }, + blockTracker: { getCurrentBlock: noop, on: noop, once: noop }, provider: { sendAsync: noop }, ethQuery: new EthQuery({ sendAsync: noop }), ethStore: { getState: noop }, From f5de16c91174fbbf208e5aef8f542d3bbbb3cb93 Mon Sep 17 00:00:00 2001 From: kumavis Date: Fri, 7 Jul 2017 14:32:03 -0700 Subject: [PATCH 26/80] test - tx controller - fix promise handling --- test/unit/tx-controller-test.js | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-) diff --git a/test/unit/tx-controller-test.js b/test/unit/tx-controller-test.js index 9dfe0b982..a5af13915 100644 --- a/test/unit/tx-controller-test.js +++ b/test/unit/tx-controller-test.js @@ -270,7 +270,7 @@ describe('Transaction Controller', function () { }) - it('does not overwrite set values', function (done) { + it('does not overwrite set values', function () { this.timeout(15000) const wrongValue = '0x05' @@ -289,9 +289,7 @@ describe('Transaction Controller', function () { const pubStub = sinon.stub(txController.txProviderUtils, 'publishTransaction') .callsArgWithAsync(1, null, originalValue) - txController.approveTransaction(txMeta.id).then((err) => { - assert.ifError(err, 'should not error') - + return txController.approveTransaction(txMeta.id).then(() => { const result = txController.getTx(txMeta.id) const params = result.txParams @@ -303,7 +301,6 @@ describe('Transaction Controller', function () { priceStub.restore() signStub.restore() pubStub.restore() - done() }) }) }) From 4fa999e4deae5451e73c126a80e541db6e3d0dc3 Mon Sep 17 00:00:00 2001 From: kumavis Date: Fri, 7 Jul 2017 19:02:34 -0700 Subject: [PATCH 27/80] tx controller - resubmit - recognize parity known hash message --- app/scripts/controllers/transactions.js | 1 + 1 file changed, 1 insertion(+) diff --git a/app/scripts/controllers/transactions.js b/app/scripts/controllers/transactions.js index 41d70194e..bcce1bf8f 100644 --- a/app/scripts/controllers/transactions.js +++ b/app/scripts/controllers/transactions.js @@ -431,6 +431,7 @@ module.exports = class TransactionController extends EventEmitter { || errorMessage.startsWith('known transaction') // parity || errorMessage === 'gas price too low to replace' + || errorMessage === 'transaction with the same hash was already imported.' ) // ignore resubmit warnings, return early if (!isKnownTx) this.setTxStatusFailed(txMeta.id, reason.message) From c53aac398a29ff7cfe0efa6c844653693d78157b Mon Sep 17 00:00:00 2001 From: kumavis Date: Fri, 7 Jul 2017 19:09:32 -0700 Subject: [PATCH 28/80] tx controller - correctly set error message on resubmit error --- app/scripts/controllers/transactions.js | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/app/scripts/controllers/transactions.js b/app/scripts/controllers/transactions.js index bcce1bf8f..e1eaba232 100644 --- a/app/scripts/controllers/transactions.js +++ b/app/scripts/controllers/transactions.js @@ -417,14 +417,13 @@ module.exports = class TransactionController extends EventEmitter { // only try resubmitting if their are transactions to resubmit if (!pending.length) return const resubmit = denodeify(this._resubmitTx.bind(this)) - pending.forEach((txMeta) => resubmit(txMeta) - .catch((reason) => { + pending.forEach((txMeta) => resubmit(txMeta).catch((err) => { /* Dont marked as failed if the error is a "known" transaction warning "there is already a transaction with the same sender-nonce but higher/same gas price" */ - const errorMessage = reason.message.toLowerCase() + const errorMessage = err.message.toLowerCase() const isKnownTx = ( // geth errorMessage === 'replacement transaction underpriced' @@ -434,7 +433,12 @@ module.exports = class TransactionController extends EventEmitter { || errorMessage === 'transaction with the same hash was already imported.' ) // ignore resubmit warnings, return early - if (!isKnownTx) this.setTxStatusFailed(txMeta.id, reason.message) + if (isKnownTx) return + // encountered real error - transition to error state + this.setTxStatusFailed(txMeta.id, { + errCode: err.errCode || err, + message: err.message, + }) })) } From c425ad4ec71c6a5d5cc7af3d2a4d42c56c3ca125 Mon Sep 17 00:00:00 2001 From: kumavis Date: Fri, 7 Jul 2017 19:13:06 -0700 Subject: [PATCH 29/80] tx controller - resubmit - correctly set error on bad nonce/balance --- app/scripts/controllers/transactions.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/app/scripts/controllers/transactions.js b/app/scripts/controllers/transactions.js index e1eaba232..18bb245de 100644 --- a/app/scripts/controllers/transactions.js +++ b/app/scripts/controllers/transactions.js @@ -453,7 +453,7 @@ module.exports = class TransactionController extends EventEmitter { // if the value of the transaction is greater then the balance, fail. if (gtBalance) { const message = 'Insufficient balance.' - this.setTxStatusFailed(txMeta.id, message) + this.setTxStatusFailed(txMeta.id, { message }) cb() return log.error(message) } @@ -461,7 +461,7 @@ module.exports = class TransactionController extends EventEmitter { // if the nonce of the transaction is lower then the accounts nonce, fail. if (txNonce < nonce) { const message = 'Invalid nonce.' - this.setTxStatusFailed(txMeta.id, message) + this.setTxStatusFailed(txMeta.id, { message }) cb() return log.error(message) } From 512b6cae81ee0e71b567a72418ac224804dfc3e6 Mon Sep 17 00:00:00 2001 From: kumavis Date: Fri, 7 Jul 2017 19:31:27 -0700 Subject: [PATCH 30/80] migration 16 - move resubmit warning back to submitted state --- app/scripts/migrations/016.js | 41 +++++++++++++++++++++++++++++++++ app/scripts/migrations/index.js | 1 + 2 files changed, 42 insertions(+) create mode 100644 app/scripts/migrations/016.js diff --git a/app/scripts/migrations/016.js b/app/scripts/migrations/016.js new file mode 100644 index 000000000..4fc534f1c --- /dev/null +++ b/app/scripts/migrations/016.js @@ -0,0 +1,41 @@ +const version = 16 + +/* + +This migration sets transactions with the 'Gave up submitting tx.' err message +to a 'failed' stated + +*/ + +const clone = require('clone') + +module.exports = { + version, + + migrate: function (originalVersionedData) { + const versionedData = clone(originalVersionedData) + versionedData.meta.version = version + try { + const state = versionedData.data + const newState = transformState(state) + versionedData.data = newState + } catch (err) { + console.warn(`MetaMask Migration #${version}` + err.stack) + } + return Promise.resolve(versionedData) + }, +} + +function transformState (state) { + const newState = state + const transactions = newState.TransactionController.transactions + newState.TransactionController.transactions = transactions.map((txMeta) => { + if (!txMeta.err) return txMeta + if (txMeta.err === 'transaction with the same hash was already imported.') { + txMeta.status = 'submitted' + delete txMeta.err + } + return txMeta + }) + return newState +} diff --git a/app/scripts/migrations/index.js b/app/scripts/migrations/index.js index 651ee6a9c..a4f9c7c4d 100644 --- a/app/scripts/migrations/index.js +++ b/app/scripts/migrations/index.js @@ -26,4 +26,5 @@ module.exports = [ require('./013'), require('./014'), require('./015'), + require('./016'), ] From de967d2dfd2119d2468263ecb9646fd0a92df195 Mon Sep 17 00:00:00 2001 From: kumavis Date: Fri, 7 Jul 2017 20:05:03 -0700 Subject: [PATCH 31/80] 3.8.5 --- CHANGELOG.md | 4 ++++ app/manifest.json | 2 +- 2 files changed, 5 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 696d68345..2c61c31b9 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,6 +2,10 @@ ## Current Master +## 3.8.5 2017-7-7 + +- Fix transaction resubmit logic to fail slightly less eagerly. + ## 3.8.4 2017-7-7 - Improve transaction resubmit logic to fail more eagerly when a user would expect it to. diff --git a/app/manifest.json b/app/manifest.json index d386e43aa..f3a1ebeff 100644 --- a/app/manifest.json +++ b/app/manifest.json @@ -1,7 +1,7 @@ { "name": "MetaMask", "short_name": "Metamask", - "version": "3.8.4", + "version": "3.8.5", "manifest_version": 2, "author": "https://metamask.io", "description": "Ethereum Browser Extension", From 11d57adc5c6fa124cbb83a907a55a0aa733bb1cc Mon Sep 17 00:00:00 2001 From: frankiebee Date: Tue, 11 Jul 2017 11:57:42 -0700 Subject: [PATCH 32/80] add "Gateway timeout" to ignored errors when resubmiting and use .includes over .startsWith --- app/scripts/controllers/transactions.js | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/app/scripts/controllers/transactions.js b/app/scripts/controllers/transactions.js index 18bb245de..933c079d2 100644 --- a/app/scripts/controllers/transactions.js +++ b/app/scripts/controllers/transactions.js @@ -427,10 +427,12 @@ module.exports = class TransactionController extends EventEmitter { const isKnownTx = ( // geth errorMessage === 'replacement transaction underpriced' - || errorMessage.startsWith('known transaction') + || errorMessage.includes('known transaction') // parity || errorMessage === 'gas price too low to replace' || errorMessage === 'transaction with the same hash was already imported.' + // other + || errorMessage.includes('gateway timeout') ) // ignore resubmit warnings, return early if (isKnownTx) return From d97c6533b87b0a9dd6937c1ca57ec05129ac619b Mon Sep 17 00:00:00 2001 From: Dan Finlay Date: Tue, 11 Jul 2017 11:59:56 -0700 Subject: [PATCH 33/80] Remove local nonce error setting. --- CHANGELOG.md | 2 ++ app/scripts/controllers/transactions.js | 8 -------- 2 files changed, 2 insertions(+), 8 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 696d68345..f53bdead5 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,6 +2,8 @@ ## Current Master +- No longer validate nonce client-side in retry loop. + ## 3.8.4 2017-7-7 - Improve transaction resubmit logic to fail more eagerly when a user would expect it to. diff --git a/app/scripts/controllers/transactions.js b/app/scripts/controllers/transactions.js index 18bb245de..02487c385 100644 --- a/app/scripts/controllers/transactions.js +++ b/app/scripts/controllers/transactions.js @@ -458,14 +458,6 @@ module.exports = class TransactionController extends EventEmitter { return log.error(message) } - // if the nonce of the transaction is lower then the accounts nonce, fail. - if (txNonce < nonce) { - const message = 'Invalid nonce.' - this.setTxStatusFailed(txMeta.id, { message }) - cb() - return log.error(message) - } - // Only auto-submit already-signed txs: if (!('rawTx' in txMeta)) return cb() From 611338c4e0b6de670f1a3ac6a0f1ddd3a2c063f7 Mon Sep 17 00:00:00 2001 From: frankiebee Date: Tue, 11 Jul 2017 12:01:59 -0700 Subject: [PATCH 34/80] use .includes --- app/scripts/controllers/transactions.js | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/app/scripts/controllers/transactions.js b/app/scripts/controllers/transactions.js index 933c079d2..c0d4841a9 100644 --- a/app/scripts/controllers/transactions.js +++ b/app/scripts/controllers/transactions.js @@ -426,11 +426,11 @@ module.exports = class TransactionController extends EventEmitter { const errorMessage = err.message.toLowerCase() const isKnownTx = ( // geth - errorMessage === 'replacement transaction underpriced' + errorMessage.includes('replacement transaction underpriced') || errorMessage.includes('known transaction') // parity - || errorMessage === 'gas price too low to replace' - || errorMessage === 'transaction with the same hash was already imported.' + || errorMessage.includes('gas price too low to replace') + || errorMessage.includes('transaction with the same hash was already imported') // other || errorMessage.includes('gateway timeout') ) From c121ac21ec3bed0381e36de7ead1b583a3da148c Mon Sep 17 00:00:00 2001 From: frankiebee Date: Tue, 11 Jul 2017 12:16:08 -0700 Subject: [PATCH 35/80] remove irrelevan code --- app/scripts/controllers/transactions.js | 1 - 1 file changed, 1 deletion(-) diff --git a/app/scripts/controllers/transactions.js b/app/scripts/controllers/transactions.js index 2b40f9456..14b423d5d 100644 --- a/app/scripts/controllers/transactions.js +++ b/app/scripts/controllers/transactions.js @@ -30,7 +30,6 @@ module.exports = class TransactionController extends EventEmitter { from: address, status: 'submitted', err: undefined, - ignore: undefined, }) }, }) From c7b9e3fb1878cebbab26d5343cc18084a601c6bb Mon Sep 17 00:00:00 2001 From: Dan Finlay Date: Tue, 11 Jul 2017 12:18:07 -0700 Subject: [PATCH 36/80] Improve insufficient balance checking in retry loop --- CHANGELOG.md | 1 + app/scripts/controllers/transactions.js | 5 +--- app/scripts/lib/tx-utils.js | 9 ++++++ test/unit/tx-utils-test.js | 38 +++++++++++++++++++++++++ 4 files changed, 49 insertions(+), 4 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index f53bdead5..395454b41 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -3,6 +3,7 @@ ## Current Master - No longer validate nonce client-side in retry loop. +- Fix bug where insufficient balance error was sometimes shown on successful transactions. ## 3.8.4 2017-7-7 diff --git a/app/scripts/controllers/transactions.js b/app/scripts/controllers/transactions.js index 02487c385..ca379a7ff 100644 --- a/app/scripts/controllers/transactions.js +++ b/app/scripts/controllers/transactions.js @@ -445,13 +445,10 @@ module.exports = class TransactionController extends EventEmitter { _resubmitTx (txMeta, cb) { const address = txMeta.txParams.from const balance = this.ethStore.getState().accounts[address].balance - const nonce = Number.parseInt(this.ethStore.getState().accounts[address].nonce) - const txNonce = Number.parseInt(txMeta.txParams.nonce) - const gtBalance = Number.parseInt(txMeta.txParams.value) > Number.parseInt(balance) if (!('retryCount' in txMeta)) txMeta.retryCount = 0 // if the value of the transaction is greater then the balance, fail. - if (gtBalance) { + if (!this.txProviderUtils.sufficientBalance(txMeta.txParams, balance)) { const message = 'Insufficient balance.' this.setTxStatusFailed(txMeta.id, { message }) cb() diff --git a/app/scripts/lib/tx-utils.js b/app/scripts/lib/tx-utils.js index 149d93102..4e780fcc0 100644 --- a/app/scripts/lib/tx-utils.js +++ b/app/scripts/lib/tx-utils.js @@ -118,6 +118,15 @@ module.exports = class txProviderUtils { } } + sufficientBalance (tx, hexBalance) { + const balance = hexToBn(hexBalance) + const value = hexToBn(tx.value) + const gasLimit = hexToBn(tx.gas) + const gasPrice = hexToBn(tx.gasPrice) + + const maxCost = value.add(gasLimit.mul(gasPrice)) + return balance.gte(maxCost) + } } diff --git a/test/unit/tx-utils-test.js b/test/unit/tx-utils-test.js index 7ace1f587..a43bcfb35 100644 --- a/test/unit/tx-utils-test.js +++ b/test/unit/tx-utils-test.js @@ -16,6 +16,44 @@ describe('txUtils', function () { })) }) + describe('#sufficientBalance', function () { + it('returns true if max tx cost is equal to balance.', function () { + const tx = { + 'value': '0x1', + 'gas': '0x2', + 'gasPrice': '0x3', + } + const balance = '0x8' + + const result = txUtils.sufficientBalance(tx, balance) + assert.ok(result, 'sufficient balance found.') + }) + + it('returns true if max tx cost is less than balance.', function () { + const tx = { + 'value': '0x1', + 'gas': '0x2', + 'gasPrice': '0x3', + } + const balance = '0x9' + + const result = txUtils.sufficientBalance(tx, balance) + assert.ok(result, 'sufficient balance found.') + }) + + it('returns false if max tx cost is more than balance.', function () { + const tx = { + 'value': '0x1', + 'gas': '0x2', + 'gasPrice': '0x3', + } + const balance = '0x6' + + const result = txUtils.sufficientBalance(tx, balance) + assert.ok(!result, 'insufficient balance found.') + }) + }) + describe('chain Id', function () { it('prepares a transaction with the provided chainId', function () { const txParams = { From 6587f6eabda08629b7fce431820f92ab275bbf75 Mon Sep 17 00:00:00 2001 From: kumavis Date: Tue, 11 Jul 2017 12:43:15 -0700 Subject: [PATCH 37/80] deps - bump prov-eng for retry on gateway timeout --- package.json | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/package.json b/package.json index 27fe7a84a..47f5db15d 100644 --- a/package.json +++ b/package.json @@ -124,7 +124,7 @@ "valid-url": "^1.0.9", "vreme": "^3.0.2", "web3": "0.19.1", - "web3-provider-engine": "^13.1.1", + "web3-provider-engine": "^13.2.0", "web3-stream-provider": "^3.0.1", "xtend": "^4.0.1" }, From 231ad48564758895bace7b0e750cdfa5577128f8 Mon Sep 17 00:00:00 2001 From: frankiebee Date: Tue, 11 Jul 2017 12:52:56 -0700 Subject: [PATCH 38/80] Use txParams --- app/scripts/lib/tx-utils.js | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/app/scripts/lib/tx-utils.js b/app/scripts/lib/tx-utils.js index 4e780fcc0..aa0cb624f 100644 --- a/app/scripts/lib/tx-utils.js +++ b/app/scripts/lib/tx-utils.js @@ -118,11 +118,11 @@ module.exports = class txProviderUtils { } } - sufficientBalance (tx, hexBalance) { + sufficientBalance (txParams, hexBalance) { const balance = hexToBn(hexBalance) - const value = hexToBn(tx.value) - const gasLimit = hexToBn(tx.gas) - const gasPrice = hexToBn(tx.gasPrice) + const value = hexToBn(txParams.value) + const gasLimit = hexToBn(txParams.gas) + const gasPrice = hexToBn(txParams.gasPrice) const maxCost = value.add(gasLimit.mul(gasPrice)) return balance.gte(maxCost) From 9f46984fee8f4fa8268ac9bd70081c58708ac8ea Mon Sep 17 00:00:00 2001 From: kumavis Date: Tue, 11 Jul 2017 14:17:47 -0700 Subject: [PATCH 39/80] metamask - on rpc err show whole error body --- 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 73093dfad..0e7ccbd66 100644 --- a/app/scripts/metamask-controller.js +++ b/app/scripts/metamask-controller.js @@ -367,7 +367,7 @@ module.exports = class MetamaskController extends EventEmitter { function onResponse (err, request, response) { if (err) return console.error(err) if (response.error) { - console.error('Error in RPC response:\n', response.error) + console.error('Error in RPC response:\n', response) } if (request.isMetamaskInternal) return log.info(`RPC (${originDomain}):`, request, '->', response) From 0cc60fda8f14174b978e49a9b1e97e0accbce31d Mon Sep 17 00:00:00 2001 From: kumavis Date: Tue, 11 Jul 2017 14:18:09 -0700 Subject: [PATCH 40/80] deps - bump prov-eng for fetch retry --- package.json | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/package.json b/package.json index 47f5db15d..fb278b398 100644 --- a/package.json +++ b/package.json @@ -124,7 +124,7 @@ "valid-url": "^1.0.9", "vreme": "^3.0.2", "web3": "0.19.1", - "web3-provider-engine": "^13.2.0", + "web3-provider-engine": "^13.2.7", "web3-stream-provider": "^3.0.1", "xtend": "^4.0.1" }, From 23d44e53996c04fb0e6fb2e1c2ca90728397aa0c Mon Sep 17 00:00:00 2001 From: kumavis Date: Tue, 11 Jul 2017 15:30:36 -0700 Subject: [PATCH 41/80] tests - disable infura test --- test/unit/infura-controller-test.js | 66 ++++++++++++++--------------- 1 file changed, 33 insertions(+), 33 deletions(-) diff --git a/test/unit/infura-controller-test.js b/test/unit/infura-controller-test.js index 7a2a114f9..912867764 100644 --- a/test/unit/infura-controller-test.js +++ b/test/unit/infura-controller-test.js @@ -1,34 +1,34 @@ // polyfill fetch -global.fetch = function () {return Promise.resolve({ - json: () => { return Promise.resolve({"mainnet": "ok", "ropsten": "degraded", "kovan": "down", "rinkeby": "ok"}) }, - }) -} -const assert = require('assert') -const InfuraController = require('../../app/scripts/controllers/infura') - -describe('infura-controller', function () { - var infuraController - - beforeEach(function () { - infuraController = new InfuraController() - }) - - describe('network status queries', function () { - describe('#checkInfuraNetworkStatus', function () { - it('should return an object reflecting the network statuses', function (done) { - this.timeout(15000) - infuraController.checkInfuraNetworkStatus() - .then(() => { - const networkStatus = infuraController.store.getState().infuraNetworkStatus - assert.equal(Object.keys(networkStatus).length, 4) - assert.equal(networkStatus.mainnet, 'ok') - assert.equal(networkStatus.ropsten, 'degraded') - assert.equal(networkStatus.kovan, 'down') - }) - .then(() => done()) - .catch(done) - - }) - }) - }) -}) +// global.fetch = function () {return Promise.resolve({ +// json: () => { return Promise.resolve({"mainnet": "ok", "ropsten": "degraded", "kovan": "down", "rinkeby": "ok"}) }, +// }) +// } +// const assert = require('assert') +// const InfuraController = require('../../app/scripts/controllers/infura') +// +// describe('infura-controller', function () { +// var infuraController +// +// beforeEach(function () { +// infuraController = new InfuraController() +// }) +// +// describe('network status queries', function () { +// describe('#checkInfuraNetworkStatus', function () { +// it('should return an object reflecting the network statuses', function (done) { +// this.timeout(15000) +// infuraController.checkInfuraNetworkStatus() +// .then(() => { +// const networkStatus = infuraController.store.getState().infuraNetworkStatus +// assert.equal(Object.keys(networkStatus).length, 4) +// assert.equal(networkStatus.mainnet, 'ok') +// assert.equal(networkStatus.ropsten, 'degraded') +// assert.equal(networkStatus.kovan, 'down') +// }) +// .then(() => done()) +// .catch(done) +// +// }) +// }) +// }) +// }) From 1448090ec76a5ab9274b27e4c71aef9970ca0214 Mon Sep 17 00:00:00 2001 From: kumavis Date: Tue, 11 Jul 2017 15:33:12 -0700 Subject: [PATCH 42/80] deps - bump prov-eng --- package.json | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/package.json b/package.json index fb278b398..10b175975 100644 --- a/package.json +++ b/package.json @@ -124,7 +124,7 @@ "valid-url": "^1.0.9", "vreme": "^3.0.2", "web3": "0.19.1", - "web3-provider-engine": "^13.2.7", + "web3-provider-engine": "^13.2.8", "web3-stream-provider": "^3.0.1", "xtend": "^4.0.1" }, From eddc8cfee7fa161b0901483998fce868d3cc4077 Mon Sep 17 00:00:00 2001 From: Dan Finlay Date: Tue, 11 Jul 2017 16:00:20 -0700 Subject: [PATCH 43/80] Version 3.8.6 --- CHANGELOG.md | 3 +++ app/manifest.json | 2 +- 2 files changed, 4 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index cb3fcfb83..535fa32f0 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,6 +2,9 @@ ## Current Master +## 3.8.6 2017-7-11 + +- Make transaction resubmission more resilient. - No longer validate nonce client-side in retry loop. - Fix bug where insufficient balance error was sometimes shown on successful transactions. diff --git a/app/manifest.json b/app/manifest.json index f3a1ebeff..2b1f6d69f 100644 --- a/app/manifest.json +++ b/app/manifest.json @@ -1,7 +1,7 @@ { "name": "MetaMask", "short_name": "Metamask", - "version": "3.8.5", + "version": "3.8.6", "manifest_version": 2, "author": "https://metamask.io", "description": "Ethereum Browser Extension", From 52b92fbe40e221c53e1c93a2e998c65833c2334d Mon Sep 17 00:00:00 2001 From: Dan Finlay Date: Wed, 12 Jul 2017 13:09:20 -0700 Subject: [PATCH 44/80] Add first version of phishing site warning Links to my own blacklist for now, since I added a package.json for easy importing. We can point at the main 408H repository once this is merged: https://github.com/409H/EtherAddressLookup/pull/24 Redirects detected phishing sites [here](https://metamask.io/phishing.html). --- app/manifest.json | 6 ++++++ app/scripts/blacklister.js | 13 +++++++++++++ gulpfile.js | 1 + package.json | 1 + 4 files changed, 21 insertions(+) create mode 100644 app/scripts/blacklister.js diff --git a/app/manifest.json b/app/manifest.json index f3a1ebeff..ac6364059 100644 --- a/app/manifest.json +++ b/app/manifest.json @@ -52,6 +52,12 @@ ], "run_at": "document_start", "all_frames": true + }, + { + "run_at": "document_end", + "matches": ["http://*/*", "https://*/*"], + "js": ["scripts/blacklister.js"], + "css": ["css/blacklister.css"] } ], "permissions": [ diff --git a/app/scripts/blacklister.js b/app/scripts/blacklister.js new file mode 100644 index 000000000..a45265a75 --- /dev/null +++ b/app/scripts/blacklister.js @@ -0,0 +1,13 @@ +const blacklistedDomains = require('etheraddresslookup/blacklists/domains.json') + +function detectBlacklistedDomain() { + var strCurrentTab = window.location.hostname + if (blacklistedDomains && blacklistedDomains.includes(strCurrentTab)) { + window.location.href = 'https://metamask.io/phishing.html' + } +} + +window.addEventListener('load', function() { + detectBlacklistedDomain() +}) + diff --git a/gulpfile.js b/gulpfile.js index cc723704a..53de7a7d9 100644 --- a/gulpfile.js +++ b/gulpfile.js @@ -172,6 +172,7 @@ gulp.task('default', ['lint'], function () { const jsFiles = [ 'inpage', 'contentscript', + 'blacklister', 'background', 'popup', ] diff --git a/package.json b/package.json index 10b175975..87312b8d1 100644 --- a/package.json +++ b/package.json @@ -68,6 +68,7 @@ "eth-sig-util": "^1.1.1", "eth-simple-keyring": "^1.1.1", "eth-token-tracker": "^1.1.2", + "etheraddresslookup": "github:flyswatter/EtherAddressLookup#AddPackageJson", "ethereumjs-tx": "^1.3.0", "ethereumjs-util": "ethereumjs/ethereumjs-util#ac5d0908536b447083ea422b435da27f26615de9", "ethereumjs-wallet": "^0.6.0", From 2c5b9da06a9b6e6455361136390784e3e774b5aa Mon Sep 17 00:00:00 2001 From: Dan Finlay Date: Wed, 12 Jul 2017 13:14:18 -0700 Subject: [PATCH 45/80] Bump changelog --- CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index cb3fcfb83..02bebbb4d 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,6 +2,7 @@ ## Current Master +- Now detects and blocks known phishing sites. - No longer validate nonce client-side in retry loop. - Fix bug where insufficient balance error was sometimes shown on successful transactions. From 0079126b7d46f0e20592117563e543531b96c36e Mon Sep 17 00:00:00 2001 From: Dan Finlay Date: Wed, 12 Jul 2017 14:33:03 -0700 Subject: [PATCH 46/80] Point blacklist at main repository --- package.json | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/package.json b/package.json index 87312b8d1..54addd51c 100644 --- a/package.json +++ b/package.json @@ -68,7 +68,7 @@ "eth-sig-util": "^1.1.1", "eth-simple-keyring": "^1.1.1", "eth-token-tracker": "^1.1.2", - "etheraddresslookup": "github:flyswatter/EtherAddressLookup#AddPackageJson", + "etheraddresslookup": "github:407H/EtherAddressLookup", "ethereumjs-tx": "^1.3.0", "ethereumjs-util": "ethereumjs/ethereumjs-util#ac5d0908536b447083ea422b435da27f26615de9", "ethereumjs-wallet": "^0.6.0", From da35f6744e3201bb75b896d3127d4f30d7e4d789 Mon Sep 17 00:00:00 2001 From: frankiebee Date: Wed, 12 Jul 2017 15:06:49 -0700 Subject: [PATCH 47/80] use new nodeify --- app/scripts/lib/nodeify.js | 27 +++++---------------- app/scripts/metamask-controller.js | 38 ++++++++++++------------------ 2 files changed, 21 insertions(+), 44 deletions(-) diff --git a/app/scripts/lib/nodeify.js b/app/scripts/lib/nodeify.js index 51d89a8fb..4e111c8b2 100644 --- a/app/scripts/lib/nodeify.js +++ b/app/scripts/lib/nodeify.js @@ -1,24 +1,9 @@ -module.exports = function (promiseFn) { - return function () { - var args = [] - for (var i = 0; i < arguments.length - 1; i++) { - args.push(arguments[i]) - } - var cb = arguments[arguments.length - 1] +const promiseToCallback = require('promise-to-callback'); - const nodeified = promiseFn.apply(this, args) - - if (!nodeified) { - const methodName = String(promiseFn).split('(')[0] - throw new Error(`The ${methodName} did not return a Promise, but was nodeified.`) - } - nodeified.then(function (result) { - cb(null, result) - }) - .catch(function (reason) { - cb(reason) - }) - - return nodeified +module.exports = function(fn, context) { + return function(){ + const args = [].slice.call(arguments) + const callback = args.pop() + promiseToCallback(fn.apply(context, args))(callback) } } diff --git a/app/scripts/metamask-controller.js b/app/scripts/metamask-controller.js index 573594b39..f1f21b29b 100644 --- a/app/scripts/metamask-controller.js +++ b/app/scripts/metamask-controller.js @@ -294,34 +294,33 @@ module.exports = class MetamaskController extends EventEmitter { submitPassword: this.submitPassword.bind(this), // PreferencesController - setSelectedAddress: nodeify(preferencesController.setSelectedAddress).bind(preferencesController), - addToken: nodeify(preferencesController.addToken).bind(preferencesController), - setCurrentAccountTab: nodeify(preferencesController.setCurrentAccountTab).bind(preferencesController), - setDefaultRpc: nodeify(this.setDefaultRpc).bind(this), - setCustomRpc: nodeify(this.setCustomRpc).bind(this), + setSelectedAddress: nodeify(preferencesController.setSelectedAddress, preferencesController), + addToken: nodeify(preferencesController.addToken, preferencesController), + setCurrentAccountTab: nodeify(preferencesController.setCurrentAccountTab, preferencesController), + setDefaultRpc: nodeify(this.setDefaultRpc, this), + setCustomRpc: nodeify(this.setCustomRpc, this), // AddressController - setAddressBook: nodeify(addressBookController.setAddressBook).bind(addressBookController), + setAddressBook: nodeify(addressBookController.setAddressBook, addressBookController), // KeyringController - setLocked: nodeify(keyringController.setLocked).bind(keyringController), - createNewVaultAndKeychain: nodeify(keyringController.createNewVaultAndKeychain).bind(keyringController), - createNewVaultAndRestore: nodeify(keyringController.createNewVaultAndRestore).bind(keyringController), - addNewKeyring: nodeify(keyringController.addNewKeyring).bind(keyringController), - saveAccountLabel: nodeify(keyringController.saveAccountLabel).bind(keyringController), - exportAccount: nodeify(keyringController.exportAccount).bind(keyringController), + setLocked: nodeify(keyringController.setLocked, keyringController), + createNewVaultAndKeychain: nodeify(keyringController.createNewVaultAndKeychain, keyringController), + createNewVaultAndRestore: nodeify(keyringController.createNewVaultAndRestore, keyringController), + addNewKeyring: nodeify(keyringController.addNewKeyring, keyringController), + saveAccountLabel: nodeify(keyringController.saveAccountLabel, keyringController), + exportAccount: nodeify(keyringController.exportAccount, keyringController), // txController - approveTransaction: nodeify(txController.approveTransaction).bind(txController), cancelTransaction: txController.cancelTransaction.bind(txController), - updateAndApproveTransaction: this.updateAndApproveTx.bind(this), + updateAndApproveTransaction: nodeify(txController.updateAndApproveTransaction, txController), // messageManager - signMessage: nodeify(this.signMessage).bind(this), + signMessage: nodeify(this.signMessage, this), cancelMessage: this.cancelMessage.bind(this), // personalMessageManager - signPersonalMessage: nodeify(this.signPersonalMessage).bind(this), + signPersonalMessage: nodeify(this.signPersonalMessage, this), cancelPersonalMessage: this.cancelPersonalMessage.bind(this), // notices @@ -502,13 +501,6 @@ module.exports = class MetamaskController extends EventEmitter { }) } - updateAndApproveTx (txMeta, cb) { - log.debug(`MetaMaskController - updateAndApproveTx: ${JSON.stringify(txMeta)}`) - const txController = this.txController - txController.updateTx(txMeta) - txController.approveTransaction(txMeta.id, cb) - } - signMessage (msgParams, cb) { log.info('MetaMaskController - signMessage') const msgId = msgParams.metamaskId From bd26ec46aa8f2d144939573a8c427e2a7743d25c Mon Sep 17 00:00:00 2001 From: frankiebee Date: Wed, 12 Jul 2017 15:07:56 -0700 Subject: [PATCH 48/80] mv updateAndApproveTx to txController --- app/scripts/controllers/transactions.js | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/app/scripts/controllers/transactions.js b/app/scripts/controllers/transactions.js index 45c6fb25a..a2842ae44 100644 --- a/app/scripts/controllers/transactions.js +++ b/app/scripts/controllers/transactions.js @@ -183,7 +183,7 @@ module.exports = class TransactionController extends EventEmitter { }, {}) } - async approveTransaction (txId, cb = warn) { + async approveTransaction (txId) { let nonceLock try { // approve @@ -199,7 +199,6 @@ module.exports = class TransactionController extends EventEmitter { await this.publishTransaction(txId, rawTx) // must set transaction to submitted/failed before releasing lock nonceLock.releaseLock() - cb() } catch (err) { this.setTxStatusFailed(txId, { errCode: err.errCode || err, @@ -208,7 +207,7 @@ module.exports = class TransactionController extends EventEmitter { // must set transaction to submitted/failed before releasing lock if (nonceLock) nonceLock.releaseLock() // continue with error chain - cb(err) + throw err } } @@ -217,6 +216,11 @@ module.exports = class TransactionController extends EventEmitter { cb() } + async updateAndApproveTransaction (txMeta) { + this.updateTx(txMeta) + await this.approveTransaction(txMeta.id) + } + getChainId () { const networkState = this.networkStore.getState() const getChainId = parseInt(networkState) From ed272dcbc082ebf9abbd7f17da1386163013c023 Mon Sep 17 00:00:00 2001 From: Dan Finlay Date: Wed, 12 Jul 2017 15:09:01 -0700 Subject: [PATCH 49/80] Bump node version --- circle.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/circle.yml b/circle.yml index 1f018ac24..66eed17d7 100644 --- a/circle.yml +++ b/circle.yml @@ -1,6 +1,6 @@ machine: node: - version: 8.0.0 + version: 8.1.4 dependencies: pre: - "npm i -g testem" From aeefcbd75bcf419833b9cc5b734e2d86f47ba6d1 Mon Sep 17 00:00:00 2001 From: frankiebee Date: Wed, 12 Jul 2017 15:10:52 -0700 Subject: [PATCH 50/80] Fix test to match behavior --- app/scripts/lib/nodeify.js | 2 +- test/unit/nodeify-test.js | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/app/scripts/lib/nodeify.js b/app/scripts/lib/nodeify.js index 4e111c8b2..299bfe624 100644 --- a/app/scripts/lib/nodeify.js +++ b/app/scripts/lib/nodeify.js @@ -1,4 +1,4 @@ -const promiseToCallback = require('promise-to-callback'); +const promiseToCallback = require('promise-to-callback') module.exports = function(fn, context) { return function(){ diff --git a/test/unit/nodeify-test.js b/test/unit/nodeify-test.js index 5aed758fa..06241334d 100644 --- a/test/unit/nodeify-test.js +++ b/test/unit/nodeify-test.js @@ -11,7 +11,7 @@ describe('nodeify', function () { } it('should retain original context', function (done) { - var nodified = nodeify(obj.promiseFunc).bind(obj) + var nodified = nodeify(obj.promiseFunc, obj) nodified('baz', function (err, res) { assert.equal(res, 'barbaz') done() From aec813eace6db96984ccbb29d8b98d60097b22e2 Mon Sep 17 00:00:00 2001 From: Dan Finlay Date: Wed, 12 Jul 2017 15:15:19 -0700 Subject: [PATCH 51/80] Correct github link --- package.json | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/package.json b/package.json index 54addd51c..16f567dd2 100644 --- a/package.json +++ b/package.json @@ -68,7 +68,7 @@ "eth-sig-util": "^1.1.1", "eth-simple-keyring": "^1.1.1", "eth-token-tracker": "^1.1.2", - "etheraddresslookup": "github:407H/EtherAddressLookup", + "etheraddresslookup": "github:409H/EtherAddressLookup", "ethereumjs-tx": "^1.3.0", "ethereumjs-util": "ethereumjs/ethereumjs-util#ac5d0908536b447083ea422b435da27f26615de9", "ethereumjs-wallet": "^0.6.0", From 76a2a59ec54cb20cd482adf724815100916d5d3e Mon Sep 17 00:00:00 2001 From: Dan Finlay Date: Wed, 12 Jul 2017 15:24:59 -0700 Subject: [PATCH 52/80] Refresh blacklist before dist --- package.json | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/package.json b/package.json index 16f567dd2..a1b1afae1 100644 --- a/package.json +++ b/package.json @@ -7,7 +7,7 @@ "start": "npm run dev", "dev": "gulp dev --debug", "disc": "gulp disc --debug", - "dist": "npm install && gulp dist", + "dist": "rm -rf node_modules/etheraddresslookup && npm install && gulp dist", "test": "npm run lint && npm run test-unit && npm run test-integration", "test-unit": "METAMASK_ENV=test mocha --require test/helper.js --recursive \"test/unit/**/*.js\"", "test-integration": "npm run buildMock && npm run buildCiUnits && testem ci -P 2", From ebe76664266d3c712ec14ac4aef7fc48b3f1b5c3 Mon Sep 17 00:00:00 2001 From: Dan Finlay Date: Wed, 12 Jul 2017 15:31:29 -0700 Subject: [PATCH 53/80] Update eth-contract-metadata on build --- package.json | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/package.json b/package.json index 10b175975..8a394ad75 100644 --- a/package.json +++ b/package.json @@ -7,7 +7,8 @@ "start": "npm run dev", "dev": "gulp dev --debug", "disc": "gulp disc --debug", - "dist": "npm install && gulp dist", + "clear": "rm -rf node_modules/eth-contract-metadata", + "dist": "npm run clear && npm install && gulp dist", "test": "npm run lint && npm run test-unit && npm run test-integration", "test-unit": "METAMASK_ENV=test mocha --require test/helper.js --recursive \"test/unit/**/*.js\"", "test-integration": "npm run buildMock && npm run buildCiUnits && testem ci -P 2", @@ -62,7 +63,7 @@ "end-of-stream": "^1.1.0", "ensnare": "^1.0.0", "eth-bin-to-ops": "^1.0.1", - "eth-contract-metadata": "^1.1.3", + "eth-contract-metadata": "^1.1.4", "eth-hd-keyring": "^1.1.1", "eth-query": "^2.1.2", "eth-sig-util": "^1.1.1", From 414b97921982b21ab5618c6f88192d4a984abf45 Mon Sep 17 00:00:00 2001 From: Dan Finlay Date: Wed, 12 Jul 2017 16:38:56 -0700 Subject: [PATCH 54/80] Version 3.9.0 --- CHANGELOG.md | 2 ++ app/manifest.json | 2 +- 2 files changed, 3 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 03f18201a..ee8589735 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,6 +2,8 @@ ## Current Master +## 3.9.0 2017-7-12 + - Now detects and blocks known phishing sites. ## 3.8.6 2017-7-11 diff --git a/app/manifest.json b/app/manifest.json index ed1d68190..30de25b6d 100644 --- a/app/manifest.json +++ b/app/manifest.json @@ -1,7 +1,7 @@ { "name": "MetaMask", "short_name": "Metamask", - "version": "3.8.6", + "version": "3.9.0", "manifest_version": 2, "author": "https://metamask.io", "description": "Ethereum Browser Extension", From 1357526dfc11caa7993e45895e52e47f27116fe5 Mon Sep 17 00:00:00 2001 From: Dan Finlay Date: Wed, 12 Jul 2017 16:42:24 -0700 Subject: [PATCH 55/80] Remove css reference --- app/manifest.json | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/app/manifest.json b/app/manifest.json index 30de25b6d..269e88e64 100644 --- a/app/manifest.json +++ b/app/manifest.json @@ -56,8 +56,7 @@ { "run_at": "document_end", "matches": ["http://*/*", "https://*/*"], - "js": ["scripts/blacklister.js"], - "css": ["css/blacklister.css"] + "js": ["scripts/blacklister.js"] } ], "permissions": [ From 27cb02bc5868673af794e0ad597fea87b2ddf175 Mon Sep 17 00:00:00 2001 From: frankiebee Date: Wed, 12 Jul 2017 18:54:01 -0700 Subject: [PATCH 56/80] add "nonce too low" to the ignored errs list for tx retrys --- app/scripts/controllers/transactions.js | 1 + 1 file changed, 1 insertion(+) diff --git a/app/scripts/controllers/transactions.js b/app/scripts/controllers/transactions.js index 43735a691..4d037ce98 100644 --- a/app/scripts/controllers/transactions.js +++ b/app/scripts/controllers/transactions.js @@ -433,6 +433,7 @@ module.exports = class TransactionController extends EventEmitter { || errorMessage.includes('transaction with the same hash was already imported') // other || errorMessage.includes('gateway timeout') + || errorMessage.includes('nonce too low') ) // ignore resubmit warnings, return early if (isKnownTx) return From de0cd6e66375f690965f60c58e70900660f565f2 Mon Sep 17 00:00:00 2001 From: frankiebee Date: Wed, 12 Jul 2017 18:56:50 -0700 Subject: [PATCH 57/80] write a migration for resubmit tx's to get put back into a submitted state --- app/scripts/migrations/017.js | 40 +++++++++++++++++++++++++++++++++ app/scripts/migrations/index.js | 1 + 2 files changed, 41 insertions(+) create mode 100644 app/scripts/migrations/017.js diff --git a/app/scripts/migrations/017.js b/app/scripts/migrations/017.js new file mode 100644 index 000000000..c9898ead7 --- /dev/null +++ b/app/scripts/migrations/017.js @@ -0,0 +1,40 @@ +const version = 17 + +/* + +This migration sets transactions who were retried and marked as failed to submitted + +*/ + +const clone = require('clone') + +module.exports = { + version, + + migrate: function (originalVersionedData) { + const versionedData = clone(originalVersionedData) + versionedData.meta.version = version + try { + const state = versionedData.data + const newState = transformState(state) + versionedData.data = newState + } catch (err) { + console.warn(`MetaMask Migration #${version}` + err.stack) + } + return Promise.resolve(versionedData) + }, +} + +function transformState (state) { + const newState = state + const transactions = newState.TransactionController.transactions + newState.TransactionController.transactions = transactions.map((txMeta) => { + if (!txMeta.status === 'failed') return txMeta + if (txMeta.retryCount > 0) { + txMeta.status = 'submitted' + delete txMeta.err + } + return txMeta + }) + return newState +} diff --git a/app/scripts/migrations/index.js b/app/scripts/migrations/index.js index a4f9c7c4d..f4c87499f 100644 --- a/app/scripts/migrations/index.js +++ b/app/scripts/migrations/index.js @@ -27,4 +27,5 @@ module.exports = [ require('./014'), require('./015'), require('./016'), + require('./017'), ] From 6086bcdf0d1346633bc41fde88bb315ead7f9227 Mon Sep 17 00:00:00 2001 From: frankiebee Date: Wed, 12 Jul 2017 20:01:07 -0700 Subject: [PATCH 58/80] limit the range for retryCount --- app/scripts/migrations/017.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/scripts/migrations/017.js b/app/scripts/migrations/017.js index c9898ead7..24959cd3a 100644 --- a/app/scripts/migrations/017.js +++ b/app/scripts/migrations/017.js @@ -30,7 +30,7 @@ function transformState (state) { const transactions = newState.TransactionController.transactions newState.TransactionController.transactions = transactions.map((txMeta) => { if (!txMeta.status === 'failed') return txMeta - if (txMeta.retryCount > 0) { + if (txMeta.retryCount > 0 && txMeta.retryCount < 2) { txMeta.status = 'submitted' delete txMeta.err } From 5e31fc97cd5a30d6f750dde7f13664e3a731ebca Mon Sep 17 00:00:00 2001 From: Dan Finlay Date: Thu, 13 Jul 2017 10:38:56 -0700 Subject: [PATCH 59/80] Redirect from malicious sites faster --- CHANGELOG.md | 2 ++ app/manifest.json | 2 +- 2 files changed, 3 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index ee8589735..7cb79bee8 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,6 +2,8 @@ ## Current Master +- Now redirects from known malicious sites faster. + ## 3.9.0 2017-7-12 - Now detects and blocks known phishing sites. diff --git a/app/manifest.json b/app/manifest.json index 269e88e64..7bf757d4c 100644 --- a/app/manifest.json +++ b/app/manifest.json @@ -54,7 +54,7 @@ "all_frames": true }, { - "run_at": "document_end", + "run_at": "document_start", "matches": ["http://*/*", "https://*/*"], "js": ["scripts/blacklister.js"] } From d6001daab81f1ad8b011363635dbe61322c1482a Mon Sep 17 00:00:00 2001 From: frankiebee Date: Thu, 13 Jul 2017 15:24:19 -0400 Subject: [PATCH 60/80] remove denodeify --- package.json | 1 - 1 file changed, 1 deletion(-) diff --git a/package.json b/package.json index 201713617..06da15179 100644 --- a/package.json +++ b/package.json @@ -56,7 +56,6 @@ "copy-to-clipboard": "^2.0.0", "debounce": "^1.0.0", "deep-extend": "^0.4.1", - "denodeify": "^1.2.1", "detect-node": "^2.0.3", "disc": "^1.3.2", "dnode": "^1.2.2", From 7eccf5905a830853bbb1932dde9a7f4536d43f55 Mon Sep 17 00:00:00 2001 From: frankiebee Date: Thu, 13 Jul 2017 15:25:43 -0400 Subject: [PATCH 61/80] make publishTransaction and signTransaction async methods --- app/scripts/controllers/transactions.js | 31 ++++++++++--------------- app/scripts/lib/tx-utils.js | 9 +++++-- test/unit/tx-controller-test.js | 20 +++++++--------- 3 files changed, 28 insertions(+), 32 deletions(-) diff --git a/app/scripts/controllers/transactions.js b/app/scripts/controllers/transactions.js index a2842ae44..707543c87 100644 --- a/app/scripts/controllers/transactions.js +++ b/app/scripts/controllers/transactions.js @@ -3,7 +3,6 @@ const async = require('async') const extend = require('xtend') const ObservableStore = require('obs-store') const ethUtil = require('ethereumjs-util') -const denodeify = require('denodeify') const TxProviderUtil = require('../lib/tx-utils') const createId = require('../lib/random-id') const NonceTracker = require('../lib/nonce-tracker') @@ -195,7 +194,7 @@ module.exports = class TransactionController extends EventEmitter { txMeta.txParams.nonce = nonceLock.nextNonce this.updateTx(txMeta) // sign transaction - const rawTx = await denodeify(this.signTransaction.bind(this))(txId) + const rawTx = await this.signTransaction(txId) await this.publishTransaction(txId, rawTx) // must set transaction to submitted/failed before releasing lock nonceLock.releaseLock() @@ -231,32 +230,27 @@ module.exports = class TransactionController extends EventEmitter { } } - signTransaction (txId, cb) { + async signTransaction (txId) { const txMeta = this.getTx(txId) const txParams = txMeta.txParams const fromAddress = txParams.from // add network/chain id txParams.chainId = this.getChainId() const ethTx = this.txProviderUtils.buildEthTxFromParams(txParams) - this.signEthTx(ethTx, fromAddress).then(() => { + const rawTx = await this.signEthTx(ethTx, fromAddress).then(() => { this.setTxStatusSigned(txMeta.id) - cb(null, ethUtil.bufferToHex(ethTx.serialize())) - }).catch((err) => { - cb(err) + return ethUtil.bufferToHex(ethTx.serialize()) }) + return rawTx } - publishTransaction (txId, rawTx) { + async publishTransaction (txId, rawTx) { const txMeta = this.getTx(txId) txMeta.rawTx = rawTx this.updateTx(txMeta) - return new Promise((resolve, reject) => { - this.txProviderUtils.publishTransaction(rawTx, (err, txHash) => { - if (err) reject(err) - this.setTxHash(txId, txHash) - this.setTxStatusSubmitted(txId) - resolve() - }) + await this.txProviderUtils.publishTransaction(rawTx).then((txHash) => { + this.setTxHash(txId, txHash) + this.setTxStatusSubmitted(txId) }) } @@ -435,8 +429,7 @@ module.exports = class TransactionController extends EventEmitter { const pending = this.getTxsByMetaData('status', 'submitted') // only try resubmitting if their are transactions to resubmit if (!pending.length) return - const resubmit = denodeify(this._resubmitTx.bind(this)) - pending.forEach((txMeta) => resubmit(txMeta).catch((err) => { + pending.forEach((txMeta) => this._resubmitTx(txMeta).catch((err) => { /* Dont marked as failed if the error is a "known" transaction warning "there is already a transaction with the same sender-nonce @@ -463,7 +456,7 @@ module.exports = class TransactionController extends EventEmitter { })) } - _resubmitTx (txMeta, cb) { + async _resubmitTx (txMeta, cb) { const address = txMeta.txParams.from const balance = this.ethStore.getState().accounts[address].balance if (!('retryCount' in txMeta)) txMeta.retryCount = 0 @@ -482,7 +475,7 @@ module.exports = class TransactionController extends EventEmitter { // Increment a try counter. txMeta.retryCount++ const rawTx = txMeta.rawTx - this.txProviderUtils.publishTransaction(rawTx, cb) + return await this.txProviderUtils.publishTransaction(rawTx, cb) } // checks the network for signed txs and diff --git a/app/scripts/lib/tx-utils.js b/app/scripts/lib/tx-utils.js index aa0cb624f..8f6943937 100644 --- a/app/scripts/lib/tx-utils.js +++ b/app/scripts/lib/tx-utils.js @@ -106,8 +106,13 @@ module.exports = class txProviderUtils { return ethTx } - publishTransaction (rawTx, cb) { - this.query.sendRawTransaction(rawTx, cb) + publishTransaction (rawTx) { + return new Promise((resolve, reject) => { + this.query.sendRawTransaction(rawTx, (err, ress) => { + if (err) reject(err) + else resolve(ress) + }) + }) } validateTxParams (txParams, cb) { diff --git a/test/unit/tx-controller-test.js b/test/unit/tx-controller-test.js index a5af13915..7b86cfe14 100644 --- a/test/unit/tx-controller-test.js +++ b/test/unit/tx-controller-test.js @@ -270,7 +270,7 @@ describe('Transaction Controller', function () { }) - it('does not overwrite set values', function () { + it('does not overwrite set values', function (done) { this.timeout(15000) const wrongValue = '0x05' @@ -283,37 +283,35 @@ describe('Transaction Controller', function () { .callsArgWithAsync(0, null, wrongValue) - const signStub = sinon.stub(txController, 'signTransaction') - .callsArgWithAsync(1, null, noop) + const signStub = sinon.stub(txController, 'signTransaction', () => Promise.resolve()) - const pubStub = sinon.stub(txController.txProviderUtils, 'publishTransaction') - .callsArgWithAsync(1, null, originalValue) + const pubStub = sinon.stub(txController.txProviderUtils, 'publishTransaction', () => Promise.resolve(originalValue)) - return txController.approveTransaction(txMeta.id).then(() => { + txController.approveTransaction(txMeta.id).then(() => { const result = txController.getTx(txMeta.id) const params = result.txParams assert.equal(params.gas, originalValue, 'gas unmodified') assert.equal(params.gasPrice, originalValue, 'gas price unmodified') - assert.equal(result.hash, originalValue, 'hash was set') + assert.equal(result.hash, originalValue, `hash was set \n got: ${result.hash} \n expected: ${originalValue}`) estimateStub.restore() priceStub.restore() signStub.restore() pubStub.restore() - }) + done() + }).catch(done) }) }) describe('#sign replay-protected tx', function () { it('prepares a tx with the chainId set', function (done) { txController.addTx({ id: '1', status: 'unapproved', metamaskNetworkId: currentNetworkId, txParams: {} }, noop) - txController.signTransaction('1', (err, rawTx) => { - if (err) return done('it should not fail') + txController.signTransaction('1').then((rawTx) => { const ethTx = new EthTx(ethUtil.toBuffer(rawTx)) assert.equal(ethTx.getChainId(), currentNetworkId) done() - }) + }).catch(done) }) }) From bda52f7cba9cd866d2244c402fed2e82e1366005 Mon Sep 17 00:00:00 2001 From: tmashuang Date: Fri, 14 Jul 2017 10:34:03 -0700 Subject: [PATCH 62/80] Infura Network response tests --- app/scripts/controllers/infura.js | 1 + test/unit/infura-controller-test.js | 92 ++++++++++++++++++----------- 2 files changed, 59 insertions(+), 34 deletions(-) diff --git a/app/scripts/controllers/infura.js b/app/scripts/controllers/infura.js index 98375b446..b34b0bc03 100644 --- a/app/scripts/controllers/infura.js +++ b/app/scripts/controllers/infura.js @@ -26,6 +26,7 @@ class InfuraController { this.store.updateState({ infuraNetworkStatus: parsedResponse, }) + return parsedResponse }) } diff --git a/test/unit/infura-controller-test.js b/test/unit/infura-controller-test.js index 912867764..b9050f4c2 100644 --- a/test/unit/infura-controller-test.js +++ b/test/unit/infura-controller-test.js @@ -1,34 +1,58 @@ -// polyfill fetch -// global.fetch = function () {return Promise.resolve({ -// json: () => { return Promise.resolve({"mainnet": "ok", "ropsten": "degraded", "kovan": "down", "rinkeby": "ok"}) }, -// }) -// } -// const assert = require('assert') -// const InfuraController = require('../../app/scripts/controllers/infura') -// -// describe('infura-controller', function () { -// var infuraController -// -// beforeEach(function () { -// infuraController = new InfuraController() -// }) -// -// describe('network status queries', function () { -// describe('#checkInfuraNetworkStatus', function () { -// it('should return an object reflecting the network statuses', function (done) { -// this.timeout(15000) -// infuraController.checkInfuraNetworkStatus() -// .then(() => { -// const networkStatus = infuraController.store.getState().infuraNetworkStatus -// assert.equal(Object.keys(networkStatus).length, 4) -// assert.equal(networkStatus.mainnet, 'ok') -// assert.equal(networkStatus.ropsten, 'degraded') -// assert.equal(networkStatus.kovan, 'down') -// }) -// .then(() => done()) -// .catch(done) -// -// }) -// }) -// }) -// }) +const assert = require('assert') +const InfuraController = require('../../app/scripts/controllers/infura') + +describe('infura-controller', function () { + var infuraController + let response + + before(async function () { + infuraController = new InfuraController() + response = await infuraController.checkInfuraNetworkStatus() + }) + + describe('Network status queries', function () { + it('should return object/json', function () { + assert.equal(typeof response, 'object') + }) + + describe('Mainnet', function () { + it('should have Mainnet', function () { + assert.equal(Object.keys(response)[0], 'mainnet') + }) + + it('should have a value for Mainnet status', function () { + assert(response.mainnet, 'Mainnet status') + }) + }) + + describe('Ropsten', function () { + it('should have Ropsten', function () { + assert.equal(Object.keys(response)[1], 'ropsten') + }) + + it('should have a value for Ropsten status', function () { + assert(response.ropsten, 'Ropsten status') + }) + }) + + describe('Kovan', function () { + it('should have Kovan', function () { + assert.equal(Object.keys(response)[2], 'kovan') + }) + + it('should have a value for Kovan status', function () { + assert(response.kovan, 'Kovan status') + }) + }) + + describe('Rinkeby', function () { + it('should have Rinkeby', function () { + assert.equal(Object.keys(response)[3], 'rinkeby') + }) + + it('should have a value for Rinkeby status', function () { + assert(response.rinkeby, 'Rinkeby status') + }) + }) + }) +}) From 02cf65d513b12f3310c2c42ad1ea87165b663e4d Mon Sep 17 00:00:00 2001 From: tmashuang Date: Fri, 14 Jul 2017 11:50:41 -0700 Subject: [PATCH 63/80] Use Let instead of var --- test/unit/infura-controller-test.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/unit/infura-controller-test.js b/test/unit/infura-controller-test.js index b9050f4c2..37cdabe3a 100644 --- a/test/unit/infura-controller-test.js +++ b/test/unit/infura-controller-test.js @@ -2,7 +2,7 @@ const assert = require('assert') const InfuraController = require('../../app/scripts/controllers/infura') describe('infura-controller', function () { - var infuraController + let infuraController let response before(async function () { From 6cf2a956c1df56aa7bdc04d94f89752b0c578f87 Mon Sep 17 00:00:00 2001 From: tmashuang Date: Fri, 14 Jul 2017 13:05:56 -0700 Subject: [PATCH 64/80] Update Sinon --- package.json | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/package.json b/package.json index 1f2d0d591..e0bb303bf 100644 --- a/package.json +++ b/package.json @@ -176,7 +176,7 @@ "react-addons-test-utils": "^15.5.1", "react-test-renderer": "^15.5.4", "react-testutils-additions": "^15.2.0", - "sinon": "^1.17.3", + "sinon": "^2.3.8", "tape": "^4.5.1", "testem": "^1.10.3", "uglifyify": "^3.0.1", From a4c7d95d0de01a37c1e9debea51eb9e2fd8bc2a7 Mon Sep 17 00:00:00 2001 From: tmashuang Date: Fri, 14 Jul 2017 13:06:42 -0700 Subject: [PATCH 65/80] Sinon stub infura network status --- test/unit/infura-controller-test.js | 32 ++++++++++++++++------------- 1 file changed, 18 insertions(+), 14 deletions(-) diff --git a/test/unit/infura-controller-test.js b/test/unit/infura-controller-test.js index 37cdabe3a..605305efa 100644 --- a/test/unit/infura-controller-test.js +++ b/test/unit/infura-controller-test.js @@ -1,57 +1,61 @@ const assert = require('assert') +const sinon = require('sinon') const InfuraController = require('../../app/scripts/controllers/infura') describe('infura-controller', function () { - let infuraController - let response + let infuraController, sandbox, networkStatus + const response = {'mainnet': 'degraded', 'ropsten': 'ok', 'kovan': 'ok', 'rinkeby': 'down'} before(async function () { infuraController = new InfuraController() - response = await infuraController.checkInfuraNetworkStatus() + sandbox = sinon.sandbox.create() + sinon.stub(infuraController, 'checkInfuraNetworkStatus').resolves(response) + networkStatus = await infuraController.checkInfuraNetworkStatus() + }) + + after(function () { + sandbox.restore() }) describe('Network status queries', function () { - it('should return object/json', function () { - assert.equal(typeof response, 'object') - }) describe('Mainnet', function () { it('should have Mainnet', function () { - assert.equal(Object.keys(response)[0], 'mainnet') + assert.equal(Object.keys(networkStatus)[0], 'mainnet') }) it('should have a value for Mainnet status', function () { - assert(response.mainnet, 'Mainnet status') + assert.equal(networkStatus.mainnet, 'degraded') }) }) describe('Ropsten', function () { it('should have Ropsten', function () { - assert.equal(Object.keys(response)[1], 'ropsten') + assert.equal(Object.keys(networkStatus)[1], 'ropsten') }) it('should have a value for Ropsten status', function () { - assert(response.ropsten, 'Ropsten status') + assert.equal(networkStatus.ropsten, 'ok') }) }) describe('Kovan', function () { it('should have Kovan', function () { - assert.equal(Object.keys(response)[2], 'kovan') + assert.equal(Object.keys(networkStatus)[2], 'kovan') }) it('should have a value for Kovan status', function () { - assert(response.kovan, 'Kovan status') + assert.equal(networkStatus.kovan, 'ok') }) }) describe('Rinkeby', function () { it('should have Rinkeby', function () { - assert.equal(Object.keys(response)[3], 'rinkeby') + assert.equal(Object.keys(networkStatus)[3], 'rinkeby') }) it('should have a value for Rinkeby status', function () { - assert(response.rinkeby, 'Rinkeby status') + assert.equal(networkStatus.rinkeby, 'down') }) }) }) From a4e567ffc5a01cb54c73c724c5117988c056fa49 Mon Sep 17 00:00:00 2001 From: Dan Finlay Date: Mon, 17 Jul 2017 09:56:25 -0700 Subject: [PATCH 66/80] Add support page link to help page Also adjust github link to be a new bug link, which goes to the new issue page. --- CHANGELOG.md | 1 + ui/app/info.js | 10 ++++++++-- 2 files changed, 9 insertions(+), 2 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 7cb79bee8..d7b6316db 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -3,6 +3,7 @@ ## Current Master - Now redirects from known malicious sites faster. +- Added a link to our new support page to the help screen. ## 3.9.0 2017-7-12 diff --git a/ui/app/info.js b/ui/app/info.js index e8470de97..cb2e41f5b 100644 --- a/ui/app/info.js +++ b/ui/app/info.js @@ -97,11 +97,17 @@ InfoScreen.prototype.render = function () { paddingLeft: '30px', }}, [ + h('div.fa.fa-support', [ + h('a.info', { + href: 'http://metamask.consensyssupport.happyfox.com', + target: '_blank', + }, 'Visit our Support Center'), + ]), h('div.fa.fa-github', [ h('a.info', { - href: 'https://github.com/MetaMask/faq', + href: 'https://github.com/MetaMask/metamask-extension/issues/new', target: '_blank', - }, 'Need Help? Read our FAQ!'), + }, 'Found a bug? Report it!'), ]), h('div', [ h('a', { From 614501e743a0c1584062c78a25e6b9a3ddf10aab Mon Sep 17 00:00:00 2001 From: Dan Finlay Date: Mon, 17 Jul 2017 14:16:39 -0700 Subject: [PATCH 67/80] Fix transaction confirmation ordering Newest tx or message will now always appear last, and a new tx proposed after the user has a confirmation box open will never change the confirmation to a different tx proposed. Fixes #1637 --- CHANGELOG.md | 1 + test/unit/tx-helper-test.js | 17 +++++++++++++++++ ui/lib/tx-helper.js | 6 +++++- 3 files changed, 23 insertions(+), 1 deletion(-) create mode 100644 test/unit/tx-helper-test.js diff --git a/CHANGELOG.md b/CHANGELOG.md index d7b6316db..c9149287f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,6 +4,7 @@ - Now redirects from known malicious sites faster. - Added a link to our new support page to the help screen. +- Fixed bug where a new transaction would be shown over the current transaction, creating a possible timing attack against user confirmation. ## 3.9.0 2017-7-12 diff --git a/test/unit/tx-helper-test.js b/test/unit/tx-helper-test.js new file mode 100644 index 000000000..cc6543c30 --- /dev/null +++ b/test/unit/tx-helper-test.js @@ -0,0 +1,17 @@ +const assert = require('assert') +const txHelper = require('../../ui/lib/tx-helper') + +describe('txHelper', function () { + it('always shows the oldest tx first', function () { + const metamaskNetworkId = 1 + const txs = { + a: { metamaskNetworkId, time: 3 }, + b: { metamaskNetworkId, time: 1 }, + c: { metamaskNetworkId, time: 2 }, + } + + const sorted = txHelper(txs, null, null, metamaskNetworkId) + assert.equal(sorted[0].time, 1, 'oldest tx first') + assert.equal(sorted[2].time, 3, 'newest tx last') + }) +}) diff --git a/ui/lib/tx-helper.js b/ui/lib/tx-helper.js index ec19daf64..afc62e7b6 100644 --- a/ui/lib/tx-helper.js +++ b/ui/lib/tx-helper.js @@ -12,6 +12,10 @@ module.exports = function (unapprovedTxs, unapprovedMsgs, personalMsgs, network) const personalValues = valuesFor(personalMsgs) log.debug(`tx helper found ${personalValues.length} unsigned personal messages`) allValues = allValues.concat(personalValues) + allValues = allValues.sort((a, b) => { + return a.time > b.time + }) - return allValues.sort(txMeta => txMeta.time) + return allValues } + From 948f3880a3b1af86c3b587d250d0376bbf547d06 Mon Sep 17 00:00:00 2001 From: frankiebee Date: Mon, 17 Jul 2017 16:38:44 -0400 Subject: [PATCH 68/80] turn off auto faucet and remove file --- CHANGELOG.md | 1 + app/scripts/lib/auto-faucet.js | 20 -------------------- app/scripts/metamask-controller.js | 4 ---- 3 files changed, 1 insertion(+), 24 deletions(-) delete mode 100644 app/scripts/lib/auto-faucet.js diff --git a/CHANGELOG.md b/CHANGELOG.md index d7b6316db..ddb07e6a1 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,6 +2,7 @@ ## Current Master +- No longer automatically request 1 ropsten ether for the first account in a new vault. - Now redirects from known malicious sites faster. - Added a link to our new support page to the help screen. diff --git a/app/scripts/lib/auto-faucet.js b/app/scripts/lib/auto-faucet.js deleted file mode 100644 index 38d54ba5e..000000000 --- a/app/scripts/lib/auto-faucet.js +++ /dev/null @@ -1,20 +0,0 @@ -const uri = 'https://faucet.metamask.io/' -const METAMASK_DEBUG = 'GULP_METAMASK_DEBUG' -const env = process.env.METAMASK_ENV - -module.exports = function (address) { - // Don't faucet in development or test - if (METAMASK_DEBUG === true || env === 'test') return - global.log.info('auto-fauceting:', address) - const data = address - const headers = new Headers() - headers.append('Content-type', 'application/rawdata') - fetch(uri, { - method: 'POST', - headers, - body: data, - }) - .catch((err) => { - console.error(err) - }) -} diff --git a/app/scripts/metamask-controller.js b/app/scripts/metamask-controller.js index c6c3fde1e..11dcde2c1 100644 --- a/app/scripts/metamask-controller.js +++ b/app/scripts/metamask-controller.js @@ -20,7 +20,6 @@ const MessageManager = require('./lib/message-manager') const PersonalMessageManager = require('./lib/personal-message-manager') const TransactionController = require('./controllers/transactions') const ConfigManager = require('./lib/config-manager') -const autoFaucet = require('./lib/auto-faucet') const nodeify = require('./lib/nodeify') const accountImporter = require('./account-import-strategies') const getBuyEthUrl = require('./lib/buy-eth-url') @@ -90,9 +89,6 @@ module.exports = class MetamaskController extends EventEmitter { this.keyringController.on('newAccount', (address) => { this.preferencesController.setSelectedAddress(address) }) - this.keyringController.on('newVault', (address) => { - autoFaucet(address) - }) // address book controller this.addressBookController = new AddressBookController({ From 0ebec8d0a8da85177128902210ba7b7a0a5c3d38 Mon Sep 17 00:00:00 2001 From: tmashuang Date: Tue, 18 Jul 2017 09:08:24 -0700 Subject: [PATCH 69/80] Change Ci Badge url for readme --- README.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/README.md b/README.md index afeb96ae5..309423f4c 100644 --- a/README.md +++ b/README.md @@ -1,4 +1,4 @@ -# MetaMask Plugin [![Build Status](https://circleci.com/gh/MetaMask/metamask-plugin.svg?style=shield&circle-token=a1ddcf3cd38e29267f254c9c59d556d513e3a1fd)](https://circleci.com/gh/MetaMask/metamask-plugin) +# MetaMask Plugin [![Build Status](https://circleci.com/gh/MetaMask/metamask-extension.svg?style=svg&circle-token=a1ddcf3cd38e29267f254c9c59d556d513e3a1fd)](https://circleci.com/gh/MetaMask/metamask-extension) ## Developing Compatible Dapps From d9a195f86b88b4c52b200fdba6ed3c0658251f11 Mon Sep 17 00:00:00 2001 From: tmashuang Date: Tue, 18 Jul 2017 09:20:48 -0700 Subject: [PATCH 70/80] Shield looks better --- README.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/README.md b/README.md index 309423f4c..45fb68c78 100644 --- a/README.md +++ b/README.md @@ -1,4 +1,4 @@ -# MetaMask Plugin [![Build Status](https://circleci.com/gh/MetaMask/metamask-extension.svg?style=svg&circle-token=a1ddcf3cd38e29267f254c9c59d556d513e3a1fd)](https://circleci.com/gh/MetaMask/metamask-extension) +# MetaMask Plugin [![Build Status](https://circleci.com/gh/MetaMask/metamask-extension.svg?style=shield&circle-token=a1ddcf3cd38e29267f254c9c59d556d513e3a1fd)](https://circleci.com/gh/MetaMask/metamask-extension) ## Developing Compatible Dapps From 4f9fc8014a1830c9ec7bd35b77f5ee4f5a089fb8 Mon Sep 17 00:00:00 2001 From: kumavis Date: Tue, 18 Jul 2017 12:48:16 -0700 Subject: [PATCH 71/80] nonce-tracker - validate nonce calc components --- app/scripts/lib/nonce-tracker.js | 12 +++++++++--- 1 file changed, 9 insertions(+), 3 deletions(-) diff --git a/app/scripts/lib/nonce-tracker.js b/app/scripts/lib/nonce-tracker.js index ab2893b10..cea798915 100644 --- a/app/scripts/lib/nonce-tracker.js +++ b/app/scripts/lib/nonce-tracker.js @@ -1,4 +1,5 @@ const EthQuery = require('eth-query') +const assert = require('assert') class NonceTracker { @@ -21,10 +22,15 @@ class NonceTracker { // and pending count are from the same block const currentBlock = await this._getCurrentBlock() const pendingTransactions = this.getPendingTransactions(address) - const baseCount = await this._getTxCount(address, currentBlock) - const nextNonce = parseInt(baseCount) + pendingTransactions.length + const pendingCount = pendingTransactions.length + assert(Number.isInteger(pendingCount), 'nonce-tracker - pendingCount is an integer') + const baseCountHex = await this._getTxCount(address, currentBlock) + const baseCount = parseInt(baseCountHex, 16) + assert(Number.isInteger(baseCount), 'nonce-tracker - baseCount is an integer') + const nextNonce = baseCount + pendingCount + assert(Number.isInteger(nextNonce), 'nonce-tracker - nextNonce is an integer') // return next nonce and release cb - return { nextNonce: nextNonce.toString(16), releaseLock } + return { nextNonce: '0x' + nextNonce.toString(16), releaseLock } } async _getCurrentBlock () { From d249da77d7f9602507f708b4778c2128737d4dc5 Mon Sep 17 00:00:00 2001 From: kumavis Date: Tue, 18 Jul 2017 13:59:56 -0700 Subject: [PATCH 72/80] nonce-tracker - return nonce as integer --- app/scripts/lib/nonce-tracker.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/scripts/lib/nonce-tracker.js b/app/scripts/lib/nonce-tracker.js index cea798915..ba05fd124 100644 --- a/app/scripts/lib/nonce-tracker.js +++ b/app/scripts/lib/nonce-tracker.js @@ -30,7 +30,7 @@ class NonceTracker { const nextNonce = baseCount + pendingCount assert(Number.isInteger(nextNonce), 'nonce-tracker - nextNonce is an integer') // return next nonce and release cb - return { nextNonce: '0x' + nextNonce.toString(16), releaseLock } + return { nextNonce, releaseLock } } async _getCurrentBlock () { From 67fdba5e42d8deac1dcbb4a82fd3d22b944e639a Mon Sep 17 00:00:00 2001 From: kumavis Date: Tue, 18 Jul 2017 14:00:43 -0700 Subject: [PATCH 73/80] transaction - promisify _checkPendingTxs --- app/scripts/controllers/transactions.js | 68 +++++++++++++++---------- 1 file changed, 41 insertions(+), 27 deletions(-) diff --git a/app/scripts/controllers/transactions.js b/app/scripts/controllers/transactions.js index 61e96ca13..1fc48aadd 100644 --- a/app/scripts/controllers/transactions.js +++ b/app/scripts/controllers/transactions.js @@ -3,6 +3,7 @@ const async = require('async') const extend = require('xtend') const ObservableStore = require('obs-store') const ethUtil = require('ethereumjs-util') +const pify = require('pify') const TxProviderUtil = require('../lib/tx-utils') const createId = require('../lib/random-id') const NonceTracker = require('../lib/nonce-tracker') @@ -481,35 +482,48 @@ module.exports = class TransactionController extends EventEmitter { // checks the network for signed txs and // if confirmed sets the tx status as 'confirmed' - _checkPendingTxs () { - var signedTxList = this.getFilteredTxList({status: 'submitted'}) - if (!signedTxList.length) return - signedTxList.forEach((txMeta) => { - var txHash = txMeta.hash - var txId = txMeta.id - if (!txHash) { - const errReason = { - errCode: 'No hash was provided', - message: 'We had an error while submitting this transaction, please try again.', - } - return this.setTxStatusFailed(txId, errReason) + async _checkPendingTxs () { + const signedTxList = this.getFilteredTxList({status: 'submitted'}) + try { + await Promise.all(signedTxList.map((txMeta) => this._checkPendingTx(txMeta))) + } catch (err) { + console.error('TransactionController - Error updating pending transactions') + console.error(err) + } + } + + async _checkPendingTx (txMeta) { + const txHash = txMeta.hash + const txId = txMeta.id + // extra check in case there was an uncaught error during the + // signature and submission process + if (!txHash) { + const errReason = { + errCode: 'No hash was provided', + message: 'We had an error while submitting this transaction, please try again.', } - this.query.getTransactionByHash(txHash, (err, txParams) => { - if (err || !txParams) { - if (!txParams) return - txMeta.err = { - isWarning: true, - errorCode: err, - message: 'There was a problem loading this transaction.', - } - this.updateTx(txMeta) - return log.error(err) + this.setTxStatusFailed(txId, errReason) + return + } + // get latest transaction status + let txParams + try { + txParams = await pify((cb) => this.query.getTransactionByHash(txHash, cb))() + if (!txParams) return + if (txParams.blockNumber) { + this.setTxStatusConfirmed(txId) + } + } catch (err) { + if (err || !txParams) { + txMeta.err = { + isWarning: true, + errorCode: err, + message: 'There was a problem loading this transaction.', } - if (txParams.blockNumber) { - this.setTxStatusConfirmed(txId) - } - }) - }) + this.updateTx(txMeta) + log.error(err) + } + } } } From aa48ed34c458874914c44400fb68885069625a6f Mon Sep 17 00:00:00 2001 From: kumavis Date: Tue, 18 Jul 2017 15:11:29 -0700 Subject: [PATCH 74/80] nonce-tracker - fix lock mechanism to be a real mutex --- app/scripts/lib/nonce-tracker.js | 26 +++++++++++++++----------- package.json | 1 + 2 files changed, 16 insertions(+), 11 deletions(-) diff --git a/app/scripts/lib/nonce-tracker.js b/app/scripts/lib/nonce-tracker.js index ba05fd124..7a450cb78 100644 --- a/app/scripts/lib/nonce-tracker.js +++ b/app/scripts/lib/nonce-tracker.js @@ -1,5 +1,6 @@ const EthQuery = require('eth-query') const assert = require('assert') +const Mutex = require('await-semaphore').Mutex class NonceTracker { @@ -13,10 +14,8 @@ class NonceTracker { // releaseLock must be called // releaseLock must be called after adding signed tx to pending transactions (or discarding) async getNonceLock (address) { - // await lock free - await this.lockMap[address] - // take lock - const releaseLock = this._takeLock(address) + // await lock free, then take lock + const releaseLock = await this._takeMutex(address) // calculate next nonce // we need to make sure our base count // and pending count are from the same block @@ -41,13 +40,18 @@ class NonceTracker { }) } - _takeLock (lockId) { - let releaseLock = null - // create and store lock - const lock = new Promise((resolve, reject) => { releaseLock = resolve }) - this.lockMap[lockId] = lock - // setup lock teardown - lock.then(() => delete this.lockMap[lockId]) + _lookupMutex (lockId) { + let mutex = this.lockMap[lockId] + if (!mutex) { + mutex = new Mutex() + this.lockMap[lockId] = mutex + } + return mutex + } + + async _takeMutex (lockId) { + const mutex = this._lookupMutex(lockId) + const releaseLock = await mutex.acquire() return releaseLock } diff --git a/package.json b/package.json index e0bb303bf..d40ad068b 100644 --- a/package.json +++ b/package.json @@ -47,6 +47,7 @@ }, "dependencies": { "async": "^1.5.2", + "await-semaphore": "^0.1.1", "babel-runtime": "^6.23.0", "bip39": "^2.2.0", "bluebird": "^3.5.0", From 12d6f2162791b421bd51313b0063e144b47ed868 Mon Sep 17 00:00:00 2001 From: kumavis Date: Tue, 18 Jul 2017 15:27:15 -0700 Subject: [PATCH 75/80] transactions - block nonce-tracker while updating pending transactions --- app/scripts/controllers/transactions.js | 3 ++ app/scripts/lib/nonce-tracker.js | 45 ++++++++++++++++--------- 2 files changed, 33 insertions(+), 15 deletions(-) diff --git a/app/scripts/controllers/transactions.js b/app/scripts/controllers/transactions.js index 1fc48aadd..5f3d84ebe 100644 --- a/app/scripts/controllers/transactions.js +++ b/app/scripts/controllers/transactions.js @@ -484,12 +484,15 @@ module.exports = class TransactionController extends EventEmitter { // if confirmed sets the tx status as 'confirmed' async _checkPendingTxs () { const signedTxList = this.getFilteredTxList({status: 'submitted'}) + // in order to keep the nonceTracker accurate we block it while updating pending transactions + const nonceGlobalLock = await this.nonceTracker.getGlobalLock() try { await Promise.all(signedTxList.map((txMeta) => this._checkPendingTx(txMeta))) } catch (err) { console.error('TransactionController - Error updating pending transactions') console.error(err) } + nonceGlobalLock.releaseLock() } async _checkPendingTx (txMeta) { diff --git a/app/scripts/lib/nonce-tracker.js b/app/scripts/lib/nonce-tracker.js index 7a450cb78..b76dac4e8 100644 --- a/app/scripts/lib/nonce-tracker.js +++ b/app/scripts/lib/nonce-tracker.js @@ -11,9 +11,18 @@ class NonceTracker { this.lockMap = {} } + async getGlobalLock () { + const globalMutex = this._lookupMutex('global') + // await global mutex free + const releaseLock = await globalMutex.acquire() + return { releaseLock } + } + // releaseLock must be called // releaseLock must be called after adding signed tx to pending transactions (or discarding) async getNonceLock (address) { + // await global mutex free + await this._globalMutexFree() // await lock free, then take lock const releaseLock = await this._takeMutex(address) // calculate next nonce @@ -40,21 +49,6 @@ class NonceTracker { }) } - _lookupMutex (lockId) { - let mutex = this.lockMap[lockId] - if (!mutex) { - mutex = new Mutex() - this.lockMap[lockId] = mutex - } - return mutex - } - - async _takeMutex (lockId) { - const mutex = this._lookupMutex(lockId) - const releaseLock = await mutex.acquire() - return releaseLock - } - async _getTxCount (address, currentBlock) { const blockNumber = currentBlock.number return new Promise((resolve, reject) => { @@ -64,6 +58,27 @@ class NonceTracker { }) } + async _globalMutexFree () { + const globalMutex = this._lookupMutex('global') + const release = await globalMutex.acquire() + release() + } + + async _takeMutex (lockId) { + const mutex = this._lookupMutex(lockId) + const releaseLock = await mutex.acquire() + return releaseLock + } + + _lookupMutex (lockId) { + let mutex = this.lockMap[lockId] + if (!mutex) { + mutex = new Mutex() + this.lockMap[lockId] = mutex + } + return mutex + } + } module.exports = NonceTracker From 2832713a46f74131ecf8111a49572ec98dd73bb6 Mon Sep 17 00:00:00 2001 From: kumavis Date: Tue, 18 Jul 2017 15:30:32 -0700 Subject: [PATCH 76/80] changelog - add nonce-tracker note --- CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 5d15b3512..4d265d318 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,6 +6,7 @@ - Now redirects from known malicious sites faster. - Added a link to our new support page to the help screen. - Fixed bug where a new transaction would be shown over the current transaction, creating a possible timing attack against user confirmation. +- Fixed bug in nonce tracker where an incorrect nonce would be calculated. ## 3.9.0 2017-7-12 From 51c5bebdf5ec4c4ac0e2878bd503e39a379d79c2 Mon Sep 17 00:00:00 2001 From: Dan Finlay Date: Wed, 19 Jul 2017 11:44:00 -0700 Subject: [PATCH 77/80] Lowered minimum gas price to 1 gwei --- CHANGELOG.md | 1 + ui/app/components/pending-tx.js | 2 +- 2 files changed, 2 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 4d265d318..e68a7ab89 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,7 @@ - Added a link to our new support page to the help screen. - Fixed bug where a new transaction would be shown over the current transaction, creating a possible timing attack against user confirmation. - Fixed bug in nonce tracker where an incorrect nonce would be calculated. +- Lowered minimum gas price to 1 Gwei. ## 3.9.0 2017-7-12 diff --git a/ui/app/components/pending-tx.js b/ui/app/components/pending-tx.js index d7d602f31..5324ccd64 100644 --- a/ui/app/components/pending-tx.js +++ b/ui/app/components/pending-tx.js @@ -15,7 +15,7 @@ const addressSummary = util.addressSummary const nameForAddress = require('../../lib/contract-namer') const BNInput = require('./bn-as-decimal-input') -const MIN_GAS_PRICE_GWEI_BN = new BN(2) +const MIN_GAS_PRICE_GWEI_BN = new BN(1) const GWEI_FACTOR = new BN(1e9) const MIN_GAS_PRICE_BN = MIN_GAS_PRICE_GWEI_BN.mul(GWEI_FACTOR) const MIN_GAS_LIMIT_BN = new BN(21000) From dcf025782b01265710a61a63da60d7d006df06b0 Mon Sep 17 00:00:00 2001 From: Dan Finlay Date: Wed, 19 Jul 2017 11:56:53 -0700 Subject: [PATCH 78/80] Version 3.9.1 --- CHANGELOG.md | 2 ++ app/manifest.json | 2 +- package.json | 2 +- 3 files changed, 4 insertions(+), 2 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index e68a7ab89..bf18bb361 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,6 +2,8 @@ ## Current Master +## 3.9.1 2017-7-19 + - No longer automatically request 1 ropsten ether for the first account in a new vault. - Now redirects from known malicious sites faster. - Added a link to our new support page to the help screen. diff --git a/app/manifest.json b/app/manifest.json index 7bf757d4c..eadd99590 100644 --- a/app/manifest.json +++ b/app/manifest.json @@ -1,7 +1,7 @@ { "name": "MetaMask", "short_name": "Metamask", - "version": "3.9.0", + "version": "3.9.1", "manifest_version": 2, "author": "https://metamask.io", "description": "Ethereum Browser Extension", diff --git a/package.json b/package.json index d40ad068b..375902d09 100644 --- a/package.json +++ b/package.json @@ -67,7 +67,7 @@ "eth-contract-metadata": "^1.1.4", "eth-hd-keyring": "^1.1.1", "eth-query": "^2.1.2", - "eth-sig-util": "^1.1.1", + "eth-sig-util": "^1.2.2", "eth-simple-keyring": "^1.1.1", "eth-token-tracker": "^1.1.2", "etheraddresslookup": "github:409H/EtherAddressLookup", From 9018810ac20e7f73a34af564f208231196179f57 Mon Sep 17 00:00:00 2001 From: Dan Finlay Date: Thu, 20 Jul 2017 09:55:10 -0700 Subject: [PATCH 79/80] Update tutorial links --- README.md | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/README.md b/README.md index 45fb68c78..bf1f37d1f 100644 --- a/README.md +++ b/README.md @@ -4,7 +4,13 @@ If you're a web dapp developer, we've got two types of guides for you: -- If you've never built a Dapp before, we've got a gentle introduction on [Developing Dapps with Truffle and MetaMask](https://blog.metamask.io/developing-for-metamask-with-truffle/). +### New Dapp Developers + +- We recommend this [Learning Solidity](https://karl.tech/learning-solidity-part-1-deploy-a-contract/) tutorial series by Karl Floersch. +- We wrote a (slightly outdated now) gentle introduction on [Developing Dapps with Truffle and MetaMask](https://medium.com/metamask/developing-ethereum-dapps-with-truffle-and-metamask-aa8ad7e363ba). + +### Current Dapp Developers + - If you have a Dapp, and you want to ensure compatibility, [here is our guide on building MetaMask-compatible Dapps](https://github.com/MetaMask/faq/blob/master/DEVELOPERS.md) ## Building locally From 366ddc62488c99494b3973e157bf9a1c9d5af0cb Mon Sep 17 00:00:00 2001 From: Dan Finlay Date: Thu, 20 Jul 2017 09:55:47 -0700 Subject: [PATCH 80/80] Add user support link --- README.md | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/README.md b/README.md index bf1f37d1f..d7086ae91 100644 --- a/README.md +++ b/README.md @@ -1,5 +1,9 @@ # MetaMask Plugin [![Build Status](https://circleci.com/gh/MetaMask/metamask-extension.svg?style=shield&circle-token=a1ddcf3cd38e29267f254c9c59d556d513e3a1fd)](https://circleci.com/gh/MetaMask/metamask-extension) +## Support + +If you're a user seeking support, [here is our support site](http://metamask.consensyssupport.happyfox.com). + ## Developing Compatible Dapps If you're a web dapp developer, we've got two types of guides for you: