MetaMask / metamask-extension

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

Let dapps request a specific version when a snap is installed #13499

Closed rekmarks closed 2 years ago

rekmarks commented 2 years ago

Dapps should be able to request to install a specific version or version range of a snap.

rekmarks commented 2 years ago

@holantonela I marked this as in need of designs, but before doing anything, we should review whether the version of the installed snap is adequately represented during installation. We probably want to display it on the dapp's permission request to install the snap, and I don't believe that we are at the moment.

rekmarks commented 2 years ago

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

Gudahtt commented 2 years ago

It's not entirely clear to me what work should be included here. The shunkworks changes needed will be handled in https://github.com/MetaMask/snaps-skunkworks/issues/222 presumably.

That leaves the RPC method changes (i.e. adding version as an optional property, presumably), and associated documentation. Presumably this also requires UI changes to show this requested version during the install flow, and showing that in the list of site permissions on the snap settings page.

What I'm less sure of is how to handle errors. If we can't find a version in the requested range, should we throw it back to the dapp without telling the user? Or leaving the display to them, rather. If we tell the user, where should that error be shown?

The same error handling question applies for the case where there is an existing install with a different version (irrespective of whether we can find a matching version on npm).

rekmarks commented 2 years ago

@Gudahtt the error work certainly falls under this issue. I'm also uncertain of what do. I suppose for MVP purposes, we can just throw it back to the dapp without telling the user, as you suggested. Otherwise, we would have to display the error in the confirmation popup UI.

Presumably this also requires UI changes to show this requested version during the install flow, and showing that in the list of site permissions on the snap settings page.

This seems to imply that the requested version should be a caveat. While the permission request for the snap will include the parameter specifying the snap's version, I'm not sure whether the version should be a caveat or not. I was thinking it could simply be a parameter during the request to use a snap ~that's thrown away afterwards~.

Edit: Ah, we will of course want to display the requested version somewhere on the settings page, even though it doesn't have to be a caveat.

Gudahtt commented 2 years ago

I agree that we should store it somewhere but not as a caveat

Gudahtt commented 2 years ago

Throwing the error back to the dapp makes sense as an MVP! I can't imagine us not doing that to be honest, so the real question is whether to return the error to the dapp or to both return it and show the user. We can add a user-facing error later if we feel that it's appropriate.

holantonela commented 2 years ago

First WIP to review 📌

If the dapp requires a newer snap https://www.figma.com/file/mKIkB5V0k2iYUpz7w7syGO/Untitled?node-id=1%3A1171

If the publisher has a new version https://www.figma.com/file/mKIkB5V0k2iYUpz7w7syGO/Untitled?node-id=1%3A1388

For both scenarios:

FrederikBolding commented 2 years ago

Doing a PR to add the version to the Snap install UI as discussed in the design sync. @rekmarks @holantonela

Not sure if we should split this issue into more issues to make it clearer which parts of the UI is missing.

FrederikBolding commented 2 years ago

PR up here: https://github.com/MetaMask/metamask-extension/pull/13931

Let me know if we should link it to this issue or create multiple issues.