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

[BUG] Connector Deeplinks are not working #379

Closed
eduardopelitti opened this issue Apr 30, 2024 · 6 comments
Closed

[BUG] Connector Deeplinks are not working #379

eduardopelitti opened this issue Apr 30, 2024 · 6 comments
Labels
bug Something isn't working

Comments

@eduardopelitti
Copy link

eduardopelitti commented Apr 30, 2024

Describe the bug

When using a connector that requires Deeplinks to trigger connection requests to desktop apps, it always shows the QR code.

To reproduce

Choose any connector that requires deeplinks to work (e.g.: Zerion), but in our case we patched the library to support the Bloom Wallet, a desktop wallet.

Expected behavior

When choosing a connector that requires deeplinks to work, it should open the deeplink.

Screenshots

Screenshot 2024-04-29 at 12 45 46
Screenshot 2024-04-29 at 12 45 49

Environment details

Browser: Brave, Safari, Chrome.

Additional context

The issue appears to be in the following file:

https://github.com/family/connectkit/blob/main/packages/connectkit/src/components/Common/ConnectorList/index.tsx, in these lines:

let deeplink =
    !wallet.isInstalled && isMobile
      ? wallet.getWalletConnectDeeplink?.(uri ?? '')
      : undefined;

  const redirectToMoreWallets = isMobile && isWalletConnectConnector(wallet.id);
  if (redirectToMoreWallets) deeplink = undefined; // mobile redirects to more wallets page

  return (
    <ConnectorButton
      type="button"
      as={deeplink ? 'a' : undefined}
      href={deeplink ? deeplink : undefined}
      disabled={context.route !== routes.CONNECTORS}
      onClick={
        deeplink
          ? undefined
          : () => {
              if (redirectToMoreWallets) {
                context.setRoute(routes.MOBILECONNECTORS);
              } else {
                context.setRoute(routes.CONNECT);
                context.setConnector({ id: wallet.id });
              }
            }
      }
    >

Notice that the logic conditions for deeplink will never be met on a real scenario:

    let deeplink =
    !wallet.isInstalled && isMobile
      ? wallet.getWalletConnectDeeplink?.(uri ?? '')
      : undefined;
     

So a user with the installed library and using the dApp from desktop, will never use the deeplink, necessary to open the connection request in the Wallet.

Changing these conditions to just wallet.isInstalled makes the deep links work properly, but I'm not sure how it affects the rest of the cases that you're handling there.

We can work on a PR to solve this, but would need some additional guidance.


Additional context from the original issue:

#353 (comment)

Special thanks to @MarkNerdi from the Bloom team for helping out and pointing us in the right direction.

@eduardopelitti eduardopelitti added the bug Something isn't working label Apr 30, 2024
@eduardopelitti
Copy link
Author

Is this library still being maintained @lochie?

If you're accepting PRs we can help but not receiving any reply in the past few weeks makes it feels abandoned.

@lochie
Copy link
Member

lochie commented May 20, 2024

hey @eduardopelitti, yes we are accepting PRs! ive just been on some other projects and havent had dedicated time for updates here so i apologise for being MIA

@aschenkel
Copy link
Contributor

aschenkel commented Jun 3, 2024

Hi guys,

as mentioned on @eduardopelitti opening comment, the issue can be found on this line https://github.com/family/connectkit/blob/main/packages/connectkit/src/components/Common/ConnectorList/index.tsx#L91
and it seems like we need a property to identify when a DESKTOP wallet needs to open a deep link.

When isMobile is false, there are cases where we still need to use the deeplink for a small group of desktop wallets, without messing up with browser connectors such as MM.

I couldn't find a prop inside the wallet object that could be used for this.
@lochie are you familiar with an existing property that could be used for this case?

if not, I think adding a new property on the walletConfigs for an app that needs to open a deep link on desktop its needed

@lochie
Copy link
Member

lochie commented Jun 4, 2024

@aschenkel sounds like this could become more complicated if there's a wallet that has both desktop deeplinking and also requiring a QR code option, which at that point they should probably be doing a PR to wagmi to include their own connector.

it does indeed sound like we need a new property here to flag if it should deeplink for desktop. it could be shouldDeeplinkDesktop or something similar.

would you have a list of some of these wallets that require this support? i'd love to test their current UX.

@aschenkel
Copy link
Contributor

aschenkel commented Jun 4, 2024

Right now I think the only desktop wallet that is not working and needs this fix is ledgerLive.

There is also an open PR for supporting Bloom wallet -which hopefully we can merge shortly- that needs it as well.

I've just created a PR that address it here #392

@eduardopelitti
Copy link
Author

I'll mark this issue as closed by #392

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

3 participants