Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Wait for extension unlock before processing eth_requestAccounts #8149

Merged
merged 12 commits into from
Mar 23, 2020
Merged

Wait for extension unlock before processing eth_requestAccounts #8149

merged 12 commits into from
Mar 23, 2020

Conversation

rekmarks
Copy link
Member

@rekmarks rekmarks commented Mar 2, 2020 •
edited
Loading

Summary

  • Adds unlock event and related functionality to controllers:
    • Updates eth-keyring-controller to version with unlock event
    • MetamaskController
      • Add addUnlockListener method
    • AppStateController
      • Add functionality for waiting on extension unlock
      • Add methods: getUnlockPromise, handleUnlock
    • Adds to notification badge count for every request waiting on unlock
  • PermissionsController: Waits for the extension to become unlocked before attempting to process eth_requestAccounts, if the eth_accounts permission is already granted

Details

  • Does not open the extension popup when receiving eth_requestAccounts if the eth_accounts permission is already granted
    • We should create a separate PR if we want to support this functionality, as we have to make the popup close on unlock if there are no notifications/confirmations to attend to, which is its own set of work

@rekmarks rekmarks requested a review from danjm as a code owner March 2, 2020 02:51
@metamaskbot
Copy link
Collaborator

Builds ready [0b34220]
Page Load Metrics (694 ± 51 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeNotificationfirstPaint36128582713
domContentLoaded36392369310651
load36792569410551
domInteractive36292369210651

@metamaskbot
Copy link
Collaborator

Builds ready [f0774b7]
Page Load Metrics (811 ± 35 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeNotificationfirstPaint39116692412
domContentLoaded71910518097435
load72010538117435
domInteractive71810518097435

@metamaskbot
Copy link
Collaborator

Builds ready [f37486f]
Page Load Metrics (600 ± 50 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeNotificationfirstPaint368747178
domContentLoaded34167659910349
load34267760010350
domInteractive34167659810349

@rekmarks rekmarks added the DO-NOT-MERGE Pull requests that should not be merged label Mar 2, 2020
@rekmarks rekmarks force-pushed the account-await-unlock branch 2 times, most recently from 11dac4a to 6797e4d Compare March 6, 2020 19:15
@metamaskbot
Copy link
Collaborator

Builds ready [6797e4d]
Page Load Metrics (642 ± 57 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeNotificationfirstPaint3691522010
domContentLoaded35377264011957
load35577564211957
domInteractive35377264011957

@rekmarks rekmarks force-pushed the account-await-unlock branch from 6797e4d to c03451f Compare March 6, 2020 23:15
@metamaskbot
Copy link
Collaborator

Builds ready [c03451f]
Page Load Metrics (684 ± 49 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeNotificationfirstPaint3910951188
domContentLoaded37283068310249
load37783168410149
domInteractive37282968210249

@metamaskbot
Copy link
Collaborator

Builds ready [6064c50]
Page Load Metrics (585 ± 67 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeNotificationfirstPaint339543146
domContentLoaded32087658313866
load32287858513967
domInteractive32087658313866

@metamaskbot
Copy link
Collaborator

Builds ready [597ac58]
Page Load Metrics (583 ± 37 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeNotificationfirstPaint339143157
domContentLoaded3396375817838
load3436385837837
domInteractive3396365817838

@rekmarks rekmarks force-pushed the account-await-unlock branch from 597ac58 to 1eec2c2 Compare March 16, 2020 17:15
@metamaskbot
Copy link
Collaborator

Builds ready [1eec2c2]
Page Load Metrics (599 ± 47 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeNotificationfirstPaint34126512713
domContentLoaded3357025979747
load3377045999747
domInteractive3347025979847

@rekmarks rekmarks added area-permissions Issues relating to exposing permissions from the trusted MetaMask context to less-trusted contexts. and removed DO-NOT-MERGE Pull requests that should not be merged labels Mar 16, 2020
@metamaskbot
Copy link
Collaborator

Builds ready [8b4c19e]
Page Load Metrics (591 ± 58 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeNotificationfirstPaint358949199
domContentLoaded33472658912158
load33972859112058
domInteractive33472658912158

Gudahtt
Gudahtt requested changes Mar 16, 2020
View reviewed changes
Copy link
Member

@Gudahtt Gudahtt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure we're handing keyring construction properly; i.e. we might need to emit unlocked after the initial keyring construction, to drain anything queued up during onboarding

app/scripts/metamask-controller.js Outdated Show resolved Hide resolved
app/scripts/controllers/permissions/methodMiddleware.js Show resolved Hide resolved
@metamaskbot
Copy link
Collaborator

Builds ready [d2ae43d]
Page Load Metrics (831 ± 32 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeNotificationfirstPaint40125792914
domContentLoaded72810368296732
load73510408316732
domInteractive72810368286732

@metamaskbot
Copy link
Collaborator

Builds ready [7820d6d]
Page Load Metrics (667 ± 35 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeNotificationfirstPaint36120572612
domContentLoaded4217606657235
load4237616677235
domInteractive4217596657235

@rekmarks rekmarks force-pushed the account-await-unlock branch from f97d2a7 to 57759aa Compare March 18, 2020 17:07
@rekmarks rekmarks requested a review from Gudahtt March 18, 2020 17:17
@metamaskbot
Copy link
Collaborator

Builds ready [e373bf0]
Page Load Metrics (684 ± 46 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeNotificationfirstPaint38118642813
domContentLoaded3568566829646
load3588576849646
domInteractive3568556829646

@rekmarks rekmarks requested a review from Gudahtt March 20, 2020 15:58
@rekmarks rekmarks requested a review from jennypollack as a code owner March 20, 2020 17:31
Gudahtt
Gudahtt reviewed Mar 20, 2020
View reviewed changes
yarn.lock Show resolved Hide resolved
@metamaskbot
Copy link
Collaborator

Builds ready [feaa634]
Page Load Metrics (649 ± 33 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeNotificationfirstPaint358945115
domContentLoaded3817246476833
load3867256496833
domInteractive3817246476833

@metamaskbot
Copy link
Collaborator

Builds ready [56433c5]
Page Load Metrics (649 ± 20 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeNotificationfirstPaint3511546178
domContentLoaded5977886484220
load5987906494220
domInteractive5977876474220

@rekmarks rekmarks mentioned this pull request Mar 20, 2020
Merged
@rekmarks rekmarks force-pushed the account-await-unlock branch from 56433c5 to 8e29b91 Compare March 20, 2020 19:53
@rekmarks rekmarks requested a review from Gudahtt March 20, 2020 20:07
rekmarks
rekmarks commented Mar 20, 2020
View reviewed changes
yarn.lock Show resolved Hide resolved
@metamaskbot
Copy link
Collaborator

Builds ready [29265b6]
Page Load Metrics (538 ± 57 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeNotificationfirstPaint338141136
domContentLoaded32067353611957
load32467453811857
domInteractive31967253611957

Gudahtt
Gudahtt approved these changes Mar 20, 2020
View reviewed changes
Copy link
Member

@Gudahtt Gudahtt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! Seems to work for me locally after that last Keyring Controller update

@metamaskbot
Copy link
Collaborator

Builds ready [28bcb95]
Page Load Metrics (663 ± 25 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeNotificationfirstPaint378847147
domContentLoaded6137976625325
load6157996635325
domInteractive6137976625325

@danfinlay danfinlay merged commit 2301d99 into develop Mar 23, 2020
@danfinlay danfinlay deleted the account-await-unlock branch March 23, 2020 16:25
@rekmarks
Copy link
Member Author

Note for posterity: force-merged because a codeowner temporarily lost access to GitHub.

@tennox tennox mentioned this pull request Apr 15, 2021
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Gudahtt Gudahtt Gudahtt approved these changes

@danjm danjm Awaiting requested review from danjm

@danfinlay danfinlay Awaiting requested review from danfinlay

@whymarrh whymarrh Awaiting requested review from whymarrh

@jennypollack jennypollack Awaiting requested review from jennypollack

Assignees
No one assigned
Labels
area-permissions Issues relating to exposing permissions from the trusted MetaMask context to less-trusted contexts.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@rekmarks @metamaskbot @Gudahtt @danfinlay