Introduce ModuleError struct (#10776)

* better partial eq impl

* introduce module error

* fmt

* import module error

* Apply suggestions from code review

Co-authored-by: Bastian Köcher <bkchr@users.noreply.github.com>

* fixes

* fmt

Co-authored-by: Bastian Köcher <bkchr@users.noreply.github.com>

substrate/frame/election-provider-multi-phase/src/lib.rs

@@ -255,7 +255,7 @@ use sp_runtime::{
 		InvalidTransaction, TransactionPriority, TransactionSource, TransactionValidity,
 		TransactionValidityError, ValidTransaction,
 	},
-	DispatchError, PerThing, Perbill, RuntimeDebug, SaturatedConversion,
+	DispatchError, ModuleError, PerThing, Perbill, RuntimeDebug, SaturatedConversion,
 };
 use sp_std::prelude::*;
 
@@ -1579,7 +1579,7 @@ impl<T: Config> ElectionProvider for Pallet<T> {
 /// number.
 pub fn dispatch_error_to_invalid(error: DispatchError) -> InvalidTransaction {
 	let error_number = match error {
-		DispatchError::Module { error, .. } => error,
+		DispatchError::Module(ModuleError { error, .. }) => error,
 		_ => 0,
 	};
 	InvalidTransaction::Custom(error_number)

substrate/frame/election-provider-multi-phase/src/unsigned.rs

@@ -752,7 +752,7 @@ mod tests {
 	use sp_runtime::{
 		offchain::storage_lock::{BlockAndTime, StorageLock},
 		traits::ValidateUnsigned,
-		PerU16,
+		ModuleError, PerU16,
 	};
 
 	type Assignment = crate::unsigned::Assignment<Runtime>;
@@ -922,8 +922,8 @@ mod tests {
 	#[test]
 	#[should_panic(expected = "Invalid unsigned submission must produce invalid block and \
 	                           deprive validator from their authoring reward.: \
-	                           Module { index: 2, error: 1, message: \
+	                           Module(ModuleError { index: 2, error: 1, message: \
-	                           Some(\"PreDispatchWrongWinnerCount\") }")]
+	                           Some(\"PreDispatchWrongWinnerCount\") })")]
 	fn unfeasible_solution_panics() {
 		ExtBuilder::default().build_and_execute(|| {
 			roll_to(25);
@@ -1033,11 +1033,11 @@ mod tests {
 
 			assert_eq!(
 				MultiPhase::mine_check_save_submit().unwrap_err(),
-				MinerError::PreDispatchChecksFailed(DispatchError::Module {
+				MinerError::PreDispatchChecksFailed(DispatchError::Module(ModuleError {
 					index: 2,
 					error: 1,
 					message: Some("PreDispatchWrongWinnerCount"),
-				}),
+				})),
 			);
 		})
 	}

substrate/frame/elections-phragmen/src/lib.rs

@@ -1132,7 +1132,7 @@ mod tests {
 	use sp_runtime::{
 		testing::Header,
 		traits::{BlakeTwo256, IdentityLookup},
-		BuildStorage,
+		BuildStorage, ModuleError,
 	};
 	use substrate_test_utils::assert_eq_uvec;
 
@@ -2514,7 +2514,7 @@ mod tests {
 			let unwrapped_error = Elections::remove_member(Origin::root(), 4, true).unwrap_err();
 			assert!(matches!(
 				unwrapped_error.error,
-				DispatchError::Module { message: Some("InvalidReplacement"), .. }
+				DispatchError::Module(ModuleError { message: Some("InvalidReplacement"), .. })
 			));
 			assert!(unwrapped_error.post_info.actual_weight.is_some());
 		});
@@ -2537,7 +2537,7 @@ mod tests {
 			let unwrapped_error = Elections::remove_member(Origin::root(), 4, false).unwrap_err();
 			assert!(matches!(
 				unwrapped_error.error,
-				DispatchError::Module { message: Some("InvalidReplacement"), .. }
+				DispatchError::Module(ModuleError { message: Some("InvalidReplacement"), .. })
 			));
 			assert!(unwrapped_error.post_info.actual_weight.is_some());
 		});

substrate/frame/support/procedural/src/pallet/expand/error.rs

@@ -124,11 +124,11 @@ pub fn expand_error(def: &mut Def) -> proc_macro2::TokenStream {
 				>::index::<Pallet<#type_use_gen>>()
 					.expect("Every active module has an index in the runtime; qed") as u8;
 
-				#frame_support::sp_runtime::DispatchError::Module {
+				#frame_support::sp_runtime::DispatchError::Module(#frame_support::sp_runtime::ModuleError {
 					index,
 					error: err.as_u8(),
 					message: Some(err.as_str()),
-				}
+				})
 			}
 		}
 	)

substrate/frame/support/src/error.rs

@@ -153,11 +153,11 @@ macro_rules! decl_error {
 					::index::<$module<$generic $(, $inst_generic)?>>()
 					.expect("Every active module has an index in the runtime; qed") as u8;
 
-				$crate::sp_runtime::DispatchError::Module {
+				$crate::sp_runtime::DispatchError::Module($crate::sp_runtime::ModuleError {
 					index,
 					error: err.as_u8(),
 					message: Some(err.as_str()),
-				}
+				})
 			}
 		}
 	};

substrate/frame/support/test/tests/construct_runtime.rs

@@ -27,7 +27,7 @@ use sp_core::{sr25519, H256};
 use sp_runtime::{
 	generic,
 	traits::{BlakeTwo256, Verify},
-	DispatchError,
+	DispatchError, ModuleError,
 };
 use sp_std::cell::RefCell;
 
@@ -363,47 +363,47 @@ mod origin_test {
 fn check_modules_error_type() {
 	assert_eq!(
 		Module1_1::fail(system::Origin::<Runtime>::Root.into()),
-		Err(DispatchError::Module { index: 31, error: 0, message: Some("Something") }),
+		Err(DispatchError::Module(ModuleError { index: 31, error: 0, message: Some("Something") })),
 	);
 	assert_eq!(
 		Module2::fail(system::Origin::<Runtime>::Root.into()),
-		Err(DispatchError::Module { index: 32, error: 0, message: Some("Something") }),
+		Err(DispatchError::Module(ModuleError { index: 32, error: 0, message: Some("Something") })),
 	);
 	assert_eq!(
 		Module1_2::fail(system::Origin::<Runtime>::Root.into()),
-		Err(DispatchError::Module { index: 33, error: 0, message: Some("Something") }),
+		Err(DispatchError::Module(ModuleError { index: 33, error: 0, message: Some("Something") })),
 	);
 	assert_eq!(
 		NestedModule3::fail(system::Origin::<Runtime>::Root.into()),
-		Err(DispatchError::Module { index: 34, error: 0, message: Some("Something") }),
+		Err(DispatchError::Module(ModuleError { index: 34, error: 0, message: Some("Something") })),
 	);
 	assert_eq!(
 		Module1_3::fail(system::Origin::<Runtime>::Root.into()),
-		Err(DispatchError::Module { index: 6, error: 0, message: Some("Something") }),
+		Err(DispatchError::Module(ModuleError { index: 6, error: 0, message: Some("Something") })),
 	);
 	assert_eq!(
 		Module1_4::fail(system::Origin::<Runtime>::Root.into()),
-		Err(DispatchError::Module { index: 3, error: 0, message: Some("Something") }),
+		Err(DispatchError::Module(ModuleError { index: 3, error: 0, message: Some("Something") })),
 	);
 	assert_eq!(
 		Module1_5::fail(system::Origin::<Runtime>::Root.into()),
-		Err(DispatchError::Module { index: 4, error: 0, message: Some("Something") }),
+		Err(DispatchError::Module(ModuleError { index: 4, error: 0, message: Some("Something") })),
 	);
 	assert_eq!(
 		Module1_6::fail(system::Origin::<Runtime>::Root.into()),
-		Err(DispatchError::Module { index: 1, error: 0, message: Some("Something") }),
+		Err(DispatchError::Module(ModuleError { index: 1, error: 0, message: Some("Something") })),
 	);
 	assert_eq!(
 		Module1_7::fail(system::Origin::<Runtime>::Root.into()),
-		Err(DispatchError::Module { index: 2, error: 0, message: Some("Something") }),
+		Err(DispatchError::Module(ModuleError { index: 2, error: 0, message: Some("Something") })),
 	);
 	assert_eq!(
 		Module1_8::fail(system::Origin::<Runtime>::Root.into()),
-		Err(DispatchError::Module { index: 12, error: 0, message: Some("Something") }),
+		Err(DispatchError::Module(ModuleError { index: 12, error: 0, message: Some("Something") })),
 	);
 	assert_eq!(
 		Module1_9::fail(system::Origin::<Runtime>::Root.into()),
-		Err(DispatchError::Module { index: 13, error: 0, message: Some("Something") }),
+		Err(DispatchError::Module(ModuleError { index: 13, error: 0, message: Some("Something") })),
 	);
 }
 

substrate/frame/support/test/tests/pallet.rs

@@ -29,7 +29,7 @@ use sp_io::{
 	hashing::{blake2_128, twox_128, twox_64},
 	TestExternalities,
 };
-use sp_runtime::DispatchError;
+use sp_runtime::{DispatchError, ModuleError};
 
 pub struct SomeType1;
 impl From<SomeType1> for u64 {
@@ -654,7 +654,11 @@ fn error_expand() {
 	);
 	assert_eq!(
 		DispatchError::from(pallet::Error::<Runtime>::InsufficientProposersBalance),
-		DispatchError::Module { index: 1, error: 0, message: Some("InsufficientProposersBalance") },
+		DispatchError::Module(ModuleError {
+			index: 1,
+			error: 0,
+			message: Some("InsufficientProposersBalance")
+		}),
 	);
 }
 

substrate/frame/support/test/tests/pallet_instance.rs

@@ -25,7 +25,7 @@ use sp_io::{
 	hashing::{blake2_128, twox_128, twox_64},
 	TestExternalities,
 };
-use sp_runtime::DispatchError;
+use sp_runtime::{DispatchError, ModuleError};
 
 #[frame_support::pallet]
 pub mod pallet {
@@ -341,7 +341,11 @@ fn error_expand() {
 	);
 	assert_eq!(
 		DispatchError::from(pallet::Error::<Runtime>::InsufficientProposersBalance),
-		DispatchError::Module { index: 1, error: 0, message: Some("InsufficientProposersBalance") },
+		DispatchError::Module(ModuleError {
+			index: 1,
+			error: 0,
+			message: Some("InsufficientProposersBalance")
+		}),
 	);
 
 	assert_eq!(
@@ -358,7 +362,11 @@ fn error_expand() {
 		DispatchError::from(
 			pallet::Error::<Runtime, pallet::Instance1>::InsufficientProposersBalance
 		),
-		DispatchError::Module { index: 2, error: 0, message: Some("InsufficientProposersBalance") },
+		DispatchError::Module(ModuleError {
+			index: 2,
+			error: 0,
+			message: Some("InsufficientProposersBalance")
+		}),
 	);
 }
 

substrate/primitives/runtime/src/lib.rs

@@ -461,9 +461,29 @@ pub type DispatchResult = sp_std::result::Result<(), DispatchError>;
 /// about the `Dispatchable` that is only known post dispatch.
 pub type DispatchResultWithInfo<T> = sp_std::result::Result<T, DispatchErrorWithPostInfo<T>>;
 
-/// Reason why a dispatch call failed.
+/// Reason why a pallet call failed.
 #[derive(Eq, Clone, Copy, Encode, Decode, Debug, TypeInfo)]
 #[cfg_attr(feature = "std", derive(Serialize, Deserialize))]
+pub struct ModuleError {
+	/// Module index, matching the metadata module index.
+	pub index: u8,
+	/// Module specific error value.
+	pub error: u8,
+	/// Optional error message.
+	#[codec(skip)]
+	#[cfg_attr(feature = "std", serde(skip_deserializing))]
+	pub message: Option<&'static str>,
+}
+
+impl PartialEq for ModuleError {
+	fn eq(&self, other: &Self) -> bool {
+		(self.index == other.index) && (self.error == other.error)
+	}
+}
+
+/// Reason why a dispatch call failed.
+#[derive(Eq, Clone, Copy, Encode, Decode, Debug, TypeInfo, PartialEq)]
+#[cfg_attr(feature = "std", derive(Serialize, Deserialize))]
 pub enum DispatchError {
 	/// Some error occurred.
 	Other(
@@ -476,16 +496,7 @@ pub enum DispatchError {
 	/// A bad origin.
 	BadOrigin,
 	/// A custom error in a module.
-	Module {
+	Module(ModuleError),
-		/// Module index, matching the metadata module index.
-		index: u8,
-		/// Module specific error value.
-		error: u8,
-		/// Optional error message.
-		#[codec(skip)]
-		#[cfg_attr(feature = "std", serde(skip_deserializing))]
-		message: Option<&'static str>,
-	},
 	/// At least one consumer is remaining so the account cannot be destroyed.
 	ConsumerRemaining,
 	/// There are no providers so the account cannot be created.
@@ -515,8 +526,8 @@ impl DispatchError {
 	/// Return the same error but without the attached message.
 	pub fn stripped(self) -> Self {
 		match self {
-			DispatchError::Module { index, error, message: Some(_) } =>
+			DispatchError::Module(ModuleError { index, error, message: Some(_) }) =>
-				DispatchError::Module { index, error, message: None },
+				DispatchError::Module(ModuleError { index, error, message: None }),
 			m => m,
 		}
 	}
@@ -624,7 +635,8 @@ impl From<DispatchError> for &'static str {
 			DispatchError::Other(msg) => msg,
 			DispatchError::CannotLookup => "Cannot lookup",
 			DispatchError::BadOrigin => "Bad origin",
-			DispatchError::Module { message, .. } => message.unwrap_or("Unknown module error"),
+			DispatchError::Module(ModuleError { message, .. }) =>
+				message.unwrap_or("Unknown module error"),
 			DispatchError::ConsumerRemaining => "Consumer remaining",
 			DispatchError::NoProviders => "No providers",
 			DispatchError::TooManyConsumers => "Too many consumers",
@@ -650,7 +662,7 @@ impl traits::Printable for DispatchError {
 			Self::Other(err) => err.print(),
 			Self::CannotLookup => "Cannot lookup".print(),
 			Self::BadOrigin => "Bad origin".print(),
-			Self::Module { index, error, message } => {
+			Self::Module(ModuleError { index, error, message }) => {
 				index.print();
 				error.print();
 				if let Some(msg) = message {
@@ -683,30 +695,6 @@ where
 	}
 }
 
-impl PartialEq for DispatchError {
-	fn eq(&self, other: &Self) -> bool {
-		use DispatchError::*;
-
-		match (self, other) {
-			(CannotLookup, CannotLookup) |
-			(BadOrigin, BadOrigin) |
-			(ConsumerRemaining, ConsumerRemaining) |
-			(NoProviders, NoProviders) => true,
-
-			(Token(l), Token(r)) => l == r,
-			(Other(l), Other(r)) => l == r,
-			(Arithmetic(l), Arithmetic(r)) => l == r,
-
-			(
-				Module { index: index_l, error: error_l, .. },
-				Module { index: index_r, error: error_r, .. },
-			) => (index_l == index_r) && (error_l == error_r),
-
-			_ => false,
-		}
-	}
-}
-
 /// This type specifies the outcome of dispatching a call to a module.
 ///
 /// In case of failure an error specific to the module is returned.
@@ -932,11 +920,18 @@ mod tests {
 
 	#[test]
 	fn dispatch_error_encoding() {
-		let error = DispatchError::Module { index: 1, error: 2, message: Some("error message") };
+		let error = DispatchError::Module(ModuleError {
+			index: 1,
+			error: 2,
+			message: Some("error message"),
+		});
 		let encoded = error.encode();
 		let decoded = DispatchError::decode(&mut &encoded[..]).unwrap();
 		assert_eq!(encoded, vec![3, 1, 2]);
-		assert_eq!(decoded, DispatchError::Module { index: 1, error: 2, message: None });
+		assert_eq!(
+			decoded,
+			DispatchError::Module(ModuleError { index: 1, error: 2, message: None })
+		);
 	}
 
 	#[test]
@@ -948,9 +943,9 @@ mod tests {
 			Other("bar"),
 			CannotLookup,
 			BadOrigin,
-			Module { index: 1, error: 1, message: None },
+			Module(ModuleError { index: 1, error: 1, message: None }),
-			Module { index: 1, error: 2, message: None },
+			Module(ModuleError { index: 1, error: 2, message: None }),
-			Module { index: 2, error: 1, message: None },
+			Module(ModuleError { index: 2, error: 1, message: None }),
 			ConsumerRemaining,
 			NoProviders,
 			Token(TokenError::NoFunds),
@@ -975,8 +970,8 @@ mod tests {
 
 		// Ignores `message` field in `Module` variant.
 		assert_eq!(
-			Module { index: 1, error: 1, message: Some("foo") },
+			Module(ModuleError { index: 1, error: 1, message: Some("foo") }),
-			Module { index: 1, error: 1, message: None },
+			Module(ModuleError { index: 1, error: 1, message: None }),
 		);
 	}