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

feat: add Brave Wallet #448

Merged
merged 1 commit into from
May 31, 2022

Conversation

markdalgleish
Copy link
Contributor

@markdalgleish markdalgleish commented May 31, 2022

I had to make a couple of decisions here that might need some discussion before merging:

  • I made sure that Brave Wallet is only visible in the wallet list when using Brave. It seemed like a bad user experience to me to prompt users to download an entirely different web browser just to use its built-in wallet. If we did want to support this flow, we currently only have messaging for installing browser extensions (i.e. "The MetaMask extension is not installed in your browser") so we'd need to come up with proper messaging for downloading another browser.
  • I added Brave Wallet to the getDefaultWallets list, but as per the previous point, this will only make a difference to Brave users. I thought this was a good idea due to the difficulty of detecting injected wallets while also dealing with wallets that impersonate MetaMask. If we leave this up to developers, it's very difficult to get it right.

Also note that, as part of this work, I noticed that the "Injected Wallet" fallback wasn't visible in Brave initially even though this should have been the case. Turns out it's because Brave Wallet impersonates MetaMask by setting window.ethereum.isMetaMask to true. However, the MetaMask option didn't work as a fallback because wagmi correctly identifies that Brave Wallet is impersonating MetaMask. To fix this, I updated our MetaMask installation check to match wagmi's.

@markdalgleish markdalgleish requested a review from a team as a code owner May 31, 2022 05:18
@linear
Copy link

linear bot commented May 31, 2022

@vercel
Copy link

vercel bot commented May 31, 2022

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated
rainbowkit-example ✅ Ready (Inspect) Visit Preview May 31, 2022 at 5:18AM (UTC)
rainbowkit-site ✅ Ready (Inspect) Visit Preview May 31, 2022 at 5:18AM (UTC)

@peduarte
Copy link
Contributor

Nice man, some super interesting finds there.

I created a Brave Wallet to test this out, and kept MetaMask. While having both wallets activated, when I click on the connect button I only the option to connect to MetaMask, and Brave isnt on the list.

Then, I deactivated MetaMask and was able to see and successfully connect to Brave Wallet.

Should I be seeing the option to choose between MetaMask or Brave Wallet, in the case where I have them both? Or is it not possible because there can only be one injected one?

@markdalgleish
Copy link
Contributor Author

markdalgleish commented May 31, 2022

From what I could see, you can't use Brave Wallet and MetaMask at the same time. You can see which one is active by checking window.ethereum.isBraveWallet in the console. If this is undefined then we don't show Brave in the list.

Brave lets you configure which wallet should get priority when multiple are available:

Screen Shot 2022-06-01 at 7 43 03 am

@nickbytes
Copy link
Contributor

I like these decisions. I find the way Brave treats Metamask to be a little strange tbh. I think this is a good solution.

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

Successfully merging this pull request may close these issues.

None yet

3 participants