Add sudo::remove_key

prdoc/pr_2165.prdoc

@@ -0,0 +1,17 @@
+title: Allow `sudo::set_key` to remove the key
+
+doc:
+  - audience: Core Dev
+    description: |
+      Pallet `Sudo` now has the ability to remove the sudo key. This is a less-invasive way of rendering the sudo pallet useless without needing a code upgrade.
+
+migrations:
+  db: []
+
+  runtime: []
+
+crates:
+  - name: pallet-sudo
+    semver: minor
+
+host_functions: []

substrate/frame/sudo/src/benchmarking.rs

@@ -22,57 +22,56 @@ 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);
+		_(RawOrigin::Signed(caller.clone()), Some(new_sudoer_lookup));
 
-		assert_last_event::<T>(Event::KeyChanged { old_sudoer: Some(caller) }.into());
+		assert_last_event::<T>(Event::KeyChanged { old: Some(caller), new: Some(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, SEED);
-		let who_lookup = T::Lookup::unlookup(who.clone());
+		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.clone()));
+		_(RawOrigin::Signed(caller), who_lookup, Box::new(call));
 
-		assert_last_event::<T>(Event::SudoAsDone { sudo_result: Ok(()) }.into())
+		assert_last_event::<T>(Event::SudoAsDone { sudo_result: Ok(()) })
 	}
 
 	impl_benchmark_test_suite!(Pallet, crate::mock::new_bench_ext(), crate::mock::Test);

substrate/frame/sudo/src/lib.rs

@@ -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 {
@@ -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();
@@ -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())
 		}
@@ -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>,
+			new: Option<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);
-			let new = T::Lookup::lookup(new)?;
+			Self::ensure_sudo(origin)?;
+
+			let new = new.map(T::Lookup::lookup).transpose()?;
+			Self::deposit_event(Event::KeyChanged { old: Key::<T>::get(), new: new.clone() });
+			Key::<T>::set(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())
 		}
@@ -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();
@@ -287,17 +268,14 @@ 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())
 		}
@@ -313,8 +291,10 @@ 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: Option<T::AccountId>,
 		},
 		/// A [sudo_as](Pallet::sudo_as) call just took place.
 		SudoAsDone {
@@ -324,9 +304,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,
 	}
 
@@ -345,8 +325,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() != Some(sender) {
+				Err(Error::<T>::RequireSudo.into())
+			} else {
+				Ok(())
 			}
 		}
 	}

substrate/frame/sudo/src/mock.rs

@@ -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")]

substrate/frame/sudo/src/tests.rs

@@ -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,
@@ -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,
@@ -140,29 +134,38 @@ fn sudo_unchecked_weight_emits_events_correctly() {
 fn set_key_basics() {
 	new_test_ext(1).execute_with(|| {
 		// A root `key` can change the root `key`
-		assert_ok!(Sudo::set_key(RuntimeOrigin::signed(1), 2));
+		assert_ok!(Sudo::set_key(RuntimeOrigin::signed(1), Some(2)));
 		assert_eq!(Sudo::key(), Some(2u64));
 	});
 
 	new_test_ext(1).execute_with(|| {
 		// A non-root `key` will trigger a `RequireSudo` error and a non-root `key` cannot change
 		// the root `key`.
-		assert_noop!(Sudo::set_key(RuntimeOrigin::signed(2), 3), Error::<Test>::RequireSudo);
+		assert_noop!(Sudo::set_key(RuntimeOrigin::signed(2), Some(3)), Error::<Test>::RequireSudo);
 	});
 }
 
 #[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) }));
+		assert_ok!(Sudo::set_key(RuntimeOrigin::signed(1), Some(2)));
+		System::assert_has_event(TestEvent::Sudo(Event::KeyChanged { old: Some(1), new: Some(2) }));
 		// Double check.
-		assert_ok!(Sudo::set_key(RuntimeOrigin::signed(2), 4));
-		System::assert_has_event(TestEvent::Sudo(Event::KeyChanged { old_sudoer: Some(2) }));
+		assert_ok!(Sudo::set_key(RuntimeOrigin::signed(2), Some(4)));
+		System::assert_has_event(TestEvent::Sudo(Event::KeyChanged { old: Some(2), new: Some(4) }));
+	});
+}
+
+#[test]
+fn set_key_remove_key() {
+	new_test_ext(1).execute_with(|| {
+		// A root `key` can change the root `key` to `None`
+		assert_ok!(Sudo::set_key(RuntimeOrigin::signed(1), None));
+		assert!(Sudo::key().is_none());
+		System::assert_has_event(TestEvent::Sudo(Event::KeyChanged { old: Some(1), new: None }));
+
+		assert_noop!(Sudo::set_key(RuntimeOrigin::signed(1), Some(1)), Error::<Test>::RequireSudo);
 	});
 }
 
@@ -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,