feat: add qr keyring package #60
Conversation
mikesposito
force-pushed
the
add-qr-keyring
branch
from
05effd9 to
ccd708e
Compare
|
Thank you @mikesposito! Maybe we should import this using |
|
@danroc this PR is not moving the package from another source - this is a new implementation, and only shares a similar account derivation strategy from URs with the original keyring package currently used on clients Do you think we should bring in its commit history anyway? Also, the package is in a Keystone monorepo, which would include commits to other packages potentially unrelated |
|
Sorry @mikesposito, I didn't realise it was a new implementation! In this case it makes sense to start fresh, thank you for the clarification. |
|
This PR is marked as stale because it has been open for 60 days with no activity. Please remove the stale label or leave a comment, or it will be closed in 14 days. |
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
|
Caution Review the following alerts detected in dependencies. According to your organization's Security Policy, you must resolve all "Block" alerts before proceeding. Learn more about Socket for GitHub.
|
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
| * @returns An array of IndexedAddress objects, each containing the address and its index | ||
| * @throws Will throw an error if the source is not initialized | ||
| */ | ||
| getAddressesPage(page: number, pageSize = 5): IndexedAddress[] { |
There was a problem hiding this comment.
hmm, I just noticed that the original package uses a different function to build pages when in "account" mode: https://github.com/KeystoneHQ/keystone-sdk-base/blob/22815365ecafac31c5845b4cc9d48c44985b11b8/packages/base-eth-keyring/src/BaseKeyring.ts#L382
I probably need to adapt this code to have the same behavior in "account" mode.
There was a problem hiding this comment.
Though, looking at this closely, it does seem that __getLedgerLivePage doesn't do anything different compared to __getNormalPage, besides skipping index caching. We are probably good to go in terms of how close this code is to the original.
It does seem that in Account mode the keyring should only list and unlock specific indexes passed from the device at pair time, throwing error when calling addressForIndex for missing indexes: this means that in cases where there is an incomplete accounts page in Account mode the user will be unable to fetch that page
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
|
@metamaskbot publish-preview |
|
Preview builds have been published. See these instructions (from the Expand for full list of packages and versions. |
|
@metamaskbot publish-preview |
|
Preview builds have been published. See these instructions (from the Expand for full list of packages and versions. |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
|
@metamaskbot publish-preview |
|
Preview builds have been published. See these instructions (from the Expand for full list of packages and versions. |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
There was a problem hiding this comment.
Bug: Incorrect Transaction Casting Causes Errors
The code incorrectly casts non-legacy transactions to FeeMarketEIP1559Transaction. The @ethereumjs/tx library supports other types, such as AccessListEIP2930Transaction (EIP-2930). Since the getMessageToSign() method is available on all TypedTransaction types, this cast is unnecessary and can lead to runtime errors or incorrect behavior.
packages/keyring-eth-qr/src/device.ts#L339-L348
accounts/packages/keyring-eth-qr/src/device.ts
Lines 339 to 348 in 8e585a0
Was this report helpful? Give feedback by reacting with 👍 or 👎
This is a new Keyring implementation compatible with QR-based wallets, meant to replace
@keystonehq/metamask-airgapped-keyring.The goals are:
This package can be tested on Extension through this PR: MetaMask/metamask-extension#33893
QRKeyringimplementation #144How 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 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:
SerializedURobject to theQrKeyringconstructor.QrKeyring.pairDevice()method, which allows the client to pass aSerializedURobject at any time.QrKeyring.getFirstPage()method, which will trigger a scan request if no device is paired yet.Extension uses
getFirstPage()to trigger the pairing flow, which will then request a scan through the bridgeLike for other Hardare wallets, the pairing flow is initiated in
connectHardware()method. See the sequence diagram below for the pairing flow on Extension, and see the ExtensionrequestScancallback for the implementation of the scan logic.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 accountsMobile
TBD - Likely using
.pairDevice()given the current implementation of QR interactions on mobile.Signing flow
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 signatureAccount addition flow
No scan required after the initial pairing.
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 accountsExamples