[Bug]: [MV3] Hardware Wallet Support

[andrewpeters9]

Describe the bug

Current interactions with hardware wallets, namely Trezor and Ledger wallets, are not supported by Chrome MV3 changes.

Steps to reproduce

Some of the ways the issue can be reproduced:

• Initial instantiation/unlock of the KeyringController if a Trezor or Ledger hardware wallet is stored in the vault
• Various background requests that lead to interactions with hardware wallets, such as connectHardware and transaction-related requests

Error messages or log output

No response

Version

develop-oct16

Build type

No response

Browser

Chrome

Operating system

MacOS

Hardware wallet

No response

Additional context

General approaches on how to solve the issue:

1. Address hardware-wallet connection issues within the background scripts by removing incompatibilities (briefly: DOM references (mainly iframe creations), and the loading of dynamic content (i.e. the Trezor connect popup). This would be done through the elaboration of the Keyrings, and use of APIs such as WebHID and WebUSB
2. or, allow for hardware-wallet connections to be conducted within content scripts.

Why I believe # 2 is the better choice:

• Less exposure to cross-browser implementations/supports for the APIs required for # 1
  • WebUSB (in-dev in Chromium, but potential lack of consensus cross-browser?): https://chromestatus.com/feature/5200265459269632
  • WebHID (developed (behind flag), but no cross-browser signal yet): https://chromestatus.com/feature/6305751386554368
• Less exposure to cross-browser implementation of MV3 as a whole:
  • It appears that there isn't complete consensus between how FF wants to implement MV3 relative to Chrome's implementation. See the end of this post: https://blog.mozilla.org/addons/2022/05/18/manifest-v3-in-firefox-recap-next-steps/
• Solves all hardware issues in one fell-swoop
• Would likely simplify testing
• Provides more flexibility if non-Metamask maintained connection dependencies require updates (potentially due to new Hardware etc.)
• Depending on the specifically implemented approach:
  • Can simplify eth-keyring-controller code in the long-term
  • Will simplify a reversal back to ServiceWorker-contained hardware connections once MV3 standards are matured and non-MM maintained connection libraries provide in-built support for MV3 execution

Suggested approach can be summarised into one verbose sentence:

Try to execute methods within hardware keyrings on the background-side first via a wrapper around the hardware keyring controller, upon (potential) failure defer to a promisified frontend execution of said method.

Why not run all Hardware Keyrings solely in the frontend?

• Some - most - Hardware Keyring methods don't error in ServiceWorker's
• Trying ServiceWorker execution first will speed up overall execution
• Allows for a smoother transition to ServiceWorker execution only if that's decided upon in the future
• Removes the need for modifications to eth-keyring-controller
• ... I have some more trivial points that are harder to articulate, so if you'd like further elaboration on these thoughts then feel free to ask and I'll try my best

Brief Overview of what the approach would entail:

Non- metamask-extension changes:

• Move non-MV3 compatible constructor code into a new Keyring method, potentially init() or build()

  • Reason being is that constructor ()s can't be turned into a promise
    • And because the solution would involve keeping the promise open upon hardware connection failure, we would need to ensure that these constructors can be executed ServiceWorker-side without issue
  • Examples include: https://github.com/MetaMask/eth-trezor-keyring/blob/main/index.js#L63-L68
  • and https://github.com/MetaMask/eth-ledger-bridge-keyring/blob/main/index.js#L39-L44

  • to improve compatibility, these lines of code would still be executed during instantiation by default, just not when a feature flag is used (our MV3 flag). e.g.:

    class TrezorKeyring extends EventEmitter {
    constructor(opts = {}) {
     ...
    
     if(!opts.isManifestV3) {
        this.init()
     }
    }
    
    init (){
     TrezorConnect.on('DEVICE_EVENT', (event) => {
        if (event && event.payload && event.payload.features) {
          this.model = event.payload.features.model;
        }
     });
     TrezorConnect.init({ manifest: TREZOR_CONNECT_MANIFEST });
    }
    }

    Background Changes:

• Wrap all Hardware keyrings with a proxy, e.g. HardwareKeyring or HardwareKeyringWrapper that implements all keyring methods (either programmatically or explicitly). An example of this wrapper would look something like this:

  class HardwareKeyringWrapper {
  constructor(keyring) {
      this.keyring = keyring;
  }
  
  // ...
  
  unlock(...args) {
      try {
          return this.keyring.unlock(...args);
      } catch (e) {
          // determine if the error is actually due to MV3 changes or not
          if (isManifestV3Error(e)) {
              return new Promise((resolve, reject) => {
                  // if error is due to MV3 then add the Keyring's method execution to the 
                  // KeyringEventBatcher's mempool in order to be sent to client-side
                  addToEventBatcher(this.keyring.type, 'unlock', serialize(args), resolve, reject);
              })
          } else {
              return Promise.reject(e);
          }
      }
  }
  
  // ...
  }

• Provide the wrapped Hardware keyrings with a controller, e.g. KeyringEventBatcher
  • This basically provides the keyring wrappers with a mempool / event queue (and potentially other shared methods later on) for them to add their failed Keyring method calls to. (I'll provide a snipper for this shortly)
• Pass the hardware keyrings to the KeyringController in a new way like so:

  this.keyringController = new KeyringController({
    // keyringTypes: additionalKeyrings
    keyringTypes: this.keyringEventBatcher.keyrings,
    initState: initState.KeyringController,
    encryptor: opts.encryptor || undefined,
  });

• The KeyringEventBatcher mempool is cleared periodically (perhaps every 500ms, as I know there are some limitations to how frequently ServiceWorker's can send messages to the client
• callBackgroundMethod then receives these execution requests from the KeyringEventBatcher
  • An additional proxy could be layered into callBackgroundMethod to pick out only Promise responses from the KeyringEventBatcher (yet to be investigated how to best implement)
• Requests from KeyringEventBatcher to callBackgroundMethod can then be controlled via something along the lines of ClientKeyringController or something along those lines

Client-Background Comms Changes:

Note: there are likely quite a few ways to approach the client-background comms implementation of this solution. Feel free to suggest changes/alternatives Given that there are quite a few places where the clientside is calling background methods that potentially need an additional callback, addressing the issue where each callBackgroundMethod is executed appears too messy.

Instead, a cleaner approach appears to be to persist the initial promise (the callBackgroundMethod promise), this can be done by:

1. Attaching a clientside proxy to callBackgroundMethod's, likely around https://github.com/MetaMask/metamask-extension/blob/develop/ui/store/action-queue/index.js#L133 or somewhere else in the action-queue/index.js file. The proxy can intercept promise resolves/reject, and communicate with a ClientKeyringController (if a certain method is passed along) before resolving the promise
2. or, elaborating the RPC comms to allow for a promisifiable message initiated from the background. This would involve elaborating the methods that are handled here https://github.com/MetaMask/metamask-extension/blob/develop/ui/index.js#L39-L52

If there's no preference, then I believe # 2 would be a better choice as it affords the opportunity for background-initiated promises that aren't related to call stacks which have callBackgroundMethod in them - it also appears less intrusive.

Diagram of Proposed Solution:

[danjm]

@andrewpeters9 I want to make sure I understand:

Is this proposing that the webhid and webusb connections would be done from inside a service worker that is created in the contentscript?

[andrewpeters9]

@danjm It's proposing that the connections (WebHid + WebUSB) would be done client-side, but their Keyrings would be called by the service worker. I'll DM you regarding a quick chat.

[github-actions[bot]]

This issue has been automatically marked as stale because it has not had recent activity in the last 90 days. It will be closed in 45 days. Thank you for your contributions.

[github-actions[bot]]

This issue was closed because there has been no follow up activity in the last 45 days. If you feel this was closed in error, please reopen and provide evidence on the latest release of the extension. Thank you for your contributions.