Add clippy and rustfmt to CI (paritytech#143)

* Add clippy/rustfmt checks to CI

Add clippy check to CI

Apply uses correctly

Specify runs-on

Use checkout specified from check-prettier

Try specifying toolchain with clippy as components

Apply to correct action

Specify nightly without date

Use 'rustup component add clippy' instead of toolchain

Uses on steps

Typo

Specify rustfmt and clippy in rust-toolchain file

Try only rustfmt

Undo

Revert rust-toolchain file

Add --workspace arg to clippy

* rustup clippy in init.sh

* Introduce 'install toolchain' step for Clippy

* Add rustfmt check

Add rustfmt check

--

* Update .rustfmt.toml to remove license checking (for now)

* Enforce rustfmt rules on codebase

* Update Cargo.lock

* Resolve clippy complaints

* Apply rustfmt

* Appease clippy @ node/parachain/src/chain_spec.rs

Co-authored-by: Joshy Orndorff <[email protected]>

* Prefer unwrap_or_else to unwrap_or

* Avoid borrowed-box

* Add a pre-commit hook for rustfmt/clippy using rusty-hook

Co-authored-by: Joshy Orndorff <[email protected]>
Co-authored-by: Joshy Orndorff <[email protected]>

Author: notlesh

.github/workflows/release.yml

@@ -98,6 +98,36 @@ jobs:
       - name: Check with Prettier
         run: npx prettier --check --ignore-path .gitignore  '**/*.(yml|js|ts|json)'
 
+  check-clippy:
+    name: "Code checks"
+    runs-on: ubuntu-latest
+    steps:
+      - name: Checkout
+        if: github.event_name == 'pull_request_target'
+        uses: actions/checkout@v2
+        with:
+          fetch-depth: 0
+          ref: refs/pull/${{ github.event.pull_request.number }}/merge
+
+      - name: Checkout
+        if: github.event_name != 'pull_request_target'
+        uses: actions/checkout@v2
+
+      - name: Install toolchain
+        uses: actions-rs/toolchain@v1
+        with:
+          profile: minimal
+          toolchain: nightly-2020-10-03
+          components: clippy, rustfmt
+          target: wasm32-unknown-unknown
+          default: true
+
+          # - name: Check with Clippy
+        # run: cargo clippy --workspace --tests -- -D warnings
+
+      - name: Format code with rustfmt
+        run: cargo fmt -- --check
+
   ####### Building and Testing binaries #######
 
   build:

.rustfmt.toml

@@ -5,4 +5,6 @@ reorder_imports = true
 hard_tabs = true
 max_width = 100
 
-license_template_path = "FILE_TEMPLATE"
+# TODO: add template for license checking.
+#       this will still fail for some licenses where e.g. Parity's license is used
+# license_template_path = "FILE_TEMPLATE"

.rusty-hook.toml

@@ -0,0 +1,5 @@
+[hooks]
+pre-commit = "cargo fmt -- --check && cargo clippy"
+
+[logging]
+verbose = true

Cargo.lock

@@ -16,8 +16,8 @@
 
 use cumulus_primitives::ParaId;
 use moonbeam_runtime::{
-	AccountId, BalancesConfig, GenesisConfig, SudoConfig, SystemConfig,
-	ParachainInfoConfig, WASM_BINARY, EthereumChainIdConfig, EVMConfig, EthereumConfig
+	AccountId, BalancesConfig, EVMConfig, EthereumChainIdConfig, EthereumConfig, GenesisConfig,
+	ParachainInfoConfig, SudoConfig, SystemConfig, WASM_BINARY,
 };
 use sc_chain_spec::{ChainSpecExtension, ChainSpecGroup};
 use sc_service::ChainType;
@@ -40,7 +40,7 @@ pub struct Extensions {
 
 impl Extensions {
 	/// Try to get the extension from the given `ChainSpec`.
-	pub fn try_get(chain_spec: &Box<dyn sc_service::ChainSpec>) -> Option<&Self> {
+	pub fn try_get(chain_spec: &dyn sc_service::ChainSpec) -> Option<&Self> {
 		sc_chain_spec::get_extension(chain_spec.extensions())
 	}
 }
@@ -53,22 +53,15 @@ pub fn get_chain_spec(para_id: ParaId) -> ChainSpec {
 		move || {
 			testnet_genesis(
 				AccountId::from_str("6Be02d1d3665660d22FF9624b7BE0551ee1Ac91b").unwrap(),
-				vec![
-					AccountId::from_str("6Be02d1d3665660d22FF9624b7BE0551ee1Ac91b").unwrap(),
-				],
+				vec![AccountId::from_str("6Be02d1d3665660d22FF9624b7BE0551ee1Ac91b").unwrap()],
 				para_id,
 				1280, //ChainId
 			)
 		},
 		vec![],
 		None,
 		None,
-		Some(
-			serde_json::from_str(
-				"{\"tokenDecimals\": 18}"
-			)
-			.expect("Provided valid json map")
-		),
+		Some(serde_json::from_str("{\"tokenDecimals\": 18}").expect("Provided valid json map")),
 		Extensions {
 			relay_chain: "local_testnet".into(),
 			para_id: para_id.into(),
@@ -97,8 +90,10 @@ fn testnet_genesis(
 				.collect(),
 		}),
 		pallet_sudo: Some(SudoConfig { key: root_key }),
-		parachain_info: Some(ParachainInfoConfig { parachain_id: para_id }),
-		pallet_ethereum_chain_id: Some(EthereumChainIdConfig { chain_id: chain_id }),
+		parachain_info: Some(ParachainInfoConfig {
+			parachain_id: para_id,
+		}),
+		pallet_ethereum_chain_id: Some(EthereumChainIdConfig { chain_id }),
 		pallet_evm: Some(EVMConfig {
 			accounts: BTreeMap::new(),
 		}),

node/parachain/src/chain_spec.rs

@@ -16,7 +16,6 @@
 
 use std::path::PathBuf;
 
-use sc_cli;
 use structopt::StructOpt;
 
 /// Sub-commands supported by the collator.

node/parachain/src/cli.rs

@@ -24,8 +24,8 @@ use log::info;
 use moonbeam_runtime::Block;
 use polkadot_parachain::primitives::AccountIdConversion;
 use sc_cli::{
-	ChainSpec, CliConfiguration, ImportParams, KeystoreParams, NetworkParams, Result,
-	RuntimeVersion, SharedParams, SubstrateCli, DefaultConfigurationValues,
+	ChainSpec, CliConfiguration, DefaultConfigurationValues, ImportParams, KeystoreParams,
+	NetworkParams, Result, RuntimeVersion, SharedParams, SubstrateCli,
 };
 use sc_service::{
 	config::{BasePath, PrometheusConfig},
@@ -117,15 +117,15 @@ impl SubstrateCli for RelayChainCli {
 	}
 
 	fn load_spec(&self, id: &str) -> std::result::Result<Box<dyn sc_service::ChainSpec>, String> {
-		polkadot_cli::Cli::from_iter([RelayChainCli::executable_name().to_string()].iter())
-			.load_spec(id)
+		polkadot_cli::Cli::from_iter([RelayChainCli::executable_name()].iter()).load_spec(id)
 	}
 
 	fn native_runtime_version(chain_spec: &Box<dyn ChainSpec>) -> &'static RuntimeVersion {
 		polkadot_cli::Cli::native_runtime_version(chain_spec)
 	}
 }
 
+#[allow(clippy::borrowed_box)]
 fn extract_genesis_wasm(chain_spec: &Box<dyn sc_service::ChainSpec>) -> Result<Vec<u8>> {
 	let mut storage = chain_spec.build_storage()?;
 
@@ -254,14 +254,14 @@ pub fn run() -> Result<()> {
 				// TODO
 				let key = sp_core::Pair::generate().0;
 
-				let extension = chain_spec::Extensions::try_get(&config.chain_spec);
+				let extension = chain_spec::Extensions::try_get(&*config.chain_spec);
 				let relay_chain_id = extension.map(|e| e.relay_chain.clone());
 				let para_id = extension.map(|e| e.para_id);
 
 				let polkadot_cli = RelayChainCli::new(
 					config.base_path.as_ref().map(|x| x.path().join("polkadot")),
 					relay_chain_id,
-					[RelayChainCli::executable_name().to_string()]
+					[RelayChainCli::executable_name()]
 						.iter()
 						.chain(cli.relaychain_args.iter()),
 				);

node/parachain/src/command.rs

@@ -18,6 +18,8 @@ use cumulus_network::build_block_announce_validator;
 use cumulus_service::{
 	prepare_node_config, start_collator, start_full_node, StartCollatorParams, StartFullNodeParams,
 };
+use frontier_consensus::FrontierBlockImport;
+use moonbeam_runtime::{opaque::Block, RuntimeApi};
 use polkadot_primitives::v0::CollatorPair;
 use sc_executor::native_executor_instance;
 pub use sc_executor::NativeExecutor;
@@ -26,8 +28,6 @@ use sp_core::Pair;
 use sp_runtime::traits::BlakeTwo256;
 use sp_trie::PrefixedMemoryDB;
 use std::sync::Arc;
-use frontier_consensus::FrontierBlockImport;
-use moonbeam_runtime::{RuntimeApi, opaque::Block};
 // Our native executor instance.
 native_executor_instance!(
 	pub Executor,
@@ -42,26 +42,17 @@ type FullBackend = TFullBackend<Block>;
 ///
 /// Use this macro if you don't actually need the full service, but just the builder in order to
 /// be able to perform chain operations.
+#[allow(clippy::type_complexity)]
 pub fn new_partial(
 	config: &Configuration,
 ) -> Result<
 	PartialComponents<
 		FullClient,
 		FullBackend,
 		(),
-		sp_consensus::import_queue::BasicQueue<
-			Block,
-			PrefixedMemoryDB<BlakeTwo256>,
-		>,
-		sc_transaction_pool::FullPool<
-			Block,
-			FullClient,
-		>,
-		FrontierBlockImport<
-			Block,
-			Arc<FullClient>,
-			FullClient,
-		>,
+		sp_consensus::import_queue::BasicQueue<Block, PrefixedMemoryDB<BlakeTwo256>>,
+		sc_transaction_pool::FullPool<Block, FullClient>,
+		FrontierBlockImport<Block, Arc<FullClient>, FullClient>,
 	>,
 	sc_service::Error,
 > {
@@ -80,18 +71,14 @@ pub fn new_partial(
 		client.clone(),
 	);
 
-	let frontier_block_import = FrontierBlockImport::new(
-		client.clone(),
-		client.clone(),
-		true
-	);
+	let frontier_block_import = FrontierBlockImport::new(client.clone(), client.clone(), true);
 
 	let import_queue = cumulus_consensus::import_queue::import_queue(
 		client.clone(),
 		frontier_block_import.clone(),
 		inherent_data_providers.clone(),
 		&task_manager.spawn_handle(),
-		registry.clone(),
+		registry,
 	)?;
 
 	let params = PartialComponents {
@@ -119,7 +106,7 @@ async fn start_node_impl<RB>(
 	id: polkadot_primitives::v0::Id,
 	validator: bool,
 	_rpc_ext_builder: RB,
-) -> sc_service::error::Result<(TaskManager,Arc<FullClient>)>
+) -> sc_service::error::Result<(TaskManager, Arc<FullClient>)>
 where
 	RB: Fn(
 			Arc<TFullClient<Block, RuntimeApi, Executor>>,
@@ -163,13 +150,13 @@ where
 	let block_import = params.other;
 	let (network, network_status_sinks, system_rpc_tx, start_network) =
 		sc_service::build_network(sc_service::BuildNetworkParams {
-				config: &parachain_config,
-				client: client.clone(),
-				transaction_pool: transaction_pool.clone(),
-				spawn_handle: task_manager.spawn_handle(),
-				import_queue,
-				on_demand: None,
-				block_announce_validator_builder: Some(Box::new(|_| block_announce_validator)),
+			config: &parachain_config,
+			client: client.clone(),
+			transaction_pool: transaction_pool.clone(),
+			spawn_handle: task_manager.spawn_handle(),
+			import_queue,
+			on_demand: None,
+			block_announce_validator_builder: Some(Box::new(|_| block_announce_validator)),
 		})?;
 
 	let is_authority = parachain_config.role.is_authority();
@@ -191,17 +178,14 @@ where
 				command_sink: None,
 			};
 
-			moonbeam_rpc::create_full(
-				deps,
-				subscription_task_executor.clone()
-			)
+			moonbeam_rpc::create_full(deps, subscription_task_executor.clone())
 		})
 	};
 
 	sc_service::spawn_tasks(sc_service::SpawnTasksParams {
 		on_demand: None,
 		remote_blockchain: None,
-		rpc_extensions_builder: rpc_extensions_builder,
+		rpc_extensions_builder,
 		client: client.clone(),
 		transaction_pool: transaction_pool.clone(),
 		task_manager: &mut task_manager,
@@ -232,7 +216,7 @@ where
 
 		let params = StartCollatorParams {
 			para_id: id,
-			block_import: block_import,
+			block_import,
 			proposer_factory,
 			inherent_data_providers: params.inherent_data_providers,
 			block_status: client.clone(),