MetaMask / metamask-snaps-beta

Fork of MetaMask that supports plugins! Read the Wiki!
https://github.com/MetaMask/metamask-snaps-beta/wiki
MIT License
143 stars 57 forks source link

Add method for getting permitted plugins #68

Closed rekmarks closed 4 years ago

rekmarks commented 5 years ago

There's no dedicated way for domains to check if a plugin is installed and permitted. We should add an external wallet_ RPC method to check whether a plugin is installed, and potentially get metadata about e.g. the version and so on.

This suggestion did not originally contain the "permitted" condition. We should not let external domains poll for plugins without having permissions to them.

This method will either be an entirely new wallet_getPlugins RPC method or a modification of the soon-to-be-implemented wallet_installPlugins method.

danfinlay commented 5 years ago

This could be a privacy leak. When is it valuable to detect a plugin without just asking permission to interact with it?

rekmarks commented 5 years ago

This suggestion originally came from @oed. The goal is for consumers of our API to have a means of checking whether a plugin is installed without initiating the installation or permission request flow. This would enable better UX, e.g. by not displaying the Connect button if the plugin is already installed.

Always reinstalling the plugin when the permission for it is requested is (as discussed internally) a stopgap measure. However, using wallet_requestPermissions to check if a plugin is installed would still re-request the permission for the plugin. Whether we'll change that behavior in the necessary timeframe or at all is an open question.

Furthermore, in the future we'll want to let API consumers know what version of a plugin is installed, and we'll need something like wallet_getPluginManifest to accomplish that. Alternatively, we could add that information to the plugin permission itself, but that seems like poor separation of concerns.

Finally, this would not cause any privacy leaks because we already allow API consumers to call wallet_getPermissions.

oed commented 5 years ago

@danfinlay Right now when you ask for permission the install plugin is always displayed. If in the future that is only displayed if you have not yet been granted permission I think that would be fine as well. At least for my use case.

danfinlay commented 4 years ago

Right now when you ask for permission the install plugin is always displayed. If in the future that is only displayed if you have not yet been granted permission I think that would be fine as well. At least for my use case.

Yes I agree this is fine. We could already easily do a check: If the plugin is installed, only ask the user to connect to it. Right now we always re-install really just as a developer convenience feature, but we should probably replace it as soon as we have a reasonable plugin-update mechanism (maybe the mm-plugin serve API could host a websocket that notifies it to reload for now).

Furthermore, in the future we'll want to let API consumers know what version of a plugin is installed

I'm not certain how we'll address this, but it may be more like a dependency graph: Let dapps/plugins specify the versions they want. Maybe updates are always hosted at new addresses, or maybe we define a special wallet-plugin ENS content type that includes a map of versions, so that a plugin origin can specify a variety of versions that can be referenced.

oed commented 4 years ago

Yes I agree this is fine. We could already easily do a check: If the plugin is installed, only ask the user to connect to it.

This is fine if: The user is only asked the first time they use app A that uses plugin B. The second time the user comes to app A they should not have to reaffirm that app A can use plugin B (it's fine if there is some timeout here).

awalias commented 4 years ago

what is currently the best way to check whether a user has previously granted a given permission for the current domain?

rekmarks commented 4 years ago

@awalias I'll make sure this is added to the Wiki where appropriate but for now:

Does this answer your question?

Edit: updated wiki: https://github.com/MetaMask/metamask-snaps-beta/wiki/Plugin-and-Dapp-Permissions

awalias commented 4 years ago

thanks @rekmarks , I eventually got wallet_getPermissions to work - my problem was that simply having import Web3 from 'web3'; (web3@1.2.4) in react was causing the ethereum.send with wallet_getPermissions to return undefined. I'm assuming it does something to the provider, but definitely not playing nice together atm. I switched to ethers.js for now and seems to be working ok.

rekmarks commented 4 years ago

This issue has been updated to say that plugins must be "installed and permitted" in order to be returned by the yet-to-be-implemented wallet_getPlugins method.

There's also a world where we modify the soon-to-be-implemented wallet_installPlugins to include a force: boolean parameter which will attempt to force plugin reinstallation. If that parameter is false and the plugin is already installed, that can just return data about the plugin. wallet_getPlugins seems nice to have for completeness, though.