Compat w/ Brave to support multi web3 providers #7728

bbondy · 2019-12-24T21:39:57Z

This implements the spec changes needed for better compatibility with Brave:
brave/brave-browser#7503

It makes it so both extensions can co-exist.

This might not be the cleanest way to accomplish it. Happy to receive review comments and I'll improve it. Thanks!

danfinlay

We try to avoid adding complexity to the metamask-controller, which is ideally not platform-aware. I recommend adding the platform-dependent logic either in background.js or as a function passed in the platform parameter for the MM-controller.

danfinlay · 2019-12-24T22:38:25Z

app/scripts/metamask-controller.js

@@ -74,6 +76,21 @@ module.exports = class MetamaskController extends EventEmitter {
   */
  constructor (opts) {
    super()
+    chrome.runtime.onMessage.addListener((request, sender, sendResponse) => {


We currently try to keep platform-specific API usage outside of the metamask-controller, so that it can be more platform-portable.

The way we currently do this is using the platform object passed into the MetaMask controller's constructor options.

In this case, I think an easier (and more flexible) approach would be putting this logic in background.js, which already has a runtime.onConnectExternal.addListener callback, which I think is a perfect place to add this conditional platform-distinguishing logic:
https://github.com/MetaMask/metamask-extension/blob/develop/app/scripts/background.js#L320

Gudahtt

I have some reservations about the injectScript call being moved into a message handler. This would make it asynchronous, I believe? Not only is the script injection a performance-critical moment (as a delay here means delaying initialization), it also means that we're no longer guaranteeing the globals injected by MetaMask are present before a dapp tries to access them. e.g. it may be possible, if the message response is sufficiently slow, for the dapp to run prior to the script being injected. This could break many dapps.

Could you move this braveWallet API into the contentscript instead? If we could check extension.braveWallet synchronously directly in the contentscript, it'd eliminate both of the above concerns.

This implements the spec changes needed for better compatibility with Brave: brave/brave-browser#7503 It makes it so both extensions can co-exist.

Gudahtt

Thanks, this is fantastic! This addresses my earlier concerns.

Gudahtt · 2020-01-15T21:37:55Z

app/scripts/contentscript.js

@@ -1,3 +1,5 @@
+/*global chrome*/


I was going to suggest that you use extensionizer instead, but that wouldn't work - that doesn't expose the braveWallet API.

Maybe we should update extensionizer to add that API? 🤔 In the meantime, this seems OK

If braveWallet is a similar API to the chrome/extension APIs that are common in browsers, then it would be a good fit. By the name, it sounds more like a provider..

braveWallet is an additional API. It would need to be added to the list of APIs in extensionizer. At the moment, that package only exports the APIs in that list.

It's not a cross-browser API of course, so I wasn't sure whether it was a good fit for that package.

Gudahtt · 2020-01-15T21:39:10Z

app/scripts/contentscript.js

-  start()
+  // If this is Brave, it requires coordination to know if we should be the
+  // web3 provider.
+  if (chrome.braveWallet && chrome.braveWallet.getWeb3Provider) {


Suggested change

if (chrome.braveWallet && chrome.braveWallet.getWeb3Provider) {

if (chrome && chrome.braveWallet && chrome.braveWallet.getWeb3Provider) {

Correct me if I'm wrong, but I suspect that the chrome global wouldn't exist on Firefox? I think we need to check for that here too.

Yeah, since chrome doesn't exist in firefox, this first clause would throw an undefined error. Either needs to be typeof chrome !== 'undefined' or you could import extensionizer and check extension.braveWallet to avoid this problem.

bbondy · 2020-01-15T21:45:41Z

@Gudahtt @danfinlay
Sorry for the delay and thanks for the suggestion.

I updated it so it's all done from the content-script now but it's still async. I'm looking for a way to do it sync still. I'll update again once I figure that out.

danfinlay

Looks like it would crash Firefox in its current form.

danfinlay · 2020-01-16T19:28:36Z

app/scripts/contentscript.js

-  start()
+  // If this is Brave, it requires coordination to know if we should be the
+  // web3 provider.
+  if (chrome.braveWallet && chrome.braveWallet.getWeb3Provider) {


Yeah, since chrome doesn't exist in firefox, this first clause would throw an undefined error. Either needs to be typeof chrome !== 'undefined' or you could import extensionizer and check extension.braveWallet to avoid this problem.

danfinlay · 2020-01-16T19:28:42Z

app/scripts/contentscript.js

@@ -1,3 +1,5 @@
+/*global chrome*/


If braveWallet is a similar API to the chrome/extension APIs that are common in browsers, then it would be a good fit. By the name, it sounds more like a provider..

danfinlay · 2020-01-16T19:29:29Z

app/scripts/contentscript.js

+  if (chrome.braveWallet && chrome.braveWallet.getWeb3Provider) {
+    chrome.braveWallet.getWeb3Provider((provider) => {
+      if (provider === extension.runtime.id) {
+        injectScript(inpageBundle)


I agree with @Gudahtt that injecting async is asking for trouble, so I really recommend addressing that for Brave's sake. Since this doesn't introduce any async operation to our WebExtension-based injection, I'm okay with this.

Yep I'll get it working sync.

I'll add the typeof check on chrome too.

bbondy · 2020-01-17T20:14:27Z

I think I have a way to do this entirely without any changes to the extension itself. It would disable the particular content script if the extension is not the web3 provider. What do you think of that approach? Currently on MetaMask and the Brave fork of MetaMask would be supported so there'd be overlap functionality for things like phishing detection.

A second content script could be addd for outside of web3 functionality later if we wanted which wouldn't be disabled when the installed extension is not the web3 provider.

Gudahtt · 2020-01-17T20:39:09Z

@bbondy That sounds great! That would be a lot simpler.

The chrome.braveWallet.getWeb3Provider API used earlier would still be useful for us to detect this scenario, and inform users what's going on. Otherwise they might try to use MetaMask via the browser action button and not understand why it can't connect. We can write up some copy explaining how to switch between MetaMask and Brave's wallet.

So would it be possible to leave that API accessible from the background? I realize I was the one to asked it to be moved to the contentscript, sorry about that 😅.

bbondy · 2020-01-17T21:52:03Z

@Gudahtt yep that's a great idea so you can still inform the user based on the same condition I did in this PR.
It was a 1 line change to move it to the contentScripts context so no sweat 👍

bbondy · 2020-01-21T21:30:14Z

Confirming the new approach with no extension changes will work. It'll dynamically disable the content script. We might want to do work in the future to break out functionality into 2 different content scripts depending on if it is web3 related or not, but I think that's low priority.

bbondy · 2020-02-03T17:09:07Z

FYI that this landed on Brave Nightly and will be on Dev channel within a few days. Within 6 weeks it'll on Release channel assuming we don't uplift it.

danfinlay requested changes Dec 24, 2019

View reviewed changes

bbondy force-pushed the develop branch 2 times, most recently from f72db47 to 6a12d7e Compare January 3, 2020 16:14

Gudahtt self-requested a review January 6, 2020 16:24

Gudahtt reviewed Jan 7, 2020

View reviewed changes

bbondy closed this Jan 15, 2020

bbondy force-pushed the develop branch from 6a12d7e to be08cfe Compare January 15, 2020 21:26

Compat w/ Brave to support multi web3 providers

cf9d710

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 reopened this Jan 15, 2020

Gudahtt reviewed Jan 15, 2020

View reviewed changes

Gudahtt requested changes Jan 15, 2020

View reviewed changes

danfinlay requested changes Jan 16, 2020

View reviewed changes

bbondy closed this Jan 21, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Compat w/ Brave to support multi web3 providers #7728

Compat w/ Brave to support multi web3 providers #7728

bbondy commented Dec 24, 2019

danfinlay left a comment

danfinlay Dec 24, 2019

Gudahtt left a comment •

edited

Loading

Gudahtt left a comment •

edited

Loading

Gudahtt Jan 15, 2020

danfinlay Jan 16, 2020

Gudahtt Jan 16, 2020

Gudahtt Jan 15, 2020

danfinlay Jan 16, 2020

bbondy commented Jan 15, 2020

danfinlay left a comment

danfinlay Jan 16, 2020

danfinlay Jan 16, 2020

danfinlay Jan 16, 2020

bbondy Jan 17, 2020

bbondy Jan 17, 2020

bbondy commented Jan 17, 2020

Gudahtt commented Jan 17, 2020 •

edited

Loading

bbondy commented Jan 17, 2020

bbondy commented Jan 21, 2020

bbondy commented Feb 3, 2020 •

edited

Loading

	if (chrome.braveWallet && chrome.braveWallet.getWeb3Provider) {
	if (chrome && chrome.braveWallet && chrome.braveWallet.getWeb3Provider) {

Compat w/ Brave to support multi web3 providers #7728

Compat w/ Brave to support multi web3 providers #7728

Conversation

bbondy commented Dec 24, 2019

danfinlay left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Gudahtt left a comment • edited Loading

Choose a reason for hiding this comment

Gudahtt left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bbondy commented Jan 15, 2020

danfinlay left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bbondy commented Jan 17, 2020

Gudahtt commented Jan 17, 2020 • edited Loading

bbondy commented Jan 17, 2020

bbondy commented Jan 21, 2020

bbondy commented Feb 3, 2020 • edited Loading

Gudahtt left a comment •

edited

Loading

Gudahtt left a comment •

edited

Loading

Gudahtt commented Jan 17, 2020 •

edited

Loading

bbondy commented Feb 3, 2020 •

edited

Loading