-
Notifications
You must be signed in to change notification settings - Fork 60
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
Allow optional signature when registering notifications #1797
Conversation
// 1. Delete previous subscriptions (and by cascade, notification types) | ||
// to avoid duplicates, and clearing "unknown" subscriptions | ||
await sql` | ||
DELETE FROM notification_subscriptions | ||
WHERE chain_id = ${safe.chainId} | ||
AND safe_address = ${safe.address} | ||
AND ( | ||
signer_address = ${args.signerAddress ?? null} | ||
OR ( | ||
signer_address IS NULL | ||
) | ||
) | ||
AND push_notification_device_id = ${device.id} | ||
`; |
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.
The subscription is still device based so this shouldn't cause issues of "unsubscriptions".
Pull Request Test Coverage Report for Build 10452769483Details
💛 - Coveralls |
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.
👏🏻
return this.notificationsDatasource.deleteSubscription({ | ||
deviceUuid: args.deviceUuid, | ||
chainId: args.chainId, | ||
safeAddress: args.safeAddress, | ||
}); |
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.
Another small nit, but this could remain as this.notificationsDatasource.deleteSubscription(args)
, couldn't it?
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.
Changed in da9bf55.
Summary
It should be possible to register for "public" notifications, e.g. incoming/outgoing assets, without a signing key.
This adapts the
POST
/v2/register/notifications
endpoint to make theaccessToken
optional and, if not present, register for notifications as a non-owner/delegate with nosigner_address
saved. When notifications are dispatched, if nosigner_address
is present, only "public" notifications are therefore dispatch to the related Firebase token.Changes
notification_subscriptions['signer_address']
column nullable