blocknative / web3-onboard

Client library to onboard users to web3 apps
https://onboard.blocknative.com/
MIT License
845 stars 502 forks source link

otherProviderFlagsExist for metamask is unnecessary #1900

Closed puderty closed 1 year ago

puderty commented 1 year ago

Current Behavior

for metamask, you have a otherProviderFlagsExist check, and lot of wallet set isMetamask to true with their own flag. And then the user can't connect by metamask even if the wallet is metamask compatible. Why is this necessary?

Expected Behavior

No response

Steps To Reproduce

connect to dapp demo in okx mobile wallet by click on metamask

What package is effected by this issue?

@web3-onboard/core

Is this a build or a runtime issue?

Runtime

Package Version

latest

Node Version

No response

What browsers are you seeing the problem on?

No response

Relevant log output

No response

Anything else?

No response

Sanity Check

puderty commented 1 year ago

And also why is platform restricted to desktop for okx wallet? image

Adamj1232 commented 1 year ago

@puderty the OKX wallet integration was done by the OKX team. If you would like to open a PR to add mobile support and test please feel free to do so. otherProvidersFlagsExists method is necessary to reduce spoofing. Wallets should not set both flags to be true, this is bad UX and misleading users. MetaMask is the only wallet that should set isMetaMask to true and MetaMask should be the only wallet that opens if the user selects MetaMask. Im not sure what you mean by MetaMask compatible, can you please clarify?

puderty commented 1 year ago

For some dapp not using your sdk, and if they only integrate with metamask, if we don't set ismetamask to true, the dapp cannot connect to okx mobile wallet. So we have to set it for compatible reason, but your check will broken it and make it cannot connect by click on metamask when using blocknative.

I'm from okx wallet mobile team, if otherProvidersFlagsExists is necessary, can you change the platform from desktop to all for okx wallet?

Adamj1232 commented 1 year ago

@puderty set it for compatible reason is spoofing and is a misleading UX. If you are from the OKX team please pass this along to the dev team. If the wallet is to be used the provider flag needs to be specified and not rely upon MetaMask to mislead people into selecting one wallet and getting another. I opened a PR to update the devices for OKX: #1901

puderty commented 1 year ago

But can we remove otherProvidersFlagsExists on mobile? Because the on mobile app there should be only one injected ethereum object and it's injected by the mobile web3 wallet the user is using.

Adamj1232 commented 1 year ago

@puderty that update was made as part of this PR - https://github.com/blocknative/web3-onboard/pull/1901/files and will be merged into staging shortly. This should allow OKX wallet to display properly on mobile. We wont be removing otherProvidersFlagsExists from MetaMask as this is to avoid spoofing of wallets.

puderty commented 1 year ago

Can you add some flag under window object so we can check that the dapp is using this library? So we can avoid setting other flags for dapps using this library. Such as window.isUsingBlockNative = true.

Adamj1232 commented 1 year ago

We don't alter the provider object on the window so there is not a flag we can set. Please try using the latest alpha packages of core, react or vue to see if the update to the device settings for OKX wallet solves the issue - See #1901 for those versions

Adamj1232 commented 1 year ago

@puderty support should be stabilized with a release going out today. We would be interested in doing a marketing post to our community around wallet support. Feel free to reach out for a contact within Blocknative if interested

puderty commented 1 year ago

Sure. I still suggest we remove the check on mobile, because for every mobile web3 wallet, there's only one injected window.ethereum, and every wallet should do their best to help their users to connect the dapp to the wallet. A lot of dapps only show Metamask as the only option. There's no spoofing on mobile web3 wallet or mobile browser.