Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
26 changes: 26 additions & 0 deletions lightning/src/offers/invoice_request.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2040,6 +2040,32 @@ mod tests {
Err(e) => assert_eq!(e, Bolt12SemanticError::MissingAmount),
}

// An offer with amount_msats(0) is normalized to None, so invoice request must provide amount.
match OfferBuilder::new(recipient_pubkey())
.amount_msats(0)
.build()
.unwrap()
.request_invoice(&expanded_key, nonce, &secp_ctx, payment_id)
.unwrap()
.build_and_sign()
{
Ok(_) => panic!("expected error"),
Err(e) => assert_eq!(e, Bolt12SemanticError::MissingAmount),
}

// But providing an amount in the invoice request should succeed.
let invoice_request = OfferBuilder::new(recipient_pubkey())
.amount_msats(0)
.build()
.unwrap()
.request_invoice(&expanded_key, nonce, &secp_ctx, payment_id)
.unwrap()
.amount_msats(1000)
.unwrap()
.build_and_sign()
.unwrap();
assert_eq!(invoice_request.amount_msats(), Some(1000));

match OfferBuilder::new(recipient_pubkey())
.amount_msats(1000)
.supported_quantity(Quantity::Unbounded)
Expand Down
53 changes: 53 additions & 0 deletions lightning/src/offers/offer.rs
Original file line number Diff line number Diff line change
Expand Up @@ -405,6 +405,10 @@ macro_rules! offer_builder_methods { (
if amount_msats > MAX_VALUE_MSAT {
return Err(Bolt12SemanticError::InvalidAmount);
}
// An amount of 0 is equivalent to not setting an amount, so default to None.
if amount_msats == 0 {
$self.offer.amount = None;
}
Comment on lines +408 to +411
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm somewhat unsure about this change. It could hide an error on the user's end. It might be better to fail in this case instead of allowing them to accept any amount.

},
Some(Amount::Currency { .. }) => return Err(Bolt12SemanticError::UnsupportedCurrency),
None => {},
Expand Down Expand Up @@ -1309,8 +1313,12 @@ impl TryFrom<FullOfferTlvStream> for OfferContents {
(None, Some(amount_msats)) if amount_msats > MAX_VALUE_MSAT => {
return Err(Bolt12SemanticError::InvalidAmount);
},
// An amount of 0 is equivalent to not setting an amount, so default to None.
(None, Some(0)) => None,
Copy link
Contributor

Choose a reason for hiding this comment

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

Hmmm... I'd rather we not use the same representation for two different serializations. It may still work because when creating the InvoiceRequest we'll serialize using Offer::bytes. If we didn't use that, the issuer may fail to authenticate the offer when receiving an InvoiceRequest.

(None, Some(amount_msats)) => Some(Amount::Bitcoin { amount_msats }),
(Some(_), None) => return Err(Bolt12SemanticError::MissingAmount),
// An amount of 0 with a currency is equivalent to not setting an amount.
(Some(_), Some(0)) => None,
Comment on lines +1320 to +1321
Copy link
Contributor

Choose a reason for hiding this comment

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

This prevents the currency code check. Also, I don't think it's useful to drop information from the Offer even if we still retain it in Offer::bytes. At very least it would be surprising if viewing an Offer that is suppose to have a currency but LDK shows that it doesn't.

(Some(currency_bytes), Some(amount)) => {
let iso4217_code = CurrencyCode::new(currency_bytes)
.map_err(|_| Bolt12SemanticError::InvalidCurrencyCode)?;
Expand Down Expand Up @@ -1702,6 +1710,21 @@ mod tests {
Ok(_) => panic!("expected error"),
Err(e) => assert_eq!(e, Bolt12SemanticError::InvalidAmount),
}

// An amount of 0 is equivalent to not setting an amount and should default to None.
let offer = OfferBuilder::new(pubkey(42)).amount_msats(0).build().unwrap();
assert_eq!(offer.amount(), None);
assert_eq!(offer.as_tlv_stream().0.amount, None);

// Verify roundtrip: offer with amount=0 serializes and parses back with amount=None.
let serialized = offer.to_string();
match serialized.parse::<Offer>() {
Ok(reparsed) => {
assert_eq!(reparsed.amount(), None);
assert_eq!(reparsed.as_tlv_stream().0.amount, None);
},
Err(e) => panic!("error parsing offer: {:?}", e),
}
}

#[test]
Expand Down Expand Up @@ -1974,6 +1997,36 @@ mod tests {
Bolt12ParseError::InvalidSemantics(Bolt12SemanticError::InvalidCurrencyCode)
),
}

// An amount of 0 is equivalent to not setting an amount and should default to None.
let mut tlv_stream = offer.as_tlv_stream();
tlv_stream.0.amount = Some(0);
tlv_stream.0.currency = None;

let mut encoded_offer = Vec::new();
tlv_stream.write(&mut encoded_offer).unwrap();

match Offer::try_from(encoded_offer) {
Ok(offer) => {
assert_eq!(offer.amount(), None);
},
Err(e) => panic!("error parsing offer: {:?}", e),
}

// An amount of 0 with a currency is also equivalent to not setting an amount.
let mut tlv_stream = offer.as_tlv_stream();
tlv_stream.0.amount = Some(0);
tlv_stream.0.currency = Some(b"USD");

let mut encoded_offer = Vec::new();
tlv_stream.write(&mut encoded_offer).unwrap();

match Offer::try_from(encoded_offer) {
Ok(offer) => {
assert_eq!(offer.amount(), None);
},
Err(e) => panic!("error parsing offer: {:?}", e),
}
}

#[test]
Expand Down