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

Use QRKeyring from KeyringController #20502

Merged
merged 10 commits into from
Sep 26, 2023
Merged

Conversation

mikesposito
Copy link
Member

Explanation

With this PR the extension starts to use the QRKeyring provided by KeyringController, instead of declaring and managing it directly in the client.

Manual Testing Steps

  • QR Hardware Wallets should work as before.

Pre-merge author checklist

  • I've clearly explained:
    • What problem this PR is solving
    • How this problem was solved
    • How reviewers can test my changes
  • Sufficient automated test coverage has been added

Pre-merge reviewer checklist

  • Manual testing (e.g. pull and build branch, run in browser, test code being changed)
  • PR is linked to the appropriate GitHub issue
  • IF this PR fixes a bug in the release milestone, add this PR to the release milestone

If further QA is required (e.g. new feature, complex testing steps, large refactor), add the Extension QA Board label.

In this case, a QA Engineer approval will be be required.

@mikesposito mikesposito requested a review from a team as a code owner August 17, 2023 08:41
@github-actions
Copy link
Contributor

github-actions bot commented Aug 17, 2023

CLA Signature Action: All authors have signed the CLA. You may need to manually re-run the blocking PR check if it doesn't pass in a few minutes.

@mikesposito mikesposito changed the title refactor: use QRKeyring from KeyringController Use QRKeyring from KeyringController Aug 17, 2023
@mikesposito mikesposito changed the title Use QRKeyring from KeyringController Use QRKeyring from KeyringController Aug 17, 2023
@mikesposito mikesposito force-pushed the refactor/use-qr-keyring branch from 3a6f05d to 6334e1b Compare August 17, 2023 09:06
@codecov
Copy link

codecov bot commented Aug 17, 2023

Codecov Report

Attention: 6 lines in your changes are missing coverage. Please review.

Comparison is base (ac5bdba) 68.39% compared to head (7bdc1b0) 68.35%.
Report is 1 commits behind head on develop.

Additional details and impacted files
@@             Coverage Diff             @@
##           develop   #20502      +/-   ##
===========================================
- Coverage    68.39%   68.35%   -0.05%     
===========================================
  Files         1007     1007              
  Lines        40252    40260       +8     
  Branches     10764    10770       +6     
===========================================
- Hits         27529    27516      -13     
- Misses       12723    12744      +21     
Files Coverage Δ
app/scripts/controllers/app-state.js 39.10% <100.00%> (+0.29%) ⬆️
app/scripts/metamask-controller.js 53.14% <45.45%> (-0.88%) ⬇️

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

@metamaskbot
Copy link
Collaborator

Builds ready [6334e1b]
Page Load Metrics (1627 ± 49 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint107162129157
domContentLoaded14511885162510149
load14511885162710249
domInteractive14511885162510149
Bundle size diffs [🚀 Bundle size reduced!]
  • background: -164 Bytes (-0.00%)
  • ui: 0 Bytes (0.00%)
  • common: 0 Bytes (0.00%)

@mikesposito mikesposito force-pushed the refactor/use-qr-keyring branch 2 times, most recently from 0263a6c to 6334e1b Compare August 17, 2023 13:30
@metamaskbot
Copy link
Collaborator

Builds ready [6334e1b]
Page Load Metrics (1705 ± 67 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint1092721493617
domContentLoaded15052035170414067
load15062035170513967
domInteractive15052035170414067
Bundle size diffs [🚀 Bundle size reduced!]
  • background: -164 Bytes (-0.00%)
  • ui: 0 Bytes (0.00%)
  • common: 0 Bytes (0.00%)

@cryptodev-2s cryptodev-2s force-pushed the refactor/use-qr-keyring branch from 6334e1b to 2516be6 Compare August 28, 2023 13:11
@metamaskbot
Copy link
Collaborator

Builds ready [2516be6]
Page Load Metrics (1696 ± 163 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint1002401423517
domContentLoaded138323931696340163
load138323931696340163
domInteractive138323931696340163
Bundle size diffs [🚀 Bundle size reduced!]
  • background: -164 Bytes (-0.00%)
  • ui: 0 Bytes (0.00%)
  • common: 0 Bytes (0.00%)

@cryptodev-2s cryptodev-2s force-pushed the refactor/use-qr-keyring branch 2 times, most recently from f200d6e to 1176bd0 Compare August 29, 2023 13:25
@metamaskbot
Copy link
Collaborator

Builds ready [1176bd0]
Page Load Metrics (1864 ± 104 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint1172161522914
domContentLoaded151422861863217104
load151422861864217104
domInteractive151422861863217104
Bundle size diffs [🚀 Bundle size reduced!]
  • background: -164 Bytes (-0.00%)
  • ui: 0 Bytes (0.00%)
  • common: 0 Bytes (0.00%)

@cryptodev-2s cryptodev-2s self-assigned this Aug 30, 2023
@cryptodev-2s cryptodev-2s force-pushed the refactor/use-qr-keyring branch from 1176bd0 to 8b8e5e9 Compare August 30, 2023 16:38
@mikesposito mikesposito force-pushed the refactor/use-qr-keyring branch 4 times, most recently from 0071617 to ccaef57 Compare September 7, 2023 10:32
@metamaskbot
Copy link
Collaborator

Builds ready [ccaef57]
Page Load Metrics (1686 ± 73 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint117183144178
domContentLoaded14071984168515373
load14071984168615373
domInteractive14071984168515373
Bundle size diffs [🚨 Warning! Bundle size has increased!]
  • background: 97 Bytes (0.00%)
  • ui: 22 Bytes (0.00%)
  • common: 12 Bytes (0.00%)

@metamaskbot
Copy link
Collaborator

Builds ready [ccaef57]
Page Load Metrics (1686 ± 73 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint117183144178
domContentLoaded14071984168515373
load14071984168615373
domInteractive14071984168515373
Bundle size diffs [🚨 Warning! Bundle size has increased!]
  • background: 97 Bytes (0.00%)
  • ui: 22 Bytes (0.00%)
  • common: 12 Bytes (0.00%)

@mikesposito mikesposito force-pushed the refactor/use-qr-keyring branch from d0e137c to 83765a0 Compare September 11, 2023 11:01
@metamaskbot
Copy link
Collaborator

Builds ready [83765a0]
Page Load Metrics (1682 ± 84 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint1081971442412
domContentLoaded14212002168117484
load14222003168217584
domInteractive14212002168117484
Bundle size diffs [🚨 Warning! Bundle size has increased!]
  • background: 97 Bytes (0.00%)
  • ui: 22 Bytes (0.00%)
  • common: 12 Bytes (0.00%)

@socket-security
Copy link

socket-security bot commented Sep 26, 2023

Updated and removed dependencies detected. Learn more about Socket for GitHub ↗︎

Packages Version New capabilities Transitives Size Publisher
@metamask/keyring-controller 7.5.0...8.0.0 None +0/-1 140 kB metamaskbot

🚮 Removed packages: @metamask/[email protected], @metamask/[email protected]

@socket-security
Copy link

socket-security bot commented Sep 26, 2023

👍 Dependency issues cleared. Learn more about Socket for GitHub ↗︎

This PR previously contained dependency changes with security issues that have been resolved, removed, or ignored.

@mikesposito mikesposito force-pushed the refactor/use-qr-keyring branch 2 times, most recently from 971bb2b to 01c50cf Compare September 26, 2023 13:41
@mikesposito
Copy link
Member Author

@metamaskbot update-policies

@metamaskbot
Copy link
Collaborator

Policies updated

@metamaskbot metamaskbot requested review from a team as code owners September 26, 2023 14:14
Comment on lines +873 to +889
allowedActions: [
'KeyringController:getState',
'KeyringController:signMessage',
'KeyringController:signPersonalMessage',
'KeyringController:signTypedMessage',
'KeyringController:decryptMessage',
'KeyringController:getEncryptionPublicKey',
'KeyringController:getKeyringsByType',
'KeyringController:getKeyringForAccount',
'KeyringController:getAccounts',
],
allowedEvents: [
'KeyringController:accountRemoved',
'KeyringController:lock',
'KeyringController:stateChange',
'KeyringController:lock',
'KeyringController:unlock',
'KeyringController:accountRemoved',
'KeyringController:qrKeyringStateChange',
Copy link
Member Author

Choose a reason for hiding this comment

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

These are related to @metamask/keyring-controller update v7.4 -> v8.0

@metamaskbot
Copy link
Collaborator

Builds ready [dfa7991]
Page Load Metrics (1504 ± 241 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint87133102136
domContentLoaded7013097157
load8121551504503241
domInteractive7013097157
Bundle size diffs [🚨 Warning! Bundle size has increased!]
  • background: 178.19 KiB (5.02%)
  • ui: 0 Bytes (0.00%)
  • common: 0 Bytes (0.00%)

@mikesposito
Copy link
Member Author

@SocketSecurity ignore @metamask/[email protected]

This is our package

@mikesposito mikesposito requested a review from Gudahtt September 26, 2023 15:30
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!

@mikesposito mikesposito force-pushed the refactor/use-qr-keyring branch from dfa7991 to 7bdc1b0 Compare September 26, 2023 15:37
Copy link
Contributor

@kanthesha kanthesha left a comment

Choose a reason for hiding this comment

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

LGTM!

@mikesposito mikesposito merged commit 941b734 into develop Sep 26, 2023
9 checks passed
@mikesposito mikesposito deleted the refactor/use-qr-keyring branch September 26, 2023 16:19
@github-actions github-actions bot locked and limited conversation to collaborators Sep 26, 2023
@metamaskbot metamaskbot added the release-11.3.0 Issue or pull request that will be included in release 11.3.0 label Sep 26, 2023
@metamaskbot
Copy link
Collaborator

Builds ready [7bdc1b0]
Page Load Metrics (969 ± 339 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint831921012613
domContentLoaded71187942512
load821638969707339
domInteractive71187942512
Bundle size diffs [🚨 Warning! Bundle size has increased!]
  • background: 178.19 KiB (5.01%)
  • ui: 0 Bytes (0.00%)
  • common: 0 Bytes (0.00%)

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
release-11.3.0 Issue or pull request that will be included in release 11.3.0 team-wallet-framework
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Keyring consolidation: Use QRKeyring from core KeyringController
5 participants