Synchronously update transaction status

[Gudahtt]

All transaction status updates were moved into a setTimeout callback and wrapped in a try...catch block in #4131, apparently in an attempt to prevent failures in event subscribers from interrupting the transaction logic. The try...catch block did accomplish that, but by putting the status update in a setTimeout callback the operation was made asynchronous.

Transaction status updates now happen unpredictably, in some future event loop from when they're triggered. This creates a race condition, where the transaction status update may occur before or after subsequent state changes. This also introduces a risk of accidentally undoing a change to the transaction state, as the update made to the transaction inside the setTimeout callback uses a reference to txMeta obtained synchronously before the setTimeout call. Any replacement of the txMeta between the setTxStatus call and the execution of the timeout would be erased. Luckily the txMeta object is more often than not mutated rather than replaced, which may explain why we haven't seen this happen yet.

Everything seems to work correctly with the setTimeout call removed, and now the transaction logic is easier to understand.

[brad-decker]

Noice, Does this make the logic we discussed in slack more reliable vis-a-vis the nonce sorted list being dependant on the first tx to be resolved before the second tx can be processed? Thus making a sort of 'queue' terminology or ux pattern appropriate?

[Gudahtt]

This was the result of the investigation I did in attempting to answer that question 😅 . I don't think this changes anything regarding the transaction list UI - both before and after this change, the queue UX pattern was appropriate. But the logic should be easier to follow now - this should ensure that if the transaction has a nonce, it must have been approved as well (previously it may have been given a nonce first, before being set to approved). it might also prevent some bug that I'm not aware of, e.g. a transaction getting into an invalid state. I plan on doing more investigation later into what problems this may have caused - I didn't find any bugs yet, but I haven't yet looked very hard.

[metamaskbot]

Builds ready [8234d5e]

• builds: chrome, firefox, opera
• bundle viz: background, ui, inpage, contentscript, ui-libs, bg-libs, phishing-detect
• dep viz: background
• all artifacts

Page Load Metrics (680 ± 29 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	31	80	39	11	5
		domContentLoaded	610	811	679	60	29
		load	612	813	680	60	29
		domInteractive	610	811	678	59	29

[whymarrh]

It’s also worth noting that #4131 was before “safe-event-emitter” as well 🤦🏽‍♂️