Avoid duplicate function definitions

Avoid duplicate function definitions for:
- ensure_owner_or_root()
- ensure_not_halted()
- set_owner()
- set_operating_mode() / set_operational()

Signed-off-by: Serban Iorga <serban@parity.io>

bridges/modules/grandpa/src/lib.rs

@@ -37,12 +37,12 @@
 #![allow(clippy::large_enum_variant)]
 
 use bp_header_chain::{justification::GrandpaJustification, InitializationData};
-use bp_runtime::{BlockNumberOf, Chain, HashOf, HasherOf, HeaderOf};
+use bp_runtime::{BlockNumberOf, Chain, HashOf, HasherOf, HeaderOf, OwnedBridgeModule};
 use finality_grandpa::voter_set::VoterSet;
 use frame_support::{ensure, fail};
-use frame_system::{ensure_signed, RawOrigin};
+use frame_system::ensure_signed;
 use sp_finality_grandpa::{ConsensusLog, GRANDPA_ENGINE_ID};
-use sp_runtime::traits::{BadOrigin, Header as HeaderT, Zero};
+use sp_runtime::traits::{Header as HeaderT, Zero};
 use sp_std::{boxed::Box, convert::TryInto};
 
 mod extension;
@@ -117,6 +117,18 @@ pub mod pallet {
 		}
 	}
 
+	impl<T: Config<I>, I: 'static> OwnedBridgeModule<T> for Pallet<T, I> {
+		const LOG_TARGET: &'static str = "runtime::bridge-grandpa";
+		const OPERATING_MODE_KEY: &'static str = "IsHalted";
+		type OwnerStorage = PalletOwner<T, I>;
+		type OperatingMode = bool;
+		type OperatingModeStorage = IsHalted<T, I>;
+
+		fn is_halted() -> bool {
+			Self::OperatingModeStorage::get()
+		}
+	}
+
 	#[pallet::call]
 	impl<T: Config<I>, I: 'static> Pallet<T, I> {
 		/// Verify a target header is finalized according to the given finality proof.
@@ -135,7 +147,7 @@ pub mod pallet {
 			finality_target: Box<BridgedHeader<T, I>>,
 			justification: GrandpaJustification<BridgedHeader<T, I>>,
 		) -> DispatchResultWithPostInfo {
-			ensure_operational::<T, I>()?;
+			Self::ensure_not_halted().map_err(Error::<T, I>::BridgeModule)?;
 			let _ = ensure_signed(origin)?;
 
 			ensure!(Self::request_count() < T::MaxRequests::get(), <Error<T, I>>::TooManyRequests);
@@ -198,7 +210,7 @@ pub mod pallet {
 			origin: OriginFor<T>,
 			init_data: super::InitializationData<BridgedHeader<T, I>>,
 		) -> DispatchResultWithPostInfo {
-			ensure_owner_or_root::<T, I>(origin)?;
+			Self::ensure_owner_or_root(origin)?;
 
 			let init_allowed = !<BestFinalized<T, I>>::exists();
 			ensure!(init_allowed, <Error<T, I>>::AlreadyInitialized);
@@ -217,43 +229,16 @@ pub mod pallet {
 		///
 		/// May only be called either by root, or by `PalletOwner`.
 		#[pallet::weight((T::DbWeight::get().reads_writes(1, 1), DispatchClass::Operational))]
-		pub fn set_owner(
+		pub fn set_owner(origin: OriginFor<T>, new_owner: Option<T::AccountId>) -> DispatchResult {
-			origin: OriginFor<T>,
+			<Self as OwnedBridgeModule<_>>::set_owner(origin, new_owner)
-			new_owner: Option<T::AccountId>,
-		) -> DispatchResultWithPostInfo {
-			ensure_owner_or_root::<T, I>(origin)?;
-			match new_owner {
-				Some(new_owner) => {
-					PalletOwner::<T, I>::put(&new_owner);
-					log::info!(target: "runtime::bridge-grandpa", "Setting pallet Owner to: {:?}", new_owner);
-				},
-				None => {
-					PalletOwner::<T, I>::kill();
-					log::info!(target: "runtime::bridge-grandpa", "Removed Owner of pallet.");
-				},
-			}
-
-			Ok(().into())
 		}
 
 		/// Halt or resume all pallet operations.
 		///
 		/// May only be called either by root, or by `PalletOwner`.
 		#[pallet::weight((T::DbWeight::get().reads_writes(1, 1), DispatchClass::Operational))]
-		pub fn set_operational(
+		pub fn set_operational(origin: OriginFor<T>, operational: bool) -> DispatchResult {
-			origin: OriginFor<T>,
+			Self::set_operating_mode(origin, !operational)
-			operational: bool,
-		) -> DispatchResultWithPostInfo {
-			ensure_owner_or_root::<T, I>(origin)?;
-			<IsHalted<T, I>>::put(!operational);
-
-			if operational {
-				log::info!(target: "runtime::bridge-grandpa", "Resuming pallet operations.");
-			} else {
-				log::warn!(target: "runtime::bridge-grandpa", "Stopping pallet operations.");
-			}
-
-			Ok(().into())
 		}
 	}
 
@@ -310,7 +295,7 @@ pub mod pallet {
 
 	/// If true, all pallet transactions are failed immediately.
 	#[pallet::storage]
-	pub(super) type IsHalted<T: Config<I>, I: 'static = ()> = StorageValue<_, bool, ValueQuery>;
+	pub type IsHalted<T: Config<I>, I: 'static = ()> = StorageValue<_, bool, ValueQuery>;
 
 	#[pallet::genesis_config]
 	pub struct GenesisConfig<T: Config<I>, I: 'static = ()> {
@@ -364,10 +349,10 @@ pub mod pallet {
 		NotInitialized,
 		/// The pallet has already been initialized.
 		AlreadyInitialized,
-		/// All pallet operations are halted.
-		Halted,
 		/// The storage proof doesn't contains storage root. So it is invalid for given header.
 		StorageRootMismatch,
+		/// Error generated by the `OwnedBridgeModule` trait.
+		BridgeModule(bp_runtime::OwnedBridgeModuleError),
 	}
 
 	/// Check the given header for a GRANDPA scheduled authority set change. If a change
@@ -513,26 +498,6 @@ pub mod pallet {
 			insert_header::<T, I>(header, hash);
 		}
 	}
-
-	/// Ensure that the origin is either root, or `PalletOwner`.
-	fn ensure_owner_or_root<T: Config<I>, I: 'static>(origin: T::Origin) -> Result<(), BadOrigin> {
-		match origin.into() {
-			Ok(RawOrigin::Root) => Ok(()),
-			Ok(RawOrigin::Signed(ref signer))
-				if Some(signer) == <PalletOwner<T, I>>::get().as_ref() =>
-				Ok(()),
-			_ => Err(BadOrigin),
-		}
-	}
-
-	/// Ensure that the pallet is in operational mode (not halted).
-	fn ensure_operational<T: Config<I>, I: 'static>() -> Result<(), Error<T, I>> {
-		if <IsHalted<T, I>>::get() {
-			Err(<Error<T, I>>::Halted)
-		} else {
-			Ok(())
-		}
-	}
 }
 
 impl<T: Config<I>, I: 'static> Pallet<T, I> {
@@ -799,7 +764,10 @@ mod tests {
 			initialize_substrate_bridge();
 
 			assert_ok!(Pallet::<TestRuntime>::set_operational(Origin::root(), false));
-			assert_noop!(submit_finality_proof(1), Error::<TestRuntime>::Halted);
+			assert_noop!(
+				submit_finality_proof(1),
+				Error::<TestRuntime>::BridgeModule(bp_runtime::OwnedBridgeModuleError::Halted)
+			);
 
 			assert_ok!(Pallet::<TestRuntime>::set_operational(Origin::root(), true));
 			assert_ok!(submit_finality_proof(1));

bridges/modules/messages/src/lib.rs

@@ -61,17 +61,16 @@ use bp_messages::{
 	OutboundMessageDetails, Parameter as MessagesParameter, UnrewardedRelayer,
 	UnrewardedRelayersState,
 };
-use bp_runtime::{ChainId, Size};
+use bp_runtime::{ChainId, OwnedBridgeModule, Size};
 use codec::{Decode, Encode};
 use frame_support::{
 	fail,
 	traits::Get,
 	weights::{Pays, PostDispatchInfo},
 };
-use frame_system::RawOrigin;
 use num_traits::{SaturatingAdd, Zero};
 use sp_core::H256;
-use sp_runtime::traits::{BadOrigin, Convert};
+use sp_runtime::traits::Convert;
 use sp_std::{
 	cell::RefCell, cmp::PartialOrd, collections::vec_deque::VecDeque, marker::PhantomData,
 	ops::RangeInclusive, prelude::*,
@@ -217,6 +216,18 @@ pub mod pallet {
 	#[pallet::without_storage_info]
 	pub struct Pallet<T, I = ()>(PhantomData<(T, I)>);
 
+	impl<T: Config<I>, I: 'static> OwnedBridgeModule<T> for Pallet<T, I> {
+		const LOG_TARGET: &'static str = "runtime::bridge-messages";
+		const OPERATING_MODE_KEY: &'static str = "PalletOperatingMode";
+		type OwnerStorage = PalletOwner<T, I>;
+		type OperatingMode = OperatingMode;
+		type OperatingModeStorage = PalletOperatingMode<T, I>;
+
+		fn is_halted() -> bool {
+			Self::OperatingModeStorage::get() == OperatingMode::Halted
+		}
+	}
+
 	#[pallet::call]
 	impl<T: Config<I>, I: 'static> Pallet<T, I> {
 		/// Change `PalletOwner`.
@@ -224,18 +235,7 @@ pub mod pallet {
 		/// May only be called either by root, or by `PalletOwner`.
 		#[pallet::weight((T::DbWeight::get().reads_writes(1, 1), DispatchClass::Operational))]
 		pub fn set_owner(origin: OriginFor<T>, new_owner: Option<T::AccountId>) -> DispatchResult {
-			ensure_owner_or_root::<T, I>(origin)?;
+			<Self as OwnedBridgeModule<_>>::set_owner(origin, new_owner)
-			match new_owner {
-				Some(new_owner) => {
-					PalletOwner::<T, I>::put(&new_owner);
-					log::info!(target: "runtime::bridge-messages", "Setting pallet Owner to: {:?}", new_owner);
-				},
-				None => {
-					PalletOwner::<T, I>::kill();
-					log::info!(target: "runtime::bridge-messages", "Removed Owner of pallet.");
-				},
-			}
-			Ok(())
 		}
 
 		/// Halt or resume all/some pallet operations.
@@ -246,14 +246,7 @@ pub mod pallet {
 			origin: OriginFor<T>,
 			operating_mode: OperatingMode,
 		) -> DispatchResult {
-			ensure_owner_or_root::<T, I>(origin)?;
+			<Self as OwnedBridgeModule<_>>::set_operating_mode(origin, operating_mode)
-			PalletOperatingMode::<T, I>::put(operating_mode);
-			log::info!(
-				target: "runtime::bridge-messages",
-				"Setting messages pallet operating mode to {:?}.",
-				operating_mode,
-			);
-			Ok(())
 		}
 
 		/// Update pallet parameter.
@@ -267,7 +260,7 @@ pub mod pallet {
 			origin: OriginFor<T>,
 			parameter: T::Parameter,
 		) -> DispatchResult {
-			ensure_owner_or_root::<T, I>(origin)?;
+			Self::ensure_owner_or_root(origin)?;
 			parameter.save();
 			Self::deposit_event(Event::ParameterUpdated(parameter));
 			Ok(())
@@ -297,7 +290,7 @@ pub mod pallet {
 			nonce: MessageNonce,
 			additional_fee: T::OutboundMessageFee,
 		) -> DispatchResultWithPostInfo {
-			ensure_not_halted::<T, I>()?;
+			Self::ensure_not_halted().map_err(Error::<T, I>::BridgeModule)?;
 			// if someone tries to pay for already-delivered message, we're rejecting this intention
 			// (otherwise this additional fee will be locked forever in relayers fund)
 			//
@@ -368,7 +361,7 @@ pub mod pallet {
 			messages_count: u32,
 			dispatch_weight: Weight,
 		) -> DispatchResultWithPostInfo {
-			ensure_not_halted::<T, I>()?;
+			Self::ensure_not_halted().map_err(Error::<T, I>::BridgeModule)?;
 			let relayer_id_at_this_chain = ensure_signed(origin)?;
 
 			// reject transactions that are declaring too many messages
@@ -512,7 +505,7 @@ pub mod pallet {
 			proof: MessagesDeliveryProofOf<T, I>,
 			relayers_state: UnrewardedRelayersState,
 		) -> DispatchResultWithPostInfo {
-			ensure_not_halted::<T, I>()?;
+			Self::ensure_not_halted().map_err(Error::<T, I>::BridgeModule)?;
 
 			// why do we need to know the weight of this (`receive_messages_delivery_proof`) call?
 			// Because we may want to return some funds for messages that are not processed by the
@@ -669,8 +662,8 @@ pub mod pallet {
 
 	#[pallet::error]
 	pub enum Error<T, I = ()> {
-		/// All pallet operations are halted.
+		/// Pallet is not in Normal operating mode.
-		Halted,
+		NotOperatingNormally,
 		/// Message has been treated as invalid by chain verifier.
 		MessageRejectedByChainVerifier,
 		/// Message has been treated as invalid by lane verifier.
@@ -695,6 +688,8 @@ pub mod pallet {
 		/// The number of actually confirmed messages is going to be larger than the number of
 		/// messages in the proof. This may mean that this or bridged chain storage is corrupted.
 		TryingToConfirmMoreMessagesThanExpected,
+		/// Error generated by the `OwnedBridgeModule` trait.
+		BridgeModule(bp_runtime::OwnedBridgeModuleError),
 	}
 
 	/// Optional pallet owner.
@@ -975,30 +970,10 @@ where
 	relayers_rewards
 }
 
-/// Ensure that the origin is either root, or `PalletOwner`.
-fn ensure_owner_or_root<T: Config<I>, I: 'static>(origin: T::Origin) -> Result<(), BadOrigin> {
-	match origin.into() {
-		Ok(RawOrigin::Root) => Ok(()),
-		Ok(RawOrigin::Signed(ref signer))
-			if Some(signer) == Pallet::<T, I>::module_owner().as_ref() =>
-			Ok(()),
-		_ => Err(BadOrigin),
-	}
-}
-
 /// Ensure that the pallet is in normal operational mode.
 fn ensure_normal_operating_mode<T: Config<I>, I: 'static>() -> Result<(), Error<T, I>> {
 	if PalletOperatingMode::<T, I>::get() != OperatingMode::Normal {
-		Err(Error::<T, I>::Halted)
+		Err(Error::<T, I>::NotOperatingNormally)
-	} else {
-		Ok(())
-	}
-}
-
-/// Ensure that the pallet is not halted.
-fn ensure_not_halted<T: Config<I>, I: 'static>() -> Result<(), Error<T, I>> {
-	if PalletOperatingMode::<T, I>::get() == OperatingMode::Halted {
-		Err(Error::<T, I>::Halted)
 	} else {
 		Ok(())
 	}
@@ -1442,12 +1417,12 @@ mod tests {
 					REGULAR_PAYLOAD,
 					REGULAR_PAYLOAD.declared_weight,
 				),
-				Error::<TestRuntime, ()>::Halted,
+				Error::<TestRuntime, ()>::NotOperatingNormally,
 			);
 
 			assert_noop!(
 				Pallet::<TestRuntime>::increase_message_fee(Origin::signed(1), TEST_LANE_ID, 1, 1,),
-				Error::<TestRuntime, ()>::Halted,
+				Error::<TestRuntime, ()>::BridgeModule(bp_runtime::OwnedBridgeModuleError::Halted),
 			);
 
 			assert_noop!(
@@ -1458,7 +1433,7 @@ mod tests {
 					1,
 					REGULAR_PAYLOAD.declared_weight,
 				),
-				Error::<TestRuntime, ()>::Halted,
+				Error::<TestRuntime, ()>::BridgeModule(bp_runtime::OwnedBridgeModuleError::Halted),
 			);
 
 			assert_noop!(
@@ -1480,7 +1455,7 @@ mod tests {
 						last_delivered_nonce: 1,
 					},
 				),
-				Error::<TestRuntime, ()>::Halted,
+				Error::<TestRuntime, ()>::BridgeModule(bp_runtime::OwnedBridgeModuleError::Halted),
 			);
 		});
 	}
@@ -1500,7 +1475,7 @@ mod tests {
 					REGULAR_PAYLOAD,
 					REGULAR_PAYLOAD.declared_weight,
 				),
-				Error::<TestRuntime, ()>::Halted,
+				Error::<TestRuntime, ()>::NotOperatingNormally,
 			);
 
 			assert_ok!(Pallet::<TestRuntime>::increase_message_fee(

bridges/primitives/runtime/Cargo.toml

@@ -15,6 +15,7 @@ scale-info = { version = "2.1.1", default-features = false, features = ["derive"
 # Substrate Dependencies
 
 frame-support = { git = "https://github.com/paritytech/substrate", branch = "master", default-features = false }
+frame-system = { git = "https://github.com/paritytech/substrate", branch = "master", default-features = false }
 sp-core = { git = "https://github.com/paritytech/substrate", branch = "master", default-features = false }
 sp-io = { git = "https://github.com/paritytech/substrate", branch = "master", default-features = false }
 sp-runtime = { git = "https://github.com/paritytech/substrate", branch = "master", default-features = false }
@@ -30,6 +31,7 @@ default = ["std"]
 std = [
 	"codec/std",
 	"frame-support/std",
+	"frame-system/std",
 	"hash-db/std",
 	"num-traits/std",
 	"scale-info/std",

bridges/primitives/runtime/src/lib.rs

@@ -18,11 +18,16 @@
 
 #![cfg_attr(not(feature = "std"), no_std)]
 
-use codec::Encode;
+use codec::{Decode, Encode, FullCodec};
-use frame_support::{RuntimeDebug, StorageHasher};
+use frame_support::{
+	log, pallet_prelude::DispatchResult, PalletError, RuntimeDebug, StorageHasher, StorageValue,
+};
+use frame_system::RawOrigin;
+use scale_info::TypeInfo;
 use sp_core::{hash::H256, storage::StorageKey};
 use sp_io::hashing::blake2_256;
-use sp_std::{convert::TryFrom, vec, vec::Vec};
+use sp_runtime::traits::BadOrigin;
+use sp_std::{convert::TryFrom, fmt::Debug, vec, vec::Vec};
 
 pub use chain::{
 	AccountIdOf, AccountPublicOf, BalanceOf, BlockNumberOf, Chain, EncodedOrDecodedCall, HashOf,
@@ -257,6 +262,79 @@ pub fn storage_value_key(pallet_prefix: &str, value_name: &str) -> StorageKey {
 	StorageKey(final_key)
 }
 
+/// Error generated by the `OwnedBridgeModule` trait.
+#[derive(Encode, Decode, TypeInfo, PalletError)]
+pub enum OwnedBridgeModuleError {
+	/// All pallet operations are halted.
+	Halted,
+}
+
+/// Bridge module that has owner and operating mode
+pub trait OwnedBridgeModule<T: frame_system::Config> {
+	/// The target that will be used when publishing logs related to this module.
+	const LOG_TARGET: &'static str;
+	const OPERATING_MODE_KEY: &'static str;
+
+	type OwnerStorage: StorageValue<T::AccountId, Query = Option<T::AccountId>>;
+	type OperatingMode: Copy + Debug + FullCodec;
+	type OperatingModeStorage: StorageValue<Self::OperatingMode>;
+
+	/// Check if the module is halted.
+	fn is_halted() -> bool;
+
+	/// Ensure that the origin is either root, or `PalletOwner`.
+	fn ensure_owner_or_root(origin: T::Origin) -> Result<(), BadOrigin> {
+		match origin.into() {
+			Ok(RawOrigin::Root) => Ok(()),
+			Ok(RawOrigin::Signed(ref signer))
+				if Self::OwnerStorage::get().as_ref() == Some(signer) =>
+				Ok(()),
+			_ => Err(BadOrigin),
+		}
+	}
+
+	/// Ensure that the module is not halted.
+	fn ensure_not_halted() -> Result<(), OwnedBridgeModuleError> {
+		match Self::is_halted() {
+			true => Err(OwnedBridgeModuleError::Halted),
+			false => Ok(()),
+		}
+	}
+
+	/// Change the owner of the module.
+	fn set_owner(origin: T::Origin, maybe_owner: Option<T::AccountId>) -> DispatchResult {
+		Self::ensure_owner_or_root(origin)?;
+		match maybe_owner {
+			Some(owner) => {
+				Self::OwnerStorage::put(&owner);
+				log::info!(target: Self::LOG_TARGET, "Setting pallet Owner to: {:?}", owner);
+			},
+			None => {
+				Self::OwnerStorage::kill();
+				log::info!(target: Self::LOG_TARGET, "Removed Owner of pallet.");
+			},
+		}
+
+		Ok(())
+	}
+
+	/// Halt or resume all/some module operations.
+	fn set_operating_mode(
+		origin: T::Origin,
+		operating_mode: Self::OperatingMode,
+	) -> DispatchResult {
+		Self::ensure_owner_or_root(origin)?;
+		Self::OperatingModeStorage::put(operating_mode);
+		log::info!(
+			target: Self::LOG_TARGET,
+			"Setting operating mode ( {} = {:?}).",
+			Self::OPERATING_MODE_KEY,
+			operating_mode
+		);
+		Ok(())
+	}
+}
+
 #[cfg(test)]
 mod tests {
 	use super::*;

bridges/relays/lib-substrate-relay/src/lib.rs

@@ -97,12 +97,15 @@ impl<AccountId> TaggedAccount<AccountId> {
 	pub fn tag(&self) -> String {
 		match *self {
 			TaggedAccount::Headers { ref bridged_chain, .. } => format!("{}Headers", bridged_chain),
-			TaggedAccount::Parachains { ref bridged_chain, .. } =>
+			TaggedAccount::Parachains { ref bridged_chain, .. } => {
-				format!("{}Parachains", bridged_chain),
+				format!("{}Parachains", bridged_chain)
-			TaggedAccount::Messages { ref bridged_chain, .. } =>
+			},
-				format!("{}Messages", bridged_chain),
+			TaggedAccount::Messages { ref bridged_chain, .. } => {
-			TaggedAccount::MessagesPalletOwner { ref bridged_chain, .. } =>
+				format!("{}Messages", bridged_chain)
-				format!("{}MessagesPalletOwner", bridged_chain),
+			},
+			TaggedAccount::MessagesPalletOwner { ref bridged_chain, .. } => {
+				format!("{}MessagesPalletOwner", bridged_chain)
+			},
 		}
 	}
 }