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.

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

chore: Refactor offscreen creation logic #25302

Merged
merged 6 commits into from
Jun 18, 2024

Conversation

FrederikBolding
Copy link
Member

@FrederikBolding FrederikBolding commented Jun 13, 2024

Description

Refactor initialization logic to defer creation of the offscreen document until the MetaMaskController is initialized. This adds a offscreenPromise to the controller that can be awaited for functionality that requires the offscreen document to be created.

Additionally this PR adds a message that the offscreen document will send once initial execution of the offscreen page has finished. This is awaited in the offscreenPromise.

We await offscreenPromise before unlocking the keyrings as some keyrings rely on the offscreen document to process requests, e.g. hardware wallets.

There may be room for more improvements here though, that I have not tackled in this PR. As the hardware wallet logic doesn't seem to wait for iframes to fully load, so there is a chance of some missed messages.

I have tested that hardware wallet support, at least for Ledger, is still working following the changes in this PR.

Open in GitHub Codespaces

@FrederikBolding FrederikBolding requested review from a team as code owners June 13, 2024 16:29
@metamaskbot metamaskbot added the INVALID-PR-TEMPLATE PR's body doesn't match template label Jun 13, 2024
@FrederikBolding FrederikBolding force-pushed the fb/offscreen-boot-refactor branch 2 times, most recently from cc677f2 to 1cf397e Compare June 14, 2024 08:34
@metamaskbot
Copy link
Collaborator

Builds ready [219ff99]
Page Load Metrics (173 ± 247 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint70253993718
domContentLoaded10271342
load452410173513247
domInteractive10271342
Bundle size diffs [🚨 Warning! Bundle size has increased!]
  • background: 1.16 KiB (0.03%)
  • ui: 0 Bytes (0.00%)
  • common: 28 Bytes (0.00%)

Copy link

codecov bot commented Jun 14, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 65.41%. Comparing base (68d35f0) to head (bf601f7).

Additional details and impacted files
@@             Coverage Diff             @@
##           develop   #25302      +/-   ##
===========================================
- Coverage    65.42%   65.41%   -0.01%     
===========================================
  Files         1381     1381              
  Lines        54695    54711      +16     
  Branches     14340    14351      +11     
===========================================
+ Hits         35779    35786       +7     
- Misses       18916    18925       +9     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

david0xd
david0xd previously approved these changes Jun 14, 2024
GuillaumeRx
GuillaumeRx previously approved these changes Jun 14, 2024
app/scripts/offscreen.js Outdated Show resolved Hide resolved
@FrederikBolding FrederikBolding dismissed stale reviews from GuillaumeRx and david0xd via dd14d6f June 18, 2024 10:45
@FrederikBolding FrederikBolding force-pushed the fb/offscreen-boot-refactor branch from 219ff99 to dd14d6f Compare June 18, 2024 10:45
@FrederikBolding FrederikBolding requested a review from Gudahtt June 18, 2024 10:46
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!

@metamaskbot
Copy link
Collaborator

Builds ready [bf601f7]
Page Load Metrics (51 ± 3 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint671058594
domContentLoaded9221231
load42645163
domInteractive9221231
Bundle size diffs [🚨 Warning! Bundle size has increased!]
  • background: 1.26 KiB (0.04%)
  • ui: 0 Bytes (0.00%)
  • common: 28 Bytes (0.00%)

@FrederikBolding FrederikBolding merged commit e70e44f into develop Jun 18, 2024
74 checks passed
@FrederikBolding FrederikBolding deleted the fb/offscreen-boot-refactor branch June 18, 2024 11:43
@github-actions github-actions bot locked and limited conversation to collaborators Jun 18, 2024
@metamaskbot metamaskbot added release-12.1.0 Issue or pull request that will be included in release 12.1.0 release-12.0.0 Issue or pull request that will be included in release 12.0.0 and removed release-12.1.0 Issue or pull request that will be included in release 12.1.0 labels Jun 18, 2024
@metamaskbot
Copy link
Collaborator

Missing release label release-12.0.0 on PR. Adding release label release-12.0.0 on PR and removing other release labels(release-12.1.0), as PR was cherry-picked in branch 12.0.0.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
INVALID-PR-TEMPLATE PR's body doesn't match template release-12.0.0 Issue or pull request that will be included in release 12.0.0 team-extension-platform
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants