pendulum-chain / vortex

https://app.vortexfinance.co/
1 stars 1 forks source link

Fix wallet connection issues on mobile with appkit #274

Closed ebma closed 1 week ago

ebma commented 1 week ago

Changes

Replaces rainbowkit with appkit.

Closes #258.

Findings

I did some tests and found that using the latest dependencies of wagmi/viem done in #268 in combination with Appkit works quite well. To elaborate, I found that the 'No matching key. History' error did not occur with Appkit ever. Neither for connecting the wallet nor for disconnecting nor when signing a message. Tested with and without an app refresh in between.

It's still not perfect though. I noticed that signing requests don't always trigger the respective app. For example, when I connect to the prototype with WalletConnect and Zerion on my phone, the connection to the account works well. Zerion opens, let's me choose my account, etc. However, when a transaction signature is requested, Zerion never opens automatically. It just seems as if nothing happens to the user. When I open Zerion after the signing is requested (and it seems like nothing has happened) I can see the signing request in Zerion though. For Metamask it's similar but a little different. The wallet connection with Metamask works well bug when it comes to signing a transaction, sometimes I see an intent where I can open Metamask and sometimes I don't. When I don't see the intent to open the app, again, I can manually open Metamask and will see the request for signing.

I found a related bug ticket https://github.com/reown-com/appkit/issues/1504 and it seems like this is not only an AppKit problem but also something related to wagmi. As a side note, I also found another bug ticket https://github.com/reown-com/appkit/issues/2076 mentioning that automatic redirect back to the calling application is not possible on iOS 17. This worked on my Android phone when using Metamask.

Outlook

All in all, I think this is an improvement over the connection with rainbowkit. I tested the current version on polygon-prototype-staging again (which has rainbowkit + the latest dependencies of wagmi/viem) and I still encountered the 'No matching key. History' and other similar errors. This didn't happen to me with Appkit. Thus, I would suggest we try to deal with the shortcomings on Appkit, possibly showing a notification to the user when a signing is requested that if they don't see any action appearing on screen, they should check their connected wallet application manually. It's not a great UX but at least it works technically.

netlify[bot] commented 1 week ago

Deploy Preview for pendulum-pay ready!

Name Link
Latest commit
Latest deploy log https://app.netlify.com/sites/pendulum-pay/deploys/673759cbdde7c499ed81aca7
Deploy Preview https://deploy-preview-274--pendulum-pay.netlify.app
Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

github-actions[bot] commented 1 week ago

yarn.lock changes

Summary

Status Count
ADDED 31
UPDATED 15
DOWNGRADED 5
REMOVED 21
Click to toggle table visibility
| Name | Status | Previous | Current | | :- | :-: | :-: | :-: | | `@emotion/hash` | [REMOVED](#) | 0.9.1 | - | | `@ethersproject/abstract-provider` | [ADDED](#) | - | 5.7.0 | | `@ethersproject/abstract-signer` | [ADDED](#) | - | 5.7.0 | | `@ethersproject/address` | [ADDED](#) | - | 5.7.0 | | `@ethersproject/base64` | [ADDED](#) | - | 5.7.0 | | `@ethersproject/bignumber` | [ADDED](#) | - | 5.7.0 | | `@ethersproject/bytes` | [ADDED](#) | - | 5.7.0 | | `@ethersproject/constants` | [ADDED](#) | - | 5.7.0 | | `@ethersproject/hash` | [ADDED](#) | - | 5.7.0 | | `@ethersproject/keccak256` | [ADDED](#) | - | 5.7.0 | | `@ethersproject/logger` | [ADDED](#) | - | 5.7.0 | | `@ethersproject/networks` | [ADDED](#) | - | 5.7.1 | | `@ethersproject/properties` | [ADDED](#) | - | 5.7.0 | | `@ethersproject/rlp` | [ADDED](#) | - | 5.7.0 | | `@ethersproject/signing-key` | [ADDED](#) | - | 5.7.0 | | `@ethersproject/strings` | [ADDED](#) | - | 5.7.0 | | `@ethersproject/transactions` | [ADDED](#) | - | 5.7.0 | | `@ethersproject/web` | [ADDED](#) | - | 5.7.1 | | `@lit-labs/ssr-dom-shim` | [UPDATED](#) | 1.2.0 | 1.2.1 | | `@lit/reactive-element` | [UPDATED](#) | 1.6.3 | 2.0.4 | | `@rainbow-me/rainbowkit` | [REMOVED](#) | 2.2.0 | - | | `@reown/appkit` | [ADDED](#) | - | 1.3.1 | | `@reown/appkit-adapter-wagmi` | [ADDED](#) | - | 1.3.1 | | `@reown/appkit-common` | [ADDED](#) | - | 1.3.1 | | `@reown/appkit-core` | [ADDED](#) | - | 1.3.1 | | `@reown/appkit-polyfills` | [ADDED](#) | - | 1.3.1 | | `@reown/appkit-scaffold-ui` | [ADDED](#) | - | 1.3.1 | | `@reown/appkit-siwe` | [ADDED](#) | - | 1.3.1 | | `@reown/appkit-ui` | [ADDED](#) | - | 1.3.1 | | `@reown/appkit-utils` | [ADDED](#) | - | 1.3.1 | | `@reown/appkit-wallet` | [ADDED](#) | - | 1.3.1 | | `@safe-global/safe-apps-provider` | [UPDATED](#) | 0.18.3 | 0.18.4 | | `@vanilla-extract/css` | [REMOVED](#) | 1.15.5 | - | | `@vanilla-extract/dynamic` | [REMOVED](#) | 2.1.2 | - | | `@vanilla-extract/private` | [REMOVED](#) | 1.0.6 | - | | `@vanilla-extract/sprinkles` | [REMOVED](#) | 1.6.3 | - | | `@wagmi/connectors` | [UPDATED](#) | 5.3.7 | 5.3.8 | | `@wagmi/core` | [UPDATED](#) | 2.14.4 | 2.14.5 | | `@walletconnect/core` | [UPDATED](#) | 2.17.0 | 2.17.2 | | `@walletconnect/sign-client` | [UPDATED](#) | 2.17.0 | 2.17.2 | | `@walletconnect/types` | [UPDATED](#) | 2.17.0 | 2.17.2 | | `@walletconnect/utils` | [UPDATED](#) | 2.17.0 | 2.17.2 | | `base-x` | [ADDED](#) | - | 5.0.0 | | `bs58` | [ADDED](#) | - | 6.0.0 | | `csstype` | [DOWNGRADED](#) | 3.1.3 | 3.1.2 | | `dayjs` | [ADDED](#) | - | 1.11.10 | | `dedent` | [REMOVED](#) | 1.5.3 | - | | `deep-object-diff` | [REMOVED](#) | 1.1.9 | - | | `deepmerge` | [REMOVED](#) | 4.3.1 | - | | `detect-node-es` | [REMOVED](#) | 1.1.0 | - | | `get-nonce` | [REMOVED](#) | 1.0.1 | - | | `isomorphic-unfetch` | [REMOVED](#) | 3.1.0 | - | | `js-sha3` | [ADDED](#) | - | 0.8.0 | | `lit` | [UPDATED](#) | 2.8.0 | 3.1.0 | | `lit-element` | [UPDATED](#) | 3.3.3 | 4.1.1 | | `lit-html` | [UPDATED](#) | 2.8.0 | 3.2.1 | | `lru-cache` | [DOWNGRADED](#) | 10.4.3 | 10.2.0 | | `media-query-parser` | [REMOVED](#) | 2.0.2 | - | | `modern-ahocorasick` | [REMOVED](#) | 1.0.1 | - | | `ox` | [UPDATED](#) | 0.1.0 | 0.1.2 | | `qrcode` | [DOWNGRADED](#) | 1.5.4 | 1.5.3 | | `react-remove-scroll` | [REMOVED](#) | 2.6.0 | - | | `react-remove-scroll-bar` | [REMOVED](#) | 2.3.6 | - | | `react-style-singleton` | [REMOVED](#) | 2.2.1 | - | | `ua-parser-js` | [REMOVED](#) | 1.0.38 | - | | `uint8arrays` | [DOWNGRADED](#) | 3.1.1 | 3.1.0 | | `unfetch` | [REMOVED](#) | 4.2.0 | - | | `use-callback-ref` | [REMOVED](#) | 1.3.2 | - | | `use-sidecar` | [REMOVED](#) | 1.1.2 | - | | `viem` | [UPDATED](#) | 2.21.43 | 2.21.44 | | `wagmi` | [UPDATED](#) | 2.12.29 | 2.12.30 | | `zod` | [DOWNGRADED](#) | 3.23.8 | 3.22.4 |
gianfra-t commented 1 week ago

Tested this:

Setup: Motorola edge 30 ultra. Chrome + Metamask

First attempt: I connected a wallet and it consistently worked, meaning click on confirm prompt me to move to Metamask and sign.

Forcing a failure: Nevertheless, I then disconnected from my wallet on Vortex, click on connect again, created a new account on Metamask and connected to that one. After this, clicking on confirm was consistently "failing" (not showing the pop up), yet in Metamask I could see the request.

After a few attempts at changing back again to my first wallet (which was failing many times but finally succeeded), I changed back and forth from my first address and my second.

Seems to be that always with my second address, the prompt to Metamask would not show while with the first it would.

ebma commented 1 week ago

Seems to be that always with my second address, the prompt to Metamask would not show while with the first it would.

That's an interesting observation 🤔 Could still be that it's a coincidence. As for me, I always tested with the same account and sometimes the prompt worked, sometimes it did not.

But at least you can confirm that with manually opening the app, signing was always possible.

gianfra-t commented 1 week ago

But at least you can confirm that with manually opening the app, signing was always possible.

yes so far that's the case in this example.

TorstenStueber commented 1 week ago

I tested on iOS Safari.

Generally I can't properly switch between wallets. The main menu to select wallets looks as follows: WhatsApp Image 2024-11-13 at 18 06 52(1)

The "Browser Wallet" entry just shows the following page (the explanation is confusing – I don't have a browser wallet installed, so it is to be expected that none is found here): WhatsApp Image 2024-11-13 at 18 06 53

The "All Wallets" entry leads to a long list of browser, however, 1) it does not contain Phantom and 2) it is not able to determine that I have Rabby wallet installed. This is unexpected as the "Browser Wallet" logo in the main menu indicates that both wallets should be well supported.

Metamask

Trust Wallet

Zerion Wallet

I wanted to test Phantom and Rabby wallet but couldn't for the reasons at the beginning of my comment.

The selected address at the top of the screen overlaps with the logo on mobile:

WhatsApp Image 2024-11-13 at 18 16 46

TorstenStueber commented 1 week ago

In general: I think this works better than rainbowkit, but I would like to test Rabby and Phantom as well, in order to see whether the issues with Zerion are an exception.

We should test the proper flow next in order to see whether switching to the anchor UI causes some extra issues.

ebma commented 1 week ago

@TorstenStueber it's possible to define wallets that need to show up in the list. I already did this for Metamask here as it was also missing in the list for some reason. I'll try to do the same for Rabby and Phantom so you can test with them as well. I'll also remove the custom logic for early signing so it can be tested with the 'normal' flow.

ebma commented 1 week ago

@TorstenStueber you can give this another try. I added the config for rabby and phantom and restored the handler for the 'Confirm' button.

TorstenStueber commented 1 week ago

@ebma unfortunately, neither Rabby nor Phantom work.

If I click on the Phantom button, I get this: WhatsApp Image 2024-11-14 at 10 12 21

If I click on the Rabby button, I get this view, but there is no browser popup to open the app. If I click on "Open", a new tab for the url https://chromewebstore.google.com/unsupported opens (I am not even sure why it opens the Chrome web store, I have been testing with Safari).

WhatsApp Image 2024-11-14 at 10 12 21(1)

TorstenStueber commented 1 week ago

I tested the flow with the normal "Confirm" behavior (including the anchor flow) with Metamask and everything works nicely!

So even if Rabby and Phantom dont' work properly right now, this still looks like a big win for me.

@pendulum-chain/devs did anyone also test on Desktop? Anything not working there?

ebma commented 1 week ago

I think that's because neither Rabby nor Phantom appear as 'WalletConnect certified' on the list here.

image

So the walletconnect support with Appkit seems to be limited to these wallets. Which can be good or bad. Assuming that the 'WalletConnect certified' means that the support is tested, it would ensure that we only show options that are confirmed to work.

TorstenStueber commented 1 week ago

Okay, I think we can live with this. It's a shame that Rabby is not supported as there seem to be a number of people using it but possibly support will be added and then we will support it out of the box.

So shall we go ahead, open this PR for review and then aim to merge?

ebma commented 1 week ago

The code is ready for review.

ebma commented 1 week ago

did anyone also test on Desktop? Anything not working there?

I missed that message before. I tested this on desktop as well and it works well for me. Tested both the walletconnect option connecting to the desktop browser with the phone (using Zerion) and the usual connection with Metamask and transaction signing was requested as expected.

@pendulum-chain/product do you also want to check out the deploy preview? As the underlying library changed, the connector dialog now looks different. Please have a look and let us know if it's good to go from your side.

vadaynujra commented 1 week ago

Tested on desktop Windows + Chrome. Got second signing request. Let's go ahead and merge?

ebma commented 1 week ago

For some reason, the deploy preview failed when building the first time but clearing the cache and retrying a second time worked, see here. So I'll merge this regardless.