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

Split out default wallet prefs for Ethereum and Solana #24079

Closed
mirkoschubert opened this issue Jul 15, 2022 · 9 comments · Fixed by brave/brave-core#14288
Closed

Split out default wallet prefs for Ethereum and Solana #24079

mirkoschubert opened this issue Jul 15, 2022 · 9 comments · Fixed by brave/brave-core#14288
Assignees
Labels

Comments

@mirkoschubert
Copy link

Description

If you set the Brave wallet as the default cryptocurrency wallet in v1.41.96 and have another Solana wallet extension such as the Phantom wallet installed, you can't connect to dApps with the 3rd party wallet, even if you didn't create any Solana account in the Brave wallet.

Steps to Reproduce

  1. Install the latest Brave browser (v1.41.96) on macOS (not tested on Windows or Linux, but I suspect it would be the same result)
  2. Set up the Brave wallet without creating a Solana wallet
  3. Set the Default Cryptocurrency Wallet to Brave Wallet at brave://settings/wallet
  4. Install a 3rd party Solana wallet extension (tested with the Phantom wallet)
  5. Import or set up a Solana wallet in the Phantom wallet
  6. Try to connect to any dApp (tested with magiceden.io, solsea.io and others) with your Phantom wallet

Actual result:

After trying to connect to a dApp with a 3rd party Solana Wallet (e.g. Phantom Wallet) nothing happens and the connection failed. In the console of the Dev Tools there is an error message:

Uncaught TypeError: Cannot redefine property: solana

screenshot 2022-07-15 at 12 54 45

Expected result:

The dApp should easily connect to the 3rd party Solana wallet extension even with the Brave Wallet as default wallet, if no Solana wallet was created in the Brave wallet.

Reproduces how often:

Easily reproduced

Brave version (brave://version info)

Brave: 1.41.96 Chromium: 103.0.5060.114 (Official Build) (x86_64)
Revision: a1c2360c5b02a6d4d6ab33796ad8a268a6128226-refs/branch-heads/5060@{#1124}
OS: macOS Version 12.4 (Build 21F79)

Version/Channel Information:

  • Can you reproduce this issue with the current release? YES
  • Can you reproduce this issue with the beta channel? UNTESTED
  • Can you reproduce this issue with the nightly channel? UNTESTED

I didn't have any problems before the 1.41.96 release.

Other Additional Information:

  • Does the issue resolve itself when disabling Brave Shields? NO
  • Does the issue resolve itself when disabling Brave Rewards? UNTESTED
  • Is the issue reproducible on the latest version of Chrome? NO

The issue is only applicable if a Brave wallet is present. I couldn't reproduce the issue with the Phantom extension in a private window, where the Brave wallet isn't enabled. On other browsers such as Chrome the issue doesn't appear.

Explanation:

It seems that the Solana part of the Brave wallet interferes with 3rd party Solana wallet extensions. Even if no Solana wallet has been created within the Brave wallet the window.solana object will be written before any other Solana wallets.

There is a temporary solution: The user can always set the Default cryptocurrency wallet to Brave Wallet (Prefer extensions) at brave://settings/wallet. This fixes it for now.

But many users would update the Brave browser without knowing that something has changed and would get this issue instantly. They wouldn't think of changing this setting.

Possible Solution:

With the default wallet set to Brave Wallet, but no Solana wallet created, the window.solana object shouldn't be written as well. So 3rd party Solana wallets would work as expected until the user creates or imports a Solana wallet within the Brave wallet.

Maybe you should also warn the user if changes like that wouldn't work with current setting after an update.

@mirkoschubert mirkoschubert changed the title Brave Wallet interferes with other Solana Wallet extensions when loggin in to dApps Brave Wallet interferes with other Solana Wallet extensions when connecting to dApps Jul 15, 2022
@srirambv
Copy link
Contributor

cc: @darkdh

@srirambv srirambv added feature/web3/wallet Integrating Ethereum+ wallet support feature/web3/wallet/solana labels Jul 15, 2022
@srirambv
Copy link
Contributor

srirambv commented Jul 15, 2022

Probable solution would be doing this #23898 rather than completely removing window.solana

cc: @bbondy @jamesmudgett

@jamesmudgett
Copy link

@mirkoschubert could you share what you have set for "Default cryptocurrency wallet" in brave://settings/wallet?

@mirkoschubert
Copy link
Author

@jamesmudgett Already did. 😄 I had Brave Wallet and fixed it with Brave Wallet (Prefer extensions) for the moment.

But that's the issue: If you have set it to Brave Wallet, but don't have a Solana account in the Brave wallet, it shouldn't interfere with extensions as well. Setting up the window.solana object without any need is the problem here. 😉

@jamesmudgett
Copy link

That's not the way defaults work ;) unless you mean we should break into more granular defaults like:

  • Default Ethereum/EVM wallet
  • Default Solana wallet

@mirkoschubert
Copy link
Author

@jamesmudgett Btw, it did work with Solana support and Brave Wallet as default before 😉 The issue just appeared in the last version when (Solana) dApp support was introduced to Brave.

But more granular defaults would fix the issue as well. 😊

@jamesmudgett jamesmudgett added the priority/P2 A bad problem. We might uplift this to the next planned release. label Jul 20, 2022
@jamesmudgett
Copy link

Screen Shot 2022-07-20 at 9 55 38 AM

  1. Create a new row with identical default wallet options.
  2. Rename the two options @rmcfadden3 to confirm the text or offer up alternatives.

"Default Ethereum/EVM wallet"
"Default Solana wallet"

@rmcfadden3
Copy link

@jamesmudgett — no edits from me. your copy makes sense.

@bbondy bbondy changed the title Brave Wallet interferes with other Solana Wallet extensions when connecting to dApps Split out default wallet prefs for Ethereum and Solana Jul 21, 2022
@bbondy bbondy self-assigned this Jul 21, 2022
@bbondy bbondy added this to the 1.43.x - Nightly milestone Jul 25, 2022
@srirambv
Copy link
Contributor

Verification passed on

Brave 1.43.81 Chromium: 104.0.5112.102 (Official Build) (64-bit)
Revision 8e5396254975ef939f2ef7d0bd334e48a052b536-refs/branch-heads/5112@{#1478}
OS Linux
  • Verified steps from brave/brave-core#14288
  • Verified there is an individual setting to set default wallet provider for Solana and Ethereum
  • Verified setting either/both of them to Brave Wallet or None triggers the default wallet notification in brave://wallet
  • Encountered #24932
Ethereum Solana
image image

Verification passed on

Brave 1.43.81 Chromium: 104.0.5112.102 (Official Build) (64-bit)
Revision 8e5396254975ef939f2ef7d0bd334e48a052b536-refs/branch-heads/5112@{#1478}
OS Windows 11 Version 21H2 (Build 22000.795)
  • Verified steps from brave/brave-core#14288
  • Verified there is an individual setting to set default wallet provider for Solana and Ethereum
  • Verified setting either/both of them to Brave Wallet or None triggers the default wallet notification in brave://wallet
  • Encountered #24932
Ethereum Solana
image image

Verification passed on

Brave 1.43.81 Chromium: 104.0.5112.102 (Official Build) (arm64)
Revision 8e5396254975ef939f2ef7d0bd334e48a052b536-refs/branch-heads/5112@{#1478}
OS macOS Version 12.4 (Build 21F79)
  • Verified steps from brave/brave-core#14288
  • Verified there is an individual setting to set default wallet provider for Solana and Ethereum
  • Verified setting either/both of them to Brave Wallet or None triggers the default wallet notification in brave://wallet
  • Encountered #24932
Ethereum Solana
image image

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

Successfully merging a pull request may close this issue.

5 participants