feat: add nip47 notifications endpoint #60
Conversation
| } | ||
|
|
||
| subscription := Subscription{ | ||
| RelayUrl: requestData.RelayUrl, |
There was a problem hiding this comment.
is this optional and we default to our relay?
There was a problem hiding this comment.
Yup it defaults to cfg.DefaultRelayURL ("wss://relay.getalby.com/v1") when not set
README.md
Outdated
| > | relayUrl | optional | string | If no relay is provided, it uses the default relay | | ||
| > | webhookUrl | required | string | Webhook URL to publish events | | ||
| > | walletPubkey | optional | string | Public key of the wallet service | | ||
| > | pubkey | optional | string | Public key of the user | |
There was a problem hiding this comment.
I think these should not be optional, right?
There was a problem hiding this comment.
😄 I didn't know what to do here, it's actually either-or for pubkey and wallet pubkey. Even without specifying both we can still listen to response kind events on the relay, but yeah ideally we should provide both.
There was a problem hiding this comment.
However if you don't (especially if you do not provide the pubkey), http-nostr will send extra events which cannot be decrypted with the subscriber's private key and will probably cause issues with the subscribing application. I think it actually makes sense to require them both to ensure the app is subscribing to what it thinks it's subscribing.
There was a problem hiding this comment.
you miss that if you build a wallet then you specify only the walletPubkey.
what can be subscribed should ultimately be up for some authentication. e.g. proof you are the wallet if you only subscribe to the walletPubkey
and proof you are the pubkey if you add the pubkey.
There was a problem hiding this comment.
but this either/or should be reflected in the docs. simply marking them both as optional is wrong.
There was a problem hiding this comment.
Ah, I see. I thought this was for the new NIP-47 notifications which are always sent in one direction, not for standard NIP-47 request/responses
There was a problem hiding this comment.
@bumi if this is for notifications then I would say all are required, or if not, should notifications be a separate API endpoint so that the requirements and responses are clearly separated from this one?
There was a problem hiding this comment.
@rolznz what do you mean?
for me this API was either for apps that want to subscribe to relevant notifications or for wallets.
but you're right, lets make it only for apps and we require a
wallet pubkey and a connection pubkey.
There was a problem hiding this comment.
Cool, so now we require both connection pubkey and wallet pubkey, and we keep listening to all the notification events the user is receiving from the NWC wallet provider
That means:
- This is not related to request / response events
- This can also be implemented with the normal subscription handler, but this stands as a convenient way to subscribe to notifs
internal/nostr/nostr.go
Outdated
| WebhookUrl: requestData.WebhookUrl, | ||
| Open: true, | ||
| Authors: &[]string{requestData.WalletPubkey}, | ||
| Kinds: &[]int{23195}, |
There was a problem hiding this comment.
this is for nip-47 notifications, right? it should be 23196
There was a problem hiding this comment.
This is for NIP-47 response events, so this should be good Changed to listen to NIP-47 notifications ✅
im-adithya
changed the title
Fixes #54