Fix WalletConnect sessions disappearing #21350

clauxx · 2024-09-30T13:49:31Z

Summary

The sessions were disappearing due to a bug in the session fetching logic, which was unnecessarily convoluted with multiple events. Here, the bug was fixed and the session logic was simplified into functions, which minimized the number of events we use.

Changes:

isolated and moved session fetching and persistance sync logic into one function
isolated and moved session disconnection logic
isolated and moved session approval logic
the bug should be fixed as a result of these changes
added event tests for session

Testing notes

Since there are changes in session management, we should test the following flows:

fetching connected dapps
approval flow
disconnect dapp flow

status: ready

…sappearing status-im/status-go@effde33...effde33

status-im-auto · 2024-09-30T13:56:05Z

Jenkins Builds

Click to see older builds (8)

❔	Commit	#️⃣	Finished (UTC)	Duration	Platform	Result
❌	`2b22627`	#2	2024-09-30 13:56:00	~3 min	`tests`	📄`log`
✔️	`2b22627`	#2	2024-09-30 13:59:52	~6 min	`android`	🤖`apk` 📲
✔️	`2b22627`	#2	2024-09-30 14:00:21	~7 min	`android-e2e`	🤖`apk` 📲
✔️	`2b22627`	#2	2024-09-30 14:02:03	~9 min	`ios`	📱`ipa` 📲

✔️	`c625a43`	#3	2024-09-30 14:08:52	~4 min	`tests`	📄`log`
✔️	`c625a43`	#3	2024-09-30 14:12:30	~8 min	`android-e2e`	🤖`apk` 📲
✔️	`c625a43`	#3	2024-09-30 14:12:45	~8 min	`ios`	📱`ipa` 📲
✔️	`c625a43`	#3	2024-09-30 14:12:57	~9 min	`android`	🤖`apk` 📲

❔	Commit	#️⃣	Finished (UTC)	Duration	Platform	Result
✔️	`29944f5`	#4	2024-10-01 10:11:55	~5 min	`tests`	📄`log`
✔️	`29944f5`	#4	2024-10-01 10:13:58	~7 min	`android-e2e`	🤖`apk` 📲
✔️	`29944f5`	#4	2024-10-01 10:14:28	~7 min	`android`	🤖`apk` 📲
✔️	`29944f5`	#4	2024-10-01 10:16:26	~9 min	`ios`	📱`ipa` 📲

✔️	`c4ce1ac`	#6	2024-10-02 12:06:34	~4 min	`tests`	📄`log`
✔️	`c4ce1ac`	#6	2024-10-02 12:09:33	~7 min	`android`	🤖`apk` 📲
✔️	`c4ce1ac`	#6	2024-10-02 12:09:50	~7 min	`android-e2e`	🤖`apk` 📲
✔️	`c4ce1ac`	#6	2024-10-02 12:10:37	~8 min	`ios`	📱`ipa` 📲

seanstrom

Looking good, left a couple of comments 📓

My main overall question is to understand a little bit more about how wallet connect events/effects are working now. For example, how do the changes resolve the issue, I think it's because of the way we structuring the effects to avoid some kind of inconsistent state, but I'm not sure.

Really like that we're defining re-usable effects too ♻️

seanstrom · 2024-10-01T10:41:52Z

src/status_im/contexts/wallet/wallet_connect/events/session_proposals.cljs

               [:effects.wallet-connect/approve-session
-                {:web3-wallet web3-wallet
-                 :proposal    current-proposal
-                 :networks    session-networks
-                 :accounts    accounts
-                 :on-success  (fn [approved-session]
-                                (log/info "Wallet Connect session approved")
-                                (rf/dispatch [:wallet-connect/reset-current-session-proposal])
-                                (rf/dispatch [:wallet-connect/persist-session
-                                              approved-session]))
-                 :on-fail     (fn [error]
-                                (log/error "Wallet Connect session approval failed"
-                                           {:error error
-                                            :event :wallet-connect/approve-session})
-                                (rf/dispatch
-                                 [:wallet-connect/reset-current-session-proposal]))}])
+                {:web3-wallet      web3-wallet
+                 :proposal-request current-proposal
+                 :session-networks session-networks
+                 :address          current-address
+                 :on-success       #(rf/dispatch [:wallet-connect/approve-session-success %])
+                 :on-fail          #(rf/dispatch [:wallet-connect/approve-session-error %])}])


Can we use rf/call-continuation inside the definition for :effects.wallet-connect/approve-session?

If we do that then we can define :on-success and :on-failure with vectors:

[:effects.wallet-connect/approve-session {:web3-wallet web3-wallet :proposal-request current-proposal :session-networks session-networks :address current-address :on-success [:wallet-connect/approve-session-success] :on-fail [:wallet-connect/approve-session-error]}]

For reference, here's an example of how the effect would use rf/call-continuation to work promesa: https://github.com/status-im/status-mobile/blob/seanstrom/chore-refactor-screens-definitions/src/status_im/contexts/wallet/effects.cljs#L63-L68

seanstrom · 2024-10-01T10:43:02Z

src/status_im/contexts/wallet/wallet_connect/events/sessions.cljs

+     {:fx [[:effects.wallet-connect/get-sessions
+            {:online?     (-> db :network/status (= :online))
+             :web3-wallet (get db :wallet-connect/web3-wallet)
+             :addresses   addresses
+             :on-success  #(rf/dispatch [:wallet-connect/get-sessions-success %])
+             :on-error    #(rf/dispatch [:wallet-connect/get-sessions-error %])}]]})))


Similar suggestion about using rf/call-continuation for this effect too 🙏

pavloburykh · 2024-10-01T12:26:24Z

@clauxx thank you for the PR. Unfortunately I am still facing issue on IOS.

ISSUE 1 'Failed to get dapps connection' error and empty dapp list on relogin (IOS)

Reproducing on iPhone 15, IOS 17.6.1
Steps:

Connect to dapp
Perform relogin
Observe the error
Open dapps list
Check if connected dapp is displayed

Actual result: 'Failed to get dapps connection' error. Dapps list is empty.

logs (48).zip

clauxx · 2024-10-01T12:32:07Z

src/status_im/contexts/wallet/wallet_connect/events/sessions.cljs

+ :wallet-connect/delete-session
 (fn [{:keys [db]} [topic]]
-   (log/info "Removing session from persistance and state" topic)
   {:db (update db
                :wallet-connect/sessions
                (fn [sessions]
                  (->> sessions
                       (remove #(= (:topic %) topic))
-                       (into []))))
-    :fx [[:json-rpc/call
-          [{:method     "wallet_disconnectWalletConnectSession"
-            :params     [topic]
-            :on-success #(log/info "Wallet Connect session disconnected")
-            :on-error   #(log/info "Wallet Connect session persistence failed" %)}]]]}))
+                       (into []))))}))


@seanstrom so here is the cause of the bug. This event was called when disconnecting a dapp and when syncing persisted dapps. We shouldn't be removing active sessions from the db when syncing the persisted dapps. The faulty "sync" was likely caused by the expiry date check, but hard to tell the cause since it's difficult to reproduce.

clauxx · 2024-10-02T12:07:07Z

@pavloburykh could you try again please? I think I found the root cause for this issue, being that the wallet was finalizing its initialization and loading the accounts after we attempt to fetch the sessions, leading to the empty list, but it's hard to tell since it's very hard to reproduce for me.

EDIT: if possible, even if the bug is fixed, could you please attach the logs?

pavloburykh · 2024-10-02T13:03:29Z

@pavloburykh could you try again please? I think I found the root cause for this issue, being that the wallet was finalizing its initialization and loading the accounts after we attempt to fetch the sessions, leading to the empty list, but it's hard to tell since it's very hard to reproduce for me.

EDIT: if possible, even if the bug is fixed, could you please attach the logs?

Thanks @clauxx, ISSUE 1 is fixed now. I will proceed with the rest of testing.

Here are the logs from my IOS
logs (49).zip

status-im-auto · 2024-10-02T14:19:55Z

100% of end-end tests have passed

Total executed tests: 7
Failed tests: 0
Expected to fail tests: 0
Passed tests: 7

Passed tests (7)

Click to expand

Class TestOneToOneChatMultipleSharedDevicesNewUi:

1. test_1_1_chat_non_latin_messages_stack_update_profile_photo, id: 702745
Device sessions Device 1: Steps, video, logs Device 2: Steps, video, logs

Class TestWalletOneDevice:

1. test_wallet_add_remove_regular_account, id: 727231
Device sessions Device 1: Steps, video, logs

Class TestCommunityOneDeviceMerged:

1. test_community_copy_and_paste_message_in_chat_input, id: 702742
Device sessions Device 1: Steps, video, logs
2. test_restore_multiaccount_with_waku_backup_remove_switch, id: 703133
Device sessions Device 1: Steps, video, logs

Class TestWalletMultipleDevice:

1. test_wallet_send_asset_from_drawer, id: 727230

2. test_wallet_send_eth, id: 727229

Class TestCommunityMultipleDeviceMerged:

1. test_community_message_edit, id: 702843
Device sessions Device 1: Steps, video, logs Device 2: Steps, video, logs

pavloburykh · 2024-10-02T14:29:47Z

@clauxx thank you again for the fixes. No issues from my side. PR is ready for merge.

clauxx added 6 commits September 27, 2024 12:13

feat: moved disconnect logic to function

f8abc17

ref: moved rpc calls to rpc ns

dde0865

ref: moved session approval logic to function

abc0ca4

fix: small fixes for sessions

35b341a

test: wallet-connect/on-session-delete event test

4b6dae3

test: added event tests for wc sessions

ca2ab1f

clauxx requested review from ilmotta, alwx and shivekkhurana September 30, 2024 13:49

clauxx self-assigned this Sep 30, 2024

Merge remote-tracking branch 'origin/develop' into cl-fix-sessions-di…

2b22627

…sappearing status-im/status-go@effde33...effde33

fix: require sessions events ns

c625a43

clauxx requested review from briansztamfater and seanstrom and removed request for ilmotta October 1, 2024 10:06

Merge branch 'develop' into cl-fix-sessions-disappearing

29944f5

seanstrom reviewed Oct 1, 2024

View reviewed changes

alwx approved these changes Oct 1, 2024

View reviewed changes

pavloburykh self-assigned this Oct 1, 2024

pavloburykh added the Tested - Issues label Oct 1, 2024

clauxx commented Oct 1, 2024

View reviewed changes

pavloburykh mentioned this pull request Oct 1, 2024

Dapps disappear from connected list after relogin (IOS) #21275

Closed

clauxx added 2 commits October 2, 2024 14:58

fix: the wallet was loaded after wc sometimes

aa150e1

Merge branch 'develop' into cl-fix-sessions-disappearing

c4ce1ac

pavloburykh added request-manual-qa Tested - OK and removed Tested - Issues labels Oct 2, 2024

clauxx merged commit d748ccd into develop Oct 2, 2024
6 checks passed

clauxx deleted the cl-fix-sessions-disappearing branch October 2, 2024 15:16

pavloburykh mentioned this pull request Oct 15, 2024

[#21335] feat: add dApp analytics #21379

Merged

pavloburykh mentioned this pull request Oct 30, 2024

Failed to get dapps connection' error and empty dapp list on relogin #21530

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix WalletConnect sessions disappearing #21350

Fix WalletConnect sessions disappearing #21350

clauxx commented Sep 30, 2024

status-im-auto commented Sep 30, 2024 •

edited

Loading

seanstrom left a comment

seanstrom Oct 1, 2024

seanstrom Oct 1, 2024

pavloburykh commented Oct 1, 2024

clauxx Oct 1, 2024

clauxx commented Oct 2, 2024 •

edited

Loading

pavloburykh commented Oct 2, 2024 •

edited

Loading

status-im-auto commented Oct 2, 2024

Class TestOneToOneChatMultipleSharedDevicesNewUi:

Class TestWalletOneDevice:

Class TestCommunityOneDeviceMerged:

Class TestWalletMultipleDevice:

Class TestCommunityMultipleDeviceMerged:

pavloburykh commented Oct 2, 2024

Fix WalletConnect sessions disappearing #21350

Fix WalletConnect sessions disappearing #21350

Conversation

clauxx commented Sep 30, 2024

Summary

Testing notes

status-im-auto commented Sep 30, 2024 • edited Loading

Jenkins Builds

seanstrom left a comment

Choose a reason for hiding this comment

seanstrom Oct 1, 2024

Choose a reason for hiding this comment

seanstrom Oct 1, 2024

Choose a reason for hiding this comment

pavloburykh commented Oct 1, 2024

ISSUE 1 'Failed to get dapps connection' error and empty dapp list on relogin (IOS)

clauxx Oct 1, 2024

Choose a reason for hiding this comment

clauxx commented Oct 2, 2024 • edited Loading

pavloburykh commented Oct 2, 2024 • edited Loading

status-im-auto commented Oct 2, 2024

100% of end-end tests have passed

Passed tests (7)

Class TestOneToOneChatMultipleSharedDevicesNewUi:

Class TestWalletOneDevice:

Class TestCommunityOneDeviceMerged:

Class TestWalletMultipleDevice:

Class TestCommunityMultipleDeviceMerged:

pavloburykh commented Oct 2, 2024

status-im-auto commented Sep 30, 2024 •

edited

Loading

clauxx commented Oct 2, 2024 •

edited

Loading

pavloburykh commented Oct 2, 2024 •

edited

Loading