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

[WalletConnect] Unsupported namespace methods in the approved session #709

Closed
Sparragus opened this issue Jun 19, 2023 · 3 comments
Closed

Comments

@Sparragus
Copy link

Sparragus commented Jun 19, 2023

Description

When a dapp wants to connect to a wallet using WalletConnect V2, it needs to propose a session. In the proposed session, the dapp declares which RPC methods are required for it to work properly. It can also declare additional optional methods.

If the wallet application supports the required methods, but only a subset of the optional methods, it should approve the session with only the method/namespace it supports.

In the WalletConnect Safe App, we're approving ALL the methods requested (required + optional) even when the Wallet Connect Safe App doesn't support them. This is happening here: https://github.com/safe-global/safe-react-apps/blob/development/apps/wallet-connect/src/hooks/useWalletConnectV2.tsx#L221

Instead of approving the session with all the requested methods, only supported methods should be approved.

The list of currently supported methods can be found in the safe-apps-provider.

Impact

This bug is currently affecting users that use wagmi to connect their dapp to the WalletConnect Safe App. Personally, it's impacting me in my work (Immunefi)), but also other users in the internet. See https://twitter.com/0xDrewF/status/1665724631360077825, for example.

The bug is blowing up for wagmi users because their connector asks if there is support for wallet_addEthereumChain, the the Safe App is saying yes. See https://github.com/wagmi-dev/references/blob/main/packages/connectors/src/walletConnect.ts#LL239C34-L239C34. It then proceeds to use a mehtod which isn't supported.

Suggested changes

  1. Revert commits d9bf862 and d39bcb1. These commits were introduced in feat(wallet-connect): update safe-apps-sdk to enable synchronous off-chain signatures #657. FWIW: When this change was introduced, there was acknowledgement that it was an out-of-scope change. See d9bf862#r1185284454.
  2. Release a new version of the WalletConnect Safe App.
  3. Fix the bug by checking the list of required and optional methods, and only approve the session if the required can be fulfilled, and only with the supported optional methods.

I'll be happy to assist with these changes. As a starting point, here's a PR: #710

Environment

Steps to reproduce

  1. Go to https://kmdt6c.csb.app/ (source code here: https://codesandbox.io/s/black-butterfly-kmdt6c?file=/src/App.tsx)
  2. Click connect, and connect to a Safe. Here's one I use in goerli. https://app.safe.global/apps/open?safe=gor:0xA632d4A98eA44A5bf02229158d2c954B3782eB50&appUrl=https%3A%2F%2Fapps-portal.safe.global%2Fwallet-connect.
  3. You'll see an error on both apps: "wallet_addEthereumChain" not implemented.

Expected result

No error. Successful WalletConnect v2 connection

Obtained result

An error on both apps: "wallet_addEthereumChain" not implemented.

Screenshots

image
image

@Sparragus
Copy link
Author

Fixed in #706.

@zigzag-way
Copy link

I still get this bug.
For ex I can not connect to THENA.FI using SafePal. I get this error while trying to connect through WalletConnect.

@damianlluch
Copy link

I think I have the same error.

IMG_C9F078BD97FA-1

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

Successfully merging a pull request may close this issue.

3 participants