Use Clippy in CI (#162)

* Configure CI to run clippy * Fix up clippy issues in vesting * Fix up clippy issues in utilities * Fix up clippy issues in tokens * Fix up clippy issues in schedule-update * Fix up clippy issues in oracle * Fix up clippy issues in gradually-update * Fix up clippy issues in currencies * Fix up clippy issues in auction * Fix formatting * Deny warnings when running clippy on CI * Fix clippy issues in benchmarking
open-web3-stack · May 14, 2020 · 4cf321e · 4cf321e
1 parent 1710692
commit 4cf321e
Show file tree

Hide file tree

Showing 12 changed files with 56 additions and 42 deletions.
diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml
@@ -36,6 +36,10 @@ jobs:
       run: rustup target add wasm32-unknown-unknown
     - name: Check format
       run: make dev-format-check
+    - name: Install clippy
+      run: rustup component add clippy
+    - name: Run clippy
+      run: cargo clippy -- -D warnings
     - name: Update
       run: cargo update
     - name: Check for Wasm

diff --git a/auction/src/lib.rs b/auction/src/lib.rs
@@ -1,4 +1,6 @@
 #![cfg_attr(not(feature = "std"), no_std)]
+// Disable the following two lints since they originate from an external macro (namely decl_storage)
+#![allow(clippy::string_lit_as_bytes)]
 
 use frame_support::{decl_error, decl_event, decl_module, decl_storage, ensure, IterableStorageDoubleMap, Parameter};
 use frame_system::{self as system, ensure_signed};

diff --git a/benchmarking/src/lib.rs b/benchmarking/src/lib.rs
@@ -133,15 +133,15 @@ pub use sp_runtime::traits::{Dispatchable, One, Zero};
 ///
 /// ```ignore
 /// sort_vector {
-/// 	let x in 1 .. 10000;
-/// 	let mut m = Vec::<u32>::new();
-/// 	for i in (0..x).rev() {
-/// 		m.push(i);
-/// 	}
+///     let x in 1 .. 10000;
+///     let mut m = Vec::<u32>::new();
+///     for i in (0..x).rev() {
+///         m.push(i);
+///     }
 /// }: {
-/// 	m.sort();
+///     m.sort();
 /// } verify {
-/// 	ensure!(m[0] == 0, "You forgot to sort!")
+///     ensure!(m[0] == 0, "You forgot to sort!")
 /// }
 /// ```
 ///

diff --git a/currencies/src/lib.rs b/currencies/src/lib.rs
@@ -585,24 +585,24 @@ where
 
 	fn set_lock(lock_id: LockIdentifier, who: &AccountId, amount: Self::Balance) {
 		Currency::set_lock(
-			lock_id.into(),
+			lock_id,
 			who,
 			BalanceConvert::from(amount).into(),
-			(WithdrawReason::Transfer | WithdrawReason::Reserve).into(),
+			WithdrawReason::Transfer | WithdrawReason::Reserve,
 		);
 	}
 
 	fn extend_lock(lock_id: LockIdentifier, who: &AccountId, amount: Self::Balance) {
 		Currency::extend_lock(
-			lock_id.into(),
+			lock_id,
 			who,
 			BalanceConvert::from(amount).into(),
-			(WithdrawReason::Transfer | WithdrawReason::Reserve).into(),
+			WithdrawReason::Transfer | WithdrawReason::Reserve,
 		);
 	}
 
 	fn remove_lock(lock_id: LockIdentifier, who: &AccountId) {
-		Currency::remove_lock(lock_id.into(), who);
+		Currency::remove_lock(lock_id, who);
 	}
 }
 
@@ -644,7 +644,7 @@ where
 		value: Self::Balance,
 		status: BalanceStatus,
 	) -> result::Result<Self::Balance, DispatchError> {
-		Currency::repatriate_reserved(slashed, beneficiary, BalanceConvert::from(value).into(), status.into())
+		Currency::repatriate_reserved(slashed, beneficiary, BalanceConvert::from(value).into(), status)
 			.map(|a| BalanceConvert::from(a).into())
 	}
 }
diff --git a/gradually-update/src/lib.rs b/gradually-update/src/lib.rs
@@ -1,4 +1,6 @@
 #![cfg_attr(not(feature = "std"), no_std)]
+// Disable the following two lints since they originate from an external macro (namely decl_storage)
+#![allow(clippy::string_lit_as_bytes)]
 
 use codec::{Decode, Encode};
 use frame_support::{decl_error, decl_event, decl_module, decl_storage, ensure, storage, traits::Get};
@@ -116,6 +118,8 @@ impl<T: Trait> Module<T> {
 		}
 
 		let mut gradually_updates = GraduallyUpdates::get();
+
+		#[allow(clippy::redundant_clone)] // FIXME: This looks like a bug in clippy
 		for (i, update) in gradually_updates.clone().iter().enumerate() {
 			let current_value = storage::unhashed::get::<StorageValue>(&update.key).unwrap_or_default();
 			let current_value_u128 = u128::from_le_bytes(Self::convert_vec_to_u8(&current_value));
@@ -127,12 +131,11 @@ impl<T: Trait> Module<T> {
 
 			let target_u128 = u128::from_le_bytes(Self::convert_vec_to_u8(&update.target_value));
 
-			let new_value_u128: u128;
-			if current_value_u128 > target_u128 {
-				new_value_u128 = (current_value_u128.checked_sub(step_u128).unwrap()).max(target_u128);
+			let new_value_u128 = if current_value_u128 > target_u128 {
+				(current_value_u128.checked_sub(step_u128).unwrap()).max(target_u128)
 			} else {
-				new_value_u128 = (current_value_u128.checked_add(step_u128).unwrap()).min(target_u128);
-			}
+				(current_value_u128.checked_add(step_u128).unwrap()).min(target_u128)
+			};
 
 			// current_value equal target_value, remove gradually_update
 			if new_value_u128 == target_u128 {
@@ -155,10 +158,11 @@ impl<T: Trait> Module<T> {
 		GraduallyUpdateBlockNumber::<T>::put(now);
 	}
 
+	#[allow(clippy::ptr_arg)]
 	fn convert_vec_to_u8(input: &StorageValue) -> [u8; 16] {
 		let mut array: [u8; 16] = [0; 16];
 		for (i, v) in input.iter().enumerate() {
-			array[i] = v.clone();
+			array[i] = *v;
 		}
 		array
 	}

diff --git a/oracle/src/default_combine_data.rs b/oracle/src/default_combine_data.rs
@@ -24,12 +24,7 @@ where
 		let now = T::Time::now();
 		let mut valid_values = values
 			.into_iter()
-			.filter_map(|x| {
-				if x.timestamp + expires_in > now {
-					return Some(x);
-				}
-				None
-			})
+			.filter(|x| x.timestamp + expires_in > now)
 			.collect::<Vec<TimestampedValueOf<T>>>();
 
 		let count = valid_values.len() as u32;
@@ -41,6 +36,6 @@ where
 		valid_values.sort_by(|a, b| a.value.cmp(&b.value));
 
 		let median_index = count / 2;
-		return Some(valid_values[median_index as usize].clone());
+		Some(valid_values[median_index as usize].clone())
 	}
 }
diff --git a/oracle/src/lib.rs b/oracle/src/lib.rs
@@ -1,4 +1,6 @@
 #![cfg_attr(not(feature = "std"), no_std)]
+// Disable the following two lints since they originate from an external macro (namely decl_storage)
+#![allow(clippy::string_lit_as_bytes)]
 
 mod default_combine_data;
 mod mock;
@@ -149,7 +151,7 @@ impl<T: Trait> Module<T> {
 	}
 }
 
-#[derive(Encode, Decode, Clone, Eq, PartialEq)]
+#[derive(Encode, Decode, Clone, Eq, PartialEq, Default)]
 pub struct CheckOperator<T: Trait + Send + Sync>(marker::PhantomData<T>);
 
 impl<T: Trait + Send + Sync> CheckOperator<T> {
@@ -204,7 +206,7 @@ impl<T: Trait + Send + Sync> SignedExtension for CheckOperator<T> {
 				..Default::default()
 			});
 		}
-		return Ok(ValidTransaction::default());
+		Ok(ValidTransaction::default())
 	}
 }
 

diff --git a/schedule-update/src/lib.rs b/schedule-update/src/lib.rs
@@ -1,4 +1,6 @@
 #![cfg_attr(not(feature = "std"), no_std)]
+// Disable the following two lints since they originate from an external macro (namely decl_storage)
+#![allow(clippy::string_lit_as_bytes)]
 
 use codec::{Decode, Encode};
 use frame_support::{
@@ -160,7 +162,7 @@ decl_module! {
 					origin = frame_system::RawOrigin::Root.into();
 				}
 
-				let result = call.dispatch(origin.clone());
+				let result = call.dispatch(origin);
 				if let Err(e) = result {
 					 Self::deposit_event(RawEvent::ScheduleDispatchFail(id, e.error));
 				} else {
@@ -184,7 +186,7 @@ decl_module! {
 					origin = frame_system::RawOrigin::Root.into();
 				}
 
-				let result = call.dispatch(origin.clone());
+				let result = call.dispatch(origin);
 				if let Err(e) = result {
 					Self::deposit_event(RawEvent::ScheduleDispatchFail(id, e.error));
 				} else {

diff --git a/tokens/src/lib.rs b/tokens/src/lib.rs
@@ -32,6 +32,8 @@
 //! The tokens module depends on the `GenesisConfig`. Endowed accounts could be configured in genesis configs.
 
 #![cfg_attr(not(feature = "std"), no_std)]
+// Disable the following two lints since they originate from an external macro (namely decl_storage)
+#![allow(clippy::redundant_closure_call, clippy::string_lit_as_bytes)]
 
 use codec::{Decode, Encode};
 use frame_support::{decl_error, decl_event, decl_module, decl_storage, ensure, traits::Get, Parameter};
@@ -429,10 +431,7 @@ impl<T: Trait> MultiLockableCurrency<T::AccountId> for Module<T> {
 		if amount.is_zero() {
 			return;
 		}
-		let mut new_lock = Some(BalanceLock {
-			id: lock_id,
-			amount: amount,
-		});
+		let mut new_lock = Some(BalanceLock { id: lock_id, amount });
 		let mut locks = Self::locks(currency_id, who)
 			.into_iter()
 			.filter_map(|lock| {
@@ -455,10 +454,7 @@ impl<T: Trait> MultiLockableCurrency<T::AccountId> for Module<T> {
 		if amount.is_zero() {
 			return;
 		}
-		let mut new_lock = Some(BalanceLock {
-			id: lock_id,
-			amount: amount,
-		});
+		let mut new_lock = Some(BalanceLock { id: lock_id, amount });
 		let mut locks = Self::locks(currency_id, who)
 			.into_iter()
 			.filter_map(|lock| {

diff --git a/utilities/src/fixed_u128.rs b/utilities/src/fixed_u128.rs
@@ -274,7 +274,7 @@ impl<'de> Deserialize<'de> for FixedU128 {
 		D: Deserializer<'de>,
 	{
 		let s = String::deserialize(deserializer)?;
-		FixedU128::try_from_u128_str(&s).map_err(|err_str| de::Error::custom(err_str))
+		FixedU128::try_from_u128_str(&s).map_err(de::Error::custom)
 	}
 }
 

diff --git a/utilities/src/linked_item.rs b/utilities/src/linked_item.rs
@@ -32,7 +32,7 @@ where
 	}
 
 	fn read(key: &Key, value: Option<Value>) -> LinkedItem<Value> {
-		Storage::get(&(key.clone(), value)).unwrap_or_else(|| Default::default())
+		Storage::get(&(key.clone(), value)).unwrap_or_else(Default::default)
 	}
 
 	fn take(key: &Key, value: Value) -> LinkedItem<Value> {

diff --git a/vesting/src/lib.rs b/vesting/src/lib.rs
@@ -21,6 +21,8 @@
 //! - `update_vesting_schedules` - Update all vesting schedules under an account, `root` origin required.
 
 #![cfg_attr(not(feature = "std"), no_std)]
+// Disable the following two lints since they originate from an external macro (namely decl_storage)
+#![allow(clippy::redundant_closure_call, clippy::string_lit_as_bytes)]
 
 use codec::{Decode, Encode, HasCompact};
 use frame_support::{
@@ -87,6 +89,13 @@ impl<BlockNumber: AtLeast32Bit + Copy, Balance: AtLeast32Bit + Copy> VestingSche
 
 pub type BalanceOf<T> = <<T as Trait>::Currency as Currency<<T as frame_system::Trait>::AccountId>>::Balance;
 pub type VestingScheduleOf<T> = VestingSchedule<<T as frame_system::Trait>::BlockNumber, BalanceOf<T>>;
+pub type ScheduledItem<T> = (
+	<T as frame_system::Trait>::AccountId,
+	<T as frame_system::Trait>::BlockNumber,
+	<T as frame_system::Trait>::BlockNumber,
+	u32,
+	BalanceOf<T>,
+);
 
 pub trait Trait: frame_system::Trait {
 	type Event: From<Event<Self>> + Into<<Self as frame_system::Trait>::Event>;
@@ -106,7 +115,7 @@ decl_storage! {
 	}
 
 	add_extra_genesis {
-		config(vesting): Vec<(T::AccountId, T::BlockNumber, T::BlockNumber, u32, BalanceOf<T>)>;
+		config(vesting): Vec<ScheduledItem<T>>;
 	}
 }
 
@@ -206,7 +215,7 @@ impl<T: Trait> Module<T> {
 	) -> DispatchResult {
 		let schedule_amount = Self::ensure_valid_vesting_schedule(&schedule)?;
 		let total_amount = Self::locked_balance(to)
-			.checked_add(&schedule_amount.into())
+			.checked_add(&schedule_amount)
 			.ok_or(Error::<T>::NumOverflow)?;
 
 		T::Currency::transfer(from, to, schedule_amount, ExistenceRequirement::AllowDeath)?;