From a9b9ca5fa8bbff462284bd4a1d7e73ce46bcbb45 Mon Sep 17 00:00:00 2001
From: Peter Goodspeed-Niklaus <coriolinus@users.noreply.github.com>
Date: Fri, 20 Mar 2020 15:00:01 +0100
Subject: [PATCH] Factor out can_set_code (#5317)

* Factor out can_set_code

The motivation for this feature is parachain runtime upgrades, which
happen in two different moments: the initial call, when the checks
should be performed, and at the actual upgrade block. It's much
better and more maintainable to just call `can_set_code` in the
function than to manually copy those checks.

I'm not entirely thrilled with the interface, which requires
cloning the entire code vector in `set_code`. However, it looks
like something in `decl_module!` does not play nicely with
references in its function arguments. If `code` has a lifetime,
it _must_ be named, which gives us this signature.

```rust
pub fn can_set_code<'a>(origin, code: &'a [u8]) {
```

Unfortunately, attempting to compile with that signature generates
a very large and baffling collection of build errors, so I decided
to abandon that path for now.

* make can_set_code non-dispatchable per PR revew

Not only can we now borrow the `code` slice, but we also no longer
need to worry about people sending a `can_set_code` transaction because
of some misunderstanding.

* move into existing impl block
---
 substrate/frame/system/src/lib.rs | 41 ++++++++++++++++++++-----------
 1 file changed, 27 insertions(+), 14 deletions(-)

diff --git a/substrate/frame/system/src/lib.rs b/substrate/frame/system/src/lib.rs
index 6c30fbd755..e59788c600 100644
--- a/substrate/frame/system/src/lib.rs
+++ b/substrate/frame/system/src/lib.rs
@@ -484,20 +484,7 @@ decl_module! {
 		/// Set the new runtime code.
 		#[weight = SimpleDispatchInfo::FixedOperational(200_000)]
 		pub fn set_code(origin, code: Vec<u8>) {
-			ensure_root(origin)?;
-
-			let current_version = T::Version::get();
-			let new_version = sp_io::misc::runtime_version(&code)
-				.and_then(|v| RuntimeVersion::decode(&mut &v[..]).ok())
-				.ok_or_else(|| Error::<T>::FailedToExtractRuntimeVersion)?;
-
-			if new_version.spec_name != current_version.spec_name {
-				Err(Error::<T>::InvalidSpecName)?
-			}
-
-			if new_version.spec_version <= current_version.spec_version {
-				Err(Error::<T>::SpecVersionNeedsToIncrease)?
-			}
+			Self::can_set_code(origin, &code)?;
 
 			storage::unhashed::put_raw(well_known_keys::CODE, &code);
 			Self::deposit_event(RawEvent::CodeUpdated);
@@ -1009,6 +996,32 @@ impl<T: Trait> Module<T> {
 			Module::<T>::on_killed_account(who.clone());
 		}
 	}
+
+	/// Determine whether or not it is possible to update the code.
+	///
+	/// This function has no side effects and is idempotent, but is fairly
+	/// heavy. It is automatically called by `set_code`; in most cases,
+	/// a direct call to `set_code` is preferable. It is useful to call
+	/// `can_set_code` when it is desirable to perform the appropriate
+	/// runtime checks without actually changing the code yet.
+	pub fn can_set_code(origin: T::Origin, code: &[u8]) -> Result<(), sp_runtime::DispatchError> {
+		ensure_root(origin)?;
+
+		let current_version = T::Version::get();
+		let new_version = sp_io::misc::runtime_version(&code)
+			.and_then(|v| RuntimeVersion::decode(&mut &v[..]).ok())
+			.ok_or_else(|| Error::<T>::FailedToExtractRuntimeVersion)?;
+
+		if new_version.spec_name != current_version.spec_name {
+			Err(Error::<T>::InvalidSpecName)?
+		}
+
+		if new_version.spec_version <= current_version.spec_version {
+			Err(Error::<T>::SpecVersionNeedsToIncrease)?
+		}
+
+		Ok(())
+	}
 }
 
 /// Event handler which calls on_created_account when it happens.