Skip to content
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

refactor to use lnd_grpc client #40

Closed
wants to merge 1 commit into from

Conversation

PierreRochard
Copy link

@PierreRochard PierreRochard commented Mar 8, 2019

Hi @C-Otto ! @willcl-ark and I are organizing a google hangouts meeting about lnd_grpc and the Python LND ecosystem next week ( Thursday March 14th, at 20:00 GMT ) if you are interested in attending.

Send me an email pierre@rochard.org and I'll add you to the call.

We'll also be livestreaming / recording it for those who just want to listen in or can't make it that day.

I think it would be great if we all worked together where there is overlap!

This pull request changes quite a few things, so I don't necessarily expect it to get merged, I'm happy to split it up or make it more gradual.

@wamde
Copy link
Collaborator

wamde commented Mar 9, 2019

Thanks for the PR, Pierre.
Since it touches quite a few things, would you mind splitting it up in several parts, something like:

  • switch to lnd_grpc
  • refactor/rewrite of the arguments parser
  • the rest

@wamde wamde self-requested a review March 9, 2019 09:15
@PierreRochard
Copy link
Author

@wamde happy to do so, I'll open separate pull requests.

Would you be interested in joining our call next week?

@wamde
Copy link
Collaborator

wamde commented Mar 9, 2019

Thanks. Not live, but I can catch the recording afterwards if you do record it.

@C-Otto
Copy link
Owner

C-Otto commented Mar 10, 2019

Thanks a lot for the PR! I'll also have a look at the recordings. Several things came to my mind:

  1. Splitting this PR into several would really help. For example, configuring the LND server (IP and such) is something we already mentioned in issue Specify a different lnd directory #20.
  2. I added the lnd class (file?) for the purpose of putting lnd communication in there. Sadly, you deleted it, instead of making use of it. I don't think you need to modify a lot of the other code.
  3. Having one (two?) dedicated projects for the communication with lnd is great, but currently I don't see the advantage. Your project seems to be quite new, and doesn't provide any new functionality. I propose we wait until your project is more mature before using it in this script. As a workaround, and to advertise your idea/code, you could provide a fork with the required changes.

@C-Otto
Copy link
Owner

C-Otto commented Mar 18, 2019

@PierreRochard where can I find the recordings?

@C-Otto
Copy link
Owner

C-Otto commented Mar 28, 2019

Closing due to lack of activity

@C-Otto C-Otto closed this Mar 28, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants