-
Notifications
You must be signed in to change notification settings - Fork 5k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Open notification UI when eth_requestAccounts waits for unlock #8508
Conversation
Builds ready [15ccf9e]
Page Load Metrics (652 ± 36 ms)
|
app/scripts/controllers/app-state.js
Outdated
* @returns {Promise<void>} A promise that resolves when the extension is | ||
* unlocked, or immediately if the extension is already unlocked. | ||
*/ | ||
getUnlockPromise () { | ||
getUnlockPromise (showUnlockRequest) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: maybe this could be renamed to shouldShowUnlockRequest
? Since the function is also called showUnlockRequest
, I thought this was the function on my first read through until reading that JSDoc comment more closely.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Or, alternatively, you could remove this parameter. It looks like it's always set to true
in practice.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point. I renamed it. It is always set to true
in practice, but in case we ever want to use it for something else, it ought to save someone else time in the future, at virtually no cost today.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm having trouble thinking of a situation where we'd want it to be false
though. In general, keeping a parameter in "just in case" seems like an odd choice, but 🤷♂️
Builds ready [6733155]
Page Load Metrics (682 ± 18 ms)
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
Summary
This is a suggested feature.
Callers of
AppStateController.getUnlockPromise
can now cause the notification UI to open via a boolean parameter.Justification
Currently, if the user lands a dapp that has permissions when the extension is locked, the dapp can't tell whether the user is already connected. Therefore, the dapp is likely to display a "Connect" button. This is what we do in our test dapp.
If the user clicks a "Connect" button in the above case, the extension browser action icon will get a badge, but the UI won't automatically open. This is in distinction to clicking a "Connect" button in a dapp that does not have permissions, which will cause the UI to open.
With this PR, the UI will open when the user clicks "Connect", regardless of whether the dapp has permissions or not. If the dapp has permissions, the UI will immediately close upon unlocking.
I believe that this is probably better than the UI not opening at all; it certainly feels more responsive to me.