Skip to content
This repository was archived by the owner on Nov 15, 2023. It is now read-only.
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
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
Prev Previous commit
Next Next commit
remove IsCallable usage. Breaking: utility::batch(root, calls) no lon…
…ger bypass BasicCallFilter
  • Loading branch information
gui1117 committed Jun 10, 2020
commit 705149d623b0ed64048cc00e19300f28539cc5ea
14 changes: 1 addition & 13 deletions bin/node/runtime/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@ use frame_support::{
traits::{Currency, Imbalance, KeyOwnerProofSystem, OnUnbalanced, Randomness, LockIdentifier},
};
use frame_system::{EnsureRoot, EnsureOneOf};
use frame_support::traits::{Filter, InstanceFilter};
use frame_support::traits::InstanceFilter;
use codec::{Encode, Decode};
use sp_core::{
crypto::KeyTypeId,
Expand Down Expand Up @@ -113,15 +113,6 @@ pub fn native_version() -> NativeVersion {

type NegativeImbalance = <Balances as Currency<AccountId>>::NegativeImbalance;

pub struct BaseFilter;
impl Filter<Call> for BaseFilter {
fn filter(_call: &Call) -> bool {
true
}
}
pub struct IsCallable;
frame_support::impl_filter_stack!(IsCallable, BaseFilter, Call, is_callable);

pub struct DealWithFees;
impl OnUnbalanced<NegativeImbalance> for DealWithFees {
fn on_unbalanceds<B>(mut fees_then_tips: impl Iterator<Item=NegativeImbalance>) {
Expand Down Expand Up @@ -184,7 +175,6 @@ impl frame_system::Trait for Runtime {
impl pallet_utility::Trait for Runtime {
type Event = Event;
type Call = Call;
type IsCallable = IsCallable;
}

parameter_types! {
Expand All @@ -202,7 +192,6 @@ impl pallet_multisig::Trait for Runtime {
type DepositBase = DepositBase;
type DepositFactor = DepositFactor;
type MaxSignatories = MaxSignatories;
type IsCallable = IsCallable;
}

parameter_types! {
Expand Down Expand Up @@ -252,7 +241,6 @@ impl pallet_proxy::Trait for Runtime {
type Event = Event;
type Call = Call;
type Currency = Balances;
type IsCallable = IsCallable;
type ProxyType = ProxyType;
type ProxyDepositBase = ProxyDepositBase;
type ProxyDepositFactor = ProxyDepositFactor;
Expand Down
16 changes: 2 additions & 14 deletions frame/multisig/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,7 @@ use sp_std::prelude::*;
use codec::{Encode, Decode};
use sp_io::hashing::blake2_256;
use frame_support::{decl_module, decl_event, decl_error, decl_storage, Parameter, ensure, RuntimeDebug};
use frame_support::{traits::{Get, ReservableCurrency, Currency, Filter, FilterStack, ClearFilterGuard},
use frame_support::{traits::{Get, ReservableCurrency, Currency},
weights::{Weight, GetDispatchInfo, DispatchClass, Pays},
dispatch::{DispatchResultWithPostInfo, DispatchErrorWithPostInfo, PostDispatchInfo},
};
Expand Down Expand Up @@ -87,9 +87,6 @@ pub trait Trait: frame_system::Trait {

/// The maximum amount of signatories allowed in the multisig.
type MaxSignatories: Get<u16>;

/// Is a given call compatible with the proxying subsystem?
type IsCallable: FilterStack<<Self as Trait>::Call>;
}

/// A global extrinsic index, formed as the extrinsic index within a block, together with that
Expand Down Expand Up @@ -151,8 +148,6 @@ decl_error! {
WrongTimepoint,
/// A timepoint was given, yet no multisig operation is underway.
UnexpectedTimepoint,
/// A call with a `false` `IsCallable` filter was attempted.
Uncallable,
}
}

Expand All @@ -175,8 +170,6 @@ decl_event! {
/// A multisig operation has been cancelled. First param is the account that is
/// cancelling, third is the multisig account, fourth is hash of the call.
MultisigCancelled(AccountId, Timepoint<BlockNumber>, AccountId, CallHash),
/// A call with a `false` IsCallable filter was attempted.
Uncallable(u32),
}
}

Expand Down Expand Up @@ -220,8 +213,7 @@ decl_module! {
/// Register approval for a dispatch to be made from a deterministic composite account if
/// approved by a total of `threshold - 1` of `other_signatories`.
///
/// If there are enough, then dispatch the call. Calls must each fulfil the `IsCallable`
/// filter.
/// If there are enough, then dispatch the call.
///
/// Payment: `DepositBase` will be reserved if this is the first approval, plus
/// `threshold` times `DepositFactor`. It is returned once this dispatch happens or
Expand Down Expand Up @@ -280,10 +272,6 @@ decl_module! {
call: Box<<T as Trait>::Call>,
) -> DispatchResultWithPostInfo {
let who = ensure_signed(origin)?;
// We're now executing as a freshly authenticated new account, so the previous call
// restrictions no longer apply.
let _guard = ClearFilterGuard::<T::IsCallable, <T as Trait>::Call>::new();
ensure!(T::IsCallable::filter(call.as_ref()), Error::<T>::Uncallable);
ensure!(threshold >= 1, Error::<T>::ZeroThreshold);
let max_sigs = T::MaxSignatories::get() as usize;
ensure!(!other_signatories.is_empty(), Error::<T>::TooFewSignatories);
Expand Down
13 changes: 2 additions & 11 deletions frame/proxy/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,7 @@ use sp_runtime::{DispatchResult, traits::{Dispatchable, Zero}};
use sp_runtime::traits::Member;
use frame_support::{
decl_module, decl_event, decl_error, decl_storage, Parameter, ensure, traits::{
Get, ReservableCurrency, Currency, Filter, FilterStack, ClearFilterGuard, InstanceFilter,
Get, ReservableCurrency, Currency, InstanceFilter,
OriginTrait, IsType,
}, weights::{GetDispatchInfo, constants::{WEIGHT_PER_MICROS, WEIGHT_PER_NANOS}},
dispatch::{PostDispatchInfo, IsSubType},
Expand All @@ -66,9 +66,6 @@ pub trait Trait: frame_system::Trait {
/// The currency mechanism.
type Currency: ReservableCurrency<Self::AccountId>;

/// Is a given call compatible with the proxying subsystem?
type IsCallable: FilterStack<<Self as Trait>::Call>;

/// A kind of proxy; specified with the proxy and passed in to the `IsProxyable` fitler.
/// The instance filter determines whether a given call may be proxied under this type.
type ProxyType: Parameter + Member + Ord + PartialOrd + InstanceFilter<<Self as Trait>::Call>
Expand Down Expand Up @@ -106,8 +103,6 @@ decl_error! {
NotFound,
/// Sender is not a proxy of the account to be proxied.
NotProxy,
/// A call with a `false` `IsCallable` filter was attempted.
Uncallable,
/// A call which is incompatible with the proxy type's filter was attempted.
Unproxyable,
/// Account is already a proxy.
Expand Down Expand Up @@ -172,11 +167,7 @@ decl_module! {
.find(|x| &x.0 == &who && force_proxy_type.as_ref().map_or(true, |y| &x.1 == y))
.ok_or(Error::<T>::NotProxy)?;

// We're now executing as a freshly authenticated new account, so the previous call
// restrictions no longer apply.
let _clear_guard = ClearFilterGuard::<T::IsCallable, <T as Trait>::Call>::new();
ensure!(T::IsCallable::filter(&call), Error::<T>::Uncallable);

// This is a freshly authenticated new account, the origin restrictions doesn't apply.
let mut origin: T::Origin = frame_system::RawOrigin::Signed(real).into();
origin.add_filter(move |c: &<T as frame_system::Trait>::Call| {
let c = <T as Trait>::Call::from_ref(c);
Expand Down
42 changes: 8 additions & 34 deletions frame/utility/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -54,12 +54,12 @@ use sp_std::prelude::*;
use codec::{Encode, Decode};
use sp_core::TypeId;
use sp_io::hashing::blake2_256;
use frame_support::{decl_module, decl_event, decl_error, decl_storage, Parameter, ensure};
use frame_support::{decl_module, decl_event, decl_storage, Parameter};
use frame_support::{
traits::{Filter, FilterStack, ClearFilterGuard, OriginTrait},
traits::OriginTrait,
weights::{Weight, GetDispatchInfo, DispatchClass, FunctionOf, Pays}, dispatch::PostDispatchInfo,
};
use frame_system::{self as system, ensure_signed, ensure_root};
use frame_system::{self as system, ensure_signed};
use sp_runtime::{DispatchError, DispatchResult, traits::Dispatchable};

mod tests;
Expand All @@ -73,22 +73,12 @@ pub trait Trait: frame_system::Trait {
/// The overarching call type.
type Call: Parameter + Dispatchable<Origin=Self::Origin, PostInfo=PostDispatchInfo>
+ GetDispatchInfo + From<frame_system::Call<Self>>;

/// Is a given call compatible with the proxying subsystem?
type IsCallable: FilterStack<<Self as Trait>::Call>;
}

decl_storage! {
trait Store for Module<T: Trait> as Utility {}
}

decl_error! {
pub enum Error for Module<T: Trait> {
/// A call with a `false` `IsCallable` filter was attempted.
Uncallable,
}
}

decl_event! {
/// Events type.
pub enum Event {
Expand All @@ -97,8 +87,6 @@ decl_event! {
BatchInterrupted(u32, DispatchError),
/// Batch of dispatches completed fully with no error.
BatchCompleted,
/// A call with a `false` IsCallable filter was attempted.
Uncallable(u32),
}
}

Expand All @@ -112,16 +100,11 @@ impl TypeId for IndexedUtilityModuleId {

decl_module! {
pub struct Module<T: Trait> for enum Call where origin: T::Origin {
type Error = Error<T>;

/// Deposit one of this module's events by using the default implementation.
fn deposit_event() = default;

/// Send a batch of dispatch calls.
///
/// This will execute until the first one fails and then stop. Calls must fulfil the
/// `IsCallable` filter unless the origin is `Root`.
///
/// May be called from any origin.
///
/// - `calls`: The calls to be dispatched from the same origin.
Expand Down Expand Up @@ -156,12 +139,8 @@ decl_module! {
Pays::Yes,
)]
fn batch(origin, calls: Vec<<T as Trait>::Call>) {
let is_root = ensure_root(origin.clone()).is_ok();
for (index, call) in calls.into_iter().enumerate() {
if !is_root && !T::IsCallable::filter(&call) {
Self::deposit_event(Event::Uncallable(index as u32));
return Ok(())
}
// TODO TODO: root no longer bypass IsCallable filter.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

dispatch_bypass_filter ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ah I removed the TODO and put this into breaking change in this PR description.
dispatch_bypass_filter is not available here because this is the outer call not a pallet call.

The outer call for now only implements Dispatchable. and there is no way to bypass the filter. This was on design in order not to bypass BaseCallFilter unintentionnally.
I can fix it by introducing a new trait on the outer call that allow to bypass filter. or by introducing a way to build an origin withut BaseCallFilter.
But anyway I'm not sure of the real need actually

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see, I am not familiar with the requirements of utility::batch, but to me your decision looks better; i.e. there is no easy way to bypass the filter.

let result = call.dispatch(origin.clone());
if let Err(e) = result {
Self::deposit_event(Event::BatchInterrupted(index as u32, e.error));
Expand All @@ -173,9 +152,6 @@ decl_module! {

/// Send a call through an indexed pseudonym of the sender.
///
/// The call must fulfil only the pre-cleared `IsCallable` filter (i.e. only the level of
/// filtering that remains after calling `take()`).
///
/// NOTE: If you need to ensure that any account-based filtering is honored (i.e. because
/// you expect `proxy` to have been used prior in the call stack and you want it to apply to
/// any sub-accounts), then use `as_limited_sub` instead.
Expand All @@ -195,18 +171,17 @@ decl_module! {
)]
fn as_sub(origin, index: u16, call: Box<<T as Trait>::Call>) -> DispatchResult {
let who = ensure_signed(origin)?;
// We're now executing as a freshly authenticated new account, so the previous call
// restrictions no longer apply.
let _guard = ClearFilterGuard::<T::IsCallable, <T as Trait>::Call>::new();
ensure!(T::IsCallable::filter(&call), Error::<T>::Uncallable);

// This is a freshly authenticated new account, the origin restrictions doesn't apply.
let pseudonym = Self::sub_account_id(who, index);
call.dispatch(frame_system::RawOrigin::Signed(pseudonym).into())
.map(|_| ()).map_err(|e| e.error)
}

/// Send a call through an indexed pseudonym of the sender.
///
/// Calls must each fulfil the `IsCallable` filter; it is not cleared before.
/// Filter from origin are passed along. The call will be dispatched with an origin which
/// use the same filter as the origin of this call.
///
/// NOTE: If you need to ensure that any account-based filtering is not honored (i.e.
/// because you expect `proxy` to have been used prior in the call stack and you do not want
Expand All @@ -228,7 +203,6 @@ decl_module! {
fn as_limited_sub(origin, index: u16, call: Box<<T as Trait>::Call>) -> DispatchResult {
let mut origin = origin;
let who = ensure_signed(origin.clone())?;
ensure!(T::IsCallable::filter(&call), Error::<T>::Uncallable);
let pseudonym = Self::sub_account_id(who, index);
origin.set_caller_from(frame_system::RawOrigin::Signed(pseudonym));
call.dispatch(origin).map(|_| ()).map_err(|e| e.error)
Expand Down