feat: add endpoint for registering alby go notifications #128
Conversation
internal/nostr/expo.go
Outdated
| }) | ||
| } | ||
|
|
||
| for _, existingSubscription := range existingSubscriptions { |
There was a problem hiding this comment.
could the DB query above be improved instead of having to do a for loop after (more filtering?)
internal/nostr/nostr.go
Outdated
| go svc.startSubscription(svc.Ctx, &subscription, nil, svc.handleSubscribedEvent) | ||
| handleEvent := svc.handleSubscribedEvent | ||
| if sub.PushToken != "" { | ||
| handleEvent = svc.handleSubscribedExpoNotification |
There was a problem hiding this comment.
Can the naming be improved here? we are still handling a subscribed event, right - we're just notifying in a different way?
There was a problem hiding this comment.
Changed to be simple - handlePushNotification, wdyt?
There was a problem hiding this comment.
I think it's still inconsistent with handleSubscribedEvent
cmd/server/main.go
Outdated
| @@ -50,6 +50,7 @@ func main() { | |||
| e.POST("/nip47", svc.NIP47Handler) | |||
| e.POST("/nip47/webhook", svc.NIP47WebhookHandler) | |||
| e.POST("/nip47/notifications", svc.NIP47NotificationHandler) | |||
| e.POST("/nip47/notifications/go", svc.NIP47ExpoNotificationHandler) | |||
There was a problem hiding this comment.
don't use app specific names here.
You implement an universal API here that can also be used by other applications.
(e.g. /nip47/notifications/expo)
Also try to be consistent.
maybe we move the webhook endpoin then to:
/nip47/notifications/webhook
There was a problem hiding this comment.
Done 👍
Regarding webhook, it's a one-time notification endpoint, and it would also be a breaking change. Should we do that?
To register:
Make a
POSTcall tohttp://localhost:8080/nip47/notifications/gowith body:It'll then start subscribing to all notifications of the app pubkey on the relay and keeps sending notifications to the mobile app