Fix UniFFI library-mode bindgen and add Kotlin bindings#20
Conversation
|
Important Review skippedBot user detected. To trigger a single review, invoke the You can disable this status message by setting the 📝 WalkthroughWalkthroughThis PR adds Kotlin UniFFI bindings and an Android example, refactors Rust node implementations from trait-based to inherent async methods with Arc-based callbacks and a global Tokio runtime, updates UniFFI export gating, expands Node.js/TypeScript bindings, and adjusts build configs (Cargo/xcframework/gradle). Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
Comment |
- Remove #[uniffi::export] from trait impl blocks (causes "no interface LightningNode" error) - Add inherent method implementations for each node struct when uniffi feature is enabled - Keep LightningNode trait for non-uniffi use cases (Rust consumers) - Add cdylib to crate-type for library-mode bindgen - Create bindings/kotlin directory with uniffi-bindgen helper - Add build.sh script to generate Kotlin bindings - Create example Kotlin project demonstrating usage Co-authored-by: ntheile <1273575+ntheile@users.noreply.github.com>
Address code review feedback by clarifying that inherent impl blocks are required because UniFFI cannot export trait impl blocks. Co-authored-by: ntheile <1273575+ntheile@users.noreply.github.com>
|
@copilot how to build it? |
To build the Kotlin bindings: cd bindings/kotlin
./build.sh --releaseThis will:
Prerequisites:
For Android, you'll also need to cross-compile for the target architecture: # Example for arm64-v8a
cargo build --package lni --features uniffi --release --target aarch64-linux-androidThen copy the generated |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
bindings/lni_react_native/LniReactNativeFramework.xcframework/Info.plist (1)
8-36: Critical: Both library entries reference the same binary path.Both the simulator and device library entries point to the same
BinaryPathandLibraryPath(liblni.a). XCFrameworks require separate binaries for simulator and device variants because they have different ABIs:
- Simulator binaries are built with the simulator target (e.g.,
aarch64-apple-ios-sim)- Device binaries are built with the device target (e.g.,
aarch64-apple-ios)The same binary cannot work correctly for both contexts. Typically, XCFramework directories contain separate binaries:
ios-arm64/liblni.a (device binary) ios-arm64-simulator/liblni.a (simulator binary)While the identifier swap in this change fixes the variant labeling, it doesn't address the underlying issue of both entries pointing to identical binary paths. This will likely cause runtime failures.
🔎 Verification script to check XCFramework structure
#!/bin/bash # Check the actual directory structure of the XCFramework fd -t f "liblni.a" bindings/lni_react_native/LniReactNativeFramework.xcframework/ # Also check for any build scripts that generate this XCFramework rg -nP "xcodebuild.*-create-xcframework|lipo" bindings/lni_react_native/ -A 5 -B 5
🧹 Nitpick comments (10)
bindings/kotlin/build.sh (1)
13-13: Consider adding stricter error handling.The script uses
set -ewhich is good. For even more robust error handling, consider addingset -u(error on undefined variables) andset -o pipefail(propagate errors in pipes).🔎 Proposed enhancement
-set -e +set -euo pipefailbindings/kotlin/example/gradle/wrapper/gradle-wrapper.properties (1)
1-7: Standard Gradle wrapper configuration.The wrapper properties are correctly configured for Gradle 8.5 with distribution validation enabled.
Consider verifying if Gradle 8.5 is the desired version or if a more recent stable version should be used:
#!/bin/bash # Check latest Gradle version curl -s https://services.gradle.org/versions/current | jq -r '.version'bindings/kotlin/example/app/build.gradle.kts (1)
66-66: Update JNA to version 5.18.1.JNA 5.13.0 is outdated; the latest version is 5.18.1 (released September 2025). No known security vulnerabilities were found in 5.13.0, but upgrading ensures access to bug fixes and security improvements from intervening versions.
readme.md (1)
435-568: Comprehensive Kotlin bindings documentation.The new Kotlin bindings section is thorough and well-structured, covering:
- Prerequisites (NDK, Rust targets)
- Linker configuration with clear examples
- Build commands for different scenarios
- Generated output locations
- Practical usage examples with Strike and CLN nodes
- Project integration steps
The documentation provides developers with everything needed to get started with the Kotlin bindings.
Optional: Address markdown formatting hints from static analysis
Static analysis flagged some minor markdown formatting issues:
- Inconsistent heading styles (ATX vs Setext)
- Unordered list indentation inconsistencies
- Headings not starting at line beginning
These don't affect functionality but could improve document consistency. Consider running a markdown formatter like
prettierormarkdownlint --fixto auto-correct these.crates/lni/lnd/lib.rs (1)
150-152: Inconsistent static variable naming convention.
macaroonuses lowercase while other static refs (URL,TEST_PAYMENT_HASH, etc.) useSCREAMING_SNAKE_CASE. Consider renaming toMACAROONfor consistency.Proposed fix
- static ref macaroon: String = { + static ref MACAROON: String = { dotenv().ok(); env::var("LND_MACAROON").expect("LND_MACAROON must be set") };And update the usage on line 169:
- macaroon: macaroon.clone(), + macaroon: MACAROON.clone(),bindings/kotlin/example/app/src/main/kotlin/com/lni/example/MainActivity.kt (1)
508-532: Consider replacingrunBlockingwith a non-blocking pattern.Using
runBlockinginside callbacks blocks the calling thread. While this works for the demo, a more idiomatic approach would be to usekotlinx.coroutines.channels.ChannelorMutableStateFlowto communicate events without blocking.Alternative approach using Channel
// Instead of runBlocking in callbacks: val statusChannel = Channel<InvoiceStatus>(Channel.CONFLATED) val callback = object : OnInvoiceEventCallback { override fun success(transaction: Transaction?) { statusChannel.trySend(InvoiceStatus.Success(transaction)) } // ... similar for pending/failure } // In a coroutine scope, collect from the channel: launch(Dispatchers.Main) { for (status in statusChannel) { onStatusUpdate(status) } }crates/lni/blink/lib.rs (2)
133-133: Unused importstd::thread.This import appears to be unused in the test module.
Proposed fix
use std::env; use std::sync::{Arc, Mutex}; - use std::thread;
267-270: Redundant assertion -Vec::len()is always >= 0.
txns.len()returnsusize, which cannot be negative. Consider replacing with a more meaningful assertion or simply removing it.Proposed fix
Ok(txns) => { dbg!(&txns); - // Validate we can parse transactions - assert!(txns.len() >= 0, "Should contain at least zero transactions"); + // Successfully listed transactions + println!("Listed {} transactions", txns.len()); }crates/lni/phoenixd/lib.rs (1)
114-116: Stub implementation returns empty string.The
decodemethod always returns an empty string. If Phoenixd doesn't support decode functionality, consider returning an appropriate error instead of silently returning empty data, which could mislead callers.🔎 Suggested alternative
pub async fn decode(&self, _str: String) -> Result<String, ApiError> { - Ok("".to_string()) + Err(ApiError::NotSupported { + reason: "decode is not supported by Phoenixd".to_string(), + }) }Verify if Phoenixd API supports decode functionality. If it does, this stub should be implemented; if not, returning an error would be more informative than an empty string.
bindings/lni_nodejs/index.d.ts (1)
177-179: Empty interface flagged by static analysis.Per Biome: "An empty interface is equivalent to
{}." This is auto-generated from Rust'sCloseChannelResponsestruct. If no fields are needed, consider using a type alias in the Rust source, or add a comment/placeholder field to indicate intentional emptiness.Since this is auto-generated, the fix would need to be applied to the corresponding Rust struct definition rather than this TypeScript file.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (42)
Cargo.tomlbindings/kotlin/.gitignorebindings/kotlin/Cargo.tomlbindings/kotlin/README.mdbindings/kotlin/build.shbindings/kotlin/example/README.mdbindings/kotlin/example/app/build.gradle.ktsbindings/kotlin/example/app/src/main/AndroidManifest.xmlbindings/kotlin/example/app/src/main/kotlin/com/lni/example/Main.ktbindings/kotlin/example/app/src/main/kotlin/com/lni/example/MainActivity.ktbindings/kotlin/example/app/src/main/res/values/strings.xmlbindings/kotlin/example/app/src/main/res/values/themes.xmlbindings/kotlin/example/build.gradle.ktsbindings/kotlin/example/gradle.propertiesbindings/kotlin/example/gradle/wrapper/gradle-wrapper.propertiesbindings/kotlin/example/gradlewbindings/kotlin/example/gradlew.batbindings/kotlin/example/settings.gradle.ktsbindings/kotlin/uniffi-bindgen.rsbindings/lni_nodejs/index.d.tsbindings/lni_nodejs/index.jsbindings/lni_nodejs/main.mjsbindings/lni_react_native/LniReactNativeFramework.xcframework/Info.plistbindings/lni_react_native/build.shcrates/lni/Cargo.tomlcrates/lni/blink/api.rscrates/lni/blink/lib.rscrates/lni/cln/api.rscrates/lni/cln/lib.rscrates/lni/lib.rscrates/lni/lnd/api.rscrates/lni/lnd/lib.rscrates/lni/nwc/api.rscrates/lni/nwc/lib.rscrates/lni/phoenixd/api.rscrates/lni/phoenixd/lib.rscrates/lni/speed/api.rscrates/lni/speed/lib.rscrates/lni/strike/api.rscrates/lni/strike/lib.rscrates/lni/types.rsreadme.md
🧰 Additional context used
🧬 Code graph analysis (8)
crates/lni/lnd/api.rs (3)
crates/lni/cln/api.rs (2)
on_invoice_events(805-819)params(282-285)crates/lni/speed/lib.rs (1)
on_invoice_events(119-125)crates/lni/types.rs (1)
on_invoice_events(39-43)
bindings/kotlin/example/app/src/main/kotlin/com/lni/example/Main.kt (1)
bindings/lni_nodejs/index.d.ts (11)
StrikeConfig(57-63)StrikeNode(64-66)CreateInvoiceParams(235-247)ListTransactionsParams(225-230)NwcConfig(48-53)NwcNode(54-56)BlinkConfig(67-73)BlinkNode(74-76)PayInvoiceParams(261-272)OnInvoiceEventParams(273-278)LookupInvoiceParams(231-234)
crates/lni/types.rs (1)
bindings/lni_nodejs/index.d.ts (12)
ClnNode(35-37)LndNode(45-47)PhoenixdNode(13-15)NwcNode(54-56)Transaction(110-124)PayInvoiceParams(261-272)PayInvoiceResponse(209-213)CreateOfferParams(257-260)Offer(248-256)LookupInvoiceParams(231-234)ListTransactionsParams(225-230)OnInvoiceEventParams(273-278)
crates/lni/strike/lib.rs (2)
crates/lni/types.rs (8)
get_info(21-21)create_invoice(22-22)pay_invoice(23-23)create_offer(24-24)lookup_invoice(33-33)list_transactions(34-37)decode(38-38)on_invoice_events(39-43)bindings/lni_nodejs/src/strike.rs (1)
new(18-20)
crates/lni/nwc/lib.rs (2)
crates/lni/nwc/api.rs (10)
get_info(20-78)create_invoice(80-108)pay_invoice(110-134)get_offer(136-139)list_offers(141-144)pay_offer(146-154)lookup_invoice(156-189)list_transactions(191-229)decode(231-234)on_invoice_events(306-318)crates/lni/types.rs (10)
get_info(21-21)create_invoice(22-22)pay_invoice(23-23)get_offer(25-25)list_offers(26-26)pay_offer(27-32)lookup_invoice(33-33)list_transactions(34-37)decode(38-38)on_invoice_events(39-43)
crates/lni/cln/lib.rs (2)
crates/lni/cln/api.rs (3)
get_info(59-135)params(282-285)on_invoice_events(805-819)crates/lni/types.rs (2)
get_info(21-21)on_invoice_events(39-43)
crates/lni/speed/lib.rs (2)
bindings/lni_nodejs/index.d.ts (2)
OnInvoiceEventParams(273-278)Transaction(110-124)bindings/lni_nodejs/src/speed.rs (2)
on_invoice_events(129-146)new(13-15)
crates/lni/phoenixd/lib.rs (1)
crates/lni/types.rs (11)
get_info(21-21)create_invoice(22-22)pay_invoice(23-23)create_offer(24-24)get_offer(25-25)list_offers(26-26)pay_offer(27-32)lookup_invoice(33-33)list_transactions(34-37)decode(38-38)on_invoice_events(39-43)
🪛 Biome (2.1.2)
bindings/lni_nodejs/index.d.ts
[error] 177-179: An empty interface is equivalent to {}.
Safe fix: Use a type alias instead.
(lint/suspicious/noEmptyInterface)
🪛 detekt (1.23.8)
bindings/kotlin/example/app/src/main/kotlin/com/lni/example/MainActivity.kt
[warning] 293-293: String.format("%.8f", balanceBtc) uses implicitly default locale for string formatting.
(detekt.potential-bugs.ImplicitDefaultLocale)
🪛 markdownlint-cli2 (0.18.1)
bindings/kotlin/example/README.md
7-7: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
readme.md
251-251: Heading style
Expected: setext; Actual: atx
(MD003, heading-style)
435-435: Heading style
Expected: setext; Actual: atx
(MD003, heading-style)
435-435: Headings must start at the beginning of the line
(MD023, heading-start-left)
479-479: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
480-480: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
481-481: Unordered list indentation
Expected: 4; Actual: 8
(MD007, ul-indent)
482-482: Unordered list indentation
Expected: 4; Actual: 8
(MD007, ul-indent)
483-483: Unordered list indentation
Expected: 4; Actual: 8
(MD007, ul-indent)
484-484: Unordered list indentation
Expected: 4; Actual: 8
(MD007, ul-indent)
498-498: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
499-499: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
500-500: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
566-566: Heading style
Expected: setext; Actual: atx
(MD003, heading-style)
566-566: Headings must start at the beginning of the line
(MD023, heading-start-left)
🔇 Additional comments (63)
bindings/lni_react_native/build.sh (1)
14-14: This line should remain commented — it's a reference script, not an automated build.The
build.shfile is a manual reference guide for developers, not an automated CI script. The commentedrustup target addline documents an optional manual setup step, consistent with other commented examples (like pod install and iOS example commands). The actual iOS build is handled byyarn ubrn:ios, which likely manages Rust target setup automatically through theubrnbuild system. Developers should follow CONTRIBUTING.md guidance usingyarn example ios.Likely an incorrect or invalid review comment.
Cargo.toml (1)
2-2: LGTM! Workspace configuration properly updated.The addition of
resolver = "2"ensures proper feature unification across the workspace, and adding"bindings/kotlin"correctly integrates the new Kotlin bindings package as a workspace member.Also applies to: 6-6
bindings/kotlin/uniffi-bindgen.rs (1)
1-3: LGTM! Standard uniffi-bindgen wrapper.This follows the standard pattern for creating a custom UniFFI bindgen binary by delegating to the library's CLI entry point.
crates/lni/phoenixd/api.rs (1)
658-658: LGTM! Callback ownership updated for async/shared context.The change from
Box<dyn OnInvoiceEventCallback>toArc<dyn OnInvoiceEventCallback>enables shared ownership semantics, which is appropriate for async contexts where the callback may need to be cloned or shared across tasks. This aligns with the PR's UniFFI bindgen fixes and thread-safety requirements.Note: This is a breaking API change for existing consumers, but it's intentional per the PR objectives.
crates/lni/speed/api.rs (1)
578-578: LGTM! Callback ownership updated for async/shared context.The change from
Box<dyn OnInvoiceEventCallback>toArc<dyn OnInvoiceEventCallback>is consistent with the project-wide refactoring for UniFFI compatibility and provides appropriate shared ownership semantics for async callback usage.bindings/kotlin/Cargo.toml (1)
1-12: Theuniffiversion (0.29.0) is consistent across the workspace, matching the versions used in the mainlnicrate. No compatibility issues present.bindings/kotlin/.gitignore (1)
1-39: LGTM!The .gitignore correctly excludes generated artifacts (Kotlin bindings, native libraries, build outputs) consistent with the build workflow where developers generate bindings and native libraries locally via build.sh.
crates/lni/Cargo.toml (1)
9-9: LGTM!Adding
cdylibis essential for UniFFI bindgen and Android JNI support. The clear comment explaining each crate type's purpose is helpful.bindings/kotlin/example/gradlew.bat (1)
1-92: LGTM!Standard Gradle wrapper script with proper license header. No issues detected.
bindings/kotlin/build.sh (1)
86-113: No issues found with uniffi-bindgen command syntax.The bindgen invocation at line 107 uses the correct syntax for UniFFI 0.29.0. The command
generate --library "$LIB_FILE" --language kotlin --out-dir "$OUTPUT_DIR"matches the documented library mode interface, with all flags (--library,--language,--out-dir) properly supported. Using--package lni-kotlin-bindgeninstead of the standard--bin uniffi-bindgenis a valid variation when the bindgen tool is a local workspace package.bindings/kotlin/example/app/src/main/res/values/strings.xml (1)
1-3: LGTM!Standard Android string resource file with appropriate app name for the example project.
bindings/kotlin/example/gradle.properties (1)
1-5: LGTM!Standard Gradle properties with appropriate settings for an Android/Kotlin project. The JVM heap allocation and encoding settings are reasonable for Android builds.
bindings/kotlin/example/app/src/main/res/values/themes.xml (1)
1-6: LGTM!Standard Android theme configuration with Material Light styling and transparent status bar, which is appropriate for modern Android apps using Compose.
bindings/lni_nodejs/main.mjs (1)
248-250: LGTM!Test invocation toggle is appropriate for development. The Speed node test path is now exercised, consistent with the SpeedNode bindings exported in the updated index.js/index.d.ts.
bindings/kotlin/example/app/src/main/AndroidManifest.xml (1)
1-22: LGTM!Standard Android manifest configuration with appropriate permissions and activity setup. The INTERNET permission is required for LNI API calls, and the exported MainActivity with LAUNCHER intent is correctly configured.
crates/lni/strike/api.rs (1)
686-697: LGTM: Arc enables thread-safe callback sharing.The change from
Box<dyn OnInvoiceEventCallback>toArc<dyn OnInvoiceEventCallback>correctly enables shared ownership of the callback across async tasks. This is appropriate for the polling loop where the callback is moved into a closure that executes across multiple async iterations.crates/lni/blink/api.rs (1)
668-678: LGTM: Consistent Arc-based callback pattern.The callback parameter change to
Arc<dyn OnInvoiceEventCallback>aligns with the project-wide pattern for thread-safe callback sharing. This is consistent with the Strike implementation and appropriate for the async polling context.crates/lni/lnd/api.rs (2)
111-111: LGTM: Conditional UniFFI exports properly implemented.The switch to
cfg_attr(feature = "uniffi", uniffi::export(async_runtime = "tokio"))ensures UniFFI exports are only active when the feature is enabled, addressing the bindgen failure root cause described in the PR objectives.Also applies to: 198-198, 343-343, 359-359, 415-415, 503-503, 525-525
347-347: LGTM: Arc improves thread-safety for callbacks.The callback type change from
Box<dyn OnInvoiceEventCallback>toArc<dyn OnInvoiceEventCallback>enables safe sharing across threads, consistent with similar changes across all node implementations.crates/lni/nwc/api.rs (1)
306-318: LGTM: Callback type aligned with other implementations.The Arc-based callback signature maintains consistency across the codebase and enables thread-safe callback sharing in async contexts.
crates/lni/cln/api.rs (1)
805-819: LGTM: Arc enables safe callback sharing in spawned task.The Arc callback allows the spawned tokio task to share ownership, preventing lifetime issues while maintaining thread-safety.
crates/lni/lib.rs (2)
26-91: LGTM: Macro effectively consolidates trait delegation boilerplate.The
impl_lightning_node!macro provides a clean solution for implementing theLightningNodetrait by delegating to inherent methods. Thecfg(not(feature = "uniffi"))gate ensures this only applies to non-UniFFI builds, aligning with the PR's strategy of using inherent impls for UniFFI and trait impls for Rust consumers.
152-152: LGTM: Conditional UniFFI export properly implemented.The
cfg_attrensuressay_after_with_tokiois only exported to UniFFI when the feature is enabled, maintaining consistency with the broader refactoring strategy.bindings/kotlin/example/app/build.gradle.kts (2)
34-41: LGTM: Java and Kotlin version alignment is correct.Both Java compatibility and Kotlin JVM target are consistently set to version 17, ensuring bytecode compatibility.
51-55: Good practice: Lint configuration documents UniFFI requirement.The comment clearly explains why the
NewApilint is disabled, noting that UniFFI-generated code usesjava.lang.ref.Cleaner(API 33+) but has runtime fallbacks. This is an appropriate workaround.bindings/kotlin/example/settings.gradle.kts (1)
1-18: LGTM! Standard Gradle configuration with best practices.The settings file correctly configures plugin and dependency resolution with centralized repositories. Using
FAIL_ON_PROJECT_REPOSensures reproducible builds by preventing accidental dependency resolution from project-local repositories.bindings/kotlin/example/gradlew (1)
1-249: Standard Gradle wrapper - no review needed.This is an auto-generated Gradle wrapper script from the official Gradle project. These files are standard boilerplate and typically don't require code review.
bindings/lni_nodejs/index.js (1)
313-315: LGTM! InvoiceType export added correctly.The InvoiceType enum is now properly exposed from the native binding, matching the pattern used for other exports. This aligns with the expanded TypeScript definitions that include InvoiceType.
crates/lni/types.rs (3)
4-16: Well-structured conditional compilation for UniFFI.The
cfg(not(feature = "uniffi"))gates properly separate the Rust-native trait-based API from the UniFFI-exported inherent implementations. This allows Rust consumers to continue using theLightningNodetrait while foreign language bindings use generated inherent methods.
417-424: Correct use ofwith_foreignfor foreign trait implementations.The
#[uniffi::export(with_foreign)]attribute enables foreign languages (Kotlin/Swift) to implement theOnInvoiceEventCallbacktrait and pass instances to Rust. This is the recommended UniFFI approach for callback interfaces.
18-44: Breaking change: Callback ownership changed from Box to Arc.The
on_invoice_eventscallback parameter has changed fromBox<dyn OnInvoiceEventCallback>toArc<dyn OnInvoiceEventCallback>. This enables shared ownership and is thread-safe, which aligns with UniFFI requirements for foreign language callback interop.All call sites across the codebase have been properly updated to use
Arc::new(callback). The readme.md example at line 203 correctly demonstrates the updated usage pattern.readme.md (2)
203-203: Example correctly updated to use Arc.The callback example has been updated from
Box::new(callback)toArc::new(callback), matching the API change incrates/lni/types.rs.
251-266: Clear Kotlin/Android example instructions.The added build and run instructions for the Kotlin example are concise and actionable, covering the essential steps for generating bindings, building for Android, and running the example app.
bindings/kotlin/example/app/src/main/kotlin/com/lni/example/Main.kt (3)
1-32: LGTM - Well-structured example demonstrating LNI Kotlin bindings.The main entry point and example orchestration are clear and follow good patterns with proper error handling via try/finally blocks ensuring
node.close()is called.
37-109: Good demonstration of Strike node lifecycle and error handling.The example properly demonstrates configuration, multiple API calls, callback usage, and resource cleanup with
node.close()in the finally block.
200-289: Supplementary example functions are well-documented.
payInvoiceExampleandlookupInvoiceExampleserve as reference implementations. Consider adding a brief comment noting these are available for users to adapt, or demonstrating them with mock data if desired.crates/lni/strike/lib.rs (3)
55-129: Inherent impl block correctly exposes async methods for UniFFI.The migration from trait-based to inherent implementation is well-executed. All methods delegate appropriately to the API layer, and the
on_invoice_eventscallback correctly usesArc<dyn OnInvoiceEventCallback>for thread-safe sharing across async boundaries.
131-133: Correct conditional trait implementation for Rust consumers.The
impl_lightning_node!macro under#[cfg(not(feature = "uniffi"))]ensures Rust consumers can still use theLightningNodetrait interface while UniFFI consumers use the inherent methods directly.
336-339: Test correctly updated to use Arc-wrapped callback.The test properly uses
std::sync::Arc::new(callback)to match the updatedon_invoice_eventssignature.crates/lni/lnd/lib.rs (2)
52-127: Inherent impl block mirrors StrikeNode pattern correctly.The LndNode implementation follows the same consistent pattern as StrikeNode, with proper delegation to the API layer and correct
Arc<dyn OnInvoiceEventCallback>usage.
465-468: Test correctly updated for Arc-based callback.crates/lni/nwc/lib.rs (3)
52-110: NwcNode inherent implementation follows the established pattern.Consistent with StrikeNode and LndNode implementations. All async methods properly delegate to the API layer.
112-114: Trait macro usage is correct.
297-300: Test correctly updated for Arc-based callback.bindings/kotlin/example/app/src/main/kotlin/com/lni/example/MainActivity.kt (2)
393-401: Well-designed sealed class for invoice status state management.The
InvoiceStatussealed class with its variants provides type-safe, exhaustive state handling in the UI layer.
37-267: UI implementation is well-structured for the demo.The composables follow Material3 guidelines and handle loading states, error cases, and real-time monitoring appropriately.
crates/lni/speed/lib.rs (3)
54-126: SpeedNode inherent implementation follows the consistent pattern.All methods properly delegate to the API layer, and the callback signature correctly uses
Arc<dyn OnInvoiceEventCallback>.
128-130: Trait macro usage is correct for non-UniFFI builds.
320-323: Test correctly updated for Arc-based callback.crates/lni/blink/lib.rs (3)
55-119: BlinkNode inherent implementation follows the consistent pattern.All methods properly delegate to the API layer with correct
Arc<dyn OnInvoiceEventCallback>usage for thread-safe callback sharing.
121-123: Trait macro usage is correct for non-UniFFI builds.
316-319: Test correctly updated for Arc-based callback.crates/lni/cln/lib.rs (4)
9-10: LGTM!Conditional import of
LightningNodeonly for non-uniffi builds aligns with the PR's goal of separating UniFFI-exported inherent methods from trait-based Rust consumer APIs.
53-55: LGTM!The inherent impl block pattern with
uniffi::export(async_runtime = "tokio")correctly exposes async methods directly for UniFFI bindgen while avoiding the trait-based export issue described in the PR.
144-146: LGTM!The macro-based trait implementation for non-uniffi builds maintains backward compatibility for Rust consumers who depend on the
LightningNodetrait.
499-501: LGTM!Test correctly updated to wrap the callback with
Arc::new()to match the newArc<dyn OnInvoiceEventCallback>parameter type.crates/lni/phoenixd/lib.rs (5)
8-9: LGTM!Conditional import pattern consistent with other node implementations in this PR.
54-59: LGTM!Inherent impl block with UniFFI export attribute correctly configured for async tokio runtime.
118-124: LGTM!The
on_invoice_eventsmethod correctly awaits the inner call, unlike the CLN implementation which is missing.await.
127-129: LGTM!Macro-based trait implementation for non-uniffi builds consistent with CLN pattern.
361-363: LGTM!Test correctly updated to use
Arc::new()for the callback.bindings/lni_nodejs/index.d.ts (2)
1-4: Auto-generated file - review for correctness only.This file is auto-generated by NAPI-RS. Any structural changes should be made in the Rust source files that drive the generation.
287-409: LGTM!The class declarations correctly expose all node types with their respective methods. The API surface appropriately reflects differences between node implementations (e.g.,
StrikeNodemethods returning sync values while others returnPromise).
bindings/kotlin/example/app/src/main/kotlin/com/lni/example/MainActivity.kt
Show resolved
Hide resolved
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (2)
bindings/kotlin/example/app/src/main/kotlin/com/lni/example/MainActivity.kt (1)
296-296: Use explicit locale for String.format to ensure consistent formatting.This issue was previously identified:
String.format("%.8f", balanceBtc)uses the implicit default locale, which can produce inconsistent decimal separators across different user locales.crates/lni/cln/lib.rs (1)
135-141: The missing.awaithas been addressed.The
on_invoice_eventsmethod now correctly awaits the inner async call at line 140. This resolves the previous review concern about the future not being polled.
🧹 Nitpick comments (5)
bindings/lni_react_native/example/src/App.tsx (1)
131-134: Consider omitting explicit undefined values for optional fields.Since
searchis an optional field inLookupInvoiceParams, explicitly setting it toundefinedis redundant. The same applies to other parameter creation calls throughout the file (lines 147-148, 171).🔎 Proposed refactor
const lookupParams = LookupInvoiceParams.create({ paymentHash: testInvoiceHash, - search: undefined, });Apply similar changes to:
- Lines 147-148 (ListTransactionsParams)
- Line 171 (CreateOfferParams)
crates/lni/lib.rs (1)
45-139: Consider handlingJoinErrorinstead of unwrapping.The
.unwrap()calls onspawn(...).awaitwill propagate panics from spawned tasks. If a task panics, theJoinErroris unwrapped and the panic propagates, which may be acceptable but could cause surprising behavior for callers.🔎 Optional: Propagate JoinError as ApiError
If you want more graceful handling, you could convert
JoinErrortoApiError:async fn get_info(&self) -> Result<crate::NodeInfo, crate::ApiError> { let this = self.clone(); crate::TOKIO_RUNTIME.spawn(async move { <$node_type>::get_info(&this).await - }).await.unwrap() + }).await.unwrap_or_else(|e| Err(crate::ApiError::Api { + reason: format!("Task panicked: {}", e) + })) }crates/lni/phoenixd/lib.rs (1)
114-116: Stub implementation fordecode.This method returns an empty string rather than delegating to an API implementation. If Phoenixd doesn't support decoding, consider returning a more informative error like
Err(ApiError::Api { reason: "decode not implemented for PhoenixdNode".to_string() })for consistency with howcreate_offerhandles unsupported operations in other nodes.bindings/lni_nodejs/index.d.ts (2)
177-179: Empty interface flagged by static analysis.
CloseChannelResponseis an empty interface, which is equivalent to{}. Per the static analysis hint, consider using a type alias instead if no properties are expected.🔎 Proposed fix
-export interface CloseChannelResponse { - -} +export type CloseChannelResponse = Record<string, never>Alternatively, if properties will be added later, this can remain as-is with a TODO comment.
385-392: Note: StrikeNode uses synchronous methods for offer operations.Unlike other node classes,
StrikeNodedeclarescreateOffer,getOffer,listOffers,payOffer, anddecodeas synchronous (returning values directly rather thanPromise<...>). This likely reflects that these operations return stub/error values immediately. Consider documenting this behavioral difference for consumers who may expect uniform async APIs across all node types.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (19)
bindings/kotlin/example/app/src/main/kotlin/com/lni/example/MainActivity.ktbindings/lni_nodejs/index.d.tsbindings/lni_nodejs/src/cln.rsbindings/lni_nodejs/src/phoenixd.rsbindings/lni_react_native/example/src/App.tsxcrates/lni/Cargo.tomlcrates/lni/blink/api.rscrates/lni/blink/lib.rscrates/lni/cln/api.rscrates/lni/cln/lib.rscrates/lni/lib.rscrates/lni/lnd/lib.rscrates/lni/nwc/lib.rscrates/lni/phoenixd/lib.rscrates/lni/speed/api.rscrates/lni/speed/lib.rscrates/lni/strike/api.rscrates/lni/strike/lib.rscrates/lni/types.rs
🚧 Files skipped from review as they are similar to previous changes (2)
- crates/lni/blink/lib.rs
- crates/lni/blink/api.rs
🧰 Additional context used
🧬 Code graph analysis (6)
crates/lni/cln/api.rs (7)
bindings/lni_nodejs/src/cln.rs (1)
on_invoice_events(120-135)crates/lni/blink/api.rs (2)
on_invoice_events(668-678)poll_invoice_events(629-666)crates/lni/cln/lib.rs (1)
on_invoice_events(135-141)crates/lni/speed/lib.rs (1)
on_invoice_events(119-125)crates/lni/types.rs (1)
on_invoice_events(41-45)crates/lni/lnd/api.rs (2)
on_invoice_events(344-356)poll_invoice_events(308-341)crates/lni/nwc/api.rs (1)
poll_invoice_events(272-303)
bindings/lni_nodejs/src/phoenixd.rs (2)
crates/lni/cln/api.rs (1)
params(282-285)bindings/lni_nodejs/main.mjs (1)
params(60-65)
crates/lni/phoenixd/lib.rs (2)
bindings/lni_nodejs/index.d.ts (11)
Transaction(110-124)CreateOfferParams(258-261)PhoenixdNode(13-15)NodeInfo(95-109)CreateInvoiceParams(235-248)PayInvoiceParams(262-273)PayInvoiceResponse(209-213)Offer(249-257)LookupInvoiceParams(231-234)ListTransactionsParams(225-230)OnInvoiceEventParams(274-279)bindings/lni_nodejs/src/phoenixd.rs (9)
get_info(35-39)create_invoice(42-53)pay_invoice(56-60)create_offer(63-67)get_offer(70-74)pay_offer(90-99)lookup_invoice(77-87)list_transactions(102-109)on_invoice_events(112-127)
bindings/lni_react_native/example/src/App.tsx (2)
bindings/lni_nodejs/main.mjs (15)
node(125-125)node(141-141)node(160-160)node(174-174)node(188-188)node(202-202)node(216-216)invoice(17-21)config(115-118)config(131-134)config(147-153)config(165-167)config(179-181)config(193-195)config(207-209)bindings/lni_nodejs/index.d.ts (4)
CreateInvoiceParams(235-248)LookupInvoiceParams(231-234)ListTransactionsParams(225-230)CreateOfferParams(258-261)
crates/lni/cln/lib.rs (2)
bindings/lni_nodejs/index.d.ts (8)
PayInvoiceParams(262-273)PayInvoiceResponse(209-213)Transaction(110-124)NodeInfo(95-109)CreateInvoiceParams(235-248)CreateOfferParams(258-261)Offer(249-257)OnInvoiceEventParams(274-279)crates/lni/types.rs (11)
get_info(23-23)create_invoice(24-24)pay_invoice(25-25)create_offer(26-26)get_offer(27-27)list_offers(28-28)pay_offer(29-34)lookup_invoice(35-35)list_transactions(36-39)decode(40-40)on_invoice_events(41-45)
crates/lni/speed/lib.rs (2)
crates/lni/speed/api.rs (4)
get_info(86-133)create_invoice(135-226)pay_invoice(228-299)on_invoice_events(575-585)crates/lni/types.rs (4)
get_info(23-23)create_invoice(24-24)pay_invoice(25-25)on_invoice_events(41-45)
🪛 Biome (2.1.2)
bindings/lni_nodejs/index.d.ts
[error] 177-179: An empty interface is equivalent to {}.
Safe fix: Use a type alias instead.
(lint/suspicious/noEmptyInterface)
🪛 detekt (1.23.8)
bindings/kotlin/example/app/src/main/kotlin/com/lni/example/MainActivity.kt
[warning] 296-296: String.format("%.8f", balanceBtc) uses implicitly default locale for string formatting.
(detekt.potential-bugs.ImplicitDefaultLocale)
🔇 Additional comments (36)
bindings/lni_react_native/example/src/App.tsx (2)
119-123: LGTM: Parameter factory usage is correct.All parameter objects correctly use the
.create()factory pattern with appropriate types (e.g.,BigIntfor numeric values). The implementation is consistent and follows the new API design.Also applies to: 131-134, 144-149, 169-172
21-28: All polymorphic API exports are correctly configured and available.The
LightningNodetrait and all factory functions (createLndNode,createClnNode,createStrikeNode,createPhoenixdNode,createBlinkNode,createNwcNode,createSpeedNode) are properly defined in the Rust source (crates/lni/lib.rsandcrates/lni/types.rs) and marked with#[cfg_attr(feature = "uniffi", uniffi::export)]. The uniffi-bindgen-react-native build process will convert these Rust exports to TypeScript bindings with camelCase naming conventions. The generated TypeScript files don't currently exist in the repository because they're created at build time, which is expected behavior. The implementation correctly adopts the polymorphic pattern throughout the file.crates/lni/Cargo.toml (2)
9-9: LGTM! cdylib addition supports UniFFI library-mode bindgen.The addition of "cdylib" to the crate-type list is necessary for generating UniFFI bindings in library mode, as described in the PR objectives. The inline comment clearly documents the purpose of each crate type.
41-41: LGTM! once_cell supports static runtime initialization.The
once_celldependency is used for the staticTOKIO_RUNTIMEinitialization pattern referenced in the AI summary, supporting the Arc-based callback architecture across node implementations.bindings/kotlin/example/app/src/main/kotlin/com/lni/example/MainActivity.kt (4)
21-35: LGTM! MainActivity setup follows Android best practices.The activity correctly uses
setContentwith MaterialTheme and properly delegates to a composable screen.
269-307: LGTM! Balance retrieval properly handles concurrency and errors.The function correctly uses
withContext(Dispatchers.IO)for network operations, handles bothApiExceptionand generic exceptions, and formats output appropriately.
402-480: LGTM! Polymorphism demonstration is clear and educational.The
getBalanceFromAnyNodeanddemonstratePolymorphismfunctions effectively showcase the polymorphicLightningNodeinterface and provide helpful documentation for users.
483-556: LGTM! Invoice status UI is well-structured.The sealed class hierarchy for
InvoiceStatusand theInvoiceStatusCardcomposable provide a clean, type-safe way to represent and display invoice states with appropriate visual feedback.bindings/lni_nodejs/src/cln.rs (1)
39-51: LGTM! Sensible default for invoice_type parameter.The addition of
.unwrap_or(lni::InvoiceType::Bolt11)provides a sensible default wheninvoice_typeis not specified, aligning with the pattern used across other node implementations in this PR. This improves the developer experience without breaking existing functionality.bindings/lni_nodejs/src/phoenixd.rs (1)
42-53: LGTM! Consistent invoice_type defaulting.The defaulting behavior matches the CLN implementation and maintains consistency across the node binding layer. This ensures predictable behavior when
invoice_typeis omitted.crates/lni/speed/api.rs (2)
135-226: LGTM! Invoice creation uses helper method for type resolution.The use of
invoice_params.get_invoice_type()at line 139 provides a cleaner abstraction for determining the invoice type, likely with the same Bolt11 defaulting behavior seen in other node implementations. The BOLT11 invoice parsing and transaction mapping logic is correct.
575-585: LGTM! Arc-based callback enables safe shared ownership.The change from
Box<dyn OnInvoiceEventCallback>toArc<dyn OnInvoiceEventCallback>at line 578 aligns with the PR-wide pattern of using Arc for callbacks in async contexts. This enables safe shared ownership when callbacks need to be invoked from multiple contexts or cloned.crates/lni/cln/api.rs (1)
805-816: LGTM! Arc-based callback aligns with the cross-crate refactor.The signature change from
Box<dyn OnInvoiceEventCallback>toArc<dyn OnInvoiceEventCallback>is correct for shared ownership in async contexts. The closure captures theArcby move, enabling cheap cloning when needed.crates/lni/strike/api.rs (2)
134-134: Good use of the new accessor method.Using
get_invoice_type()ensures consistent defaulting toBolt11wheninvoice_typeisNone, aligning with theCreateInvoiceParamschanges intypes.rs.
686-697: LGTM! Consistent with the Arc-based callback pattern.The
on_invoice_eventssignature and implementation match the other node modules.crates/lni/lib.rs (2)
9-18: Global Tokio runtime initialization looks correct.The multi-threaded runtime with
enable_all()is appropriate for async I/O operations. TheLazyinitialization ensures it's created once on first access.
247-292: Factory functions enable clean polymorphic access.These functions correctly return
Arc<dyn LightningNode>for use in Kotlin/Swift bindings without manual wrapper code.crates/lni/strike/lib.rs (3)
55-129: Inherent impl approach is correct for UniFFI compatibility.The methods correctly delegate to
crate::strike::api::*functions. The#[cfg_attr(feature = "uniffi", uniffi::export(async_runtime = "tokio"))]attribute ensures these are exported only when the UniFFI feature is enabled.
337-337: Test correctly updated to useArc::new(callback).
131-132: No action required. The macro invocation is intentionally unconditional per the design documented in the macro definition. Theimpl_lightning_node!macro generates a Rust trait implementation for internal polymorphic access (Arc<dyn LightningNode>), while UniFFI's#[uniffi::export]generates FFI bindings for external language consumption. These serve different purposes and do not conflict—the macro's documentation explicitly states it "works for both UniFFI and non-UniFFI builds" and "avoids code duplication between UniFFI exports and trait implementations."Likely an incorrect or invalid review comment.
crates/lni/types.rs (3)
17-46: LightningNode trait withwith_foreignenables foreign implementations.The
#[cfg_attr(feature = "uniffi", uniffi::export(with_foreign))]attribute correctly allows Kotlin/Swift to both use and implement this trait. TheArc<dyn OnInvoiceEventCallback>parameter type is appropriate for cross-language callbacks.
323-372: Well-designed optionalinvoice_typewith safe defaulting.Making
invoice_typeoptional with aget_invoice_type()accessor that defaults toBolt11improves API ergonomics while maintaining backward compatibility.
438-446: Callback trait correctly useswith_foreignfor foreign implementations.The
#[cfg_attr(feature = "uniffi", uniffi::export(with_foreign))]attribute enables Kotlin/Swift to implementOnInvoiceEventCallbackand pass instances to Rust.crates/lni/lnd/lib.rs (3)
52-127: Inherent impl follows the established pattern correctly.All methods delegate to
crate::lnd::api::*functions. The UniFFI export attribute is correctly applied.
129-131: Macro invocation not gated - same consideration as StrikeNode.Like
StrikeNode, the macro invocation is not behind#[cfg(not(feature = "uniffi"))]. Ensure this doesn't cause conflicts when UniFFI is enabled.
467-467: Test correctly updated to useArc::new(callback).crates/lni/nwc/lib.rs (3)
52-110: Inherent impl follows the established pattern correctly.The implementation is consistent with other node modules (Strike, LND, CLN, etc.).
112-114: Macro invocation not gated - consistent with other nodes.Same pattern as other node implementations. If the macro is intended to always generate the trait impl regardless of UniFFI feature, this is fine.
298-298: Test correctly updated to useArc::new(callback).crates/lni/cln/lib.rs (2)
144-147: LGTM - Proper conditional trait implementation.The
impl_lightning_node!macro pattern correctly gates the trait implementation for non-UniFFI builds while UniFFI exports the inherent methods directly. This aligns with the PR's approach across all node implementations.
499-500: Test correctly updated for Arc-based callback.The test properly wraps the callback in
Arc::new()to match the updatedon_invoice_eventssignature.crates/lni/phoenixd/lib.rs (1)
127-130: LGTM - Consistent macro-based trait implementation.The conditional trait implementation via
impl_lightning_node!follows the established pattern.crates/lni/speed/lib.rs (2)
54-126: LGTM - Consistent inherent impl structure.The
SpeedNodeimplementation follows the established pattern:
- Async methods properly await API calls
create_offerappropriately returns an error for unsupported functionalityon_invoice_eventscorrectly acceptsArc<dyn OnInvoiceEventCallback>and awaits the inner call
128-131: LGTM - Macro-based trait implementation.Consistent with the pattern used across all node implementations in this PR.
bindings/lni_nodejs/index.d.ts (2)
326-327: Verify sync vs async consistency forcreateOffer.
LndNode.createOfferreturnsOffer(sync) whilecreateOfferAsyncat line 340 returnsPromise<Transaction>. The sync version also returns a different type (OffervsTransaction). Ensure this inconsistency is intentional and documented, as it may confuse consumers expecting uniform async behavior across node types.
87-90: LGTM - Well-defined enum type.The
InvoiceTypeenum with string values'Bolt11'and'Bolt12'provides clear type safety for invoice type selection.
bindings/kotlin/example/app/src/main/kotlin/com/lni/example/MainActivity.kt
Show resolved
Hide resolved
Co-authored-by: ntheile <1273575+ntheile@users.noreply.github.com>
* Initial plan * Add Spark node integration using Breez Spark SDK Co-authored-by: ntheile <1273575+ntheile@users.noreply.github.com> * Address code review feedback - improve type safety and validation Co-authored-by: ntheile <1273575+ntheile@users.noreply.github.com> * spark test fixes * Update Spark implementation to support polymorphism pattern from PR #20 Co-authored-by: ntheile <1273575+ntheile@users.noreply.github.com> * add bip39 and spark test * generateMnemonic kotlin, nodejs, and nodejs * add generateMnemonic react native * add spark to nodejs * spark not feature * . * . * fix kotlin and spark * fix RN spark test * spark swift test * v0.2.0 swift package * remove yarn.lock, add to gitignore * fix on_invoice_events for spark * update RN --------- Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: ntheile <1273575+ntheile@users.noreply.github.com> Co-authored-by: nicktee <ntheile@gmail.com>
UniFFI bindgen fails with "no interface LightningNode" when generating Kotlin bindings from
liblni.so. The root cause:#[uniffi::export]on trait impl blocks expects the trait itself to be UniFFI-exported, butLightningNodeis a plain Rust trait.Solution
Use inherent impl blocks for UniFFI instead of trait impl blocks:
Changes
Node implementations (
strike,blink,nwc,phoenixd,lnd,cln,speed):LightningNodeonly when neededtypes.rs: Gate
LightningNodetrait andLightningNodeEnumwith#[cfg(not(feature = "uniffi"))]Cargo.toml: Add
cdylibcrate-type for library-mode bindgenbindings/kotlin/: New directory with uniffi-bindgen helper and build script
bindings/kotlin/example/: Gradle project demonstrating usage
Usage
cd bindings/kotlin ./build.sh --releaseGenerates
src/main/kotlin/uniffi/lni/lni.ktwith all node classes and async methods.Original prompt
✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.
Summary by CodeRabbit
New Features
New Features
Documentation
Improvements
✏️ Tip: You can customize this high-level summary in your review settings.