Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improve compatibility with MetaMask and allow web3 provider selection #7503

Closed
bbondy opened this issue Dec 20, 2019 · 3 comments · Fixed by brave/brave-core#4273
Closed

Comments

@bbondy
Copy link
Member

bbondy commented Dec 20, 2019

Objective

In Brave, if the user has MetaMask installed, we currently show an infobar on a Dapp page that asks the user to disable one of either Crypto Wallets or MetaMask.

Screen Shot 2019-12-20 at 12 08 58 PM

This is over disabling functionality that we don’t need to, when really we should just be asking which web3 provider they want to use for that page. The current implementation also allows for various edge cases where users can be in a stuck broken state when trying to use Dapps.

This is intended to allow Crypto Wallets to remain enabled, and to improve the frustrations that users encounter by having two web3 providers installed. It also enables us to keep other future functionality enabled as we enhance Crypto Wallets for things like exchange integrations.

Future enhancements / iterations

Explicitly not part of this spec:

  • Supporting web3 providers outside of MetaMask and Crypto Wallets.
  • Later work not part of this spec will likely add P3A reporting for which web3 provider is used

Targeted platforms

  • Desktop, mobile doesn’t have Crypto Wallets at all yet.

Description

The work can be cleanly divided into 4 areas:

  1. Settings work: Remove the setting in preferences which disables Crypto Wallets. There will be no need for this setting anymore. Replace this setting with an option to pick your web3 provider.
  2. Migration work: How the existing user choice works into the new functionality.
  3. Infobar work: Repurpose our MetaMask infobar to ask the user which Web3 provider to use.
  4. Adding an API to obtain the web3 provider: An API will be made available to both MetaMask and Crypto Wallets to be able to check which web3 provider should be used.

Settings work

The old settings switch to turn off Crypto Wallets will be removed. See the next section on Migration work to see how the existing user choice will be handled.

Screen Shot 2019-12-20 at 12 09 30 PM

The options for the drop down menu will be: Ask, None, Crypto Wallets, and MetaMask if it is installed. The default will be Ask until an explicit option is set. The new preference will be stored with the pref name of brave.wallet.web3_provider in per-profile prefs which are visible in chrome://prefs-internals/. If the user explicitly uses Brave wallet first and opts in and the setting is set to Brave, then even installing MetaMask later won’t change the setting from Brave.

If Ask is selected, UI will be shown as per the section on Infobar work
If None is selected, the entire extension will always be lazy loaded, even if it is installed.
If Crypto Wallets is selected, only Crypto Wallets will be used as a web3 provider.
If MetaMask is selected, only MetaMask will be used as a web3 provider.

Migration work

Some users in the past had seen the old infobar and selected either to use MetaMask or Crypto Wallets. We have only in the past actively disabled Crypto Wallets, so no one will have MetaMask disabled by us.

A one time migration will be done to determine which value should be used for the new setting brave.wallet.web3_provider.

  • If Crypto Wallets was disabled and MetaMask is installed, it will be set to MetaMask.
  • If Crypto Wallets is diabled, and MetaMask is not installed, None will be set.
  • If Crypto Wallets is enabled, and MetaMask is still installed, it will be set to Brave.
  • If CryptoWallets is enabled and it has been installed (i.e. was lazy loaded before), but MetaMask is not installed, Brave will be used.
  • If CryptoWallets is enabled and it has not been installed yet (due to lazy loading), and MetaMask is not installed, Ask will be set.

An automated test will be added for each of these migration cases to ensure they work correctly.

Migration code will be added in chrome/browser/prefs/browser_prefs.cc via MigrateObsoleteBrowserPrefs.

Infobar work

If MetaMask is not installed, the existing UX will continue to be valid:

Screen Shot 2019-12-20 at 12 10 17 PM

If MetaMask is installed, the existing infobar UX highlighted in the Objective section will be repurposed to the following.

Screen Shot 2019-12-20 at 12 11 17 PM

Closing the infobar will use neither provider, it will keep the setting mentioned in the last section to “Ask”.

If the user’s setting is “None”, no infobar will be shown.

Depending on which option is selected, the setting mentioned above will be set to either “Crypto Wallets” or “MetaMask”. “None” can only be set from the settings page.
Adding an API to obtain the web3 provider
A new API will be exposed to both Crypto Wallets and MetaMask:
chrome.cryptoWallets.getWeb3Provider((extensionId) => {})

The other APIs available within chrome.cryptoWallets will NOT be accessible to MetaMask.

It will return the extensionId for the provider to use or a blank string.
A web3 provider should only inject existing MetaMask code that provides web3 to the page if its extensionId matches the return value from chrome.cryptoWallets.getWeb3Provider.

Possible values for the extensionId in the above callback will be:

  • nkbihfbeogaeaoehlefnkodbefgpgknn (metamask)
  • odbfpeeihdkbihmopkbjmoonfanlbfcl (Brave)
  • A blank string which signals that no explicit web3 provider was set, and Brave will show native UI. Note that a blank string will also be returned if the user explicitly went to settings and selected None.

QA plan

Test plan being added in wiki here
https://github.com/brave/qa-resources/wiki/%5BWIP%5D-Multiple-Web3-provider-support-test

Open Questions

None at this time.

@mikedotexe
Copy link

This is such a well-written issue, holy cow.

bbondy added a commit to bbondy/metamask-extension that referenced this issue Jan 15, 2020
This implements the spec changes needed for better compatibility with Brave:
brave/brave-browser#7503

It makes it so both extensions can co-exist.
@bbondy bbondy added the CI/skip label Jan 31, 2020
@bbondy bbondy added this to the 1.5.x - Dev milestone Feb 4, 2020
@GeetaSarvadnya
Copy link

GeetaSarvadnya commented Mar 11, 2020

Verification passed on

Brave 1.5.109 Chromium: 80.0.3987.132 (Official Build) beta (64-bit)
Revision fcea73228632975e052eb90fcf6cd1752d3b42b4-refs/branch-heads/3987@{#974}
OS Windows 10 OS Version 1803 (Build 17134.1006)
  • Verified the test plan mentioned in the description

Clean profile:

  • Verified that the drop-down menu shows Ask, None and Crypto Wallets options when MM isn't installed. By default, the Ask option is selected
    image

  • Verified that the drop-down menu shows Ask, None, Crypto Wallets and Metamask options when MM is installed. By default, the Ask option is selected
    image

  • Verified that the infobar is shown on a Dapp page to use Web3 providers when MM is installed in brave and the default option is Ask
    image

  • Verified that the infobar is not removed until one of the Web3 provider is selected on Dapp page

  • Verified that default settings changed from Ask to Crypto Wallet when MM is installed and Use Crypto Wallet option is selected from the infobar on Dapp page
    image

  • Verified that the Metamask option is not shown in the dropdown when MM is removed from brave

  • Verified that the infobar is not shown on Dapp page even though MM is installed when None is selected in settings
    image

  • Verified that default settings changed from Ask to MetaMask when MM is installed and Use MetaMask option is selected from the infobar on Dapp page
    image

  • Install MM on a clean profile and select Crypto Wallet from web3 provider dropdown, verified that the infobar is not shown on the Dapp page and only Crypto Wallets will be used as a web3 provider.

  • Install MM on a clean profile and select MetaMask from web3 provider dropdown, verified that the infobar is not shown on the Dapp page and only MetaMask will be used as a web3 provider.

  • Verified that the existing UI is shown to setup Crypto Wallet on Dapp page when MM is not installed. Also verified that click on Setup navigates to brave://wallets which allows creating CW and option Crypto Wallet will be selected in web3 provider dropdown
    image

  • Create a CW on a clean profile and install MM and open a Dapp page, verified that the infobar is shown with Use Crypto Wallet and Use MetaMask options in Dapp page and default option Ask in settings remained unchanged

  • Install MM on a clean profile and open a Dapp page, verified that the infobar is shown with Use Crypto Wallet and Use MetaMask options in Dapp page and default option Ask in settings remained unchanged

  • In a clean profile open a Dapp page and verified that existing UI is shown with infobar Setup CW to interact with this app and others like it and default option Ask in settings remained unchanged

  • Clean profile 1.5.x, create CW ( download component + create wallet) via the hamburger menu, verified that the Web3 provider is showing Ask option in the dropdown as expected.

  • Clean profile 1.5.x, create CW ( download component + create wallet) via the brave://settings Crypto wallet header menu, verified that the Web3 provider is showing Ask option in the dropdown as expected.

  • Clean profile 1.5.x, open Dapp page cryptokitties.com and click on Setup and create a crypto wallet (download component + create wallet), verified that the Web3 provider is showing Crypto Wallet option in the dropdown as expected.

Upgrade Profile

  • Install MM in 1.4.x and then upgrade to 1.5.x, after upgrade verified that the option Crypto Wallet is selected in the web3 provider dropdown instead of Ask - Logged Incorrect Web3 provider is selected in settings #8656
  • Verified that if Crypto Wallets is disabled in 1.4.x and MetaMask is installed, after upgrade to 1.5.x, the default option is set to MetaMask in the web3 provider dropdown.
  • Verified that if Crypto Wallets is disabled in 1.4.x and MetaMask is not installed, after upgrade to 1.5.x, the default option is set to None in the web3 provider dropdown in settings.
  • Verified that if CryptoWallets is enabled and wallet created in 1.4.x and MetaMask is not installed, after upgrade t 1.5.x, the default option Crypto Wallet is selected in the web3 provider dropdown
  • Verified that if CryptoWallets is enabled (just component download don't create wallet) and MetaMask is not installed in 1.4.x, after upgrade to 1.5.x, the option Crypto Wallet is selected in the web3 provider dropdown
  • Install both CW and MM ( just download the components don't create wallet) in 1.4.x and then upgrade to 1.5.x. after upgrade the web3 provider is showing Crypto Wallet selected in the dropdown instead of Ask - added scenario in Incorrect Web3 provider is selected in settings #8656
  • Create both CW and MM wallets in 1.4.x and then upgrade to 1.5.x, after upgrade the web3 provider is showing Crypto Wallet selected in the dropdown instead of Ask - added scenario in Incorrect Web3 provider is selected in settings #8656
  • Enable CW and install MM and open cryptokitties.com Dapp page and select the Use MetaMask option from infobar and then upgrade to 1.5.x. after upgrade verified that MetaMask is selected in the web3 provider dropdown.
  • Install MM and open cryptokitties.com Dapp page and select the Use CryptoWallet option from infobar in 1.4.x and then upgrade to 1.5.x. after upgrade verified that Crypto wallet is selected in the web3 provider dropdown.
  • Open cryptokitties.com Dapp page in clean profile in 1.4.x and click on Setup in the infobar which enables Cryptowallet (just component download) and then upgrade to 1.5.x, after upgrade verified that Crypto wallet is selected in the web3 provider dropdown.

Verification passed on

Brave 1.5.111 Chromium: 80.0.3987.132 (Official Build) (64-bit)
Revision fcea73228632975e052eb90fcf6cd1752d3b42b4-refs/branch-heads/3987@{#974}
OS Linux
  • Verified the test plan mentioned in the description

Clean profile:

  • Verified that the drop-down menu shows Ask, None and Crypto Wallets options when MM isn't installed. By default, the Ask option is selected
    image

  • Verified that the drop-down menu shows Ask, None, Crypto Wallets and Metamask options when MM is installed. By default, the Ask option is selected
    image

  • Verified that the infobar is shown on a Dapp page to use Web3 providers when MM is installed in brave and the default option is Ask
    image

  • Verified that the infobar is not removed until one of the Web3 provider is selected on Dapp page

  • Verified that default settings changed from Ask to Crypto Wallet when MM is installed and Use Crypto Wallet option is selected from the infobar on Dapp page

  • Verified that the Metamask option is not shown in the dropdown when MM is removed from brave

  • Verified that the infobar is not shown on Dapp page even though MM is installed when None is selected in settings

  • Verified that default settings changed from Ask to MetaMask when MM is installed and Use MetaMask option is selected from the infobar on Dapp page

  • Install MM on a clean profile and select Crypto Wallet from web3 provider dropdown, verified that the infobar is not shown on the Dapp page and only Crypto Wallets will be used as a web3 provider.

  • Install MM on a clean profile and select MetaMask from web3 provider dropdown, verified that the infobar is not shown on the Dapp page and only MetaMask will be used as a web3 provider.

  • Verified that the existing UI is shown to setup Crypto Wallet on Dapp page when MM is not installed. Also verified that click on Setup navigates to brave://wallets which allows creating CW and option Crypto Wallet will be selected in web3 provider dropdown

  • Create a CW on a clean profile and install MM and open a Dapp page, verified that the infobar is shown with Use Crypto Wallet and Use MetaMask options in Dapp page and default option Ask in settings remained unchanged

  • Install MM on a clean profile and open a Dapp page, verified that the infobar is shown with Use Crypto Wallet and Use MetaMask options in Dapp page and default option Ask in settings remained unchanged

  • In a clean profile open a Dapp page and verified that existing UI is shown with infobar Setup CW to interact with this app and others like it and default option Ask in settings remained unchanged

  • Clean profile 1.5.x, create CW ( download component + create wallet) via the hamburger menu, verified that the Web3 provider is showing Ask option in the dropdown as expected.

  • Clean profile 1.5.x, create CW ( download component + create wallet) via the brave://settings Crypto wallet header menu, verified that the Web3 provider is showing Ask option in the dropdown as expected.

  • Clean profile 1.5.x, open Dapp page cryptokitties.com and click on Setup and create a crypto wallet (download component + create wallet), verified that the Web3 provider is showing Crypto Wallet option in the dropdown as expected.

Upgrade Profile

  • Install MM in 1.4.x and then upgrade to 1.5.x, after upgrade verified that the option Crypto Wallet is selected in the web3 provider dropdown instead of Ask - Encountered Incorrect Web3 provider is selected in settings #8656
  • Verified that if Crypto Wallets is disabled in 1.4.x and MetaMask is installed, after upgrade to 1.5.x, the default option is set to MetaMask in the web3 provider dropdown.
  • Verified that if Crypto Wallets is disabled in 1.4.x and MetaMask is not installed, after upgrade to 1.5.x, the default option is set to None in the web3 provider dropdown in settings.
  • Verified that if CryptoWallets is enabled and wallet created in 1.4.x and MetaMask is not installed, after upgrade t 1.5.x, the default option Crypto Wallet is selected in the web3 provider dropdown
  • Verified that if CryptoWallets is enabled (just component download don't create wallet) and MetaMask is not installed in 1.4.x, after upgrade to 1.5.x, the option Crypto Wallet is selected in the web3 provider dropdown
  • Install both CW and MM ( just download the components don't create wallet) in 1.4.x and then upgrade to 1.5.x. after upgrade the web3 provider is showing Crypto Wallet selected in the dropdown instead of Ask - Encountered Incorrect Web3 provider is selected in settings #8656
  • Create both CW and MM wallets in 1.4.x and then upgrade to 1.5.x, after upgrade the web3 provider is showing Crypto Wallet selected in the dropdown instead of Ask - Encountered Incorrect Web3 provider is selected in settings #8656
  • Enable CW and install MM and open cryptokitties.com Dapp page and select the Use MetaMask option from infobar and then upgrade to 1.5.x. after upgrade verified that MetaMask is selected in the web3 provider dropdown.
  • Install MM and open cryptokitties.com Dapp page and select the Use CryptoWallet option from infobar in 1.4.x and then upgrade to 1.5.x. after upgrade verified that Crypto wallet is selected in the web3 provider dropdown.
  • Open cryptokitties.com Dapp page in clean profile in 1.4.x and click on Setup in the infobar which enables Cryptowallet (just component download) and then upgrade to 1.5.x, after upgrade verified that Crypto wallet is selected in the web3 provider dropdown.

Verification PASSED on macOS 10.15.3 x64 Catalina using the following build:

Brave | 1.5.112 Chromium: 80.0.3987.132 (Official Build) (64-bit)
-- | --
Revision | fcea73228632975e052eb90fcf6cd1752d3b42b4-refs/branch-heads/3987@{#974}
OS | macOS Version 10.15.3 (Build 19D76)

Basically went through all the cases that @GeetaSarvadnya outlined above and @bbondy outlined via #7503 (comment). I had more detailed notes but lost them when attempting to update this post and hit a merge conflict. After I refreshed, everything was gone. Notes were basically similar to @GeetaSarvadnya.

@GeetaSarvadnya
Copy link

GeetaSarvadnya commented Mar 12, 2020

@bbondy: I don't see new pref name brave.wallet.web3_provider in chrome://prefs-internals/ - I have verified in both clean and upgraded profiles. let me know if I am missing anything.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants