diff --git a/CHANGELOG.md b/CHANGELOG.md index 3966ea1bb..395454b41 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,6 +2,13 @@ ## 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 + +- 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", diff --git a/app/scripts/controllers/transactions.js b/app/scripts/controllers/transactions.js index fb0219fc3..45c6fb25a 100644 --- a/app/scripts/controllers/transactions.js +++ b/app/scripts/controllers/transactions.js @@ -432,46 +432,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/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/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 = {