From 1723c4307856819e12bddafacf5312f0dda0318e Mon Sep 17 00:00:00 2001 From: Keith Yeung Date: Mon, 23 May 2022 17:47:36 +0100 Subject: [PATCH] Introduce #[pallet::call_index] attribute to dispatchables (#11381) * Introduce #[pallet::call_index] attribute to dispatchables * cargo fmt * Add more docs and prevent duplicates of call indices * Add UI test for conflicting call indices * cargo fmt Co-authored-by: parity-processbot <> --- .../procedural/src/pallet/expand/call.rs | 2 + .../procedural/src/pallet/parse/call.rs | 92 ++++++++++++++++--- frame/support/src/lib.rs | 19 +++- .../pallet_ui/call_conflicting_indices.rs | 24 +++++ .../pallet_ui/call_conflicting_indices.stderr | 11 +++ .../test/tests/pallet_ui/call_invalid_attr.rs | 20 ++++ .../tests/pallet_ui/call_invalid_attr.stderr | 5 + .../tests/pallet_ui/call_invalid_index.rs | 21 +++++ .../tests/pallet_ui/call_invalid_index.stderr | 5 + .../pallet_ui/call_multiple_call_index.rs | 22 +++++ .../pallet_ui/call_multiple_call_index.stderr | 5 + 11 files changed, 208 insertions(+), 18 deletions(-) create mode 100644 frame/support/test/tests/pallet_ui/call_conflicting_indices.rs create mode 100644 frame/support/test/tests/pallet_ui/call_conflicting_indices.stderr create mode 100644 frame/support/test/tests/pallet_ui/call_invalid_attr.rs create mode 100644 frame/support/test/tests/pallet_ui/call_invalid_attr.stderr create mode 100644 frame/support/test/tests/pallet_ui/call_invalid_index.rs create mode 100644 frame/support/test/tests/pallet_ui/call_invalid_index.stderr create mode 100644 frame/support/test/tests/pallet_ui/call_multiple_call_index.rs create mode 100644 frame/support/test/tests/pallet_ui/call_multiple_call_index.stderr diff --git a/frame/support/procedural/src/pallet/expand/call.rs b/frame/support/procedural/src/pallet/expand/call.rs index 60546e1a96779..ac52463b8aab6 100644 --- a/frame/support/procedural/src/pallet/expand/call.rs +++ b/frame/support/procedural/src/pallet/expand/call.rs @@ -42,6 +42,7 @@ pub fn expand_call(def: &mut Def) -> proc_macro2::TokenStream { let pallet_ident = &def.pallet_struct.pallet; let fn_name = methods.iter().map(|method| &method.name).collect::>(); + let call_index = methods.iter().map(|method| method.call_index).collect::>(); let new_call_variant_fn_name = fn_name .iter() .map(|fn_name| quote::format_ident!("new_call_variant_{}", fn_name)) @@ -177,6 +178,7 @@ pub fn expand_call(def: &mut Def) -> proc_macro2::TokenStream { ), #( #( #[doc = #fn_doc] )* + #[codec(index = #call_index)] #fn_name { #( #[allow(missing_docs)] diff --git a/frame/support/procedural/src/pallet/parse/call.rs b/frame/support/procedural/src/pallet/parse/call.rs index f532d339f1205..75c85474dcfe7 100644 --- a/frame/support/procedural/src/pallet/parse/call.rs +++ b/frame/support/procedural/src/pallet/parse/call.rs @@ -18,6 +18,7 @@ use super::helper; use frame_support_procedural_tools::get_doc_literals; use quote::ToTokens; +use std::collections::HashMap; use syn::spanned::Spanned; /// List of additional token to be used for parsing. @@ -25,6 +26,7 @@ mod keyword { syn::custom_keyword!(Call); syn::custom_keyword!(OriginFor); syn::custom_keyword!(weight); + syn::custom_keyword!(call_index); syn::custom_keyword!(compact); syn::custom_keyword!(T); syn::custom_keyword!(pallet); @@ -55,14 +57,17 @@ pub struct CallVariantDef { pub args: Vec<(bool, syn::Ident, Box)>, /// Weight formula. pub weight: syn::Expr, + /// Call index of the dispatchable. + pub call_index: u8, /// Docs, used for metadata. pub docs: Vec, } /// Attributes for functions in call impl block. -/// Parse for `#[pallet::weight(expr)]` -pub struct FunctionAttr { - weight: syn::Expr, +/// Parse for `#[pallet::weight(expr)]` or `#[pallet::call_index(expr)] +pub enum FunctionAttr { + CallIndex(u8), + Weight(syn::Expr), } impl syn::parse::Parse for FunctionAttr { @@ -72,11 +77,22 @@ impl syn::parse::Parse for FunctionAttr { syn::bracketed!(content in input); content.parse::()?; content.parse::()?; - content.parse::()?; - let weight_content; - syn::parenthesized!(weight_content in content); - Ok(FunctionAttr { weight: weight_content.parse::()? }) + let lookahead = content.lookahead1(); + if lookahead.peek(keyword::weight) { + content.parse::()?; + let weight_content; + syn::parenthesized!(weight_content in content); + Ok(FunctionAttr::Weight(weight_content.parse::()?)) + } else if lookahead.peek(keyword::call_index) { + content.parse::()?; + let call_index_content; + syn::parenthesized!(call_index_content in content); + let index = call_index_content.parse::()?; + Ok(FunctionAttr::CallIndex(index.base10_parse()?)) + } else { + Err(lookahead.error()) + } } } @@ -145,6 +161,8 @@ impl CallDef { } let mut methods = vec![]; + let mut indices = HashMap::new(); + let mut last_index: Option = None; for impl_item in &mut item.items { if let syn::ImplItem::Method(method) = impl_item { if !matches!(method.vis, syn::Visibility::Public(_)) { @@ -182,18 +200,58 @@ impl CallDef { return Err(syn::Error::new(method.sig.span(), msg)) } - let mut call_var_attrs: Vec = - helper::take_item_pallet_attrs(&mut method.attrs)?; - - if call_var_attrs.len() != 1 { - let msg = if call_var_attrs.is_empty() { + let (mut weight_attrs, mut call_idx_attrs): (Vec, Vec) = + helper::take_item_pallet_attrs(&mut method.attrs)?.into_iter().partition( + |attr| { + if let FunctionAttr::Weight(_) = attr { + true + } else { + false + } + }, + ); + + if weight_attrs.len() != 1 { + let msg = if weight_attrs.is_empty() { "Invalid pallet::call, requires weight attribute i.e. `#[pallet::weight($expr)]`" } else { "Invalid pallet::call, too many weight attributes given" }; return Err(syn::Error::new(method.sig.span(), msg)) } - let weight = call_var_attrs.pop().unwrap().weight; + let weight = match weight_attrs.pop().unwrap() { + FunctionAttr::Weight(w) => w, + _ => unreachable!("checked during creation of the let binding"), + }; + + if call_idx_attrs.len() > 1 { + let msg = "Invalid pallet::call, too many call_index attributes given"; + return Err(syn::Error::new(method.sig.span(), msg)) + } + let call_index = call_idx_attrs.pop().map(|attr| match attr { + FunctionAttr::CallIndex(idx) => idx, + _ => unreachable!("checked during creation of the let binding"), + }); + + let final_index = match call_index { + Some(i) => i, + None => + last_index.map_or(Some(0), |idx| idx.checked_add(1)).ok_or_else(|| { + let msg = "Call index doesn't fit into u8, index is 256"; + syn::Error::new(method.sig.span(), msg) + })?, + }; + last_index = Some(final_index); + + if let Some(used_fn) = indices.insert(final_index, method.sig.ident.clone()) { + let msg = format!( + "Call indices are conflicting: Both functions {} and {} are at index {}", + used_fn, method.sig.ident, final_index, + ); + let mut err = syn::Error::new(used_fn.span(), &msg); + err.combine(syn::Error::new(method.sig.ident.span(), msg)); + return Err(err) + } let mut args = vec![]; for arg in method.sig.inputs.iter_mut().skip(1) { @@ -223,7 +281,13 @@ impl CallDef { let docs = get_doc_literals(&method.attrs); - methods.push(CallVariantDef { name: method.sig.ident.clone(), weight, args, docs }); + methods.push(CallVariantDef { + name: method.sig.ident.clone(), + weight, + call_index: final_index, + args, + docs, + }); } else { let msg = "Invalid pallet::call, only method accepted"; return Err(syn::Error::new(impl_item.span(), msg)) diff --git a/frame/support/src/lib.rs b/frame/support/src/lib.rs index d73a01187bfcc..03f1553293a47 100644 --- a/frame/support/src/lib.rs +++ b/frame/support/src/lib.rs @@ -1553,6 +1553,15 @@ pub mod pallet_prelude { /// used using `#[pallet::compact]`, function must return `DispatchResultWithPostInfo` or /// `DispatchResult`. /// +/// Each dispatchable may also be annotated with the `#[pallet::call_index($idx)]` attribute, +/// which defines and sets the codec index for the dispatchable function in the `Call` enum. +/// +/// All call indexes start from 0, until it encounters a dispatchable function with a defined +/// call index. The dispatchable function that lexically follows the function with a defined +/// call index will have that call index, but incremented by 1, e.g. if there are 3 +/// dispatchable functions `fn foo`, `fn bar` and `fn qux` in that order, and only `fn bar` has +/// a call index of 10, then `fn qux` will have an index of 11, instead of 1. +/// /// All arguments must implement `Debug`, `PartialEq`, `Eq`, `Decode`, `Encode`, `Clone`. For /// ease of use, bound the trait `Member` available in frame_support::pallet_prelude. /// @@ -1566,16 +1575,18 @@ pub mod pallet_prelude { /// **WARNING**: modifying dispatchables, changing their order, removing some must be done with /// care. Indeed this will change the outer runtime call type (which is an enum with one /// variant per pallet), this outer runtime call can be stored on-chain (e.g. in -/// pallet-scheduler). Thus migration might be needed. +/// pallet-scheduler). Thus migration might be needed. To mitigate against some of this, the +/// `#[pallet::call_index($idx)]` attribute can be used to fix the order of the dispatchable so +/// that the `Call` enum encoding does not change after modification. /// /// ### Macro expansion /// -/// The macro create an enum `Call` with one variant per dispatchable. This enum implements: +/// The macro creates an enum `Call` with one variant per dispatchable. This enum implements: /// `Clone`, `Eq`, `PartialEq`, `Debug` (with stripped implementation in `not("std")`), /// `Encode`, `Decode`, `GetDispatchInfo`, `GetCallName`, `UnfilteredDispatchable`. /// -/// The macro implement on `Pallet`, the `Callable` trait and a function `call_functions` which -/// returns the dispatchable metadatas. +/// The macro implement the `Callable` trait on `Pallet` and a function `call_functions` which +/// returns the dispatchable metadata. /// /// # Extra constants: `#[pallet::extra_constants]` optional /// diff --git a/frame/support/test/tests/pallet_ui/call_conflicting_indices.rs b/frame/support/test/tests/pallet_ui/call_conflicting_indices.rs new file mode 100644 index 0000000000000..395766c7cd331 --- /dev/null +++ b/frame/support/test/tests/pallet_ui/call_conflicting_indices.rs @@ -0,0 +1,24 @@ +#[frame_support::pallet] +mod pallet { + use frame_support::pallet_prelude::DispatchResultWithPostInfo; + + #[pallet::config] + pub trait Config: frame_system::Config {} + + #[pallet::pallet] + pub struct Pallet(core::marker::PhantomData); + + #[pallet::call] + impl Pallet { + #[pallet::weight(0)] + #[pallet::call_index(10)] + pub fn foo(origin: OriginFor) -> DispatchResultWithPostInfo {} + + #[pallet::weight(0)] + #[pallet::call_index(10)] + pub fn bar(origin: OriginFor) -> DispatchResultWithPostInfo {} + } +} + +fn main() { +} diff --git a/frame/support/test/tests/pallet_ui/call_conflicting_indices.stderr b/frame/support/test/tests/pallet_ui/call_conflicting_indices.stderr new file mode 100644 index 0000000000000..5d0c90609c2a1 --- /dev/null +++ b/frame/support/test/tests/pallet_ui/call_conflicting_indices.stderr @@ -0,0 +1,11 @@ +error: Call indices are conflicting: Both functions foo and bar are at index 10 + --> tests/pallet_ui/call_conflicting_indices.rs:15:10 + | +15 | pub fn foo(origin: OriginFor) -> DispatchResultWithPostInfo {} + | ^^^ + +error: Call indices are conflicting: Both functions foo and bar are at index 10 + --> tests/pallet_ui/call_conflicting_indices.rs:19:10 + | +19 | pub fn bar(origin: OriginFor) -> DispatchResultWithPostInfo {} + | ^^^ diff --git a/frame/support/test/tests/pallet_ui/call_invalid_attr.rs b/frame/support/test/tests/pallet_ui/call_invalid_attr.rs new file mode 100644 index 0000000000000..118d3c92f360d --- /dev/null +++ b/frame/support/test/tests/pallet_ui/call_invalid_attr.rs @@ -0,0 +1,20 @@ +#[frame_support::pallet] +mod pallet { + use frame_support::pallet_prelude::DispatchResultWithPostInfo; + use frame_system::pallet_prelude::OriginFor; + + #[pallet::config] + pub trait Config: frame_system::Config {} + + #[pallet::pallet] + pub struct Pallet(core::marker::PhantomData); + + #[pallet::call] + impl Pallet { + #[pallet::weird_attr] + pub fn foo(origin: OriginFor) -> DispatchResultWithPostInfo {} + } +} + +fn main() { +} diff --git a/frame/support/test/tests/pallet_ui/call_invalid_attr.stderr b/frame/support/test/tests/pallet_ui/call_invalid_attr.stderr new file mode 100644 index 0000000000000..3f680203a262f --- /dev/null +++ b/frame/support/test/tests/pallet_ui/call_invalid_attr.stderr @@ -0,0 +1,5 @@ +error: expected `weight` or `call_index` + --> tests/pallet_ui/call_invalid_attr.rs:14:13 + | +14 | #[pallet::weird_attr] + | ^^^^^^^^^^ diff --git a/frame/support/test/tests/pallet_ui/call_invalid_index.rs b/frame/support/test/tests/pallet_ui/call_invalid_index.rs new file mode 100644 index 0000000000000..0b40691ca43a3 --- /dev/null +++ b/frame/support/test/tests/pallet_ui/call_invalid_index.rs @@ -0,0 +1,21 @@ +#[frame_support::pallet] +mod pallet { + use frame_support::pallet_prelude::DispatchResultWithPostInfo; + use frame_system::pallet_prelude::OriginFor; + + #[pallet::config] + pub trait Config: frame_system::Config {} + + #[pallet::pallet] + pub struct Pallet(core::marker::PhantomData); + + #[pallet::call] + impl Pallet { + #[pallet::weight(0)] + #[pallet::call_index(256)] + pub fn foo(origin: OriginFor) -> DispatchResultWithPostInfo {} + } +} + +fn main() { +} diff --git a/frame/support/test/tests/pallet_ui/call_invalid_index.stderr b/frame/support/test/tests/pallet_ui/call_invalid_index.stderr new file mode 100644 index 0000000000000..1e07a4974bf69 --- /dev/null +++ b/frame/support/test/tests/pallet_ui/call_invalid_index.stderr @@ -0,0 +1,5 @@ +error: number too large to fit in target type + --> tests/pallet_ui/call_invalid_index.rs:15:24 + | +15 | #[pallet::call_index(256)] + | ^^^ diff --git a/frame/support/test/tests/pallet_ui/call_multiple_call_index.rs b/frame/support/test/tests/pallet_ui/call_multiple_call_index.rs new file mode 100644 index 0000000000000..5753ecd076c80 --- /dev/null +++ b/frame/support/test/tests/pallet_ui/call_multiple_call_index.rs @@ -0,0 +1,22 @@ +#[frame_support::pallet] +mod pallet { + use frame_support::pallet_prelude::DispatchResultWithPostInfo; + use frame_system::pallet_prelude::OriginFor; + + #[pallet::config] + pub trait Config: frame_system::Config {} + + #[pallet::pallet] + pub struct Pallet(core::marker::PhantomData); + + #[pallet::call] + impl Pallet { + #[pallet::weight(0)] + #[pallet::call_index(1)] + #[pallet::call_index(2)] + pub fn foo(origin: OriginFor) -> DispatchResultWithPostInfo {} + } +} + +fn main() { +} diff --git a/frame/support/test/tests/pallet_ui/call_multiple_call_index.stderr b/frame/support/test/tests/pallet_ui/call_multiple_call_index.stderr new file mode 100644 index 0000000000000..ba22b012745c1 --- /dev/null +++ b/frame/support/test/tests/pallet_ui/call_multiple_call_index.stderr @@ -0,0 +1,5 @@ +error: Invalid pallet::call, too many call_index attributes given + --> tests/pallet_ui/call_multiple_call_index.rs:17:7 + | +17 | pub fn foo(origin: OriginFor) -> DispatchResultWithPostInfo {} + | ^^