Expose Bolt11Invoice type in bindings #522
Conversation
|
👋 Thanks for assigning @tnull as a reviewer! |
|
🔔 1st Reminder Hey @valentinewallace! This PR has been waiting for your review. |
src/uniffi_types.rs
Outdated
| self.inner | ||
| } | ||
|
|
||
| pub fn from_string(invoice_str: String) -> Result<Arc<Self>, Error> { |
There was a problem hiding this comment.
Let's move this up as it's essentially a constructor. I think we can also rename this from_str and have it take a &str, if we add [ByRef] to the argument in the UDL.
We could also try if using impl std::str::FromStr for Arc<Bolt11Invoice> .. actually would work, which would be the preferred way (also see https://mozilla.github.io/uniffi-rs/latest/types/interfaces.html#exposing-methods-from-standard-rust-traits)
There was a problem hiding this comment.
Thanks! Tried implementing std::str::FromStr. It failed due to rust's orphan rule - can't implement a foreign trait for a foreign type. Renamed the method to from_str and made the parameter changes.
There was a problem hiding this comment.
It failed due to rust's orphan rule - can't implement a foreign trait for a foreign type.
Huh, but that would only be relevant if you tried implementing FromStr on the wrong Bolt11Invoice, i.e., the one from lightning-invoice, as the one we define here is local.
There was a problem hiding this comment.
Just noticed that I tried to implement for Arc<Bolt11Invoice>. Will give this another try - thanks!
There was a problem hiding this comment.
I investigated this a bit further. From what I understand, UniFFi requires interface types to be Arc-wrapped when crossing the FFI boundary, which is why we need to set it accordingly in bolt11.rs
#[cfg(feature = "uniffi")]
type Bolt11Invoice = Arc<crate::uniffi_types::Bolt11Invoice>;
When our doctest code calls .parse(), Rust tries to infer the target type based on how the value is used later. Since it's passed to functions expecting the Arc-wrapped alias (when the UniFFI flag is on), it tries to parse directly into Arc<Bolt11Invoice> for which we can not implement std::str::FromStr because of the orphan rule (Arc is a foreign type).
I still think the cleanest solution is to implement std::str::FromStr for Bolt11Invoice as we currently do and disable the doctests when uniffi is enabled. What do you think?
There was a problem hiding this comment.
Ah, thanks for the clarification.
I still think the cleanest solution is to implement
std::str::FromStrforBolt11Invoiceas we currently do and disable the doctests when uniffi is enabled. What do you think?
Yeah, I don't mind this approach too much, feel free to go for it.
| @@ -23,6 +23,8 @@ | |||
| //! controlled via commands such as [`start`], [`stop`], [`open_channel`], [`send`], etc.: | |||
| //! | |||
| //! ```no_run | |||
| //! # #[cfg(not(feature = "uniffi"))] | |||
There was a problem hiding this comment.
I still think we can avoid this if we drop the explicit import of Bolt11Invoice alltogether and transition this example to use "INVOICE_STR".parse().unwrap(). If we do this, we should probably switch over the entire example (here and in README from the Type::from_str("..") pattern to use "..".parse() pattern in a prefactor commit (to keep it consistent).
bindings/ldk_node.udl
Outdated
| u64 min_final_cltv_expiry_delta(); | ||
| u64? amount_milli_satoshis(); | ||
| boolean is_expired(); | ||
| u64 duration_since_epoch(); |
There was a problem hiding this comment.
Mhh, it's a bit weird that these are not actually returning a Duration, and we're not stating the unit here. Let's rename them to make more sense.
This is also missing quite a few more variants, some of them are trivial to add (timestamp, fallback_addresses, etc, for example).
alexanderwiederin
force-pushed
the
bolt11-invoice-uniffi
branch
10 times, most recently
from
6352b5d to
15153ef
Compare
|
Please let me know when this is ready for another round of review! |
alexanderwiederin
force-pushed
the
bolt11-invoice-uniffi
branch
5 times, most recently
from
e203658 to
be1d8a1
Compare
…bindings - Convert Bolt11Invoice from string type to full interface in UDL - Define required Bolt11Invoice methods in the UDL interface (expiry_time_seconds, min_final_cltv_expiry_delta, amount_milli_satoshis, is_expired, etc.) - Create Bolt11Invoice struct in uniffi_types.rs to wrap LdkBolt11Invoice - Implement methods required by the UDL interface - Add From/Into implementations for conversion between wrapper and LDK types - Add tests for the wrapper struct
- Update type aliases for Bolt11Invoice based on feature flags - Add maybe_convert_invoice and maybe_wrap_invoice functions for type conversion - Update payment code to work with the new wrapper type - Modify liquidity and unified_qr modules to handle wrapped invoices - Skip doctest execution when uniffi feature is enabled to avoid conflicting Bolt11Invoice type assumptions
alexanderwiederin
force-pushed
the
bolt11-invoice-uniffi
branch
from
be1d8a1 to
de87eb0
Compare
First PR for #504.
This PR converts Bolt11Invoice from a string typedef to a full interface in the UDL, providing direct access to invoice properties across language bindings.
The scope has been limited to primitive properties for simplicity, with plans to extend the interface in future PRs.
Changes
Benefits
Future work will extend the interface with more complex properties like route hints and features.