diff options
| -rw-r--r-- | CHANGELOG.md | 3 | ||||
| -rw-r--r-- | app/scripts/controllers/transactions.js | 36 | ||||
| -rw-r--r-- | app/scripts/lib/tx-utils.js | 9 | ||||
| -rw-r--r-- | app/scripts/metamask-controller.js | 2 | ||||
| -rw-r--r-- | app/scripts/migrations/016.js | 41 | ||||
| -rw-r--r-- | app/scripts/migrations/index.js | 1 | ||||
| -rw-r--r-- | package.json | 2 | ||||
| -rw-r--r-- | test/unit/infura-controller-test.js | 66 | ||||
| -rw-r--r-- | test/unit/tx-utils-test.js | 38 |
9 files changed, 143 insertions, 55 deletions
diff --git a/CHANGELOG.md b/CHANGELOG.md index 2c61c31b9..cb3fcfb83 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,6 +2,9 @@ ## 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.5 2017-7-7 - Fix transaction resubmit logic to fail slightly less eagerly. diff --git a/app/scripts/controllers/transactions.js b/app/scripts/controllers/transactions.js index 41d70194e..43735a691 100644 --- a/app/scripts/controllers/transactions.js +++ b/app/scripts/controllers/transactions.js @@ -417,46 +417,42 @@ 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' - || errorMessage.startsWith('known transaction') + errorMessage.includes('replacement transaction underpriced') + || errorMessage.includes('known transaction') // parity - || errorMessage === 'gas price too low to replace' + || errorMessage.includes('gas price too low to replace') + || errorMessage.includes('transaction with the same hash was already imported') + // other + || errorMessage.includes('gateway timeout') ) // 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, + }) })) } _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() - 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) + this.setTxStatusFailed(txMeta.id, { message }) cb() return log.error(message) } 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/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) 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'), ] diff --git a/package.json b/package.json index 27fe7a84a..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.1.1", + "web3-provider-engine": "^13.2.8", "web3-stream-provider": "^3.0.1", "xtend": "^4.0.1" }, 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) +// +// }) +// }) +// }) +// }) 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 = { |