Skip to content
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

Drop user-provided payer_id from InvoiceRequestBuilder #3198

Open
TheBlueMatt opened this issue Jul 22, 2024 · 8 comments · May be fixed by #3264
Open

Drop user-provided payer_id from InvoiceRequestBuilder #3198

TheBlueMatt opened this issue Jul 22, 2024 · 8 comments · May be fixed by #3264
Assignees
Labels
Take a Friday Leave a Friday Stomp the Bugs, Without Much Commitment
Milestone

Comments

@TheBlueMatt
Copy link
Collaborator

We shouldn't have supported this to begin with, and we should drop it before people start using it.

@TheBlueMatt TheBlueMatt added this to the 0.0.124 milestone Jul 22, 2024
@jkczyz jkczyz self-assigned this Jul 22, 2024
@jkczyz
Copy link
Contributor

jkczyz commented Jul 22, 2024

Wouldn't this prevent anyone from using the offers module in a standalone way? They'd be required to use ExpandedKey and PaymentId, which are more ChannelManager-specific.

@TheBlueMatt
Copy link
Collaborator Author

You could use ExpandedKey and PaymentId without a ChannelManager? PaymentId still retains its meaning here - we use it to derive the per-payment metadata, which users still need to make constant per payment. Similar for ExpandedKey, even if a bit less so - its just some key material, which users still need to deal with but there's just some extra key material there that's not used.

@tnull
Copy link
Contributor

tnull commented Jul 22, 2024

I understand that allowing to override payer_id is a potentially dangerous API, but wonder if we should/need to keep payer_id exposed for inbound payments, e.g., if other implementations choose to use it differently than us?

@TheBlueMatt
Copy link
Collaborator Author

Honestly we should probably kill off payer_id entirely, but mostly I think we want to very, very strongly discourage anyone from using payer_id for the purpose of identifying payers. Its really dangerous to use that way and causes social costs if people do. IMO we should remove it if only so that people who want it come ask us and we can tell them to stop breaking lightning.

@TheBlueMatt
Copy link
Collaborator Author

Slipping. We should still do this so users don't rely on this API, but there's no super strong reason to do it now.

@TheBlueMatt TheBlueMatt removed this from the 0.0.124 milestone Aug 8, 2024
@TheBlueMatt TheBlueMatt added the Take a Friday Leave a Friday Stomp the Bugs, Without Much Commitment label Aug 8, 2024
@TheBlueMatt TheBlueMatt added this to the 0.1 milestone Aug 8, 2024
@jkczyz
Copy link
Contributor

jkczyz commented Aug 16, 2024

Was just looking at LNDK's usage. It no longer uses a fixed key to sign in order to fix the caching issue -- the spec was changed to cache based on payer_metadata instead of payer_id, so that should no longer be an issue anyway. However, LNDK does use derived keys from LND for signing. I'm not sure if this is a hard requirement though (cc: @orbitalturtle). Just want to raise that before going forward with this change.

See lndk-org/lndk#133.

@orbitalturtle
Copy link
Contributor

@jkczyz Yeah I'm pretty sure it's not a hard requirement for LNDK so moving in that direction should be fine for us! So if I'm understanding right, in the future we'd want to use request_invoice_deriving_payer_id and let LDK derive the payer_id instead of using request_invoice_deriving_metadata?

@jkczyz
Copy link
Contributor

jkczyz commented Aug 17, 2024

Yes, and doing so means you would call build_and_sign instead of build and then sign as attempting to use the latter methods will fail to compile.

@jkczyz jkczyz linked a pull request Aug 21, 2024 that will close this issue
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Take a Friday Leave a Friday Stomp the Bugs, Without Much Commitment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants