Skip to content

Commit e085b42

Browse files
refactor: use @metamask/eth-qr-keyring (#33893)
<!-- Please submit this PR as a draft initially. Do not mark it as "Ready for review" until the template has been completely filled out, and PR status checks have passed at least once. --> dependent on: - ~~MetaMask/accounts#60 - ~~MetaMask/core#6031 ## **Description** <!-- Write a short description of the changes included in this pull request, also include relevant motivation and context. Have in mind the following questions: 1. What is the reason for the change? 2. What is the improvement/solution? --> This PR uses the `@metamask/eth-qr-keyring` package to handle QR code keyring functionality, replacing the previous implementation from `@keystonehq/metamask-airgapped-keyring`. The change should be transparent to users, as the package is a drop-in replacement. Though, under the hood, it allows to greatly simplify code related to QRKeyring-related scanning and signing, and should improve the overall ergonomics of interactions with QRKeyring. Moreover, this PR adds the first e2e tests for QR code hardware wallets, covering: - Pairing a QR code hardware wallet - Adding multiple accounts from the QR code hardware wallet [![Open in GitHub Codespaces](https://github.com/codespaces/badge.svg)](https://codespaces.new/MetaMask/metamask-extension/pull/33893?quickstart=1) ## **Related issues** Fixes: ## **Manual testing steps** ### **Does it work with new HW devices?** 1. Checkout to this branch, and run `yarn && yarn webpack` or `yarn && yarn start` to build the extension. 2. Install the extension in your browser, and pair a QR-based hardware wallet (e.g. a Keystone device, or install AirGap Vault or imToken on an iOS/Android device). 3. Add some (3) accounts to the QRKeyring, and ensure that you can see them in the extension. 4. Go to `https://metamask.github.io/test-dapp/`, connect to the extension. 5. Ensure that you can: - Sign transactions - Sign typed data - Sign personal messages 6. Remove one QR account 7. Forget the QR device ### **Does it work with HW devices already paired?** 1. Checkout to main, and run `yarn && yarn webpack` or `yarn && yarn start` to build the extension. 2. Install the extension in your browser, and pair a QR-based hardware wallet (e.g. a Keystone device, or install AirGap Vault on an iOS/Android device). 3. Add some (3) accounts to the QRKeyring, and ensure that you can see them in the extension. 4. Checkout to this branch, and run `yarn && yarn webpack` or `yarn && yarn start` to build the extension. 5. Go to `chrome://extensions/`, and reload the extension. 6. Login to the extension, and ensure that you can see the same accounts as before. ## **Screenshots/Recordings** <!-- If applicable, add screenshots and/or recordings to visualize the before and after of your change. --> ### **Before** <!-- [screenshots/recordings] --> ### **After** <!-- [screenshots/recordings] --> ## **Pre-merge author checklist** - [ ] I've followed [MetaMask Contributor Docs](https://github.com/MetaMask/contributor-docs) and [MetaMask Extension Coding Standards](https://github.com/MetaMask/metamask-extension/blob/main/.github/guidelines/CODING_GUIDELINES.md). - [ ] I've completed the PR template to the best of my ability - [ ] I’ve included tests if applicable - [ ] I’ve documented my code using [JSDoc](https://jsdoc.app/) format if applicable - [ ] I’ve applied the right labels on the PR (see [labeling guidelines](https://github.com/MetaMask/metamask-extension/blob/main/.github/guidelines/LABELING_GUIDELINES.md)). Not required for external contributors. ## **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. --------- Co-authored-by: MetaMask Bot <metamaskbot@users.noreply.github.com>
1 parent 0f8ce54 commit e085b42

33 files changed

Lines changed: 674 additions & 683 deletions

.storybook/main.js

Lines changed: 0 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -49,10 +49,6 @@ module.exports = {
4949
config.resolve.alias['../../../store/actions'] = require.resolve(
5050
'../ui/__mocks__/actions.js',
5151
);
52-
// Import within controller-utils crashes storybook.
53-
config.resolve.alias['@ethereumjs/util'] = require.resolve(
54-
'../ui/__mocks__/ethereumjs-util.js',
55-
);
5652
config.resolve.alias['./useNftCollectionsMetadata'] = require.resolve(
5753
'../ui/__mocks__/useNftCollectionsMetadata.js',
5854
);

.yarn/patches/@keystonehq-metamask-airgapped-keyring-npm-0.15.2-94dd4b40d7.patch

Lines changed: 0 additions & 60 deletions
This file was deleted.

app/scripts/background.js

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -634,6 +634,9 @@ async function initialize(backup) {
634634
// eslint-disable-next-line @typescript-eslint/no-require-imports, @typescript-eslint/no-var-requires, node/global-require
635635
ledgerBridge: require('../../test/stub/keyring-bridge')
636636
.FakeLedgerBridge,
637+
// Use `require` to make it easier to exclude this test code from the Browserify build.
638+
// eslint-disable-next-line @typescript-eslint/no-require-imports, @typescript-eslint/no-var-requires, node/global-require
639+
qrBridge: require('../../test/stub/keyring-bridge').FakeQrBridge,
637640
},
638641
}
639642
: {};

app/scripts/constants/sentry-state.ts

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -72,7 +72,7 @@ export const SENTRY_BACKGROUND_STATE = {
7272
notificationGasPollTokens: true,
7373
outdatedBrowserWarningLastShown: true,
7474
popupGasPollTokens: true,
75-
qrHardware: true,
75+
activeQrCodeScanRequest: true,
7676
recoveryPhraseReminderHasBeenShown: true,
7777
recoveryPhraseReminderLastShown: true,
7878
showBetaHeader: true,

app/scripts/controllers/app-state-controller.test.ts

Lines changed: 3 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -3,7 +3,6 @@ import type {
33
AcceptRequest,
44
AddApprovalRequest,
55
} from '@metamask/approval-controller';
6-
import { KeyringControllerQRKeyringStateChangeEvent } from '@metamask/keyring-controller';
76
import { Browser } from 'webextension-polyfill';
87
import {
98
ENVIRONMENT_TYPE_POPUP,
@@ -696,9 +695,7 @@ type WithControllerCallback<ReturnValue> = ({
696695
| AddApprovalRequest
697696
| AcceptRequest
698697
| PreferencesControllerGetStateAction,
699-
| AppStateControllerEvents
700-
| PreferencesControllerStateChangeEvent
701-
| KeyringControllerQRKeyringStateChangeEvent
698+
AppStateControllerEvents | PreferencesControllerStateChangeEvent
702699
>;
703700
}) => ReturnValue;
704701

@@ -717,9 +714,7 @@ async function withController<ReturnValue>(
717714
| AddApprovalRequest
718715
| AcceptRequest
719716
| PreferencesControllerGetStateAction,
720-
| AppStateControllerEvents
721-
| PreferencesControllerStateChangeEvent
722-
| KeyringControllerQRKeyringStateChangeEvent
717+
AppStateControllerEvents | PreferencesControllerStateChangeEvent
723718
>();
724719
const appStateMessenger = controllerMessenger.getRestricted({
725720
name: 'AppStateController',
@@ -728,10 +723,7 @@ async function withController<ReturnValue>(
728723
`ApprovalController:acceptRequest`,
729724
`PreferencesController:getState`,
730725
],
731-
allowedEvents: [
732-
`PreferencesController:stateChange`,
733-
`KeyringController:qrKeyringStateChange`,
734-
],
726+
allowedEvents: [`PreferencesController:stateChange`],
735727
});
736728
controllerMessenger.registerActionHandler(
737729
'PreferencesController:getState',

0 commit comments

Comments
 (0)