Skip to content

Extend API to allow invoice creation with a description hash #438

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

Merged
merged 2 commits into from
Jan 23, 2025

Conversation

joostjager
Copy link
Contributor

@joostjager joostjager commented Jan 21, 2025

Fixes #325

@joostjager joostjager force-pushed the invoice-description-hash branch 2 times, most recently from be6be7e to f958924 Compare January 21, 2025 15:14
@joostjager joostjager force-pushed the invoice-description-hash branch 3 times, most recently from 1eff1ae to 6b131b1 Compare January 23, 2025 12:04
@joostjager joostjager force-pushed the invoice-description-hash branch from 6b131b1 to 7e8a8ab Compare January 23, 2025 12:07
Copy link
Collaborator

@tnull tnull left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! Looks pretty good, but I think we can clean it up a bit further if we follow the approach of #434. Let me know if I should tackle that in a follow-up though!


/// Represents the description of an invoice which has to be either a directly included string or
/// a hash of a description provided out of band.
pub enum Bolt11InvoiceDescription {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's move this type definition to uniffi_types.rs directly.

pub fn receive(
&self, amount_msat: u64, description: &str, expiry_secs: u32,
&self, amount_msat: u64, description: &lightning_invoice::Bolt11InvoiceDescription,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think rather than duplicating all of these, we should be able to follow the same approach as #434, i.e.,

a) add a local type alias

use lightning_invoice::Bolt11InvoiceDescription as LdkBolt11InvoiceDescription;

#[cfg(not(feature = "uniffi"))]
type Bolt11InvoiceDescription = LdkBolt11InvoiceDescription;
#[cfg(feature = "uniffi")]
type Bolt11InvoiceDescription = crate::uniffi_types::Bolt11InvoiceDescription;

above and have the receives use a macro like this:

macro_rules! maybe_convert_description {
	($description: expr) => {{
		#[cfg(not(feature = "uniffi"))]
		{
			$description
		}
		#[cfg(feature = "uniffi")]
		{
			&LdkBolt11InvoiceDescription::try_from($description)?
		}
	}};
}

(could also consider using it in the receive_inner/receive_via_jit_channel_inner, but the former is reused in unified_qr, which complicates things. So probably easier to do the conversion before giving the description to the _inners).

) -> Result<Bolt11Invoice, Error> {
self.receive_inner(Some(amount_msat), description, expiry_secs, None)
}

#[cfg(feature = "uniffi")]
pub fn receive(
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I double checked this as it confused me that it be possible to omit docs here. And indeed it doesn't work: note that if you run cargo doc --features uniffi --open, all the receive variants wouldn't have any docs on them.

For non-Uniffi we forbid missing docs on a crate-wide level (see top of lib.rs), but unfortunately we can't do this under the uniffi feature as some of the generated code doesn't have docs on it, which would have the deny(missing_docs) lint fail. This is why we didn't catch the missing docs at build time with the uniffi feature.

@tnull
Copy link
Collaborator

tnull commented Jan 23, 2025

Feel free to undraft.

@joostjager joostjager marked this pull request as ready for review January 23, 2025 15:31
@tnull
Copy link
Collaborator

tnull commented Jan 23, 2025

Flaky python test is pre-existing. Going ahead and landing this.

@tnull tnull merged commit 92ee62f into lightningdevkit:main Jan 23, 2025
8 of 13 checks passed
tnull added a commit that referenced this pull request Jan 24, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Bolt11Payment::receive* methods should accept a Bolt11InvoiceDescription instead of a &str
2 participants