Proposal: Defensive trait for infallible frame operations (#10626)

* add a blueprint of how defensive traits could look like * add something for arithmetic as well * add some use cases in different pallets * some build fixing * Some new stuff and examples * Fix deadly bug * add more doc. * undo faulty change to assets pallet * Update frame/support/src/traits/misc.rs Co-authored-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io> * some review comments * remove draft comment * Fix ident test * Fix proxy tests as well * a few new ideas * Fix build * Fix doc Co-authored-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>
2026-04-26 20:27:58 +00:00 · 2022-01-24 17:18:13 +01:00
parent 9daea28085
commit d1ff02d31e
11 changed files with 402 additions and 44 deletions
@@ -20,9 +20,363 @@
 use crate::dispatch::Parameter;
 use codec::{CompactLen, Decode, DecodeAll, Encode, EncodeLike, Input, MaxEncodedLen};
 use scale_info::{build::Fields, meta_type, Path, Type, TypeInfo, TypeParameter};
+use sp_arithmetic::traits::{CheckedAdd, CheckedMul, CheckedSub, Saturating};
 use sp_runtime::{traits::Block as BlockT, DispatchError};
 use sp_std::{cmp::Ordering, prelude::*};

+const DEFENSIVE_OP_PUBLIC_ERROR: &'static str = "a defensive failure has been triggered; please report the block number at https://github.com/paritytech/substrate/issues";
+const DEFENSIVE_OP_INTERNAL_ERROR: &'static str = "Defensive failure has been triggered!";
+
+/// Prelude module for all defensive traits to be imported at once.
+pub mod defensive_prelude {
+	pub use super::{Defensive, DefensiveOption, DefensiveResult};
+}
+
+/// A trait to handle errors and options when you are really sure that a condition must hold, but
+/// not brave enough to `expect` on it, or a default fallback value makes more sense.
+///
+/// This trait mostly focuses on methods that eventually unwrap the inner value. See
+/// [`DefensiveResult`] and [`DefensiveOption`] for methods that specifically apply to the
+/// respective types.
+///
+/// Each function in this trait will have two side effects, aside from behaving exactly as the name
+/// would suggest:
+///
+/// 1. It panics on `#[debug_assertions]`, so if the infallible code is reached in any of the tests,
+///    you realize.
+/// 2. It will log an error using the runtime logging system. This might help you detect such bugs
+///    in production as well. Note that the log message, as of now, are not super expressive. Your
+///    best shot of fully diagnosing the error would be to infer the block number of which the log
+///    message was emitted, then re-execute that block using `check-block` or `try-runtime`
+///    subcommands in substrate client.
+pub trait Defensive<T> {
+	/// Exactly the same as `unwrap_or`, but it does the defensive warnings explained in the trait
+	/// docs.
+	fn defensive_unwrap_or(self, other: T) -> T;
+
+	/// Exactly the same as `unwrap_or_else`, but it does the defensive warnings explained in the
+	/// trait docs.
+	fn defensive_unwrap_or_else<F: FnOnce() -> T>(self, f: F) -> T;
+
+	/// Exactly the same as `unwrap_or_default`, but it does the defensive warnings explained in the
+	/// trait docs.
+	fn defensive_unwrap_or_default(self) -> T
+	where
+		T: Default;
+
+	/// Does not alter the inner value at all, but it will log warnings if the inner value is `None`
+	/// or `Err`.
+	///
+	/// In some ways, this is like  `.defensive_map(|x| x)`.
+	///
+	/// This is useful as:
+	/// ```nocompile
+	/// if let Some(inner) = maybe_value().defensive() {
+	/// 	 	..
+	/// }
+	/// ```
+	fn defensive(self) -> Self;
+}
+
+/// Subset of methods similar to [`Defensive`] that can only work for a `Result`.
+pub trait DefensiveResult<T, E> {
+	/// Defensively map the error into another return type, but you are really sure that this
+	/// conversion should never be needed.
+	fn defensive_map_err<F, O: FnOnce(E) -> F>(self, o: O) -> Result<T, F>;
+
+	/// Defensively map and unpack the value to something else (`U`), or call the default callback
+	/// if `Err`, which should never happen.
+	fn defensive_map_or_else<U, D: FnOnce(E) -> U, F: FnOnce(T) -> U>(self, default: D, f: F) -> U;
+
+	/// Defensively transform this result into an option, discarding the `Err` variant if it
+	/// happens, which should never happen.
+	fn defensive_ok(self) -> Option<T>;
+
+	/// Exactly the same as `map`, but it prints the appropriate warnings if the value being mapped
+	/// is `Err`.
+	fn defensive_map<U, F: FnOnce(T) -> U>(self, f: F) -> Result<U, E>;
+}
+
+/// Subset of methods similar to [`Defensive`] that can only work for a `Option`.
+pub trait DefensiveOption<T> {
+	/// Potentially map and unpack the value to something else (`U`), or call the default callback
+	/// if `None`, which should never happen.
+	fn defensive_map_or_else<U, D: FnOnce() -> U, F: FnOnce(T) -> U>(self, default: D, f: F) -> U;
+
+	/// Defensively transform this option to a result.
+	fn defensive_ok_or_else<E, F: FnOnce() -> E>(self, err: F) -> Result<T, E>;
+
+	/// Exactly the same as `map`, but it prints the appropriate warnings if the value being mapped
+	/// is `None`.
+	fn defensive_map<U, F: FnOnce(T) -> U>(self, f: F) -> Option<U>;
+}
+
+impl<T> Defensive<T> for Option<T> {
+	fn defensive_unwrap_or(self, or: T) -> T {
+		match self {
+			Some(inner) => inner,
+			None => {
+				debug_assert!(false, "{}", DEFENSIVE_OP_INTERNAL_ERROR);
+				frame_support::log::error!(
+					target: "runtime",
+					"{}",
+					DEFENSIVE_OP_PUBLIC_ERROR
+				);
+				or
+			},
+		}
+	}
+
+	fn defensive_unwrap_or_else<F: FnOnce() -> T>(self, f: F) -> T {
+		match self {
+			Some(inner) => inner,
+			None => {
+				debug_assert!(false, "{}", DEFENSIVE_OP_INTERNAL_ERROR);
+				frame_support::log::error!(
+					target: "runtime",
+					"{}",
+					DEFENSIVE_OP_PUBLIC_ERROR
+				);
+				f()
+			},
+		}
+	}
+
+	fn defensive_unwrap_or_default(self) -> T
+	where
+		T: Default,
+	{
+		match self {
+			Some(inner) => inner,
+			None => {
+				debug_assert!(false, "{}", DEFENSIVE_OP_INTERNAL_ERROR);
+				frame_support::log::error!(
+					target: "runtime",
+					"{}",
+					DEFENSIVE_OP_PUBLIC_ERROR
+				);
+				Default::default()
+			},
+		}
+	}
+
+	fn defensive(self) -> Self {
+		match self {
+			Some(inner) => Some(inner),
+			None => {
+				debug_assert!(false, "{}", DEFENSIVE_OP_INTERNAL_ERROR);
+				frame_support::log::error!(
+					target: "runtime",
+					"{}",
+					DEFENSIVE_OP_PUBLIC_ERROR
+				);
+				None
+			},
+		}
+	}
+}
+
+impl<T, E: sp_std::fmt::Debug> Defensive<T> for Result<T, E> {
+	fn defensive_unwrap_or(self, or: T) -> T {
+		match self {
+			Ok(inner) => inner,
+			Err(e) => {
+				debug_assert!(false, "{}: {:?}", DEFENSIVE_OP_INTERNAL_ERROR, e);
+				frame_support::log::error!(
+					target: "runtime",
+					"{}: {:?}",
+					DEFENSIVE_OP_PUBLIC_ERROR,
+					e
+				);
+				or
+			},
+		}
+	}
+
+	fn defensive_unwrap_or_else<F: FnOnce() -> T>(self, f: F) -> T {
+		match self {
+			Ok(inner) => inner,
+			Err(e) => {
+				debug_assert!(false, "{}: {:?}", DEFENSIVE_OP_INTERNAL_ERROR, e);
+				frame_support::log::error!(
+					target: "runtime",
+					"{}: {:?}",
+					DEFENSIVE_OP_PUBLIC_ERROR,
+					e
+				);
+				f()
+			},
+		}
+	}
+
+	fn defensive_unwrap_or_default(self) -> T
+	where
+		T: Default,
+	{
+		match self {
+			Ok(inner) => inner,
+			Err(e) => {
+				debug_assert!(false, "{}: {:?}", DEFENSIVE_OP_INTERNAL_ERROR, e);
+				frame_support::log::error!(
+					target: "runtime",
+					"{}: {:?}",
+					DEFENSIVE_OP_PUBLIC_ERROR,
+					e
+				);
+				Default::default()
+			},
+		}
+	}
+
+	fn defensive(self) -> Self {
+		match self {
+			Ok(inner) => Ok(inner),
+			Err(e) => {
+				debug_assert!(false, "{}: {:?}", DEFENSIVE_OP_INTERNAL_ERROR, e);
+				frame_support::log::error!(
+					target: "runtime",
+					"{}: {:?}",
+					DEFENSIVE_OP_PUBLIC_ERROR,
+					e
+				);
+				Err(e)
+			},
+		}
+	}
+}
+
+impl<T, E: sp_std::fmt::Debug> DefensiveResult<T, E> for Result<T, E> {
+	fn defensive_map_err<F, O: FnOnce(E) -> F>(self, o: O) -> Result<T, F> {
+		self.map_err(|e| {
+			debug_assert!(false, "{}: {:?}", DEFENSIVE_OP_INTERNAL_ERROR, e);
+			frame_support::log::error!(
+				target: "runtime",
+				"{}: {:?}",
+				DEFENSIVE_OP_PUBLIC_ERROR,
+				e
+			);
+			o(e)
+		})
+	}
+
+	fn defensive_map_or_else<U, D: FnOnce(E) -> U, F: FnOnce(T) -> U>(self, default: D, f: F) -> U {
+		self.map_or_else(
+			|e| {
+				debug_assert!(false, "{}: {:?}", DEFENSIVE_OP_INTERNAL_ERROR, e);
+				frame_support::log::error!(
+					target: "runtime",
+					"{}: {:?}",
+					DEFENSIVE_OP_PUBLIC_ERROR,
+					e
+				);
+				default(e)
+			},
+			f,
+		)
+	}
+
+	fn defensive_ok(self) -> Option<T> {
+		match self {
+			Ok(inner) => Some(inner),
+			Err(e) => {
+				debug_assert!(false, "{}: {:?}", DEFENSIVE_OP_INTERNAL_ERROR, e);
+				frame_support::log::error!(
+					target: "runtime",
+					"{}: {:?}",
+					DEFENSIVE_OP_PUBLIC_ERROR,
+					e
+				);
+				None
+			},
+		}
+	}
+
+	fn defensive_map<U, F: FnOnce(T) -> U>(self, f: F) -> Result<U, E> {
+		match self {
+			Ok(inner) => Ok(f(inner)),
+			Err(e) => {
+				debug_assert!(false, "{}: {:?}", DEFENSIVE_OP_INTERNAL_ERROR, e);
+				frame_support::log::error!(
+					target: "runtime",
+					"{}: {:?}",
+					DEFENSIVE_OP_PUBLIC_ERROR,
+					e
+				);
+				Err(e)
+			},
+		}
+	}
+}
+
+impl<T> DefensiveOption<T> for Option<T> {
+	fn defensive_map_or_else<U, D: FnOnce() -> U, F: FnOnce(T) -> U>(self, default: D, f: F) -> U {
+		self.map_or_else(
+			|| {
+				debug_assert!(false, "{}", DEFENSIVE_OP_INTERNAL_ERROR);
+				frame_support::log::error!(
+					target: "runtime",
+					"{}",
+					DEFENSIVE_OP_PUBLIC_ERROR,
+				);
+				default()
+			},
+			f,
+		)
+	}
+
+	fn defensive_ok_or_else<E, F: FnOnce() -> E>(self, err: F) -> Result<T, E> {
+		self.ok_or_else(|| {
+			debug_assert!(false, "{}", DEFENSIVE_OP_INTERNAL_ERROR);
+			frame_support::log::error!(
+				target: "runtime",
+				"{}",
+				DEFENSIVE_OP_PUBLIC_ERROR,
+			);
+			err()
+		})
+	}
+
+	fn defensive_map<U, F: FnOnce(T) -> U>(self, f: F) -> Option<U> {
+		match self {
+			Some(inner) => Some(f(inner)),
+			None => {
+				debug_assert!(false, "{}", DEFENSIVE_OP_INTERNAL_ERROR);
+				frame_support::log::error!(
+					target: "runtime",
+					"{}",
+					DEFENSIVE_OP_PUBLIC_ERROR,
+				);
+				None
+			},
+		}
+	}
+}
+
+/// A variant of [`Defensive`] with the same rationale, for the arithmetic operations where in
+/// case an infallible operation fails, it saturates.
+pub trait DefensiveSaturating {
+	/// Add `self` and `other` defensively.
+	fn defensive_saturating_add(self, other: Self) -> Self;
+	/// Subtract `other` from `self` defensively.
+	fn defensive_saturating_sub(self, other: Self) -> Self;
+	/// Multiply `self` and `other` defensively.
+	fn defensive_saturating_mul(self, other: Self) -> Self;
+}
+
+// NOTE: A bit unfortunate, since T has to be bound by all the traits needed. Could make it
+// `DefensiveSaturating<T>` to mitigate.
+impl<T: Saturating + CheckedAdd + CheckedMul + CheckedSub> DefensiveSaturating for T {
+	fn defensive_saturating_add(self, other: Self) -> Self {
+		self.checked_add(&other).defensive_unwrap_or_else(|| self.saturating_add(other))
+	}
+	fn defensive_saturating_sub(self, other: Self) -> Self {
+		self.checked_sub(&other).defensive_unwrap_or_else(|| self.saturating_sub(other))
+	}
+	fn defensive_saturating_mul(self, other: Self) -> Self {
+		self.checked_mul(&other).defensive_unwrap_or_else(|| self.saturating_mul(other))
+	}
+}
+
 /// Try and collect into a collection `C`.
 pub trait TryCollect<C> {
 	type Error;
@@ -72,6 +426,7 @@ impl<T: Default> Get<T> for GetDefault {

 macro_rules! impl_const_get {
 	($name:ident, $t:ty) => {
+		#[derive($crate::RuntimeDebug)]
 		pub struct $name<const T: $t>;
 		impl<const T: $t> Get<$t> for $name<T> {
 			fn get() -> $t {