David pointed out something on a call we had where he went over some of his findings. the status 'failed' is not in the PRIORITY_STATUS_HASH.

The only way for a failed transaction to be the primary transaction is simply it being the latest transaction for a nonce. The transactionSelector that is a predicate of this selector, returns a list of transactions sorted by time. Therefore cancellations, retries, and transactions submitted with custom nonce equal to an existing transaction nonce will always be the primaryTransaction for a group.

initialTransaction gets set appropriately due to the comparison of time

I don't know if only network failures have this error object, but if that's true then that seems like a pretty good way to draw the distinction.

There are two failure states we'd want to catch here:

primaryTransaction.status === FAILED and primaryTransaction.txReceipt.status === '0x0' these are two separate failure states. The latter could be primaryTransaction.status === CONFIRMED

additionally you don't want to drop the CONFIRMED check, here..so a stab at the logic might be:

const previousPrimaryIsFailed = nonceProps.primaryTransaction.status === FAILED_STATUS || nonceProps.primaryTransaction?.txReceipt?.status === '0x0';

if (status === CONFIRMED_STATUS || previousPrimaryIsFailed || txTime > primaryTxTime) {
  // do stuff
}

Hmm. That doesn't seem quite right either. We want to treat the two types of failure (on-chain and network) in opposite ways, not in the same way.

e.g. something like this:

const previousPrimaryIsNetworkFailure = nonceProps.primaryTransaction.status === FAILED_STATUS && nonceProps.primaryTransaction?.txReceipt?.status !== '0x0';
const currentTransactionIsOnChainFailure = transaction.txReceipt?.status === '0x0'

if (status === CONFIRMED_STATUS || currentTransactionIsOnChainFailure || previousPrimaryIsNetworkFailure || txTime > primaryTxTime) {
  // do stuff
}

So there are three priorities here. Anything should be prioritized over a network failure, but a status of confirmed or an on-chain failure should be prioritized above pending, because they both represent a "final" state for the transaction that can't change from there on.

Though, we may be able to assume that on-chain failures have a status of confirmed. I was unable to confirm that it was possible for an on-chain failure to get assigned a failed status.

We're off on something because, per my PR's console statements, the primary transaction is being replaced every time.

Oh! Sorry I was imagining my snippet as being inside the if (status in PRIORITY_STATUS_HASH) block. Though that wouldn't have made sense either because FAILED isn't in that object.

So maybe something like this:

if (status === CONFIRMED_STATUS || currentTransactionIsOnChainFailure || previousPrimaryIsNetworkFailure || (txTime > primaryTxTime && status in PRIORITY_STATUS_HASH)) {

The intent here is to use the most recent transaction among the "priority" statuses - e.g. the ones we typically prefer to display. The on-chain failure is one strange exception to this, as is the "previous transaction network failure" case.

So I have tested our usual workflow for triggering the bug reported in #9435, and this fix resolves that issue, for sure;

I have also tested what happens if a user submits a custom nonce transaction with the same nonce as a group of transactions in the state described in #9435 👍

1. submit tx on Rinkeby with 0.6 gasPrice.
2. Speed up tx with gasPrice 1 and extremely high gasLimit (above network tolerance)
3. --on develop this tx would now show as failed, on this PR it shows as pending (correct)--
4. Submit a new transaction with the same nonce as the prior two, with a different value to be able to distinguish in UI
5. TX in UI updates to reflect new value in the pending state
6. TX is confirmed and continues to reflect the appropriate custom nonce tx.

The only thing I haven't yet been able to test is on-chain failures because I don't know how to replicate them -- however, the logic looks good, and it would follow the same trajectory as network failures, so I think I'm 👍 on this. I have a couple of nits/thoughts.

const previousPrimaryIsNetworkFailure = nonceProps.primaryTransaction.status === FAILED_STATUS && nonceProps.primaryTransaction?.txReceipt?.status !== '0x0';

I'm not sure that the latter half of the check is necessary; we'd want to handle on-chain failures as the primary transaction in the same way as network failures. Any TX that is failed should be superseded with a non-failed tx of the same nonce, regardless of failure reason.

const currentTransactionIsOnChainFailure = transaction?.txReceipt?.status === '0x0'

So, previously if an on-chain failure were in the 'confirmed' status, it would have been appropriately marked as the primaryTransaction. However, transactions that were in a 'failed' status would have been skipped and potentially not set as the primaryTransaction. So I think this logic in the if block makes sense to make sure that on-chain failures are properly identified in the UI. However, if the previous primaryTransaction is still pending, we wouldn't want the on-chain failure to supersede it, which would result in a similar scenario like the one mentioned in #9435. It should be noted that I'm running through a mental model here and haven't tested with real or synthetic on-chain failures.

Example Scenario:

3 transactions, oldest to newest
1 - Pending transaction
2 - On-chain failure
3 - Network failure

1. on evaluating the first transaction 1 is set as both primary and initial.
2. on evaluating the second transaction, transaction 2 would be set at the primary transaction due to the || currentTransactionIsOnChainFailure check
3. on evaluating the third transaction it is skipped due to it not being confirmed, the current transaction is not an on-chain failure, the previous primaryTransaction is not a network failure and its status is not in the PRIORITY_STATUS_HASH.

The logic does seem to work the same regardless of the order of transactions:

1. On evaluating the third transaction (now first) it is set as both primary and initial
2. On evaluating the second transaction it is set as the primary transaction due to being an on-chain failure.
3. On evaluating the first transaction (now third) it is skipped because it's an older transaction.

However, if the previous primaryTransaction is still pending, we wouldn't want the on-chain failure to supersede it, which would result in a similar scenario like the one mentioned in #9435.

If a transaction has been confirmed or failed on-chain, it should not be possible for it to still be "pending" as well. That's... kinda what it means for it to be confirmed. Or that was my understanding anyway. That's why the confirmed status was treated as a higher priority than the others.

At least to my understanding, you're right that an on-chain failure would prevent any transaction with the same nonce from being accepted -- an on-chain failure most likely means we should display a failed transaction to the user. I think that might make my other feedback counterproductive as well, we'd want to override a failed transaction as primary only if it was not an on-chain failure. Protecting us from accidentally overriding a transaction that was, in reality, confirmed but then failed on-chain.

I think I might be taking back my feedback -- meaning this would be good as is?

I should have seen #9687 (comment) and the subsequent conversation. My apologies for leading this astray and countering your advice, @Gudahtt

No problem! I agree that this seems correct as-is.

Sorry for the confusion @darkwing!