Skip to content

Commit

Permalink
Fix Origins Used in Runtime Benchmarks (paritytech#12037)
Browse files Browse the repository at this point in the history
* fix identity benchmark origin

* fix benchmarking macro

* bounties

* democracy

* scheduler

* tips and treasury

* unused

* dunno, this is causing issues
  • Loading branch information
shawntabrizi authored and ark0f committed Feb 27, 2023
1 parent 5686d29 commit 959259e
Show file tree
Hide file tree
Showing 8 changed files with 65 additions and 41 deletions.
14 changes: 7 additions & 7 deletions frame/benchmarking/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -547,7 +547,7 @@ macro_rules! benchmarks_iter {
( $( $names:tt )* )
( $( $names_extra:tt )* )
( $( $names_skip_meta:tt )* )
$name:ident { $( $code:tt )* }: _ ( $origin:expr $( , $arg:expr )* )
$name:ident { $( $code:tt )* }: _ $(<$origin_type:ty>)? ( $origin:expr $( , $arg:expr )* )
$( $rest:tt )*
) => {
$crate::benchmarks_iter! {
Expand All @@ -557,7 +557,7 @@ macro_rules! benchmarks_iter {
( $( $names )* )
( $( $names_extra )* )
( $( $names_skip_meta )* )
$name { $( $code )* }: _ ( $origin $( , $arg )* )
$name { $( $code )* }: _ $(<$origin_type>)? ( $origin $( , $arg )* )
verify { }
$( $rest )*
}
Expand All @@ -570,7 +570,7 @@ macro_rules! benchmarks_iter {
( $( $names:tt )* )
( $( $names_extra:tt )* )
( $( $names_skip_meta:tt )* )
$name:ident { $( $code:tt )* }: $dispatch:ident ( $origin:expr $( , $arg:expr )* )
$name:ident { $( $code:tt )* }: $dispatch:ident $(<$origin_type:ty>)? ( $origin:expr $( , $arg:expr )* )
$( $rest:tt )*
) => {
$crate::benchmarks_iter! {
Expand All @@ -580,7 +580,7 @@ macro_rules! benchmarks_iter {
( $( $names )* )
( $( $names_extra )* )
( $( $names_skip_meta )* )
$name { $( $code )* }: $dispatch ( $origin $( , $arg )* )
$name { $( $code )* }: $dispatch $(<$origin_type>)? ( $origin $( , $arg )* )
verify { }
$( $rest )*
}
Expand All @@ -593,7 +593,7 @@ macro_rules! benchmarks_iter {
( $( $names:tt )* )
( $( $names_extra:tt )* )
( $( $names_skip_meta:tt )* )
$name:ident { $( $code:tt )* }: $eval:block
$name:ident { $( $code:tt )* }: $(<$origin_type:ty>)? $eval:block
$( $rest:tt )*
) => {
$crate::benchmarks_iter!(
Expand All @@ -603,7 +603,7 @@ macro_rules! benchmarks_iter {
( $( $names )* )
( $( $names_extra )* )
( $( $names_skip_meta )* )
$name { $( $code )* }: $eval
$name { $( $code )* }: $(<$origin_type>)? $eval
verify { }
$( $rest )*
);
Expand All @@ -617,7 +617,7 @@ macro_rules! to_origin {
$origin.into()
};
($origin:expr, $origin_type:ty) => {
<T::Origin as From<$origin_type>>::from($origin)
<<T as frame_system::Config>::Origin as From<$origin_type>>::from($origin)
};
}

Expand Down
33 changes: 18 additions & 15 deletions frame/bounties/src/benchmarking.rs
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,8 @@ fn create_approved_bounties<T: Config<I>, I: 'static>(n: u32) -> Result<(), &'st
setup_bounty::<T, I>(i, T::MaximumReasonLength::get());
Bounties::<T, I>::propose_bounty(RawOrigin::Signed(caller).into(), value, reason)?;
let bounty_id = BountyCount::<T, I>::get() - 1;
Bounties::<T, I>::approve_bounty(RawOrigin::Root.into(), bounty_id)?;
let approve_origin = T::ApproveOrigin::successful_origin();
Bounties::<T, I>::approve_bounty(approve_origin, bounty_id)?;
}
ensure!(BountyApprovals::<T, I>::get().len() == n as usize, "Not all bounty approved");
Ok(())
Expand Down Expand Up @@ -67,14 +68,10 @@ fn create_bounty<T: Config<I>, I: 'static>(
let curator_lookup = T::Lookup::unlookup(curator.clone());
Bounties::<T, I>::propose_bounty(RawOrigin::Signed(caller).into(), value, reason)?;
let bounty_id = BountyCount::<T, I>::get() - 1;
Bounties::<T, I>::approve_bounty(RawOrigin::Root.into(), bounty_id)?;
let approve_origin = T::ApproveOrigin::successful_origin();
Bounties::<T, I>::approve_bounty(approve_origin.clone(), bounty_id)?;
Treasury::<T, I>::on_initialize(T::BlockNumber::zero());
Bounties::<T, I>::propose_curator(
RawOrigin::Root.into(),
bounty_id,
curator_lookup.clone(),
fee,
)?;
Bounties::<T, I>::propose_curator(approve_origin, bounty_id, curator_lookup.clone(), fee)?;
Bounties::<T, I>::accept_curator(RawOrigin::Signed(curator).into(), bounty_id)?;
Ok((curator_lookup, bounty_id))
}
Expand All @@ -100,17 +97,20 @@ benchmarks_instance_pallet! {
let (caller, curator, fee, value, reason) = setup_bounty::<T, I>(0, T::MaximumReasonLength::get());
Bounties::<T, I>::propose_bounty(RawOrigin::Signed(caller).into(), value, reason)?;
let bounty_id = BountyCount::<T, I>::get() - 1;
}: _(RawOrigin::Root, bounty_id)
let approve_origin = T::ApproveOrigin::successful_origin();
}: _<T::Origin>(approve_origin, bounty_id)

propose_curator {
setup_pot_account::<T, I>();
let (caller, curator, fee, value, reason) = setup_bounty::<T, I>(0, T::MaximumReasonLength::get());
let curator_lookup = T::Lookup::unlookup(curator);
Bounties::<T, I>::propose_bounty(RawOrigin::Signed(caller).into(), value, reason)?;
let bounty_id = BountyCount::<T, I>::get() - 1;
Bounties::<T, I>::approve_bounty(RawOrigin::Root.into(), bounty_id)?;
let approve_origin = T::ApproveOrigin::successful_origin();
Bounties::<T, I>::approve_bounty(approve_origin, bounty_id)?;
Treasury::<T, I>::on_initialize(T::BlockNumber::zero());
}: _(RawOrigin::Root, bounty_id, curator_lookup, fee)
let approve_origin = T::ApproveOrigin::successful_origin();
}: _<T::Origin>(approve_origin, bounty_id, curator_lookup, fee)

// Worst case when curator is inactive and any sender unassigns the curator.
unassign_curator {
Expand All @@ -128,9 +128,10 @@ benchmarks_instance_pallet! {
let curator_lookup = T::Lookup::unlookup(curator.clone());
Bounties::<T, I>::propose_bounty(RawOrigin::Signed(caller).into(), value, reason)?;
let bounty_id = BountyCount::<T, I>::get() - 1;
Bounties::<T, I>::approve_bounty(RawOrigin::Root.into(), bounty_id)?;
let approve_origin = T::ApproveOrigin::successful_origin();
Bounties::<T, I>::approve_bounty(approve_origin.clone(), bounty_id)?;
Treasury::<T, I>::on_initialize(T::BlockNumber::zero());
Bounties::<T, I>::propose_curator(RawOrigin::Root.into(), bounty_id, curator_lookup, fee)?;
Bounties::<T, I>::propose_curator(approve_origin, bounty_id, curator_lookup, fee)?;
}: _(RawOrigin::Signed(curator), bounty_id)

award_bounty {
Expand Down Expand Up @@ -169,14 +170,16 @@ benchmarks_instance_pallet! {
let (caller, curator, fee, value, reason) = setup_bounty::<T, I>(0, 0);
Bounties::<T, I>::propose_bounty(RawOrigin::Signed(caller).into(), value, reason)?;
let bounty_id = BountyCount::<T, I>::get() - 1;
}: close_bounty(RawOrigin::Root, bounty_id)
let approve_origin = T::ApproveOrigin::successful_origin();
}: close_bounty<T::Origin>(approve_origin, bounty_id)

close_bounty_active {
setup_pot_account::<T, I>();
let (curator_lookup, bounty_id) = create_bounty::<T, I>()?;
Treasury::<T, I>::on_initialize(T::BlockNumber::zero());
let bounty_id = BountyCount::<T, I>::get() - 1;
}: close_bounty(RawOrigin::Root, bounty_id)
let approve_origin = T::ApproveOrigin::successful_origin();
}: close_bounty<T::Origin>(approve_origin, bounty_id)
verify {
assert_last_event::<T, I>(Event::BountyCanceled { index: bounty_id }.into())
}
Expand Down
4 changes: 3 additions & 1 deletion frame/democracy/src/benchmarking.rs
Original file line number Diff line number Diff line change
Expand Up @@ -318,7 +318,9 @@ benchmarks! {
for i in 0 .. p {
add_proposal::<T>(i)?;
}
}: _(RawOrigin::Root, 0)

let cancel_origin = T::CancelProposalOrigin::successful_origin();
}: _<T::Origin>(cancel_origin, 0)

cancel_referendum {
let referendum_index = add_referendum::<T>(0)?;
Expand Down
26 changes: 18 additions & 8 deletions frame/identity/src/benchmarking.rs
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,10 @@ use super::*;

use crate::Pallet as Identity;
use frame_benchmarking::{account, benchmarks, whitelisted_caller};
use frame_support::{ensure, traits::Get};
use frame_support::{
ensure,
traits::{EnsureOrigin, Get},
};
use frame_system::RawOrigin;
use sp_runtime::traits::Bounded;

Expand All @@ -38,7 +41,8 @@ fn add_registrars<T: Config>(r: u32) -> Result<(), &'static str> {
for i in 0..r {
let registrar: T::AccountId = account("registrar", i, SEED);
let _ = T::Currency::make_free_balance_be(&registrar, BalanceOf::<T>::max_value());
Identity::<T>::add_registrar(RawOrigin::Root.into(), registrar.clone())?;
let registrar_origin = T::RegistrarOrigin::successful_origin();
Identity::<T>::add_registrar(registrar_origin, registrar.clone())?;
Identity::<T>::set_fee(RawOrigin::Signed(registrar.clone()).into(), i, 10u32.into())?;
let fields =
IdentityFields(
Expand Down Expand Up @@ -114,7 +118,8 @@ benchmarks! {
add_registrar {
let r in 1 .. T::MaxRegistrars::get() - 1 => add_registrars::<T>(r)?;
ensure!(Registrars::<T>::get().len() as u32 == r, "Registrars not set up correctly.");
}: _(RawOrigin::Root, account("registrar", r + 1, SEED))
let origin = T::RegistrarOrigin::successful_origin();
}: _<T::Origin>(origin, account("registrar", r + 1, SEED))
verify {
ensure!(Registrars::<T>::get().len() as u32 == r + 1, "Registrars not added.");
}
Expand Down Expand Up @@ -259,7 +264,8 @@ benchmarks! {

let r in 1 .. T::MaxRegistrars::get() - 1 => add_registrars::<T>(r)?;

Identity::<T>::add_registrar(RawOrigin::Root.into(), caller.clone())?;
let registrar_origin = T::RegistrarOrigin::successful_origin();
Identity::<T>::add_registrar(registrar_origin, caller.clone())?;
let registrars = Registrars::<T>::get();
ensure!(registrars[r as usize].as_ref().unwrap().fee == 0u32.into(), "Fee already set.");
}: _(RawOrigin::Signed(caller), r, 100u32.into())
Expand All @@ -274,7 +280,8 @@ benchmarks! {

let r in 1 .. T::MaxRegistrars::get() - 1 => add_registrars::<T>(r)?;

Identity::<T>::add_registrar(RawOrigin::Root.into(), caller.clone())?;
let registrar_origin = T::RegistrarOrigin::successful_origin();
Identity::<T>::add_registrar(registrar_origin, caller.clone())?;
let registrars = Registrars::<T>::get();
ensure!(registrars[r as usize].as_ref().unwrap().account == caller, "id not set.");
}: _(RawOrigin::Signed(caller), r, account("new", 0, SEED))
Expand All @@ -289,7 +296,8 @@ benchmarks! {

let r in 1 .. T::MaxRegistrars::get() - 1 => add_registrars::<T>(r)?;

Identity::<T>::add_registrar(RawOrigin::Root.into(), caller.clone())?;
let registrar_origin = T::RegistrarOrigin::successful_origin();
Identity::<T>::add_registrar(registrar_origin, caller.clone())?;
let fields = IdentityFields(
IdentityField::Display | IdentityField::Legal | IdentityField::Web | IdentityField::Riot
| IdentityField::Email | IdentityField::PgpFingerprint | IdentityField::Image | IdentityField::Twitter
Expand Down Expand Up @@ -318,7 +326,8 @@ benchmarks! {
Identity::<T>::set_identity(user_origin.clone(), Box::new(info))?;
};

Identity::<T>::add_registrar(RawOrigin::Root.into(), caller.clone())?;
let registrar_origin = T::RegistrarOrigin::successful_origin();
Identity::<T>::add_registrar(registrar_origin, caller.clone())?;
Identity::<T>::request_judgement(user_origin, r, 10u32.into())?;
}: _(RawOrigin::Signed(caller), r, user_lookup, Judgement::Reasonable)
verify {
Expand Down Expand Up @@ -350,7 +359,8 @@ benchmarks! {
)?;
}
ensure!(IdentityOf::<T>::contains_key(&target), "Identity not set");
}: _(RawOrigin::Root, target_lookup)
let origin = T::ForceOrigin::successful_origin();
}: _<T::Origin>(origin, target_lookup)
verify {
ensure!(!IdentityOf::<T>::contains_key(&target), "Identity not removed");
}
Expand Down
15 changes: 10 additions & 5 deletions frame/scheduler/src/benchmarking.rs
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,6 @@ use frame_support::{
ensure,
traits::{OnInitialize, PreimageProvider, PreimageRecipient},
};
use frame_system::RawOrigin;
use sp_runtime::traits::Hash;
use sp_std::{prelude::*, vec};

Expand All @@ -32,6 +31,8 @@ use frame_system::Pallet as System;

const BLOCK_NUMBER: u32 = 2;

type SystemOrigin<T> = <T as frame_system::Config>::Origin;

/// Add `n` named items to the schedule.
///
/// For `resolved`:
Expand Down Expand Up @@ -210,7 +211,8 @@ benchmarks! {
let call = Box::new(CallOrHashOf::<T>::Value(inner_call));

fill_schedule::<T>(when, s, true, true, Some(false))?;
}: _(RawOrigin::Root, when, periodic, priority, call)
let schedule_origin = T::ScheduleOrigin::successful_origin();
}: _<SystemOrigin<T>>(schedule_origin, when, periodic, priority, call)
verify {
ensure!(
Agenda::<T>::get(when).len() == (s + 1) as usize,
Expand All @@ -224,7 +226,8 @@ benchmarks! {

fill_schedule::<T>(when, s, true, true, Some(false))?;
assert_eq!(Agenda::<T>::get(when).len(), s as usize);
}: _(RawOrigin::Root, when, 0)
let schedule_origin = T::ScheduleOrigin::successful_origin();
}: _<SystemOrigin<T>>(schedule_origin, when, 0)
verify {
ensure!(
Lookup::<T>::get(0.encode()).is_none(),
Expand All @@ -248,7 +251,8 @@ benchmarks! {
let call = Box::new(CallOrHashOf::<T>::Value(inner_call));

fill_schedule::<T>(when, s, true, true, Some(false))?;
}: _(RawOrigin::Root, id, when, periodic, priority, call)
let schedule_origin = T::ScheduleOrigin::successful_origin();
}: _<SystemOrigin<T>>(schedule_origin, id, when, periodic, priority, call)
verify {
ensure!(
Agenda::<T>::get(when).len() == (s + 1) as usize,
Expand All @@ -261,7 +265,8 @@ benchmarks! {
let when = BLOCK_NUMBER.into();

fill_schedule::<T>(when, s, true, true, Some(false))?;
}: _(RawOrigin::Root, 0.encode())
let schedule_origin = T::ScheduleOrigin::successful_origin();
}: _<SystemOrigin<T>>(schedule_origin, 0.encode())
verify {
ensure!(
Lookup::<T>::get(0.encode()).is_none(),
Expand Down
2 changes: 1 addition & 1 deletion frame/staking/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -71,6 +71,6 @@ runtime-benchmarks = [
"frame-benchmarking/runtime-benchmarks",
"frame-election-provider-support/runtime-benchmarks",
"rand_chacha",
"sp-staking/runtime-benchmarks"
"sp-staking/runtime-benchmarks",
]
try-runtime = ["frame-support/try-runtime"]
3 changes: 2 additions & 1 deletion frame/tips/src/benchmarking.rs
Original file line number Diff line number Diff line change
Expand Up @@ -190,7 +190,8 @@ benchmarks_instance_pallet! {
let reason_hash = T::Hashing::hash(&reason[..]);
let hash = T::Hashing::hash_of(&(&reason_hash, &beneficiary));
ensure!(Tips::<T, I>::contains_key(hash), "tip does not exist");
}: _(RawOrigin::Root, hash)
let reject_origin = T::RejectOrigin::successful_origin();
}: _<T::Origin>(reject_origin, hash)

impl_benchmark_test_suite!(TipsMod, crate::tests::new_test_ext(), crate::tests::Test);
}
9 changes: 6 additions & 3 deletions frame/treasury/src/benchmarking.rs
Original file line number Diff line number Diff line change
Expand Up @@ -99,7 +99,8 @@ benchmarks_instance_pallet! {
beneficiary_lookup
)?;
let proposal_id = Treasury::<T, _>::proposal_count() - 1;
}: _(RawOrigin::Root, proposal_id)
let reject_origin = T::RejectOrigin::successful_origin();
}: _<T::Origin>(reject_origin, proposal_id)

approve_proposal {
let p in 0 .. T::MaxApprovals::get() - 1;
Expand All @@ -111,7 +112,8 @@ benchmarks_instance_pallet! {
beneficiary_lookup
)?;
let proposal_id = Treasury::<T, _>::proposal_count() - 1;
}: _(RawOrigin::Root, proposal_id)
let approve_origin = T::ApproveOrigin::successful_origin();
}: _<T::Origin>(approve_origin, proposal_id)

remove_approval {
let (caller, value, beneficiary_lookup) = setup_proposal::<T, _>(SEED);
Expand All @@ -122,7 +124,8 @@ benchmarks_instance_pallet! {
)?;
let proposal_id = Treasury::<T, _>::proposal_count() - 1;
Treasury::<T, I>::approve_proposal(RawOrigin::Root.into(), proposal_id)?;
}: _(RawOrigin::Root, proposal_id)
let reject_origin = T::RejectOrigin::successful_origin();
}: _<T::Origin>(reject_origin, proposal_id)

on_initialize_proposals {
let p in 0 .. T::MaxApprovals::get();
Expand Down

0 comments on commit 959259e

Please sign in to comment.