hicommonwealth / commonwealth

A platform for decentralized communities
https://commonwealth.im
GNU General Public License v3.0
67 stars 42 forks source link

Coinbase Wallet Login Issues #5629

Closed RhysCommonwealth closed 9 months ago

RhysCommonwealth commented 10 months ago

Describe the bug

The only option to login with Coinbase Wallet right now is through the Metamask option. When Metamask & Coinbase are both turned on in the browser, the Metamask login option button doesn't do anything (i.e.: launch a metamask login popup) @RhysCommonwealth was able to have the Metamask login option button work once. I was prompted to pick either MM or CB wallet. After signing with CB Wallet, I pressed the "create new account" button and then got an error stating "unable to create account" -> this is how far an element finance admin is getting.

CB Wallet needs to be off in the browser in order for a user to login with MM

To Reproduce

try logging in with CB wallet on Commonwealth

Expected behavior

Additional context

lzach83 commented 9 months ago

Attempted to reproduce this bug, having @RhysCommonwealth reach out to the EU to gather more steps on this.

CowMuon commented 9 months ago

This is a top Growth priority, re-assigning to Ege, who noticed the upstream library needs to be updated (Mihir is still working on OZ for Divastaking, we really need this fix to go out asap, Coinbase users are currently stuck, which is not a good look.

RhysCommonwealth commented 9 months ago

More context: our point of contact for an enterprise account (Element finance) hasn't been able to login to his profile for 27 days

RhysCommonwealth commented 9 months ago

Control Panel error:

Invalid Ethereum address: commonwealth.im wants yo…31596B0e843306B6E4bB13DF4a4B3214113:/commonwealth', stack: 'Error: Invalid Ethereum address: commonwealth.im w…ofbddgcijnmhnfnkdnaad/requestProvider.js:2:38277)', docUrl: 'https://docs.cloud.coinbase.com/wallet-sdk/docs/er…6B0e843306B6E4bB13DF4a4B3214113%3A%2Fcommonwealth

egetekiner commented 9 months ago

We held a meeting and figured, we have three separated issues under this ticket:

1) The Metamask login issue is specifically reproducible and affects the Brave browser. 2) Once logged in and try to add a new wallet/address, the options are not working on all browsers 3) During the log-in flow, once the user clicks Metamask, it is supposed to open a new pop-up screen that gives options for both Coinbase and Matamask.

During the call, we spotted a couple of possible scenarios risen up. The main issue might be about the session keys among different wallet addresses and we need to understand the logic and the nature of this issue.

A possible quick fix for the Coinbase issue, we can add an option to connect the Coinbase wallet to the login screen.

Let's have another call with @masvelio and @Rotorsoft alonside with @ianrowan, @egetekiner and @RhysCommonwealth to go over and resolve this issue.

ianrowan commented 9 months ago

As a starter to a solution here we will want to use some similar logic to this in the future:

if (window.ethereum.providers) {
  const availableWallets = window.ethereum.providers.filter(provider => {
    return provider.name === 'MetaMask' || provider.name === 'Coinbase Wallet'; // select 1 per web wallet in code base
  });

  if (availableWallets.length > 1) {
    // Display UI for user selection
    // ...
  } else {
    const provider = availableWallets[0];
    const web3 = new Web3(provider);
    // ...
  }
} else {
  // Handle case when no providers are available
}

We could only filter metamask in the metemask_web_wallet.ts and allow all other window.ethereum injections to flow into a another web_wallet and/or create a specific wallet for coinbase wallet that only selects the coinbase provider. This paired with addition of a coinbase wallet button on the UI probably makes the most sense product wise

*Note: Its annoying all of these wallets compete for window.ethereum space and typically write scripts to override metamask as default on extension install. We're ultimately at the mercy of changing circumstances here(for example if I install phantom after coinbase, it overrides coinbase as well). Only solution I see would be to write individual web_wallets per evm wallet we want to support outside of wallet connect or use Off the shelf evm modals such as https://web3modal.com/ (Note, this is not explicitly wallet connect but a modal package they built to support wallet connect and extensions in one modal)

CC @egetekiner @CowMuon

jnaviask commented 9 months ago

Superseded by #5968 #5969 #5970 #5971

ianrowan commented 9 months ago

Want to note I think we should also create a spike to investigate potential usage of https://web3modal.com/ long term For reference:

image

CC @jnaviask @CowMuon