Add tests & Service's Configuration has optional fields that shouldn't be optional (#4842)

Related to #4776 
Related to https://github.com/paritytech/polkadot/pull/832

To summarize the changes:
1. I did not manage to validate with types the service's Configuration. But I did reduce the possibility of errors by moving all the "fill" functions to their respective structopts
2. I split params.rs to multiple modules: one module params for just CLI parameters and one module commands for CLI subcommands (and RunCmd). Every command and params are in their own file so things are grouped better together and easier to remove
3. I removed the run and run_subcommand helpers as they are not helping much anymore. Running a command is always a set of 3 commands: 1. init 2. update config 3. run. This still allow the user to change the config before arguments get parsed or right after.
4. I added tests for all subcommands.
5. [deleted]

Overall the aim is to improve the situation with the Configuration and the optional parameters, add tests, make the API more consistent and simpler.

substrate/bin/node/cli/src/command.rs

@@ -14,13 +14,13 @@
 // You should have received a copy of the GNU General Public License
 // along with Substrate.  If not, see <http://www.gnu.org/licenses/>.
 
-use sc_cli::{VersionInfo, error};
+use sc_cli::VersionInfo;
 use sc_service::{Roles as ServiceRoles};
 use node_transaction_factory::RuntimeAdapter;
 use crate::{Cli, service, ChainSpec, load_spec, Subcommand, factory_impl::FactoryState};
 
 /// Parse command line arguments into service configuration.
-pub fn run<I, T>(args: I, version: VersionInfo) -> error::Result<()>
+pub fn run<I, T>(args: I, version: VersionInfo) -> sc_cli::Result<()>
 where
 	I: Iterator<Item = T>,
 	T: Into<std::ffi::OsString> + Clone,
@@ -28,19 +28,22 @@ where
 	let args: Vec<_> = args.collect();
 	let opt = sc_cli::from_iter::<Cli, _>(args.clone(), &version);
 
-	let mut config = sc_service::Configuration::new(&version);
+	let mut config = sc_service::Configuration::from_version(&version);
 
 	match opt.subcommand {
-		None => sc_cli::run(
-			config,
-			opt.run,
-			service::new_light,
-			service::new_full,
-			load_spec,
-			&version,
-		),
+		None => {
+			opt.run.init(&version)?;
+			opt.run.update_config(&mut config, load_spec, &version)?;
+			opt.run.run(
+				config,
+				service::new_light,
+				service::new_full,
+				&version,
+			)
+		},
 		Some(Subcommand::Inspect(cmd)) => {
-			cmd.init(&mut config, load_spec, &version)?;
+			cmd.init(&version)?;
+			cmd.update_config(&mut config, load_spec, &version)?;
 
 			let client = sc_service::new_full_client::<
 				node_runtime::Block, node_runtime::RuntimeApi, node_executor::Executor, _, _,
@@ -50,25 +53,27 @@ where
 			cmd.run(inspect)
 		},
 		Some(Subcommand::Benchmark(cmd)) => {
-			cmd.init(&mut config, load_spec, &version)?;
+			cmd.init(&version)?;
+			cmd.update_config(&mut config, load_spec, &version)?;
 
 			cmd.run::<_, _, node_runtime::Block, node_executor::Executor>(config)
 		},
 		Some(Subcommand::Factory(cli_args)) => {
-			sc_cli::init(&cli_args.shared_params, &version)?;
-			sc_cli::init_config(&mut config, &cli_args.shared_params, &version, load_spec)?;
-			sc_cli::fill_import_params(
+			cli_args.shared_params.init(&version)?;
+			cli_args.shared_params.update_config(&mut config, load_spec, &version)?;
+			cli_args.import_params.update_config(
 				&mut config,
-				&cli_args.import_params,
 				ServiceRoles::FULL,
 				cli_args.shared_params.dev,
 			)?;
 
-			sc_cli::fill_config_keystore_in_memory(&mut config)?;
+			config.use_in_memory_keystore()?;
 
 			match ChainSpec::from(config.expect_chain_spec().id()) {
 				Some(ref c) if c == &ChainSpec::Development || c == &ChainSpec::LocalTestnet => {},
-				_ => panic!("Factory is only supported for development and local testnet."),
+				_ => return Err(
+					"Factory is only supported for development and local testnet.".into()
+				),
 			}
 
 			// Setup tracing.
@@ -77,7 +82,9 @@ where
 					cli_args.import_params.tracing_receiver.into(), tracing_targets
 				);
 				if let Err(e) = tracing::subscriber::set_global_default(subscriber) {
-					panic!("Unable to set global default subscriber {}", e);
+					return Err(
+						format!("Unable to set global default subscriber {}", e).into()
+					);
 				}
 			}
 
@@ -96,12 +103,13 @@ where
 
 			Ok(())
 		},
-		Some(Subcommand::Base(subcommand)) => sc_cli::run_subcommand(
-			config,
-			subcommand,
-			load_spec,
-			|config: service::NodeConfiguration| Ok(new_full_start!(config).0),
-			&version,
-		),
+		Some(Subcommand::Base(subcommand)) => {
+			subcommand.init(&version)?;
+			subcommand.update_config(&mut config, load_spec, &version)?;
+			subcommand.run(
+				config,
+				|config: service::NodeConfiguration| Ok(new_full_start!(config).0),
+			)
+		},
 	}
 }