search

MetaMask / metamask-extension

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

Snap installation sometimes gets auto-rejected #13192

Closed Gudahtt closed 2 years ago

Gudahtt commented 2 years ago

Describe the bug When a site requests permission to use a snap that has not yet been installed, a request is made for the snap to be installed as well. We had intended the snap installation to be a part of the same "Connect" flow used for the site, but the snap installation request timing prevents that from working sometimes.

There are three possible scenarios:

Steps to reproduce (REQUIRED)

  1. Create a Flask build on the snaps branch (i.e. yarn build dev --build-type flask), install the build in a browser, then complete onboarding.
  2. Navigate to https://metamask.github.io/snap-template/
  3. Click "Connect"
  4. One of the three scenarios described above should play out during the connection process. The 2nd scenario is a bug as well, but a less disruptive one. This issue primarily concerns the 3rd scenario, which is a more severe bug.

Expected behavior The snap installation should always happen during the "Connect" flow for the site to first request access to the snap (i.e. as described in scenario 1 above)

Gudahtt commented 2 years ago

This might happen sometimes with dapp-suggested confirmations as well. We auto-reject any pending confirmations when the notification window is closed, but we don't guarantee that the UI knew about all of the confirmations we're rejecting.

Not totally confident what we want to do here :thinking: We could keep track of which confirmations were "seen" by the UI, but they might not have been rendered in time for the user to see them. Even if they were rendered, they might have been rendered just momentarily, too quickly for the user to notice and consider. We have to draw the line somewhere though. For the purpose of this issue, drawing the line at what the UI knows about would suffice, but that might still result in somewhat-confusing dapp interactions.

rekmarks commented 2 years ago

Hey team! Please add your planning poker estimate with ZenHub @Gudahtt @hmalik88 @shanejonas

Gudahtt commented 2 years ago

This was mostly fixed by #13194. Now confirmations should only get rejected when the user explicitly closes the popup window. They are no longer rejected when the window closes automatically as a result of the last confirmation being approved or rejected.

This is still a problem though:

The snap installation request is made late enough that it happens after the "Connect" flow popup closes, so it triggers a new popup.

That will be more challenging to fix, so I will create a new ticket for that problem and close this one.