From 9370f42ab35f7013f70b703884c9884888a3b153 Mon Sep 17 00:00:00 2001 From: gupnik <17176722+gupnik@users.noreply.github.com> Date: Mon, 24 Apr 2023 17:34:15 +0530 Subject: [PATCH] Adds force_origin support (#13845) * Adds force_origin support * Moves a couple of tests to showcase v2 with force_origin * Adds remaining tests * adds documentation * minor * adds test for invalid origin * ".git/.scripts/commands/fmt/fmt.sh" * updates param to use MaxCalls * Fixes compilation error * Updates doc comment * Fixes test outputs * Fixes test output * ".git/.scripts/commands/fmt/fmt.sh" --------- Co-authored-by: command-bot <> --- frame/benchmarking/src/lib.rs | 7 + frame/lottery/src/benchmarking.rs | 127 ++++++++++++------ frame/support/procedural/src/benchmark.rs | 35 +++-- .../test/tests/benchmark_ui/invalid_origin.rs | 17 +++ .../tests/benchmark_ui/invalid_origin.stderr | 16 +++ 5 files changed, 141 insertions(+), 61 deletions(-) create mode 100644 frame/support/test/tests/benchmark_ui/invalid_origin.rs create mode 100644 frame/support/test/tests/benchmark_ui/invalid_origin.stderr diff --git a/frame/benchmarking/src/lib.rs b/frame/benchmarking/src/lib.rs index 7110c378d581e..b11e164fe8e44 100644 --- a/frame/benchmarking/src/lib.rs +++ b/frame/benchmarking/src/lib.rs @@ -160,6 +160,13 @@ pub use v1::*; /// The underscore will be substituted with the name of the benchmark (i.e. the name of the /// function in the benchmark function definition). /// +/// In case of a `force_origin` where you want to elevate the privileges of the provided origin, +/// this is the general syntax: +/// ```ignore +/// #[extrinsic_call] +/// _(force_origin as T::RuntimeOrigin, 0u32.into(), 0); +/// ``` +/// /// Regardless of whether `#[extrinsic_call]` or `#[block]` is used, this attribute also serves /// the purpose of designating the boundary between the setup code portion of the benchmark /// (everything before the `#[extrinsic_call]` or `#[block]` attribute) and the verification diff --git a/frame/lottery/src/benchmarking.rs b/frame/lottery/src/benchmarking.rs index 9216464b07d35..1510d250dbeaf 100644 --- a/frame/lottery/src/benchmarking.rs +++ b/frame/lottery/src/benchmarking.rs @@ -21,7 +21,12 @@ use super::*; -use frame_benchmarking::v1::{account, benchmarks, whitelisted_caller, BenchmarkError}; +use crate::Pallet as Lottery; +use frame_benchmarking::{ + impl_benchmark_test_suite, + v1::{account, whitelisted_caller, BenchmarkError}, + v2::*, +}; use frame_support::{ storage::bounded_vec::BoundedVec, traits::{EnsureOrigin, OnInitialize}, @@ -29,8 +34,6 @@ use frame_support::{ use frame_system::RawOrigin; use sp_runtime::traits::{Bounded, Zero}; -use crate::Pallet as Lottery; - // Set up and start a lottery fn setup_lottery(repeat: bool) -> Result<(), &'static str> { let price = T::Currency::minimum_balance(); @@ -50,72 +53,100 @@ fn setup_lottery(repeat: bool) -> Result<(), &'static str> { Ok(()) } -benchmarks! { - buy_ticket { +#[benchmarks] +mod benchmarks { + use super::*; + + #[benchmark] + fn buy_ticket() -> Result<(), BenchmarkError> { let caller = whitelisted_caller(); T::Currency::make_free_balance_be(&caller, BalanceOf::::max_value()); setup_lottery::(false)?; // force user to have a long vec of calls participating let set_code_index: CallIndex = Lottery::::call_to_index( - &frame_system::Call::::set_code{ code: vec![] }.into() + &frame_system::Call::::set_code { code: vec![] }.into(), )?; let already_called: (u32, BoundedVec) = ( LotteryIndex::::get(), BoundedVec::::try_from(vec![ set_code_index; - T::MaxCalls::get().saturating_sub(1) as usize - ]).unwrap(), + T::MaxCalls::get().saturating_sub(1) + as usize + ]) + .unwrap(), ); Participants::::insert(&caller, already_called); let call = frame_system::Call::::remark { remark: vec![] }; - }: _(RawOrigin::Signed(caller), Box::new(call.into())) - verify { + + #[extrinsic_call] + _(RawOrigin::Signed(caller), Box::new(call.into())); + assert_eq!(TicketsCount::::get(), 1); + + Ok(()) } - set_calls { - let n in 0 .. T::MaxCalls::get() as u32; + #[benchmark] + fn set_calls(n: Linear<0, { T::MaxCalls::get() }>) -> Result<(), BenchmarkError> { let calls = vec![frame_system::Call::::remark { remark: vec![] }.into(); n as usize]; let origin = T::ManagerOrigin::try_successful_origin().map_err(|_| BenchmarkError::Weightless)?; assert!(CallIndices::::get().is_empty()); - }: _(origin, calls) - verify { + + #[extrinsic_call] + _(origin as T::RuntimeOrigin, calls); + if !n.is_zero() { assert!(!CallIndices::::get().is_empty()); } + + Ok(()) } - start_lottery { + #[benchmark] + fn start_lottery() -> Result<(), BenchmarkError> { let price = BalanceOf::::max_value(); let end = 10u32.into(); let payout = 5u32.into(); let origin = T::ManagerOrigin::try_successful_origin().map_err(|_| BenchmarkError::Weightless)?; - }: _(origin, price, end, payout, true) - verify { + + #[extrinsic_call] + _(origin as T::RuntimeOrigin, price, end, payout, true); + assert!(crate::Lottery::::get().is_some()); + + Ok(()) } - stop_repeat { + #[benchmark] + fn stop_repeat() -> Result<(), BenchmarkError> { setup_lottery::(true)?; assert_eq!(crate::Lottery::::get().unwrap().repeat, true); let origin = T::ManagerOrigin::try_successful_origin().map_err(|_| BenchmarkError::Weightless)?; - }: _(origin) - verify { + + #[extrinsic_call] + _(origin as T::RuntimeOrigin); + assert_eq!(crate::Lottery::::get().unwrap().repeat, false); + + Ok(()) } - on_initialize_end { + #[benchmark] + fn on_initialize_end() -> Result<(), BenchmarkError> { setup_lottery::(false)?; let winner = account("winner", 0, 0); // User needs more than min balance to get ticket T::Currency::make_free_balance_be(&winner, T::Currency::minimum_balance() * 10u32.into()); // Make sure lottery account has at least min balance too let lottery_account = Lottery::::account_id(); - T::Currency::make_free_balance_be(&lottery_account, T::Currency::minimum_balance() * 10u32.into()); + T::Currency::make_free_balance_be( + &lottery_account, + T::Currency::minimum_balance() * 10u32.into(), + ); // Buy a ticket let call = frame_system::Call::::remark { remark: vec![] }; Lottery::::buy_ticket(RawOrigin::Signed(winner.clone()).into(), Box::new(call.into()))?; @@ -124,29 +155,37 @@ benchmarks! { // Assert that lotto is set up for winner assert_eq!(TicketsCount::::get(), 1); assert!(!Lottery::::pot().1.is_zero()); - }: { - // Generate `MaxGenerateRandom` numbers for worst case scenario - for i in 0 .. T::MaxGenerateRandom::get() { - Lottery::::generate_random_number(i); + + #[block] + { + // Generate `MaxGenerateRandom` numbers for worst case scenario + for i in 0..T::MaxGenerateRandom::get() { + Lottery::::generate_random_number(i); + } + // Start lottery has block 15 configured for payout + Lottery::::on_initialize(15u32.into()); } - // Start lottery has block 15 configured for payout - Lottery::::on_initialize(15u32.into()); - } - verify { + assert!(crate::Lottery::::get().is_none()); assert_eq!(TicketsCount::::get(), 0); assert_eq!(Lottery::::pot().1, 0u32.into()); - assert!(!T::Currency::free_balance(&winner).is_zero()) + assert!(!T::Currency::free_balance(&winner).is_zero()); + + Ok(()) } - on_initialize_repeat { + #[benchmark] + fn on_initialize_repeat() -> Result<(), BenchmarkError> { setup_lottery::(true)?; let winner = account("winner", 0, 0); // User needs more than min balance to get ticket T::Currency::make_free_balance_be(&winner, T::Currency::minimum_balance() * 10u32.into()); // Make sure lottery account has at least min balance too let lottery_account = Lottery::::account_id(); - T::Currency::make_free_balance_be(&lottery_account, T::Currency::minimum_balance() * 10u32.into()); + T::Currency::make_free_balance_be( + &lottery_account, + T::Currency::minimum_balance() * 10u32.into(), + ); // Buy a ticket let call = frame_system::Call::::remark { remark: vec![] }; Lottery::::buy_ticket(RawOrigin::Signed(winner.clone()).into(), Box::new(call.into()))?; @@ -155,20 +194,24 @@ benchmarks! { // Assert that lotto is set up for winner assert_eq!(TicketsCount::::get(), 1); assert!(!Lottery::::pot().1.is_zero()); - }: { - // Generate `MaxGenerateRandom` numbers for worst case scenario - for i in 0 .. T::MaxGenerateRandom::get() { - Lottery::::generate_random_number(i); + + #[block] + { + // Generate `MaxGenerateRandom` numbers for worst case scenario + for i in 0..T::MaxGenerateRandom::get() { + Lottery::::generate_random_number(i); + } + // Start lottery has block 15 configured for payout + Lottery::::on_initialize(15u32.into()); } - // Start lottery has block 15 configured for payout - Lottery::::on_initialize(15u32.into()); - } - verify { + assert!(crate::Lottery::::get().is_some()); assert_eq!(LotteryIndex::::get(), 2); assert_eq!(TicketsCount::::get(), 0); assert_eq!(Lottery::::pot().1, 0u32.into()); - assert!(!T::Currency::free_balance(&winner).is_zero()) + assert!(!T::Currency::free_balance(&winner).is_zero()); + + Ok(()) } impl_benchmark_test_suite!(Lottery, crate::mock::new_test_ext(), crate::mock::Test); diff --git a/frame/support/procedural/src/benchmark.rs b/frame/support/procedural/src/benchmark.rs index 315a6000bc8bf..28b5aa1b983b7 100644 --- a/frame/support/procedural/src/benchmark.rs +++ b/frame/support/procedural/src/benchmark.rs @@ -53,7 +53,7 @@ mod keywords { #[derive(Clone)] struct ParamDef { name: String, - typ: Type, + _typ: Type, start: syn::GenericArgument, end: syn::GenericArgument, } @@ -229,7 +229,7 @@ fn parse_params(item_fn: &ItemFn) -> Result> { let args = segment.arguments.to_token_stream().into(); let Ok(args) = syn::parse::(args) else { return invalid_param(typ.span()) }; - params.push(ParamDef { name, typ: typ.clone(), start: args.start, end: args.end }); + params.push(ParamDef { name, _typ: typ.clone(), start: args.start, end: args.end }); } Ok(params) } @@ -681,7 +681,6 @@ pub fn benchmarks( struct UnrolledParams { param_ranges: Vec, param_names: Vec, - param_types: Vec, } impl UnrolledParams { @@ -703,14 +702,7 @@ impl UnrolledParams { quote!(#name) }) .collect(); - let param_types: Vec = params - .iter() - .map(|p| { - let typ = &p.typ; - quote!(#typ) - }) - .collect(); - UnrolledParams { param_ranges, param_names, param_types } + UnrolledParams { param_ranges, param_names } } } @@ -726,7 +718,6 @@ fn expand_benchmark( Ok(ident) => ident, Err(err) => return err.to_compile_error().into(), }; - let home = quote!(#krate::v2); let codec = quote!(#krate::frame_support::codec); let traits = quote!(#krate::frame_support::traits); let setup_stmts = benchmark_def.setup_stmts; @@ -738,7 +729,6 @@ fn expand_benchmark( let unrolled = UnrolledParams::from(&benchmark_def.params); let param_names = unrolled.param_names; let param_ranges = unrolled.param_ranges; - let param_types = unrolled.param_types; let type_use_generics = match is_instance { false => quote!(T), @@ -763,6 +753,18 @@ fn expand_benchmark( } expr_call.args = final_args; + let origin = match origin { + Expr::Cast(t) => { + let ty = t.ty.clone(); + quote! { + <::RuntimeOrigin as From<#ty>>::from(#origin); + } + }, + _ => quote! { + #origin.into(); + }, + }; + // determine call name (handles `_` and normal call syntax) let expr_span = expr_call.span(); let call_err = || { @@ -803,7 +805,7 @@ fn expand_benchmark( let __call_decoded = as #codec::Decode> ::decode(&mut &__benchmarked_call_encoded[..]) .expect("call is encoded above, encoding must be correct"); - let __origin = #origin.into(); + let __origin = #origin; as #traits::UnfilteredDispatchable>::dispatch_bypass_filter( __call_decoded, __origin, @@ -877,11 +879,6 @@ fn expand_benchmark( // benchmark function definition #fn_def - // compile-time assertions that each referenced param type implements ParamRange - #( - #home::assert_impl_all!(#param_types: #home::ParamRange); - )* - #[allow(non_camel_case_types)] #( #fn_attrs diff --git a/frame/support/test/tests/benchmark_ui/invalid_origin.rs b/frame/support/test/tests/benchmark_ui/invalid_origin.rs new file mode 100644 index 0000000000000..81cfc39790969 --- /dev/null +++ b/frame/support/test/tests/benchmark_ui/invalid_origin.rs @@ -0,0 +1,17 @@ +use frame_benchmarking::v2::*; +#[allow(unused_imports)] +use frame_support_test::Config; +use frame_support_test::Call; + +#[benchmarks] +mod benches { + use super::*; + + #[benchmark] + fn bench() { + #[extrinsic_call] + thing(1); + } +} + +fn main() {} diff --git a/frame/support/test/tests/benchmark_ui/invalid_origin.stderr b/frame/support/test/tests/benchmark_ui/invalid_origin.stderr new file mode 100644 index 0000000000000..ab8499d995d47 --- /dev/null +++ b/frame/support/test/tests/benchmark_ui/invalid_origin.stderr @@ -0,0 +1,16 @@ +error[E0599]: no variant or associated item named `new_call_variant_thing` found for enum `Call` in the current scope + --> tests/benchmark_ui/invalid_origin.rs:6:1 + | +6 | #[benchmarks] + | ^^^^^^^^^^^^^ variant or associated item not found in `Call` + | + = note: this error originates in the attribute macro `benchmarks` (in Nightly builds, run with -Z macro-backtrace for more info) + +error[E0277]: the trait bound `::RuntimeOrigin: From<{integer}>` is not satisfied + --> tests/benchmark_ui/invalid_origin.rs:6:1 + | +6 | #[benchmarks] + | ^^^^^^^^^^^^^ the trait `From<{integer}>` is not implemented for `::RuntimeOrigin` + | + = note: required for `{integer}` to implement `Into<::RuntimeOrigin>` + = note: this error originates in the attribute macro `benchmarks` (in Nightly builds, run with -Z macro-backtrace for more info)