MetaMask / metamask-extension

:globe_with_meridians: :electric_plug: The MetaMask browser extension enables browsing Ethereum blockchain enabled websites
https://metamask.io
Other
11.87k stars 4.85k forks source link

[Bug]: login fails when moving from mv2 to mv3 #16674

Closed jpuri closed 1 year ago

jpuri commented 1 year ago

Describe the bug

The changes to keep user loggedin in MV3 are merged now PR.

An issue now noticed is that if user is moving from mv2 to mv3 extension they are not able to login into extension but need to use seed phrase to recover their account once. Login gives following error:

Error Image

More details here: https://github.com/MetaMask/metamask-extension/pull/15558#issuecomment-1324500449

This issue is blocker for MV3 Beta release.

darkwing commented 1 year ago

Adding some important details here.

The problematic code comes form eth-ledger-bridge-keyring: https://github.com/MetaMask/eth-ledger-bridge-keyring/blob/main/index.js#L40

MV2 allowed us to use a persistent background with the DOM API so that we could inject an iframe to talk to a Ledger via webhid. Since MV3 service workers don't get DOM access, we're seeing this error.

While another team is working on the hardware wallet changes for MV3, I can think of a few solutions:

darkwing commented 1 year ago

The second solution proposed above (Longish term) is probably as simple as this:

https://gist.github.com/darkwing/1181c19c4d1ed1f296a71c3acc4bed1c

At present, when calling getKeyringForDevice, we create the keyring if not present. The usage inside attemptLedgerTransportCreation doesn't make sense if there is no Ledger keyring.

My gist requires two types of testing:

  1. Just test that it fixes the issue on an existing MV2 and/or MV3 MetaMask install (i.e. the original supported issue)
  2. Test that adding a Ledger account creates the Ledger Keyring properly and you can connect to webhid immediately.
darkwing commented 1 year ago

This could be even simpler, but skips some stuff in getKeyringForDevice that could be necessary(?):

https://gist.github.com/darkwing/d2b1fb2b5ed7c2803ccf0b955fd44c17

darkwing commented 1 year ago

The two gists above are for a long term fix; in the short term, we'll likely need to do the try/catch for MV3.

jpuri commented 1 year ago

I created these 2 PRs to fix the issue:

https://github.com/MetaMask/metamask-extension/pull/16684

https://github.com/MetaMask/KeyringController/pull/169

@darkwing : can you plz share ur feedback.