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

feat_: dapp connected notification #5616

Draft
wants to merge 3 commits into
base: develop
Choose a base branch
from

Conversation

yqrashawn
Copy link
Contributor

@yqrashawn yqrashawn commented Jul 29, 2024

create a new activity center notification when new wallet connect session added

mobile PR status-im/status-mobile#20905
mobile ISSUE status-im/status-mobile#20467

Important changes:

  • add a InjectedMessenger in wallet service to avoid import cycle
  • inject messenger into wallet service
  • create and persist new notification at the end of AddWalletConnectSession
  • add wallet connect session topic, dapp name, URL, icon URL to activity_center_notifications table

@status-im-auto
Copy link
Member

status-im-auto commented Jul 29, 2024

Jenkins Builds

Click to see older builds (30)
Commit #️⃣ Finished (UTC) Duration Platform Result
✖️ 9bc67a0 #1 2024-07-29 07:23:24 ~1 min tests 📄log
✔️ 9bc67a0 #1 2024-07-29 07:24:45 ~2 min tests-rpc 📄log
✔️ 9bc67a0 #1 2024-07-29 07:25:54 ~3 min linux 📦zip
✔️ 9bc67a0 #1 2024-07-29 07:26:49 ~4 min ios 📦zip
✔️ 9bc67a0 #1 2024-07-29 07:27:36 ~5 min android 📦aar
✔️ c9d0277 #2 2024-07-29 07:29:29 ~1 min android 📦aar
✔️ c9d0277 #2 2024-07-29 07:29:29 ~2 min tests-rpc 📄log
✔️ c9d0277 #2 2024-07-29 07:30:08 ~3 min ios 📦zip
✔️ c9d0277 #2 2024-07-29 07:30:50 ~3 min linux 📦zip
✖️ c9d0277 #2 2024-07-29 08:05:16 ~38 min tests 📄log
✔️ f794844 #3 2024-07-29 08:18:09 ~2 min tests-rpc 📄log
✔️ f794844 #3 2024-07-29 08:18:45 ~3 min ios 📦zip
✔️ f794844 #3 2024-07-29 08:19:42 ~4 min linux 📦zip
✔️ f794844 #3 2024-07-29 08:21:06 ~5 min android 📦aar
✔️ f794844 #3 2024-07-29 09:00:32 ~44 min tests 📄log
✔️ f406cb5 #4 2024-07-29 11:49:16 ~2 min tests-rpc 📄log
✔️ f406cb5 #4 2024-07-29 11:49:29 ~2 min linux 📦zip
✔️ f406cb5 #4 2024-07-29 11:52:33 ~5 min android 📦aar
✔️ f406cb5 #4 2024-07-29 11:52:34 ~5 min ios 📦zip
✔️ f406cb5 #4 2024-07-29 12:33:38 ~46 min tests 📄log
✔️ 177688f #5 2024-07-31 03:02:51 ~2 min tests-rpc 📄log
✔️ 177688f #5 2024-07-31 03:03:03 ~2 min linux 📦zip
✔️ 177688f #5 2024-07-31 03:03:04 ~2 min android 📦aar
✔️ 177688f #5 2024-07-31 03:03:46 ~3 min ios 📦zip
✔️ 177688f #5 2024-07-31 03:44:10 ~43 min tests 📄log
✔️ d608df7 #6 2024-08-28 15:15:51 ~4 min tests-rpc 📄log
✔️ d608df7 #6 2024-08-28 15:15:59 ~4 min linux 📦zip
✔️ d608df7 #6 2024-08-28 15:17:46 ~6 min android 📦aar
✔️ d608df7 #6 2024-08-28 15:22:58 ~11 min ios 📦zip
✔️ d608df7 #6 2024-08-28 15:47:41 ~35 min tests 📄log
Commit #️⃣ Finished (UTC) Duration Platform Result
✖️ 708ddec #7 2024-09-03 16:23:11 ~1 min tests 📄log
✔️ 708ddec #7 2024-09-03 16:24:22 ~2 min tests-rpc 📄log
✔️ 708ddec #7 2024-09-03 16:25:47 ~4 min linux 📦zip
✔️ 708ddec #7 2024-09-03 16:27:06 ~5 min android 📦aar
✔️ 708ddec #7 2024-09-03 16:28:10 ~6 min ios 📦zip
✔️ 24f56c7 #8 2024-09-03 16:26:56 ~2 min tests-rpc 📄log
✔️ 24f56c7 #8 2024-09-03 16:28:21 ~2 min linux 📦zip
✔️ 24f56c7 #8 2024-09-03 16:32:22 ~5 min android 📦aar
✔️ 24f56c7 #8 2024-09-03 16:34:55 ~5 min ios 📦zip
✔️ 24f56c7 #8 2024-09-03 16:56:38 ~32 min tests 📄log

@yqrashawn yqrashawn force-pushed the feat/wallet-connect-session-notification branch from 9bc67a0 to c9d0277 Compare July 29, 2024 07:26
@yqrashawn yqrashawn self-assigned this Jul 29, 2024
@yqrashawn yqrashawn force-pushed the feat/wallet-connect-session-notification branch 2 times, most recently from f794844 to f406cb5 Compare July 29, 2024 11:46
@yqrashawn yqrashawn marked this pull request as ready for review July 29, 2024 11:48
Copy link
Contributor

@qfrank qfrank left a comment

Choose a reason for hiding this comment

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

LGTM, left minor comment

protocol/activity_center_persistence.go Outdated Show resolved Hide resolved
@yqrashawn yqrashawn force-pushed the feat/wallet-connect-session-notification branch from f406cb5 to 177688f Compare July 31, 2024 03:00
@yqrashawn
Copy link
Contributor Author

Will request manual QA on the mobile side after 2.30 release

@yqrashawn yqrashawn removed their assignment Aug 6, 2024
@yqrashawn
Copy link
Contributor Author

}

_, err := m.persistence.SaveActivityCenterNotification(notification, true)

Copy link
Collaborator

Choose a reason for hiding this comment

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

To notify client here, you'd do something like this:

if m.config.messengerSignalsHandler != nil {
    response := &MessengerResponse{}
    response.AddActivityCenterNotification(notification)
    m.config.messengerSignalsHandler.MessengerResponse(response)
}

Copy link
Collaborator

Choose a reason for hiding this comment

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

Though combine it with my next comment.

Copy link
Contributor

Choose a reason for hiding this comment

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

thanks, i added and now works fine.

protocol/messenger_walletconnect.go Outdated Show resolved Hide resolved
services/wallet/api.go Show resolved Hide resolved
@mohsen-ghafouri mohsen-ghafouri force-pushed the feat/wallet-connect-session-notification branch from d608df7 to 708ddec Compare September 3, 2024 16:21
Copy link
Collaborator

@igor-sirotin igor-sirotin left a comment

Choose a reason for hiding this comment

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

Sorry for blocking.
We're discussing now with the @status-im/status-go-guild how to properly pass the Mesenger to Wallet in such cases

@mohsen-ghafouri
Copy link
Contributor

Hey @igor-sirotin, please let me know about final decision once ready and if i should apply any changes here, thanks

@dlipicar
Copy link
Contributor

dlipicar commented Sep 6, 2024

hey @mohsen-ghafouri !
we've got a draft with some guidelines for solving dependency between modules. For this particular case I feel the best solution is to use a pub-sub mechanism: https://www.notion.so/Proposal-Inter-module-dependency-01698e14a2f4432ab3080e3c37ad3177?pvs=4#1a187cb9021949698490811ad0e75f2b

Logic:

  • The dapps module is in charge of managing dapp sessions. Other modules might want to react whenever a session state changes (added, removed), so it's good to emit some sort of event when that happens.
  • Instead of injecting messenger into the dapps module, make it simply subscribe to that event and build the notification.
    That way we prevent adding tighter coupling between wallet and the messenger, since dapps doesn't need to know who'll consume that event.

cc @status-im/status-go-guild

@igor-sirotin
Copy link
Collaborator

@mohsen-ghafouri what's the state on this one? Please let me know if you need any help with the subscriber pattern!

@mohsen-ghafouri
Copy link
Contributor

Hey @igor-sirotin, sorry i was working on other priority task, will be in touch once i started working on this. thanks

@mohsen-ghafouri mohsen-ghafouri marked this pull request as draft September 19, 2024 06:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants