From e61001f60daa4f3d96c3b3f54cd3a8b00051ebe0 Mon Sep 17 00:00:00 2001 From: Jeffrey Czyz Date: Thu, 2 May 2024 11:43:04 -0500 Subject: [PATCH] Only require description when offer has an amount The spec was changed to allow excluding an offer description if the offer doesn't have an amount. However, it is still required when the amount is set. --- lightning/src/offers/invoice.rs | 12 ++--- lightning/src/offers/invoice_request.rs | 5 ++- lightning/src/offers/offer.rs | 60 ++++++++++++++++--------- 3 files changed, 48 insertions(+), 29 deletions(-) diff --git a/lightning/src/offers/invoice.rs b/lightning/src/offers/invoice.rs index d2e2eecde..47e05edf9 100644 --- a/lightning/src/offers/invoice.rs +++ b/lightning/src/offers/invoice.rs @@ -717,7 +717,7 @@ macro_rules! invoice_accessors { ($self: ident, $contents: expr) => { /// From [`Offer::description`] or [`Refund::description`]. /// /// [`Offer::description`]: crate::offers::offer::Offer::description - pub fn description(&$self) -> PrintableString { + pub fn description(&$self) -> Option { $contents.description() } @@ -952,12 +952,12 @@ impl InvoiceContents { } } - fn description(&self) -> PrintableString { + fn description(&self) -> Option { match self { InvoiceContents::ForOffer { invoice_request, .. } => { invoice_request.inner.offer.description() }, - InvoiceContents::ForRefund { refund, .. } => refund.description(), + InvoiceContents::ForRefund { refund, .. } => Some(refund.description()), } } @@ -1546,7 +1546,7 @@ mod tests { assert_eq!(unsigned_invoice.offer_chains(), Some(vec![ChainHash::using_genesis_block(Network::Bitcoin)])); assert_eq!(unsigned_invoice.metadata(), None); assert_eq!(unsigned_invoice.amount(), Some(&Amount::Bitcoin { amount_msats: 1000 })); - assert_eq!(unsigned_invoice.description(), PrintableString("")); + assert_eq!(unsigned_invoice.description(), Some(PrintableString(""))); assert_eq!(unsigned_invoice.offer_features(), Some(&OfferFeatures::empty())); assert_eq!(unsigned_invoice.absolute_expiry(), None); assert_eq!(unsigned_invoice.message_paths(), &[]); @@ -1590,7 +1590,7 @@ mod tests { assert_eq!(invoice.offer_chains(), Some(vec![ChainHash::using_genesis_block(Network::Bitcoin)])); assert_eq!(invoice.metadata(), None); assert_eq!(invoice.amount(), Some(&Amount::Bitcoin { amount_msats: 1000 })); - assert_eq!(invoice.description(), PrintableString("")); + assert_eq!(invoice.description(), Some(PrintableString(""))); assert_eq!(invoice.offer_features(), Some(&OfferFeatures::empty())); assert_eq!(invoice.absolute_expiry(), None); assert_eq!(invoice.message_paths(), &[]); @@ -1688,7 +1688,7 @@ mod tests { assert_eq!(invoice.offer_chains(), None); assert_eq!(invoice.metadata(), None); assert_eq!(invoice.amount(), None); - assert_eq!(invoice.description(), PrintableString("")); + assert_eq!(invoice.description(), Some(PrintableString(""))); assert_eq!(invoice.offer_features(), None); assert_eq!(invoice.absolute_expiry(), None); assert_eq!(invoice.message_paths(), &[]); diff --git a/lightning/src/offers/invoice_request.rs b/lightning/src/offers/invoice_request.rs index 90d6d5bf3..aa6d7c067 100644 --- a/lightning/src/offers/invoice_request.rs +++ b/lightning/src/offers/invoice_request.rs @@ -1263,7 +1263,7 @@ mod tests { assert_eq!(unsigned_invoice_request.chains(), vec![ChainHash::using_genesis_block(Network::Bitcoin)]); assert_eq!(unsigned_invoice_request.metadata(), None); assert_eq!(unsigned_invoice_request.amount(), Some(&Amount::Bitcoin { amount_msats: 1000 })); - assert_eq!(unsigned_invoice_request.description(), PrintableString("")); + assert_eq!(unsigned_invoice_request.description(), Some(PrintableString(""))); assert_eq!(unsigned_invoice_request.offer_features(), &OfferFeatures::empty()); assert_eq!(unsigned_invoice_request.absolute_expiry(), None); assert_eq!(unsigned_invoice_request.paths(), &[]); @@ -1295,7 +1295,7 @@ mod tests { assert_eq!(invoice_request.chains(), vec![ChainHash::using_genesis_block(Network::Bitcoin)]); assert_eq!(invoice_request.metadata(), None); assert_eq!(invoice_request.amount(), Some(&Amount::Bitcoin { amount_msats: 1000 })); - assert_eq!(invoice_request.description(), PrintableString("")); + assert_eq!(invoice_request.description(), Some(PrintableString(""))); assert_eq!(invoice_request.offer_features(), &OfferFeatures::empty()); assert_eq!(invoice_request.absolute_expiry(), None); assert_eq!(invoice_request.paths(), &[]); @@ -1996,6 +1996,7 @@ mod tests { } let invoice_request = OfferBuilder::new(recipient_pubkey()) + .description("foo".to_string()) .amount(Amount::Currency { iso4217_code: *b"USD", amount: 1000 }) .build_unchecked() .request_invoice(vec![1; 32], payer_pubkey()).unwrap() diff --git a/lightning/src/offers/offer.rs b/lightning/src/offers/offer.rs index baac94951..6df9b30a4 100644 --- a/lightning/src/offers/offer.rs +++ b/lightning/src/offers/offer.rs @@ -209,9 +209,8 @@ impl MetadataStrategy for DerivedMetadata {} macro_rules! offer_explicit_metadata_builder_methods { ( $self: ident, $self_type: ty, $return_type: ty, $return_value: expr ) => { - /// Creates a new builder for an offer setting an empty [`Offer::description`] and using the - /// [`Offer::signing_pubkey`] for signing invoices. The associated secret key must be remembered - /// while the offer is valid. + /// Creates a new builder for an offer using the [`Offer::signing_pubkey`] for signing invoices. + /// The associated secret key must be remembered while the offer is valid. /// /// Use a different pubkey per offer to avoid correlating offers. /// @@ -225,7 +224,7 @@ macro_rules! offer_explicit_metadata_builder_methods { ( pub fn new(signing_pubkey: PublicKey) -> Self { Self { offer: OfferContents { - chains: None, metadata: None, amount: None, description: String::new(), + chains: None, metadata: None, amount: None, description: None, features: OfferFeatures::empty(), absolute_expiry: None, issuer: None, paths: None, supported_quantity: Quantity::One, signing_pubkey: Some(signing_pubkey), }, @@ -264,7 +263,7 @@ macro_rules! offer_derived_metadata_builder_methods { ($secp_context: ty) => { let metadata = Metadata::DerivedSigningPubkey(derivation_material); Self { offer: OfferContents { - chains: None, metadata: Some(metadata), amount: None, description: String::new(), + chains: None, metadata: Some(metadata), amount: None, description: None, features: OfferFeatures::empty(), absolute_expiry: None, issuer: None, paths: None, supported_quantity: Quantity::One, signing_pubkey: Some(node_id), }, @@ -330,7 +329,7 @@ macro_rules! offer_builder_methods { ( /// /// Successive calls to this method will override the previous setting. pub fn description($($self_mut)* $self: $self_type, description: String) -> $return_type { - $self.offer.description = description; + $self.offer.description = Some(description); $return_value } @@ -373,6 +372,10 @@ macro_rules! offer_builder_methods { ( None => {}, } + if $self.offer.amount.is_some() && $self.offer.description.is_none() { + $self.offer.description = Some(String::new()); + } + if let Some(chains) = &$self.offer.chains { if chains.len() == 1 && chains[0] == $self.offer.implied_chain() { $self.offer.chains = None; @@ -551,7 +554,7 @@ pub(super) struct OfferContents { chains: Option>, metadata: Option, amount: Option, - description: String, + description: Option, features: OfferFeatures, absolute_expiry: Option, issuer: Option, @@ -585,7 +588,7 @@ macro_rules! offer_accessors { ($self: ident, $contents: expr) => { /// A complete description of the purpose of the payment. Intended to be displayed to the user /// but with the caveat that it has not been verified in any way. - pub fn description(&$self) -> $crate::util::string::PrintableString { + pub fn description(&$self) -> Option<$crate::util::string::PrintableString> { $contents.description() } @@ -809,8 +812,8 @@ impl OfferContents { self.amount.as_ref() } - pub fn description(&self) -> PrintableString { - PrintableString(&self.description) + pub fn description(&self) -> Option { + self.description.as_ref().map(|description| PrintableString(description)) } pub fn features(&self) -> &OfferFeatures { @@ -954,7 +957,7 @@ impl OfferContents { metadata: self.metadata(), currency, amount, - description: Some(&self.description), + description: self.description.as_ref(), features, absolute_expiry: self.absolute_expiry.map(|duration| duration.as_secs()), paths: self.paths.as_ref(), @@ -1092,10 +1095,9 @@ impl TryFrom for OfferContents { (Some(iso4217_code), Some(amount)) => Some(Amount::Currency { iso4217_code, amount }), }; - let description = match description { - None => return Err(Bolt12SemanticError::MissingDescription), - Some(description) => description, - }; + if amount.is_some() && description.is_none() { + return Err(Bolt12SemanticError::MissingDescription); + } let features = features.unwrap_or_else(OfferFeatures::empty); @@ -1166,7 +1168,7 @@ mod tests { assert!(offer.supports_chain(ChainHash::using_genesis_block(Network::Bitcoin))); assert_eq!(offer.metadata(), None); assert_eq!(offer.amount(), None); - assert_eq!(offer.description(), PrintableString("")); + assert_eq!(offer.description(), None); assert_eq!(offer.offer_features(), &OfferFeatures::empty()); assert_eq!(offer.absolute_expiry(), None); #[cfg(feature = "std")] @@ -1183,7 +1185,7 @@ mod tests { metadata: None, currency: None, amount: None, - description: Some(&String::from("")), + description: None, features: None, absolute_expiry: None, paths: None, @@ -1421,7 +1423,7 @@ mod tests { .description("foo".into()) .build() .unwrap(); - assert_eq!(offer.description(), PrintableString("foo")); + assert_eq!(offer.description(), Some(PrintableString("foo"))); assert_eq!(offer.as_tlv_stream().description, Some(&String::from("foo"))); let offer = OfferBuilder::new(pubkey(42)) @@ -1429,8 +1431,15 @@ mod tests { .description("bar".into()) .build() .unwrap(); - assert_eq!(offer.description(), PrintableString("bar")); + assert_eq!(offer.description(), Some(PrintableString("bar"))); assert_eq!(offer.as_tlv_stream().description, Some(&String::from("bar"))); + + let offer = OfferBuilder::new(pubkey(42)) + .amount_msats(1000) + .build() + .unwrap(); + assert_eq!(offer.description(), Some(PrintableString(""))); + assert_eq!(offer.as_tlv_stream().description, Some(&String::from(""))); } #[test] @@ -1655,6 +1664,14 @@ mod tests { panic!("error parsing offer: {:?}", e); } + let offer = OfferBuilder::new(pubkey(42)) + .description("foo".to_string()) + .amount_msats(1000) + .build().unwrap(); + if let Err(e) = offer.to_string().parse::() { + panic!("error parsing offer: {:?}", e); + } + let mut tlv_stream = offer.as_tlv_stream(); tlv_stream.description = None; @@ -1875,7 +1892,7 @@ mod bolt12_tests { // Malformed: empty assert_eq!( "lno1".parse::(), - Err(Bolt12ParseError::InvalidSemantics(Bolt12SemanticError::MissingDescription)), + Err(Bolt12ParseError::InvalidSemantics(Bolt12SemanticError::MissingSigningPubkey)), ); // Malformed: truncated at type @@ -2000,7 +2017,8 @@ mod bolt12_tests { // Missing offer_description assert_eq!( - "lno1zcss9mk8y3wkklfvevcrszlmu23kfrxh49px20665dqwmn4p72pksese".parse::(), + // TODO: Match the spec once it is updated. + "lno1pqpq86qkyypwa3eyt44h6txtxquqh7lz5djge4afgfjn7k4rgrkuag0jsd5xvxg".parse::(), Err(Bolt12ParseError::InvalidSemantics(Bolt12SemanticError::MissingDescription)), ); -- 2.39.5