WalletConnect URIs (esp. Deep Link) do not connect when opened from Android browser

w4-jake commented 2 years ago

Severity (reporter's perspective) and XY problem discussion

Other developers may of course have different priorities, but for us it is very high priority that a user be able to connect their MetaMask wallet to our web app when our web app is accessed on an Android device's browser. Our preferred approach so far has been to open a deep link for the WalletConnect protocol, but if there is a better approach, then any guidance would be appreciated!

And if there is any more we can do to assist the team in looking into this issue, please do not hesitate to let us know. Thank you!

Describe the bug

Similar to #5004 and #4955, but the scenario (Deep Link opened from Android browser) is slightly different, so I have made a new issue. Apologies if this comes across as a duplicate!

In a browser like Chrome, Firefox, DuckDuckGo, etc. on an Android mobile device, opening a deep link to MetaMask with a valid WalletConnect URI parameter results in the MetaMask app opening, but no option to 'Connect' appears.

Screenshots

Please compare the two videos below. Opening an identically formatted deep link from a browser like Safari on an iOS device works as expected.

To Reproduce

Receive a WalletConnect session URI. It should start with wc: (see: WalletConnect v1 docs)
With this received URI walletConnectUri, construct a deep link in the form metamask://wc?uri=${walletConnectUri}.
Open this deep link in a browser on a mobile device with MetaMask installed.
See whether option to 'Connect' appears after MetaMask opens.

Please reference the sample Flutter app from the videos below, where this issue can be reproduced and tested on a variety of different environments. We discuss extensively which kind of links (raw wc: URI, deep link, universal link) work in which kind of environment here.

Expected behavior

After MetaMask opens, the connecting app information, account information, 'Connect' button, 'Cancel" button, and so on should show. Please see the iOS screen capture.

Device info

Android

Device: Galaxy S9
OS: Android 10
App Version: v5.9.1 (995)

iOS

Device: iPhone 6
OS: iOS 12.5.6
App Version: v5.9.1 (995)

Screen captures

Android browser

https://user-images.githubusercontent.com/52257615/200474088-50c03752-4707-484d-9b45-cced56db4bd9.mp4

iOS browser

https://user-images.githubusercontent.com/52257615/200474033-e5560a7d-1aed-439f-87ee-732611c033c3.MOV

to be added after bug submission by internal support / PM Severity

How critical is the impact of this bug on a user?
Add stats if available on % of customers impacted
Is this visible to all users?
Is this tech debt?

lunajuan commented 1 year ago

I noticed a similar behavior on android when using the metamask protocol. In my case it didn't work when the wallet-connect uri was encoded but it did work if it wasn't.

Seems encoded wallet-connect uri is considered invalid here: https://github.com/MetaMask/metamask-mobile/blob/18dc20b8cc9f3f0707f5eb04f2ac88ee320c93bb/app/core/DeeplinkManager.js#L321

w4-jake commented 1 year ago

Hi @lunajuan, thanks for pointing that out. Just to make sure I fully understand, by the wallet-connect URI being 'encoded', do you mean the difference between ':' (not URI encoded) and '%3A' (URI encoded) in a URI like metamask://wc?uri=wc%3A50eb69e9...?

It looks like the logic that parses the URI is provided from @walletconnect/utils. I wonder if an encoded URI is not being correctly parsed inside the parseWalletConnectUri funtion.

https://github.com/MetaMask/metamask-mobile/blob/18dc20b8cc9f3f0707f5eb04f2ac88ee320c93bb/app/core/WalletConnect.js#L451-L457

In my sample Flutter app for reproducing the issue, I am not anywhere encoding the Wallet Connect URI before attempting to open it. Hmm...do some operating systems automatically encode the URI, while others do not?

w4-jake commented 1 year ago

Ahh, I think I found the parseWalletConnectUri function. It is located now in the walletconnect-legacy repo, not the current WalletConnect utils repo, as it is from the v1 protocol.

https://github.com/WalletConnect/walletconnect-legacy/blob/4563223a9fe2ff099e7a66788813697e09b1e35c/packages/utils/src/session.ts#L14-L34

Looks like it is looking for that unencoded ':' character, so this confirms why an encoded URI is not being parsed correctly.

Is there any way that MetaMask can unencode incoming deep links inside the WalletConnect.isValidUri function so that this legacy WalletConnect parsing function always receives a URI of the form wc:${handshakeTopic}..., not wc%3A${handshakeTopic}...?

Alternatively, is there a better solution here that invloves either

working directly with the WalletConnect developers to have the parseWalletConnectUri function itself accept encoded URIs, or
making sure on our end in Flutter that regardless of the device or OS, Metamask always is sent an unencoded URI? (Not sure if this is even possible as that might be OS/device dependent.)

lunajuan commented 1 year ago

yeah, that's what I meant by an encoded uri. I'm working in react native so I just went with your second option which is to not encode the uri. Interestingly, other wallet apps I tested wanted the uri to be encoded hahaha!

Seems like WalletConnect team really wants everyone to use v2 so I would think your second choice is best option for now if you can figure it out. 🤞

side note, I wonder why the uri is not validate in a similar way if you use the universal link https://github.com/MetaMask/metamask-mobile/blob/18dc20b8cc9f3f0707f5eb04f2ac88ee320c93bb/app/core/DeeplinkManager.js#L223-L229

w4-jake commented 1 year ago

Yeah, that makes sense. I am very much looking forward to v2 support for MetaMask...looks like WalletConnect team wants all wallets to support v2 within just a few weeks, but I'm not sure if the MetaMask team's timeline will coincide. (Here's the most relevant issue I could find.)

So it seems like a lost cause to get WalletConnect to change their legacy parser. Perhaps with Flutter there is a good way to make sure an unencoded URI is always sent regardless of device/OS. But if there is no reliable way to do this, then it would be nice if MetaMask could simply decode the URI inside the isValidUri function before passing to WalletConnect's parser.

As for the universal link, interesting that the way to validate is different! My sample app never once got universal link to work so I passed over considering it altogether.

andreahaku commented 1 year ago

we are planning to support WalletConnect v2 in late Q1 2023. In the meantime we are working on a refactor of the Deeplink manager that should solve this issues.

tarrouye commented 1 year ago

@andreahaku we are planning to support WalletConnect v2 in late Q1 2023. In the meantime we are working on a refactor of the Deeplink manager that should solve this issues.

late Q1 2023 is far past the deadline WalletConnect has set for wallets of December 16th, 2022 https://medium.com/walletconnect/walletconnect-v1-0-sunset-notice-and-migration-schedule-8af9d3720d2e

MetaMask being one of the leading wallets in terms of adoption, dragging your feet on this holds the whole industry back as everyone must maintain v1 support because we can't drop MetaMask support 😞

andreahaku commented 1 year ago

@tarrouye WC v2 and v1 can and will live together. So you can keep compatibility with v1 while implementing v2.

The integration of v2 requires a series of changes that we are already implementing as they are already planned as part of MetaMask mobile evolution. When all those pieces will be completed, we'll be ready to integrate WCv2 as part of it. BTW, we are in close contact with WalletConnect and they are aware of our scheduling.

Most importantly, there are still standards to be fully defined and that both WC and MetaMask are contributing (along with other actors) to define. So we are not dragging anyone. We are just waiting for times to be mature for a secure standard implementation of such a shift.

tarrouye commented 1 year ago

@tarrouye WC v2 and v1 can and will live together. So you can keep compatibility with v1 while implementing v2.

The integration of v2 requires a series of changes that we are already implementing as they are already planned as part of MetaMask mobile evolution. When all those pieces will be completed, we'll be ready to integrate WCv2 as part of it. BTW, we are in close contact with WalletConnect and they are aware of our scheduling.

Most importantly, there are still standards to be fully defined and that both WC and MetaMask are contributing (along with other actors) to define. So we are not dragging anyone. We are just waiting for times to be mature for a secure standard implementation of such a shift.

Per WC documentation, wallets can support v1 and v2 simultaneously but Dapps should only support one or the other. So until all major wallets use v2, Dapps cannot move to v2: https://medium.com/walletconnect/walletconnect-sign-v2-0-the-final-release-is-here-864b21e8d1ca and https://docs.walletconnect.com/2.0/advanced/migrating-from-v1.0 and

(from their Discord)

Is this a mis-communication ? Could you point to the standards you refer to ? 🙏 Thanks for your answers :)

w4-jake commented 1 year ago

Thanks @andreahaku for the update on the Deeplink manager. I look forward to trying everything out when the refactor you mentioned is finished!

github-actions[bot] commented 1 year ago

This issue has been automatically marked as stale because it has not had recent activity in the last 90 days. It will be closed in 7 days. Thank you for your contributions.

ZZZDAD commented 1 year ago

@tarrouye WC v2 and v1 can and will live together. So you can keep compatibility with v1 while implementing v2.

The integration of v2 requires a series of changes that we are already implementing as they are already planned as part of MetaMask mobile evolution. When all those pieces will be completed, we'll be ready to integrate WCv2 as part of it. BTW, we are in close contact with WalletConnect and they are aware of our scheduling.

Most importantly, there are still standards to be fully defined and that both WC and MetaMask are contributing (along with other actors) to define. So we are not dragging anyone. We are just waiting for times to be mature for a secure standard implementation of such a shift.

@andreahaku Hello, I am very much looking forward to the fact that metamask can support the wallet connect2.0 protocol, and the development of our dapp is also waiting anxiously. May I ask if there is a time when the 2.0 protocol can be supported?

siosio34 commented 1 year ago

any update for this issue?

github-actions[bot] commented 1 year ago

This issue has been automatically marked as stale because it has not had recent activity in the last 90 days. It will be closed in 7 days. Thank you for your contributions.

github-actions[bot] commented 1 year ago

This issue was closed because it has been stalled for 7 days with no activity. If you feel this was closed in error please reopen and provide evidence on the current production app. Thank you for your contributions.

MetaMask / metamask-mobile