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

refactor: use withKeyring method (#25435) #27025

Open
wants to merge 12 commits into
base: main
Choose a base branch
from

Conversation

mikesposito
Copy link
Member

@mikesposito mikesposito commented Sep 10, 2024

Description

This PR cherry-picks def6b15 into develop, after being reverted at 821c3bd. The reason the original commit was reverted is explained in #26840.

From the original PR (#25435):

With @metamask/keyring-controller v16 a new method is available which simplify safe direct keyring interactions which are not available through KeyringController.

Through withKeyring it's possible to lock KeyringController's main mutex and select a specific keyring to interact with while KeyringController will be unusable concurrently. withKeyring also takes care of persisting keyrings and updating the controller state at the end of the operation, which makes it possible to use it in substitution of getKeyringsByType and persistAllKeyrings.

Currently, most of the direct keyring interactions in the extension are made for hardware devices and snaps.

Open in GitHub Codespaces

Related issues

Fixes: #26840
Fixes: #24276

Manual testing steps

As for the original PR (#25435):

Affected workflows may include:

  • Lock
  • Unlock
  • Add account
  • Remove account
  • Connect hardware wallet
  • All hardware wallet interactions

Screenshots/Recordings

Before

After

Pre-merge author checklist

Pre-merge reviewer checklist

  • I've manually tested the PR (e.g. pull and build branch, run the app, test code being changed).
  • I confirm that this PR addresses all acceptance criteria described in the ticket it closes and includes the necessary testing evidence such as recordings and or screenshots.

@metamaskbot
Copy link
Collaborator

Builds ready [6a165c8]
Page Load Metrics (1757 ± 85 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint24621991690374179
domContentLoaded14742189174017484
load14822197175717685
domInteractive15189453919
Bundle size diffs [🚨 Warning! Bundle size has increased!]
  • background: 225 Bytes (0.01%)
  • ui: 0 Bytes (0.00%)
  • common: 0 Bytes (0.00%)

@mikesposito mikesposito marked this pull request as ready for review September 10, 2024 14:57
@mikesposito mikesposito requested a review from a team as a code owner September 10, 2024 14:57
@mikesposito mikesposito requested a review from a team September 10, 2024 14:57
Copy link

codecov bot commented Sep 16, 2024

Codecov Report

Attention: Patch coverage is 64.00000% with 27 lines in your changes missing coverage. Please review.

Project coverage is 69.95%. Comparing base (aeb91c9) to head (d5005d0).

Files with missing lines Patch % Lines
app/scripts/metamask-controller.js 64.00% 27 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff             @@
##           develop   #27025      +/-   ##
===========================================
- Coverage    69.96%   69.95%   -0.00%     
===========================================
  Files         1441     1441              
  Lines        50102    50104       +2     
  Branches     14012    14008       -4     
===========================================
  Hits         35049    35049              
- Misses       15053    15055       +2     

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

@metamaskbot
Copy link
Collaborator

Builds ready [c483c52]
Page Load Metrics (1813 ± 106 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint20324291672513247
domContentLoaded153123841792210101
load153924311813221106
domInteractive15130432612
Bundle size diffs [🚨 Warning! Bundle size has increased!]
  • background: 225 Bytes (0.01%)
  • ui: 0 Bytes (0.00%)
  • common: 0 Bytes (0.00%)

cryptodev-2s
cryptodev-2s previously approved these changes Sep 16, 2024
Copy link

sonarcloud bot commented Sep 16, 2024

@metamaskbot
Copy link
Collaborator

Builds ready [d5005d0]
Page Load Metrics (1705 ± 104 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint22421991634393189
domContentLoaded143421441690213102
load144221481705216104
domInteractive14102352210
Bundle size diffs [🚨 Warning! Bundle size has increased!]
  • background: 225 Bytes (0.01%)
  • ui: 0 Bytes (0.00%)
  • common: 0 Bytes (0.00%)

@metamaskbot
Copy link
Collaborator

Builds ready [9ad5aad]
Page Load Metrics (1813 ± 91 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint31022351735376181
domContentLoaded15562225178518790
load15642235181318991
domInteractive17181514120
Bundle size diffs [🚨 Warning! Bundle size has increased!]
  • background: 225 Bytes (0.01%)
  • ui: 0 Bytes (0.00%)
  • common: 0 Bytes (0.00%)

@metamaskbot
Copy link
Collaborator

Builds ready [32eeb35]
Page Load Metrics (1770 ± 134 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint31221011647340163
domContentLoaded148428211748282136
load149128261770280134
domInteractive157938178
Bundle size diffs [🚨 Warning! Bundle size has increased!]
  • background: 225 Bytes (0.01%)
  • ui: 0 Bytes (0.00%)
  • common: 0 Bytes (0.00%)

Copy link

sonarcloud bot commented Sep 26, 2024

Quality Gate Failed Quality Gate failed

Failed conditions
68.6% Coverage on New Code (required ≥ 80%)

See analysis details on SonarCloud

@mikesposito
Copy link
Member Author

This is currently blocked by https://github.com/MetaMask/accounts-planning/issues/615

Copy link
Contributor

This PR has been automatically marked as stale because it has not had recent activity in the last 60 days. It will be closed in 14 days. Thank you for your contributions.

@github-actions github-actions bot added stale issues and PRs marked as stale and removed stale issues and PRs marked as stale labels Nov 29, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Needs more work from the author
3 participants