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.

Author: jkczyz

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<PrintableString> {
 		$contents.description()
 	}
 
@@ -952,12 +952,12 @@ impl InvoiceContents {
 		}
 	}
 
-	fn description(&self) -> PrintableString {
+	fn description(&self) -> Option<PrintableString> {
 		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(), &[]);

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()

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<Vec<ChainHash>>,
 	metadata: Option<Metadata>,
 	amount: Option<Amount>,
-	description: String,
+	description: Option<String>,
 	features: OfferFeatures,
 	absolute_expiry: Option<Duration>,
 	issuer: Option<String>,
@@ -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<PrintableString> {
+		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<OfferTlvStream> 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,16 +1423,23 @@ 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))
 			.description("foo".into())
 			.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::<Offer>() {
+			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::<Offer>(),
-			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::<Offer>(),
+			// TODO: Match the spec once it is updated.
+			"lno1pqpq86qkyypwa3eyt44h6txtxquqh7lz5djge4afgfjn7k4rgrkuag0jsd5xvxg".parse::<Offer>(),
 			Err(Bolt12ParseError::InvalidSemantics(Bolt12SemanticError::MissingDescription)),
 		);