diff --git a/app/scripts/controllers/transactions/index.js b/app/scripts/controllers/transactions/index.js index 63bb2ae56..021bc1c5e 100644 --- a/app/scripts/controllers/transactions/index.js +++ b/app/scripts/controllers/transactions/index.js @@ -1780,7 +1780,10 @@ export default class TransactionController extends EventEmitter { txMeta, 'transactions/pending-tx-tracker#event: tx:confirmed reference to confirmed txHash with same nonce', ); - this._dropTransaction(otherTxMeta.id); + // Drop any transaction that wasn't previously failed (off chain failure) + if (otherTxMeta.status !== TRANSACTION_STATUSES.FAILED) { + this._dropTransaction(otherTxMeta.id); + } }); } diff --git a/ui/selectors/nonce-sorted-transactions-selector.test.js b/ui/selectors/nonce-sorted-transactions-selector.test.js index 458439be9..12a22fc08 100644 --- a/ui/selectors/nonce-sorted-transactions-selector.test.js +++ b/ui/selectors/nonce-sorted-transactions-selector.test.js @@ -130,6 +130,28 @@ describe('nonceSortedTransactionsSelector', () => { ]); }); + it('should properly group a failed off-chain simple send that is superseded by a retry', () => { + const txList = [ + duplicateTx(SIMPLE_SEND_TX, { status: TRANSACTION_STATUSES.FAILED }), + duplicateTx(RETRY_TX, { time: 1 }), + ]; + + const state = getStateTree({ txList }); + + const result = nonceSortedTransactionsSelector(state); + + expect(result).toStrictEqual([ + { + nonce: '0x0', + transactions: txList, + initialTransaction: head(txList), + primaryTransaction: last(txList), + hasRetried: true, + hasCancelled: false, + }, + ]); + }); + it('should properly group a simple send that is superseded by a cancel', () => { const txList = [ duplicateTx(SIMPLE_SEND_TX, { status: TRANSACTION_STATUSES.DROPPED }), @@ -181,7 +203,7 @@ describe('nonceSortedTransactionsSelector', () => { // test case when we move and change grouping logic. const txList = [ duplicateTx(TOKEN_SEND_TX, { status: TRANSACTION_STATUSES.DROPPED }), - duplicateTx(SIMPLE_SEND_TX), + duplicateTx(SIMPLE_SEND_TX, { time: 1 }), ]; const state = getStateTree({ txList }); @@ -267,15 +289,14 @@ describe('nonceSortedTransactionsSelector', () => { ]); }); - it('should not set a failed off-chain transaction as primary, regardless of tx order', () => { + it('should not set a failed off-chain transaction as primary or initial, regardless of tx order', () => { // Scenario: // 1. You submit transaction A. // 2. Transaction A fails off-chain (the network rejects it). // 3. You submit transaction B. - // 4. You see a pending transaction (B's status) in the activity with the - // details of A. Seeing the pending status is desired. Seeing the - // details of transaction A (recipient, value, etc) is a bug to be fixed - // in a future PR. + // 4. Transaction A no longer has any visual representation in the UI. + // This is desired because we have no way currently to tell the intent + // of the transactions. const txList = [ duplicateTx(SIMPLE_SEND_TX, { status: TRANSACTION_STATUSES.FAILED }), duplicateTx(SIMPLE_SEND_TX, { @@ -292,7 +313,7 @@ describe('nonceSortedTransactionsSelector', () => { { nonce: '0x0', transactions: txList, - initialTransaction: head(txList), + initialTransaction: last(txList), primaryTransaction: last(txList), hasRetried: false, hasCancelled: false, diff --git a/ui/selectors/transactions.js b/ui/selectors/transactions.js index 1fd6b1447..15e05d2f8 100644 --- a/ui/selectors/transactions.js +++ b/ui/selectors/transactions.js @@ -17,6 +17,11 @@ import { getSelectedAddress, } from './selectors'; +const INVALID_INITIAL_TRANSACTION_TYPES = [ + TRANSACTION_TYPES.CANCEL, + TRANSACTION_TYPES.RETRY, +]; + export const incomingTxListSelector = (state) => { const { showIncomingTransactions } = state.metamask.featureFlags; if (!showIncomingTransactions) { @@ -245,6 +250,7 @@ export const nonceSortedTransactionsSelector = createSelector( status, type, time: txTime, + txReceipt, } = transaction; if (typeof nonce === 'undefined' || type === TRANSACTION_TYPES.INCOMING) { @@ -271,47 +277,161 @@ export const nonceSortedTransactionsSelector = createSelector( const { primaryTransaction: { time: primaryTxTime = 0 } = {}, + initialTransaction: { time: initialTxTime = 0 } = {}, } = nonceProps; - const previousPrimaryIsNetworkFailure = - nonceProps.primaryTransaction.status === - TRANSACTION_STATUSES.FAILED && - nonceProps.primaryTransaction?.txReceipt?.status !== '0x0'; - const currentTransactionIsOnChainFailure = - transaction?.txReceipt?.status === '0x0'; + // Current Transaction Logic Cases + // -------------------------------------------------------------------- + // Current transaction: The transaction we are examining in this loop. + // Each iteration should be in time order, but that is not guaranteed. + // -------------------------------------------------------------------- + const currentTransaction = { + // A on chain failure means the current transaction was submitted and + // considered for inclusion in a block but something prevented it + // from being included, such as slippage on gas prices and conversion + // when doing a swap. These transactions will have a '0x0' value in + // the txReceipt.status field. + isOnChainFailure: txReceipt?.status === '0x0', + // Another type of failure is a "off chain" or "network" failure, + // where the error occurs on the JSON RPC call to the network client + // (Like Infura). These transactions are never broadcast for + // inclusion and the nonce associated with them is not consumed. When + // this occurs the next transaction will have the same nonce as the + // current, failed transaction. A failed on chain transaction will + // not have the FAILED status although it should (future TODO: add a + // new FAILED_ON_CHAIN) status. I use the word "Ephemeral" here + // because a failed transaction that does not get broadcast is not + // known outside of the user's local MetaMask and the nonce + // associated will be applied to the next. + isEphemeral: + status === TRANSACTION_STATUSES.FAILED && + txReceipt?.status !== '0x0', + // We never want to use a speed up (retry) or cancel as the initial + // transaction in a group, regardless of time order. This is because + // useTransactionDisplayData cannot parse a retry or cancel because + // it lacks information on whether its a simple send, token transfer, + // etc. + isRetryOrCancel: INVALID_INITIAL_TRANSACTION_TYPES.includes(type), + // Primary transactions usually are the latest transaction by time, + // but not always. This value shows whether this transaction occurred + // after the current primary. + occurredAfterPrimary: txTime > primaryTxTime, + // Priority Statuses are those that are ones either already confirmed + // on chain, submitted to the network, or waiting for user approval. + // These statuses typically indicate a transaction that needs to have + // its status reflected in the UI. + hasPriorityStatus: status in PRIORITY_STATUS_HASH, + // A confirmed transaction is the most valid transaction status to + // display because no other transaction of the same nonce can have a + // more valid status. + isConfirmed: status === TRANSACTION_STATUSES.CONFIRMED, + // Initial transactions usually are the earliest transaction by time, + // but not always. THis value shows whether this transaction occurred + // before the current initial. + occurredBeforeInitial: txTime < initialTxTime, + // We only allow users to retry the transaction in certain scenarios + // to help shield from expensive operations and other unwanted side + // effects. This value is used to determine if the entire transaction + // group should be marked as having had a retry. + isValidRetry: + type === TRANSACTION_TYPES.RETRY && + (status in PRIORITY_STATUS_HASH || + status === TRANSACTION_STATUSES.DROPPED), + // We only allow users to cancel the transaction in certain scenarios + // to help shield from expensive operations and other unwanted side + // effects. This value is used to determine if the entire transaction + // group should be marked as having had a cancel. + isValidCancel: + type === TRANSACTION_TYPES.CANCEL && + (status in PRIORITY_STATUS_HASH || + status === TRANSACTION_STATUSES.DROPPED), + }; + // We should never assign a retry or cancel transaction as the initial, + // likewise an ephemeral transaction should not be initial. + currentTransaction.eligibleForInitial = + !currentTransaction.isRetryOrCancel && + !currentTransaction.isEphemeral; + + // If a transaction failed on chain or was confirmed then it should + // always be the primary because no other transaction is more valid. + currentTransaction.shouldBePrimary = + currentTransaction.isConfirmed || currentTransaction.isOnChainFailure; + + // Primary Transaction Logic Cases + // -------------------------------------------------------------------- + // Primary transaction: The transaction for any given nonce which has + // the most valid status on the network. + // Example: + // 1. Submit transaction A + // 2. Speed up Transaction A. + // 3. This creates a new Transaction (B) with higher gas params. + // 4. Transaction A and Transaction B are both submitted. + // 5. We expect Transaction B to be the most valid transaction to use + // for the status of the transaction group because it has higher + // gas params and should be included first. + // The following logic variables are used for edge cases that protect + // against UI bugs when this breaks down. + const previousPrimaryTransaction = { + // As we loop through the transactions in state we may temporarily + // assign a primaryTransaction that is an "Ephemeral" transaction, + // which is one that failed before being broadcast for inclusion in a + // block. When this happens, and we have another transaction to + // consider in a nonce group, we should use the new transaction. + isEphemeral: + nonceProps.primaryTransaction.status === + TRANSACTION_STATUSES.FAILED && + nonceProps.primaryTransaction?.txReceipt?.status !== '0x0', + }; + + // Initial Transaction Logic Cases + // -------------------------------------------------------------------- + // Initial Transaction: The transaciton that most likely represents the + // user's intent when creating/approving the transaction. In most cases + // this is the first transaction of a nonce group, by time, but this + // breaks down in the case of users with the advanced setting enabled + // to set their own nonces manually. In that case a user may submit two + // completely different transactions of the same nonce and they will be + // bundled together by this selector as the same activity entry. + const previousInitialTransaction = { + // As we loop through the transactions in state we may temporarily + // assign a initialTransaction that is an "Ephemeral" transaction, + // which is one that failed before being broadcast for inclusion in a + // block. When this happens, and we have another transaction to + // consider in a nonce group, we should use the new transaction. + isEphemeral: + nonceProps.initialTransaction.status === + TRANSACTION_STATUSES.FAILED && + nonceProps.initialTransaction.txReceipt?.status !== '0x0', + }; + + // Check the above logic cases and assign a new primaryTransaction if + // appropriate if ( - status === TRANSACTION_STATUSES.CONFIRMED || - currentTransactionIsOnChainFailure || - previousPrimaryIsNetworkFailure || - (txTime > primaryTxTime && status in PRIORITY_STATUS_HASH) + currentTransaction.shouldBePrimary || + previousPrimaryTransaction.isEphemeral || + (currentTransaction.occurredAfterPrimary && + currentTransaction.hasPriorityStatus) ) { nonceProps.primaryTransaction = transaction; } - const { - initialTransaction: { time: initialTxTime = 0 } = {}, - } = nonceProps; - - // Used to display the transaction action, since we don't want to overwrite the action if - // it was replaced with a cancel attempt transaction. - if (txTime < initialTxTime) { + // Check the above logic cases and assign a new initialTransaction if + // appropriate + if ( + (currentTransaction.occurredBeforeInitial && + currentTransaction.eligibleForInitial) || + (previousInitialTransaction.isEphemeral && + currentTransaction.eligibleForInitial) + ) { nonceProps.initialTransaction = transaction; } - if ( - type === TRANSACTION_TYPES.RETRY && - (status in PRIORITY_STATUS_HASH || - status === TRANSACTION_STATUSES.DROPPED) - ) { + if (currentTransaction.isValidRetry) { nonceProps.hasRetried = true; } - if ( - type === TRANSACTION_TYPES.CANCEL && - (status in PRIORITY_STATUS_HASH || - status === TRANSACTION_STATUSES.DROPPED) - ) { + if (currentTransaction.isValidCancel) { nonceProps.hasCancelled = true; } } else { @@ -341,7 +461,31 @@ export const nonceSortedTransactionsSelector = createSelector( orderedTransactionGroups, incomingTransactionGroups, ); - return unapprovedTransactionGroups.concat(orderedTransactionGroups); + return unapprovedTransactionGroups + .concat(orderedTransactionGroups) + .map((txGroup) => { + // In the case that we have a cancel or retry as initial transaction + // and there is a valid transaction in the group, we should reassign + // the other valid transaction as initial. In this case validity of the + // transaction is expanded to include off-chain failures because it is + // valid to retry those with higher gas prices. + if ( + INVALID_INITIAL_TRANSACTION_TYPES.includes( + txGroup.initialTransaction?.type, + ) + ) { + const nonRetryOrCancel = txGroup.transactions.find( + (tx) => !INVALID_INITIAL_TRANSACTION_TYPES.includes(tx.type), + ); + if (nonRetryOrCancel) { + return { + ...txGroup, + initialTransaction: nonRetryOrCancel, + }; + } + } + return txGroup; + }); }, );