Add `ChannelManager::create_bolt11_invoice` by jkczyz · Pull Request #3389 · lightningdevkit/rust-lightning

jkczyz · 2024-10-29T19:08:07Z

Now that the lightning crate depends on the lightning_invoice crate, the utility functions previously living in the latter can be implemented on ChannelManager. Additionally, the parameters are now moved to a struct in order to remove the increasingly combinatorial blow-up of methods.

The new Bolt11InvoiceParameters is used to determine what values to set in the invoice. Using None for any given parameter results in a reasonable the default or a behavior determined by the ChannelManager as detailed in the documentation.

Fixes#3375

Split Bolt11InvoiceDescription into a version used with references to the description or description hash in the invoice and an owned version of these for when constructing an invoice. The latter is useful as it removes an unnecessary clone and can be used in a future change specifying either a description or description hash in larger set of invoice parameters. Since it doesn't use a reference, it can be exposed in bindings as well.

vincenzopalazzo

LGTM, I like a lot Bolt11InvoiceParameters solution!

CI failure looks like unrelated

TheBlueMatt

Thanks!

TheBlueMatt · 2024-11-05T19:40:37Z

+	/// The description for what the invoice is for, or hash of such description.
+	pub description: Bolt11InvoiceDescription,
+
+	/// The duration since the Unix epoch signifying when the invoice was created. If not set,


Is it worth letting users override this? Not quite sure the use-case here.

The helper utilities allowed it since they didn't use highest seen timestamp in non-std. but maybe that wouldn't allow enough accuracy for invoices with short expirations?

Hmm, if someone needs "high accuracy" on their timeout (in a no-std environment) they should do it by rejecting the payment on the receive end, cause the sender can always send after expiry if they really want.

Hmmm... note that the equivalent behavior in utilities that we deprecate won't be possible with this API. So we can't really leave those as deprecated but still maintain the same behavior without duplicating code. We could remove the parameter from the deprecate methods, but seems we might as well remove them outright if anyone upgrading will need to modify the call site regardless. What do you think?

We could drop (some? all?) the no_std utility methods while deprecating the rest. I don't think this is critical functionality to maintain, and I don't think no_std use of LDK is all that common currently (aside from maybe Lexe? And historically Mutiny, of course) and a little bit of breakage for just Lexe seems fine to clean up the interface for everyone else :)

Dropped in latest push and removed duration_since_epoch parameter.

When creating a default Bolt11InvoiceParameters, having an infallible constructor avoids an unwrap.

jkczyz

Updated the ChannelManager doc examples and added a Default implementation of Bolt11InvoiceParameters.

jkczyz · 2024-11-06T17:34:26Z

+	/// The description for what the invoice is for, or hash of such description.
+	pub description: Bolt11InvoiceDescription,
+
+	/// The duration since the Unix epoch signifying when the invoice was created. If not set,


Hmmm... note that the equivalent behavior in utilities that we deprecate won't be possible with this API. So we can't really leave those as deprecated but still maintain the same behavior without duplicating code. We could remove the parameter from the deprecate methods, but seems we might as well remove them outright if anyone upgrading will need to modify the call site regardless. What do you think?

codecov · 2024-11-06T19:48:09Z

Codecov Report

Attention: Patch coverage is 64.18605% with 77 lines in your changes missing coverage. Please review.

Project coverage is 89.62%. Comparing base (2c1e828) to head (7878801).
Report is 398 commits behind head on main.

Files with missing lines	Patch %	Lines
lightning/src/ln/invoice_utils.rs	68.57%	44 Missing ⚠️
lightning/src/ln/channelmanager.rs	58.33%	20 Missing and 5 partials ⚠️
lightning-invoice/src/lib.rs	46.66%	8 Missing ⚠️

Additional details and impacted files

@@ Coverage Diff @@## main #3389 +/- ##
==========================================
+ Coverage 89.57% 89.62% +0.04% 
==========================================
Files 125 128 +3 Lines 101756 104907 +3151 Branches 101756 104907 +3151 ==========================================
+ Hits 91151 94019 +2868 - Misses 7884 8175 +291 + Partials 2721 2713 -8

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

jkczyz

Latest push fixes broken doc links.

valentinewallace · 2024-11-07T18:58:00Z

CI is still sad

jkczyz · 2024-11-07T19:05:51Z

Squashed fixups and fixed a no-std test compilation failure.

valentinewallace

Basically LGTM

valentinewallace · 2024-11-07T21:03:31Z

+ let currency = match currency {
+ Some(currency) => currency,
+ None => Network::from_chain_hash(self.chain_hash).map(Into::into).unwrap_or(Currency::Bitcoin),
+ };


I'm wondering whether it makes sense to support a currency other than the one corresponding to ChannelManager::chain_hash. It won't be receivable by this ChannelManager, IIUC. Kinda seems like a bug that it was supported previously?

Dropping it seems reasonable. @TheBlueMatt Any objections? We'd have to drop it from the deprecated utils, too.

Makes sense to me.

Currency now removed.
git diff d7e897c7e266d392149aaec7a99e3aa140af5be5..7878801be79928e0309c079ea117d33a2521b557

The upcoming ChannelManager::create_bolt11_invoice will not support setting a specific creation time, so remove that functionality from the invoice_utils functions. This will avoid duplicate code when deprecating.

When creating an invoice using a ChannelManager, payments for a specific ChainHash / Network are only valid. Use the one from the ChannelManager instead of allowing arbitrary ones in the form of a Currency.

Now that the lightning crate depends on the lightning_invoice crate, the utility functions previously living in the latter can be implemented on ChannelManager. Additionally, the parameters are now moved to a struct in order to remove the increasingly combinatorial blow-up of methods. The new Bolt11InvoiceParameters is used to determine what values to set in the invoice. Using None for any given parameter results in a reasonable the default or a behavior determined by the ChannelManager as detailed in the documentation.

The utility methods in in invoice_utils will be removed or deprecated in an upcoming commit.

ChannelManager::create_bolt11_invoice is a simpler and more flexible way of creating a BOLT11 invoice, so deprecate the corresponding functions in the invoice_utils module.

Bolt11InvoiceParameters allows for setting currency and duration_since_epoch. If currency is not set, test that the one corresponding to ChannelManager's chain hash is usd. If duration_since_epoch, is not set then highest seen timestamp is used in non-std compilations.

Update ChannelManager docs to use create_bolt11_invoice and correct references to modules in the lightning-invoice crate that no longer exist.

TheBlueMatt · 2024-11-11T14:24:04Z

+ let duration_since_epoch = {
+ use std::time::SystemTime;
+ SystemTime::now().duration_since(SystemTime::UNIX_EPOCH)
+ .expect("for the foreseeable future this shouldn't happen")


nit: this panics when time is before 1970, not too far forwards :)

TheBlueMatt · 2024-11-11T14:34:55Z

+ };
+ #[cfg(not(feature = "std"))]
+ let duration_since_epoch =
+ Duration::from_secs(self.highest_seen_timestamp.load(Ordering::Acquire) as u64);


It'd be nice to write up a comment here on why its okay to use the timestamp "raw". It could be two hours in the future but generally only like 10-30 minutes in the past. Maybe we should handle short-ish timestamps for no-std clients but its probably fine, just would be good to write down why we think that's okay.

vincenzopalazzo approved these changes Oct 30, 2024
View reviewed changes

TheBlueMatt reviewed Nov 5, 2024
View reviewed changes

jkczyz added 2 commits November 6, 2024 11:53

Add Description::empty constructor
65790b6
When creating a default Bolt11InvoiceParameters, having an infallible constructor avoids an unwrap.

Combine import statements
2a95402

jkczyz force-pushed the 2024-10-bolt11-invoice-utils branch from 2c6a75a to 8b48b47Compare November 6, 2024 19:37

jkczyz commented Nov 6, 2024
View reviewed changes

jkczyz force-pushed the 2024-10-bolt11-invoice-utils branch 2 times, most recently from ffcefff to a9bc217Compare November 7, 2024 14:31

jkczyz commented Nov 7, 2024
View reviewed changes

jkczyz requested a review from valentinewallace November 7, 2024 18:41

jkczyz force-pushed the 2024-10-bolt11-invoice-utils branch from a9bc217 to 998b081Compare November 7, 2024 19:05

valentinewallace reviewed Nov 7, 2024
View reviewed changes

Remove no-std support from invoice_utils functions
0ea225c
The upcoming ChannelManager::create_bolt11_invoice will not support setting a specific creation time, so remove that functionality from the invoice_utils functions. This will avoid duplicate code when deprecating.

jkczyz force-pushed the 2024-10-bolt11-invoice-utils branch from 998b081 to d7e897cCompare November 7, 2024 23:17

jkczyz added 6 commits November 8, 2024 11:44

Remove currency support from invoice_utils
284cb28
When creating an invoice using a ChannelManager, payments for a specific ChainHash / Network are only valid. Use the one from the ChannelManager instead of allowing arbitrary ones in the form of a Currency.

Use ChannelManager::create_bolt11_invoice in tests
ce0e182
The utility methods in in invoice_utils will be removed or deprecated in an upcoming commit.

Deprecate functions in invoice_utils module
b6ceafb
ChannelManager::create_bolt11_invoice is a simpler and more flexible way of creating a BOLT11 invoice, so deprecate the corresponding functions in the invoice_utils module.

Update BOLT11 docs in ChannelManager
7878801
Update ChannelManager docs to use create_bolt11_invoice and correct references to modules in the lightning-invoice crate that no longer exist.

jkczyz force-pushed the 2024-10-bolt11-invoice-utils branch from d7e897c to 7878801Compare November 8, 2024 18:15

valentinewallace approved these changes Nov 8, 2024
View reviewed changes

TheBlueMatt approved these changes Nov 11, 2024
View reviewed changes

TheBlueMatt merged commit 739c412 into lightningdevkit:mainNov 11, 2024

jkczyz mentioned this pull request Nov 11, 2024
Follow-ups for ChannelManager::create_bolt11_invoice#3405
Merged

TheBlueMatt mentioned this pull request Jan 13, 2025
Deprecate invoice_utils#3529
Closed

Uh oh!

Conversation

jkczyz commented Oct 29, 2024

Uh oh!

vincenzopalazzo left a comment

Choose a reason for hiding this comment

Uh oh!

TheBlueMatt left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jkczyz left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codecovBot commented Nov 6, 2024• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

jkczyz left a comment

Choose a reason for hiding this comment

Uh oh!

valentinewallace commented Nov 7, 2024

Uh oh!

jkczyz commented Nov 7, 2024

Uh oh!

valentinewallace left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

codecovBot commented Nov 6, 2024•
edited
Loading