chore(keyring-controller): remove QRKeyring-related code #6031
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
@metamaskbot publish-preview |
|
Preview builds have been published. See these instructions for more information about preview builds. Expand for full list of packages and versions. |
mikesposito
commented
Jun 25, 2025
Comment on lines
-1884
to
-1886
| await withController( | ||
| // @ts-expect-error QRKeyring is not yet compatible with Keyring type. | ||
| { keyringBuilders: [keyringBuilderFactory(QRKeyring)] }, |
There was a problem hiding this comment.
Some of these test scenarios were adding QRKeyring to builders, but without doing anything with it. These changes here may be a little confusing because of indentation change, but the only lines removed should be the ones related to keyringBuilders.
This was referenced Jun 25, 2025
github-merge-queue bot
pushed a commit
to MetaMask/accounts
that referenced
this pull request
Aug 12, 2025
<!-- Thanks for your contribution! Take a moment to answer these questions so that reviewers have the information they need to properly understand your changes: * What is the current state of things and why does it need to change? * What is the solution your changes offer and how does it work? Are there any issues or other links reviewers should consult to understand this pull request better? For instance: * Fixes #12345 * See: #67890 --> This is a new Keyring implementation compatible with QR-based wallets, meant to replace `@keystonehq/metamask-airgapped-keyring`. The goals are: - Internalize the package - Simplify internal logic (e.g. no event-based logic) - Remove the need of KeyringController special methods (See methods nuked in MetaMask/core#6031) - Provide a consumer-injectable transport bridge to allow clients to control QR scanning implementation - Improve API ergonomics - Allow clients to easily support multiple QR devices per user This package can be tested on Extension through this PR: MetaMask/metamask-extension#33893 * Fixes #144 ## How does it work? The QrKeyring in this package is split into two main components: - `QrKeyring`: The main keyring class that exposes the Keyring API. Some of its logic is implemented in internal utility classes: - `Device`: A stateful internal wrapper for the keyring's public keys derivation logic and account management. In the code it's implemented by a separate class, but it can be seen as an internal component of the keyring, and It is not exported from the package. - `QrKeyringBridge`: The interface that a transport bridge must implement: it allows the client to control how QR codes are scanned. It's supposed to be implemented by a class that is injected by the client: when using KeyringController, it can be injected through a custom keyring builder (See [this keyring builder](https://github.com/MetaMask/metamask-extension/blob/75576f2d31432563e34fbaa66c7783b77aa5124d/app/scripts/lib/qr-keyring-builder-factory.ts) that will be used by Extension). - `QrKeyringScannerBridge`: A generic bridge that allows the client to pass function hooks at construction time, which will be called when the keyring needs to scan a QR code. ### Device pairing flow Pairing is the process of initializing the keyring with a device, which is done by submitting the device details to the keyring in the form of a serialized UR, which usually comes from the "sync" QR scanned on the device. The client can trigger this process in three ways: - At construction time, by passing a `SerializedUR` object to the `QrKeyring` constructor. - In this case, the scan is done before the keyring is constructed, and thus the keyring will directly receive the QR contents. - By calling the `QrKeyring.pairDevice()` method, which allows the client to pass a `SerializedUR` object at any time. - Same as above, the scan is done outside the keyring, and the keyring will directly receive the QR contents. - By calling the `QrKeyring.getFirstPage()` method, which will trigger a scan request if no device is paired yet. - In this case, the keyring will request a scan through the bridge, and the client will implement the scan logic. #### Extension uses `getFirstPage()` to trigger the pairing flow, which will then request a scan through the bridge Like for other Hardare wallets, the pairing flow is initiated in [`connectHardware()`](https://github.com/MetaMask/metamask-extension/blob/75576f2d31432563e34fbaa66c7783b77aa5124d/app/scripts/metamask-controller.js#L5191) method. See the sequence diagram below for the pairing flow on Extension, and see [the Extension `requestScan` callback](https://github.com/MetaMask/metamask-extension/blob/75576f2d31432563e34fbaa66c7783b77aa5124d/app/scripts/metamask-controller.js#L1112) for the implementation of the scan logic. ```mermaid sequenceDiagram autonumber participant A as MM Extension create participant B as QrKeyring A->>B: getFirstPage() alt if not initialized create participant C as Bridge B->>C: requestScan(PAIRING) C-->>+A: note over A,C: The client controls the bridge and implements the scan logic A-->>-C: destroy C C->>B: return CBOR create participant D as Int. derivation logic B->>D: init(CBOR) end B->>D: accountAtIndex(0..4) destroy D D->>B: return accounts destroy B B->>A: return accounts ``` #### Mobile TBD - Likely using `.pairDevice()` given the current implementation of QR interactions on mobile. ### Signing flow ```mermaid sequenceDiagram autonumber participant A as Client create participant B as QrKeyring A->>B: sign{Transaction, PersonalMessage, TypedData}() alt not initialized B->>A: Fail end create participant C as Bridge B->>C: requestScan(SIGN) C-->>+A: note over A,C: The client controls the bridge and implements the scan logic A-->>-C: destroy C C->>B: return signature CBOR destroy B B->>A: return hex signature ``` ### Account addition flow No scan required after the initial pairing. ```mermaid sequenceDiagram autonumber participant A as Client alt non sequential account addition create participant B as QrKeyring A->>B: setAccountToUnlock( i ) end A->>B: addAccounts( n ) loop n-times create participant D as Int. derivation logic B->>D: accountAtIndex( i ) destroy D D->>B: return account end B->>A: return created accounts ``` ## Examples <!-- Are there any examples of this change being used in another repository? When considering changes to the MetaMask module template, it's strongly preferred that the change be experimented with in another repository first. This gives reviewers a better sense of how the change works, making it less likely the change will need to be reverted or adjusted later. -->
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
github-merge-queue bot
pushed a commit
to MetaMask/metamask-extension
that referenced
this pull request
Sep 1, 2025
<!-- 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 [](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 <[email protected]>
](https://codespaces.new/MetaMask/metamask-extension/pull/33893?quickstart=1){kind=link}
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Explanation
Dependent on:
feat: add qr keyring package accounts#60This PR removes all code related to the QRKeyring from KeystoneHQ, which we intend to deprecate in favor of the new QRKeyring implementation in the MetaMask accounts monorepo, that fully supports our Keyring type.
References
keyring-controllerto remove QR keyring methods #4341Changelog
Checklist