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

BOLT 12 offer encoding and building #1719

Merged
merged 6 commits into from
Nov 8, 2022

Conversation

jkczyz
Copy link
Contributor

@jkczyz jkczyz commented Sep 13, 2022

Define an interface for BOLT 12 offer messages. The underlying format consists of the original bytes and the parsed contents.

The bytes are later needed when constructing an invoice_request message. This is because it must mirror all the offer TLV records, including unknown ones, which aren't represented in the contents.

The contents will be used in invoice_request messages to avoid duplication. Some fields while required in a typical user-pays-merchant flow may not be necessary in the merchant-pays-user flow (i.e., refund).

Also, defines a builder for constructing an offer given a required description and node id.

See #1597 for ongoing BOLT 12 work depending on this PR.

@codecov-commenter
Copy link

codecov-commenter commented Sep 13, 2022

Codecov Report

Base: 90.79% // Head: 90.80% // Increases project coverage by +0.01% 🎉

Coverage data is based on head (e673125) compared to base (8f525c4).
Patch coverage: 94.52% of modified lines in pull request are covered.

❗ Current head e673125 differs from pull request most recent head 20da3f3. Consider uploading reports for the commit 20da3f3 to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1719      +/-   ##
==========================================
+ Coverage   90.79%   90.80%   +0.01%     
==========================================
  Files          87       89       +2     
  Lines       47604    47971     +367     
  Branches    47604    47971     +367     
==========================================
+ Hits        43221    43562     +341     
- Misses       4383     4409      +26     
Impacted Files Coverage Δ
lightning/src/lib.rs 100.00% <ø> (ø)
lightning/src/ln/channelmanager.rs 85.41% <ø> (ø)
lightning/src/onion_message/packet.rs 76.03% <ø> (ø)
lightning/src/util/ser_macros.rs 88.40% <80.00%> (-0.41%) ⬇️
lightning/src/util/ser.rs 91.80% <91.66%> (+0.13%) ⬆️
lightning/src/offers/offer.rs 94.59% <94.59%> (ø)
lightning/src/ln/features.rs 99.67% <100.00%> (+<0.01%) ⬆️
lightning/src/onion_message/blinded_route.rs 96.90% <100.00%> (+0.24%) ⬆️
lightning/src/routing/gossip.rs 92.48% <100.00%> (-0.03%) ⬇️
lightning/src/util/events.rs 37.62% <100.00%> (ø)
... and 5 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

u8
};
(u16) => {
::util::ser::HighZeroBytesDroppedBigSize<u16>
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm a bit uneasy about putting these directly in a struct - can we not just keep using them as wrappers at serialization-time rather than actually storing them anywhere?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Pushed a version that only uses them for decoding / encoding. It does remove a lot of intos, which is nice. Though I'd be more uneasy that the macros don't read / write correctly, whereas mistakes in the "struct using wrapper" approach can by caught be the compiler, FWIW.

Copy link
Collaborator

Choose a reason for hiding this comment

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

But given its all hidden by a struct-defining macro we end up in the same place, no?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, mostly just saying more care needs to be taken when implementing and updating the macros. While making the change, I had an intermediate state where encoding and decoding were not compatible.

tlv_record_import!($fieldty$(<$gen>)?);
)*

pub(crate) struct $name<'a> {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can't we just name this something slightly different rather than putting it in a module and having to guess the import path required (by listing the Rust prelude explicitly in our code)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Mainly was trying to keep the usage site looking like a struct. Made it explicit now. Let me know if that is what you were thinking of.

impl<'a> From<&'a String> for WithoutLength<&'a String> {
fn from(s: &'a String) -> Self { Self(s) }
}
impl From<WithoutLength<String>> for String {
Copy link
Collaborator

Choose a reason for hiding this comment

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

If we make these serialization-only wrappers we can drop the copy versions, which seems like the right thing to do.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We'd still need this to simplify deserialization, though it should be a move. I'd imagine it's equivalent to explicitly using .0.

lightning/src/util/ser.rs Outdated Show resolved Hide resolved
lightning/src/offers/offer.rs Outdated Show resolved Hide resolved

//! Data structures and encoding for `offer` messages.
//!
//! An [`Offer`] is built by the merchant for the user-pays-merchants flow or may be parsed from a
Copy link
Contributor

Choose a reason for hiding this comment

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

Offers are also good for user-pays-user "Cash App" flow, would be nice to amend the docs to include this

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The "user-pays-merchant" terminology is from the BOLT and is used to differentiate from "merchant-pays-user" (e.g., refunds, ATM) flow, which I use in a future commit. I could use "recipient" instead of the first use of "merchant" here to make it more generic. But not sure if I want to deviate from the spec by adding a new "flow". Open to other ideas so long as we can make it work with the invoice_request docs (see #1597 for current draft), which will be expanded with another builder for the "merchant-pays-user" flow.

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe we could do payer-offers-money vs payer-pays-recipient (maybe the word "offer" is confusing there)? ATM isn't really captured by "merchant" either.. I think we've historically not cared too much about sticking with spec naming/copy if there's a reason to deviate, like with wumbo, scid_privacy vs alias, etc.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Maybe we could do payer-offers-money vs payer-pays-recipient (maybe the word "offer" is confusing there)?

Hmm... payer-pays-recipient seems rather tautological. The "payer" is always the one paying a recipient. 😛

I guess "recipient" was a poor suggestion given we want to distinguish who is getting paid. Really, we are trying to distinguish who creates the "offer" (loosely since it could be an invoice_request offer) from who pays the invoice. Or even who creates the invoice.

  • In the user-pays-merchant flow, the merchant creates both the offer and the invoice. So the user pays the invoice.
  • In the merchant-pays-user flow, the merchant creates the "offer" but the user creates the invoice. So the merchant pays the invoice.

With that in mind, referring to them as "user" and "merchant" seems orthogonal to what we are trying to convey. That is, relative to who created the "offer", who is creating (or paying) the invoice? Or even, is the offer for a product/service or is the "offer" for money?

ATM isn't really captured by "merchant" either..

Well, the merchant owns the ATM, which accepts fiat and is using lighting to deliver the product (i.e. bitcoin). So I think the ATM and the refund cases are still that of the merchant paying bitcoin.

I think we've historically not cared too much about sticking with spec naming/copy if there's a reason to deviate, like with wumbo, scid_privacy vs alias, etc.

True, though we could contribute back if we have a better alternative in this case. Note that the BOLT says:

Here we use "user" as shorthand for the individual user's lightning node and "merchant" as the shorthand for the node of someone who is selling or has sold something.

Not sure if "individual user's lightning node" is really any clearer, though.

Copy link
Contributor

Choose a reason for hiding this comment

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

From my PoV, I'm fine with the BOLT copy because it's protocol-dev-facing and gets the point across. For wallet-dev-facing documentation, though, IMO there are different requirements and it'd be best to be inclusive of the range of use cases.

Given this, maybe we could have an expanded Use Cases section in the top-level docs, which include the user-pays-merchant/ATM/etc, and Offer could be documented as "an offer to be paid" and InvoiceRequest as "an offer of money", rather than each referencing a specific "flow"?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

From my PoV, I'm fine with the BOLT copy because it's protocol-dev-facing and gets the point across. For wallet-dev-facing documentation, though, IMO there are different requirements and it'd be best to be inclusive of the range of use cases.

Could you enumerate these use cases?

Given this, maybe we could have an expanded Use Cases section in the top-level docs, which include the user-pays-merchant/ATM/etc, and Offer could be documented as "an offer to be paid" and InvoiceRequest as "an offer of money", rather than each referencing a specific "flow"?

Yeah, I agree that the offers top-level module should have an expanded explanation of use cases and how the various types relate to them. I'd prefer to wait until all the types exist before writing this.

In the offer submodule, I'd like to stick to showing basic example code, though, for the specific types involved. I've updated the wording a bit. Let me know what you think.

Copy link
Contributor

Choose a reason for hiding this comment

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

The updates look good, and that plan sgtm! The missing use case from my PoV was the Cash App flow (wouldn't want to exclude the people getting tattoos lol).

lightning/src/offers/offer.rs Outdated Show resolved Hide resolved
lightning/src/offers/offer.rs Show resolved Hide resolved
Copy link
Collaborator

@TheBlueMatt TheBlueMatt left a comment

Choose a reason for hiding this comment

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

Basically looks good.

lightning/src/offers/offer.rs Outdated Show resolved Hide resolved
lightning/src/offers/offer.rs Show resolved Hide resolved
lightning/src/offers/offer.rs Outdated Show resolved Hide resolved
lightning/src/offers/offer.rs Outdated Show resolved Hide resolved
}

/// The recipient's public key used to sign invoices.
pub fn node_id(&self) -> PublicKey {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do we want to differentiate the naming here given we should encourage users to sign invoices with a different key than their node id?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Maybe signing_node_id?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe just signing_public_key or so? I don't think we want to connect it to node ids at all?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed but left as node_id in the TLV stream to match the spec, for now.

}

/// Builds an [`Offer`] from the builder's settings.
pub fn build(self) -> Offer {
Copy link
Collaborator

Choose a reason for hiding this comment

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

We should sign the offer here, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Offers no longer have signatures. The node_id is for signing invoices.

@TheBlueMatt
Copy link
Collaborator

Needs rebase on the features changes, should be easy.

}

/// Builds an [`Offer`] from the builder's settings.
pub fn build(self) -> Offer {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Offers no longer have signatures. The node_id is for signing invoices.

}

/// The recipient's public key used to sign invoices.
pub fn node_id(&self) -> PublicKey {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Maybe signing_node_id?

lightning/src/offers/offer.rs Outdated Show resolved Hide resolved
lightning/src/offers/offer.rs Outdated Show resolved Hide resolved
lightning/src/offers/offer.rs Show resolved Hide resolved
impl Offer {
/// The chain used for paying the invoice.
pub fn chain(&self) -> ChainHash {
// TODO: Update once spec is finalized
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@TheBlueMatt FYI, I don't think there was any resolution on this: lightning/bolts#798 (comment). Though I'm not sure how you are thinking about negotiating this given the sender would need to know what chains the recipient supports. Currently, the sender states which chain in the invoice_request based on what's in the offer. If it were left out, wouldn't you need another roundtrip?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

FYI, also updated this to return a Vec and made some changes to rust-bitcoin to allow us to return a slice.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I was figuring the invoice would simply contain a list of supported chains and the sender would just pick which they want to use from that list.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Don't you need a chain to figure out how to send the invoice_request in the first place?

@jkczyz
Copy link
Contributor Author

jkczyz commented Sep 16, 2022

Pushed some changes mostly rearranging some of the fixups and cleaning up some things that had been missed.

@jkczyz
Copy link
Contributor Author

jkczyz commented Sep 19, 2022

Needs rebase on the features changes, should be easy.

Rebased

lightning/src/offers/offer.rs Show resolved Hide resolved

//! Data structures and encoding for `offer` messages.
//!
//! An [`Offer`] is built by the merchant for the user-pays-merchants flow or may be parsed from a
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe we could do payer-offers-money vs payer-pays-recipient (maybe the word "offer" is confusing there)? ATM isn't really captured by "merchant" either.. I think we've historically not cared too much about sticking with spec naming/copy if there's a reason to deviate, like with wumbo, scid_privacy vs alias, etc.

lightning/src/offers/offer.rs Outdated Show resolved Hide resolved
lightning/src/offers/offer.rs Outdated Show resolved Hide resolved
lightning/src/offers/offer.rs Outdated Show resolved Hide resolved
///
/// Successive calls to this method will override the previous setting.
#[cfg(test)]
pub fn features(mut self, features: OfferFeatures) -> Self {
Copy link
Contributor

Choose a reason for hiding this comment

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

Is the plan to replace this with a basic_mpp() method when that feature bit is resolved?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think so, but I do wonder if we'd prefer to default it somehow. Thoughts?

Copy link
Contributor

Choose a reason for hiding this comment

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

Hm, I guess I don't have a strong preference between a setter and an un-setter.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, just reading through the updated BOLT 12 draft. There is a separate set of features for each message, so we only need to worry about this in the InvoiceBuilder.

lightning/src/offers/offer.rs Outdated Show resolved Hide resolved
// You may not use this file except in accordance with one or both of these
// licenses.

//! Data structures and encoding for `offer` messages.
Copy link
Contributor

Choose a reason for hiding this comment

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

"Message" implies it's a p2p message to me, is there a way to disambiguate?

Copy link
Contributor Author

@jkczyz jkczyz Sep 21, 2022

Choose a reason for hiding this comment

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

Meh... it's splitting hairs a bit, IMHO. invoice_request and invoice aren't really p2p messages either since they are delivered as part of an onion message. And invoice_request in the "send invoice" case doesn't use p2p either. But all of them are messages in a general sense. offer just has a different transport mechanism than the others. 😛

Do you have any alternative phrasing in mind?

metadata: Option<Vec<u8>>,
amount: Option<Amount>,
description: String,
features: Option<OfferFeatures>,
Copy link
Contributor

Choose a reason for hiding this comment

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

Could this just be empty() and then set features based on setters?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I preferred using Options otherwise special handling is needed for serializing empty features / Vecs. That said, for Option<Vec<_>> we could hide the Option from the API by returning a slice if Some or the empty slice if None. Similarly with Option<String> using &str. Unfortunately, I don't think we can do the same with OfferFeatures without returning a temporary. 😕

Do we prefer to hide the Option whenever possible? Seems it may make the API somewhat inconsistent but I'm open to the idea.

Copy link
Contributor

Choose a reason for hiding this comment

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

Could you clarify how that makes the API inconsistent? Seems a bit cleaner to avoid returning an Option where it makes sense. I thought the OfferTlvStreamRef could work around the serialization issues but maybe I'm missing something

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Could you clarify how that makes the API inconsistent? Seems a bit cleaner to avoid returning an Option where it makes sense.

There's a few questions that we'd have to answer. If we return &OfferFeatures instead of Option<&OfferFeature>, should we return

  • &str or Option<&String>?
  • &[T] or Option<&Vec<T>>
  • u64 or Option<&Amount>
  • Duration or Option<Duration>

I can see the argument for &[T], but I'm not as sure about &str. Is it better for the user to check against None or the empty string to determine if a string is set. Is it ok to treat the empty string as not set, and vice versa?

For amount, we'll eventually need an exchange rate interface, so this could work for each variant. But for None, should we return 0? That said, presumably a wallet would want to display the offer's native currency, so would likely need to stick with Option or add a third variant to Amount for None.

Should we stick with Option for absolute expiry or return the maximum duration if None?

I thought the OfferTlvStreamRef could work around the serialization issues but maybe I'm missing something

Yeah, I suppose it could by doing:

let features = if self.features == OfferFeatures::empty() { None } else { Some(&self.features) };

But that raises a secondary concern. When should Option be used in OfferContents? In some places like amount it probably should be. But what about issuer or paths? We'd have to be comfortable with treating an empty TLV value as the same as if the TLV record had not been set. This also means an encoded TLV might not be equal with it after parsed and re-encoded because presumably we wouldn't write the record for empty values. Similarly for features.

My intuition is to stick with Option whenever possible, especially if InvoiceRequest or Invoice need to check against the presence. But we probably can go without Option around Vec and OfferFeatures with the above caveat in mind, assuming we're ok with additional conversion logic in as_tlv_stream here and try_from in later PRs. I'm non sure about String, regardless of what the API ultimately looks like.

Let me know what you think.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, since we already don’t return Options for all getters I didn’t think of that as much of an API inconsistency(?).

  • u64 or Option<&Amount>
  • Duration or Option

I thought these two could make sense as options still. Certainly expiry being None makes sense IIUC? Fair point that amount could be weird though, is it possible to make Amounts contain NonZeroU64s?

&str or Option<&String>?

IIUC that’s only for issuer, IMO that one arguably makes sense as an Option<&String> for the reasons you stated, no strong feelings though.

features was the only one where I couldn’t see a reason for it to be None vs Some(OfferFeatures::empty() and therefore seemed weird in the public API.

We could get rid of the features() getter entirely and only have individual methods for individual features (offer.supports_mpp()). That way it could stay an Option under the hood so that parsing-and-reencoding always gets the same result. I admit I hadn’t considered that situation though.

Although, I thought since we save the offer bytes, parsing and re-encoding will always be the same anyway? Lmk what I'm missing, not sure I understood your point about how sticking with Option benefits InvReq/Inv

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, since we already don’t return Options for all getters I didn’t think of that as much of an API inconsistency(?).

Yes, very true.

  • u64 or Option<&Amount>
  • Duration or Option

I thought these two could make sense as options still. Certainly expiry being None makes sense IIUC? Fair point that amount could be weird though, is it possible to make Amounts contain NonZeroU64s?

Hadn't thought about NonZeroU64 for amount. Honestly, it's a little tough to work with, and the BOLT doesn't seem to care if it is zero. Just says it's a "minimum".

&str or Option<&String>?

IIUC that’s only for issuer, IMO that one arguably makes sense as an Option<&String> for the reasons you stated, no strong feelings though.

Yeah, just wasn't sure if we should try to do something similar to Vecs. I think I'm learning towards Option but Option<&str>, which removes some boilerplate from tests.

features was the only one where I couldn’t see a reason for it to be None vs Some(OfferFeatures::empty() and therefore seemed weird in the public API.

We could get rid of the features() getter entirely and only have individual methods for individual features (offer.supports_mpp()). That way it could stay an Option under the hood so that parsing-and-reencoding always gets the same result. I admit I hadn’t considered that situation though.

Let's use OfferFeatures::empty() without the Option for now. Could be nice to know if there are odd, unknown features.

Although, I thought since we save the offer bytes, parsing and re-encoding will always be the same anyway? Lmk what I'm missing, not sure I understood your point about how sticking with Option benefits InvReq/Inv

Ah, right! Literally ran into this yesterday when adding parsing failure tests in the next PR. Had to manually modify the TLV stream to make the test fail. 😛

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Lmk what I'm missing, not sure I understood your point about how sticking with Option benefits InvReq/Inv

Sometime we need to check if a field in the Offer is present to determine how to handle the InvoiceRequest (e.g., amount). During parsing, it's simplest to convert the OfferTlvStream to OfferContents first before performing these checks. So having the Option in OfferContents can be useful.

lightning/src/offers/offer.rs Outdated Show resolved Hide resolved
lightning/src/onion_message/mod.rs Outdated Show resolved Hide resolved
///
/// Successive calls to this method will override the previous setting.
#[cfg(test)]
pub fn features(mut self, features: OfferFeatures) -> Self {
Copy link
Contributor

Choose a reason for hiding this comment

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

Hm, I guess I don't have a strong preference between a setter and an un-setter.


//! Data structures and encoding for `offer` messages.
//!
//! An [`Offer`] is built by the merchant for the user-pays-merchants flow or may be parsed from a
Copy link
Contributor

Choose a reason for hiding this comment

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

The updates look good, and that plan sgtm! The missing use case from my PoV was the Cash App flow (wouldn't want to exclude the people getting tattoos lol).

lightning/src/offers/offer.rs Outdated Show resolved Hide resolved
metadata: Option<Vec<u8>>,
amount: Option<Amount>,
description: String,
features: Option<OfferFeatures>,
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you clarify how that makes the API inconsistent? Seems a bit cleaner to avoid returning an Option where it makes sense. I thought the OfferTlvStreamRef could work around the serialization issues but maybe I'm missing something

lightning/src/offers/offer.rs Outdated Show resolved Hide resolved
lightning/src/offers/offer.rs Outdated Show resolved Hide resolved
lightning/src/offers/offer.rs Outdated Show resolved Hide resolved
lightning/src/util/ser.rs Outdated Show resolved Hide resolved
@jkczyz
Copy link
Contributor Author

jkczyz commented Sep 22, 2022

@valentinewallace Offer API changes are in 7e932e0. Let me know if you'd prefer that I revert any of them. (cc: @TheBlueMatt)

lightning/src/offers/offer.rs Outdated Show resolved Hide resolved
lightning/src/offers/offer.rs Show resolved Hide resolved
lightning/src/offers/offer.rs Outdated Show resolved Hide resolved
lightning/src/offers/offer.rs Outdated Show resolved Hide resolved
lightning/src/util/ser.rs Outdated Show resolved Hide resolved
lightning/src/util/ser_macros.rs Show resolved Hide resolved
@valentinewallace
Copy link
Contributor

This is looking pretty good to me, I think it would be good timing for a second reviewer to come in and I'll check out #1726 in the meantime

Copy link
Collaborator

@TheBlueMatt TheBlueMatt left a comment

Choose a reason for hiding this comment

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

Some docs need more detail, but is looking good.

lightning/src/offers/offer.rs Outdated Show resolved Hide resolved
lightning/src/offers/offer.rs Outdated Show resolved Hide resolved
lightning/src/offers/offer.rs Outdated Show resolved Hide resolved
lightning/src/offers/offer.rs Outdated Show resolved Hide resolved
pub type CurrencyCode = [u8; 3];

tlv_stream!(OfferTlvStream, OfferTlvStreamRef, {
(2, chains: Vec<ChainHash>),
Copy link
Collaborator

Choose a reason for hiding this comment

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

If these are all required, should we do something like we do for lightning-invoice in the builder to make it a compile-time error to miss a field?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It depends on the context, though note that all the records use Option in the TLV structs.

For Offer, only description and node_id are required. This is currently enforced at compile-time by OfferBuilder::new requiring both as parameters.

However, for InvoiceRequest used as a "send invoice" (e.g., refund), node_id MUST NOT be set.

I also avoided the lightning-invoice-style compile time checks because IIUC it can't be easily done in bindings since each builder function returns a different type, essentially.

lightning/src/offers/offer.rs Outdated Show resolved Hide resolved
.unwrap_or_else(|| vec![ChainHash::using_genesis_block(Network::Bitcoin)])
}

/// Metadata set by the originator. Useful for authentication and validating fields.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Some of the above comments apply in the getters too.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ended up expanding comments in Offer and linking to those methods in the OfferBuilder docs to avoid repetition.

@TheBlueMatt
Copy link
Collaborator

Oh feel free to squash, as far as I care.

u8
};
(u16) => {
::util::ser::HighZeroBytesDroppedBigSize<u16>
Copy link
Collaborator

Choose a reason for hiding this comment

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

Another question here is how much magic we want in the macro - instead of having to write out a list of type -> wrapper structs here, we could just do it at the callsite, ensuring the callsite readably describes how things are being serialized, rather than it being hidden? That will also avoid having a list of things that could end up randomly doing the Wrong Thing in a surprising way.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Are you suggesting we expand encode_tlv (the call site) such that there is a tlv_record matcher for each of these types? Could do that, but it would result in a lot of repetitive boilerplate. Also, wouldn't prevent doing thing surprising given the same matchers here would have to be used there.

Note that these three helper macros are only used in one place each. I think I could define them as nested macros at the respective calls sites. But then relative to each other they would be spread all about, and it's kinda nice having them all in one place.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Well, I'm still a bit lost on why we need a new tlv_record thing at all - if all the fields are Option wrapped, everything should be option? Then its just a matter of having the tlv_stream macro understand what the field is, which would presumably mean passing u8, HighZeroBytesDroppedBigSize as separate parameters.

Copy link
Collaborator