Feature: Add Bolt12 Uniffi Type Wrappers #542

alexanderwiederin · 2025-05-19T09:55:50Z

Second PR for #504.

This PR adds full Offer-, Refund- and Bolt12Invoice- types for the FFI bindings.

Changes:

Type definitions in ldk_node.udl and uniffi_types.rs
Implementation of UniffiType trait to standardize conversions between LDK types and their FFI-compatible wrappers in uniffi_conversions.rs
Integration of wrapper types in payment handling methods in bolt12.rs and unified_qr.rs

Benefits:

Direct access to Offer, Refund and Bolt12Invoice properties from FFI languages
Consistent API across languages

Note:

Appreciate that this PR is big. Happy to break down into smaller parts if preferred.

ldk-reviews-bot · 2025-05-19T09:55:52Z

👋 Thanks for assigning @enigbe 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.

alexanderwiederin · 2025-05-19T11:53:20Z

src/uniffi_conversions.rs

+			match LdkBolt11InvoiceDescription::try_from(self) {
+				Ok(ldk_description) => ldk_description,
+				Err(e) => {
+					panic!("Failed to convert Bolt11InvoiceDescription to LDK type: {:?}", e)


Not a fan of this myself. We could refactor the Bolt11InvoiceDescription to be a struct that holds the LdkType in an inner field (similar to what we did to other types) to make this conversion safer. Looking for advice on whether it's worth introducing a breaking change for this!

Hmm, indeed, if we're going to unify the conversions, it would make sense to align LdkBolt11InvoiceDescription. Generally, we're still on 0.X, so don't care about breaking API all too much. It's much more important to add features and figure out the best API over time at this point.

But do you think LdkBolt11InvoiceDescription would even work as expected?

It would work differently. I was thinking something like this:

pub struct Bolt11InvoiceDescription { inner: lightning_invoice::Bolt11InvoiceDescription, } impl Bolt11InvoiceDescription { pub fn direct_description(&self) -> Option<String> { match &self.inner { lightning_invoice::Bolt11InvoiceDescription::Direct(description) => { Some(description.to_string()) }, _ => None, } } pub fn hash(&self) -> Option<String> { match &self.inner { lightning_invoice::Bolt11InvoiceDescription::Hash(hash) => { Some(hex_utils::to_string(hash.0.as_ref())) }, _ => None, } } }

Not ideal, because as you say it works differently, but safe. What do you think?

Mhh, but is something stopping us from splitting this up in UnffiConversionType and UniffiFallibleConversionType, where the latter would have a maybe_try_convert(value: &T, error: Error) method that would return and bubble up that error if conversion fails?

FWIW, we could even have them inherit all the other behavior from UniffiType, i.e.

trait UniffiConversionType: UniffiType or similar, no?

Deviated from your suggestion, but think I have something neat - let me know what you think! Thanks for the pointer.

tnull

Thanks, already looks pretty good! Did a first higher-level pass.

bindings/ldk_node.udl

tnull · 2025-05-22T15:22:32Z

src/uniffi_conversions.rs

+			match LdkBolt11InvoiceDescription::try_from(self) {
+				Ok(ldk_description) => ldk_description,
+				Err(e) => {
+					panic!("Failed to convert Bolt11InvoiceDescription to LDK type: {:?}", e)


Hmm, indeed, if we're going to unify the conversions, it would make sense to align LdkBolt11InvoiceDescription. Generally, we're still on 0.X, so don't care about breaking API all too much. It's much more important to add features and figure out the best API over time at this point.

But do you think LdkBolt11InvoiceDescription would even work as expected?

src/uniffi_conversions.rs

src/uniffi_types.rs

bindings/ldk_node.udl

tnull · 2025-05-29T10:32:37Z

src/uniffi_types.rs

+	}
+}
+
+impl std::str::FromStr for Bolt12Invoice {


Hmm, I think we should wait for the resolution of the discussion over at lightningdevkit/rust-lightning#3800 before we do this and roll our own custom serialization.

Hmm, AFAICT the resolution was to use the wire format for now. I'm not super happy about that, but okay.

Mmh, so what does this mean for us? We need to implement Writable and Readable for Bolt12Invoice?

src/uniffi_conversions.rs

tnull · 2025-05-29T10:44:38Z

src/uniffi_conversions.rs

+			match LdkBolt11InvoiceDescription::try_from(self) {
+				Ok(ldk_description) => ldk_description,
+				Err(e) => {
+					panic!("Failed to convert Bolt11InvoiceDescription to LDK type: {:?}", e)


Mhh, but is something stopping us from splitting this up in UnffiConversionType and UniffiFallibleConversionType, where the latter would have a maybe_try_convert(value: &T, error: Error) method that would return and bubble up that error if conversion fails?

FWIW, we could even have them inherit all the other behavior from UniffiType, i.e.

trait UniffiConversionType: UniffiType or similar, no?

tnull

Cool, I like the proposed approach. Very clean!

Just a few comments.

src/ffi/conversions.rs

tnull · 2025-06-03T14:09:30Z

src/ffi/conversions.rs

@@ -50,6 +51,20 @@ mod uniffi_impl {
 		}
 	}

+	impl UniffiType for Arc<Offer> {


Why are we implementing this on Arc<Offer>, not on Offer? I think we should do the latter if possible.

Thanks for this!

Initially, I implemented these on the Arcs because that's how they are required to pass the ffi boundary. I looked into it again and now have a solution that Arc-allocates/-extracts the types in the conversion functions (i.e. maybe_convert, maybe_wrap etc.) and only does type conversion on the implementation methods. The benefit is that we can use standard traits and remove ffi/conversions.rs entirely. Downside is that the conversion functions become slightly more complex.

I have implemented these changes on alexanderwiederin@6b5f297 for better comparison.

What do you think?

Looks a lot less invasive. However, there seem to be a few cases where we add unnecessary clones now that could lead to addtional memory usage and heap fragmentation. Can we avoid cloning when not necessary, and also enforce in the types of maybe_convert etc, that we'd always use Arc::clone(&..), ie., that we'd only ever clone the reference, and otherwise would maintain noops or move semantics?

src/ffi/types.rs

tnull · 2025-06-03T14:12:29Z

src/ffi/conversions.rs

@@ -65,6 +65,20 @@ mod uniffi_impl {
 		}
 	}

+	impl UniffiType for Arc<Refund> {


Same here, can we implement this rather for Refund directly?

This commit reorganizes the FFI architecture by introducing conversion traits for lightning types. Moves code from uniffi_types.rs to a dedicated ffi module for separation of concerns.

Implement Offer struct in ffi/types.rs to provide a wrapper around LDK's Offer for cross-language bindings. Modified payment handling in bolt12.rs to: - Support both native and FFI-compatible types via type aliasing - Implement conditional compilation for transparent FFI support - Update payment functions to handle wrapped types Added testing to verify that properties are preserved when wrapping/unwrapping between native and FFI types.

Implement Refund struct in ffi/types.rs to provide a wrapper around LDK's Refund for cross-language bindings. Modified payment handling in bolt12.rs to: - Support both native and FFI-compatible types via type aliasing - Implement conditional compilation for transparent FFI support - Update payment functions to handle wrapped types Added testing to verify that properties are preserved when wrapping/unwrapping between native and FFI types.

Implement Bolt12Invoice struct in ffi/types.rs to provide a wrapper around LDK's Bolt12Invoice for cross-language bindings. Modified payment handling in bolt12.rs to: - Support both native and FFI-compatible types via type aliasing - Implement conditional compilation for transparent FFI support - Update payment functions to handle wrapped types Added testing to verify that properties are preserved when wrapping/unwrapping between native and FFI types.

ldk-reviews-bot · 2025-06-04T12:53:35Z

🔔 1st Reminder

Hey @enigbe! 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.

alexanderwiederin commented May 19, 2025

View reviewed changes

alexanderwiederin force-pushed the uniffi-bolt12-clean branch from f07aa51 to e85a185 Compare May 20, 2025 19:25

alexanderwiederin marked this pull request as ready for review May 20, 2025 21:21

ldk-reviews-bot requested a review from valentinewallace May 20, 2025 21:22

alexanderwiederin force-pushed the uniffi-bolt12-clean branch from e85a185 to 7188bca Compare May 22, 2025 08:27

tnull requested review from tnull and removed request for valentinewallace May 22, 2025 15:13

tnull reviewed May 22, 2025

View reviewed changes

alexanderwiederin force-pushed the uniffi-bolt12-clean branch from 7188bca to 2f3bc2e Compare May 25, 2025 15:35

enigbe reviewed May 28, 2025

View reviewed changes

src/uniffi_types.rs Outdated Show resolved Hide resolved

alexanderwiederin force-pushed the uniffi-bolt12-clean branch from 2f3bc2e to 670a885 Compare May 28, 2025 13:48

tnull mentioned this pull request May 29, 2025

offers: parse invoice and invoice request lightningdevkit/rust-lightning#3800

Closed

tnull reviewed May 29, 2025

View reviewed changes

alexanderwiederin force-pushed the uniffi-bolt12-clean branch from 670a885 to 2894d87 Compare June 2, 2025 11:49

alexanderwiederin requested review from tnull and enigbe June 2, 2025 12:08

tnull mentioned this pull request Jun 3, 2025

Support passing preimage for spontaneous payments #549

Open

tnull reviewed Jun 3, 2025

View reviewed changes

alexanderwiederin added 4 commits June 3, 2025 20:52

Restructure FFI bindings with conversion traits for Lightning types

abd4bb4

This commit reorganizes the FFI architecture by introducing conversion traits for lightning types. Moves code from uniffi_types.rs to a dedicated ffi module for separation of concerns.

alexanderwiederin force-pushed the uniffi-bolt12-clean branch from 2894d87 to 3126d86 Compare June 3, 2025 20:08

Feature: Add Bolt12 Uniffi Type Wrappers #542

Are you sure you want to change the base?

Feature: Add Bolt12 Uniffi Type Wrappers #542

Conversation

alexanderwiederin commented May 19, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Changes:

Benefits:

Note:

Uh oh!

ldk-reviews-bot commented May 19, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

alexanderwiederin May 19, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

tnull left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

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!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tnull left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alexanderwiederin Jun 4, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

ldk-reviews-bot commented Jun 4, 2025

Uh oh!

Uh oh!

alexanderwiederin commented May 19, 2025 •

edited

Loading

ldk-reviews-bot commented May 19, 2025 •

edited

Loading

alexanderwiederin May 19, 2025 •

edited

Loading

alexanderwiederin Jun 4, 2025 •

edited

Loading