From 687d67cf7e4520042770432d302a0d7096685982 Mon Sep 17 00:00:00 2001 From: Keith Yeung Date: Mon, 9 May 2022 20:29:21 -0700 Subject: [PATCH 1/5] Introduce #[pallet::call_index] attribute to dispatchables --- .../procedural/src/pallet/expand/call.rs | 2 + .../procedural/src/pallet/parse/call.rs | 74 +++++++++++++++---- frame/support/src/lib.rs | 13 +++- .../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 ++ 9 files changed, 149 insertions(+), 18 deletions(-) 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..b7862151a231b 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_indices = 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_indices)] #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..95fe6e986dbd8 100644 --- a/frame/support/procedural/src/pallet/parse/call.rs +++ b/frame/support/procedural/src/pallet/parse/call.rs @@ -25,6 +25,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 +56,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 +76,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 +160,7 @@ impl CallDef { } let mut methods = vec![]; + 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 +198,42 @@ 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 = + call_index.unwrap_or_else(|| last_index.map(|idx| idx + 1).unwrap_or(0)); + last_index = Some(final_index); let mut args = vec![]; for arg in method.sig.inputs.iter_mut().skip(1) { @@ -223,7 +263,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 3e84c009b5ca6..12a2852335e51 100644 --- a/frame/support/src/lib.rs +++ b/frame/support/src/lib.rs @@ -1645,6 +1645,9 @@ 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 arguments must implement `Debug`, `PartialEq`, `Eq`, `Decode`, `Encode`, `Clone`. For /// ease of use, bound the trait `Member` available in frame_support::pallet_prelude. /// @@ -1658,16 +1661,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_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 {} + | ^^ From 85f43f00b0462a0083eb007fd529227ade2b8c54 Mon Sep 17 00:00:00 2001 From: Keith Yeung Date: Mon, 9 May 2022 23:42:01 -0700 Subject: [PATCH 2/5] cargo fmt --- .../procedural/src/pallet/parse/call.rs | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/frame/support/procedural/src/pallet/parse/call.rs b/frame/support/procedural/src/pallet/parse/call.rs index 95fe6e986dbd8..d33549bb7e3f0 100644 --- a/frame/support/procedural/src/pallet/parse/call.rs +++ b/frame/support/procedural/src/pallet/parse/call.rs @@ -199,15 +199,15 @@ impl CallDef { } 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 - } - }); + 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() { From 9932c8991e9d425943f4ba4b3457ebdd5f5d4359 Mon Sep 17 00:00:00 2001 From: Keith Yeung Date: Fri, 13 May 2022 18:48:18 -0700 Subject: [PATCH 3/5] Add more docs and prevent duplicates of call indices --- .../procedural/src/pallet/expand/call.rs | 4 ++-- .../procedural/src/pallet/parse/call.rs | 22 +++++++++++++++++-- frame/support/src/lib.rs | 6 +++++ 3 files changed, 28 insertions(+), 4 deletions(-) diff --git a/frame/support/procedural/src/pallet/expand/call.rs b/frame/support/procedural/src/pallet/expand/call.rs index b7862151a231b..ac52463b8aab6 100644 --- a/frame/support/procedural/src/pallet/expand/call.rs +++ b/frame/support/procedural/src/pallet/expand/call.rs @@ -42,7 +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_indices = methods.iter().map(|method| method.call_index).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)) @@ -178,7 +178,7 @@ pub fn expand_call(def: &mut Def) -> proc_macro2::TokenStream { ), #( #( #[doc = #fn_doc] )* - #[codec(index = #call_indices)] + #[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 d33549bb7e3f0..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. @@ -160,6 +161,7 @@ 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 { @@ -231,10 +233,26 @@ impl CallDef { _ => unreachable!("checked during creation of the let binding"), }); - let final_index = - call_index.unwrap_or_else(|| last_index.map(|idx| idx + 1).unwrap_or(0)); + 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) { let arg = if let syn::FnArg::Typed(arg) = arg { diff --git a/frame/support/src/lib.rs b/frame/support/src/lib.rs index 12a2852335e51..7bd521a3d0409 100644 --- a/frame/support/src/lib.rs +++ b/frame/support/src/lib.rs @@ -1648,6 +1648,12 @@ pub mod pallet_prelude { /// 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. /// From 113f1ff4180b981b8408954f0796f392d66cded8 Mon Sep 17 00:00:00 2001 From: Keith Yeung Date: Fri, 13 May 2022 18:55:06 -0700 Subject: [PATCH 4/5] Add UI test for conflicting call indices --- .../pallet_ui/call_conflicting_indices.rs | 24 +++++++++++++++++++ .../pallet_ui/call_conflicting_indices.stderr | 11 +++++++++ 2 files changed, 35 insertions(+) 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 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 {} + | ^^^ From 7a978df0484878596d861a700a49f22fa418638f Mon Sep 17 00:00:00 2001 From: Keith Yeung Date: Fri, 13 May 2022 19:25:25 -0700 Subject: [PATCH 5/5] cargo fmt --- frame/support/src/lib.rs | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/frame/support/src/lib.rs b/frame/support/src/lib.rs index 7bd521a3d0409..16201fd825fc4 100644 --- a/frame/support/src/lib.rs +++ b/frame/support/src/lib.rs @@ -1648,11 +1648,11 @@ pub mod pallet_prelude { /// 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 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.