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

Revise the behavior of "Extensions (Brave Wallet fallback)" setting #29853

Closed
jamesmudgett opened this issue Apr 19, 2023 · 8 comments · Fixed by brave/brave-core#18172
Closed
Assignees
Labels
feature/web3/wallet/core feature/web3/wallet Integrating Ethereum+ wallet support OS/Desktop priority/P3 The next thing for us to work on. It'll ride the trains. QA Pass-Win64 QA/Yes release-notes/include

Comments

@jamesmudgett
Copy link

jamesmudgett commented Apr 19, 2023

There are reports that Brave Wallet fallback setting "Extensions" is interfering with MM. The goal here is to detect the MM extension and avoid inserting BW as a fallback handler when the setting "Extensions" (soon to be renamed "Extensions (Brave Wallet fallback)") is enabled

For setting Extensions(BW fallback), we will only provide fallback when MM is not installed and wallet is not created.

@jamesmudgett jamesmudgett added priority/P3 The next thing for us to work on. It'll ride the trains. feature/web3/wallet Integrating Ethereum+ wallet support feature/web3/wallet/core labels Apr 19, 2023
@darkdh
Copy link
Member

darkdh commented Apr 20, 2023

Extensions (BW fallback) will still be default value with the following exceptions:

  1. If MetaMask or Phantom is installed, there will be no fallback.
  2. If BW is not created, there will be no fallback.

https://bravesoftware.slack.com/archives/C023VS4HJ6Q/p1682009313520859?thread_ts=1681911651.196279&cid=C023VS4HJ6Q

@yrliou
Copy link
Member

yrliou commented Apr 20, 2023

Assigned to @cypt4.

@yrliou yrliou changed the title Disable Brave Wallet handler if MM is installed and "Extensions (Brave Wallet fallback)" is the default setting. Revise the behavior of "Extensions (Brave Wallet fallback)" setting Apr 20, 2023
cypt4 added a commit to brave/brave-core that referenced this issue Apr 21, 2023
@bbondy
Copy link
Member

bbondy commented Apr 21, 2023

Just noting that we'll also remove the ability for web3 apps to query public blockchain when the wallet is not created by doing this. I think this is ok, but I just wanted to note that the scope of the change goes beyond not prompting the user to setup a wallet when a dapp tries to ask for permission.

cypt4 added a commit to brave/brave-core that referenced this issue Apr 24, 2023
cypt4 added a commit to brave/brave-core that referenced this issue Apr 25, 2023
…18172)

* Disable brave wallet script injection if wallet is not created or MM is installed
Resolves brave/brave-browser#29853
@brave-builds brave-builds added this to the 1.52.x - Nightly milestone Apr 25, 2023
@kjozwiak
Copy link
Member

kjozwiak commented Apr 25, 2023

@jamesmudgett @cypt4 @yrliou set the above as QA/Yes as it looks like we have some STR/Cases that @srirambv can run through via brave/brave-core#18172 (comment). Please change to QA/No if I'm wrong re: the above being a QA/Yes. It should also get checked as there's associated uplifts as per brave/brave-core#18228 & brave/brave-core#18229.

@yrliou
Copy link
Member

yrliou commented Apr 25, 2023

@kjozwiak Thanks, yes it should be QA/Yes.

@kjozwiak
Copy link
Member

The above requires 1.50.128 or higher for 1.50.x verification 👍 However, I doubt we'll have another 1.50.x release as 1.51.x is scheduled for Tuesday with C113. If we don't get a C112 update tomorrow, I'll close off the 1.50.x milestone and move this issue into 1.51.x.

@kjozwiak
Copy link
Member

kjozwiak commented May 1, 2023

Moving this into 1.51.x as it looks like we're not going to get another 1.50.x release. The above can basically be checked with https://github.com/brave/brave-browser/releases/tag/v1.51.107 or higher as the above landed into 1.51.x ~5 days ago.

@kjozwiak kjozwiak modified the milestones: 1.50.x - Release #6, 1.51.x - Release May 1, 2023
@srirambv
Copy link
Contributor

srirambv commented May 2, 2023

Verification passed on

Brave 1.51.109 Chromium: 113.0.5672.63 (Official Build) (64-bit)
Revision 0e1a4471d5ae5bf128b1bd8f4d627c8cbd55f70c-refs/branch-heads/5672@{#912}
OS Windows 11 Version 22H2 (Build 22621.1344)
  • Verified steps from brave/brave-core#18172
  • Verified when no wallet is created window.ethereum and window.solana shows as undefined
  • Verified when Brave Wallet is created window.ethereum and window.solana shows isBraveWallet: true
  • Verified when MetaMask is installed window.solana continues to show isBraveWallet: true
  • Verified when MetaMask is installed window.ethereum doesn't show isBraveWallet: true but instead shows isMetaMask: true
29853.mp4

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature/web3/wallet/core feature/web3/wallet Integrating Ethereum+ wallet support OS/Desktop priority/P3 The next thing for us to work on. It'll ride the trains. QA Pass-Win64 QA/Yes release-notes/include
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

8 participants