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

Add Dapp detection #718

Closed
bbondy opened this issue Aug 9, 2018 · 11 comments · Fixed by brave/brave-core#2678
Closed

Add Dapp detection #718

bbondy opened this issue Aug 9, 2018 · 11 comments · Fixed by brave/brave-core#2678

Comments

@bbondy
Copy link
Member

bbondy commented Aug 9, 2018

We should give a notification prompt to ask the user to install MetaMask if they visit a Dapp like cryptokitties.

See:
https://github.com/brave/browser-laptop/blob/master/app/browser/reducers/dappReducer.js

Test plan

  • With a new profile, go to cryptokitties.co and you should see a prompt to install from the website.
  • After it is installed, you should not get this prompt anymore.
  • You should not see this prompt on a new profile on other sites.
@bbondy bbondy added this to the Backlog milestone Aug 9, 2018
@lukemulks
Copy link

Would it be possible for us to preload MetaMask, as we do with b-l?

We provided a security and blockchain advantage in the market when we shipped with MM pre-installed (and inactive).

Recalling when the extension store recently removed the legit MM along with some duplicates - Brave had the advantage and benefit of being able to confirm to our user base (and the public) that our pre-loaded MM extension was authentic and up to date.

Also allows for Brave to state that we ship a web3-ready browser.

There may be some reason I'm unaware of why it's better not to do this, but mentioning as I found myself looking for MM in the dev channel.

@bbondy
Copy link
Member Author

bbondy commented Sep 21, 2018

We never pre-installed metamask, but we just made it really easy to install via the Dapp detection and by having an option directly in settings to download and install it in 1 step.

@bsclifton
Copy link
Member

When looking at this, please see this issue regarding the integration done with the Muon version of Brave:
brave/browser-laptop#15221

@rebron rebron modified the milestone: 1.x Backlog Feb 7, 2019
@bbondy bbondy added feature/web3/wallet Integrating Ethereum+ wallet support and removed feature/cryptocurrency labels Jun 10, 2019
@simonhong simonhong self-assigned this Jun 11, 2019
@simonhong
Copy link
Member

simonhong commented Jun 12, 2019

Sub tasks

  • Add Dapp detection preference in settings (If it is enabled brave extension notifies to browser)
  • Implement extension for detecting whether webpage is Dapp (trying to access window.web3) Reuse brave extension for detection
  • Implement extension API
    • braveShield.dappAvailable for notifying to browser from detecting extension when extension thinks current page is Dapp
    • braveShield.dappDetectionEnabled
  • Implement UI (by using permission request infra) for notifying to user about this page is Dapp and guiding to install our wallet extension(fork of MetaMask).

@tildelowengrimm
Copy link
Contributor

@simonhong we're currently developing a separate cryptocurrency wallet based on MetaMask. The Ðapp-detection should be integrated with that rather than prompting for a separate MetaMask install.

@simonhong
Copy link
Member

@tomlowenthal Yup, modified (I know that but forgot to mention about it) :)

@bsclifton
Copy link
Member

@bbondy could you put together a quick test plan? 😄

@bbondy bbondy modified the milestones: 0.69.x - Beta, 0.70.x - Dev Aug 14, 2019
@bbondy
Copy link
Member Author

bbondy commented Aug 14, 2019

added test plan

@kjozwiak
Copy link
Member

kjozwiak commented Oct 3, 2019

Verification PASSED on macOS 10.14.6 x64 using the following build:

Brave 0.69.132 Chromium: 77.0.3865.90 (Official Build) (64-bit)
Revision 58c425ba843df2918d9d4b409331972646c393dd-refs/branch-heads/3865@{#830}
OS macOS Version 10.14.6 (Build 18G95)

Verification passed on

Brave 0.69.132 Chromium: 77.0.3865.90 (Official Build) (64-bit)
Revision 58c425ba843df2918d9d4b409331972646c393dd-refs/branch-heads/3865@{#830}
OS Ubuntu 18.04 LTS

image

Verification passed on

Brave 0.69.132 Chromium: 77.0.3865.90 (Official Build) (64-bit)
Revision 58c425ba843df2918d9d4b409331972646c393dd-refs/branch-heads/3865@{#830}
OS Windows 10 OS Version 1803 (Build 17134.1006)

image

@tmcw
Copy link

tmcw commented Oct 15, 2019

Because my previous bug got folded into this one, let me summarize:

Detecting 'Installing a Crypto Wallet' by using a getter on window.web3 is inaccurate and misleading to users. For sites like ours, we want to keep track of globals, and something as simple as

Object.getOwnPropertyNames(window).forEach(key => window[key])

Triggers this message, which recurs even if a user clicks 'Deny'. It's a product that has nothing to do with crypto and this message is incorrect at least and a problem for user trust.

You could:

  • Make the property non-enumerable
  • Drop this check

Or something else. For now we'll have to hardcode a workaround into our app to make it avoid web3.

@srirambv
Copy link
Contributor

@tmcw your previous issue was on muon (legacy browser) Would you mind adding a new issue and we can have that triaged.

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