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

In memory creds #30

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

Evanfeenstra
Copy link

This branch has an added connect_from_memory function, that allows connecting with cert and macaroon data from memory (instead of a filepath). Useful for docker deployments etc. Tested and working.

Not sure if anyone else needs this! Or if there is a cleaner way to add this functionality

@Kixunil
Copy link
Owner

Kixunil commented Feb 28, 2023

I specifically didn't add this feature because it leads to problems. People tend to forget that lnd may change the credentials for whatever reason and then your application will break. It looks like it's working until it doesn't.

This is also documented:

This is considered the recommended way to connect to LND. An alternative function to use already-read certificate or macaroon data is currently not provided to discourage such use. LND occasionally changes that data which would lead to errors and in turn in worse application.

@benthecarman
Copy link
Contributor

benthecarman commented Apr 5, 2023

I specifically didn't add this feature because it leads to problems. People tend to forget that lnd may change the credentials for whatever reason and then your application will break. It looks like it's working until it doesn't.

This will really just end up requiring people to run a fork of this repo. Lots of applications talk to lnd when receiving the credentials from other means than the file system.

Ie here is a Voltage employee needing to make a fork tony-voltage@66490e8

@Kixunil
Copy link
Owner

Kixunil commented Apr 5, 2023

OK, if the demand is so strong I would accept this with following changes:

  • name the method maybe_unreliable_connect_from_memory to alert people of possible problems (feel free to suggest shorter name as long as it conveys "this may be problematic, not necessarily dangerous"
  • have a documentation that explains this would fail if the in-memory data is out-of sync and that the callers need to ensure reliability via other means
  • update the quoted documentation on coonect fn to no longer say it's not implemented (but keep saying that connect is recommended)

Note that the unreliability problems were already the case in BTCPayServer which was a huge PITA because I had to debug it with not-so-technical people asking why their BTCpay suddenly doesn't work.

@Evanfeenstra
Copy link
Author

ok, hows it look now?

@ok300
Copy link

ok300 commented Jan 23, 2024

+1

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.

4 participants