cancel unapproved confirmations on network change#10357
Conversation
cfdc3da to
9a35be4
Compare
Builds ready [9a35be4]
Page Load Metrics (574 ± 37 ms)
|
4950061 to
dc1b808
Compare
Builds ready [dc1b808]
Page Load Metrics (563 ± 35 ms)
|
app/scripts/constants/event-names.js
Outdated
| // Fired when the actively selected network *will* change | ||
| NETWORK_WILL_CHANGE: 'networkWillChange', | ||
| // Fired after state changes that impact the extension badge (unapproved msg count) | ||
| UPDATE_BADGE: 'updateBadge', |
There was a problem hiding this comment.
I'm not sure it makes sense to group these together 🤔 The first two are network controller events, but the third is a MetaMask controller event. They're emitted on entirely different things.
Or at the very least this difference should be more prominent in the comment descriptions.
There was a problem hiding this comment.
@Gudahtt do you have any preference for one of the following:
a. Controller specific event constants declared and exported in the controller file as EVENTS with events that are emitted by many controllers in the metamask-controller file
b. Group events in this event-names file by controller name. NETWORK.WILL_CHANGE and METAMASK_CONTROLLER.UPDATE_BADGE
Either option is good for me, and I agree with your feedback.
There was a problem hiding this comment.
I'd probably choose the first approach, but either one seems fine! These will likely be supplanted by the new controller messaging system in the future anyway I would imagine.
| * Clears all unapproved messages from memory. | ||
| */ | ||
| clearUnapproved() { | ||
| this.memStore.updateState({ |
There was a problem hiding this comment.
For some ungodly reason we store signature requests in two different places in these message manager classes. They are communicated to the UI via this memStore but the primary list is in this.messages, which remains unchanged here. Take a look at getUnapprovedMsgs for example - that would still return messages "cleared" here.
I believe the same applies to the other message manager classes as well.
There was a problem hiding this comment.
Huh, I don't disagree, but I'm very curious how this was working in practice then. I'll dig into that to understand more and submit this fix in the meantime
dc1b808 to
d4ef5bb
Compare
Builds ready [d4ef5bb]
Page Load Metrics (565 ± 36 ms)
|
| * Clears all unapproved messages from memory. | ||
| */ | ||
| clearUnapproved() { | ||
| this.messages = []; |
There was a problem hiding this comment.
This seems to clear all messages 🤔 Not just unapproved messages.
Not sure I'm reading this right or not, but it looks like in prod right now we keep messages indefinitely, but they don't get persisted so they're lost upon restart.
There was a problem hiding this comment.
Since we never do anything with approved messages, and since they get cleared on restart, clearing them when switching networks seems fine 🤷♂️
I guess we should update the method name and description though.
There was a problem hiding this comment.
I decided to just filter out the unapproved messages here instead of resetting them completely. the function signature and description now align with the code.
Builds ready [e13260f]
Page Load Metrics (654 ± 41 ms)
|
Rationale
We will soon be switching a user's actively selected network after they confirm an
addEthereumChainrequest. This can confuse other unapproved confirmations that can be associated with specific chains. This change drops all unapproved confirmations when switching networks.It also adds some new constants for event names, but only cleans up the ones that were touched as part of this work.