From 8441117ac3c16a5522c40be4f0fedb21d45b5634 Mon Sep 17 00:00:00 2001 From: Victor Baranov Date: Fri, 27 Mar 2020 23:08:21 +0300 Subject: [PATCH] Refactoring 3 before EIP - 1193 --- app/scripts/controllers/transactions/index.js | 17 +- .../controllers/transactions/nonce-tracker.js | 161 ------------ .../transactions/tx-state-manager.js | 201 +++++++++------ app/scripts/lib/cleanErrorStack.js | 8 +- package-lock.json | 15 +- package.json | 1 + test/unit/app/cleanErrorStack.spec.js | 16 +- .../transactions/nonce-tracker-test.js | 238 ------------------ .../transactions/tx-controller-test.js | 27 +- .../transactions/tx-state-manager-test.js | 92 +++++-- 10 files changed, 241 insertions(+), 535 deletions(-) delete mode 100644 app/scripts/controllers/transactions/nonce-tracker.js delete mode 100644 test/unit/app/controllers/transactions/nonce-tracker-test.js diff --git a/app/scripts/controllers/transactions/index.js b/app/scripts/controllers/transactions/index.js index 325322c13..91178b21d 100644 --- a/app/scripts/controllers/transactions/index.js +++ b/app/scripts/controllers/transactions/index.js @@ -12,9 +12,9 @@ abiDecoder.addABI(abi) import TransactionStateManager from './tx-state-manager' const TxGasUtil = require('./tx-gas-utils') const PendingTransactionTracker = require('./pending-tx-tracker') -const NonceTracker = require('./nonce-tracker') +import NonceTracker from 'nonce-tracker' import * as txUtils from './lib/util' -const cleanErrorStack = require('../../lib/cleanErrorStack') +import cleanErrorStack from '../../lib/cleanErrorStack' import log from 'loglevel' const recipientBlacklistChecker = require('./lib/recipient-blacklist-checker') const { @@ -228,18 +228,9 @@ class TransactionController extends EventEmitter { @return {txMeta} */ - async retryTransaction (originalTxId, gasPrice) { + async retryTransaction (originalTxId) { const originalTxMeta = this.txStateManager.getTx(originalTxId) - const { txParams } = originalTxMeta - const lastGasPrice = gasPrice || originalTxMeta.txParams.gasPrice - const suggestedGasPriceBN = new ethUtil.BN(ethUtil.stripHexPrefix(this.getGasPrice()), 16) - const lastGasPriceBN = new ethUtil.BN(ethUtil.stripHexPrefix(lastGasPrice), 16) - // essentially lastGasPrice * 1.1 but - // dont trust decimals so a round about way of doing that - const lastGasPriceBNBumped = lastGasPriceBN.mul(new ethUtil.BN(110, 10)).div(new ethUtil.BN(100, 10)) - // transactions that are being retried require a >=%10 bump or the clients will throw an error - txParams.gasPrice = suggestedGasPriceBN.gt(lastGasPriceBNBumped) ? `0x${suggestedGasPriceBN.toString(16)}` : `0x${lastGasPriceBNBumped.toString(16)}` - + const lastGasPrice = originalTxMeta.txParams.gasPrice const txMeta = this.txStateManager.generateTxMeta({ txParams: originalTxMeta.txParams, lastGasPrice, diff --git a/app/scripts/controllers/transactions/nonce-tracker.js b/app/scripts/controllers/transactions/nonce-tracker.js deleted file mode 100644 index 421036368..000000000 --- a/app/scripts/controllers/transactions/nonce-tracker.js +++ /dev/null @@ -1,161 +0,0 @@ -const EthQuery = require('ethjs-query') -const assert = require('assert') -const Mutex = require('await-semaphore').Mutex -/** - @param opts {Object} - @param {Object} opts.provider a ethereum provider - @param {Function} opts.getPendingTransactions a function that returns an array of txMeta - whosee status is `submitted` - @param {Function} opts.getConfirmedTransactions a function that returns an array of txMeta - whose status is `confirmed` - @class -*/ -class NonceTracker { - - constructor ({ provider, blockTracker, getPendingTransactions, getConfirmedTransactions }) { - this.provider = provider - this.blockTracker = blockTracker - this.ethQuery = new EthQuery(provider) - this.getPendingTransactions = getPendingTransactions - this.getConfirmedTransactions = getConfirmedTransactions - this.lockMap = {} - } - - /** - @returns {Promise} with the key releaseLock (the gloabl mutex) - */ - async getGlobalLock () { - const globalMutex = this._lookupMutex('global') - // await global mutex free - const releaseLock = await globalMutex.acquire() - return { releaseLock } - } - - /** - * @typedef NonceDetails - * @property {number} highestLocallyConfirmed - A hex string of the highest nonce on a confirmed transaction. - * @property {number} nextNetworkNonce - The next nonce suggested by the eth_getTransactionCount method. - * @property {number} highestSuggested - The maximum between the other two, the number returned. - */ - - /** - this will return an object with the `nextNonce` `nonceDetails` of type NonceDetails, and the releaseLock - Note: releaseLock must be called after adding a signed tx to pending transactions (or discarding). - - @param address {string} the hex string for the address whose nonce we are calculating - @returns {Promise} - */ - async getNonceLock (address) { - // await global mutex free - await this._globalMutexFree() - // await lock free, then take lock - const releaseLock = await this._takeMutex(address) - try { - // evaluate multiple nextNonce strategies - const nonceDetails = {} - const networkNonceResult = await this._getNetworkNextNonce(address) - const highestLocallyConfirmed = this._getHighestLocallyConfirmed(address) - const nextNetworkNonce = networkNonceResult.nonce - const highestSuggested = Math.max(nextNetworkNonce, highestLocallyConfirmed) - - const pendingTxs = this.getPendingTransactions(address) - const localNonceResult = this._getHighestContinuousFrom(pendingTxs, highestSuggested) || 0 - - nonceDetails.params = { - highestLocallyConfirmed, - highestSuggested, - nextNetworkNonce, - } - nonceDetails.local = localNonceResult - nonceDetails.network = networkNonceResult - - const nextNonce = Math.max(networkNonceResult.nonce, localNonceResult.nonce) - assert(Number.isInteger(nextNonce), `nonce-tracker - nextNonce is not an integer - got: (${typeof nextNonce}) "${nextNonce}"`) - - // return nonce and release cb - return { nextNonce, nonceDetails, releaseLock } - } catch (err) { - // release lock if we encounter an error - releaseLock() - throw err - } - } - - async _globalMutexFree () { - const globalMutex = this._lookupMutex('global') - const releaseLock = await globalMutex.acquire() - releaseLock() - } - - 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 - } - - async _getNetworkNextNonce (address) { - // calculate next nonce - // we need to make sure our base count - // and pending count are from the same block - const blockNumber = await this.blockTracker.getLatestBlock() - const baseCountBN = await this.ethQuery.getTransactionCount(address, blockNumber) - const baseCount = baseCountBN.toNumber() - assert(Number.isInteger(baseCount), `nonce-tracker - baseCount is not an integer - got: (${typeof baseCount}) "${baseCount}"`) - const nonceDetails = { blockNumber, baseCount } - return { name: 'network', nonce: baseCount, details: nonceDetails } - } - - _getHighestLocallyConfirmed (address) { - const confirmedTransactions = this.getConfirmedTransactions(address) - const highest = this._getHighestNonce(confirmedTransactions) - return Number.isInteger(highest) ? highest + 1 : 0 - } - - _getHighestNonce (txList) { - const nonces = txList.map((txMeta) => { - const nonce = txMeta.txParams.nonce - assert(typeof nonce, 'string', 'nonces should be hex strings') - return parseInt(nonce, 16) - }) - const highestNonce = Math.max.apply(null, nonces) - return highestNonce - } - - /** - @typedef {object} highestContinuousFrom - @property {string} - name the name for how the nonce was calculated based on the data used - @property {number} - nonce the next suggested nonce - @property {object} - details the provided starting nonce that was used (for debugging) - */ - /** - @param txList {array} - list of txMeta's - @param startPoint {number} - the highest known locally confirmed nonce - @returns {highestContinuousFrom} - */ - _getHighestContinuousFrom (txList, startPoint) { - const nonces = txList.map((txMeta) => { - const nonce = txMeta.txParams.nonce - assert(typeof nonce, 'string', 'nonces should be hex strings') - return parseInt(nonce, 16) - }) - - let highest = startPoint - while (nonces.includes(highest)) { - highest++ - } - - return { name: 'local', nonce: highest, details: { startPoint, highest } } - } - -} - -module.exports = NonceTracker diff --git a/app/scripts/controllers/transactions/tx-state-manager.js b/app/scripts/controllers/transactions/tx-state-manager.js index aa795565d..cbb57e0ed 100644 --- a/app/scripts/controllers/transactions/tx-state-manager.js +++ b/app/scripts/controllers/transactions/tx-state-manager.js @@ -1,11 +1,9 @@ -import ethUtil from 'ethereumjs-util' -import extend from 'extend' import EventEmitter from 'safe-event-emitter' import ObservableStore from 'obs-store' import log from 'loglevel' import txStateHistoryHelper from './lib/tx-state-history-helper' import createId from '../../lib/random-id' -import { getFinalStates } from './lib/util' +import { getFinalStates, normalizeTxParams } from './lib/util' /** TransactionStateManager is responsible for the state of a transaction and storing the transaction @@ -21,8 +19,8 @@ import { getFinalStates } from './lib/util'
- `'confirmed'` the tx has been included in a block.
- `'failed'` the tx failed for some reason, included on tx data.
- `'dropped'` the tx nonce was already used - @param opts {object} - @param {object} [opts.initState={ transactions: [] }] initial transactions list with the key transaction {array} + @param {Object} opts + @param {Object} [opts.initState={ transactions: [] }] initial transactions list with the key transaction {array} @param {number} [opts.txHistoryLimit] limit for how many finished transactions can hang around in state @param {function} opts.getNetwork return network number @@ -33,29 +31,33 @@ class TransactionStateManager extends EventEmitter { super() this.store = new ObservableStore( - extend({ + Object.assign({ transactions: [], - }, initState)) + }, initState)) this.txHistoryLimit = txHistoryLimit this.getNetwork = getNetwork } /** - @param opts {object} - the object to use when overwriting defaults - @returns {txMeta} the default txMeta object + @param {Object} opts - the object to use when overwriting defaults + @returns {txMeta} - the default txMeta object */ generateTxMeta (opts) { - return extend({ + const netId = this.getNetwork() + if (netId === 'loading') { + throw new Error('MetaMask is having trouble connecting to the network') + } + return Object.assign({ id: createId(), time: (new Date()).getTime(), status: 'unapproved', - metamaskNetworkId: this.getNetwork(), + metamaskNetworkId: netId, loadingDefaults: true, }, opts) } /** - @returns {array} of txMetas that have been filtered for only the current network + @returns {array} - of txMetas that have been filtered for only the current network */ getTxList () { const network = this.getNetwork() @@ -64,14 +66,14 @@ class TransactionStateManager extends EventEmitter { } /** - @returns {array} of all the txMetas in store + @returns {array} - of all the txMetas in store */ getFullTxList () { return this.store.getState().transactions } /** - @returns {array} the tx list whos status is unapproved + @returns {array} - the tx list whos status is unapproved */ getUnapprovedTxList () { const txList = this.getTxsByMetaData('status', 'unapproved') @@ -83,23 +85,40 @@ class TransactionStateManager extends EventEmitter { /** @param [address] {string} - hex prefixed address to sort the txMetas for [optional] - @returns {array} the tx list whos status is submitted if no address is provide - returns all txMetas who's status is submitted for the current network + @returns {array} - the tx list whos status is approved if no address is provide + returns all txMetas who's status is approved for the current network */ - getPendingTransactions (address) { - const opts = { status: 'submitted' } - if (address) opts.from = address + getApprovedTransactions (address) { + const opts = { status: 'approved' } + if (address) { + opts.from = address + } return this.getFilteredTxList(opts) } /** @param [address] {string} - hex prefixed address to sort the txMetas for [optional] - @returns {array} the tx list whos status is confirmed if no address is provide + @returns {array} - the tx list whos status is submitted if no address is provide + returns all txMetas who's status is submitted for the current network + */ + getPendingTransactions (address) { + const opts = { status: 'submitted' } + if (address) { + opts.from = address + } + return this.getFilteredTxList(opts) + } + + /** + @param [address] {string} - hex prefixed address to sort the txMetas for [optional] + @returns {array} - the tx list whos status is confirmed if no address is provide returns all txMetas who's status is confirmed for the current network */ getConfirmedTransactions (address) { const opts = { status: 'confirmed' } - if (address) opts.from = address + if (address) { + opts.from = address + } return this.getFilteredTxList(opts) } @@ -109,14 +128,19 @@ class TransactionStateManager extends EventEmitter { is in its final state it will allso add the key `history` to the txMeta with the snap shot of the original object - @param txMeta {Object} - @returns {object} the txMeta + @param {Object} txMeta + @returns {Object} - the txMeta */ addTx (txMeta) { - this.once(`${txMeta.id}:signed`, function (txId) { + // normalize and validate txParams if present + if (txMeta.txParams) { + txMeta.txParams = this.normalizeAndValidateTxParams(txMeta.txParams) + } + + this.once(`${txMeta.id}:signed`, function () { this.removeAllListeners(`${txMeta.id}:rejected`) }) - this.once(`${txMeta.id}:rejected`, function (txId) { + this.once(`${txMeta.id}:rejected`, function () { this.removeAllListeners(`${txMeta.id}:signed`) }) // initialize history @@ -142,13 +166,18 @@ class TransactionStateManager extends EventEmitter { transactions.splice(index, 1) } } - transactions.push(txMeta) + const newTxIndex = transactions + .findIndex((currentTxMeta) => currentTxMeta.time > txMeta.time) + + newTxIndex === -1 + ? transactions.push(txMeta) + : transactions.splice(newTxIndex, 0, txMeta) this._saveTxList(transactions) return txMeta } /** - @param txId {number} - @returns {object} the txMeta who matches the given id if none found + @param {number} txId + @returns {Object} - the txMeta who matches the given id if none found for the network returns undefined */ getTx (txId) { @@ -158,17 +187,13 @@ class TransactionStateManager extends EventEmitter { /** updates the txMeta in the list and adds a history entry - @param txMeta {Object} - the txMeta to update - @param [note] {string} - a note about the update for history + @param {Object} txMeta - the txMeta to update + @param {string} [note] - a note about the update for history */ updateTx (txMeta, note) { - // validate txParams + // normalize and validate txParams if present if (txMeta.txParams) { - if (typeof txMeta.txParams.data === 'undefined') { - delete txMeta.txParams.data - } - - this.validateTxParams(txMeta.txParams) + txMeta.txParams = this.normalizeAndValidateTxParams(txMeta.txParams) } // create txMeta snapshot for history @@ -182,7 +207,7 @@ class TransactionStateManager extends EventEmitter { // commit txMeta to state const txId = txMeta.id const txList = this.getFullTxList() - const index = txList.findIndex(txData => txData.id === txId) + const index = txList.findIndex((txData) => txData.id === txId) txList[index] = txMeta this._saveTxList(txList) } @@ -191,18 +216,31 @@ class TransactionStateManager extends EventEmitter { /** merges txParams obj onto txMeta.txParams use extend to ensure that all fields are filled - @param txId {number} - the id of the txMeta - @param txParams {object} - the updated txParams + @param {number} txId - the id of the txMeta + @param {Object} txParams - the updated txParams */ updateTxParams (txId, txParams) { const txMeta = this.getTx(txId) - txMeta.txParams = extend(txMeta.txParams, txParams) + txMeta.txParams = { ...txMeta.txParams, ...txParams } this.updateTx(txMeta, `txStateManager#updateTxParams`) } + /** + * normalize and validate txParams members + * @param {Object} txParams - txParams + */ + normalizeAndValidateTxParams (txParams) { + if (typeof txParams.data === 'undefined') { + delete txParams.data + } + txParams = normalizeTxParams(txParams, false) + this.validateTxParams(txParams) + return txParams + } + /** validates txParams members by type - @param txParams {object} - txParams to validate + @param {Object} txParams - txParams to validate */ validateTxParams (txParams) { Object.keys(txParams).forEach((key) => { @@ -210,26 +248,31 @@ class TransactionStateManager extends EventEmitter { // validate types switch (key) { case 'chainId': - if (typeof value !== 'number' && typeof value !== 'string') throw new Error(`${key} in txParams is not a Number or hex string. got: (${value})`) + if (typeof value !== 'number' && typeof value !== 'string') { + throw new Error(`${key} in txParams is not a Number or hex string. got: (${value})`) + } break default: - if (typeof value !== 'string') throw new Error(`${key} in txParams is not a string. got: (${value})`) - if (!ethUtil.isHexPrefixed(value)) throw new Error(`${key} in txParams is not hex prefixed. got: (${value})`) + if (typeof value !== 'string') { + throw new Error(`${key} in txParams is not a string. got: (${value})`) + } break } }) } -/** - @param opts {object} - an object of fields to search for eg:
+ /** + @param {Object} opts - an object of fields to search for eg:
let thingsToLookFor = {
to: '0x0..',
from: '0x0..',
- status: 'signed',
+ status: 'signed', \\ (status) => status !== 'rejected' give me all txs who's status is not rejected
err: undefined,
}
 + optionally the values of the keys can be functions for situations like where + you want all but one status. @param [initialList=this.getTxList()] - @returns a {array} of txMeta with all + @returns {array} - array of txMeta with all options matching */ /* @@ -243,7 +286,7 @@ class TransactionStateManager extends EventEmitter { 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 + or for filtering for all txs from one account and that have been 'confirmed' */ getFilteredTxList (opts, initialList) { @@ -255,18 +298,20 @@ class TransactionStateManager extends EventEmitter { } /** - @param key {string} - the key to check - @param value - the value your looking for + @param {string} key - the key to check + @param value - the value your looking for can also be a function that returns a bool @param [txList=this.getTxList()] {array} - the list to search. default is the txList from txStateManager#getTxList - @returns {array} a list of txMetas who matches the search params + @returns {array} - a list of txMetas who matches the search params */ getTxsByMetaData (key, value, txList = this.getTxList()) { + const filter = typeof value === 'function' ? value : (v) => v === value + return txList.filter((txMeta) => { if (key in txMeta.txParams) { - return txMeta.txParams[key] === value + return filter(txMeta.txParams[key]) } else { - return txMeta[key] === value + return filter(txMeta[key]) } }) } @@ -274,8 +319,8 @@ class TransactionStateManager extends EventEmitter { // get::set status /** - @param txId {number} - the txMeta Id - @return {string} the status of the tx. + @param {number} txId - the txMeta Id + @returns {string} - the status of the tx. */ getTxStatus (txId) { const txMeta = this.getTx(txId) @@ -284,7 +329,7 @@ class TransactionStateManager extends EventEmitter { /** should update the status of the tx to 'rejected'. - @param txId {number} - the txMeta Id + @param {number} txId - the txMeta Id */ setTxStatusRejected (txId) { this._setTxStatus(txId, 'rejected') @@ -293,14 +338,14 @@ class TransactionStateManager extends EventEmitter { /** should update the status of the tx to 'unapproved'. - @param txId {number} - the txMeta Id + @param {number} txId - the txMeta Id */ setTxStatusUnapproved (txId) { this._setTxStatus(txId, 'unapproved') } /** should update the status of the tx to 'approved'. - @param txId {number} - the txMeta Id + @param {number} txId - the txMeta Id */ setTxStatusApproved (txId) { this._setTxStatus(txId, 'approved') @@ -308,7 +353,7 @@ class TransactionStateManager extends EventEmitter { /** should update the status of the tx to 'signed'. - @param txId {number} - the txMeta Id + @param {number} txId - the txMeta Id */ setTxStatusSigned (txId) { this._setTxStatus(txId, 'signed') @@ -317,7 +362,7 @@ class TransactionStateManager extends EventEmitter { /** should update the status of the tx to 'submitted'. and add a time stamp for when it was called - @param txId {number} - the txMeta Id + @param {number} txId - the txMeta Id */ setTxStatusSubmitted (txId) { const txMeta = this.getTx(txId) @@ -328,7 +373,7 @@ class TransactionStateManager extends EventEmitter { /** should update the status of the tx to 'confirmed'. - @param txId {number} - the txMeta Id + @param {number} txId - the txMeta Id */ setTxStatusConfirmed (txId) { this._setTxStatus(txId, 'confirmed') @@ -336,7 +381,7 @@ class TransactionStateManager extends EventEmitter { /** should update the status of the tx to 'dropped'. - @param txId {number} - the txMeta Id + @param {number} txId - the txMeta Id */ setTxStatusDropped (txId) { this._setTxStatus(txId, 'dropped') @@ -346,24 +391,26 @@ class TransactionStateManager extends EventEmitter { /** should update the status of the tx to 'failed'. and put the error on the txMeta - @param txId {number} - the txMeta Id - @param err {erroObject} - error object + @param {number} txId - the txMeta Id + @param {erroObject} err - error object */ setTxStatusFailed (txId, err) { + const error = !err ? new Error('Internal metamask failure') : err + const txMeta = this.getTx(txId) txMeta.err = { - message: (err ? (err.message || err.error || err) : '').toString(), - rpc: err.value, - stack: err.stack, + message: error.toString(), + rpc: error.value, + stack: error.stack, } - this.updateTx(txMeta) + this.updateTx(txMeta, 'transactions:tx-state-manager#fail - add error') this._setTxStatus(txId, 'failed') } /** Removes transaction from the given address for the current network from the txList - @param address {string} - hex string of the from address on the txParams to remove + @param {string} address - hex string of the from address on the txParams to remove */ wipeTransactions (address) { // network only tx @@ -376,9 +423,9 @@ class TransactionStateManager extends EventEmitter { // Update state this._saveTxList(otherAccountTxs) } -// -// PRIVATE METHODS -// + // + // PRIVATE METHODS + // // STATUS METHODS // statuses: @@ -392,8 +439,8 @@ class TransactionStateManager extends EventEmitter { // - `'dropped'` the tx nonce was already used /** - @param txId {number} - the txMeta Id - @param status {string} - the status to set on the txMeta + @param {number} txId - the txMeta Id + @param {string} status - the status to set on the txMeta @emits tx:status-update - passes txId and status @emits ${txMeta.id}:finished - if it is a finished state. Passes the txMeta @emits update:badge @@ -423,7 +470,7 @@ class TransactionStateManager extends EventEmitter { /** Saves the new/updated txList. - @param transactions {array} - the list of transactions to save + @param {array} transactions - the list of transactions to save */ // Function is intended only for internal use _saveTxList (transactions) { @@ -436,4 +483,4 @@ class TransactionStateManager extends EventEmitter { } } -module.exports = TransactionStateManager +export default TransactionStateManager diff --git a/app/scripts/lib/cleanErrorStack.js b/app/scripts/lib/cleanErrorStack.js index 8adf55db7..daa5719bb 100644 --- a/app/scripts/lib/cleanErrorStack.js +++ b/app/scripts/lib/cleanErrorStack.js @@ -1,13 +1,13 @@ /** * Returns error without stack trace for better UI display * @param {Error} err - error - * @returns {Error} Error with clean stack trace. + * @returns {Error} - Error with clean stack trace. */ function cleanErrorStack (err) { - var name = err.name + let name = err.name name = (name === undefined) ? 'Error' : String(name) - var msg = err.message + let msg = err.message msg = (msg === undefined) ? '' : String(msg) if (name === '') { @@ -21,4 +21,4 @@ function cleanErrorStack (err) { return err } -module.exports = cleanErrorStack +export default cleanErrorStack diff --git a/package-lock.json b/package-lock.json index 30ed148cf..1e2231c3e 100644 --- a/package-lock.json +++ b/package-lock.json @@ -4674,7 +4674,6 @@ "version": "1.5.0", "resolved": "https://registry.npmjs.org/assert/-/assert-1.5.0.tgz", "integrity": "sha512-EDsgawzwoun2CZkCgtxJbv392v4nbk9XDD06zI+kQYoBM/3RBWLlEyJARDOmhAAosBjWACEkKL6S+lIZtcAubA==", - "dev": true, "requires": { "object-assign": "^4.1.1", "util": "0.10.3" @@ -4683,14 +4682,12 @@ "inherits": { "version": "2.0.1", "resolved": "https://registry.npmjs.org/inherits/-/inherits-2.0.1.tgz", - "integrity": "sha1-sX0I0ya0Qj5Wjv9xn5GwscvfafE=", - "dev": true + "integrity": "sha1-sX0I0ya0Qj5Wjv9xn5GwscvfafE=" }, "util": { "version": "0.10.3", "resolved": "https://registry.npmjs.org/util/-/util-0.10.3.tgz", "integrity": "sha1-evsa/lCAUkZInj23/g7TeTNqwPk=", - "dev": true, "requires": { "inherits": "2.0.1" } @@ -45273,6 +45270,16 @@ "integrity": "sha1-WuVUHQJGRdMqWPzdyc7s6nrjrC8=", "dev": true }, + "nonce-tracker": { + "version": "1.0.0", + "resolved": "https://registry.npmjs.org/nonce-tracker/-/nonce-tracker-1.0.0.tgz", + "integrity": "sha512-hxKokxgLvOZx9A5qPQKwL34G1/YwMC5xJWZHFUKfvwxypkn2nP0KVJjbcoXwY6pXsRRa11KdFEPW61N4YCGnWQ==", + "requires": { + "assert": "^1.4.1", + "await-semaphore": "^0.1.3", + "ethjs-query": "^0.3.8" + } + }, "nopt": { "version": "4.0.3", "resolved": "https://registry.npmjs.org/nopt/-/nopt-4.0.3.tgz", diff --git a/package.json b/package.json index 3c4f20846..820633ac8 100644 --- a/package.json +++ b/package.json @@ -157,6 +157,7 @@ "multihashes": "^0.4.12", "nanoid": "^2.1.6", "nifty-wallet-inpage-provider": "github:poanetwork/nifty-wallet-inpage-provider#1.2.3", + "nonce-tracker": "^1.0.0", "number-to-bn": "^1.7.0", "obj-multiplex": "^1.0.0", "obs-store": "^4.0.3", diff --git a/test/unit/app/cleanErrorStack.spec.js b/test/unit/app/cleanErrorStack.spec.js index 7a1ab1ed8..fa793b605 100644 --- a/test/unit/app/cleanErrorStack.spec.js +++ b/test/unit/app/cleanErrorStack.spec.js @@ -1,7 +1,7 @@ -const assert = require('assert') -const cleanErrorStack = require('../../../app/scripts/lib/cleanErrorStack') +import assert from 'assert' +import cleanErrorStack from '../../../app/scripts/lib/cleanErrorStack' -describe('Clean Error Stack', () => { +describe('Clean Error Stack', function () { const testMessage = 'Test Message' const testError = new Error(testMessage) @@ -9,24 +9,24 @@ describe('Clean Error Stack', () => { const blankErrorName = new Error(testMessage) const blankMsgError = new Error() - beforeEach(() => { + beforeEach(function () { undefinedErrorName.name = undefined blankErrorName.name = '' }) - it('tests error with message', () => { + it('tests error with message', function () { assert.equal(cleanErrorStack(testError), 'Error: Test Message') }) - it('tests error with undefined name', () => { + it('tests error with undefined name', function () { assert.equal(cleanErrorStack(undefinedErrorName).toString(), 'Error: Test Message') }) - it('tests error with blank name', () => { + it('tests error with blank name', function () { assert.equal(cleanErrorStack(blankErrorName).toString(), 'Test Message') }) - it('tests error with blank message', () => { + it('tests error with blank message', function () { assert.equal(cleanErrorStack(blankMsgError), 'Error') }) diff --git a/test/unit/app/controllers/transactions/nonce-tracker-test.js b/test/unit/app/controllers/transactions/nonce-tracker-test.js deleted file mode 100644 index 51ac390e9..000000000 --- a/test/unit/app/controllers/transactions/nonce-tracker-test.js +++ /dev/null @@ -1,238 +0,0 @@ -const assert = require('assert') -const NonceTracker = require('../../../../../app/scripts/controllers/transactions/nonce-tracker') -const MockTxGen = require('../../../../lib/mock-tx-gen') -const providerResultStub = {} - -describe('Nonce Tracker', function () { - let nonceTracker, pendingTxs, confirmedTxs - - describe('#getNonceLock', function () { - - describe('with 3 confirmed and 1 pending', function () { - beforeEach(function () { - const txGen = new MockTxGen() - confirmedTxs = txGen.generate({ status: 'confirmed' }, { count: 3 }) - pendingTxs = txGen.generate({ status: 'submitted' }, { count: 1 }) - nonceTracker = generateNonceTrackerWith(pendingTxs, confirmedTxs, '0x1') - }) - - it('should return 4', async function () { - this.timeout(15000) - const nonceLock = await nonceTracker.getNonceLock('0x7d3517b0d011698406d6e0aed8453f0be2697926') - assert.equal(nonceLock.nextNonce, '4', `nonce should be 4 got ${nonceLock.nextNonce}`) - await nonceLock.releaseLock() - }) - - it('should use localNonce if network returns a nonce lower then a confirmed tx in state', async function () { - this.timeout(15000) - const nonceLock = await nonceTracker.getNonceLock('0x7d3517b0d011698406d6e0aed8453f0be2697926') - assert.equal(nonceLock.nextNonce, '4', 'nonce should be 4') - await nonceLock.releaseLock() - }) - }) - - describe('sentry issue 476304902', function () { - beforeEach(function () { - const txGen = new MockTxGen() - pendingTxs = txGen.generate({ status: 'submitted' }, { - fromNonce: 3, - count: 29, - }) - nonceTracker = generateNonceTrackerWith(pendingTxs, [], '0x3') - }) - - it('should return 9', async function () { - this.timeout(15000) - const nonceLock = await nonceTracker.getNonceLock('0x7d3517b0d011698406d6e0aed8453f0be2697926') - assert.equal(nonceLock.nextNonce, '32', `nonce should be 32 got ${nonceLock.nextNonce}`) - await nonceLock.releaseLock() - }) - }) - - describe('issue 3670', function () { - beforeEach(function () { - const txGen = new MockTxGen() - pendingTxs = txGen.generate({ status: 'submitted' }, { - fromNonce: 6, - count: 3, - }) - nonceTracker = generateNonceTrackerWith(pendingTxs, [], '0x6') - }) - - it('should return 9', async function () { - this.timeout(15000) - const nonceLock = await nonceTracker.getNonceLock('0x7d3517b0d011698406d6e0aed8453f0be2697926') - assert.equal(nonceLock.nextNonce, '9', `nonce should be 9 got ${nonceLock.nextNonce}`) - await nonceLock.releaseLock() - }) - }) - - describe('with no previous txs', function () { - beforeEach(function () { - nonceTracker = generateNonceTrackerWith([], []) - }) - - it('should return 0', async function () { - this.timeout(15000) - const nonceLock = await nonceTracker.getNonceLock('0x7d3517b0d011698406d6e0aed8453f0be2697926') - assert.equal(nonceLock.nextNonce, '0', `nonce should be 0 returned ${nonceLock.nextNonce}`) - await nonceLock.releaseLock() - }) - }) - - describe('with multiple previous txs with same nonce', function () { - beforeEach(function () { - const txGen = new MockTxGen() - confirmedTxs = txGen.generate({ status: 'confirmed' }, { count: 1 }) - pendingTxs = txGen.generate({ - status: 'submitted', - txParams: { nonce: '0x01' }, - }, { count: 5 }) - - nonceTracker = generateNonceTrackerWith(pendingTxs, confirmedTxs, '0x0') - }) - - it('should return nonce after those', async function () { - this.timeout(15000) - const nonceLock = await nonceTracker.getNonceLock('0x7d3517b0d011698406d6e0aed8453f0be2697926') - assert.equal(nonceLock.nextNonce, '2', `nonce should be 2 got ${nonceLock.nextNonce}`) - await nonceLock.releaseLock() - }) - }) - - describe('when local confirmed count is higher than network nonce', function () { - beforeEach(function () { - const txGen = new MockTxGen() - confirmedTxs = txGen.generate({ status: 'confirmed' }, { count: 3 }) - nonceTracker = generateNonceTrackerWith([], confirmedTxs, '0x1') - }) - - it('should return nonce after those', async function () { - this.timeout(15000) - const nonceLock = await nonceTracker.getNonceLock('0x7d3517b0d011698406d6e0aed8453f0be2697926') - assert.equal(nonceLock.nextNonce, '3', `nonce should be 3 got ${nonceLock.nextNonce}`) - await nonceLock.releaseLock() - }) - }) - - describe('when local pending count is higher than other metrics', function () { - beforeEach(function () { - const txGen = new MockTxGen() - pendingTxs = txGen.generate({ status: 'submitted' }, { count: 2 }) - nonceTracker = generateNonceTrackerWith(pendingTxs, []) - }) - - it('should return nonce after those', async function () { - this.timeout(15000) - const nonceLock = await nonceTracker.getNonceLock('0x7d3517b0d011698406d6e0aed8453f0be2697926') - assert.equal(nonceLock.nextNonce, '2', `nonce should be 2 got ${nonceLock.nextNonce}`) - await nonceLock.releaseLock() - }) - }) - - describe('when provider nonce is higher than other metrics', function () { - beforeEach(function () { - const txGen = new MockTxGen() - pendingTxs = txGen.generate({ status: 'submitted' }, { count: 2 }) - nonceTracker = generateNonceTrackerWith(pendingTxs, [], '0x05') - }) - - it('should return nonce after those', async function () { - this.timeout(15000) - const nonceLock = await nonceTracker.getNonceLock('0x7d3517b0d011698406d6e0aed8453f0be2697926') - assert.equal(nonceLock.nextNonce, '5', `nonce should be 5 got ${nonceLock.nextNonce}`) - await nonceLock.releaseLock() - }) - }) - - describe('when there are some pending nonces below the remote one and some over.', function () { - beforeEach(function () { - const txGen = new MockTxGen() - pendingTxs = txGen.generate({ status: 'submitted' }, { count: 5 }) - nonceTracker = generateNonceTrackerWith(pendingTxs, [], '0x03') - }) - - it('should return nonce after those', async function () { - this.timeout(15000) - const nonceLock = await nonceTracker.getNonceLock('0x7d3517b0d011698406d6e0aed8453f0be2697926') - assert.equal(nonceLock.nextNonce, '5', `nonce should be 5 got ${nonceLock.nextNonce}`) - await nonceLock.releaseLock() - }) - }) - - describe('when there are pending nonces non sequentially over the network nonce.', function () { - beforeEach(function () { - const txGen = new MockTxGen() - txGen.generate({ status: 'submitted' }, { count: 5 }) - // 5 over that number - pendingTxs = txGen.generate({ status: 'submitted' }, { count: 5 }) - nonceTracker = generateNonceTrackerWith(pendingTxs, [], '0x00') - }) - - it('should return nonce after network nonce', async function () { - this.timeout(15000) - const nonceLock = await nonceTracker.getNonceLock('0x7d3517b0d011698406d6e0aed8453f0be2697926') - assert.equal(nonceLock.nextNonce, '0', `nonce should be 0 got ${nonceLock.nextNonce}`) - await nonceLock.releaseLock() - }) - }) - - describe('When all three return different values', function () { - beforeEach(function () { - const txGen = new MockTxGen() - confirmedTxs = txGen.generate({ status: 'confirmed' }, { count: 10 }) - pendingTxs = txGen.generate({ - status: 'submitted', - nonce: 100, - }, { count: 1 }) - // 0x32 is 50 in hex: - nonceTracker = generateNonceTrackerWith(pendingTxs, confirmedTxs, '0x32') - }) - - it('should return nonce after network nonce', async function () { - this.timeout(15000) - const nonceLock = await nonceTracker.getNonceLock('0x7d3517b0d011698406d6e0aed8453f0be2697926') - assert.equal(nonceLock.nextNonce, '50', `nonce should be 50 got ${nonceLock.nextNonce}`) - await nonceLock.releaseLock() - }) - }) - - describe('Faq issue 67', function () { - beforeEach(function () { - const txGen = new MockTxGen() - confirmedTxs = txGen.generate({ status: 'confirmed' }, { count: 64 }) - pendingTxs = txGen.generate({ - status: 'submitted', - }, { count: 10 }) - // 0x40 is 64 in hex: - nonceTracker = generateNonceTrackerWith(pendingTxs, [], '0x40') - }) - - it('should return nonce after network nonce', async function () { - this.timeout(15000) - const nonceLock = await nonceTracker.getNonceLock('0x7d3517b0d011698406d6e0aed8453f0be2697926') - assert.equal(nonceLock.nextNonce, '74', `nonce should be 74 got ${nonceLock.nextNonce}`) - await nonceLock.releaseLock() - }) - }) - }) -}) - -function generateNonceTrackerWith (pending, confirmed, providerStub = '0x0') { - const getPendingTransactions = () => pending - const getConfirmedTransactions = () => confirmed - providerResultStub.result = providerStub - const provider = { - sendAsync: (_, cb) => { cb(undefined, providerResultStub) }, - } - const blockTracker = { - getCurrentBlock: () => '0x11b568', - getLatestBlock: async () => '0x11b568', - } - return new NonceTracker({ - provider, - blockTracker, - getPendingTransactions, - getConfirmedTransactions, - }) -} diff --git a/test/unit/app/controllers/transactions/tx-controller-test.js b/test/unit/app/controllers/transactions/tx-controller-test.js index 90197a21f..4aa9d5f4a 100644 --- a/test/unit/app/controllers/transactions/tx-controller-test.js +++ b/test/unit/app/controllers/transactions/tx-controller-test.js @@ -417,28 +417,27 @@ describe('Transaction Controller', function () { }) describe('#retryTransaction', function () { - it('should create a new txMeta with the same txParams as the original one but with a higher gasPrice', function (done) { + it('should create a new txMeta with the same txParams as the original one', function (done) { const txParams = { gasPrice: '0xee6b2800', nonce: '0x00', - from: '0xB09d8505E1F4EF1CeA089D47094f5DD3464083d4', - to: '0xB09d8505E1F4EF1CeA089D47094f5DD3464083d4', + from: '0xb09d8505e1f4ef1cea089d47094f5dd3464083d4', + to: '0xb09d8505e1f4ef1cea089d47094f5dd3464083d4', data: '0x0', } txController.txStateManager._saveTxList([ - { id: 1, status: 'submitted', metamaskNetworkId: currentNetworkId, txParams, history: [{}] }, + { id: 1, status: 'submitted', metamaskNetworkId: currentNetworkId, txParams, history: [] }, ]) txController.retryTransaction(1) - .then((txMeta) => { - assert.equal(txMeta.txParams.gasPrice, '0x10642ac00', 'gasPrice should have a %10 gasPrice bump') - assert.equal(txMeta.txParams.nonce, txParams.nonce, 'nonce should be the same') - assert.equal(txMeta.txParams.from, txParams.from, 'from should be the same') - assert.equal(txMeta.txParams.to, txParams.to, 'to should be the same') - assert.equal(txMeta.txParams.data, txParams.data, 'data should be the same') - assert.ok(('lastGasPrice' in txMeta), 'should have the key `lastGasPrice`') - assert.equal(txController.txStateManager.getTxList().length, 2) - done() - }).catch(done) + .then((txMeta) => { + assert.equal(txMeta.txParams.nonce, txParams.nonce, 'nonce should be the same') + assert.equal(txMeta.txParams.from, txParams.from, 'from should be the same') + assert.equal(txMeta.txParams.to, txParams.to, 'to should be the same') + assert.equal(txMeta.txParams.data, txParams.data, 'data should be the same') + assert.ok(('lastGasPrice' in txMeta), 'should have the key `lastGasPrice`') + assert.equal(txController.txStateManager.getTxList().length, 2) + done() + }).catch(done) }) }) diff --git a/test/unit/app/controllers/transactions/tx-state-manager-test.js b/test/unit/app/controllers/transactions/tx-state-manager-test.js index a593c2d06..9c58d1be8 100644 --- a/test/unit/app/controllers/transactions/tx-state-manager-test.js +++ b/test/unit/app/controllers/transactions/tx-state-manager-test.js @@ -1,5 +1,5 @@ -const assert = require('assert') -const TxStateManager = require('../../../../../app/scripts/controllers/transactions/tx-state-manager') +import assert from 'assert' +import TxStateManager from '../../../../../app/scripts/controllers/transactions/tx-state-manager' import txStateHistoryHelper from '../../../../../app/scripts/controllers/transactions/lib/tx-state-history-helper' const noop = () => true @@ -29,7 +29,7 @@ describe('TransactionStateManager', function () { assert.equal(result[0].status, 'signed') }) - it('should emit a signed event to signal the exciton of callback', (done) => { + it('should emit a signed event to signal the exciton of callback', function (done) { const tx = { id: 1, status: 'unapproved', metamaskNetworkId: currentNetworkId, txParams: {} } const noop = function () { assert(true, 'event listener has been triggered and noop executed') @@ -43,7 +43,7 @@ describe('TransactionStateManager', function () { }) describe('#setTxStatusRejected', function () { - it('sets the tx status to rejected and removes it from history', function () { + it('sets the tx status to rejected and removes it from history', function () { const tx = { id: 1, status: 'unapproved', metamaskNetworkId: currentNetworkId, txParams: {} } txStateManager.addTx(tx) txStateManager.setTxStatusRejected(1) @@ -52,15 +52,12 @@ describe('TransactionStateManager', function () { assert.equal(result.length, 0) }) - it('should emit a rejected event to signal the exciton of callback', (done) => { + it('should emit a rejected event to signal the exciton of callback', function (done) { const tx = { id: 1, status: 'unapproved', metamaskNetworkId: currentNetworkId, txParams: {} } txStateManager.addTx(tx) - const noop = function (err, txId) { - if (err) { - console.log('Error: ', err) - } - assert(true, 'event listener has been triggered and noop executed') - done() + const noop = () => { + assert(true, 'event listener has been triggered and noop executed') + done() } txStateManager.on('1:rejected', noop) txStateManager.setTxStatusRejected(1) @@ -93,6 +90,37 @@ describe('TransactionStateManager', function () { assert.equal(result[0].id, 1) }) + it('throws error and does not add tx if txParams are invalid', function () { + const validTxParams = { + from: '0x0dcd5d886577d5081b0c52e242ef29e70be3e7bc', + to: '0x0039f22efb07a647557c7c5d17854cfd6d489ef3', + nonce: '0x3', + gas: '0x77359400', + gasPrice: '0x77359400', + value: '0x0', + data: '0x0', + } + const invalidValues = [1, true, {}, Symbol('1')] + + for (const key in validTxParams) { + for (const value of invalidValues) { + const tx = { + id: 1, + status: 'unapproved', + metamaskNetworkId: currentNetworkId, + txParams: { + ...validTxParams, + [key]: value, + }, + } + assert.throws(txStateManager.addTx.bind(txStateManager, tx), 'addTx should throw error') + const result = txStateManager.getTxList() + assert.ok(Array.isArray(result), 'txList should be an array') + assert.equal(result.length, 0, 'txList should be empty') + } + } + }) + it('does not override txs from other networks', function () { const tx = { id: 1, status: 'confirmed', metamaskNetworkId: currentNetworkId, txParams: {} } const tx2 = { id: 2, status: 'confirmed', metamaskNetworkId: otherNetworkId, txParams: {} } @@ -153,6 +181,37 @@ describe('TransactionStateManager', function () { assert.equal(result.hash, 'foo') }) + it('throws error and does not update tx if txParams are invalid', function () { + const validTxParams = { + from: '0x0dcd5d886577d5081b0c52e242ef29e70be3e7bc', + to: '0x0039f22efb07a647557c7c5d17854cfd6d489ef3', + nonce: '0x3', + gas: '0x77359400', + gasPrice: '0x77359400', + value: '0x0', + data: '0x0', + } + const invalidValues = [1, true, {}, Symbol('1')] + + txStateManager.addTx({ id: 1, status: 'unapproved', metamaskNetworkId: currentNetworkId, txParams: validTxParams }) + + for (const key in validTxParams) { + for (const value of invalidValues) { + const originalTx = txStateManager.getTx(1) + const newTx = { + ...originalTx, + txParams: { + ...originalTx.txParams, + [key]: value, + }, + } + assert.throws(txStateManager.updateTx.bind(txStateManager, newTx), 'updateTx should throw an error') + const result = txStateManager.getTx(1) + assert.deepEqual(result, originalTx, 'tx should not be updated') + } + } + }) + it('updates gas price and adds history items', function () { const originalGasPrice = '0x01' const desiredGasPrice = '0x02' @@ -241,6 +300,8 @@ describe('TransactionStateManager', function () { assert.equal(txStateManager.getFilteredTxList(filterParams).length, 5, `getFilteredTxList - ${JSON.stringify(filterParams)}`) filterParams = { to: '0xaa' } assert.equal(txStateManager.getFilteredTxList(filterParams).length, 5, `getFilteredTxList - ${JSON.stringify(filterParams)}`) + filterParams = { status: (status) => status !== 'confirmed' } + assert.equal(txStateManager.getFilteredTxList(filterParams).length, 5, `getFilteredTxList - ${JSON.stringify(filterParams)}`) }) }) @@ -283,19 +344,18 @@ describe('TransactionStateManager', function () { assert.equal(txsFromCurrentNetworkAndAddress.length, 0) assert.equal(txFromOtherNetworks.length, 2) - }) }) describe('#_removeTx', function () { - it('should remove the transaction from the storage', () => { - txStateManager._saveTxList([ {id: 1} ]) + it('should remove the transaction from the storage', function () { + txStateManager._saveTxList([ { id: 1 } ]) txStateManager._removeTx(1) assert(!txStateManager.getFullTxList().length, 'txList should be empty') }) - it('should only remove the transaction with ID 1 from the storage', () => { - txStateManager._saveTxList([ {id: 1}, {id: 2} ]) + it('should only remove the transaction with ID 1 from the storage', function () { + txStateManager._saveTxList([ { id: 1 }, { id: 2 } ]) txStateManager._removeTx(1) assert.equal(txStateManager.getFullTxList()[0].id, 2, 'txList should have a id of 2') })