wevm / references

Collection of Chains & Connectors for wagmi
MIT License
99 stars 205 forks source link

WC v2 Proposal namespaces with respect of smart contract wallets #330

Closed niallkh closed 1 year ago

niallkh commented 1 year ago

Is there an existing issue for this?

Current Behavior

More users currently are starting using smart contract wallets multichain, especially safe multisig. Smart contract wallet can be deployed selectively on specific chain. Current configuration of WalletConnectConnector doesn't respect this and sends ethereum mainnet as required network. Dapp should be able to work equally on different chains without specifying required network. Because smart contract wallet may be not deployed on required chain, but deployed on optional chains. Wallet can't continue working with dapp because it violates wallet connect protocol 2.4. Session Namespaces MUST contain at least one account in requested chains.

Code responsible for selecting required and optional chains:

const [defaultChain, ...optionalChains] = this.chains.map(({ id }) => id)
...
         chains: [defaultChain],
         optionalChains: optionalChains,

Expected Behavior

Dapp should be able to send proposal with empty required chains and not empty optional chains. From dapp development perspective I think that required chains make sense in rare cases, for instance, if dapp's contracts are deployed only on one chain. When dapp works on several chains it doesn't make sense to provide them as required because wallet may be deployed only in part of them, they should be provided as optional.

For instance, if dapp configures 1 chain then it should be required. If dapp configures several chains then all chains should be optional.

Steps To Reproduce

No response

Link to Minimal Reproducible Example (CodeSandbox, StackBlitz, etc.)

No response

Anything else?

No response

tmm commented 1 year ago

You should, follow up with the WalletConnect team on this: https://github.com/orgs/WalletConnect/discussions

niallkh commented 1 year ago

@tmm The issue is on wagmi side

glitch-txs commented 1 year ago

WalletConnect follows CAIP-25, which doesn't allow setting empty required namespaces, I guess you could be intereded in joining CAIP discussion :)

niallkh commented 1 year ago

@glitch-txs I don't see any contradictions in CAIP-25 because requiredScopes can be not present. The requiredScopes array MUST contain 1 or more scopeObjects, if present.

But it seems like issue is in implementation of ethereum provider by WalletConnect. Thanks.

glitch-txs commented 1 year ago

My bad I was thinking about this https://docs.walletconnect.com/2.0/specs/clients/sign/namespaces#12-proposal-namespaces-must-not-have-chains-empty

Ethereum-provider is an abstraction on top of universal provider to be 1193 compliant, we should look at the source code to check if what you want is possible I guess 🤔 have you tested it?

glitch-txs commented 1 year ago

My bad I as thinking about this https://docs.walletconnect.com/2.0/specs/clients/sign/namespaces#12-proposal-namespaces-must-not-have-chains-empty

Ethereum-provider is an abstraction on top of universal provider to be 1193 compliant, we should look at the source code to check if what you want is possible I guess 🤔 have you tested it?

Also as seen in the docs chains are required in this library https://docs.walletconnect.com/2.0/web/providers/ethereum#initialization So I assume this falls out of wagmi's scope

niallkh commented 1 year ago

So I assume this falls out of wagmi's scope

Yes, started discussion. Please, join. https://github.com/orgs/WalletConnect/discussions/2733

amal commented 1 year ago

With 2.9.0 empty required chains/namespaces are accepted. Now it's on wagmi to support it, isn't it?

glitch-txs commented 1 year ago

Setting empty chains breaks for some wallets, maybe we could add directly the option of chains and optionalChains for devs to hardcode in the connector

See comment

tremarkley commented 1 year ago

@tmm can this be re-opened and revisited now that WalletConnect 2.9.0 supports passing in an empty chains array to EthereumProvider.init? This change to was made specifically to help address this issue (more details here).