core icon indicating copy to clipboard operation
core copied to clipboard

Refactor the `QRKeyring` to use the `Keyring` interface

Open gantunesr opened this issue 2 years ago • 1 comments

Description

The current QRKeyring from @keystonehq/metamask-airgapped-keyring has a custom interface that is not compatible with our current implementation. This work is aimed to align the QRKeyring to use our Keyring interface.

Consider starting a new repository to fully own the QRKeyring package.

References

gantunesr avatar Jan 16 '24 13:01 gantunesr

The approach used in this PR to pair a QRKeyring shows how we could extract the QR scan process from the QRKeyring, and submit the scan result directly.

Currently, the client modals are controlled by an event emitted by the QRKeyring. But since the connection between the devices is stateless, and only uses qr codes, there is no reason why we should do this.

if we could:

  • Move the QRKeyring to an owned repo
  • Implement the Keyring<Json>'s main methods
  • Implement what currently submitCryptoHDKey and submitCryptoAccount do in a initialize method instead, so that it is compatible with our types

Then we would probably be able to have a fully compatible QRKeyring, except for methods related to pagination, e.g.:

  • getPreviousPage
  • getNextPage

Since we also need these in other hardware keyrings, we could take this chance to add those to our Keyring<Json> type, or perhaps to a separate PaginatedKeyring type to prefer composability over inheritance.

The eventual remaining methods would be then accessible through KeyringController.withKeyring

cc @Gudahtt

mikesposito avatar Jun 04 '24 12:06 mikesposito