Allow building Invoice from Bolt11InvoiceDescriptionRef

[benthecarman]

Small annoyance I found when updating in one of my downstream projects.
Now that there are 2 types of Bolt11InvoiceDescription you can't just
copy one from one invoice to a new one. This makes it so we can again
build an invoice from the ref version.

[ldk-reviews-bot]

👋 Thanks for assigning @tnull as a reviewer!
I'll wait for their review and will help manage the review process.
Once they submit their review, I'll check if a second reviewer would be helpful.

[codecov]

Codecov Report

❌ Patch coverage is 0% with 7 lines in your changes missing coverage. Please review.
✅ Project coverage is 88.82%. Comparing base (ac8f897) to head (9066f62).
⚠️ Report is 29 commits behind head on main.

Files with missing lines	Patch %	Lines
lightning-invoice/src/lib.rs	0.00%	7 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #4012      +/-   ##
==========================================
- Coverage   88.85%   88.82%   -0.04%     
==========================================
  Files         175      175              
  Lines      127710   127767      +57     
  Branches   127710   127767      +57     
==========================================
+ Hits       113478   113486       +8     
- Misses      11675    11712      +37     
- Partials     2557     2569      +12     

Flag	Coverage Δ
fuzzing	21.74% <0.00%> (-0.12%)	⬇️
tests	88.65% <0.00%> (-0.04%)	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

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

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[ldk-reviews-bot]

🔔 1st Reminder

Hey @joostjager! This PR has been waiting for your review.
Please take a look when you have a chance. If you're unable to review, please let us know so we can find another reviewer.

[tnull]

Hmm, tbh. cloning in From (which is intended to be value-to-value conversion) seems a bit anti-idiomatic. It might be cleaner to impl core::borrow::ToOwned?

That said, honestly, the design choice of using Bolt11InvoiceDescriptionRef just leads to a lot ugly/weird code in a bunch of places (see for example the unreachable in fn description, we also have some workaround code in LDK Node (especially also around exposing in bindings), etc).

Given that ~all users will end up cloning the returned strings/hashes at the callsite anyways, I think I'd personally be in favor of just dropping Bolt11InvoiceDescriptionRef and always just returning the cloned/owned values.

(opinions, @TheBlueMatt @jkczyz ?)

[ldk-reviews-bot]

👋 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.

[jkczyz]

Hmm, tbh. cloning in From (which is intended to be value-to-value conversion) seems a bit anti-idiomatic. It might be cleaner to impl core::borrow::ToOwned?

That said, honestly, the design choice of using Bolt11InvoiceDescriptionRef just leads to a lot ugly/weird code in a bunch of places (see for example the unreachable in fn description, we also have some workaround code in LDK Node (especially also around exposing in bindings), etc).

Given that ~all users will end up cloning the returned strings/hashes at the callsite anyways, I think I'd personally be in favor of just dropping Bolt11InvoiceDescriptionRef and always just returning the cloned/owned values.

(opinions, @TheBlueMatt @jkczyz ?)

I'd be more in favor of implementing core::borrow::ToOwned and update the builder to take that. Seems wrong to clone the description if you are calling the method as an accessor without any intention of owning the returned data.

[benthecarman]

It might be cleaner to impl core::borrow::ToOwned?

yeah i agree, problem is we already impl Clone which does that as well, so we'd be have to remove that and change it to give the owned version.

Given that ~all users will end up cloning the returned strings/hashes at the callsite anyways, I think I'd personally be in favor of just dropping Bolt11InvoiceDescriptionRef and always just returning the cloned/owned values.

ack

[jkczyz]

It might be cleaner to impl core::borrow::ToOwned?

yeah i agree, problem is we already impl Clone which does that as well, so we'd be have to remove that and change it to give the owned version.

Hmm... How about add InvoiceBuilder::invoice_description_ref that clones and delegates like InvoiceBuilder::invoice_description does?

Given that ~all users will end up cloning the returned strings/hashes at the callsite anyways, I think I'd personally be in favor of just dropping Bolt11InvoiceDescriptionRef and always just returning the cloned/owned values.

ack

IMO, we shouldn't have an accessor clone especially if it's gonna allocate on the heap.

[benthecarman]

Hmm... How about add InvoiceBuilder::invoice_description_ref that clones and delegates like InvoiceBuilder::invoice_description does?

Changed to that, still probably annoying there isn't a function to convert between the two, this at least solves the annoyance I ran into

[tnull]

Okay, while I still think that we could generally simplify the interface further, I'm happy to move forward with the currently proposed change which is simple enough.

[TheBlueMatt]

This really didn't need a second reviewer.

[TheBlueMatt]

That said, honestly, the design choice of using Bolt11InvoiceDescriptionRef just leads to a lot ugly/weird code in a bunch of places (see for example the unreachable in fn description, we also have some workaround code in LDK Node (especially also around exposing in bindings), etc).

So you just want to drop the accessor to fetch a description? Not quite sure I understand the goal there. The unreachable is because one of two totally separate fields has to be set.

Can you point to awkwardness in ldk-node due to this interface?

[jkczyz]

The problem is trying to build an invoice using the same description/hash from another invoice. Our accessor returns Bolt11InvoiceDescriptionRef, but our builder takes Bolt11InvoiceDescription. Maybe the accessor should return & Bolt11InvoiceDescription so it can simply be cloned? Can't recall why we have the ref type.

[TheBlueMatt]

Because internally its either a description hash field, or a description field. So the reference has to be inside the enum, not on the enum.