-
Notifications
You must be signed in to change notification settings - Fork 367
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
BOLT 12 spec updates #1972
BOLT 12 spec updates #1972
Conversation
Codecov ReportBase: 91.39% // Head: 91.66% // Increases project coverage by
📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more Additional details and impacted files@@ Coverage Diff @@
## main #1972 +/- ##
==========================================
+ Coverage 91.39% 91.66% +0.27%
==========================================
Files 98 98
Lines 57080 58640 +1560
Branches 57080 58640 +1560
==========================================
+ Hits 52167 53755 +1588
+ Misses 4913 4885 -28
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report at Codecov. |
Needs rebase |
6459cb8
to
61c9fcb
Compare
61c9fcb
to
04d89ce
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Didn't bother checking the BOLTs, but code looks good.
5d6469f
04d89ce
to
5d6469f
Compare
@@ -552,22 +550,20 @@ pub enum Quantity { | |||
/// Up to a specific number of items (inclusive). | |||
Bounded(NonZeroU64), | |||
/// One or more items. | |||
/// | |||
/// May be used with `NonZeroU64::new(1)` but prefer to use [`Quantity::One`] if only one item |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you added this comment on the wrong variant. More generally, I'd expect this to tell me why I should use one or the other, not just that I should.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch. Fixed and further documented each variant. PTAL
The spec was modified to allow setting offer_quantity_max explicitly to one. This is to support a use case where more than one item is supported but only one item is left in the inventory. Introduce a Quantity::One variant to replace Quantity::Bounded(1) so the later can be used for the explicit setting.
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.
The offer_metadata was optional but is redundant with invreq_metadata (i.e., payer_metadata) for refunds. It is now disallowed in the spec and was already unsupported by RefundBuilder.
5d6469f
to
22ea505
Compare
A few changes / clarifications to the BOLT 12 spec (lightning/bolts#798) were made after some recent discussions:
offer_quantity_max = 1
invreq_quantity
in refundsoffer_metadata
in refundsThis PR updates the
offers
module to reflect these. Based on #1926.