xmtp / xmtp-js

XMTP client SDKs, content types, and other packages written in TypeScript
https://xmtp.org/docs
211 stars 40 forks source link

Bug: Double signature is buggy on mobile wallets on iOS with walletconnect #275

Closed nmalzieu closed 1 year ago

nmalzieu commented 1 year ago

Describe the bug

On mobile we're using WalletConnect and WalletConnect links to mobile wallets using Universal Links

After discussing with WalletConnect, Universal Links only work when triggered from a user action (for instance clicking on a button)

When a user logs in to XMTP for the first time, the XMTP SDK asks for 2 signatures in a row : "Create Identity" & "Enable Identity". Since the second signature is not triggered by a user action but automatically, it often fails to open the mobile wallet.

I think we should have a way (probably a setting in Client.create or auto-detect that we are on mobile) to split those two signatures.

If the user logs in for the first time, we don't want to auto trigger a second signature but have a button to do the second signature.

Screenshot of our discussion with WalletConnect:

Capture d’écran 2023-02-15 à 17 36 45
galligan commented 1 year ago

@nmalzieu thanks for this. Am I reading this right that with WalletConnect if the second signature were initiated with a user action, this wouldn't be a problem?

nmalzieu commented 1 year ago

@galligan exactly yes. Apple Universal links are https links that are supposed to open the native app if it's installed. But they only do so if the link has been accessed from a recent user action

WalletConnect actually had some information about this on their website : https://docs.walletconnect.com/1.0/mobile-linking#ios-app-link-constraints

nmalzieu commented 1 year ago

It might work sometimes but to be sure it works 100% of the time, I think splitting the 2 signatures during a signup is the way to go.

nplasterer commented 1 year ago

I think this is related to https://github.com/xmtp/xmtp-js/issues/264.

nmalzieu commented 1 year ago

I think this is related to https://github.com/xmtp/xmtp-js/issues/264.

Totally! But wanted to first focus on the specific case of dual signature and the obvious fix: split them :)

neekolas commented 1 year ago

So, because we have one JS SDK to support web, Node.js, and mobile webviews, we'd have to be careful about rolling out a change like this. It's definitely possible, but would require some thorough testing to make sure that we don't break any of the headless flows/existing apps. We would need to default to the current flow and then add an option to make it a two-phase operation.

But clearly you aren't the only team that is going to run into it on mobile. We can prioritize it for the next major release.

nmalzieu commented 1 year ago

Got it yeah we should probably have a "default" and a feature to override the setting. In the meantime I'll hack around something on my signer instance

yash-luna commented 1 year ago

Supported in xmtp-js v8+