Expose Bolt11Invoice type in bindings #522

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.

Sign up for GitHub

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

Jump to bottom

Merged

tnull merged 1 commit into lightningdevkit:main from alexanderwiederin:bolt11-invoice-uniffi

May 12, 2025

+451 −40

Contributor

alexanderwiederin commented Apr 14, 2025 •

edited

Loading

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

Added Bolt11Invoice interface to UDL with core methods
Created wrapper struct in uniffi_types.rs with proper implementations
Updated payment code to work with the new wrapper type
Disabled doctests when uniffi feature is enabled to avoid conflicts with different Bolt11Invoice type representation
Added tests verifying property preservation during conversion

Benefits

Direct access to invoice properties from FFI languages
Consistent API across languages

ldk-reviews-bot commented Apr 14, 2025 •

edited

Loading

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

alexanderwiederin marked this pull request as ready for review

April 14, 2025 09:27

ldk-reviews-bot requested a review from valentinewallace

April 14, 2025 09:28

ldk-reviews-bot commented Apr 16, 2025

🔔 1st Reminder

Hey @valentinewallace! 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 requested review from tnull and removed request for valentinewallace

April 16, 2025 10:11

tnull reviewed

View reviewed changes

Collaborator

tnull left a comment

Thanks for looking into this!

Approach looks already pretty good, some comments

bindings/ldk_node.udl Show resolved Hide resolved

src/uniffi_types.rs Outdated Show resolved Hide resolved

src/uniffi_types.rs Outdated Show resolved Hide resolved

src/uniffi_types.rs Outdated Show resolved Hide resolved

src/uniffi_types.rs Outdated Show resolved Hide resolved

src/uniffi_types.rs Outdated Show resolved Hide resolved

src/lib.rs Show resolved Hide resolved

src/liquidity.rs Outdated Show resolved Hide resolved

src/payment/bolt11.rs Outdated Show resolved Hide resolved

bindings/ldk_node.udl Outdated Show resolved Hide resolved

alexanderwiederin force-pushed the bolt11-invoice-uniffi branch 10 times, most recently from 6352b5d to 15153ef Compare

April 22, 2025 17:01

Collaborator

tnull commented Apr 22, 2025

Please let me know when this is ready for another round of review!

alexanderwiederin force-pushed the bolt11-invoice-uniffi branch 9 times, most recently from f2552f4 to e2f37df Compare

April 24, 2025 21:27

Contributor Author

alexanderwiederin commented Apr 24, 2025

@tnull, when you have time, could you take another look? I believe the check failures are not related.

alexanderwiederin force-pushed the bolt11-invoice-uniffi branch from e2f37df to 96fc245 Compare

April 25, 2025 13:38

Contributor Author

alexanderwiederin commented Apr 25, 2025 •

edited

Loading

The first commit won't build on its own because of the function signatures in the bindings. Should I squash to one single commit?

alexanderwiederin requested a review from tnull

April 28, 2025 13:30

tnull reviewed

View reviewed changes

Collaborator

tnull left a comment

Excuse the delay here! Generally looks good, just two minor comments.

The first commit won't build on its own because of the function signatures in the bindings. Should I squash to one single commit?

Yes, if the commits don't build by themselves, it's preferable to squash, thank you!

src/uniffi_types.rs Show resolved Hide resolved

src/payment/bolt11.rs Outdated Show resolved Hide resolved

tnull mentioned this pull request

Add support for sending to human-readable names (BIP 353) #528

Open

alexanderwiederin force-pushed the bolt11-invoice-uniffi branch 2 times, most recently from a210c5e to efe36fb Compare

April 30, 2025 13:22

alexanderwiederin requested a review from tnull

May 1, 2025 10:57

alexanderwiederin force-pushed the bolt11-invoice-uniffi branch 2 times, most recently from 422a81b to 0efa3f9 Compare

May 1, 2025 14:55

ldk-reviews-bot commented May 3, 2025

🔔 1st Reminder

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

ldk-reviews-bot commented May 5, 2025

🔔 2nd Reminder

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

ldk-reviews-bot commented May 7, 2025

🔔 3rd Reminder

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

ldk-reviews-bot commented May 10, 2025

🔔 4th Reminder

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

ldk-reviews-bot commented May 12, 2025

🔔 5th Reminder

Hey @tnull! 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 reviewed

View reviewed changes

Collaborator

tnull left a comment

Excuse the delay here!

This basically looks good to me, just one comment that needs to be addressed, the DRYing up can happen in any of the follow-up PRs.

src/payment/bolt11.rs Outdated Show resolved Hide resolved

src/payment/bolt11.rs

+              type Bolt11Invoice = Arc<crate::uniffi_types::Bolt11Invoice>;
+              #[cfg(not(feature = "uniffi"))]
+              pub(crate) fn maybe_wrap_invoice(invoice: LdkBolt11Invoice) -> Bolt11Invoice {

Collaborator

tnull May 12, 2025

I'm starting to wonder if we really need type-dependant methods for everything we wrap. I think we should look to DRY them up, e.g., to be reused for Offer,Bolt12Invoice, Refund, etc.

Probably fine to leave as-is here, but in the next PR we should start DRYing the wrapping code where we can (i.e., will probably also make sense to move these to uniffi_types then).

Contributor Author

alexanderwiederin May 12, 2025

Yea, good point. Will try a couple of things for the types to come


          feat: Add Bolt11Invoice wrapper for FFI bindings

f8e758d

Implement Bolt11Invoice struct in uniffi_types to provide a wrapper
around LDK's Bolt11Invoice for cross-language bindings.

Modified payment handling in bolt11.rs to:
- Support both native and FFI-compatible invoice types via type aliasing
- Add maybe_wrap_invoice and maybe_convert_invoice helper functions
- Implement conditional compilation for transparent FFI support
- Update all payment functions to handle wrapped invoice types

Integrated with unified_qr.rs to ensure consistent invoice handling
across the QR code generation and payment workflows.

Functionality tested with test coverage to ensure that data does not
change when wrapping/unwrapping.

alexanderwiederin force-pushed the bolt11-invoice-uniffi branch from 0efa3f9 to f8e758d Compare

May 12, 2025 12:33

tnull approved these changes

View reviewed changes

Collaborator

tnull left a comment

LGTM

tnull merged commit b914731 into lightningdevkit:main

15 checks passed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet