-
Notifications
You must be signed in to change notification settings - Fork 110
Feature: Add Bolt12 Uniffi Type Wrappers #542
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
Feature: Add Bolt12 Uniffi Type Wrappers #542
Conversation
👋 Thanks for assigning @enigbe as a reviewer! |
f07aa51
to
e85a185
Compare
e85a185
to
7188bca
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.
Thanks, already looks pretty good! Did a first higher-level pass.
7188bca
to
2f3bc2e
Compare
2f3bc2e
to
670a885
Compare
670a885
to
2894d87
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.
Cool, I like the proposed approach. Very clean!
Just a few comments.
2894d87
to
3126d86
Compare
🔔 1st Reminder Hey @enigbe! This PR has been waiting for your review. |
🔔 2nd Reminder Hey @enigbe! This PR has been waiting for your review. |
🔔 3rd Reminder Hey @enigbe! This PR has been waiting for your review. |
4a4572a
to
1ebcb89
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.
Well done @alexanderwiederin,
I've reviewed this and just have a small question regarding fixed-sized arrays. The ffi/conversion
approach looked good to me, but following your conversation with @tnull has highlighted some gaps in my understanding of Uniffi's internals especially with regards to heap allocation. I'll be looking into that further.
I've also tested this and it LGTM.
#[derive(Debug, Clone, PartialEq, Eq)] | ||
pub enum OfferAmount { | ||
Bitcoin { amount_msats: u64 }, | ||
Currency { iso4217_code: String, amount: u64 }, | ||
} |
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.
Curious about why LdkAmount
is not used directly here. OfferAmount
is basically the same struct except for the iso4217_code
field. Is this because there is no support for fixed-sized arrays?
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.
Yea, that's pretty much it. OfferAmount
provides a binding-friendly representation by using String instead of a fixed-size byte array for iso4217_code. I also don't think you can automatically convert complex types across FFI language boundaries in general.
} | ||
|
||
#[cfg(feature = "uniffi")] | ||
pub fn maybe_wrap<T>(ldk_type: impl Into<T>) -> std::sync::Arc<T> { |
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.
Why is ldk_type
impl Into<T>
and not just T
?
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.
Good question! T
is not actually the type that is passed to maybe_wrap
. impl Into<T>
means that the argument we pass to maybe_wrap
can be any type that implements Into<T>
to convert itself into T
. In our case T
would be the UniFFI type that is needed at the call site.
For example, we pass LDK native types (like lightning_invoice::Bolt11Invoice
), which gets converted to their corresponding UniFFI wrapper type (like ffi::types::Bolt11Invoice
) using .into()
before being wrapped in an Arc.
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.
LGTM I think, mod one nit.
Let me know if you intend to make further changes or if this can land from your side.
1ebcb89
to
affe664
Compare
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.
affe664
to
0dd04ba
Compare
Thanks for the feedback @tnull. This can land from my side! |
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.
0dd04ba
to
f01d021
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.
LGTM
Second PR for #504.
This PR adds full Offer-, Refund- and Bolt12Invoice- types for the FFI bindings.
Changes:
ldk_node.udl
anduniffi_types.rs
uniffi_conversions.rs
bolt12.rs
andunified_qr.rs
Benefits:
Note:
Appreciate that this PR is big. Happy to break down into smaller parts if preferred.