Treat offer_amount of 0 as equivalent to None #4324
Conversation
Per BOLT12, if no specific minimum amount is required, the offer_amount field should be omitted rather than set to zero. Since an amount of zero has no practical meaning (you cannot pay zero), this change normalizes amount=0 to None in both the builder and parser, ensuring consistent internal representation. This normalization allows offers with amount=0 to be handled gracefully rather than being rejected or causing unexpected behavior downstream. Spec clarification pending: lightning/bolts#1314
|
👋 Thanks for assigning @jkczyz as a reviewer! |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #4324 +/- ##
==========================================
+ Coverage 86.61% 86.64% +0.02%
==========================================
Files 158 158
Lines 102730 102782 +52
Branches 102730 102782 +52
==========================================
+ Hits 88984 89052 +68
+ Misses 11328 11315 -13
+ Partials 2418 2415 -3
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
jkczyz
left a comment
jkczyz
left a comment
There was a problem hiding this comment.
Probably worth waiting on feedback from other implementations. Left some concerns of mine. I'd lean more towards disallowing explicit zero amounts.
| return Err(Bolt12SemanticError::InvalidAmount); | ||
| }, | ||
| // An amount of 0 is equivalent to not setting an amount, so default to None. | ||
| (None, Some(0)) => None, |
There was a problem hiding this comment.
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.
| // An amount of 0 with a currency is equivalent to not setting an amount. | ||
| (Some(_), Some(0)) => None, |
There was a problem hiding this comment.
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.
| // An amount of 0 is equivalent to not setting an amount, so default to None. | ||
| if amount_msats == 0 { | ||
| $self.offer.amount = None; | ||
| } |
There was a problem hiding this comment.
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.
|
👋 The first review has been submitted! Do you think this PR is ready for a second reviewer? If so, click here to assign a second reviewer. |
3 participants
Summary
offer_amount=0toNonein both the builder and parser for consistent internal representationamount=0(with or without currency), the amount is normalized toNoneMotivation
Per BOLT12, if no specific minimum amount is required, the
offer_amountfield should be omitted rather than set to zero. This change ensures offers withamount=0are handled gracefully rather than causing unexpected behavior downstream.Spec clarification pending: lightning/bolts#1314
Test plan
amount_msats(0)results inamount() == Noneandas_tlv_stream().0.amount == NoneNonestateamount=0(with and without currency) results inamount() == Noneamount=0requires invoice request to provide amount