-
Notifications
You must be signed in to change notification settings - Fork 2.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
invoices/invoiceregistry: generate random payaddr for keysend #4343
invoices/invoiceregistry: generate random payaddr for keysend #4343
Conversation
6b2d498
to
bee90cf
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can confirm that keysend works again. Only thing I noticed is that we don't show payment_addr
on ListInvoices
. Would be useful to add that even just for testing this PR.
invoices/invoiceregistry_test.go
Outdated
// Replaying the same context should also succeed to ensure idempotency. | ||
// This should be handled later in the pipeline by comparing the circuit | ||
// keys. | ||
err = ctx.registry.processKeySend(keysendCtx) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that testing on the external interface of invoice registry is more robust.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
my thought here was that this is a better place to assert the behavior of processKeySend
wrt to how it filters errors
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is it better? We also already have a parameterized TestKeysend
that tests on the external interface. Maybe that can be extended easily.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
afaict you can't test the idempotency of processKeySend
because it settles the keysend immediately after
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
moved everything to a white box test, but we are discussing holding off on this change and fixing as part of #4338
bee90cf
to
46ba6eb
Compare
@joostjager rebased on #4334 |
This commit automatically generates a random payment address for any new keysend payments. With the addition of the payment address index, we now enforce uniqueness of payment addresses across all invoices. As a result, if you run master currently you can only accept one keysend payment since they all share a zero-value payment address.
46ba6eb
to
7d1c528
Compare
Rolled into #4338 |
This commit automatically generates a random payment address for any new
keysend payments. With the addition of the payment address index, we now
enforce uniqueness of payment addresses across all invoices. As a
result, if you run master currently you can only accept one keysend
payment since they all share a zero-value payment address.
Follow up from #4285