CU-1uq4a7x - ValidateUnsigned instead of SignedExtension in Crowdloan

frame/crowdloan-rewards/src/lib.rs

@@ -47,15 +47,7 @@ Reference for proof mechanism: https://github.com/paritytech/polkadot/blob/maste
 	unused_extern_crates
 )]
 
-use codec::{Decode, Encode};
-use frame_support::{
-	pallet_prelude::{InvalidTransaction, ValidTransaction},
-	traits::IsSubType,
-	unsigned::{TransactionValidity, TransactionValidityError},
-};
 pub use pallet::*;
-use scale_info::TypeInfo;
-use sp_runtime::traits::{DispatchInfoOf, SignedExtension, Zero};
 
 pub mod models;
 
@@ -455,97 +447,42 @@ pub mod pallet {
 		);
 		Some(addr)
 	}
-}
-
-/// Validate `associate` calls prior to execution. Needed to avoid a DoS attack since they are
-/// otherwise free to place on chain.
-#[derive(Encode, Decode, Clone, Eq, PartialEq, TypeInfo)]
-#[scale_info(skip_type_params(T))]
-pub struct PrevalidateAssociation<T: Config + Send + Sync>(sp_std::marker::PhantomData<T>)
-where
-	<T as frame_system::Config>::Call: IsSubType<Call<T>>;
-
-impl<T: Config + Send + Sync> sp_std::fmt::Debug for PrevalidateAssociation<T>
-where
-	<T as frame_system::Config>::Call: IsSubType<Call<T>>,
-{
-	#[cfg(feature = "std")]
-	fn fmt(&self, f: &mut sp_std::fmt::Formatter) -> sp_std::fmt::Result {
-		write!(f, "PrevalidateAssociation")
-	}
-
-	#[cfg(not(feature = "std"))]
-	fn fmt(&self, _: &mut sp_std::fmt::Formatter) -> sp_std::fmt::Result {
-		Ok(())
-	}
-}
 
-#[allow(clippy::new_without_default)]
-impl<T: Config + Send + Sync> PrevalidateAssociation<T>
-where
-	<T as frame_system::Config>::Call: IsSubType<Call<T>>,
-{
-	/// Create new `SignedExtension` to validate crowdloan rewards association
-	pub fn new() -> Self {
-		Self(sp_std::marker::PhantomData)
-	}
-}
-
-impl<T: Config + Send + Sync> SignedExtension for PrevalidateAssociation<T>
-where
-	<T as frame_system::Config>::Call: IsSubType<Call<T>>,
-{
-	type AccountId = T::AccountId;
-	type Call = <T as frame_system::Config>::Call;
-	type AdditionalSigned = ();
-	type Pre = ();
-	const IDENTIFIER: &'static str = "PrevalidateAssociation";
-
-	fn additional_signed(&self) -> Result<Self::AdditionalSigned, TransactionValidityError> {
-		Ok(())
-	}
-
-	// <weight>
-	// The weight of this logic is included in the `associate` dispatchable.
-	// </weight>
-	fn validate(
-		&self,
-		_who: &Self::AccountId,
-		call: &Self::Call,
-		_info: &DispatchInfoOf<Self::Call>,
-		_len: usize,
-	) -> TransactionValidity {
-		use frame_support::traits::Get;
-
-		if let Some(Call::associate { reward_account, proof }) = IsSubType::is_sub_type(call) {
-			if Associations::<T>::get(reward_account).is_some() {
-				return InvalidTransaction::Custom(ValidityError::AlreadyAssociated as u8).into()
+	#[pallet::validate_unsigned]
+	impl<T: Config> ValidateUnsigned for Pallet<T> {
+		type Call = Call<T>;
+
+		fn validate_unsigned(_source: TransactionSource, call: &Self::Call) -> TransactionValidity {
+			if let Call::associate { reward_account, proof } = call {
+				if Associations::<T>::get(reward_account).is_some() {
+					return InvalidTransaction::Custom(ValidityError::AlreadyAssociated as u8).into()
+				}
+				let remote_account =
+					get_remote_account::<T>(proof.clone(), reward_account, T::Prefix::get())
+						.map_err(|_| {
+							Into::<TransactionValidityError>::into(InvalidTransaction::Custom(
+								ValidityError::InvalidProof as u8,
+							))
+						})?;
+				match Rewards::<T>::get(remote_account.clone()) {
+					None => InvalidTransaction::Custom(ValidityError::NoReward as u8).into(),
+					Some(reward) if reward.total.is_zero() =>
+						InvalidTransaction::Custom(ValidityError::NoReward as u8).into(),
+					Some(_) =>
+						ValidTransaction::with_tag_prefix("CrowdloanRewardsAssociationCheck")
+							.and_provides(remote_account)
+							.build(),
+				}
+			} else {
+				Err(InvalidTransaction::Call.into())
 			}
-
-			let remote_account =
-				get_remote_account::<T>(proof.clone(), reward_account, T::Prefix::get()).map_err(
-					|_| {
-						Into::<TransactionValidityError>::into(InvalidTransaction::Custom(
-							ValidityError::InvalidProof as u8,
-						))
-					},
-				)?;
-
-			match Rewards::<T>::get(remote_account) {
-				None => InvalidTransaction::Custom(ValidityError::NoReward as u8).into(),
-				Some(reward) if reward.total.is_zero() =>
-					InvalidTransaction::Custom(ValidityError::NoReward as u8).into(),
-				Some(_) => Ok(ValidTransaction::default()),
-			}
-		} else {
-			Ok(ValidTransaction::default())
 		}
 	}
-}
 
-#[repr(u8)]
-pub enum ValidityError {
-	InvalidProof = 0,
-	NoReward = 1,
-	AlreadyAssociated = 2,
+	#[repr(u8)]
+	pub enum ValidityError {
+		InvalidProof = 0,
+		NoReward = 1,
+		AlreadyAssociated = 2,
+	}
 }

frame/crowdloan-rewards/src/tests.rs

@@ -287,38 +287,21 @@ mod test_prevalidate_association {
 		with_rewards, with_rewards_default, ClaimKey, DEFAULT_NB_OF_CONTRIBUTORS,
 		DEFAULT_VESTING_PERIOD,
 	};
-
 	use crate::{
-		mocks::{Call, CrowdloanRewards, Origin, Test},
-		PrevalidateAssociation, ValidityError,
+		mocks::{CrowdloanRewards, Origin},
+		ValidityError,
 	};
-
 	use frame_support::{
 		assert_ok,
-		dispatch::{Dispatchable, GetDispatchInfo},
-		pallet_prelude::InvalidTransaction,
+		pallet_prelude::{InvalidTransaction, ValidateUnsigned},
 		unsigned::TransactionValidity,
-		weights::Pays,
 	};
-	use sp_runtime::{traits::SignedExtension, AccountId32};
+	use sp_runtime::{transaction_validity::TransactionSource, AccountId32};
 
-	fn setup_call(
-		remote_account: ClaimKey,
-		reward_account: &AccountId32,
-	) -> (TransactionValidity, Call) {
+	fn setup_call(remote_account: ClaimKey, reward_account: &AccountId32) -> TransactionValidity {
 		let proof = remote_account.proof(reward_account.clone());
-		let call = Call::CrowdloanRewards(crate::Call::associate {
-			reward_account: reward_account.clone(),
-			proof,
-		});
-		let dispatch_info = call.get_dispatch_info();
-		let validate_result = PrevalidateAssociation::<Test>::new().validate(
-			reward_account,
-			&call,
-			&dispatch_info,
-			0,
-		);
-		(validate_result, call)
+		let call = crate::Call::associate { reward_account: reward_account.clone(), proof };
+		CrowdloanRewards::validate_unsigned(TransactionSource::External, &call)
 	}
 
 	#[test]
@@ -335,24 +318,11 @@ mod test_prevalidate_association {
 			}
 
 			for (reward_account, remote_account) in accounts {
-				let (validate_result, call) = setup_call(remote_account, &reward_account);
-
+				let validate_result = setup_call(remote_account, &reward_account);
 				assert_eq!(
 					validate_result,
 					Err(InvalidTransaction::Custom(ValidityError::AlreadyAssociated as u8).into())
 				);
-
-				// make sure that invalid transactions are not free
-				assert!(matches!(
-					call.dispatch(Origin::root()),
-					Err(sp_runtime::DispatchErrorWithPostInfo {
-						post_info: frame_support::dispatch::PostDispatchInfo {
-							actual_weight: _,
-							pays_fee: Pays::Yes
-						},
-						error: _
-					})
-				));
 			}
 		});
 	}
@@ -363,24 +333,11 @@ mod test_prevalidate_association {
 			assert_ok!(CrowdloanRewards::initialize(Origin::root()));
 
 			for (reward_account, remote_account) in accounts {
-				let (validate_result, call) = setup_call(remote_account, &reward_account);
-
+				let validate_result = setup_call(remote_account, &reward_account);
 				assert_eq!(
 					validate_result,
 					Err(InvalidTransaction::Custom(ValidityError::NoReward as u8).into())
 				);
-
-				// make sure that invalid transactions are not free
-				assert!(matches!(
-					call.dispatch(Origin::root()),
-					Err(sp_runtime::DispatchErrorWithPostInfo {
-						post_info: frame_support::dispatch::PostDispatchInfo {
-							actual_weight: _,
-							pays_fee: Pays::Yes
-						},
-						error: _
-					})
-				));
 			}
 		});
 	}

integration-tests/simnode/src/chain/dali.rs

@@ -53,7 +53,6 @@ impl substrate_simnode::ChainInfo for ChainInfo {
 			),
 			system::CheckWeight::<Self::Runtime>::new(),
 			transaction_payment::ChargeTransactionPayment::<Self::Runtime>::from(0),
-			crowdloan_rewards::PrevalidateAssociation::<Self::Runtime>::new(),
 		)
 	}
 }

integration-tests/simnode/src/chain/picasso.rs

@@ -54,7 +54,6 @@ impl substrate_simnode::ChainInfo for ChainInfo {
 			),
 			system::CheckWeight::<Self::Runtime>::new(),
 			transaction_payment::ChargeTransactionPayment::<Self::Runtime>::from(0),
-			crowdloan_rewards::PrevalidateAssociation::<Self::Runtime>::new(),
 		)
 	}
 }

runtime/dali/src/lib.rs

@@ -385,7 +385,6 @@ where
 			system::CheckNonce::<Runtime>::from(nonce),
 			system::CheckWeight::<Runtime>::new(),
 			transaction_payment::ChargeTransactionPayment::<Runtime>::from(tip),
-			crowdloan_rewards::PrevalidateAssociation::<Runtime>::new(),
 		);
 		let raw_payload = SignedPayload::new(call, extra)
 			.map_err(|_e| {
@@ -912,7 +911,7 @@ construct_runtime!(
 		AssetsRegistry: assets_registry::{Pallet, Call, Storage, Event<T>} = 55,
 		GovernanceRegistry: governance_registry::{Pallet, Call, Storage, Event<T>} = 56,
 		Assets: assets::{Pallet, Call, Storage} = 57,
-		CrowdloanRewards: crowdloan_rewards::{Pallet, Call, Storage, Event<T>} = 58,
+		CrowdloanRewards: crowdloan_rewards::{Pallet, Call, Storage, Event<T>, ValidateUnsigned} = 58,
 		Vesting: vesting::{Call, Event<T>, Pallet, Storage} = 59,
 		BondedFinance: bonded_finance::{Call, Event<T>, Pallet, Storage} = 60,
 		DutchAuction: dutch_auction::{Pallet, Call, Storage, Event<T>} = 61,
@@ -934,7 +933,6 @@ pub type SignedExtra = (
 	system::CheckNonce<Runtime>,
 	system::CheckWeight<Runtime>,
 	transaction_payment::ChargeTransactionPayment<Runtime>,
-	crowdloan_rewards::PrevalidateAssociation<Runtime>,
 );
 /// Unchecked extrinsic type as expected by this runtime.
 pub type UncheckedExtrinsic = generic::UncheckedExtrinsic<Address, Call, Signature, SignedExtra>;

runtime/picasso/src/lib.rs

@@ -103,7 +103,7 @@ pub const VERSION: RuntimeVersion = RuntimeVersion {
 	spec_version: 2000,
 	impl_version: 1,
 	apis: RUNTIME_API_VERSIONS,
-	transaction_version: 2,
+	transaction_version: 1,
 };
 
 /// The version information used to identify this runtime when compiled natively.
@@ -385,7 +385,6 @@ where
 			system::CheckNonce::<Runtime>::from(nonce),
 			system::CheckWeight::<Runtime>::new(),
 			transaction_payment::ChargeTransactionPayment::<Runtime>::from(tip),
-			crowdloan_rewards::PrevalidateAssociation::<Runtime>::new(),
 		);
 		let raw_payload = SignedPayload::new(call, extra)
 			.map_err(|_e| {
@@ -816,7 +815,7 @@ construct_runtime!(
 		Factory: currency_factory::{Pallet, Storage, Event<T>} = 53,
 		GovernanceRegistry: governance_registry::{Pallet, Call, Storage, Event<T>} = 54,
 		Assets: assets::{Pallet, Call, Storage} = 55,
-		CrowdloanRewards: crowdloan_rewards::{Pallet, Call, Storage, Event<T>} = 56,
+		CrowdloanRewards: crowdloan_rewards::{Pallet, Call, Storage, Event<T>, ValidateUnsigned} = 56,
 		Vesting: vesting::{Call, Event<T>, Pallet, Storage} = 57,
 		BondedFinance: bonded_finance::{Call, Event<T>, Pallet, Storage} = 58,
 	}
@@ -836,7 +835,6 @@ pub type SignedExtra = (
 	system::CheckNonce<Runtime>,
 	system::CheckWeight<Runtime>,
 	transaction_payment::ChargeTransactionPayment<Runtime>,
-	crowdloan_rewards::PrevalidateAssociation<Runtime>,
 );
 
 /// Unchecked extrinsic type as expected by this runtime.