Migrate account manager cli client to clap derive

[eserilev]

Issue Addressed

Partially #5900

Proposed Changes

Migrate account-manager to clap derive

[macladson]

I think this PR needs some of the same love from #6300 since lighthouse am --help also doesn't work.

[mergify]

This pull request has merge conflicts. Could you please resolve them @eserilev? 🙏

[eserilev]

this one should be g2g for another review

[macladson]

Will leave a more thorough review soon, but noticed some comments that need updating

[mergify]

This pull request has merge conflicts. Could you please resolve them @eserilev? 🙏

[eserilev]

hi @chong-he could i bug you to review this PR when you get the chance :)

[chong-he]

hi @chong-he could i bug you to review this PR when you get the chance :)

I wouldn't say I am capable to review this PR now, but I am happy to do some testing on this, will post if anything.

Thanks!

[chong-he]

After chatting with @michaelsproul , I am going to give reviewing this PR a try

[chong-he]

Edit: after a closer look, I think the logging of No logfile path provided, logging to file is disabled is actually a feature, not a bug. It is telling users that logging to file is disabled, and it can be enabled with --logfile-dir to save all logs to a file. Please ignore the comments below

Looks pretty good overall. There is just this thing that I found:

For commands in lighthouse am, for example, when we run: lighthouse account wallet create --name test, this first line is printed when it shouldn't (unless --logfile-dir is specified):

No logfile path provided, logging to file is disabled
Running account manager for mainnet network
wallet-dir path: "/home/ck/.lighthouse/mainnet/wallets"

It's from here:

lighthouse/lighthouse/environment/src/lib.rs

Lines 217 to 221 in 3e6b0bd

	let file_logging_layer = match config.path {
	None => {
	eprintln!("No logfile path provided, logging to file is disabled");
	None
	}

This looks like is because from here:

lighthouse/lighthouse/src/main.rs

Lines 569 to 588 in 3e6b0bd

	let mut log_path: Option<PathBuf> = clap_utils::parse_optional(matches, "logfile-dir")?;
	if log_path.is_none() {
	log_path = match matches.subcommand() {
	Some(("beacon_node", _)) => Some(
	parse_path_or_default(matches, "datadir")?
	.join(DEFAULT_BEACON_NODE_DIR)
	.join("logs"),
	),
	Some(("validator_client", vc_matches)) => {
	let base_path = if vc_matches.contains_id("validators-dir") {
	parse_path_or_default(vc_matches, "validators-dir")?
	} else {
	parse_path_or_default(matches, "datadir")?.join(DEFAULT_VALIDATOR_DIR)
	};
	Some(base_path.join("logs"))
	}
	_ => None,
	};
	}

where account_manager falls into the None arm of the match automatically. We can add an if condition in the logging in lighthouse/lighthouse/environment/src/lib.rs to only print it for some subcommands, but that sounds a bit of a hack. I am sure you have a better solution for this.

I think logfile-dir for account_manager is not really relevant, so would be nice to not have that line printed (current lighthouse db also have this line printed)

[chong-he]

Currently the flags are not sorted alphabetically. We can add display_order = 0 to make it sorted, as in the validator client and database cli files.

Suggested change

	help = "Use the wallet identified by this name"
	)]
	help = "Use the wallet identified by this name",
	display_order = 0
	)]

This applies to all flags in the cli.rs file

[chong-he]

Small nit pick, would be good to put this after the function parse_path_with_default_in_home_dir for consistency

[mergify]

Hi @eserilev, this pull request has been closed automatically due to 30 days of inactivity. If you’d like to continue working on it, feel free to reopen at any time.

[mergify]

Hi @eserilev, this pull request has been closed automatically due to 30 days of inactivity. If you’d like to continue working on it, feel free to reopen at any time.

[mergify]

Hi @eserilev, this pull request has been closed automatically due to 30 days of inactivity. If you’d like to continue working on it, feel free to reopen at any time.

[mergify]

Hi @eserilev, this pull request has been closed automatically due to 30 days of inactivity. If you’d like to continue working on it, feel free to reopen at any time.

[macladson]

I'll be dedicating some cycles soon to getting the existing clap PRs merged as I'm keen to see these happen!