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.

Sign up for GitHub

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

Enhance Error Message for Missing Core Lightning Configuration File #211

Closed
wants to merge 2 commits into from
Closed

Enhance Error Message for Missing Core Lightning Configuration File #211

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
27a285c
feat(core): improve error message
tareknaser Nov 27, 2023
76ad241
fix(core): improve error message for missing lightning root folder
tareknaser Dec 5, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 18 additions & 3 deletions coffee_core/src/coffee.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@

use std::collections::HashMap;
use std::fmt::Debug;
use std::path::Path;
use std::vec::Vec;
use tokio::fs;

Expand Down Expand Up @@ -195,10 +196,23 @@ impl CoffeeManager {
if self.config.cln_config_path.is_none() {
return Ok(());
}
let root = self.config.cln_root.clone().unwrap();
let root = self.config.cln_root.clone();
let root = root.ok_or_else(|| error!("cln root path not found."))?;
let rpc = Client::new(format!("{root}/{}/lightning-rpc", self.config.network));
self.rpc = Some(rpc);
let path = self.config.cln_config_path.clone().unwrap();
let path = self.config.cln_config_path.clone();
let path = path.ok_or_else(|| error!("cln config path not found."))?;
Comment on lines +199 to +204
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what about the path is Some but do not exist?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The next line is

let mut file = CLNConf::new(path.clone(), true);

where we set create_if_missing to true
This is what happens when the user first runs coffee

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mh I do not think this is right, if the path do not exist this code do not make sense because CLNConf::new(path.clone(), true); is not able to find the path where to create the file.

Please test the user case that I described #209

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You are right.
This line will handle the case that the root path exists but the configuration file doesn't.
However, it will fail if the root path doesn't exist.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added another check for this case.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This line will handle the case that the root path exists but the configuration file doesn't.

This is done already by CLNConf

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not sure CLNConf handles this case.
Could you give it another try?
To reproduce:
1- delete ~/.lightning/bitcoin directory
2- try running coffee commands (eg. coffee remote list)
You will get the same error mentioned in the original issue

What CLNConf handles is the case when ~/.lightning/bitcoin/config is missing but ~/.lightning/bitcoin exists.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not sure CLNConf handles this case.

yes is does, you need to do CLNConf::new(path.clone(), false); if the file do not exist it return an error

What CLNConf handles is the case when ~/.lightning/bitcoin/config is missing but ~/.lightning/bitcoin exists.

Correct, you should check if the path exist


// The path is the path of the cln config file (eg. ~/.lightning/bitcoin/config)
// We need to check that the root folder (eg. ~/.lightning/bitcoin) exists
// otherwise we return an error.

// We can unwrap here because we are sure that the path ends with /config
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more. 

// SAFETY: We can unwrap here because we are sure that the path ends with /config

let lightning_path = path.strip_suffix("/config").unwrap();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you need to strip this, this means that you are checking in the wrong place.

The check need to be done before we build the path with the /config

if !Path::new(&lightning_path).exists() {
return Err(error!("The path {lightning_path} does not exist"));
}

let mut file = CLNConf::new(path.clone(), true);
log::info!("looking for the cln config: {path}");
file.parse()
Expand All @@ -217,7 +231,8 @@ impl CoffeeManager {
self.config.cln_config_path = Some(path_with_network);
self.config.cln_root = Some(cln_dir.to_owned());
self.load_cln_conf().await?;
let mut conf = self.cln_config.clone().unwrap();
let conf = self.cln_config.clone();
let mut conf = conf.ok_or_else(|| error!("cln config path not found."))?;
conf.add_subconf(self.coffee_cln_config.clone())
.map_err(|err| error!("{}", &err.cause))?;
conf.flush()?;
Expand Down
Loading