Do not show failed off-chain transactions details when grouped with another valid transaction of same nonce (#14497)

* fix failed off chain tx mismatch with next confirmed transaction

* dont drop failed txs when tx in confirmed

* add comment for reassigning logic

* resolve change requests

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);
+      }
     });
   }
 

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
+    // 4. Transaction A no longer has any visual representation in the UI.
-    //    details of A. Seeing the pending status is desired. Seeing the
+    //    This is desired because we have no way currently to tell the intent
-    //    details of transaction A (recipient, value, etc) is a bug to be fixed
+    //    of the transactions.
-    //    in a future PR.
     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,

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 =
+        // Current Transaction Logic Cases
-          nonceProps.primaryTransaction.status ===
+        // --------------------------------------------------------------------
-            TRANSACTION_STATUSES.FAILED &&
+        // Current transaction: The transaction we are examining in this loop.
-          nonceProps.primaryTransaction?.txReceipt?.status !== '0x0';
+        // Each iteration should be in time order, but that is not guaranteed.
-        const currentTransactionIsOnChainFailure =
+        // --------------------------------------------------------------------
-          transaction?.txReceipt?.status === '0x0';
+        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 ||
+          currentTransaction.shouldBePrimary ||
-          currentTransactionIsOnChainFailure ||
+          previousPrimaryTransaction.isEphemeral ||
-          previousPrimaryIsNetworkFailure ||
+          (currentTransaction.occurredAfterPrimary &&
-          (txTime > primaryTxTime && status in PRIORITY_STATUS_HASH)
+            currentTransaction.hasPriorityStatus)
         ) {
           nonceProps.primaryTransaction = transaction;
         }
 
-        const {
+        // Check the above logic cases and assign a new initialTransaction if
-          initialTransaction: { time: initialTxTime = 0 } = {},
+        // appropriate
-        } = nonceProps;
+        if (
-
+          (currentTransaction.occurredBeforeInitial &&
-        // Used to display the transaction action, since we don't want to overwrite the action if
+            currentTransaction.eligibleForInitial) ||
-        // it was replaced with a cancel attempt transaction.
+          (previousInitialTransaction.isEphemeral &&
-        if (txTime < initialTxTime) {
+            currentTransaction.eligibleForInitial)
+        ) {
           nonceProps.initialTransaction = transaction;
         }
 
-        if (
+        if (currentTransaction.isValidRetry) {
-          type === TRANSACTION_TYPES.RETRY &&
-          (status in PRIORITY_STATUS_HASH ||
-            status === TRANSACTION_STATUSES.DROPPED)
-        ) {
           nonceProps.hasRetried = true;
         }
 
-        if (
+        if (currentTransaction.isValidCancel) {
-          type === TRANSACTION_TYPES.CANCEL &&
-          (status in PRIORITY_STATUS_HASH ||
-            status === TRANSACTION_STATUSES.DROPPED)
-        ) {
           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;
+      });
   },
 );