Skip to content

Commit

Permalink
Avoid duplicate function definitions
Browse files Browse the repository at this point in the history 
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>
  • Loading branch information
@serban300
serban300 committed Apr 9, 2024
1 parent c8ab43c commit 7c012eb
Show file tree
Hide file tree
Showing 5 changed files with 150 additions and 124 deletions.
88 changes: 28 additions & 60 deletions bridges/modules/grandpa/src/lib.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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.
Expand All @@ -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);
Expand Down Expand Up @@ -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);
Expand All @@ -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(
origin: OriginFor<T>,
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())
pub fn set_owner(origin: OriginFor<T>, new_owner: Option<T::AccountId>) -> DispatchResult {
<Self as OwnedBridgeModule<_>>::set_owner(origin, new_owner)
}

/// 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(
origin: OriginFor<T>,
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())
pub fn set_operational(origin: OriginFor<T>, operational: bool) -> DispatchResult {
Self::set_operating_mode(origin, !operational)
}
}

Expand Down Expand Up @@ -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 = ()> {
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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> {
Expand Down Expand Up @@ -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));
Expand Down
85 changes: 30 additions & 55 deletions bridges/modules/messages/src/lib.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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::*,
Expand Down Expand Up @@ -217,25 +216,26 @@ 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`.
///
/// 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)?;
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(())
<Self as OwnedBridgeModule<_>>::set_owner(origin, new_owner)
}

/// Halt or resume all/some pallet operations.
Expand All @@ -246,14 +246,7 @@ pub mod pallet {
origin: OriginFor<T>,
operating_mode: OperatingMode,
) -> DispatchResult {
ensure_owner_or_root::<T, I>(origin)?;
PalletOperatingMode::<T, I>::put(operating_mode);
log::info!(
target: "runtime::bridge-messages",
"Setting messages pallet operating mode to {:?}.",
operating_mode,
);
Ok(())
<Self as OwnedBridgeModule<_>>::set_operating_mode(origin, operating_mode)
}

/// Update pallet parameter.
Expand All @@ -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(())
Expand Down Expand Up @@ -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)
//
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -669,8 +662,8 @@ pub mod pallet {

#[pallet::error]
pub enum Error<T, I = ()> {
/// All pallet operations are halted.
Halted,
/// Pallet is not in Normal operating mode.
NotOperatingNormally,
/// Message has been treated as invalid by chain verifier.
MessageRejectedByChainVerifier,
/// Message has been treated as invalid by lane verifier.
Expand All @@ -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.
Expand Down Expand Up @@ -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)
} 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)
Err(Error::<T, I>::NotOperatingNormally)
} else {
Ok(())
}
Expand Down Expand Up @@ -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!(
Expand All @@ -1458,7 +1433,7 @@ mod tests {
1,
REGULAR_PAYLOAD.declared_weight,
),
Error::<TestRuntime, ()>::Halted,
Error::<TestRuntime, ()>::BridgeModule(bp_runtime::OwnedBridgeModuleError::Halted),
);

assert_noop!(
Expand All @@ -1480,7 +1455,7 @@ mod tests {
last_delivered_nonce: 1,
},
),
Error::<TestRuntime, ()>::Halted,
Error::<TestRuntime, ()>::BridgeModule(bp_runtime::OwnedBridgeModuleError::Halted),
);
});
}
Expand All @@ -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(
Expand Down
2 changes: 2 additions & 0 deletions bridges/primitives/runtime/Cargo.toml
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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 }
Expand All @@ -30,6 +31,7 @@ default = ["std"]
std = [
"codec/std",
"frame-support/std",
"frame-system/std",
"hash-db/std",
"num-traits/std",
"scale-info/std",
Expand Down
Loading

0 comments on commit 7c012eb

Please sign in to comment.