sc-cli: Fix bugs after switching to clap3 (#10920)

* sc-cli: Fix bugs after switching to clap3

Before switching to clap3 we support cli options like `--reserved-nodes A B` and after you needed to
pass `--reserved-nodes` cli option multiple times `--reserved-nodes A --reserved-nodes B`. This is
fixed by setting `multiple_occurrences(true)` option. This also done for all the other `Vec` cli
options in `sc-cli`. Besides that `--sync` wasn't supporting case insensitive parsing of the value.
This is now also supported. For both regressions a test is added. Besides that the pr removes all
the `rename_all = PascalCase` attributes, because they are not needed. All other `ArgEnum`s were
checked and all are already using `ignore_case(true)`.

* Bring back `PascalCase`, because otherwise it falls back to `kebab-case`...

substrate/client/cli/src/arg_enums.rs

@@ -15,8 +15,8 @@
 
 // You should have received a copy of the GNU General Public License
 // along with this program. If not, see <https://www.gnu.org/licenses/>.
-// NOTE: we allow missing docs here because arg_enum! creates the function variants without doc
+
-#![allow(missing_docs)]
+//! Definitions of [`ArgEnum`] types.
 
 use clap::ArgEnum;
 
@@ -90,6 +90,7 @@ impl Into<sc_service::config::WasmExecutionMethod> for WasmExecutionMethod {
 #[derive(Debug, Copy, Clone, PartialEq, Eq, ArgEnum)]
 #[clap(rename_all = "PascalCase")]
 pub enum TracingReceiver {
+	/// Output the tracing records using the log.
 	Log,
 }
 
@@ -101,25 +102,33 @@ impl Into<sc_tracing::TracingReceiver> for TracingReceiver {
 	}
 }
 
-#[allow(missing_docs)]
+/// The type of the node key.
 #[derive(Debug, Copy, Clone, PartialEq, Eq, ArgEnum)]
 #[clap(rename_all = "PascalCase")]
 pub enum NodeKeyType {
+	/// Use ed25519.
 	Ed25519,
 }
 
+/// The crypto scheme to use.
 #[derive(Debug, Copy, Clone, PartialEq, Eq, ArgEnum)]
 #[clap(rename_all = "PascalCase")]
 pub enum CryptoScheme {
+	/// Use ed25519.
 	Ed25519,
+	/// Use sr25519.
 	Sr25519,
+	/// Use
 	Ecdsa,
 }
 
+/// The type of the output format.
 #[derive(Debug, Copy, Clone, PartialEq, Eq, ArgEnum)]
 #[clap(rename_all = "PascalCase")]
 pub enum OutputType {
+	/// Output as json.
 	Json,
+	/// Output as text.
 	Text,
 }
 
@@ -127,13 +136,13 @@ pub enum OutputType {
 #[derive(Debug, Copy, Clone, PartialEq, Eq, ArgEnum)]
 #[clap(rename_all = "PascalCase")]
 pub enum ExecutionStrategy {
-	// Execute with native build (if available, WebAssembly otherwise).
+	/// Execute with native build (if available, WebAssembly otherwise).
 	Native,
-	// Only execute with the WebAssembly build.
+	/// Only execute with the WebAssembly build.
 	Wasm,
-	// Execute with both native (where available) and WebAssembly builds.
+	/// Execute with both native (where available) and WebAssembly builds.
 	Both,
-	// Execute with the native build if possible; if it fails, then execute with WebAssembly.
+	/// Execute with the native build if possible; if it fails, then execute with WebAssembly.
 	NativeElseWasm,
 }
 
@@ -165,12 +174,12 @@ impl ExecutionStrategy {
 #[derive(Debug, Copy, Clone, PartialEq, ArgEnum)]
 #[clap(rename_all = "PascalCase")]
 pub enum RpcMethods {
-	// Expose every RPC method only when RPC is listening on `localhost`,
+	/// Expose every RPC method only when RPC is listening on `localhost`,
-	// otherwise serve only safe RPC methods.
+	/// otherwise serve only safe RPC methods.
 	Auto,
-	// Allow only a safe subset of RPC methods.
+	/// Allow only a safe subset of RPC methods.
 	Safe,
-	// Expose every RPC method (even potentially unsafe ones).
+	/// Expose every RPC method (even potentially unsafe ones).
 	Unsafe,
 }
 
@@ -224,22 +233,25 @@ impl Database {
 #[derive(Debug, Clone, ArgEnum)]
 #[clap(rename_all = "PascalCase")]
 pub enum OffchainWorkerEnabled {
+	/// Always have offchain worker enabled.
 	Always,
+	/// Never enable the offchain worker.
 	Never,
+	/// Only enable the offchain worker when running as validator.
 	WhenValidating,
 }
 
 /// Syncing mode.
-#[derive(Debug, Clone, Copy, ArgEnum)]
+#[derive(Debug, Clone, Copy, ArgEnum, PartialEq)]
 #[clap(rename_all = "PascalCase")]
 pub enum SyncMode {
-	// Full sync. Donwnload end verify all blocks.
+	/// Full sync. Download end verify all blocks.
 	Full,
-	// Download blocks without executing them. Download latest state with proofs.
+	/// Download blocks without executing them. Download latest state with proofs.
 	Fast,
-	// Download blocks without executing them. Download latest state without proofs.
+	/// Download blocks without executing them. Download latest state without proofs.
 	FastUnsafe,
-	// Prove finality and download the latest state.
+	/// Prove finality and download the latest state.
 	Warp,
 }
 

substrate/client/cli/src/params/network_params.rs

@@ -34,11 +34,11 @@ use std::{borrow::Cow, path::PathBuf};
 #[derive(Debug, Clone, Args)]
 pub struct NetworkParams {
 	/// Specify a list of bootnodes.
-	#[clap(long, value_name = "ADDR")]
+	#[clap(long, value_name = "ADDR", multiple_values(true))]
 	pub bootnodes: Vec<MultiaddrWithPeerId>,
 
 	/// Specify a list of reserved node addresses.
-	#[clap(long, value_name = "ADDR")]
+	#[clap(long, value_name = "ADDR", multiple_values(true))]
 	pub reserved_nodes: Vec<MultiaddrWithPeerId>,
 
 	/// Whether to only synchronize the chain with reserved nodes.
@@ -54,7 +54,7 @@ pub struct NetworkParams {
 
 	/// The public address that other nodes will use to connect to it.
 	/// This can be used if there's a proxy in front of this node.
-	#[clap(long, value_name = "PUBLIC_ADDR")]
+	#[clap(long, value_name = "PUBLIC_ADDR", multiple_values(true))]
 	pub public_addr: Vec<Multiaddr>,
 
 	/// Listen on this multiaddress.
@@ -62,7 +62,7 @@ pub struct NetworkParams {
 	/// By default:
 	/// If `--validator` is passed: `/ip4/0.0.0.0/tcp/<port>` and `/ip6/[::]/tcp/<port>`.
 	/// Otherwise: `/ip4/0.0.0.0/tcp/<port>/ws` and `/ip6/[::]/tcp/<port>/ws`.
-	#[clap(long, value_name = "LISTEN_ADDR")]
+	#[clap(long, value_name = "LISTEN_ADDR", multiple_values(true))]
 	pub listen_addr: Vec<Multiaddr>,
 
 	/// Specify p2p protocol TCP port.
@@ -137,7 +137,7 @@ pub struct NetworkParams {
 	/// - `Fast`: Download blocks and the latest state only.
 	///
 	/// - `FastUnsafe`: Same as `Fast`, but skip downloading state proofs.
-	#[clap(long, arg_enum, value_name = "SYNC_MODE", default_value = "Full")]
+	#[clap(long, arg_enum, value_name = "SYNC_MODE", default_value = "Full", ignore_case(true))]
 	pub sync: SyncMode,
 }
 
@@ -237,3 +237,55 @@ impl NetworkParams {
 		}
 	}
 }
+
+#[cfg(test)]
+mod tests {
+	use super::*;
+	use clap::Parser;
+
+	#[derive(Parser)]
+	struct Cli {
+		#[clap(flatten)]
+		network_params: NetworkParams,
+	}
+
+	#[test]
+	fn reserved_nodes_multiple_values_and_occurrences() {
+		let params = Cli::try_parse_from([
+			"",
+			"--reserved-nodes",
+			"/ip4/0.0.0.0/tcp/501/p2p/12D3KooWEBo1HUPQJwiBmM5kSeg4XgiVxEArArQdDarYEsGxMfbS",
+			"/ip4/0.0.0.0/tcp/502/p2p/12D3KooWEBo1HUPQJwiBmM5kSeg4XgiVxEArArQdDarYEsGxMfbS",
+			"--reserved-nodes",
+			"/ip4/0.0.0.0/tcp/503/p2p/12D3KooWEBo1HUPQJwiBmM5kSeg4XgiVxEArArQdDarYEsGxMfbS",
+		])
+		.expect("Parses network params");
+
+		let expected = vec![
+			MultiaddrWithPeerId::try_from(
+				"/ip4/0.0.0.0/tcp/501/p2p/12D3KooWEBo1HUPQJwiBmM5kSeg4XgiVxEArArQdDarYEsGxMfbS"
+					.to_string(),
+			)
+			.unwrap(),
+			MultiaddrWithPeerId::try_from(
+				"/ip4/0.0.0.0/tcp/502/p2p/12D3KooWEBo1HUPQJwiBmM5kSeg4XgiVxEArArQdDarYEsGxMfbS"
+					.to_string(),
+			)
+			.unwrap(),
+			MultiaddrWithPeerId::try_from(
+				"/ip4/0.0.0.0/tcp/503/p2p/12D3KooWEBo1HUPQJwiBmM5kSeg4XgiVxEArArQdDarYEsGxMfbS"
+					.to_string(),
+			)
+			.unwrap(),
+		];
+
+		assert_eq!(expected, params.network_params.reserved_nodes);
+	}
+
+	#[test]
+	fn sync_ingores_case() {
+		let params = Cli::try_parse_from(["", "--sync", "wArP"]).expect("Parses network params");
+
+		assert_eq!(SyncMode::Warp, params.network_params.sync);
+	}
+}

substrate/client/cli/src/params/shared_params.rs

@@ -46,7 +46,7 @@ pub struct SharedParams {
 	///
 	/// Log levels (least to most verbose) are error, warn, info, debug, and trace.
 	/// By default, all targets log `info`. The global log level can be set with -l<level>.
-	#[clap(short = 'l', long, value_name = "LOG_PATTERN")]
+	#[clap(short = 'l', long, value_name = "LOG_PATTERN", multiple_values(true))]
 	pub log: Vec<String>,
 
 	/// Enable detailed log output.

substrate/client/network/src/config.rs

@@ -295,7 +295,7 @@ pub fn parse_addr(mut addr: Multiaddr) -> Result<(PeerId, Multiaddr), ParseErr>
 /// assert_eq!(addr.peer_id.to_base58(), "QmSk5HQbn6LhUwDiNMseVUjuRYhEtYj4aUZ6WfWoGURpdV");
 /// assert_eq!(addr.multiaddr.to_string(), "/ip4/198.51.100.19/tcp/30333");
 /// ```
-#[derive(Debug, Clone, serde::Serialize, serde::Deserialize)]
+#[derive(Debug, Clone, serde::Serialize, serde::Deserialize, PartialEq)]
 #[serde(try_from = "String", into = "String")]
 pub struct MultiaddrWithPeerId {
 	/// Address of the node.