-
Notifications
You must be signed in to change notification settings - Fork 21
Create default data directory at ~/.ldk-server #96
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
base: main
Are you sure you want to change the base?
Conversation
|
👋 Thanks for assigning @tnull as a reviewer! |
tnull
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes sense, some initial comments.
| Some(path) => { | ||
| // Add network subdirectory for test networks to make sure we don't overwrite | ||
| // data when switching between networks. | ||
| if config_file.network != Network::Bitcoin { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, doing it like that might mean creating one seed per network also. Is this intentional, or should we still create a single seed for all networks at the top level?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah I had similar thoughts for this and the TLS key/cert. I can go either way on it. Code was simpler this way and more separation between networks can't hurt, so that's why I did this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, I think having multiple seeds is a bit confusing. If we already match the Bitcoin Core pattern with the 'mainnet is main folder other networks have sub-folders' we might also follow the pattern there, i.e., you have a single seed across networks. Same goes for the TLS certs/keys, and really any other 'general' files, IMO.
|
I took over #69 and about to reopen it but I see this is taking a different direction. I'll reopen and leave it in draft for now till this lands to reduce the conflicts. |
2ce8622 to
1bcd9f2
Compare
1bcd9f2 to
247a247
Compare
Previously, the daemon required a config file path argument and the CLI required explicit --api-key and --tls-cert flags. Now both default to reading from ~/.ldk-server/config.toml, so you can just run the daemon and CLI without having to specify any flags. We make sure to separate out data by network so we don't conflict anywhere and accidentally lose anything important
247a247 to
3bcf2cc
Compare
Previously, the daemon required a config file path argument and the CLI required explicit --api-key and --tls-cert flags. Now both default to reading from ~/.ldk-server/config.toml, so you can just run the daemon and CLI without having to specify any flags.
We make sure to separate out data by network so we don't conflict anywhere and accidentally lose anything important