Allow pallet error enum variants to contain fields (paritytech#10242)

* Allow pallet errors to contain at most one field

* Update docs on pallet::error

* Reword documentation

* cargo fmt

* Introduce CompactPalletError trait and require #[pallet::error] fields to implement them

* cargo fmt

* Do not assume tuple variants

* Add CompactPalletError derive macro

* Check for error type compactness in construct_runtime

* cargo fmt

* Derive CompactPalletError instead of implementing it directly during macro expansion

* Implement CompactPalletError on OptionBool instead of Option<bool>

* Check for type idents instead of variant ident

* Add doc comments for ErrorCompactnessTest

* Add an trait implementation of ErrorCompactnessTest for ()

* Convert the error field of DispatchError to a 4-element byte array

* Add static check for pallet error size

* Rename to MAX_PALLET_ERROR_ENCODED_SIZE

* Remove ErrorCompactnessTest trait

* Remove check_compactness

* Return only the most significant byte when constructing a custom InvalidTransaction

* Rename CompactPalletError to PalletError

* Use counter to generate unique idents for assert macros

* Make declarative pallet macros compile with pallet error size checks

* Remove unused doc comment

* Try and fix build errors

* Fix build errors

* Add macro_use for some test modules

* Test fix

* Fix compilation errors

* Remove unneeded #[macro_use]

* Resolve import ambiguity

* Make path to pallet Error enum more specific

* Fix test expectation

* Disambiguate imports

* Fix test expectations

* Revert appending pallet module name to path

* Rename bags_list::list::Error to BagError

* Fixes

* Fixes

* Fixes

* Fix test expectations

* Fix test expectation

* Add more implementations for PalletError

* Lift the 1-field requirement for nested pallet errors

* Fix UI test expectation

* Remove PalletError impl for OptionBool

* Use saturating operations

* cargo fmt

* Delete obsolete test

* Fix test expectation

* Try and use assert macro in const context

* Pull out the pallet error size check macro

* Fix UI test for const assertion

* cargo fmt

* Apply clippy suggestion

* Fix doc comment

* Docs for create_tt_return_macro

* Ensure TryInto is imported in earlier Rust editions

* Apply suggestions from code review

Co-authored-by: Bastian Köcher <[email protected]>

* Fix up comments and names

* Implement PalletError for Never

* cargo fmt

* Don't compile example code

* Bump API version for block builder

* Factor in codec attributes while derving PalletError

* Rename module and fix unit test

* Add missing attribute

* Check API version and convert ApplyExtrinsicResult accordingly

* Rename BagError to ListError

Co-authored-by: Kian Paimani <[email protected]>

* Use codec crate re-exported from frame support

* Add links to types mentioned in doc comments

Co-authored-by: Bastian Köcher <[email protected]>

* cargo fmt

* cargo fmt

* Re-add attribute for hidden docs

Co-authored-by: Bastian Köcher <[email protected]>
Co-authored-by: Kian Paimani <[email protected]>

client/block-builder/src/lib.rs

@@ -35,6 +35,7 @@ use sp_blockchain::{ApplyExtrinsicFailed, Error};
 use sp_core::ExecutionContext;
 use sp_runtime::{
 	generic::BlockId,
+	legacy,
 	traits::{Block as BlockT, Hash, HashFor, Header as HeaderT, NumberFor, One},
 	Digest,
 };
@@ -135,6 +136,7 @@ where
 pub struct BlockBuilder<'a, Block: BlockT, A: ProvideRuntimeApi<Block>, B> {
 	extrinsics: Vec<Block::Extrinsic>,
 	api: ApiRef<'a, A::Api>,
+	version: u32,
 	block_id: BlockId<Block>,
 	parent_hash: Block::Hash,
 	backend: &'a B,
@@ -183,10 +185,15 @@ where
 
 		api.initialize_block_with_context(&block_id, ExecutionContext::BlockConstruction, &header)?;
 
+		let version = api
+			.api_version::<dyn BlockBuilderApi<Block>>(&block_id)?
+			.ok_or_else(|| Error::VersionInvalid("BlockBuilderApi".to_string()))?;
+
 		Ok(Self {
 			parent_hash,
 			extrinsics: Vec::new(),
 			api,
+			version,
 			block_id,
 			backend,
 			estimated_header_size,
@@ -199,13 +206,26 @@ where
 	pub fn push(&mut self, xt: <Block as BlockT>::Extrinsic) -> Result<(), Error> {
 		let block_id = &self.block_id;
 		let extrinsics = &mut self.extrinsics;
+		let version = self.version;
 
 		self.api.execute_in_transaction(|api| {
-			match api.apply_extrinsic_with_context(
-				block_id,
-				ExecutionContext::BlockConstruction,
-				xt.clone(),
-			) {
+			let res = if version < 6 {
+				#[allow(deprecated)]
+				api.apply_extrinsic_before_version_6_with_context(
+					block_id,
+					ExecutionContext::BlockConstruction,
+					xt.clone(),
+				)
+				.map(legacy::byte_sized_error::convert_to_latest)
+			} else {
+				api.apply_extrinsic_with_context(
+					block_id,
+					ExecutionContext::BlockConstruction,
+					xt.clone(),
+				)
+			};
+
+			match res {
 				Ok(Ok(_)) => {
 					extrinsics.push(xt);
 					TransactionOutcome::Commit(Ok(()))

client/rpc/src/state/tests.rs

@@ -527,7 +527,7 @@ fn should_return_runtime_version() {
 
 	let result = "{\"specName\":\"test\",\"implName\":\"parity-test\",\"authoringVersion\":1,\
 		\"specVersion\":2,\"implVersion\":2,\"apis\":[[\"0xdf6acb689907609b\",4],\
-		[\"0x37e397fc7c91f5e4\",1],[\"0xd2bc9897eed08f15\",3],[\"0x40fe3ad401f8959a\",5],\
+		[\"0x37e397fc7c91f5e4\",1],[\"0xd2bc9897eed08f15\",3],[\"0x40fe3ad401f8959a\",6],\
 		[\"0xc6e9a76309f39b09\",1],[\"0xdd718d5cc53262d4\",1],[\"0xcbca25e39f142387\",2],\
 		[\"0xf78b278be53f454c\",2],[\"0xab3c0572291feb8b\",1],[\"0xbc9d89904f5b923f\",1]],\
 		\"transactionVersion\":1,\"stateVersion\":1}";

frame/bags-list/src/lib.rs

@@ -70,7 +70,7 @@ pub mod mock;
 mod tests;
 pub mod weights;
 
-pub use list::{notional_bag_for, Bag, Error, List, Node};
+pub use list::{notional_bag_for, Bag, List, ListError, Node};
 pub use pallet::*;
 pub use weights::WeightInfo;
 
@@ -270,7 +270,7 @@ impl<T: Config<I>, I: 'static> Pallet<T, I> {
 }
 
 impl<T: Config<I>, I: 'static> SortedListProvider<T::AccountId> for Pallet<T, I> {
-	type Error = Error;
+	type Error = ListError;
 
 	type Score = T::Score;
 
@@ -286,7 +286,7 @@ impl<T: Config<I>, I: 'static> SortedListProvider<T::AccountId> for Pallet<T, I>
 		List::<T, I>::contains(id)
 	}
 
-	fn on_insert(id: T::AccountId, score: T::Score) -> Result<(), Error> {
+	fn on_insert(id: T::AccountId, score: T::Score) -> Result<(), ListError> {
 		List::<T, I>::insert(id, score)
 	}
 

frame/bags-list/src/list/mod.rs

@@ -39,7 +39,7 @@ use sp_std::{
 };
 
 #[derive(Debug, PartialEq, Eq)]
-pub enum Error {
+pub enum ListError {
 	/// A duplicate id has been detected.
 	Duplicate,
 }
@@ -266,9 +266,9 @@ impl<T: Config<I>, I: 'static> List<T, I> {
 	/// Insert a new id into the appropriate bag in the list.
 	///
 	/// Returns an error if the list already contains `id`.
-	pub(crate) fn insert(id: T::AccountId, score: T::Score) -> Result<(), Error> {
+	pub(crate) fn insert(id: T::AccountId, score: T::Score) -> Result<(), ListError> {
 		if Self::contains(&id) {
-			return Err(Error::Duplicate)
+			return Err(ListError::Duplicate)
 		}
 
 		let bag_score = notional_bag_for::<T, I>(score);

frame/bags-list/src/list/tests.rs

@@ -248,7 +248,7 @@ mod list {
 			// then
 			assert_storage_noop!(assert_eq!(
 				List::<Runtime>::insert(3, 20).unwrap_err(),
-				Error::Duplicate
+				ListError::Duplicate
 			));
 		});
 	}

frame/bags-list/src/tests.rs

@@ -518,7 +518,7 @@ mod sorted_list_provider {
 			// then
 			assert_storage_noop!(assert_eq!(
 				BagsList::on_insert(3, 20).unwrap_err(),
-				Error::Duplicate
+				ListError::Duplicate
 			));
 		});
 	}

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

@@ -1583,7 +1583,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(ModuleError { error, .. }) => error,
+		DispatchError::Module(ModuleError { error, .. }) => error[0],
 		_ => 0,
 	};
 	InvalidTransaction::Custom(error_number)

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

@@ -931,7 +931,7 @@ mod tests {
 	#[test]
 	#[should_panic(expected = "Invalid unsigned submission must produce invalid block and \
 	                           deprive validator from their authoring reward.: \
-	                           Module(ModuleError { index: 2, error: 1, message: \
+	                           Module(ModuleError { index: 2, error: [1, 0, 0, 0], message: \
 	                           Some(\"PreDispatchWrongWinnerCount\") })")]
 	fn unfeasible_solution_panics() {
 		ExtBuilder::default().build_and_execute(|| {
@@ -1053,7 +1053,7 @@ mod tests {
 				MultiPhase::basic_checks(&solution, "mined").unwrap_err(),
 				MinerError::PreDispatchChecksFailed(DispatchError::Module(ModuleError {
 					index: 2,
-					error: 1,
+					error: [1, 0, 0, 0],
 					message: Some("PreDispatchWrongWinnerCount"),
 				})),
 			);

frame/scheduler/src/mock.rs

@@ -38,7 +38,7 @@ use sp_runtime::{
 // Logger module to track execution.
 #[frame_support::pallet]
 pub mod logger {
-	use super::*;
+	use super::{OriginCaller, OriginTrait};
 	use frame_support::pallet_prelude::*;
 	use frame_system::pallet_prelude::*;
 	use std::cell::RefCell;
@@ -71,7 +71,7 @@ pub mod logger {
 	#[pallet::call]
 	impl<T: Config> Pallet<T>
 	where
-		<T as system::Config>::Origin: OriginTrait<PalletsOrigin = OriginCaller>,
+		<T as frame_system::Config>::Origin: OriginTrait<PalletsOrigin = OriginCaller>,
 	{
 		#[pallet::weight(*weight)]
 		pub fn log(origin: OriginFor<T>, i: u32, weight: Weight) -> DispatchResult {

frame/sudo/src/mock.rs

@@ -34,7 +34,6 @@ use sp_runtime::{
 // Logger module to track execution.
 #[frame_support::pallet]
 pub mod logger {
-	use super::*;
 	use frame_support::pallet_prelude::*;
 	use frame_system::pallet_prelude::*;
 

frame/support/procedural/src/construct_runtime/mod.rs

@@ -241,6 +241,7 @@ fn construct_runtime_final_expansion(
 		expand::expand_outer_inherent(&name, &block, &unchecked_extrinsic, &pallets, &scrate);
 	let validate_unsigned = expand::expand_outer_validate_unsigned(&name, &pallets, &scrate);
 	let integrity_test = decl_integrity_test(&scrate);
+	let static_assertions = decl_static_assertions(&name, &pallets, &scrate);
 
 	let res = quote!(
 		#scrate_decl
@@ -282,6 +283,8 @@ fn construct_runtime_final_expansion(
 		#validate_unsigned
 
 		#integrity_test
+
+		#static_assertions
 	);
 
 	Ok(res)
@@ -471,3 +474,34 @@ fn decl_integrity_test(scrate: &TokenStream2) -> TokenStream2 {
 		}
 	)
 }
+
+fn decl_static_assertions(
+	runtime: &Ident,
+	pallet_decls: &[Pallet],
+	scrate: &TokenStream2,
+) -> TokenStream2 {
+	let error_encoded_size_check = pallet_decls.iter().map(|decl| {
+		let path = &decl.path;
+		let assert_message = format!(
+			"The maximum encoded size of the error type in the `{}` pallet exceeds \
+			`MAX_MODULE_ERROR_ENCODED_SIZE`",
+			decl.name,
+		);
+
+		quote! {
+			#scrate::tt_call! {
+				macro = [{ #path::tt_error_token }]
+				frame_support = [{ #scrate }]
+				~~> #scrate::assert_error_encoded_size! {
+					path = [{ #path }]
+					runtime = [{ #runtime }]
+					assert_message = [{ #assert_message }]
+				}
+			}
+		}
+	});
+
+	quote! {
+		#(#error_encoded_size_check)*
+	}
+}

frame/support/procedural/src/lib.rs

@@ -28,9 +28,11 @@ mod dummy_part_checker;
 mod key_prefix;
 mod match_and_insert;
 mod pallet;
+mod pallet_error;
 mod partial_eq_no_bound;
 mod storage;
 mod transactional;
+mod tt_macro;
 
 use proc_macro::TokenStream;
 use std::{cell::RefCell, str::FromStr};
@@ -41,9 +43,9 @@ thread_local! {
 	static COUNTER: RefCell<Counter> = RefCell::new(Counter(0));
 }
 
-/// Counter to generate a relatively unique identifier for macros querying for the existence of
-/// pallet parts. This is necessary because declarative macros gets hoisted to the crate root,
-/// which shares the namespace with other pallets containing the very same query macros.
+/// Counter to generate a relatively unique identifier for macros. This is necessary because
+/// declarative macros gets hoisted to the crate root, which shares the namespace with other pallets
+/// containing the very same macros.
 struct Counter(u64);
 
 impl Counter {
@@ -562,3 +564,14 @@ pub fn __generate_dummy_part_checker(input: TokenStream) -> TokenStream {
 pub fn match_and_insert(input: TokenStream) -> TokenStream {
 	match_and_insert::match_and_insert(input)
 }
+
+#[proc_macro_derive(PalletError, attributes(codec))]
+pub fn derive_pallet_error(input: TokenStream) -> TokenStream {
+	pallet_error::derive_pallet_error(input)
+}
+
+/// Internal macro used by `frame_support` to create tt-call-compliant macros
+#[proc_macro]
+pub fn __create_tt_macro(input: TokenStream) -> TokenStream {
+	tt_macro::create_tt_return_macro(input)
+}