Transaction notifications #4840
Conversation
|
Hi @danfinlay Could you please review this PR? I think notifications for dropped transactions is also informative, so I added that also. However it can cause problems in Firefox as mentioned in the documentation:
I experienced this a few times. Also, about "View on EtherScan" on the confirmed transaction's notification: it cannot be added as a link, so I had to handle the notifications.OnClicked event to open the transaction details on a new tab. |
There was a problem hiding this comment.
Thanks so much, @scsaba!
Two small changes I would request:
- Let's remove the dropped tx handler, the firefox issue puts it over the top for now.
- Please add a changelog entry under the
masterheader, notingNow shows notifications when transactions are completed.
app/scripts/metamask-controller.js
Outdated
| this.txController.on(`tx:status-update`, (txId, status) => { | ||
| if (status === 'confirmed' || status === 'failed' || status === 'dropped') { | ||
| const txMeta = this.txController.txStateManager.getTx(txId) | ||
| this.platform.showTransactionNotification(txMeta) |
There was a problem hiding this comment.
I like the generic handler.
app/scripts/platforms/extension.js
Outdated
| } else if (status === 'failed') { | ||
| this._showFailedTransaction(txMeta) | ||
| } else if (status === 'dropped') { | ||
| this._showDroppedTransaction(txMeta) |
There was a problem hiding this comment.
I was already on the fence about showing dropped txs, but if Firefox risks not showing any, I think I'd rather remove the dropped handler.
|
@danfinlay thank you for the quick review. I applied the requested changes. I was not sure about the changelog entry if I need to add the pull request nr also, I hope this is the correct version :) |
Fixes #4203