Add LDK Server backend#4107
Conversation
There was a problem hiding this comment.
Code Review
This pull request adds support for LDK Server as a new backend, including the necessary gRPC backend implementation, protobuf definitions, and UI updates for wallet configuration. It also updates documentation and project templates to reflect this new integration. My feedback suggests replacing the magic number '660' in the LdkServer backend with a named constant for better maintainability and moving the hardcoded API key placeholder in the WalletConfiguration view to the localization files to support internationalization.
dfefea3 to
f0c050e
Compare
There was a problem hiding this comment.
Code Review
This pull request adds support for LDK Server as a new remote node backend, implementing the LdkServer class for gRPC communication, adding Protobuf definitions, and updating the UI for node configuration. The code review feedback highlights reliability concerns in the gRPC implementation, specifically regarding potential resource leaks and the need for native timeouts. There is also a recommendation to simplify channel balance calculations and eliminate magic numbers.
There was a problem hiding this comment.
Code Review
This pull request introduces support for LDK Server as a new remote node backend. It includes a new LdkServer backend implementation using gRPC over HTTPS, updated Protobuf definitions, and UI enhancements for node configuration. Feedback suggests refining the msatToSat utility to ensure satoshi values are returned as whole numbers and replacing a magic number in the channel balance calculation with a named constant for better maintainability.
shubhamkmr04
left a comment
There was a problem hiding this comment.
@benthecarman we only need change in en.json locales
b7bb78a to
52eb30f
Compare
fixed i think |
There was a problem hiding this comment.
@benthecarman, On-chain “payment received” detection never works because LdkServer.formatOnchainTransaction never fills dest_addresses or output_details, and we are not using status too
Wire LDK Server into remote node configuration, backend selection, invoice polling, and shared LDK behavior. Add generated protobuf bindings and document the supported feature surface.
This is because we have a bug where on-chain payments aren't returned at all 😅 fixed in ldk-server here: lightningdevkit/ldk-server#219 |
Description
Relates to issue: ZEUS-0000
Wire LDK Server into remote node configuration, backend selection, invoice polling, and shared LDK behavior. Add generated protobuf bindings and document the supported feature surface.
I got it able to connect on my phone and told codex good job and to clean up and commit. Have not reviewed the actual logic yet but opening for now!
This pull request is categorized as a:
Checklist
yarn run tscand made sure my code compiles correctlyyarn run lintand made sure my code didn’t contain any problematic patternsyarn run prettierand made sure my code is formatted correctlyyarn run testand made sure all of the tests passTesting
If you modified or added a utility file, did you add new unit tests?
I have tested this PR on the following platforms (please specify OS version and phone model/VM):
I have tested this PR with the following types of nodes (please specify node version and API version where appropriate):
On-device
Remote
Locales
Third Party Dependencies and Packages
yarnafter this PR is merged inpackage.jsonandyarn.lockhave been properly updatedOther: