-
Notifications
You must be signed in to change notification settings - Fork 116
[develop] Upgrade to latest RL and drop BOLT 12 quantity #642
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
base: main
Are you sure you want to change the base?
Conversation
I've assigned @tnull as a reviewer! |
Oops, I missed #639. Anyway, I think we should drop the quantity arguments. |
The BOLT 12 `quantity` field is incredibly unlikely to get any use in practice (it assumes an online store which places some kind of static offer for each item on its site, assuming that the merchant doesn't want structured customer information nor the ability to sell more than one item in a single order, both of which do not exist in practice). Worse, supporting it requires an entire UI flow built around the "quantity" concept, something which is a nontrivial investment for downstream users of ldk-node. Because the cost/utility tradeoff isn't nearly worth it, it was dropped from the main offer-payment API in the upstream `lightning` crate (requiring a separate `ChannelManager::pay_for_offer_with_quantity` call). For the same reason, we simply drop it from our API entirely here. Absent someone who actually wants to use the `quantity` logic, there is really no reason to support it.
7be11c3
to
938ac6d
Compare
938ac6d
to
949287b
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.
Hmm, I agree that quantity
is very confusing, but given it's a BOLT12 feature that other implementations might use, I'm not fully sure if we can get rid of it? Are we certain this doesn't lead to even more interop issues / confusion down the line?
Also added a few comments, this will also need a rebase since we landed #639 first as per @joostjager's request to fix develop
breakage ASAP to be able to make progress.
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.
Please drop these changes to the static Swift files. These will only be updated/regenerated during release (though here on develop
it doesn't matter as much as on main
, which are actually part of the SwiftPM dependency).
if let Some(qty) = quantity { | ||
if qty == 0 { | ||
log_error!(self.logger, "Failed to create offer: quantity can't be zero."); | ||
return Err(Error::InvalidQuantity); |
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.
We can now also drop the InvalidQuantity
error variant.
src/payment/bolt12.rs
Outdated
match self.channel_manager.pay_for_offer( | ||
&offer, | ||
quantity, | ||
if offer.expects_quantity() { Some(1) } else { None }, |
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.
Does this do the right thing internally w.r.t Quantity::One
vs Quantity::Bounded(1)
?
We decided to make the |
Also upgrades to latest RL.