Skip to content
This repository has been archived by the owner on Sep 6, 2023. It is now read-only.

fix(wallet-connect): allow the first chain id to be optional during connector init #470

Closed
wants to merge 1 commit into from

Conversation

ElvisKrop
Copy link

Description

Resolves: #469

Additional Information

If dapp supports multiple networks, it is not correct to assume that the first one is required. Therefore, I decided to define chains parameter (required networks for connection) only if there is one network. Otherwise it should be undefined in order to accept connections with wallets that support other networks except the first one from the list of supported networks by the dapp.
Thanks in advance!

@changeset-bot
Copy link

changeset-bot bot commented Aug 8, 2023

⚠️ No Changeset found

Latest commit: 020fc97

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

@glitch-txs
Copy link
Contributor

So far LGTM, adding two comments:

  1. connect function needs refactor for this as well, since chains are handled there.
  2. IMO let's wait for some major wallets to support this config (e.g. MetaMask, Rainbow, AlphaWallet between others).

@dasanra
Copy link
Collaborator

dasanra commented Aug 10, 2023

2. IMO let's wait for some major wallets to support this config (e.g. MetaMask, Rainbow, AlphaWallet between others).

Smart contract wallet users, like Safe users, are not able to use any dApp that relies on wagmi when using WalletConnect until this fix is implemented. Except on the specific case that they try to use it on the chain that happen to be the defaultChain

More context on the fix:
WalletConnect/walletconnect-monorepo#2781

@glitch-txs
Copy link
Contributor

glitch-txs commented Aug 10, 2023

@dasanra Wagmi is not UI ready and also WalletConnectConnector offers an option to set selected chains as required on the connect function, I still think it's an issue that needs to be addressed asap though

@jxom
Copy link
Member

jxom commented Aug 10, 2023

If this is merged, this will break a lot of wallets as not all wallets support optional chains (and may also require at least one required chain).

For example, MetaMask does not support Optional Chains and they also require at least one required chain: MetaMask/metamask-mobile#6703

Will have to think how we can resolve this appropriately.

@ElvisKrop
Copy link
Author

It's great to see so much attention to this issue, guys 🙏

In my opinion, it's not correct to assume that 1st item from the list of passed networks during initialization of connector must be required. If some wallets don't support empty required chain - we should allow devs at least to decide what networks should be required.

Existing implementation on Wagmi side already breaks some dapps. Therefore, we cannot just wait.

@glitch-txs sorry, probably I'm missing something... What exactly do you wanna to refactor in connect method?

@glitch-txs
Copy link
Contributor

glitch-txs commented Aug 10, 2023

@ElvisKrop if you don't pass a chainId into connect function it still gets populated and used as required chain if I'm not mistaken.

This was my previous point, you can still choose which one is required in the connect function.

@jxom
Copy link
Member

jxom commented Aug 11, 2023

As per #470 (comment), we will need to wait.

We will get in touch with WalletConnect to see what we can do.

@tmm
Copy link
Member

tmm commented Sep 1, 2023

Once enough popular wallets support this, this PR can be re-opened on the Wagmi repo. Connectors are moving over there.

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

Successfully merging this pull request may close these issues.

bug: incorrect initialization of chains for WalletConnect V2 ethereum provider
5 participants