-
Notifications
You must be signed in to change notification settings - Fork 17
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: Firebase Push Notifications Support #45
base: main
Are you sure you want to change the base?
feat: Firebase Push Notifications Support #45
Conversation
Signed-off-by: al-rosenthal <[email protected]>
Signed-off-by: al-rosenthal <[email protected]>
Signed-off-by: al-rosenthal <[email protected]>
Signed-off-by: al-rosenthal <[email protected]>
Signed-off-by: al-rosenthal <[email protected]>
Signed-off-by: al-rosenthal <[email protected]>
Signed-off-by: al-rosenthal <[email protected]>
credo needed to emit a new event for allowing push notifications Signed-off-by: al-rosenthal <[email protected]>
Signed-off-by: al-rosenthal <[email protected]>
Signed-off-by: al-rosenthal <[email protected]>
Signed-off-by: al-rosenthal <[email protected]>
Signed-off-by: al-rosenthal <[email protected]>
Signed-off-by: al-rosenthal <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for this! I think the custom patches and events are not needed and we should look at ways to work around them.
We can discuss it tomorrow at the Credo WG call in more detail
patches/@credo-ts__core.patch
Outdated
diff --git a/build/modules/routing/RoutingEvents.d.ts b/build/modules/routing/RoutingEvents.d.ts | ||
index 578a26fca35f07bea4ed211b20c077e24e90d36c..0268c9cb1bc58d046dc46ae7a83b911a21dc39a0 100644 | ||
--- a/build/modules/routing/RoutingEvents.d.ts | ||
+++ b/build/modules/routing/RoutingEvents.d.ts |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We shouldn't rely on patches to add features. Especially not now this is a OWF project.
Can we make sure the needed changes are made to Credo instead of patching? However we've not had the need for these patches, so i'm not sure if this is needed. We have an AgentMessageProcessedEvent, and probably better is to use the queue to know whether a message was processed.
@genaris any thoughts on how to approach this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can directly add the logic of sending the push notifications here https://github.com/openwallet-foundation/didcomm-mediator-credo/blob/main/src/storage/StorageMessageQueue.ts#L96
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In general I don't like the approach of adding this custom messageType
to Forward messages: it's not part of DIDComm spec, it needs custom sender agents to work, and of course (more importantly) it gives the mediator more information that it needs to know about recipient messages. I understand the reasons behind it (send Push notifications only when we receive meaningful messages), but it will be hard to convince me to add this into Credo's standard DIDComm module 😃.
I do think there are more appropriate ways of handling push notifications, although they require more work on the client side and they don't work as good as they would in a native mobile app. That being said, I think we can still find a way to allow this behaviour in Credo DIDComm module, probably by allowing middlewares for outbound messages in such a way we let users to modify/add fields to outgoing messages before they are sent to the transport.
In regards to the specific work in this PR, I agree that this behaviour could be achieved by hooking into AgentMessageProcessedEvent
. This event will be triggered after processForwardMessage finishes, so it should work without the need of creating a new event.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for looking at this guys! I unfortunately wasn't able to make it to the meeting this morning but I'm looking forward to catching up with the video once it's uploaded.
In my testing, AgentMessageProcessedEvent
never has the connection ID that I need to fetch the device token for the push notification, the connection object in the payload is always null. I might be trying to hook into it incorrectly, here is my example code:
agent.events.on(AgentEventTypes.AgentMessageProcessed, async (event: AgentMessageProcessedEvent) => {
agent.config.logger.info(`PN: Agent Message Processed: ${event.payload.connection?.id ?? 'no connetion id'}`)
// send push notification
})
the event is triggering but it is consistently logging 'no connection id'. I tried this with messages, credential offers and proof requests, but no connection id is present.
I'm happy to remove the patch, I do like the idea of adding it to the StorageMessageQueue
as @sairanjit suggests, what do you guys think?
src/push-notifications/fcm/services/PushNotificationsFcmService.ts
Outdated
Show resolved
Hide resolved
…e.ts Co-authored-by: Timo Glastra <[email protected]> Signed-off-by: Al Rosenthal <[email protected]>
Signed-off-by: al-rosenthal <[email protected]>
Signed-off-by: al-rosenthal <[email protected]>
Signed-off-by: al-rosenthal <[email protected]>
Signed-off-by: al-rosenthal <[email protected]>
Signed-off-by: al-rosenthal <[email protected]>
Signed-off-by: al-rosenthal <[email protected]>
Signed-off-by: al-rosenthal <[email protected]>
Signed-off-by: al-rosenthal <[email protected]>
What's Changed
.env.example
updated with new firebase fieldsStorageMessageQueue.ts