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

refactor: MetaMask and Brave Wallet collision #436

Merged
merged 1 commit into from
May 9, 2022
Merged

Conversation

tmm
Copy link
Member

@tmm tmm commented May 9, 2022

Description

Additional Information

Your ENS/address: awkweb.eth

@changeset-bot
Copy link

changeset-bot bot commented May 9, 2022

⚠️ No Changeset found

Latest commit: 58f8d6f

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@vercel
Copy link

vercel bot commented May 9, 2022

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

Name Status Preview Updated
wagmi ✅ Ready (Inspect) Visit Preview May 9, 2022 at 11:11PM (UTC)

@bbondy
Copy link

bbondy commented Jun 28, 2022

@tmm why do you have this code? It seems like it's causing a lot of webcompatability issues that don't need to be there. For example we would work with https://foundation.app/feed otherwise, but this PR causes it to not be able to connect with Brave Wallet.

@tmm
Copy link
Member Author

tmm commented Jun 28, 2022

@bbondy Hi there! Looks like they are using the MetaMaskConnector instead of the InjectedConnector. The MetaMaskConnector only works with MetaMask, while the InjectedConnector works with any injected wallet (MetaMask, Brave Wallet, Coinbase Wallet, Tally Ho, etc.). You can see the difference between the two in the Connect Wallet Example.

why do you have this code?

Sometimes developers want to target specific wallets to do things, like show icons or custom interface. With injected wallets, it's hard to isolate a specific one from the group – e.g. maybe the injected wallet is unknown to the app developer or there are multiple attached to the window (e.g. Coinbase Wallet's ethereum.providers approach). This code exists to give folks the option to just target MM if they want. In fact, anyone can create a custom connector.

Happy to chat more if you have additional questions.

@bbondy
Copy link

bbondy commented Jun 28, 2022

thanks @tmm

@bbondy
Copy link

bbondy commented Jun 28, 2022

If we're API compatible with MetaMask though, what is the benefit of blocking Brave Wallet from the MetaMask option?

@bbondy
Copy link

bbondy commented Jun 29, 2022

To add a bit more depth to my previous message.

Brave Wallet is a wallet like other wallets, but it is also an implementation of the MetaMask API. That is why we set isMetaMask to true. We spend a lot of effort to make sure we are compatible so we can provide the community with an alternate implementation.

There are a lot of Dapps out there using the MetaMask connector only. We find it is important to provide not only an alternate wallet, but an alternative implementation to the MetaMask API. This allows for Dapp usage that is built for MetaMask but for people that don't want to be locked into MetaMask's control.

I have a suggestion that might be best.

Allow wallets that are compatible to MetaMask to set isMetaMask to true. And by default allow the MetaMask connector to work with it. But also expose an option that a Dapp website can use if it truly wants a more strict check. And don't enable that option by default.

@bbondy
Copy link

bbondy commented Aug 16, 2022

@tmm could you help? ^

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

2 participants