From 6236e0d4722cdc9ccf3617f9c2ce73782213c08c Mon Sep 17 00:00:00 2001 From: Jeffrey Czyz Date: Wed, 18 Jan 2023 17:29:31 -0600 Subject: [PATCH] Allow quantity in Refund The spec always allowed this but the reason was unclear. It's useful if the refund is for an invoice paid for offer where a quantity was given in the request. The description in the refund would be from the offer, which may have given a unit for each item. So allowing a quantity makes it clear how many items the refund is for. --- lightning/src/offers/refund.rs | 61 ++++++++++++++++++++++++---------- 1 file changed, 43 insertions(+), 18 deletions(-) diff --git a/lightning/src/offers/refund.rs b/lightning/src/offers/refund.rs index bee6cc7f..f800274c 100644 --- a/lightning/src/offers/refund.rs +++ b/lightning/src/offers/refund.rs @@ -120,7 +120,7 @@ impl RefundBuilder { let refund = RefundContents { payer: PayerContents(metadata), metadata: None, description, absolute_expiry: None, issuer: None, paths: None, chain: None, amount_msats, - features: InvoiceRequestFeatures::empty(), payer_id, payer_note: None, + features: InvoiceRequestFeatures::empty(), quantity: None, payer_id, payer_note: None, }; Ok(RefundBuilder { refund }) @@ -162,6 +162,20 @@ impl RefundBuilder { self } + /// Sets [`Refund::quantity`] of items. This is purely for informational purposes. It is useful + /// when the refund pertains to an [`Invoice`] that paid for more than one item from an + /// [`Offer`] as specified by [`InvoiceRequest::quantity`]. + /// + /// Successive calls to this method will override the previous setting. + /// + /// [`Invoice`]: crate::offers::invoice::Invoice + /// [`InvoiceRequest::quantity`]: crate::offers::invoice_request::InvoiceRequest::quantity + /// [`Offer`]: crate::offers::offer::Offer + pub fn quantity(mut self, quantity: u64) -> Self { + self.refund.quantity = Some(quantity); + self + } + /// Sets the [`Refund::payer_note`]. /// /// Successive calls to this method will override the previous setting. @@ -224,6 +238,7 @@ pub(super) struct RefundContents { chain: Option, amount_msats: u64, features: InvoiceRequestFeatures, + quantity: Option, payer_id: PublicKey, payer_note: Option, } @@ -285,6 +300,11 @@ impl Refund { &self.contents.features } + /// The quantity of an item that refund is for. + pub fn quantity(&self) -> Option { + self.contents.quantity + } + /// A public node id to send to in the case where there are no [`paths`]. Otherwise, a possibly /// transient pubkey. /// @@ -396,7 +416,7 @@ impl RefundContents { chain: self.chain.as_ref(), amount: Some(self.amount_msats), features, - quantity: None, + quantity: self.quantity, payer_id: Some(&self.payer_id), payer_note: self.payer_note.as_ref(), }; @@ -514,11 +534,6 @@ impl TryFrom for RefundContents { let features = features.unwrap_or_else(InvoiceRequestFeatures::empty); - // TODO: Check why this isn't in the spec. - if quantity.is_some() { - return Err(SemanticError::UnexpectedQuantity); - } - let payer_id = match payer_id { None => return Err(SemanticError::MissingPayerId), Some(payer_id) => payer_id, @@ -527,7 +542,7 @@ impl TryFrom for RefundContents { // TODO: Should metadata be included? Ok(RefundContents { payer, metadata, description, absolute_expiry, issuer, paths, chain, amount_msats, - features, payer_id, payer_note, + features, quantity, payer_id, payer_note, }) } } @@ -755,6 +770,24 @@ mod tests { assert_eq!(tlv_stream.chain, Some(&testnet)); } + #[test] + fn builds_refund_with_quantity() { + let refund = RefundBuilder::new("foo".into(), vec![1; 32], payer_pubkey(), 1000).unwrap() + .quantity(10) + .build().unwrap(); + let (_, _, tlv_stream) = refund.as_tlv_stream(); + assert_eq!(refund.quantity(), Some(10)); + assert_eq!(tlv_stream.quantity, Some(10)); + + let refund = RefundBuilder::new("foo".into(), vec![1; 32], payer_pubkey(), 1000).unwrap() + .quantity(10) + .quantity(1) + .build().unwrap(); + let (_, _, tlv_stream) = refund.as_tlv_stream(); + assert_eq!(refund.quantity(), Some(1)); + assert_eq!(tlv_stream.quantity, Some(1)); + } + #[test] fn builds_refund_with_payer_note() { let refund = RefundBuilder::new("foo".into(), vec![1; 32], payer_pubkey(), 1000).unwrap() @@ -888,6 +921,7 @@ mod tests { .path(paths[1].clone()) .chain(Network::Testnet) .features_unchecked(InvoiceRequestFeatures::unknown()) + .quantity(10) .payer_note("baz".into()) .build() .unwrap(); @@ -900,6 +934,7 @@ mod tests { assert_eq!(refund.issuer(), Some(PrintableString("bar"))); assert_eq!(refund.chain(), ChainHash::using_genesis_block(Network::Testnet)); assert_eq!(refund.features(), &InvoiceRequestFeatures::unknown()); + assert_eq!(refund.quantity(), Some(10)); assert_eq!(refund.payer_note(), Some(PrintableString("baz"))); }, Err(e) => panic!("error parsing refund: {:?}", e), @@ -967,16 +1002,6 @@ mod tests { assert_eq!(e, ParseError::InvalidSemantics(SemanticError::UnexpectedSigningPubkey)); }, } - - let mut tlv_stream = refund.as_tlv_stream(); - tlv_stream.2.quantity = Some(10); - - match Refund::try_from(tlv_stream.to_bytes()) { - Ok(_) => panic!("expected error"), - Err(e) => { - assert_eq!(e, ParseError::InvalidSemantics(SemanticError::UnexpectedQuantity)); - }, - } } #[test] -- 2.30.2