-
Notifications
You must be signed in to change notification settings - Fork 895
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
Make cln-grpc
a first-class citizen
#7167
Comments
cln-grpc
a first-class citizen
Thanks @kilrau for taking the lead here. The JSON-RPC schema, from which the GRPC bindings are generated via So we'd be attacking this on 3 fronts:
The last point already partially exists, and is what I used when building lightning/contrib/pyln-testing/pyln/testing/utils.py Lines 831 to 835 in e331532
Technically, we have What do you think? |
So first step is to fix |
My preference is to simplify APIs where necessary: the GRPC can simply use the new APIs so the only deprecation cycle is on the JSON-RPC side, and I can handle those. Can we make a checklist of what APIs are missing? GH supports those natively, so we can use this issue to coordinate and aim to complete that by next release? (Sure, that means we need it done in three weeks, but pressure makes diamonds, right?) |
I seconds the importance of this. While Building Torq and using CLN for Eden we notice that the biggest weakness that CLN has is the APIs and the gRPC. I think that improving this will have a massive impact on CLN adoption especially for enterprise. Another thing. Not having subscription for certain data forces the need to build plugins. For example for payment status and channel interception. |
Just noting that in my case, the fact it is a plugin let me keep supporting arm32v7 due to this annoying issue. #6596 The fact I can remove it easily from the build in our fork fixed my issue. (I spent 1 day trying to fix it) |
Don't want to start a new discussion, but hope for experts here to answer my question. Can gRPC methods be added dynamically at runtime for a CLN plugin? Looking for ways to make my PeerSwap Web compatible with Core Lightning. |
Here you go #7209 @rustyrussell - now even tests can catch missing calls |
I have commits ready for all of them except |
What's missing here? |
Everything is in now except the sql/commando methods and methods that dont make sense via grpc e.g. batching |
Nice! Are there tests for the coverage via #7209 ? |
Not that i know of |
I think we need tests. Then we can close here and be sure for missing grpc calls to at least cause a test to fail. @rustyrussell @cdecker |
I had started a grpc testing mode some time ago. It was swapping out the direct JSON-RPC interactions with one that was converting to grpc, call via cln-grpc and then concerts the protobuf result back to python dicts. That works nicely for some tests, but the conversion back to python is hacky (especially enum values) and so it never made it very far. I am hoping that we can pick that up again and fix it up, and add a test run to the CI. |
For future reference, this is the place we're we switch from json-rpc to grpc based on an envvar:
|
Tl;dr: We at Boltz are building a new major client on CLN communicating with CLN solely via gRPC and we dearly miss decent gRPC support in CLN.
For us this means:
cln-grpc
immediately - no more adding "on demand"cln-grpc
bugs with the same priority thanlightning-rpc
cln-grpc
in CLN by defaultThe current mode of adding gRPC calls on demand and not knowing if a node has gRPC available or not clearly doesn't work for anyone trying to build something serious:
Open Issues:
#7163
#6996
#6986
#6731
#6193
#5634
#5354
#5345
Closed Issues:
#6892
#6792
#6635
#6409
#6318
#6196
#6194
#6192
#6186
#6112
#6064
#5988
#5926
#5763
#5248
The text was updated successfully, but these errors were encountered: