diff options
-rw-r--r-- | app/scripts/controllers/transactions.js | 90 | ||||
-rw-r--r-- | app/scripts/lib/nonce-tracker.js | 49 | ||||
-rw-r--r-- | app/scripts/metamask-controller.js | 2 | ||||
-rw-r--r-- | test/unit/tx-controller-test.js | 24 |
4 files changed, 118 insertions, 47 deletions
diff --git a/app/scripts/controllers/transactions.js b/app/scripts/controllers/transactions.js index d9d9849b1..c2f98e66a 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('rawBlock', this.checkForTxInBlock.bind(this)) @@ -170,29 +176,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) { @@ -200,15 +235,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/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 diff --git a/app/scripts/metamask-controller.js b/app/scripts/metamask-controller.js index de9a15924..755bf3289 100644 --- a/app/scripts/metamask-controller.js +++ b/app/scripts/metamask-controller.js @@ -295,7 +295,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 0d35cd62c..8ce6a5a65 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,14 +18,15 @@ describe('Transaction Controller', function () { txController = new TransactionController({ networkStore: new ObservableStore(currentNetworkId), txHistoryLimit: 10, - provider: { _blockTracker: new EventEmitter()}, - 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 () { @@ -270,26 +270,25 @@ 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) 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) - txController.approveTransaction(txMeta.id, (err) => { + txController.approveTransaction(txMeta.id).then((err) => { assert.ifError(err, 'should not error') const result = txController.getTx(txMeta.id) @@ -297,15 +296,12 @@ 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() }) }) |