From 001a01e5b2ea1e288d0115e2091227378b4e3d99 Mon Sep 17 00:00:00 2001 From: Brad Decker Date: Mon, 17 May 2021 12:44:47 -0500 Subject: [PATCH] Migration to remove erroneous tx state (#11107) --- app/scripts/migrations/059.js | 52 ++++ app/scripts/migrations/059.test.js | 385 +++++++++++++++++++++++++++++ app/scripts/migrations/index.js | 1 + 3 files changed, 438 insertions(+) create mode 100644 app/scripts/migrations/059.js create mode 100644 app/scripts/migrations/059.test.js diff --git a/app/scripts/migrations/059.js b/app/scripts/migrations/059.js new file mode 100644 index 000000000..bbec1b9b8 --- /dev/null +++ b/app/scripts/migrations/059.js @@ -0,0 +1,52 @@ +import { + cloneDeep, + concat, + groupBy, + keyBy, + pickBy, + isPlainObject, +} from 'lodash'; +import { TRANSACTION_TYPES } from '../../../shared/constants/transaction'; + +const version = 59; + +/** + * Removes orphaned cancel and retry transactions that no longer have the + * original transaction in state, which results in bugs. + */ +export default { + version, + async migrate(originalVersionedData) { + const versionedData = cloneDeep(originalVersionedData); + versionedData.meta.version = version; + const state = versionedData.data; + versionedData.data = transformState(state); + return versionedData; + }, +}; + +function transformState(state) { + const transactions = state?.TransactionController?.transactions; + if (isPlainObject(transactions)) { + const nonceNetworkGroupedObject = groupBy( + Object.values(transactions), + (tx) => { + return `${tx.txParams?.nonce}-${tx.chainId ?? tx.metamaskNetworkId}`; + }, + ); + + const withoutOrphans = pickBy(nonceNetworkGroupedObject, (group) => { + return group.some( + (tx) => + tx.type !== TRANSACTION_TYPES.CANCEL && + tx.type !== TRANSACTION_TYPES.RETRY, + ); + }); + state.TransactionController.transactions = keyBy( + concat(...Object.values(withoutOrphans)), + (tx) => tx.id, + ); + } + + return state; +} diff --git a/app/scripts/migrations/059.test.js b/app/scripts/migrations/059.test.js new file mode 100644 index 000000000..bdf4263c2 --- /dev/null +++ b/app/scripts/migrations/059.test.js @@ -0,0 +1,385 @@ +import { strict as assert } from 'assert'; +import { cloneDeep } from 'lodash'; +import { + KOVAN_CHAIN_ID, + MAINNET_CHAIN_ID, + RINKEBY_CHAIN_ID, + GOERLI_CHAIN_ID, +} from '../../../shared/constants/network'; +import { + TRANSACTION_TYPES, + TRANSACTION_STATUSES, +} from '../../../shared/constants/transaction'; +import migration59 from './059'; + +const ERRONEOUS_TRANSACTION_STATE = { + 0: { + type: TRANSACTION_TYPES.CANCEL, + id: 0, + chainId: MAINNET_CHAIN_ID, + txParams: { + nonce: '0x0', + }, + }, + 1: { + type: TRANSACTION_TYPES.SENT_ETHER, + id: 1, + chainId: MAINNET_CHAIN_ID, + txParams: { + nonce: '0x1', + }, + }, + 2: { + type: TRANSACTION_TYPES.SENT_ETHER, + id: 2, + chainId: KOVAN_CHAIN_ID, + txParams: { + nonce: '0x2', + }, + }, + 3: { + type: TRANSACTION_TYPES.SENT_ETHER, + id: 3, + chainId: RINKEBY_CHAIN_ID, + txParams: { + nonce: '0x3', + }, + }, + 4: { + type: TRANSACTION_TYPES.SENT_ETHER, + id: 4, + chainId: RINKEBY_CHAIN_ID, + txParams: { + nonce: '0x4', + }, + }, + 5: { + type: TRANSACTION_TYPES.SENT_ETHER, + id: 5, + chainId: MAINNET_CHAIN_ID, + txParams: { + nonce: '0x5', + }, + }, + 6: { + type: TRANSACTION_TYPES.SENT_ETHER, + id: 6, + chainId: KOVAN_CHAIN_ID, + txParams: { + nonce: '0x6', + }, + }, + 7: { + type: TRANSACTION_TYPES.SENT_ETHER, + id: 7, + chainId: RINKEBY_CHAIN_ID, + txParams: { + nonce: '0x7', + }, + }, + 8: { + type: TRANSACTION_TYPES.SENT_ETHER, + id: 8, + chainId: RINKEBY_CHAIN_ID, + txParams: { + nonce: '0x8', + }, + }, + 9: { + type: TRANSACTION_TYPES.SENT_ETHER, + id: 9, + chainId: RINKEBY_CHAIN_ID, + status: TRANSACTION_STATUSES.UNAPPROVED, + }, +}; + +const ERRONEOUS_TRANSACTION_STATE_RETRY = { + ...ERRONEOUS_TRANSACTION_STATE, + 0: { + ...ERRONEOUS_TRANSACTION_STATE[0], + type: TRANSACTION_TYPES.RETRY, + }, +}; + +const ERRONEOUS_TRANSACTION_STATE_MIXED = { + ...ERRONEOUS_TRANSACTION_STATE, + 10: { + type: TRANSACTION_TYPES.RETRY, + id: 10, + chainId: MAINNET_CHAIN_ID, + txParams: { + nonce: '0xa', + }, + }, + 11: { + type: TRANSACTION_TYPES.RETRY, + id: 11, + chainId: MAINNET_CHAIN_ID, + txParams: { + nonce: '0xb', + }, + }, +}; + +describe('migration #59', function () { + it('should update the version metadata', async function () { + const oldStorage = { + meta: { + version: 58, + }, + data: {}, + }; + + const newStorage = await migration59.migrate(oldStorage); + assert.deepEqual(newStorage.meta, { + version: 59, + }); + }); + + it('should drop orphaned cancel transactions', async function () { + const oldStorage = { + meta: {}, + data: { + TransactionController: { + transactions: ERRONEOUS_TRANSACTION_STATE, + }, + foo: 'bar', + }, + }; + + const newStorage = await migration59.migrate(oldStorage); + const EXPECTED = cloneDeep(ERRONEOUS_TRANSACTION_STATE); + delete EXPECTED['0']; + assert.deepEqual(newStorage.data, { + TransactionController: { + transactions: EXPECTED, + }, + foo: 'bar', + }); + }); + + it('should drop orphaned cancel transactions even if a nonce exists on another network that is confirmed', async function () { + const oldStorage = { + meta: {}, + data: { + TransactionController: { + transactions: { + ...ERRONEOUS_TRANSACTION_STATE, + 11: { + ...ERRONEOUS_TRANSACTION_STATE['0'], + id: 11, + chainId: GOERLI_CHAIN_ID, + type: TRANSACTION_TYPES.SENT_ETHER, + }, + }, + }, + foo: 'bar', + }, + }; + + const newStorage = await migration59.migrate(oldStorage); + const EXPECTED = cloneDeep( + oldStorage.data.TransactionController.transactions, + ); + delete EXPECTED['0']; + assert.deepEqual(newStorage.data, { + TransactionController: { + transactions: EXPECTED, + }, + foo: 'bar', + }); + }); + + it('should not drop cancel transactions with matching non cancel or retry in same network and nonce', async function () { + const oldStorage = { + meta: {}, + data: { + TransactionController: { + transactions: { + ...ERRONEOUS_TRANSACTION_STATE, + 11: { + ...ERRONEOUS_TRANSACTION_STATE['0'], + id: 11, + type: TRANSACTION_TYPES.SENT_ETHER, + }, + }, + }, + foo: 'bar', + }, + }; + + const newStorage = await migration59.migrate(oldStorage); + assert.deepEqual(newStorage.data, { + TransactionController: { + transactions: oldStorage.data.TransactionController.transactions, + }, + foo: 'bar', + }); + }); + + it('should drop orphaned retry transactions', async function () { + const oldStorage = { + meta: {}, + data: { + TransactionController: { + transactions: ERRONEOUS_TRANSACTION_STATE_RETRY, + }, + foo: 'bar', + }, + }; + + const newStorage = await migration59.migrate(oldStorage); + const EXPECTED = cloneDeep(ERRONEOUS_TRANSACTION_STATE_RETRY); + delete EXPECTED['0']; + assert.deepEqual(newStorage.data, { + TransactionController: { + transactions: EXPECTED, + }, + foo: 'bar', + }); + }); + + it('should drop orphaned retry transactions even if a nonce exists on another network that is confirmed', async function () { + const oldStorage = { + meta: {}, + data: { + TransactionController: { + transactions: { + ...ERRONEOUS_TRANSACTION_STATE_RETRY, + 11: { + ...ERRONEOUS_TRANSACTION_STATE_RETRY['0'], + id: 11, + chainId: GOERLI_CHAIN_ID, + type: TRANSACTION_TYPES.SENT_ETHER, + }, + }, + }, + foo: 'bar', + }, + }; + + const newStorage = await migration59.migrate(oldStorage); + const EXPECTED = cloneDeep( + oldStorage.data.TransactionController.transactions, + ); + delete EXPECTED['0']; + assert.deepEqual(newStorage.data, { + TransactionController: { + transactions: EXPECTED, + }, + foo: 'bar', + }); + }); + + it('should not drop retry transactions with matching non cancel or retry in same network and nonce', async function () { + const oldStorage = { + meta: {}, + data: { + TransactionController: { + transactions: { + ...ERRONEOUS_TRANSACTION_STATE_RETRY, + 11: { + ...ERRONEOUS_TRANSACTION_STATE_RETRY['0'], + id: 11, + type: TRANSACTION_TYPES.SENT_ETHER, + }, + }, + }, + foo: 'bar', + }, + }; + + const newStorage = await migration59.migrate(oldStorage); + assert.deepEqual(newStorage.data, { + TransactionController: { + transactions: oldStorage.data.TransactionController.transactions, + }, + foo: 'bar', + }); + }); + + it('should drop all orphaned retry and cancel transactions', async function () { + const oldStorage = { + meta: {}, + data: { + TransactionController: { + transactions: ERRONEOUS_TRANSACTION_STATE_MIXED, + }, + foo: 'bar', + }, + }; + + const newStorage = await migration59.migrate(oldStorage); + // The following ERRONEOUS_TRANSACTION_STATE object only has one orphan in it + // so using it as the base for our expected output automatically removes a few + // transactions we expect to be missing. + const EXPECTED = cloneDeep(ERRONEOUS_TRANSACTION_STATE); + delete EXPECTED['0']; + assert.deepEqual(newStorage.data, { + TransactionController: { + transactions: EXPECTED, + }, + foo: 'bar', + }); + }); + + it('should do nothing if transactions state does not exist', async function () { + const oldStorage = { + meta: {}, + data: { + TransactionController: { + bar: 'baz', + }, + IncomingTransactionsController: { + foo: 'baz', + }, + foo: 'bar', + }, + }; + + const newStorage = await migration59.migrate(oldStorage); + assert.deepEqual(oldStorage.data, newStorage.data); + }); + + it('should do nothing if transactions state is empty', async function () { + const oldStorage = { + meta: {}, + data: { + TransactionController: { + transactions: {}, + bar: 'baz', + }, + foo: 'bar', + }, + }; + + const newStorage = await migration59.migrate(oldStorage); + assert.deepEqual(oldStorage.data, newStorage.data); + }); + + it('should do nothing if transactions state is not an object', async function () { + const oldStorage = { + meta: {}, + data: { + TransactionController: { + transactions: [], + bar: 'baz', + }, + foo: 'bar', + }, + }; + + const newStorage = await migration59.migrate(oldStorage); + assert.deepEqual(oldStorage.data, newStorage.data); + }); + + it('should do nothing if state is empty', async function () { + const oldStorage = { + meta: {}, + data: {}, + }; + + const newStorage = await migration59.migrate(oldStorage); + assert.deepEqual(oldStorage.data, newStorage.data); + }); +}); diff --git a/app/scripts/migrations/index.js b/app/scripts/migrations/index.js index b0c1716f5..47925fbba 100644 --- a/app/scripts/migrations/index.js +++ b/app/scripts/migrations/index.js @@ -63,6 +63,7 @@ const migrations = [ require('./056').default, require('./057').default, require('./058').default, + require('./059').default, ]; export default migrations;