Rename `Offer::signing_pubkey` to `Offer::issuer_id`

[jkczyz]

The spec was recently changed to use offer_issuer_id instead of offer_node_id. LDK always used signing_pubkey to avoid confusion with a node_id. Rename it to issuer_id now that the spec is cleared up. Continue to use signing_pubkey in signing contexts.

Also, add issuer_id methods to invoice structs. Update test vectors from earlier spec changes.

[codecov[bot]]

Codecov Report

Attention: Patch coverage is 93.59606% with 13 lines in your changes missing coverage. Please review.

Project coverage is 89.62%. Comparing base (6662c5c) to head (e11025f). Report is 8 commits behind head on main.

Files with missing lines	Patch %	Lines
lightning/src/offers/invoice.rs	70.96%	9 Missing :warning:
lightning/src/ln/channelmanager.rs	71.42%	1 Missing and 1 partial :warning:
lightning/src/offers/invoice_request.rs	96.42%	2 Missing :warning:

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3218      +/-   ##
==========================================
- Coverage   89.65%   89.62%   -0.04%     
==========================================
  Files         126      126              
  Lines      102546   102564      +18     
  Branches   102546   102564      +18     
==========================================
- Hits        91935    91918      -17     
- Misses       7890     7916      +26     
- Partials     2721     2730       +9     

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

[valentinewallace]

rustfmt check is failing

[TheBlueMatt]

Needs rebase.

[TheBlueMatt]

Feel free to squash.

[TheBlueMatt]

Needs rebase :/

[jkczyz]

Needs rebase :/

Rebased 😮‍💨

[TheBlueMatt]

LGTM, basically, feel free to squash. I do find the issuer terminology to be super confusing, though - after this PR I can do InvoiceRequest.issuer_signing_pubkey() and get back a pubkey for the Offer, not the issuer of the InvoiceRequest as the method seems to imply (and before this PR there's still InvoiceRequest.issuer() itself).

[jkczyz]

LGTM, basically, feel free to squash. I do find the issuer terminology to be super confusing, though - after this PR I can do InvoiceRequest.issuer_signing_pubkey() and get back a pubkey for the Offer, not the issuer of the InvoiceRequest as the method seems to imply (and before this PR there's still InvoiceRequest.issuer() itself).

We could introduce an OfferRef<'a> type (wrapping a reference to the InvoiceRequest, actually) and have those methods on it instead. Then we can still define them with the macro. Caller would do something like invoice_request.offer().signing_pubkey(). Would such a type be a problem for bindings?

[TheBlueMatt]

Why not just use method names that make sense in both contexts? That seems like a bunch of effort to avoid picking a more descriptive name :).

[jkczyz]

Why not just use method names that make sense in both contexts? That seems like a bunch of effort to avoid picking a more descriptive name :).

Some reason given in https://github.com/lightningdevkit/rust-lightning/pull/3218#discussion_r1715748116. It's also inconsistent with other offer field names. Could use an offer_ and invreq_ prefixes everywhere, but arguably having a different code separator between a logical "struct" and "field" is more readable than smashing everything together with underscores. The former is also more difficult to do with macros without leaving the separator on where it isn't need (e.g., Offer::offer_issuer_id).

[TheBlueMatt]

Some reason given in https://github.com/lightningdevkit/rust-lightning/pull/3218#discussion_r1715748116.

I agree with the comments there, but I don't think it specifically argues for "issuer".

It's also inconsistent with other offer field names. Could use an offer_ and invreq_ prefixes everywhere, but arguably having a different code separator between a logical "struct" and "field" is more readable than smashing everything together with underscores. The former is also more difficult to do with macros without leaving the separator on where it isn't need (e.g., Offer::offer_issuer_id).

I agree with this, but to be clear my main complaint isn't that we need Offer-issuer, but rather that issuer is ambiguous in a general sense. Using "payer" for the entity paying makes sense, but "issuer" isn't the opposite of that (I know we probably don't want to use "payee" cause that confuses things for Refunds), but there should be something better, IMO.

[TheBlueMatt]

To be clear, we also don't necessarily need to figure it out now - we already use "issuer" to describe the Offer creator, so using it again is fine, I just think we should stop using it :)

[valentinewallace]

2c -- I agree that InvoiceRequest::issuer_signing_pubkey is a bit confusing. Tbh, I don't see an issue with using payee or recipient terminology, if we want to stick to just renames and not go for Jeff's invoice_request.offer().offer_field() solution. But I don't think it's too critical to figure out right now so fine to ACK to unblock the follow-ups if Matt is on board.

[jkczyz]

2c -- I agree that InvoiceRequest::issuer_signing_pubkey is a bit confusing. Tbh, I don't see an issue with using payee or recipient terminology, if we want to stick to just renames and not go for Jeff's invoice_request.offer().offer_field() solution. But I don't think it's too critical to figure out right now so fine to ACK to unblock the follow-ups if Matt is on board.

FWIW, the offer issuer may not be the payment recipient.

[valentinewallace]

2c -- I agree that InvoiceRequest::issuer_signing_pubkey is a bit confusing. Tbh, I don't see an issue with using payee or recipient terminology, if we want to stick to just renames and not go for Jeff's invoice_request.offer().offer_field() solution. But I don't think it's too critical to figure out right now so fine to ACK to unblock the follow-ups if Matt is on board.

FWIW, the offer issuer may not be the payment recipient.

Ah, okay. Is this the payment notifications case?

[jkczyz]

Ah, okay. Is this the payment notifications case?

Yup, that's what I had in mind.

[TheBlueMatt]

Needs rebase.