feat: add multi pay invoice method #218

im-adithya · 2024-01-16T13:46:49Z

Adds multi_pay_invoice method

rolznz · 2024-01-17T04:14:40Z

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?

service.go

handle_multi_pay_invoice_request.go

rolznz · 2024-01-17T14:44:20Z

handle_multi_pay_invoice_request.go

+					}).Errorf("Failed to process event: %v", err)
+					return
+				}
+				dTag := []string{"a", fmt.Sprintf("%d:%s:%d", NIP_47_RESPONSE_KIND, event.PubKey, invoiceInfo.Id)}


I don't understand this d tag, what is the value? it doesn't seem to match the spec.

Also you construct it in 4 different places, can't you just do it once?

Yeah that's also why createAndAppendResponse existed.

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

But I thought you weren't happy with it

I don't understand this d tag, what is the value? it doesn't seem to match the spec.

I thought d tag here meant this, am I wrong?

I thought d tag here meant this, am I wrong?

I think it should be a simple tag like ["d", "ID_OR_PAYMENT_HASH HERE"],

Yeah that's also why createAndAppendResponse existed.

I mean, can't you create a variable to store the d tag at the start of the loop and then use it wherever you need it, rather than declare it 4 times? just to keep it DRY (or have a function that does it, but createAndAppendResponse did something different, right? we do not append a response any more)

Yeah that's also why createAndAppendResponse existed.

Sorry ^one of the reasons why

But changed with the new commit now (although could only dry up dTag declarations)

I think it should be a simple tag like ["d", "ID_OR_PAYMENT_HASH HERE"],

Should we confirm with Ben?

It's a d tag not an a tag. I think you got them mixed up. Can you check the spec again?

I also use d tags in the way I mentioned in the PoS app. I will fix this.

models.go

handle_multi_pay_invoice_request.go

rolznz · 2024-01-17T14:57:52Z

handle_multi_pay_invoice_request.go

+			}
+			payment.Preimage = &preimage
+			// TODO: What to do here?
+			nostrEvent.State = NOSTR_EVENT_STATE_HANDLER_EXECUTED


Hmm, we need to review these. They do not apply well to the multi methods.

I would probably set it to executed if at least one payment succeeds, otherwise error. What do you think?

I would probably set it to executed if at least one payment succeeds, otherwise error.

One thought I had was to set a counter to see how many were successful and update this according to that

frnandu · 2024-01-17T18:05:20Z

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 · 2024-01-22T10:41:27Z

handle_multi_pay_invoice_request.go

+			}
+			dTag := []string{"a", fmt.Sprintf("%d:%s:%s", NIP_47_RESPONSE_KIND, event.PubKey, id)}
+
+			hasPermission, code, message := svc.hasPermission(&app, event, request.Method, paymentRequest.MSatoshi)


using request.Method does not work here. We use the NIP_47_PAY_INVOICE_METHOD permission for any payment request

rolznz · 2024-01-22T10:42:39Z

handle_multi_pay_invoice_request.go

+					}).Errorf("Failed to process event: %v", err)
+					return
+				}
+				resp.Tags = append(resp.Tags, dTag)


You cannot change the response after it has been signed. Setting tags like this does not work.

I am fixing it.

- fix permission check - do not change tags after decrypting

rolznz · 2024-01-22T12:15:20Z

service.go

@@ -145,11 +80,65 @@ func (svc *Service) StartSubscription(ctx context.Context, sub *nostr.Subscripti
 	}
 }

-func (svc *Service) HandleEvent(ctx context.Context, event *nostr.Event) (result *nostr.Event, err error) {


All the tests still reference this method. Can you update them?

I am looking at this

handle_multi_pay_invoice_request.go

alby.go

…t method handling

im-adithya · 2024-01-23T06:47:45Z

service.go

-				Code:    NIP_47_ERROR_NOT_IMPLEMENTED,
-				Message: fmt.Sprintf("Unknown method: %s", nip47Request.Method),
-			}}, nostr.Tags{}, ss)
+		resp, err = svc.handleUnknownMethod(ctx, nip47Request, event, app, ss)


rolznz · 2024-01-23T07:17:47Z

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 · 2024-01-23T07:18:41Z

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

#230

CC @im-adithya @bumi

rolznz · 2024-01-25T04:02:03Z

service_test.go

-// TODO: split up into individual tests
-func TestHandleEvent(t *testing.T) {
+func TestBackwardsCompatibility(t *testing.T) {
+	// Test without adding a single permission


can this be done in this PR?

Actually, this is going to be cleaned up in the self-custodial version. I added a TODO in the Wails PR. Nice!

Should I just remove this completely then?

Sounds good!

service_test.go

rolznz · 2024-01-25T04:08:46Z

@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 · 2024-01-25T04:12:03Z

service_test.go

+	}
+
+	reqEvent.ID = "test_get_balance_without_permission"
+	res, err := svc.HandleGetBalanceEvent(ctx, request, reqEvent, *app, ss)


I don't think the individual handle_x events should deal with encrypting the response. Then we can test encryption/decryption separately in one place. But maybe this could be added as a TODO?

Yes, we can just return the Nip47Response, that would also DRY it up and we can avoid sending ss for no reason to those handlers

rolznz · 2024-01-25T04:13:00Z

service_test.go

 	assert.NoError(t, err)
-	assert.Equal(t, received.Error.Code, NIP_47_ERROR_UNAUTHORIZED)


why was this assert removed?

we do not have a test for this any more, it would be good to make sure using an unknown pubkey returns unauthorized

We might not be able to do that because that part is handled by HandleEvent which doesn't return anything

I think we can extract the app finding logic to a new method

rolznz · 2024-01-30T13:11:10Z

service.go

+		}).Error(result.Error)
+		return
+	}
+	nostrEvent.ReplyId = resp.ID


Can we address #231 in this PR?

rolznz · 2024-02-02T04:45:56Z

Moved to getAlby#3

feat: add multi pay invoice method

5962c6b

chore: publish in multi handler

ddbe70d

rolznz reviewed Jan 17, 2024

View reviewed changes

service.go Outdated Show resolved Hide resolved

rolznz reviewed Jan 17, 2024

View reviewed changes

service.go Show resolved Hide resolved

rolznz reviewed Jan 17, 2024

View reviewed changes

handle_multi_pay_invoice_request.go Outdated Show resolved Hide resolved

rolznz reviewed Jan 17, 2024

View reviewed changes

models.go Outdated Show resolved Hide resolved

rolznz reviewed Jan 17, 2024

View reviewed changes

handle_multi_pay_invoice_request.go Outdated Show resolved Hide resolved

rolznz reviewed Jan 17, 2024

View reviewed changes

handle_multi_pay_invoice_request.go Outdated Show resolved Hide resolved

rolznz reviewed Jan 17, 2024

View reviewed changes

chore: refactoring

09ae1fb

im-adithya force-pushed the task-multi-pay branch from 902eb20 to 09ae1fb Compare January 18, 2024 06:59

chore: DRY up dtag usage

b1a43fe

rolznz reviewed Jan 22, 2024

View reviewed changes

fix: issues with multi_pay_invoice

3fad688

- fix permission check - do not change tags after decrypting

rolznz reviewed Jan 22, 2024

View reviewed changes

handle_multi_pay_invoice_request.go Show resolved Hide resolved

chore: use sync mutex for concurrency support

dad7b90

rolznz reviewed Jan 22, 2024

View reviewed changes

alby.go Outdated Show resolved Hide resolved

rolznz added 2 commits January 23, 2024 11:40

fix: add one second to oauth token expiry check

451f7d6

chore: split handleUnknownMethod function, add TODO about inconsisten…

1e3d720

…t method handling

im-adithya commented Jan 23, 2024

View reviewed changes

im-adithya mentioned this pull request Jan 24, 2024

Add a replies table to store reply events #231

Closed

chore: fix tests

3e8f41c

rolznz reviewed Jan 25, 2024

View reviewed changes

service_test.go Show resolved Hide resolved

rolznz reviewed Jan 25, 2024

View reviewed changes

im-adithya added 2 commits January 25, 2024 12:44

chore: add todos

560b226

Merge branch 'feat/wails-v2' into task-multi-pay

4b8f608

im-adithya changed the base branch from main to feat/wails-v2 January 25, 2024 07:34

im-adithya added 2 commits January 25, 2024 13:25

fix: same event id in tests

b86ff49

chore: remove test for backwards compatibility

01786f8

rolznz reviewed Jan 30, 2024

View reviewed changes

service.go

}).Error(result.Error)

return

}

nostrEvent.ReplyId = resp.ID

Copy link

Contributor

rolznz Jan 30, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we address #231 in this PR?

rolznz mentioned this pull request Feb 2, 2024

Feat: multi_pay_invoice and multi_pay_keysend methods getAlby/nostr-wallet-connect-next#3

Merged

9 tasks

rolznz closed this Feb 2, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add multi pay invoice method #218

feat: add multi pay invoice method #218

im-adithya commented Jan 16, 2024

rolznz commented Jan 17, 2024 •

edited

Loading

rolznz Jan 17, 2024

im-adithya Jan 18, 2024

im-adithya Jan 18, 2024

rolznz Jan 18, 2024 •

edited

Loading

im-adithya Jan 18, 2024

im-adithya Jan 18, 2024

rolznz Jan 22, 2024

rolznz Jan 22, 2024

rolznz Jan 17, 2024

rolznz Jan 17, 2024

im-adithya Jan 18, 2024

frnandu commented Jan 17, 2024

rolznz Jan 22, 2024

rolznz Jan 22, 2024

rolznz Jan 22, 2024

rolznz Jan 23, 2024

im-adithya Jan 23, 2024

rolznz commented Jan 23, 2024 •

edited

Loading

rolznz commented Jan 23, 2024

rolznz Jan 25, 2024

rolznz Jan 25, 2024

im-adithya Jan 25, 2024

rolznz Jan 25, 2024

rolznz commented Jan 25, 2024

rolznz Jan 25, 2024

im-adithya Jan 25, 2024

rolznz Jan 25, 2024

rolznz Jan 25, 2024

im-adithya Jan 25, 2024

rolznz Jan 25, 2024

rolznz Jan 30, 2024

rolznz commented Feb 2, 2024

		assert.NoError(t, err)
		assert.Equal(t, received.Error.Code, NIP_47_ERROR_UNAUTHORIZED)

feat: add multi pay invoice method #218

feat: add multi pay invoice method #218

Conversation

im-adithya commented Jan 16, 2024

rolznz commented Jan 17, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rolznz Jan 18, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

frnandu commented Jan 17, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rolznz commented Jan 23, 2024 • edited Loading

Tests

rolznz commented Jan 23, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rolznz commented Jan 25, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rolznz commented Feb 2, 2024

rolznz commented Jan 17, 2024 •

edited

Loading

rolznz Jan 18, 2024 •

edited

Loading

rolznz commented Jan 23, 2024 •

edited

Loading