im-adithya commented 7 months ago

Adds multi_pay_invoice method

rolznz commented 7 months ago

Hmm, I am not sure this is the best way to do it because of the introduced conditional below and now we have to handle resp and resps, and now there is a lot of array handling and returning an array of resps which we should try to avoid because it forces us to wait for all responses to come back before sending them.

if resp != nil {
        svc.publishEvent(ctx, sub, event, resp)
    }
    if resps != nil {
        ...
    }

Also HandleMultiPayInvoiceEvent has it's own waitgroup so that function will not return until all payments have been sent, which means responses won't be sent as soon as they are available.

Maybe we can move svc.publishEvent to the individual handlers (and maybe create a new handler for HandleNotImplemented for the default case in the switch), then the multi_x ones can call svc.publishEvent once per element in the array.

Then we should also remove createAndAppendResponse and not deal with returning an array of responses.

Does that make sense? do you see any issues doing it this way?

frnandu commented 7 months ago

After this is ready for merge, we should also make the changes (cherry pick maybe?) on https://github.com/getAlby/nwc.getalby.com so that Alby backend for NWC also has this methods implemented.

rolznz commented 7 months ago

Tests

No test was written yet for the new multi_pay_invoice method
The tests are all broken due to the changes in this PR
I think the tests are hard to work with right now because they always expect a finished nostr event back which needs to be decoded each time. This is incompatible with multi_* methods.
The tests are more complicated because they always need to decrypt the event to check the content. I think it is better to test encryption/decryption once and not deal with it in the other tests, and split up the event handling so the individual functionality can be tested (plus all tests in one file like this is very hard to maintain)

But this will take some time to fix. Do you guys have any thoughts / opinions here? @im-adithya @bumi

rolznz commented 7 months ago

PR to address some of the TODOs / issues in this PR:

https://github.com/getAlby/nostr-wallet-connect/pull/230

CC @im-adithya @bumi

rolznz commented 7 months ago

@im-adithya can you change the merge branch to feat/wails-v2 and merge feat/wails-v2 into this branch?

We decided we will not add the multi_* support to nwc.getalby.com for now.

rolznz commented 7 months ago

Moved to https://github.com/getAlby/nostr-wallet-connect-next/pull/3

getAlby / nostr-wallet-connect

feat: add multi pay invoice method #218

Tests