Skip to content

Commit

Permalink
Add sudo::remove_key (paritytech#2165)
Browse files Browse the repository at this point in the history 
Changes:
- Adds a new call `remove_key` to the sudo pallet to permanently remove
the sudo key.
- Remove some clones and general maintenance

---------

Signed-off-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>
Co-authored-by: Bastian Köcher <git@kchr.de>
Co-authored-by: command-bot <>
  • Loading branch information
@ggwpez @bkchr
ggwpez and bkchr authored Nov 8, 2023
1 parent 2657185 commit b19fce0
Show file tree
Hide file tree
Showing 5 changed files with 155 additions and 115 deletions.
44 changes: 27 additions & 17 deletions substrate/frame/sudo/src/benchmarking.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -22,57 +22,67 @@ use crate::Pallet;
use frame_benchmarking::v2::*;
use frame_system::RawOrigin;

const SEED: u32 = 0;

fn assert_last_event<T: Config>(generic_event: <T as Config>::RuntimeEvent) {
frame_system::Pallet::<T>::assert_last_event(generic_event.into());
fn assert_last_event<T: Config>(generic_event: crate::Event<T>) {
let re: <T as Config>::RuntimeEvent = generic_event.into();
frame_system::Pallet::<T>::assert_last_event(re.into());
}

#[benchmarks( where <T as Config>::RuntimeCall: From<frame_system::Call<T>>)]
#[benchmarks(where <T as Config>::RuntimeCall: From<frame_system::Call<T>>)]
mod benchmarks {
use super::*;

#[benchmark]
fn set_key() {
let caller: T::AccountId = whitelisted_caller();
Key::<T>::put(caller.clone());
Key::<T>::put(&caller);

let new_sudoer: T::AccountId = account("sudoer", 0, SEED);
let new_sudoer: T::AccountId = account("sudoer", 0, 0);
let new_sudoer_lookup = T::Lookup::unlookup(new_sudoer.clone());

#[extrinsic_call]
_(RawOrigin::Signed(caller.clone()), new_sudoer_lookup);

assert_last_event::<T>(Event::KeyChanged { old_sudoer: Some(caller) }.into());
assert_last_event::<T>(Event::KeyChanged { old: Some(caller), new: new_sudoer });
}

#[benchmark]
fn sudo() {
let caller: T::AccountId = whitelisted_caller();
Key::<T>::put(caller.clone());
Key::<T>::put(&caller);

let call: <T as Config>::RuntimeCall = frame_system::Call::remark { remark: vec![] }.into();
let call = frame_system::Call::remark { remark: vec![] }.into();

#[extrinsic_call]
_(RawOrigin::Signed(caller.clone()), Box::new(call.clone()));
_(RawOrigin::Signed(caller), Box::new(call));

assert_last_event::<T>(Event::Sudid { sudo_result: Ok(()) }.into())
assert_last_event::<T>(Event::Sudid { sudo_result: Ok(()) })
}

#[benchmark]
fn sudo_as() {
let caller: T::AccountId = whitelisted_caller();
Key::<T>::put(caller.clone());

let call: <T as Config>::RuntimeCall = frame_system::Call::remark { remark: vec![] }.into();
let call = frame_system::Call::remark { remark: vec![] }.into();

let who: T::AccountId = account("as", 0, 0);
let who_lookup = T::Lookup::unlookup(who);

#[extrinsic_call]
_(RawOrigin::Signed(caller), who_lookup, Box::new(call));

assert_last_event::<T>(Event::SudoAsDone { sudo_result: Ok(()) })
}

let who: T::AccountId = account("as", 0, SEED);
let who_lookup = T::Lookup::unlookup(who.clone());
#[benchmark]
fn remove_key() {
let caller: T::AccountId = whitelisted_caller();
Key::<T>::put(&caller);

#[extrinsic_call]
_(RawOrigin::Signed(caller), who_lookup, Box::new(call.clone()));
_(RawOrigin::Signed(caller.clone()));

assert_last_event::<T>(Event::SudoAsDone { sudo_result: Ok(()) }.into())
assert_last_event::<T>(Event::KeyRemoved {});
}

impl_benchmark_test_suite!(Pallet, crate::mock::new_bench_ext(), crate::mock::Test);
Expand Down
92 changes: 50 additions & 42 deletions substrate/frame/sudo/src/lib.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -146,7 +146,7 @@ type AccountIdLookupOf<T> = <<T as frame_system::Config>::Lookup as StaticLookup
pub mod pallet {
use super::{DispatchResult, *};
use frame_support::pallet_prelude::*;
use frame_system::pallet_prelude::*;
use frame_system::{pallet_prelude::*, RawOrigin};

/// Default preludes for [`Config`].
pub mod config_preludes {
Expand Down Expand Up @@ -190,11 +190,6 @@ pub mod pallet {
#[pallet::call]
impl<T: Config> Pallet<T> {
/// Authenticates the sudo key and dispatches a function call with `Root` origin.
///
/// The dispatch origin for this call must be _Signed_.
///
/// ## Complexity
/// - O(1).
#[pallet::call_index(0)]
#[pallet::weight({
let dispatch_info = call.get_dispatch_info();
Expand All @@ -207,12 +202,11 @@ pub mod pallet {
origin: OriginFor<T>,
call: Box<<T as Config>::RuntimeCall>,
) -> DispatchResultWithPostInfo {
// This is a public call, so we ensure that the origin is some signed account.
let sender = ensure_signed(origin)?;
ensure!(Self::key().map_or(false, |k| sender == k), Error::<T>::RequireSudo);
Self::ensure_sudo(origin)?;

let res = call.dispatch_bypass_filter(frame_system::RawOrigin::Root.into());
let res = call.dispatch_bypass_filter(RawOrigin::Root.into());
Self::deposit_event(Event::Sudid { sudo_result: res.map(|_| ()).map_err(|e| e.error) });

// Sudo user does not pay a fee.
Ok(Pays::No.into())
}
Expand All @@ -222,47 +216,37 @@ pub mod pallet {
/// Sudo user to specify the weight of the call.
///
/// The dispatch origin for this call must be _Signed_.
///
/// ## Complexity
/// - O(1).
#[pallet::call_index(1)]
#[pallet::weight((*weight, call.get_dispatch_info().class))]
pub fn sudo_unchecked_weight(
origin: OriginFor<T>,
call: Box<<T as Config>::RuntimeCall>,
weight: Weight,
) -> DispatchResultWithPostInfo {
// This is a public call, so we ensure that the origin is some signed account.
let sender = ensure_signed(origin)?;
Self::ensure_sudo(origin)?;
let _ = weight; // We don't check the weight witness since it is a root call.
ensure!(Self::key().map_or(false, |k| sender == k), Error::<T>::RequireSudo);

let res = call.dispatch_bypass_filter(frame_system::RawOrigin::Root.into());
let res = call.dispatch_bypass_filter(RawOrigin::Root.into());
Self::deposit_event(Event::Sudid { sudo_result: res.map(|_| ()).map_err(|e| e.error) });

// Sudo user does not pay a fee.
Ok(Pays::No.into())
}

/// Authenticates the current sudo key and sets the given AccountId (`new`) as the new sudo
/// key.
///
/// The dispatch origin for this call must be _Signed_.
///
/// ## Complexity
/// - O(1).
#[pallet::call_index(2)]
#[pallet::weight(T::WeightInfo::set_key())]
pub fn set_key(
origin: OriginFor<T>,
new: AccountIdLookupOf<T>,
) -> DispatchResultWithPostInfo {
// This is a public call, so we ensure that the origin is some signed account.
let sender = ensure_signed(origin)?;
ensure!(Self::key().map_or(false, |k| sender == k), Error::<T>::RequireSudo);
Self::ensure_sudo(origin)?;

let new = T::Lookup::lookup(new)?;
Self::deposit_event(Event::KeyChanged { old: Key::<T>::get(), new: new.clone() });
Key::<T>::put(new);

Self::deposit_event(Event::KeyChanged { old_sudoer: Key::<T>::get() });
Key::<T>::put(&new);
// Sudo user does not pay a fee.
Ok(Pays::No.into())
}
Expand All @@ -271,9 +255,6 @@ pub mod pallet {
/// a given account.
///
/// The dispatch origin for this call must be _Signed_.
///
/// ## Complexity
/// - O(1).
#[pallet::call_index(3)]
#[pallet::weight({
let dispatch_info = call.get_dispatch_info();
Expand All @@ -287,17 +268,29 @@ pub mod pallet {
who: AccountIdLookupOf<T>,
call: Box<<T as Config>::RuntimeCall>,
) -> DispatchResultWithPostInfo {
// This is a public call, so we ensure that the origin is some signed account.
let sender = ensure_signed(origin)?;
ensure!(Self::key().map_or(false, |k| sender == k), Error::<T>::RequireSudo);
Self::ensure_sudo(origin)?;

let who = T::Lookup::lookup(who)?;

let res = call.dispatch_bypass_filter(frame_system::RawOrigin::Signed(who).into());

let res = call.dispatch_bypass_filter(RawOrigin::Signed(who).into());
Self::deposit_event(Event::SudoAsDone {
sudo_result: res.map(|_| ()).map_err(|e| e.error),
});

// Sudo user does not pay a fee.
Ok(Pays::No.into())
}

/// Permanently removes the sudo key.
///
/// **This cannot be un-done.**
#[pallet::call_index(4)]
#[pallet::weight(T::WeightInfo::remove_key())]
pub fn remove_key(origin: OriginFor<T>) -> DispatchResultWithPostInfo {
Self::ensure_sudo(origin)?;

Self::deposit_event(Event::KeyRemoved {});
Key::<T>::kill();

// Sudo user does not pay a fee.
Ok(Pays::No.into())
}
Expand All @@ -313,9 +306,13 @@ pub mod pallet {
},
/// The sudo key has been updated.
KeyChanged {
/// The old sudo key if one was previously set.
old_sudoer: Option<T::AccountId>,
/// The old sudo key (if one was previously set).
old: Option<T::AccountId>,
/// The new sudo key (if one was set).
new: T::AccountId,
},
/// The key was permanently removed.
KeyRemoved,
/// A [sudo_as](Pallet::sudo_as) call just took place.
SudoAsDone {
/// The result of the call made by the sudo user.
Expand All @@ -324,9 +321,9 @@ pub mod pallet {
}

#[pallet::error]
/// Error for the Sudo pallet
/// Error for the Sudo pallet.
pub enum Error<T> {
/// Sender must be the Sudo account
/// Sender must be the Sudo account.
RequireSudo,
}

Expand All @@ -345,8 +342,19 @@ pub mod pallet {
#[pallet::genesis_build]
impl<T: Config> BuildGenesisConfig for GenesisConfig<T> {
fn build(&self) {
if let Some(ref key) = self.key {
Key::<T>::put(key);
Key::<T>::set(self.key.clone());
}
}

impl<T: Config> Pallet<T> {
/// Ensure that the caller is the sudo key.
pub(crate) fn ensure_sudo(origin: OriginFor<T>) -> DispatchResult {
let sender = ensure_signed(origin)?;

if Self::key().map_or(false, |k| k == sender) {
Ok(())
} else {
Err(Error::<T>::RequireSudo.into())
}
}
}
Expand Down
4 changes: 3 additions & 1 deletion substrate/frame/sudo/src/mock.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -156,7 +156,9 @@ pub fn new_test_ext(root_key: u64) -> sp_io::TestExternalities {
sudo::GenesisConfig::<Test> { key: Some(root_key) }
.assimilate_storage(&mut t)
.unwrap();
t.into()
let mut ext: sp_io::TestExternalities = t.into();
ext.execute_with(|| System::set_block_number(1));
ext
}

#[cfg(feature = "runtime-benchmarks")]
Expand Down
28 changes: 14 additions & 14 deletions substrate/frame/sudo/src/tests.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -59,9 +59,6 @@ fn sudo_basics() {
#[test]
fn sudo_emits_events_correctly() {
new_test_ext(1).execute_with(|| {
// Set block number to 1 because events are not emitted on block 0.
System::set_block_number(1);

// Should emit event to indicate success when called with the root `key` and `call` is `Ok`.
let call = Box::new(RuntimeCall::Logger(LoggerCall::privileged_i32_log {
i: 42,
Expand Down Expand Up @@ -118,9 +115,6 @@ fn sudo_unchecked_weight_basics() {
#[test]
fn sudo_unchecked_weight_emits_events_correctly() {
new_test_ext(1).execute_with(|| {
// Set block number to 1 because events are not emitted on block 0.
System::set_block_number(1);

// Should emit event to indicate success when called with the root `key` and `call` is `Ok`.
let call = Box::new(RuntimeCall::Logger(LoggerCall::privileged_i32_log {
i: 42,
Expand Down Expand Up @@ -154,15 +148,24 @@ fn set_key_basics() {
#[test]
fn set_key_emits_events_correctly() {
new_test_ext(1).execute_with(|| {
// Set block number to 1 because events are not emitted on block 0.
System::set_block_number(1);

// A root `key` can change the root `key`.
assert_ok!(Sudo::set_key(RuntimeOrigin::signed(1), 2));
System::assert_has_event(TestEvent::Sudo(Event::KeyChanged { old_sudoer: Some(1) }));
System::assert_has_event(TestEvent::Sudo(Event::KeyChanged { old: Some(1), new: 2 }));
// Double check.
assert_ok!(Sudo::set_key(RuntimeOrigin::signed(2), 4));
System::assert_has_event(TestEvent::Sudo(Event::KeyChanged { old_sudoer: Some(2) }));
System::assert_has_event(TestEvent::Sudo(Event::KeyChanged { old: Some(2), new: 4 }));
});
}

#[test]
fn remove_key_works() {
new_test_ext(1).execute_with(|| {
assert_ok!(Sudo::remove_key(RuntimeOrigin::signed(1)));
assert!(Sudo::key().is_none());
System::assert_has_event(TestEvent::Sudo(Event::KeyRemoved {}));

assert_noop!(Sudo::remove_key(RuntimeOrigin::signed(1)), Error::<Test>::RequireSudo);
assert_noop!(Sudo::set_key(RuntimeOrigin::signed(1), 1), Error::<Test>::RequireSudo);
});
}

Expand Down Expand Up @@ -201,9 +204,6 @@ fn sudo_as_basics() {
#[test]
fn sudo_as_emits_events_correctly() {
new_test_ext(1).execute_with(|| {
// Set block number to 1 because events are not emitted on block 0.
System::set_block_number(1);

// A non-privileged function will work when passed to `sudo_as` with the root `key`.
let call = Box::new(RuntimeCall::Logger(LoggerCall::non_privileged_log {
i: 42,
Expand Down
Loading

0 comments on commit b19fce0

Please sign in to comment.