In memory creds - Githubissues

Kixunil / tonic_lnd

Rust implementation of LND RPC client using async GRPC library `tonic`

31 stars 44 forks source link

In memory creds #30

Open Evanfeenstra opened 1 year ago

Evanfeenstra commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 https://github.com/tony-voltage/tonic_lnd/commit/66490e81bf4c03d4e6ce7d837c411200d139d7ea

Kixunil commented 1 year ago

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 commented 1 year ago

ok, hows it look now?

ok300 commented 8 months ago