Skip to content

Deduplicate owner calls for some pallets #1477

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
serban300 merged 1 commit into from
Jun 27, 2022
Merged

Deduplicate owner calls for some pallets #1477

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions Cargo.lock
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

88 changes: 28 additions & 60 deletions modules/grandpa/src/lib.rs
View file
Open in desktop
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 modules/messages/src/lib.rs
View file
Open in desktop
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 primitives/runtime/Cargo.toml
View file
Open in desktop
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