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

[#21853] fix: check deeplink pending request after wallet connect loaded #21873

Open
wants to merge 2 commits into
base: develop
Choose a base branch
from

Conversation

mohsen-ghafouri
Copy link
Contributor

@mohsen-ghafouri mohsen-ghafouri commented Dec 27, 2024

fixes #21853

Summary

  • "cannot read property" error is displayed on profiles list page
  • Handle pending dapp pair request after wallet connect loaded successfully

Areas that maybe impacted

  • Dapps interactions

Test Note

Regarding the issue discussed in PR #21833, here’s an explanation of how dApp requests are handled and the current limitations:

Handling dApp Requests

  1. Event Listener

    • The app registers an event listener to handle incoming dApp requests.
    • However, this listener is only registered after the wallet screen loads. If a request is sent before the listener is active, it will not be processed.
  2. Deep Link

    • Currently, pairing requests from a dApp can be handled through deep links, where the dApp sends a URI containing the pairing information to the app.
    • However, sign requests are not handled through deep links. This means any signing request (e.g., transactions or message signing) that is initiated via deep link will not work if user is not logged in already.
    • If the app is launched through a deep link for a pairing request, the process works as expected, as shown in the video below.
  3. Push Notification (Not Yet Implemented)

    • Push notifications are intended to handle scenarios where the app is closed, and the request is sent to a different device.
    • The issue in PR #21833 highlights this limitation. Currently, push notifications are not supported for any type of request.

for PR #21833 we can have a separate issue so i can discuss it with @clauxx and check for any solution.

Steps to test

  1. Connect to the Dapp(in my case Uniswap) using universal scanner
  2. Log out
  3. Make any transaction Using Dapp (approve or swap)

Result

Simulator.Screen.Recording.-.iPhone.13.-.2024-12-27.at.17.11.05.mp4

status: ready

@mohsen-ghafouri mohsen-ghafouri self-assigned this Dec 27, 2024
@status-im-auto
Copy link
Member

status-im-auto commented Dec 27, 2024

Jenkins Builds

Commit #️⃣ Finished (UTC) Duration Platform Result
✔️ f5ae111 #1 2024-12-27 13:00:17 ~4 min tests 📄log
✔️ f5ae111 #1 2024-12-27 13:02:48 ~6 min android-e2e 🤖apk 📲
✔️ f5ae111 #1 2024-12-27 13:03:19 ~7 min ios 📱ipa 📲
✔️ f5ae111 #1 2024-12-27 13:03:23 ~7 min android 🤖apk 📲
✔️ 1c22eb2 #2 2025-01-03 09:20:10 ~5 min tests 📄log
✔️ 1c22eb2 #2 2025-01-03 09:22:03 ~6 min ios 📱ipa 📲
✔️ 1c22eb2 #2 2025-01-03 09:23:47 ~8 min android-e2e 🤖apk 📲
✔️ 1c22eb2 #2 2025-01-03 09:24:35 ~9 min android 🤖apk 📲

@mohsen-ghafouri mohsen-ghafouri marked this pull request as ready for review December 27, 2024 14:17
@mohsen-ghafouri mohsen-ghafouri requested review from alwx, smohamedjavid and vkjr and removed request for alwx December 27, 2024 14:20
Comment on lines 18 to 25
(if web3-wallet
{:db (dissoc db :wallet-connect/waiting-pair-url)
:fx [[:effects.wallet-connect/pair
{:web3-wallet web3-wallet
:url url
:on-fail #(log/error "Failed to pair with dApp" {:error %})
:on-success #(log/info "dApp paired successfully")}]]}
{:db (assoc db :wallet-connect/waiting-pair-url url)}))))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's a bit difficult to understand why we're assoc/dissoc-ing the waiting-pair-url here you know the context already.

I'd suggest to isolate the usage of waiting-pair-url inside 2 events so its purpose is clearer (see an attempt below). The process-deeplink event would be dispatched by the universal-links handler (status-im.common.universal-links), while the pair-with-pending-deeplink would be dispatched in :wallet-connect/on-init-success. I assume the other changes (in :wallet-connect/send-response and :wallet-connect/pair), as well as the delay from the deeplink handler, wouldn't be necessary anymore, but let me know if that's not the case.

Also renamed it to :wallet-connect/pending-url as "pending" seems more fitting than "waiting", but that's just a nitpick.

(rf/reg-event-fx
 :wallet-connect/process-deeplink
 (fn [{:keys [db]} [url]]
   (let [web3-wallet (get db :wallet-connect/web3-wallet)]
     (if web3-wallet
       {:fx [[:dispatch [:wallet-connect/on-scan-connection url]]]}
       {:db (assoc db :wallet-connect/pending-url url)}))))

(rf/reg-event-fx
 :wallet-connect/pair-with-pending-deeplink
 (fn [{:keys [db]}]
   (when-let [pending-url (get db :wallet-connect/pending-url)]
     {:db (dissoc db :wallet-connect/pending-url)
      :fx [[:dispatch [:wallet-connect/on-scan-connection pending-url]]]})))

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Make sense will apply changes.

:on-success (fn []
(rf/dispatch [:wallet-connect/redirect-to-dapp])
(log/info "Successfully sent Wallet Connect response to dApp"))}]]}))))
(when web3-wallet
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why is this needed? When would we send a response without the web3-wallet already initialized?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the only reason for this issue #21853 (comment) when i was checking it was this event triggered before web3-wallet have initiated

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

might be that it's no longer needed, since we're not supposed to process requests without logging in i.e. without web3-wallet being initialized.

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

Successfully merging this pull request may close these issues.

"cannot read property" error is displayed on profiles list page when the Dapp attempts to send a signature
3 participants