-
Notifications
You must be signed in to change notification settings - Fork 2
Expose core JNI helpers to Kotlin and demo via Android example #33
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
Conversation
niteshbalusu11
commented
Nov 20, 2025
- add JNI wrappers for wallet ops + extra methods (maintenanceRefresh, tryClaimLightningReceive, offboardAll, peakKeyPair, verifyMessage, bolt11Invoice) and log config use - add Kotlin facade (NitroArkNative) and example RN module to surface these native calls - wire Android demo buttons in example app to exercise the JNI path with config passthrough and logging
Summary of ChangesHello @niteshbalusu11, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the Android integration of the NitroArk library by providing direct access to its core C++/Rust functionalities via JNI and a Kotlin facade. This allows Android developers to interact with critical wallet operations natively, potentially improving performance and offering a more direct integration path. The changes also include a comprehensive demonstration within the example application, showcasing how to utilize these new native capabilities. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
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.
Code Review
This pull request successfully exposes core JNI helpers to Kotlin and provides a demonstration through an Android example. The new C++ JNI layer, Kotlin facade, and React Native demo module are well-structured. My review focuses on improving the robustness of the JNI layer and the maintainability of the example app. Specifically, I've identified a critical issue with JSON serialization that could lead to invalid data, a performance optimization for JNI calls, and an opportunity to reduce code duplication in the TypeScript example. Addressing these points will enhance the quality and reliability of the implementation.
|
/gemini review |
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.
Code Review
This pull request successfully exposes core JNI helpers to Kotlin and adds a demonstration in the Android example app. The Kotlin and TypeScript changes are well-implemented. However, the C++ JNI code introduces several critical issues related to memory safety and resource management. These include potential dangling pointers, a null pointer dereference vulnerability, and JNI local reference leaks that could lead to application crashes. These critical and high-severity issues in NitroArkJni.cpp must be addressed before merging.
- add JNI wrappers for wallet ops + extra methods (maintenanceRefresh, tryClaimLightningReceive, offboardAll, peakKeyPair, verifyMessage, bolt11Invoice) and log config use - add Kotlin facade (NitroArkNative) and example RN module to surface these native calls - wire Android demo buttons in example app to exercise the JNI path with config passthrough and logging
1e2d492 to
0043168
Compare
|
/gemini review |
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.
Code Review
This pull request successfully exposes core JNI helpers to Kotlin and demonstrates their use in the Android example app. The new C++ JNI layer, Kotlin facade, and React Native module are well-structured. My review focuses on critical JNI correctness issues, including potential memory leaks and dangling pointers that could lead to crashes. I've also included suggestions for performance optimization by caching JNI lookups, and improving code maintainability by reducing duplication and ensuring consistent naming. Addressing these points will significantly improve the robustness and performance of the native integration.
react-native-nitro-ark/android/src/main/java/com/margelo/nitro/nitroark/NativeTypes.kt
Show resolved
Hide resolved
|
/gemini review |
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.
Code Review
This pull request successfully exposes core JNI helpers to Kotlin and demonstrates their use via an Android example. The changes are comprehensive, including updates to the Android build script, a new C++ JNI layer, a Kotlin facade, and an example React Native module. My review focuses on improving the maintainability of the build script and addressing several correctness, performance, and resource management issues within the new C++ JNI implementation. Key feedback includes refactoring duplicated logic in the build script, correcting potential JNI local reference leaks, suggesting performance optimizations for JNI calls, and flagging a critical issue with the use of thread_local that could lead to bugs.
|
/gemini review |
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.
Code Review
This pull request successfully exposes core JNI helpers to Kotlin, providing a native Android interface for wallet operations, and includes a comprehensive example app to demonstrate this new functionality. The changes are well-structured, adding support for the armeabi-v7a architecture and creating a clean Kotlin facade.
My review focuses on improving the robustness and maintainability of the new code. I've identified a critical issue in the C++ JNI layer related to a dangling pointer in logging, and a few high-severity issues concerning risky pointer usage and improper JNI exception handling. Additionally, I've suggested refactoring opportunities in the Android build script to reduce code duplication. These changes will enhance the stability and long-term maintainability of the native code.
| __android_log_print(ANDROID_LOG_INFO, LOG_TAG, | ||
| "load_wallet(native) datadir=%s regtest=%s signet=%s bitcoin=%s birthday_height=%s ark=%s " | ||
| "esplora=%s bitcoind=%s", | ||
| datadir.c_str(), opts.regtest ? "true" : "false", opts.signet ? "true" : "false", | ||
| opts.bitcoin ? "true" : "false", | ||
| opts.birthday_height != nullptr ? std::to_string(*opts.birthday_height).c_str() : "null", | ||
| config.ark.c_str(), config.esplora.c_str(), config.bitcoind.c_str()); |
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.
There is a potential dangling pointer issue here. std::to_string(*opts.birthday_height) creates a temporary std::string object. Calling .c_str() on it gives a pointer to its internal buffer. This temporary string is destroyed at the end of the statement (at the semicolon), leaving a dangling pointer that __android_log_print might try to access, leading to undefined behavior.
To fix this, you should store the result of std::to_string in a variable that lives until after __android_log_print is called.
const std::string birthday_height_str =
opts.birthday_height != nullptr ? std::to_string(*opts.birthday_height) : "null";
__android_log_print(ANDROID_LOG_INFO, LOG_TAG,
"load_wallet(native) datadir=%s regtest=%s signet=%s bitcoin=%s birthday_height=%s ark=%s "
"esplora=%s bitcoind=%s",
datadir.c_str(), opts.regtest ? "true" : "false", opts.signet ? "true" : "false",
opts.bitcoin ? "true" : "false", birthday_height_str.c_str(),
config.ark.c_str(), config.esplora.c_str(), config.bitcoind.c_str());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.
hsjoberg
left a comment
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.
Looks good. Too much JNI stuff to review accurate behavior but I guess we will see in practice how it goes
