Add "Retry" option for failed transactions

[rickycodes]

re: #7193

saw the this.handleRetry functionality and just reused that? not sure if this is actually what we want mind you. also do we want this to only be available for failed transactions? or does it make sense for it to be displayed on pending as well?

[rickycodes]

Okay, I think this is correct (functionality wise):

I did remove some stuff from ui/app/pages/routes/index.js and ui/app/components/app/sidebars/sidebar.component.js to get the sidebar to appear in the case of a failed transaction (which left the test-lint job broken). I'm not sure what the history around sidebarShouldClose is, but things seem to work without it? I didn't want to completely remove the code surrounding that stuff as I am sure it's needed in some other context that I am blissfully unaware of.

[Gudahtt]

Oh wait, maybe I understand sidebarShouldClose now - perhaps it was intended to close the sidebar if the transaction completes while the sidebar is open? In which case a speedup would no longer be needed. cc @danjm

[danjm]

@Gudahtt Well, I didn't remember its purpose, but it does sidebarShouldClose have the affect you've described. Essentially, if the sidebar is open and one of two changes occur, it will close:
(1) appState.sidebar goes from having a truthy transaction property to not having one
(2) appState.sidebar.transaction.id is no longer found in the ids of the currently pending transactions

The only real use case for that is what you describe: when the sidebar is open for a transaction that goes from pending to either failed or submitted before the sidebar is closed by the user.

[danjm]

Left a few comments, but this is pretty closed to finished.

[rickycodes]

@danjm it seems to me we should just be calling dispatchHideSidebar() in the transaction container instead of the sidebarShouldClose prop stuff that's currently there?

(2) appState.sidebar.transaction.id is no longer found in the ids of the currently pending transactions

that will not work in the case of a failed transaction if I am understanding things correctly?

[danjm]

it seems to me we should just be calling dispatchHideSidebar() in the transaction container instead of the sidebarShouldClose prop stuff that's currently there

Maybe... let me think through and edge case out loud. Suppose the sidebar is opened for a pending transaction, and then when that transaction goes from submitted to either failed or confirmed, we dispatch the action from the transactions component... this makes sense.

Now, what if we open the sidebar for a retry for a failed transaction while there is another transaction pending, and then while the sidebar is open the pending transaction is confirmed. Then I guess the transaction-list-item needs to know the id of the transaction in the sidebar and decide whether to dispatch the action based on if the id matches the transaction going from submitted to confirmed...

Yeah I guess that will be okay too. The only gotcha could be that there are two different lists of transactions (pendingTransactions and completedTransactions) and when one goes from pending to confirmed/failed, it is remove from one list and added to the other. This might be addressable by dispatching the action at the transaction-list level, or by maybe by making use of componentWillUnmount. Just be aware of that issue.

In terms of number of code changes, I think my alternative solution proposed in my earlier comment will be simplest, but I think what you've suggested might be cleanest and most intuitive (because the transactionShouldClose thing is definitely not that intuitive)

[rickycodes]

Okay, this should be good now /cc @danjm

[danjm]

Looks good now! Nice work

[whymarrh]

LGTM, let's be sure to use the "Squash and merge" option here on GitHub

[Gudahtt]

I suspect that this would result in the sidebar staying open if it went from pending to failed, which seems... bad. 🤔

[Gudahtt]

We could move sidebarShouldClose into component state (along with the status), then compare in getDerivedStateFromProps the current status to the previous status to determine whether such a transition occurred, or whether it was failed from the start.

Or even better, we could move it into Redux. We could respond to UPDATE_METAMASK_STATE in the appState store. If the sidebar is open, and if the sidebar transaction has just transitioned from a pending state, then we could set the sidebar to closed directly.

In general I think we should be responding to background state changes in Redux, not in the UI. Either in the reducers or the actions. In this case, responding in the reducer seems to make the most sense.
(Well technically if we were to do this in an action, it'd be less "responding" and more "coordinating changes")

[rickycodes]

should we revert then? I was looking at this and thinking the same thing re: "Or even better, we could move it into Redux. We could respond to UPDATE_METAMASK_STATE in the appState store"...

If this is going to cause issues in rc we should not ?

[Gudahtt]

Well... given that the impact of this regression is minor, I think it'd be fine to leave it if we think we can fix it within the week (before the RC is released). Then we can revert as a fallback, if that timeline doesn't work out.

[danjm]

yeah, I don't think a revert is necessary. This will happen very very rarely, but should still be fixed.

[rickycodes]

okay we can spend some time this week looking at this