Kwargs, and additional subsystem support #30
Conversation
|
I also starting using this library: git+ssh://git@github.com/wearefair/protobuf-to-dict, which is a nicer way to convert the data to a dictionary than using googles protobuf library. |
|
@adrienemery Think we can get this merged in? Im looking to do more consistent work on this repository |
There was a problem hiding this comment.
Hey, thanks for the PR 👍🏽
Overall this looks good - I added a few comments. Let me know if any comments don't make sense. I have not been active in the LND space for a while so would still like to spin up a local node to test these changes against before merging.
Thanks again for the PR
lndgrpc/aio/async_client.py
Outdated
| @handle_rpc_errors | ||
| async def send_payment_v2(self, payment_request): | ||
| """Send a payment over lightning""" | ||
| request = router.SendPaymentRequest(payment_request=payment_request,**kwargs) |
There was a problem hiding this comment.
There are no **kwargs in the send_payment_v2 method to pass through here - I think it needs to be removed.
There was a problem hiding this comment.
I should probably remove this async work.
Anyways though, I use **kwargs a lot throught the pr, it allows the user to specify arbitrary inputs to the function. That way we dont have to update our code/ bloat out code with arguments for every possible input variable names, we can just define required ones, but allows any others to be passed in an used.
lndgrpc/client.py
Outdated
| try: | ||
| response = self._walletunlocker_stub.UnlockWallet(request) | ||
| return response | ||
| except Exception as e: |
There was a problem hiding this comment.
Can we try put a more target exception here if there is a known expeted error - maybe we can wrap it in a nicer error with a message as you have below. But I would avoid catching all errors. Or just let it fail and let the caller decide how to handle the errors. Thoughts?
lndgrpc/client.py
Outdated
| request = router.SendPaymentRequest(**kwargs) | ||
| last_response = None | ||
| for response in self._router_stub.SendPaymentV2(request): | ||
| print(response) |
There was a problem hiding this comment.
I think we should not have a print statement here. Maybe instead we use a logger in this file and have this set to debug or info level. That way the caller can limit what they want to see by the log level.
lndgrpc/client.py
Outdated
| @handle_rpc_errors | ||
| def get_info(self): | ||
| response = self._ln_stub.GetInfo(ln.GetInfoRequest()) | ||
| return response | ||
|
|
||
| @handle_rpc_errors | ||
| def bake_macaroon(self, permissions, root_key_id): |
There was a problem hiding this comment.
lol bake_macaroon this is a great method name :)
lndgrpc/client.py
Outdated
| @@ -147,16 +242,16 @@ def describe_graph(self): | |||
| return response | |||
|
|
|||
| @handle_rpc_errors | |||
| def get_channel_info(self, channel_id): | |||
| def get_chan_info(self, channel_id): | |||
There was a problem hiding this comment.
I don't think we should break existing method names. Lets leave it as is.
There was a problem hiding this comment.
can we keep both and put a deprecation warning maybe? I just want to follow the docs exactly to not have to think about it
kornpow
force-pushed
the
master
branch
6 times, most recently
from
ce6d303
to
4bcf338
Compare
kornpow
force-pushed
the
master
branch
2 times, most recently
from
013e69e
to
89e528a
Compare
use old code shell
LND v0.16.0-beta.rc2: Regenerated proto files
Github Actions
LND v0.16.0-beta.rc5: Regenerated proto files
This is a work-in-progress PR of the work I am doing to add support for additional LND subsystems, and enable usage of all variables in rpc calls, instead of having to manually add them to this code.