Add plumbing for sending push notifications #1227

klochowicz · 2023-09-06T04:23:21Z

store the FCM tokens sent from the app in the coordinator
add a notification service that sends push notifications to users (provide a sender as a means to send messages from different parts of the system).

Next step would be actually using this new NotificationService to send notifications.

bonomat

A few nits but overall I think this goes into the right direction.

coordinator/Cargo.toml

coordinator/src/notification_service.rs

bonomat · 2023-09-06T08:26:15Z

mobile/lib/main.dart

@@ -63,12 +63,22 @@ import 'features/stable/stable_value_change_notifier.dart';
 final GlobalKey<NavigatorState> _rootNavigatorKey = GlobalKey<NavigatorState>(debugLabel: 'root');
 final GlobalKey<NavigatorState> _shellNavigatorKey = GlobalKey<NavigatorState>(debugLabel: 'shell');

-void main() {
+void main() async {


Does this work? My limited flutter knowledge says you'd need

Suggested change

void main() async {

Future<void> main() async {

Also, can the main be async?

yes, it works and all the examples in the official Dart documentation show it like this:

https://dart.dev/codelabs/async-await#working-with-futures-async-and-await

mobile/pubspec.lock

coordinator/src/db/user.rs

mobile/lib/main.dart

holzeis

It would be nice if we could combine the notifications logic for the websocket with the push notifications into a dedicated module.

coordinator/src/routes.rs

mobile/lib/main.dart

mobile/native/src/api.rs

klochowicz · 2023-09-13T05:44:24Z

It would be nice if we could combine the notifications logic for the websocket with the push notifications into a dedicated module.

I'm afraid I don't follow you. would you mind to elaborate?

holzeis · 2023-09-13T08:14:03Z

It would be nice if we could combine the notifications logic for the websocket with the push notifications into a dedicated module.

I'm afraid I don't follow you. would you mind to elaborate?

No need to action in this PR, but we currently have an api for notifying the user using the websocket and I guess now we get a second for notifying via push notification. It would be nice if we could have a single api to notify the user, e.g. with a combination try first websocket if not possible notify on push notification.

holzeis

Let's give it a go if it works 👍

holzeis · 2023-09-13T08:16:36Z

coordinator/src/notification_service.rs

+    fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
+        match self {
+            NotificationKind::ChannelClose => write!(f, "ChannelClose"),
+            NotificationKind::PositionSoonToExpire => write!(f, "PositionSoonToExpire"),


nit: maybe that's the rollover notification?

I thought about it for a while, but the it's before rollover, and the user can decide whether to rollover or not.
I wonder whether we should also allow notifications after a successful rollover etc?

bonomat

LGTM

klochowicz · 2023-09-13T19:07:01Z

It would be nice if we could combine the notifications logic for the websocket with the push notifications into a dedicated module.

I'm afraid I don't follow you. would you mind to elaborate?

No need to action in this PR, but we currently have an api for notifying the user using the websocket and I guess now we get a second for notifying via push notification. It would be nice if we could have a single api to notify the user, e.g. with a combination try first websocket if not possible notify on push notification.

Oh, I see. I'm not sure whether this can be achieved, because they're constraints are different.

Websocket stream is current info to online & authenticated users.
Push notifications is information for users that are can be offline.

It seems that it's easy to draw the line between the content of this information, e.g. there's no reason to send the same info on both channels.
I imagine that the push notification subsystem can be achieved by having their own tokio tasks that monitor for things, blissfully unaware that there is such a thing as websocket stream from the orderbook (and therefore having loose coupling).

Another thing that IMHO makes them logically separate is that the websocket stream comes from orderbook, whereas the push notifications are sent from the coordinator (our LSP).

It seems to be not always triggered.

also: - Add FCM package, - Ensure that firebase code is always run

Sometimes the app would panic at startup (for local deployments for me), and I figured it was trivial to make it more resilient in this case. We now log an error if for whatever reason we couldn't display the node id at startup (e.g. node wasn't ready) - this is not a critical bug, and it gets logged at later stage anyway.

We need to store FCM tokens to be able to notify the users about important events, e.g. when LSP wants to close a channel, or when their position with the coordinator is about to expire.

Move it into a separate module instead of polluting main.dart

These functions didn't really fit into `order` module.

klochowicz requested review from luckysori and holzeis September 6, 2023 04:23

klochowicz self-assigned this Sep 6, 2023

klochowicz linked an issue Sep 6, 2023 that may be closed by this pull request

Store user's FCM tokens, correlated with pubkeys #1174

Closed

This was referenced Sep 6, 2023

Send the notifications to the users when a position is about to expire (e.g. in 1h) and when it expired. #1212

Closed

Send a push notification to the app when LSP wants to collab close the expired position #1211

Closed

bonomat reviewed Sep 6, 2023

View reviewed changes

holzeis reviewed Sep 10, 2023

View reviewed changes

coordinator/src/routes.rs Show resolved Hide resolved

mobile/lib/main.dart Outdated Show resolved Hide resolved

mobile/native/src/api.rs Outdated Show resolved Hide resolved

klochowicz force-pushed the feat/store-fcm-tokens-in-coordinator branch 4 times, most recently from 4f4f527 to 3723463 Compare September 13, 2023 06:57

holzeis approved these changes Sep 13, 2023

View reviewed changes

holzeis reviewed Sep 13, 2023

View reviewed changes

bonomat approved these changes Sep 13, 2023

View reviewed changes

klochowicz added 4 commits September 13, 2023 13:12

chore: Add log message about Firebase during startup

6518acc

It seems to be not always triggered.

feat: Add notification service using FCM package

4602695

also: - Add FCM package, - Ensure that firebase code is always run

chore(coordinator): Add fcm_token field into users db

60b2f15

We need to store FCM tokens to be able to notify the users about important events, e.g. when LSP wants to close a channel, or when their position with the coordinator is about to expire.

klochowicz force-pushed the feat/store-fcm-tokens-in-coordinator branch from 3723463 to 60c9ee4 Compare September 13, 2023 21:25

klochowicz added 3 commits September 13, 2023 14:26

feat: Store the FCM token in the coordinator

d1d3780

chore: Extract all Dart notification-related code

9b83015

Move it into a separate module instead of polluting main.dart

chore: Extract non-order related functions into users module

aaceaee

These functions didn't really fit into `order` module.

klochowicz force-pushed the feat/store-fcm-tokens-in-coordinator branch from 60c9ee4 to aaceaee Compare September 13, 2023 21:27

klochowicz enabled auto-merge September 13, 2023 21:51

klochowicz added this pull request to the merge queue Sep 13, 2023

Merged via the queue into main with commit e9827cd Sep 13, 2023
7 checks passed

klochowicz deleted the feat/store-fcm-tokens-in-coordinator branch September 13, 2023 22:26

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add plumbing for sending push notifications #1227

Add plumbing for sending push notifications #1227

klochowicz commented Sep 6, 2023

bonomat left a comment

bonomat Sep 6, 2023

bonomat Sep 6, 2023

klochowicz Sep 13, 2023

holzeis left a comment

klochowicz commented Sep 13, 2023

holzeis commented Sep 13, 2023

holzeis left a comment

holzeis Sep 13, 2023

klochowicz Sep 13, 2023

bonomat left a comment

klochowicz commented Sep 13, 2023

Add plumbing for sending push notifications #1227

Add plumbing for sending push notifications #1227

Conversation

klochowicz commented Sep 6, 2023

bonomat left a comment

Choose a reason for hiding this comment

bonomat Sep 6, 2023

Choose a reason for hiding this comment

bonomat Sep 6, 2023

Choose a reason for hiding this comment

klochowicz Sep 13, 2023

Choose a reason for hiding this comment

holzeis left a comment

Choose a reason for hiding this comment

klochowicz commented Sep 13, 2023

holzeis commented Sep 13, 2023

holzeis left a comment

Choose a reason for hiding this comment

holzeis Sep 13, 2023

Choose a reason for hiding this comment

klochowicz Sep 13, 2023

Choose a reason for hiding this comment

bonomat left a comment

Choose a reason for hiding this comment

klochowicz commented Sep 13, 2023