feat: Send push notifications for expired and expiring positions #1297

klochowicz · 2023-09-14T05:42:03Z

Send notification to the user if their position is about to expire.
Add unit tests for the logic determining which notification to send.
periodically load all relevant positions with their fcm_tokens from the DB and
attempt to deliver notifications

coordinator/src/bin/coordinator.rs

holzeis · 2023-09-14T09:21:01Z

coordinator/src/notification_service.rs

+) -> Result<()> {
+    let users = user::all(conn)?;
+
+    let positions_with_fcm_tokens = positions::Position::get_all_positions_with_expiry_after(


🔧 I this could be nicely done with an inner join!

At least we should only load users which have an expired position.

I mentioned that in the comment, but I don't think the optimisation is worthwhile at this stage?
🤔 especially if we're doing it very infrequently? (potentially, if we rework according to your other suggestions, we would only do this during the rollover weekend)

how strongly do you feel about it? I'm happy to rework it if you feel it's worth the time investment to optimise it.

coordinator/src/notification_service.rs

the premise is not true. I am delivering one notification of each.

holzeis · 2023-09-14T18:15:20Z

📣 With that logic we are sending the user every 30 minutes a push notification. That's too much!

have you read the code and seen the unit tests? this is certainly not the case!

Fair enough I miss read the condition, but wouldn't it still send 2 notifications as you are checking for 1 hour after but run it ever 30 minutes? Anyways it's surely not as critical as sending a notification non-stop.

If you had something specific in mind, it would be helpful to put it on the ticket, to avoid impedance mismatch at this stage.

I hadn't something specific in mind it was just a proposal after looking over your PR. However we should align it with the rest of the code. We don't need another task that regularly checks if a position has expired we already have that. See expired_positions.

I would actually probably prefer moving with this forward and adding the additional ideas that you've described. They're not either-or, and they can be done in a follow-up.

I've convert the above into a ticket so we can discuss it freely there.

I am not blocking that PR as the the amount of push notifications are less than assumed when asking for changes, however I don't see a rush in bringing this out in anyway possible, we should take the time to get this to a point where we are all happy with it.

luckysori · 2023-09-15T08:15:51Z

coordinator/src/notification_service.rs

+        if let Some(fcm_token) = maybe_fcm_token {
+            Some((p, fcm_token))
+        } else {
+            tracing::warn!(?p, "No FCM token for position");
+            None
+        }


🔧 This can just be a map (or a find_map altogether).

the reason I unfolded is that I wanted to print out the warning instead of just ignoring the ones that didn't have a FCM token.

if you have a solution that does it even neater with that requirement, please share, as I always am keen on improving my iterator-fu 🙏☺️

Fair enough. I think you could have done that within the find_map, but this code has diverged so I can't really give it a go myself anymore!

coordinator/src/notification_service.rs

luckysori · 2023-09-15T08:25:43Z

coordinator/src/notification_service.rs

+        if position.expiry_timestamp <= now
+            && now < position.expiry_timestamp + time::Duration::hours(1)


❓ Can we use a Range to define this predicate? It should be more readable if possible.

cool idea! I'll look into it!

there's no ranges support for OffsetDateTime unfortunately :(

klochowicz · 2023-09-16T00:34:09Z

📣 With that logic we are sending the user every 30 minutes a push notification. That's too much!

have you read the code and seen the unit tests? this is certainly not the case!

Fair enough I miss read the condition, but wouldn't it still send 2 notifications as you are checking for 1 hour after but run it ever 30 minutes? Anyways it's surely not as critical as sending a notification non-stop.

Yes, you're correct - it would do it twice. I wanted to avoid the situation when e.g. people get it 1 min before the expiry, and can't react.

If you had something specific in mind, it would be helpful to put it on the ticket, to avoid impedance mismatch at this stage.

I hadn't something specific in mind it was just a proposal after looking over your PR. However we should align it with the rest of the code. We don't need another task that regularly checks if a position has expired we already have that. See expired_positions.

This one only checks within particular time window, and I prefer that for separating concerns and concentrating logic of when to send notifications in one place. An infrequent read from a DB is not that costly, readability and testability is preferred IMHO.

I would actually probably prefer moving with this forward and adding the additional ideas that you've described. They're not either-or, and they can be done in a follow-up.
I've convert the above into a ticket so we can discuss it freely there.

I am not blocking that PR as the the amount of push notifications are less than assumed when asking for changes, however I don't see a rush in bringing this out in anyway possible, we should take the time to get this to a point where we are all happy with it.

isn't this the default situation with every review we have that doesn't solve a critical bug in production? 🤓

FYI: The only drawback is that #1211, which I'm currently working on, is depending on it.

bonomat

Looks good.

coordinator/src/bin/coordinator.rs

bonomat · 2023-09-15T06:40:07Z

coordinator/src/notification_service.rs

+        if position.expiry_timestamp <= now
+            && now < position.expiry_timestamp + time::Duration::hours(1)


Is my understanding correct that the user will get a notification that his position has expired up to 1h after the position has expired?

yes, I tried to limit the amount of notifications to not overwhelm the user (cutoff time arbitrarily chosen, it's now with the constant).
the notification itself does not expire on the phone, unless user willingly dismisses it.

bonomat · 2023-09-15T06:41:05Z

coordinator/src/notification_service.rs

+        } else if position.expiry_timestamp > now
+            && position.expiry_timestamp <= now + time::Duration::hours(1)


Is my understanding correct that the user receives this notification and has 1h to come online before his position is expires?

yes, that's the limitation currently (perhaps a 1h window of delivery e.g. ~12h before it expires would be better!), ie. at the start at the "rollover window".

coordinator/src/db/positions.rs

coordinator/src/notifications.rs

klochowicz · 2023-09-19T23:27:15Z

@holzeis I've rebased on your recent work, trying to improve the naming somewhat, so I'd like to hear your opinion on this. I think my naming doesn't cut it perhaps either - so I was thinking maybe feed or user_feed would be best? these are messages that people get when they're connected, in contrast to the notifications that arrive at any time, even if they've have their position closed.
thoughts?

Also, I've moved the window to [13h,12h] before expiry, and adjusted polling interval to be just under 1h (to minimise the doubling of the notifications).

holzeis

@holzeis I've rebased on your recent work, trying to improve the naming somewhat, so I'd like to hear your opinion on this. I think my naming doesn't cut it perhaps either - so I was thinking maybe feed or user_feed would be best? these are messages that people get when they're connected, in contrast to the notifications that arrive at any time, even if they've have their position closed. thoughts?

Also, I've moved the window to [13h,12h] before expiry, and adjusted polling interval to be just under 1h (to minimise the doubling of the notifications).

I think it already looks good. I am not a the best in naming things either, but I guess we can improve that over time.

What I would have liked to see is a more general purpose notification service, that is managing the user data in a set as we are currently doing it with the authenticated_users. Upon startup, we do anyways always send the fcm token information, so instead of having a REST API we could send it over with the websocket authentication message, store on signin and keep it in memory with our authenticated users. That way the notification service could as well cache that information (However, I do not think that is a huge bottleneck anyways - It's just a thought)

I've left some additional remarks, but they are not blocking! We can also address them in a follow up PR.

holzeis · 2023-09-20T07:57:39Z

coordinator/src/bin/coordinator.rs

@@ -214,7 +219,7 @@ async fn main() -> Result<()> {
        notifier.clone(),
        network,
    );
-    let _handle = rollover::monitor(pool.clone(), tx_user_feed.clone(), notifier, network);
+    let _handle = rollover::monitor(pool.clone(), tx_user_feed.clone(), notifier);


🔧 I guess that commit will not compile without the network passed as argument here.

coordinator/src/bin/coordinator.rs

holzeis · 2023-09-20T08:01:06Z

coordinator/src/message.rs

-pub enum Notification {
-    Message {
+/// Message sent to users via the websocket.
+pub enum CoordinatorMessage {


🔧 CoordinatorMessage is not correct here as this also includes messages from the orderbook. How about WebsocketNotification? Inititially, I though that we could as well include the push notification types into this Notifications as well, and include something like a Type e.g. (OnlyIfOnline, Always, OnlyPush) and then the notification service could first try on the websocket and if failed leave a push notification.

I feel like the websocket messages and the push notifcations have somewhat of a relationship, as I might want to inform the user about a missed websocket message.

I'll change it to WebsocketMessage, thank you fro the suggestion.

I'm not sure whether it would make sense to couple them together (in my mind push notifications are completely separate thing, that user can opt-in into; and also needs FCM key to work locally).

edit: there was already WebsocketMessage somewhere else, so I ended up with OrderbookMessage (these are after all messages delivered via the orderbook websocket stream).

coordinator/src/notifications.rs

coordinator/src/orderbook/trading.rs

Push notifications are sent in the following time windows: 1. Expiring positions - [EXPIRING_THRESHOLD, now] 2. Expired positions: - [now, EXPIRED_THRESHOLD] Here both of the thresholds are set to 1h. - Add unit tests for the logic determining which notification to send. - periodically load all relevant positions with their fcm_tokens from the DB and attempt to deliver notifications Notification task runs every 30 mins, so that the user would get 2 of each. This way they'll likely get it at least 30 mins before the expiry.

When the other party is not online, send a notification that a position has expired.

…xpiry Create a window [START_OF_EXPIRING_POSITION, END_OF_EXPIRING_POSITION] to better configure the notification about imminent position expiry. Configure the window to [13h,12h] before expiry.

Improve documentation and adjust how often we run the check to minimise the changes of user receiving 2 notifications (it can still happen, but it is around 3% now (overlapping the time a bit to ensure we don't out miss sending out the notification).

klochowicz requested review from luckysori, holzeis and bonomat September 14, 2023 05:58

klochowicz self-assigned this Sep 14, 2023

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

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

Closed

holzeis requested changes Sep 14, 2023

View reviewed changes

holzeis self-requested a review September 14, 2023 09:42

This comment was marked as resolved.

Sign in to view

This comment has been minimized.

Sign in to view

klochowicz mentioned this pull request Sep 14, 2023

Send push notifications about the rollover weekend and force-closure of DLC channel #1300

Closed

luckysori reviewed Sep 15, 2023

View reviewed changes

klochowicz force-pushed the feat/send-a-notification-when-the-order-is-about-to-expire branch from 21662ee to a07531e Compare September 16, 2023 00:46

klochowicz requested review from luckysori and holzeis September 16, 2023 00:52

klochowicz force-pushed the feat/send-a-notification-when-the-order-is-about-to-expire branch from a07531e to 33a7016 Compare September 18, 2023 22:35

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

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

Closed

bonomat approved these changes Sep 19, 2023

View reviewed changes

klochowicz force-pushed the feat/send-a-notification-when-the-order-is-about-to-expire branch 2 times, most recently from 772c827 to a771df2 Compare September 19, 2023 23:23

klochowicz force-pushed the feat/send-a-notification-when-the-order-is-about-to-expire branch from a771df2 to 83786a0 Compare September 20, 2023 02:04

holzeis approved these changes Sep 20, 2023

View reviewed changes

klochowicz added 4 commits September 20, 2023 14:51

feat: Send a notification prompt when executing AsyncTrade

bcab978

When the other party is not online, send a notification that a position has expired.

chore: Notify about the soon expiring positions at least 12h before e…

c694d36

…xpiry Create a window [START_OF_EXPIRING_POSITION, END_OF_EXPIRING_POSITION] to better configure the notification about imminent position expiry. Configure the window to [13h,12h] before expiry.

klochowicz force-pushed the feat/send-a-notification-when-the-order-is-about-to-expire branch from 83786a0 to 8f887fc Compare September 20, 2023 21:52

klochowicz enabled auto-merge September 20, 2023 21:54

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

Merged via the queue into main with commit 033ff5e Sep 20, 2023
7 checks passed

klochowicz deleted the feat/send-a-notification-when-the-order-is-about-to-expire branch September 20, 2023 22:23

klochowicz mentioned this pull request Sep 27, 2023

Notify user on a position that will expire soon using FCM #1070

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Send push notifications for expired and expiring positions #1297

feat: Send push notifications for expired and expiring positions #1297

klochowicz commented Sep 14, 2023

holzeis Sep 14, 2023

klochowicz Sep 16, 2023 •

edited

Loading

This comment was marked as resolved.

This comment has been minimized.

holzeis commented Sep 14, 2023

luckysori Sep 15, 2023

klochowicz Sep 15, 2023

luckysori Sep 21, 2023

luckysori Sep 15, 2023

klochowicz Sep 15, 2023

klochowicz Sep 16, 2023

klochowicz commented Sep 16, 2023

bonomat left a comment

bonomat Sep 15, 2023

klochowicz Sep 19, 2023

bonomat Sep 15, 2023

klochowicz Sep 19, 2023

klochowicz commented Sep 19, 2023

holzeis left a comment •

edited

Loading

holzeis Sep 20, 2023 •

edited

Loading

holzeis Sep 20, 2023 •

edited

Loading

klochowicz Sep 20, 2023

klochowicz Sep 20, 2023 •

edited

Loading

		if position.expiry_timestamp <= now
		&& now < position.expiry_timestamp + time::Duration::hours(1)

		} else if position.expiry_timestamp > now
		&& position.expiry_timestamp <= now + time::Duration::hours(1)

feat: Send push notifications for expired and expiring positions #1297

feat: Send push notifications for expired and expiring positions #1297

Conversation

klochowicz commented Sep 14, 2023

Choose a reason for hiding this comment

klochowicz Sep 16, 2023 • edited Loading

Choose a reason for hiding this comment

This comment was marked as resolved.

This comment has been minimized.

holzeis commented Sep 14, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

klochowicz commented Sep 16, 2023

bonomat left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

klochowicz commented Sep 19, 2023

holzeis left a comment • edited Loading

Choose a reason for hiding this comment

holzeis Sep 20, 2023 • edited Loading

Choose a reason for hiding this comment

holzeis Sep 20, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

klochowicz Sep 20, 2023 • edited Loading

Choose a reason for hiding this comment

klochowicz Sep 16, 2023 •

edited

Loading

holzeis left a comment •

edited

Loading

holzeis Sep 20, 2023 •

edited

Loading

holzeis Sep 20, 2023 •

edited

Loading

klochowicz Sep 20, 2023 •

edited

Loading