384 add clippy to ci (#459)

* add clippy step for ci * clippy fixes * fix dependencies for clippy all-targets compilation * more clippy fixes * clippy in different step for integration test crate * yml format * rollback clippy warning fixes * clippy warning fixes main target * ignore some unused import warnings * avoid double reference * remove unnecessary clone * run cargo fmt * fix some clippy warnings * remove some warnings * unused imports, ignore local impl definition
pendulum-chain · Jun 14, 2024 · 47b0666 · 47b0666
1 parent 3835d27
commit 47b0666
Show file tree

Hide file tree

Showing 27 changed files with 139 additions and 114 deletions.
diff --git a/.github/workflows/test-code.yml b/.github/workflows/test-code.yml
@@ -9,31 +9,11 @@ on:
       - main
 
 jobs:
-  build-check:
-    runs-on: ubuntu-latest
-
-    steps:
-      - uses: actions/checkout@v3
-      - uses: ./.github/actions/shared
-
-      - name: Install toolchain
-        # Call `rustup show` as a hack so that the toolchain defined in rust-toolchain.toml is installed
-        run: rustup show
-
-      # Enable this for clippy linting.
-      # - name: Check and Lint Code
-      #   run: cargo +nightly-2021-12-01 clippy -- -D warnings
-
-      - name: Install Protoc
-        uses: arduino/setup-protoc@v1
-        with:
-          repo-token: ${{ secrets.GITHUB_TOKEN }}
-
-      - name: Check Code
-        run: cargo check
-
   test-code:
     runs-on: ubuntu-latest
+    env:
+      # Make sure CI fails on all warnings, including Clippy lints
+      RUSTFLAGS: "-Dwarnings"
 
     steps:
       - uses: actions/checkout@v3
@@ -64,3 +44,27 @@ jobs:
         with:
           toolchain: nightly-2024-04-18
           command: test
+
+      - name: Clippy -- Main
+        uses: actions-rs/cargo@v1
+        with:
+          toolchain: nightly-2024-04-18
+          command: clippy
+          args: --all-features -- -W clippy::all -A clippy::style -A forgetting_copy_types -A forgetting_references
+
+      - name: Clippy -- All Targets (except integration)
+        uses: actions-rs/cargo@v1
+        with:
+          toolchain: nightly-2024-04-18
+          command: clippy
+          # We are a bit more forgiving when it comes to the code in tests and only check for correctness
+          args: --workspace --all-features --all-targets --exclude runtime-integration-tests -- -A clippy::all -W clippy::correctness -A forgetting_copy_types -A forgetting_references
+
+      - name: Clippy -- Integration
+        uses: actions-rs/cargo@v1
+        with:
+          toolchain: nightly-2024-04-18
+          command: clippy
+          # We are a bit more forgiving when it comes to the code in tests and only check for correctness
+          args: --package runtime-integration-tests --all-features --all-targets -- -A clippy::all -W clippy::correctness -A forgetting_copy_types -A forgetting_references
+
diff --git a/chain-extensions/price/src/lib.rs b/chain-extensions/price/src/lib.rs
@@ -46,7 +46,7 @@ where
 	T: SysConfig + pallet_contracts::Config + dia_oracle::Config,
 	<T as SysConfig>::AccountId: UncheckedFrom<<T as SysConfig>::Hash> + AsRef<[u8]>,
 {
-	fn call<E: Ext>(&mut self, env: Environment<E, InitState>) -> Result<RetVal, DispatchError>
+	fn call<E>(&mut self, env: Environment<E, InitState>) -> Result<RetVal, DispatchError>
 	where
 		E: Ext<T = T>,
 		<E::T as SysConfig>::AccountId: UncheckedFrom<<E::T as SysConfig>::Hash> + AsRef<[u8]>,
@@ -74,7 +74,7 @@ where
 	}
 }
 
-fn get_coin_info<E: Ext, T>(
+fn get_coin_info<E, T>(
 	env: Environment<'_, '_, E, InitState>,
 	overhead_weight: Weight,
 ) -> Result<RetVal, DispatchError>

diff --git a/chain-extensions/token/src/lib.rs b/chain-extensions/token/src/lib.rs
@@ -73,9 +73,9 @@ where
 		+ orml_currencies_allowance_extension::Config,
 	<T as SysConfig>::AccountId: UncheckedFrom<<T as SysConfig>::Hash> + AsRef<[u8]>,
 	Tokens: orml_traits::MultiCurrency<AccountId, CurrencyId = CurrencyId>,
-	AccountId: sp_std::fmt::Debug + Decode,
+	AccountId: sp_std::fmt::Debug + Decode + core::clone::Clone,
 {
-	fn call<E: Ext>(&mut self, env: Environment<E, InitState>) -> Result<RetVal, DispatchError>
+	fn call<E>(&mut self, env: Environment<E, InitState>) -> Result<RetVal, DispatchError>
 	where
 		E: Ext<T = T>,
 		<E::T as SysConfig>::AccountId: UncheckedFrom<<E::T as SysConfig>::Hash> + AsRef<[u8]>,
@@ -92,23 +92,22 @@ where
 			0,
 		);
 
-		let result = match func_id {
+		match func_id {
 			FuncId::TotalSupply => total_supply(env, overhead_weight),
 			FuncId::BalanceOf => balance_of(env, overhead_weight),
 			FuncId::Transfer => transfer(env, overhead_weight),
 			FuncId::Allowance => allowance(env, overhead_weight),
 			FuncId::Approve => approve(env, overhead_weight),
 			FuncId::TransferFrom => transfer_from(env, overhead_weight),
-		};
-		result
+		}
 	}
 
 	fn enabled() -> bool {
 		true
 	}
 }
 
-fn total_supply<E: Ext, T, Tokens, AccountId>(
+fn total_supply<E, T, Tokens, AccountId>(
 	env: Environment<'_, '_, E, InitState>,
 	overhead_weight: Weight,
 ) -> Result<RetVal, DispatchError>
@@ -147,7 +146,7 @@ where
 	return Ok(RetVal::Converging(ChainExtensionOutcome::Success.as_u32()))
 }
 
-fn balance_of<E: Ext, T, Tokens, AccountId>(
+fn balance_of<E, T, Tokens, AccountId>(
 	env: Environment<'_, '_, E, InitState>,
 	overhead_weight: Weight,
 ) -> Result<RetVal, DispatchError>
@@ -189,7 +188,7 @@ where
 	return Ok(RetVal::Converging(ChainExtensionOutcome::Success.as_u32()))
 }
 
-fn transfer<E: Ext, T, Tokens, AccountId>(
+fn transfer<E, T, Tokens, AccountId>(
 	env: Environment<'_, '_, E, InitState>,
 	overhead_weight: Weight,
 ) -> Result<RetVal, DispatchError>
@@ -200,7 +199,7 @@ where
 		+ orml_currencies::Config<MultiCurrency = Tokens, AccountId = AccountId>
 		+ orml_currencies_allowance_extension::Config,
 	E: Ext<T = T>,
-	AccountId: sp_std::fmt::Debug,
+	AccountId: sp_std::fmt::Debug + core::clone::Clone,
 	Tokens: orml_traits::MultiCurrency<AccountId, CurrencyId = CurrencyId>,
 	(CurrencyId, AccountId, <Tokens as MultiCurrency<AccountId>>::Balance): Decode,
 {
@@ -229,14 +228,14 @@ where
 
 	<orml_currencies::Pallet<T> as MultiCurrency<T::AccountId>>::transfer(
 		currency_id,
-		&env.ext().caller(),
+		env.ext().caller(),
 		&recipient,
 		amount,
 	)?;
 	return Ok(RetVal::Converging(ChainExtensionOutcome::Success.as_u32()))
 }
 
-fn allowance<E: Ext, T, Tokens, AccountId>(
+fn allowance<E, T, Tokens, AccountId>(
 	env: Environment<'_, '_, E, InitState>,
 	overhead_weight: Weight,
 ) -> Result<RetVal, DispatchError>
@@ -281,7 +280,7 @@ where
 	return Ok(RetVal::Converging(ChainExtensionOutcome::Success.as_u32()))
 }
 
-fn approve<E: Ext, T, Tokens, AccountId>(
+fn approve<E, T, Tokens, AccountId>(
 	env: Environment<'_, '_, E, InitState>,
 	overhead_weight: Weight,
 ) -> Result<RetVal, DispatchError>
@@ -292,7 +291,7 @@ where
 		+ orml_currencies::Config<MultiCurrency = Tokens, AccountId = AccountId>
 		+ orml_currencies_allowance_extension::Config,
 	E: Ext<T = T>,
-	AccountId: sp_std::fmt::Debug,
+	AccountId: sp_std::fmt::Debug + core::clone::Clone,
 	Tokens: orml_traits::MultiCurrency<AccountId, CurrencyId = CurrencyId>,
 	(CurrencyId, AccountId, <Tokens as MultiCurrency<AccountId>>::Balance): Decode,
 {
@@ -320,14 +319,14 @@ where
 
 	orml_currencies_allowance_extension::Pallet::<T>::do_approve_transfer(
 		currency_id,
-		&env.ext().caller(),
+		env.ext().caller(),
 		&spender,
 		amount,
 	)?;
 	return Ok(RetVal::Converging(ChainExtensionOutcome::Success.as_u32()))
 }
 
-fn transfer_from<E: Ext, T, Tokens, AccountId>(
+fn transfer_from<E, T, Tokens, AccountId>(
 	env: Environment<'_, '_, E, InitState>,
 	overhead_weight: Weight,
 ) -> Result<RetVal, DispatchError>
@@ -338,7 +337,7 @@ where
 		+ orml_currencies::Config<MultiCurrency = Tokens, AccountId = AccountId>
 		+ orml_currencies_allowance_extension::Config,
 	E: Ext<T = T>,
-	AccountId: sp_std::fmt::Debug,
+	AccountId: sp_std::fmt::Debug + core::clone::Clone,
 	Tokens: orml_traits::MultiCurrency<AccountId, CurrencyId = CurrencyId>,
 	(AccountId, CurrencyId, AccountId, <Tokens as MultiCurrency<AccountId>>::Balance): Decode,
 {
@@ -372,7 +371,7 @@ where
 	orml_currencies_allowance_extension::Pallet::<T>::do_transfer_approved(
 		currency_id,
 		&owner,
-		&env.ext().caller(),
+		env.ext().caller(),
 		&recipient,
 		amount,
 	)?;

diff --git a/node/src/chain_spec.rs b/node/src/chain_spec.rs
@@ -34,7 +34,7 @@ const SAFE_XCM_VERSION: u32 = xcm::prelude::XCM_VERSION;
 pub fn create_pendulum_multisig_account(id: &str) -> AccountId {
 	let mut signatories: Vec<_> = pendulum::SUDO_SIGNATORIES
 		.iter()
-		.chain(vec![id].iter())
+		.chain([id].iter())
 		.map(|ss58| AccountId::from_ss58check(ss58).unwrap())
 		.collect();
 	signatories.sort();
@@ -384,7 +384,7 @@ pub fn pendulum_config() -> PendulumChainSpec {
 		pendulum::TREASURY_ALLOCATION * 20 / 100,
 	));
 
-	let multisig_identifiers = vec![
+	let multisig_identifiers = [
 		pendulum::MULTISIG_ID_GENESIS,
 		pendulum::MULTISIG_ID_TEAM,
 		pendulum::MULTISIG_ID_CL_RESERVES,

diff --git a/node/src/command.rs b/node/src/command.rs
@@ -404,19 +404,19 @@ pub fn run() -> Result<()> {
 						.map_err(|e| format!("Error: {:?}", e))?;
 
 				match runner.config().chain_spec.identify() {
-					ChainIdentity::Amplitude => runner.async_run(|config| {
+					ChainIdentity::Amplitude => runner.async_run(|_config| {
 						Ok((cmd.run::<Block, ExtendedHostFunctions<
 							sp_io::SubstrateHostFunctions,
 							<AmplitudeRuntimeExecutor as NativeExecutionDispatch>::ExtendHostFunctions,
 						>, _>(Some(substrate_info(BLOCK_TIME_MILLIS))), task_manager))
 					}),
-					ChainIdentity::Foucoco => runner.async_run(|config| {
+					ChainIdentity::Foucoco => runner.async_run(|_config| {
 						Ok((cmd.run::<Block, ExtendedHostFunctions<
 							sp_io::SubstrateHostFunctions,
 							<FoucocoRuntimeExecutor as NativeExecutionDispatch>::ExtendHostFunctions,
 						>, _>(Some(substrate_info(BLOCK_TIME_MILLIS))), task_manager))
 					}),
-					ChainIdentity::Pendulum => runner.async_run(|config| {
+					ChainIdentity::Pendulum => runner.async_run(|_config| {
 						Ok((cmd.run::<Block, ExtendedHostFunctions<
 							sp_io::SubstrateHostFunctions,
 							<PendulumRuntimeExecutor as NativeExecutionDispatch>::ExtendHostFunctions,

diff --git a/pallets/orml-currencies-allowance-extension/src/lib.rs b/pallets/orml-currencies-allowance-extension/src/lib.rs
@@ -238,7 +238,7 @@ pub mod pallet {
 	}
 }
 
-#[allow(clippy::forget_non_drop, clippy::swap_ptr_to_ref, clippy::forget_ref, clippy::forget_copy)]
+#[allow(clippy::forget_non_drop, clippy::swap_ptr_to_ref, forgetting_references, forgetting_copy_types)]
 #[cfg_attr(test, mockable)]
 impl<T: Config> Pallet<T> {
 	// Check the amount approved to be spent by an owner to a delegate

diff --git a/pallets/orml-tokens-management-extension/src/lib.rs b/pallets/orml-tokens-management-extension/src/lib.rs
@@ -124,14 +124,14 @@ pub mod pallet {
 				T::CurrencyIdChecker::is_valid_currency_id(&currency_id),
 				Error::<T>::NotOwnableCurrency
 			);
-			ensure!(!CurrencyData::<T>::contains_key(&currency_id), Error::<T>::AlreadyCreated);
+			ensure!(!CurrencyData::<T>::contains_key(currency_id), Error::<T>::AlreadyCreated);
 
 			let deposit = T::AssetDeposit::get();
 			ext::orml_tokens::reserve::<T>(T::DepositCurrency::get(), &creator, deposit)
 				.map_err(|_| Error::<T>::InsufficientBalance)?;
 
 			CurrencyData::<T>::insert(
-				currency_id.clone(),
+				currency_id,
 				CurrencyDetails {
 					owner: creator.clone(),
 					issuer: creator.clone(),
@@ -175,7 +175,7 @@ pub mod pallet {
 			ensure!(origin == currency_data.issuer, Error::<T>::NoPermission);
 
 			// do mint via orml-currencies
-			let _ = ext::orml_tokens::mint::<T>(currency_id, &to, amount)?;
+			ext::orml_tokens::mint::<T>(currency_id, &to, amount)?;
 
 			Self::deposit_event(Event::Mint { currency_id, to, amount });
 			Ok(())
@@ -207,7 +207,7 @@ pub mod pallet {
 			ensure!(origin == currency_data.admin, Error::<T>::NoPermission);
 
 			// do burn via orml-currencies
-			let _ = ext::orml_tokens::burn::<T>(currency_id, &from, amount)?;
+			ext::orml_tokens::burn::<T>(currency_id, &from, amount)?;
 
 			Self::deposit_event(Event::Burned { currency_id, from, amount });
 			Ok(())
@@ -231,7 +231,7 @@ pub mod pallet {
 		) -> DispatchResult {
 			let origin = ensure_signed(origin)?;
 
-			CurrencyData::<T>::try_mutate(currency_id.clone(), |maybe_details| {
+			CurrencyData::<T>::try_mutate(currency_id, |maybe_details| {
 				let details = maybe_details.as_mut().ok_or(Error::<T>::NotCreated)?;
 				ensure!(origin == details.owner, Error::<T>::NoPermission);
 
@@ -241,7 +241,7 @@ pub mod pallet {
 				details.owner = new_owner.clone();
 
 				// move reserved balance to the new owner's account
-				let _ = ext::orml_tokens::repatriate_reserve::<T>(
+				ext::orml_tokens::repatriate_reserve::<T>(
 					T::DepositCurrency::get(),
 					&origin,
 					&new_owner,
@@ -271,13 +271,13 @@ pub mod pallet {
 		) -> DispatchResult {
 			ensure_root(origin)?;
 
-			CurrencyData::<T>::try_mutate(currency_id.clone(), |maybe_details| {
+			CurrencyData::<T>::try_mutate(currency_id, |maybe_details| {
 				let details = maybe_details.as_mut().ok_or(Error::<T>::NotCreated)?;
 
 				if details.owner == new_owner {
 					return Ok(())
 				}
-				let _ = ext::orml_tokens::repatriate_reserve::<T>(
+				ext::orml_tokens::repatriate_reserve::<T>(
 					T::DepositCurrency::get(),
 					&details.owner,
 					&new_owner,
@@ -311,7 +311,7 @@ pub mod pallet {
 		) -> DispatchResult {
 			let origin = ensure_signed(origin)?;
 
-			CurrencyData::<T>::try_mutate(currency_id.clone(), |maybe_details| {
+			CurrencyData::<T>::try_mutate(currency_id, |maybe_details| {
 				let details = maybe_details.as_mut().ok_or(Error::<T>::NotCreated)?;
 
 				ensure!(origin == details.owner, Error::<T>::NoPermission);

diff --git a/pallets/parachain-staking/Cargo.toml b/pallets/parachain-staking/Cargo.toml
@@ -36,9 +36,10 @@ frame-benchmarking = {git = "https://github.com/paritytech/substrate", branch =
 [features]
 default = ["std"]
 runtime-benchmarks = [
-  "frame-benchmarking",
+  "frame-benchmarking/runtime-benchmarks",
   "frame-support/runtime-benchmarks",
   "frame-system/runtime-benchmarks",
+  "pallet-balances/runtime-benchmarks",
 ]
 std = [
   "frame-support/std",

diff --git a/pallets/parachain-staking/rpc/src/lib.rs b/pallets/parachain-staking/rpc/src/lib.rs
@@ -72,14 +72,14 @@ where
 		let at = at.unwrap_or_else(|| self.client.info().best_hash);
 
 		api.get_unclaimed_staking_rewards(at, account)
-			.map_err(|_e| internal_err(format!("Unable to get unclaimed staking rewards")))
+			.map_err(|_e| internal_err("Unable to get unclaimed staking rewards"))
 	}
 
 	fn get_staking_rates(&self, at: Option<<Block as BlockT>::Hash>) -> RpcResult<StakingRates> {
 		let api = self.client.runtime_api();
 		let at = at.unwrap_or_else(|| self.client.info().best_hash);
 
 		api.get_staking_rates(at)
-			.map_err(|_e| internal_err(format!("Unable to get staking rates")))
+			.map_err(|_e| internal_err("Unable to get staking rates"))
 	}
 }
diff --git a/pallets/parachain-staking/src/benchmarking.rs b/pallets/parachain-staking/src/benchmarking.rs
@@ -307,7 +307,7 @@ benchmarks! {
 		let n in (T::MinCollators::get() + 1) .. T::MaxTopCandidates::get() - 1;
 		let m in 0 .. T::MaxDelegatorsPerCollator::get();
 
-		let u = T::MaxUnstakeRequests::get() as u32 - 1;
+		let u = T::MaxUnstakeRequests::get() - 1;
 		let candidates = setup_collator_candidates::<T>(n, None);
 		for (i, c) in candidates.iter().enumerate() {
 			fill_delegators::<T>(m, c.clone(), i.saturated_into::<u32>());
@@ -532,7 +532,7 @@ benchmarks! {
 	}
 
 	unlock_unstaked {
-		let u in 1 .. (T::MaxUnstakeRequests::get() as u32 - 1);
+		let u in 1 .. (T::MaxUnstakeRequests::get() - 1);
 
 		let candidate = account("collator", 0u32, COLLATOR_ACCOUNT_SEED);
 		let free_balance = T::CurrencyBalance::from(u128::MAX);