Skip to content
This repository has been archived by the owner on Nov 15, 2023. It is now read-only.

Introduce frame::system::Error::InvalidOrigin #4422

Closed
wants to merge 3 commits into from
Closed

Introduce frame::system::Error::InvalidOrigin #4422

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
df3c323
Introduce frame::system::Error::InvalidOrigin
stanislav-tkach Dec 17, 2019
3e29a08
Fix tests
stanislav-tkach Dec 17, 2019
ca9728d
Merge branch 'master' into stas-4409-bad-origin-error
stanislav-tkach Dec 17, 2019
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: 0 additions & 1 deletion client/consensus/babe/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -487,7 +487,6 @@ impl<B, C, E, I, Error, SO> sc_consensus_slots::SimpleSlotWorker<B> for BabeWork
let slot_lenience = slot_info.number.saturating_sub(parent_slot + 1);

let slot_lenience = std::cmp::min(slot_lenience, BACKOFF_CAP);
let slot_duration = slot_info.duration << (slot_lenience / BACKOFF_STEP);

if slot_lenience >= 1 {
debug!(target: "babe", "No block for {} slots. Applying 2^({}/{}) lenience",
Expand Down
4 changes: 2 additions & 2 deletions frame/collective/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,14 +26,14 @@
use sp_std::{prelude::*, result};
use sp_core::u32_trait::Value as U32;
use sp_runtime::RuntimeDebug;
use sp_runtime::traits::{Hash, EnsureOrigin};
use sp_runtime::traits::Hash;
use frame_support::weights::SimpleDispatchInfo;
use frame_support::{
dispatch::{Dispatchable, Parameter}, codec::{Encode, Decode},
traits::{ChangeMembers, InitializeMembers}, decl_module, decl_event,
decl_storage, ensure,
};
use frame_system::{self as system, ensure_signed, ensure_root};
use frame_system::{self as system, ensure_signed, ensure_root, EnsureOrigin};

/// Simple index type for proposal counting.
pub type ProposalIndex = u32;
Expand Down
15 changes: 8 additions & 7 deletions frame/democracy/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ use sp_std::prelude::*;
use sp_std::{result, convert::TryFrom};
use sp_runtime::{
RuntimeDebug,
traits::{Zero, Bounded, CheckedMul, CheckedDiv, EnsureOrigin, Hash, Dispatchable, Saturating},
traits::{Zero, Bounded, CheckedMul, CheckedDiv, Hash, Dispatchable, Saturating},
};
use codec::{Ref, Encode, Decode, Input, Output, Error};
use frame_support::{
Expand All @@ -35,7 +35,7 @@ use frame_support::{
OnFreeBalanceZero, OnUnbalanced
}
};
use frame_system::{self as system, ensure_signed, ensure_root};
use frame_system::{self as system, ensure_signed, ensure_root, EnsureOrigin};

mod vote_threshold;
pub use vote_threshold::{Approved, VoteThreshold};
Expand Down Expand Up @@ -1093,6 +1093,7 @@ mod tests {
traits::Contains,
weights::Weight,
};
use frame_system::Error as SystemError;
use sp_core::H256;
use sp_runtime::{traits::{BlakeTwo256, IdentityLookup, Bounded}, testing::Header, Perbill};
use pallet_balances::BalanceLock;
Expand Down Expand Up @@ -1540,7 +1541,7 @@ mod tests {
);
assert!(Democracy::referendum_info(r).is_some());

assert_noop!(Democracy::emergency_cancel(Origin::signed(3), r), "Invalid origin");
assert_noop!(Democracy::emergency_cancel(Origin::signed(3), r), SystemError::InvalidOrigin.into());
assert_ok!(Democracy::emergency_cancel(Origin::signed(4), r));
assert!(Democracy::referendum_info(r).is_none());

Expand Down Expand Up @@ -1624,7 +1625,7 @@ mod tests {
assert_noop!(Democracy::external_propose(
Origin::signed(1),
set_balance_proposal_hash(2),
), "Invalid origin");
), SystemError::InvalidOrigin.into());
assert_ok!(Democracy::external_propose(
Origin::signed(2),
set_balance_proposal_hash_and_note(2),
Expand Down Expand Up @@ -1653,7 +1654,7 @@ mod tests {
assert_noop!(Democracy::external_propose_majority(
Origin::signed(1),
set_balance_proposal_hash(2)
), "Invalid origin");
), SystemError::InvalidOrigin.into());
assert_ok!(Democracy::external_propose_majority(
Origin::signed(3),
set_balance_proposal_hash_and_note(2)
Expand All @@ -1678,7 +1679,7 @@ mod tests {
assert_noop!(Democracy::external_propose_default(
Origin::signed(3),
set_balance_proposal_hash(2)
), "Invalid origin");
), SystemError::InvalidOrigin.into());
assert_ok!(Democracy::external_propose_default(
Origin::signed(1),
set_balance_proposal_hash_and_note(2)
Expand Down Expand Up @@ -1706,7 +1707,7 @@ mod tests {
Origin::signed(3),
set_balance_proposal_hash_and_note(2)
));
assert_noop!(Democracy::fast_track(Origin::signed(1), h, 3, 2), "Invalid origin");
assert_noop!(Democracy::fast_track(Origin::signed(1), h, 3, 2), SystemError::InvalidOrigin.into());
assert_ok!(Democracy::fast_track(Origin::signed(5), h, 0, 0));
assert_eq!(
Democracy::referendum_info(0),
Expand Down
4 changes: 2 additions & 2 deletions frame/identity/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -69,13 +69,13 @@ use sp_std::prelude::*;
use sp_std::{fmt::Debug, ops::Add, iter::once};
use enumflags2::BitFlags;
use codec::{Encode, Decode};
use sp_runtime::{traits::{StaticLookup, EnsureOrigin, Zero}, RuntimeDebug};
use sp_runtime::{traits::{StaticLookup, Zero}, RuntimeDebug};
use frame_support::{
decl_module, decl_event, decl_storage, ensure, dispatch::Result,
traits::{Currency, ReservableCurrency, OnUnbalanced, Get},
weights::SimpleDispatchInfo,
};
use frame_system::{self as system, ensure_signed, ensure_root};
use frame_system::{self as system, ensure_signed, ensure_root, EnsureOrigin};

type BalanceOf<T> = <<T as Trait>::Currency as Currency<<T as frame_system::Trait>::AccountId>>::Balance;
type NegativeImbalanceOf<T> = <<T as Trait>::Currency as Currency<<T as frame_system::Trait>::AccountId>>::NegativeImbalance;
Expand Down
2 changes: 1 addition & 1 deletion frame/membership/Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,13 +15,13 @@ sp-runtime = { version = "2.0.0", default-features = false, path = "../../primit

[dev-dependencies]
sp-core = { version = "2.0.0", path = "../../primitives/core" }
sp-runtime = { version = "2.0.0", path = "../../primitives/runtime" }

[features]
default = ["std"]
std = [
"serde",
"codec/std",
"sp-runtime/std",
"sp-std/std",
"sp-io/std",
"frame-support/std",
Expand Down
3 changes: 1 addition & 2 deletions frame/membership/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,8 +28,7 @@ use frame_support::{
traits::{ChangeMembers, InitializeMembers},
weights::SimpleDispatchInfo,
};
use frame_system::{self as system, ensure_root, ensure_signed};
use sp_runtime::traits::EnsureOrigin;
use frame_system::{self as system, ensure_root, ensure_signed, EnsureOrigin};

pub trait Trait<I=DefaultInstance>: frame_system::Trait {
/// The overarching event type.
Expand Down
4 changes: 2 additions & 2 deletions frame/nicks/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,14 +40,14 @@

use sp_std::prelude::*;
use sp_runtime::{
traits::{StaticLookup, EnsureOrigin, Zero}
traits::{StaticLookup, Zero}
};
use frame_support::{
decl_module, decl_event, decl_storage, ensure,
traits::{Currency, ReservableCurrency, OnUnbalanced, Get},
weights::SimpleDispatchInfo,
};
use frame_system::{self as system, ensure_signed, ensure_root};
use frame_system::{self as system, ensure_signed, ensure_root, EnsureOrigin};

type BalanceOf<T> = <<T as Trait>::Currency as Currency<<T as frame_system::Trait>::AccountId>>::Balance;
type NegativeImbalanceOf<T> = <<T as Trait>::Currency as Currency<<T as frame_system::Trait>::AccountId>>::NegativeImbalance;
Expand Down
4 changes: 2 additions & 2 deletions frame/scored-pool/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -97,9 +97,9 @@ use frame_support::{
decl_module, decl_storage, decl_event, ensure,
traits::{ChangeMembers, InitializeMembers, Currency, Get, ReservableCurrency},
};
use frame_system::{self as system, ensure_root, ensure_signed};
use frame_system::{self as system, ensure_root, ensure_signed, EnsureOrigin};
use sp_runtime::{
traits::{EnsureOrigin, SimpleArithmetic, MaybeSerializeDeserialize, Zero, StaticLookup},
traits::{SimpleArithmetic, MaybeSerializeDeserialize, Zero, StaticLookup},
};

type BalanceOf<T, I> = <<T as Trait<I>>::Currency as Currency<<T as frame_system::Trait>::AccountId>>::Balance;
Expand Down
8 changes: 3 additions & 5 deletions frame/staking/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -272,7 +272,7 @@ use sp_runtime::{
curve::PiecewiseLinear,
traits::{
Convert, Zero, One, StaticLookup, CheckedSub, Saturating, Bounded, SaturatedConversion,
SimpleArithmetic, EnsureOrigin, ModuleDispatchError,
SimpleArithmetic, ModuleDispatchError,
}
};
use sp_staking::{
Expand All @@ -281,7 +281,7 @@ use sp_staking::{
};
#[cfg(feature = "std")]
use sp_runtime::{Serialize, Deserialize};
use frame_system::{self as system, ensure_signed, ensure_root};
use frame_system::{self as system, ensure_signed, ensure_root, EnsureOrigin};

use sp_phragmen::{ExtendedBalance, PhragmenStakedAssignment};

Expand Down Expand Up @@ -794,8 +794,6 @@ decl_error! {
AlreadyBonded,
/// Controller is already paired.
AlreadyPaired,
/// Should be the root origin or the `T::SlashCancelOrigin`.
BadOrigin,
/// Targets cannot be empty.
EmptyTargets,
/// Duplicate index.
Expand Down Expand Up @@ -1191,7 +1189,7 @@ decl_module! {
T::SlashCancelOrigin::try_origin(origin)
.map(|_| ())
.or_else(ensure_root)
.map_err(|_| Error::BadOrigin)?;
.map_err(|e| e.as_str())?;

let mut slash_indices = slash_indices;
slash_indices.sort_unstable();
Expand Down
20 changes: 17 additions & 3 deletions frame/system/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -93,8 +93,7 @@ use serde::Serialize;
use sp_std::prelude::*;
#[cfg(any(feature = "std", test))]
use sp_std::map;
use sp_std::marker::PhantomData;
use sp_std::fmt::Debug;
use sp_std::{result, marker::PhantomData, fmt::Debug};
use sp_version::RuntimeVersion;
use sp_runtime::{
RuntimeDebug,
Expand All @@ -105,7 +104,7 @@ use sp_runtime::{
},
traits::{
self, CheckEqual, SimpleArithmetic, Zero, SignedExtension, Lookup, LookupError,
SimpleBitOps, Hash, Member, MaybeDisplay, EnsureOrigin, SaturatedConversion,
SimpleBitOps, Hash, Member, MaybeDisplay, SaturatedConversion,
MaybeSerialize, MaybeSerializeDeserialize, StaticLookup, One, Bounded,
},
};
Expand All @@ -126,6 +125,20 @@ use sp_core::ChangesTrieConfiguration;

pub mod offchain;

/// Some sort of check on the origin is performed by this object.
pub trait EnsureOrigin<OuterOrigin> {
/// A return type.
type Success;

/// Perform the origin check.
fn ensure_origin(o: OuterOrigin) -> result::Result<Self::Success, Error> {
Self::try_origin(o).map_err(|_| Error::InvalidOrigin)
}

/// Perform the origin check.
fn try_origin(o: OuterOrigin) -> result::Result<Self::Success, OuterOrigin>;
}

/// Handler for when a new account has been created.
#[impl_trait_for_tuples::impl_for_tuples(30)]
pub trait OnNewAccount<AccountId> {
Expand Down Expand Up @@ -323,6 +336,7 @@ decl_error! {
RequireSignedOrigin,
RequireRootOrigin,
RequireNoOrigin,
InvalidOrigin,
}
}

Expand Down
8 changes: 4 additions & 4 deletions frame/treasury/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -67,11 +67,11 @@ use frame_support::traits::{
};
use sp_runtime::{Permill, ModuleId};
use sp_runtime::traits::{
Zero, EnsureOrigin, StaticLookup, AccountIdConversion, Saturating, ModuleDispatchError,
Zero, StaticLookup, AccountIdConversion, Saturating, ModuleDispatchError,
};
use frame_support::weights::SimpleDispatchInfo;
use codec::{Encode, Decode};
use frame_system::{self as system, ensure_signed};
use frame_system::{self as system, ensure_signed, EnsureOrigin};

type BalanceOf<T> = <<T as Trait>::Currency as Currency<<T as frame_system::Trait>::AccountId>>::Balance;
type PositiveImbalanceOf<T> = <<T as Trait>::Currency as Currency<<T as frame_system::Trait>::AccountId>>::PositiveImbalance;
Expand Down Expand Up @@ -168,7 +168,7 @@ decl_module! {
/// # </weight>
#[weight = SimpleDispatchInfo::FixedOperational(100_000)]
fn reject_proposal(origin, #[compact] proposal_id: ProposalIndex) {
T::RejectOrigin::ensure_origin(origin).map_err(|e| Into::<&str>::into(e))?;
T::RejectOrigin::ensure_origin(origin).map_err(|e| e.as_str())?;
Copy link
Member

Choose a reason for hiding this comment

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

why does it need to be .as_str()? can't the error be returned back directly?

Copy link
Member

@niklasad1 niklasad1 Dec 18, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

I think ensure_origin returns system::Error which needs to be converted to treasury::Error accordingly to work, so it can't be returned directly.

let proposal = <Proposals<T>>::take(proposal_id).ok_or(Error::InvalidProposalIndex)?;

let value = proposal.bond;
Expand All @@ -186,7 +186,7 @@ decl_module! {
/// # </weight>
#[weight = SimpleDispatchInfo::FixedOperational(100_000)]
fn approve_proposal(origin, #[compact] proposal_id: ProposalIndex) {
T::ApproveOrigin::ensure_origin(origin).map_err(|e| Into::<&str>::into(e))?;
T::ApproveOrigin::ensure_origin(origin).map_err(|e| e.as_str())?;

ensure!(<Proposals<T>>::exists(proposal_id), Error::InvalidProposalIndex);

Expand Down
22 changes: 0 additions & 22 deletions primitives/runtime/src/traits.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -134,28 +134,6 @@ impl<
}
}

/// An error type that indicates that the origin is invalid.
Copy link
Member

Choose a reason for hiding this comment

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

I think the trait should stay here and we introduce the BadOrigin type directly here.

We already have some inherent errors that are declared by decl_error!. I would add a new one BadOrigin that implements From<BadOrigin> that is defined in this file here.

Copy link
Member

Choose a reason for hiding this comment

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

I have some upcoming changes in my mind for the error stuff, that would conflict in the way it is implemented currently.

#[derive(Encode, Decode)]
pub struct InvalidOrigin;

impl From<InvalidOrigin> for &'static str {
fn from(_: InvalidOrigin) -> &'static str {
"Invalid origin"
}
}

/// Some sort of check on the origin is performed by this object.
pub trait EnsureOrigin<OuterOrigin> {
/// A return type.
type Success;
/// Perform the origin check.
fn ensure_origin(o: OuterOrigin) -> result::Result<Self::Success, InvalidOrigin> {
Self::try_origin(o).map_err(|_| InvalidOrigin)
}
/// Perform the origin check.
fn try_origin(o: OuterOrigin) -> result::Result<Self::Success, OuterOrigin>;
}

/// An error that indicates that a lookup failed.
#[derive(Encode, Decode)]
pub struct LookupError;
Expand Down