NWC: add support for payment notifications

[ThomasFarstrike]

This is a first pull request for payment notifications.

Here's an example of the usage:

nwc->subscribeNotifications(
        [](nostr::NotificationResponse res) {
            nostr::Nip47Notification notification = res.notification;
            Serial.println("Notification Type: " + notification.notificationType);
            Serial.println("Type: " + notification.type);
            Serial.println("Amount: " + NostrString_fromUInt(notification.amount));
            Serial.println("Payment Hash: " + notification.paymentHash);
            Serial.println("Description: " + notification.description);
            Serial.println("Description Hash: " + notification.descriptionHash);
            Serial.println("Invoice: " + notification.invoice);
            Serial.println("Preimage: " + notification.preimage);
            Serial.println("Fees Paid: " + NostrString_fromUInt(notification.feesPaid));
            Serial.println("Created At: " + NostrString_fromUInt(notification.createdAt));
            Serial.println("Settled At: " + NostrString_fromUInt(notification.settledAt));
        },
        [](NostrString errorCode, NostrString errorMessage) {
            Serial.println("Notification Error: " + errorCode + " - " + errorMessage);
        }
    );

It might be good to extend the examples to show a bit more, like listTransactions and subscribeNotifications, in a separate pull request, or in the same one.

[ThomasFarstrike]

OK, I'm pretty happy with how this turned out. Ready to merge, as far as I'm concerned!

[ThomasFarstrike]

I'll to submit a dedicated Arduino example, that is picked up and listed under "Examples" by the Arduino IDE, in a separate pull request. It will include things like getBalance, listTransactions and subscribeNotifications.

[ThomasFarstrike]

Done.

It's already built into the latest Lightning Piggy release so I hope you'll be able to merge this @riccardobl - no pressure! ;-)

[maxbrito500]

Fantastic work!

[riccardobl]

As always, thank you for the contribution.
Sorry if it's taking a bit longer to review this, I'm in between projects.

From an initial review, it looks good! But wouldn’t it be better to make a dedicated method for subscribing to notifications instead of reusing sendEvent?

[ThomasFarstrike]

Sorry if it's taking a bit longer to review this, I'm in between projects.

No problem. I tried to keep it as small and low diff as possible, but I know it's much more than just a one-liner :-)

From an initial review, it looks good! But wouldn’t it be better to make a dedicated method for subscribing to notifications instead of reusing sendEvent?

That's actually what I did originally! But then those 2 functions looked so similar that I expected you to ask the opposite "wouldn't it be better to make one generic method?" so I made it into one.

I think it's really a very close call, hard to say which is best. I guess it depends a bit on how the code evolves in the future too.

I would propose to merge it with the one method, as it is now, because this works very well and has gotten quite a bit of real world testing. And then to see what the future brings. If in the future they diverge more, or it starts to get more complex, or you just don't like the look of it, then they can easily be duplicated into 2 separate functions. But perhaps in the future it doesn't need changing because it just works (tm) or for some reason it simplifies/converges further, so then it can stay as is, with the benefit of a smaller codebase, less maintenance, no copy-paste etc...

[riccardobl]

Sorry for the late reply, i had time to come back to this only now.

I've considered what you are saying, but even so, all the branching to check if eventToSend is nullptr imo is adding complexity and making the code less maintainable.

Also this might be a pain for devs trying to debug errors in their code, since "sendEvent" will silently turn into "subscribeToNotifications" if the event is nullptr, that is not expected by someone who doesn't know the code.

Imo the cleaner approach would be to handle the subscription inside NWC::subscribeNotifications instead of calling sendEvent. "Code duplication" in the sense of calling subscribeMany in different place, is fine, since that's how the api is designed.

[ThomasFarstrike]

Ok that makes sense, I'll refactor it a bit...

[ThomasFarstrike]

Done! Please have a look when you can :-)

[Copilot]

Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.

[riccardobl]

looks good, thank you