Skip to content
This repository was archived by the owner on Nov 15, 2023. It is now read-only.

Benchmark's successful origin api update #13146

Merged
muharem merged 15 commits into from
Feb 1, 2023
Merged

Benchmark's successful origin api update #13146

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
15 commits
Select commit Hold shift + click to select a range
cbe6311
try successful origin unimplemented by default
muharem Jan 15, 2023
f7b8aa3
error as a default impl for try_successful_origin
muharem Jan 19, 2023
325232f
remove successful_origin func of EnsureOrigin trait
muharem Jan 19, 2023
d1ffeed
default impl -> unimplemented!()
muharem Jan 20, 2023
441c24f
update EnsureOriginWithArg
muharem Jan 20, 2023
b32b3a7
fix EnsureOriginWithArg
muharem Jan 20, 2023
92841c0
prefix unused arg with underscore
muharem Jan 20, 2023
3795be3
use try_successful_origin instead successful_origin, map err to Weigh…
muharem Jan 20, 2023
9afc56a
fix tests
muharem Jan 20, 2023
a1c2d28
Merge remote-tracking branch 'origin/master' into muharem-try-success…
muharem Jan 20, 2023
3e46afd
remove default impl
muharem Jan 22, 2023
ecb6cae
unwrap for indirect origin dep
muharem Jan 23, 2023
41316a4
replace unwrap by expect with a message
muharem Jan 24, 2023
815c850
Merge remote-tracking branch 'origin/master' into muharem-try-success…
muharem Jan 29, 2023
de5f764
Merge remote-tracking branch 'origin/master' into muharem-try-success…
Jan 31, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
unwrap for indirect origin dep
  • Loading branch information
@muharem
muharem committed Jan 23, 2023
commit ecb6caec3ad524742c0df45b3ac9a5c6da0fd66f
8 changes: 3 additions & 5 deletions frame/democracy/src/benchmarking.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -202,7 +202,7 @@ benchmarks! {
assert_ok!(Democracy::<T>::referendum_status(ref_index));
// Place our proposal in the external queue, too.
assert_ok!(Democracy::<T>::external_propose(
T::ExternalOrigin::try_successful_origin().map_err(|_| BenchmarkError::Weightless)?,
T::ExternalOrigin::try_successful_origin().unwrap(),
make_proposal::<T>(0)
));
let origin =
Expand Down Expand Up @@ -257,8 +257,7 @@ benchmarks! {
}

fast_track {
let origin_propose =
T::ExternalDefaultOrigin::try_successful_origin().map_err(|_| BenchmarkError::Weightless)?;
let origin_propose = T::ExternalDefaultOrigin::try_successful_origin().unwrap();
let proposal = make_proposal::<T>(0);
let proposal_hash = proposal.hash();
Democracy::<T>::external_propose_default(origin_propose, proposal)?;
Expand All @@ -277,8 +276,7 @@ benchmarks! {
let proposal = make_proposal::<T>(0);
let proposal_hash = proposal.hash();

let origin_propose = T::ExternalDefaultOrigin::try_successful_origin()
.map_err(|_| BenchmarkError::Weightless)?;
let origin_propose = T::ExternalDefaultOrigin::try_successful_origin().unwrap();
Democracy::<T>::external_propose_default(origin_propose, proposal)?;

let mut vetoers: BoundedVec<T::AccountId, _> = Default::default();
Expand Down
15 changes: 5 additions & 10 deletions frame/identity/src/benchmarking.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,8 +42,7 @@ fn add_registrars<T: Config>(r: u32) -> Result<(), &'static str> {
let registrar: T::AccountId = account("registrar", i, SEED);
let registrar_lookup = T::Lookup::unlookup(registrar.clone());
let _ = T::Currency::make_free_balance_be(&registrar, BalanceOf::<T>::max_value());
let registrar_origin =
T::RegistrarOrigin::try_successful_origin().map_err(|_| BenchmarkError::Weightless)?;
let registrar_origin = T::RegistrarOrigin::try_successful_origin().unwrap();
Identity::<T>::add_registrar(registrar_origin, registrar_lookup)?;
Identity::<T>::set_fee(RawOrigin::Signed(registrar.clone()).into(), i, 10u32.into())?;
let fields =
Expand Down Expand Up @@ -282,8 +281,7 @@ benchmarks! {

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

let registrar_origin =
T::RegistrarOrigin::try_successful_origin().map_err(|_| BenchmarkError::Weightless)?;
let registrar_origin = T::RegistrarOrigin::try_successful_origin().unwrap();
Identity::<T>::add_registrar(registrar_origin, caller_lookup)?;
let registrars = Registrars::<T>::get();
ensure!(registrars[r as usize].as_ref().unwrap().fee == 0u32.into(), "Fee already set.");
Expand All @@ -300,8 +298,7 @@ benchmarks! {

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

let registrar_origin =
T::RegistrarOrigin::try_successful_origin().map_err(|_| BenchmarkError::Weightless)?;
let registrar_origin = T::RegistrarOrigin::try_successful_origin().unwrap();
Identity::<T>::add_registrar(registrar_origin, caller_lookup)?;
let registrars = Registrars::<T>::get();
ensure!(registrars[r as usize].as_ref().unwrap().account == caller, "id not set.");
Expand All @@ -319,8 +316,7 @@ benchmarks! {

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

let registrar_origin =
T::RegistrarOrigin::try_successful_origin().map_err(|_| BenchmarkError::Weightless)?;
let registrar_origin = T::RegistrarOrigin::try_successful_origin().unwrap();
Identity::<T>::add_registrar(registrar_origin, caller_lookup)?;
let fields = IdentityFields(
IdentityField::Display | IdentityField::Legal | IdentityField::Web | IdentityField::Riot
Expand Down Expand Up @@ -352,8 +348,7 @@ benchmarks! {
let info_hash = T::Hashing::hash_of(&info);
Identity::<T>::set_identity(user_origin.clone(), Box::new(info))?;

let registrar_origin =
T::RegistrarOrigin::try_successful_origin().map_err(|_| BenchmarkError::Weightless)?;
let registrar_origin = T::RegistrarOrigin::try_successful_origin().unwrap();
Identity::<T>::add_registrar(registrar_origin, caller_lookup)?;
Identity::<T>::request_judgement(user_origin, r, 10u32.into())?;
}: _(RawOrigin::Signed(caller), r, user_lookup, Judgement::Reasonable, info_hash)
Expand Down
3 changes: 1 addition & 2 deletions frame/lottery/src/benchmarking.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,8 +43,7 @@ fn setup_lottery<T: Config>(repeat: bool) -> Result<(), &'static str> {
];
// Last call will be the match for worst case scenario.
calls.push(frame_system::Call::<T>::remark { remark: vec![] }.into());
let origin =
T::ManagerOrigin::try_successful_origin().map_err(|_| BenchmarkError::Weightless)?;
let origin = T::ManagerOrigin::try_successful_origin().unwrap();
Lottery::<T>::set_calls(origin.clone(), calls)?;
Lottery::<T>::start_lottery(origin, price, length, delay, repeat)?;
Ok(())
Expand Down
26 changes: 10 additions & 16 deletions frame/membership/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -368,14 +368,9 @@ mod benchmark {

const SEED: u32 = 0;

fn set_members<T: Config<I>, I: 'static>(
members: Vec<T::AccountId>,
prime: Option<usize>,
) -> Result<(), BenchmarkError> {
let reset_origin =
T::ResetOrigin::try_successful_origin().map_err(|_| BenchmarkError::Weightless)?;
let prime_origin =
T::PrimeOrigin::try_successful_origin().map_err(|_| BenchmarkError::Weightless)?;
fn set_members<T: Config<I>, I: 'static>(members: Vec<T::AccountId>, prime: Option<usize>) {
let reset_origin = T::ResetOrigin::try_successful_origin().unwrap();
let prime_origin = T::PrimeOrigin::try_successful_origin().unwrap();

assert_ok!(<Membership<T, I>>::reset_members(reset_origin, members.clone()));
if let Some(prime) = prime.map(|i| members[i].clone()) {
Expand All @@ -384,15 +379,14 @@ mod benchmark {
} else {
assert_ok!(<Membership<T, I>>::clear_prime(prime_origin));
}
Ok(())
}

benchmarks_instance_pallet! {
add_member {
let m in 1 .. (T::MaxMembers::get() - 1);

let members = (0..m).map(|i| account("member", i, SEED)).collect::<Vec<T::AccountId>>();
set_members::<T, I>(members, None)?;
set_members::<T, I>(members, None);
let new_member = account::<T::AccountId>("add", m, SEED);
let new_member_lookup = T::Lookup::unlookup(new_member.clone());
}: {
Expand All @@ -411,7 +405,7 @@ mod benchmark {
let m in 2 .. T::MaxMembers::get();

let members = (0..m).map(|i| account("member", i, SEED)).collect::<Vec<T::AccountId>>();
set_members::<T, I>(members.clone(), Some(members.len() - 1))?;
set_members::<T, I>(members.clone(), Some(members.len() - 1));

let to_remove = members.first().cloned().unwrap();
let to_remove_lookup = T::Lookup::unlookup(to_remove.clone());
Expand All @@ -432,7 +426,7 @@ mod benchmark {
let m in 2 .. T::MaxMembers::get();

let members = (0..m).map(|i| account("member", i, SEED)).collect::<Vec<T::AccountId>>();
set_members::<T, I>(members.clone(), Some(members.len() - 1))?;
set_members::<T, I>(members.clone(), Some(members.len() - 1));
let add = account::<T::AccountId>("member", m, SEED);
let add_lookup = T::Lookup::unlookup(add.clone());
let remove = members.first().cloned().unwrap();
Expand All @@ -456,7 +450,7 @@ mod benchmark {
let m in 1 .. T::MaxMembers::get();

let members = (1..m+1).map(|i| account("member", i, SEED)).collect::<Vec<T::AccountId>>();
set_members::<T, I>(members.clone(), Some(members.len() - 1))?;
set_members::<T, I>(members.clone(), Some(members.len() - 1));
let mut new_members = (m..2*m).map(|i| account("member", i, SEED)).collect::<Vec<T::AccountId>>();
}: {
assert_ok!(<Membership<T, I>>::reset_members(
Expand All @@ -477,7 +471,7 @@ mod benchmark {
// worse case would be to change the prime
let members = (0..m).map(|i| account("member", i, SEED)).collect::<Vec<T::AccountId>>();
let prime = members.last().cloned().unwrap();
set_members::<T, I>(members.clone(), Some(members.len() - 1))?;
set_members::<T, I>(members.clone(), Some(members.len() - 1));

let add = account::<T::AccountId>("member", m, SEED);
let add_lookup = T::Lookup::unlookup(add.clone());
Expand All @@ -497,7 +491,7 @@ mod benchmark {
let members = (0..m).map(|i| account("member", i, SEED)).collect::<Vec<T::AccountId>>();
let prime = members.last().cloned().unwrap();
let prime_lookup = T::Lookup::unlookup(prime.clone());
set_members::<T, I>(members, None)?;
set_members::<T, I>(members, None);
}: {
assert_ok!(<Membership<T, I>>::set_prime(
T::PrimeOrigin::try_successful_origin().map_err(|_| BenchmarkError::Weightless)?,
Expand All @@ -513,7 +507,7 @@ mod benchmark {
let m in 1 .. T::MaxMembers::get();
let members = (0..m).map(|i| account("member", i, SEED)).collect::<Vec<T::AccountId>>();
let prime = members.last().cloned().unwrap();
set_members::<T, I>(members, None)?;
set_members::<T, I>(members, None);
}: {
assert_ok!(<Membership<T, I>>::clear_prime(
T::PrimeOrigin::try_successful_origin().map_err(|_| BenchmarkError::Weightless)?,
Expand Down
20 changes: 10 additions & 10 deletions frame/preimage/src/benchmarking.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -63,7 +63,7 @@ benchmarks! {
whitelist_account!(caller);
let (preimage, hash) = sized_preimage_and_hash::<T>(s);
assert_ok!(Preimage::<T>::request_preimage(
T::ManagerOrigin::try_successful_origin().map_err(|_| BenchmarkError::Weightless)?,
T::ManagerOrigin::try_successful_origin().unwrap(),
hash,
));
}: note_preimage(RawOrigin::Signed(caller), preimage)
Expand All @@ -75,7 +75,7 @@ benchmarks! {
let s in 0 .. MAX_SIZE;
let (preimage, hash) = sized_preimage_and_hash::<T>(s);
assert_ok!(Preimage::<T>::request_preimage(
T::ManagerOrigin::try_successful_origin().map_err(|_| BenchmarkError::Weightless)?,
T::ManagerOrigin::try_successful_origin().unwrap(),
hash,
));
}: note_preimage<T::RuntimeOrigin>(
Expand All @@ -99,7 +99,7 @@ benchmarks! {
unnote_no_deposit_preimage {
let (preimage, hash) = preimage_and_hash::<T>();
assert_ok!(Preimage::<T>::note_preimage(
T::ManagerOrigin::try_successful_origin().map_err(|_| BenchmarkError::Weightless)?,
T::ManagerOrigin::try_successful_origin().unwrap(),
preimage,
));
}: unnote_preimage<T::RuntimeOrigin>(
Expand Down Expand Up @@ -127,7 +127,7 @@ benchmarks! {
request_no_deposit_preimage {
let (preimage, hash) = preimage_and_hash::<T>();
assert_ok!(Preimage::<T>::note_preimage(
T::ManagerOrigin::try_successful_origin().map_err(|_| BenchmarkError::Weightless)?,
T::ManagerOrigin::try_successful_origin().unwrap(),
preimage,
));
}: request_preimage<T::RuntimeOrigin>(
Expand All @@ -151,7 +151,7 @@ benchmarks! {
request_requested_preimage {
let (_, hash) = preimage_and_hash::<T>();
assert_ok!(Preimage::<T>::request_preimage(
T::ManagerOrigin::try_successful_origin().map_err(|_| BenchmarkError::Weightless)?,
T::ManagerOrigin::try_successful_origin().unwrap(),
hash,
));
}: request_preimage<T::RuntimeOrigin>(
Expand All @@ -166,11 +166,11 @@ benchmarks! {
unrequest_preimage {
let (preimage, hash) = preimage_and_hash::<T>();
assert_ok!(Preimage::<T>::request_preimage(
T::ManagerOrigin::try_successful_origin().map_err(|_| BenchmarkError::Weightless)?,
T::ManagerOrigin::try_successful_origin().unwrap(),
hash,
));
assert_ok!(Preimage::<T>::note_preimage(
T::ManagerOrigin::try_successful_origin().map_err(|_| BenchmarkError::Weightless)?,
T::ManagerOrigin::try_successful_origin().unwrap(),
preimage,
));
}: _<T::RuntimeOrigin>(
Expand All @@ -183,7 +183,7 @@ benchmarks! {
unrequest_unnoted_preimage {
let (_, hash) = preimage_and_hash::<T>();
assert_ok!(Preimage::<T>::request_preimage(
T::ManagerOrigin::try_successful_origin().map_err(|_| BenchmarkError::Weightless)?,
T::ManagerOrigin::try_successful_origin().unwrap(),
hash,
));
}: unrequest_preimage<T::RuntimeOrigin>(
Expand All @@ -196,11 +196,11 @@ benchmarks! {
unrequest_multi_referenced_preimage {
let (_, hash) = preimage_and_hash::<T>();
assert_ok!(Preimage::<T>::request_preimage(
T::ManagerOrigin::try_successful_origin().map_err(|_| BenchmarkError::Weightless)?,
T::ManagerOrigin::try_successful_origin().unwrap(),
hash,
));
assert_ok!(Preimage::<T>::request_preimage(
T::ManagerOrigin::try_successful_origin().map_err(|_| BenchmarkError::Weightless)?,
T::ManagerOrigin::try_successful_origin().unwrap(),
hash,
));
}: unrequest_preimage<T::RuntimeOrigin>(
Expand Down
28 changes: 14 additions & 14 deletions frame/ranked-collective/src/benchmarking.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,20 +31,20 @@ fn assert_last_event<T: Config<I>, I: 'static>(generic_event: <T as Config<I>>::
frame_system::Pallet::<T>::assert_last_event(generic_event.into());
}

fn make_member<T: Config<I>, I: 'static>(rank: Rank) -> Result<T::AccountId, BenchmarkError> {
fn make_member<T: Config<I>, I: 'static>(rank: Rank) -> T::AccountId {
let who = account::<T::AccountId>("member", MemberCount::<T, I>::get(0), SEED);
let who_lookup = T::Lookup::unlookup(who.clone());
assert_ok!(Pallet::<T, I>::add_member(
T::PromoteOrigin::try_successful_origin().map_err(|_| BenchmarkError::Weightless)?,
T::PromoteOrigin::try_successful_origin().unwrap(),
Copy link
Member

@ggwpez ggwpez Jan 23, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So the benchmark now requires there to be a valid PromoteOrigin?
I guess for some basic origins its fine, but for the optional origins we should abort as Weighless since otherwise we would force the chain developer to specify an origin although they dont want.

Copy link
Contributor Author

@muharem muharem Jan 23, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I see the point.
Both options has pros/cons.
Aborting with Weighless here will set weight to 0 for many extrinsics (ex. remove_member) which do not require the PromoteOrigin itself. Which will be wrong, if for example a developer wants temporary disable a promotion only.
Probably a better solution will be to not make a benchmark depend on other (not its direct origin guard) origins.

Copy link
Member

@ggwpez ggwpez Jan 23, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah right, this is in a helper function. Yea just wanted to point it out in general, not a big problem.

Copy link
Member

@bkchr bkchr Jan 23, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But can we then have an expect with a proper description of the error?

Copy link
Contributor Author

@muharem muharem Jan 24, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@bkchr done! for this PR and companions

who_lookup.clone(),
));
for _ in 0..rank {
assert_ok!(Pallet::<T, I>::promote_member(
T::PromoteOrigin::try_successful_origin().map_err(|_| BenchmarkError::Weightless)?,
T::PromoteOrigin::try_successful_origin().unwrap(),
who_lookup.clone(),
));
}
Ok(who)
who
}

benchmarks_instance_pallet! {
Expand All @@ -63,10 +63,10 @@ benchmarks_instance_pallet! {
remove_member {
let r in 0 .. 10;
let rank = r as u16;
let first = make_member::<T, I>(rank)?;
let who = make_member::<T, I>(rank)?;
let first = make_member::<T, I>(rank);
let who = make_member::<T, I>(rank);
let who_lookup = T::Lookup::unlookup(who.clone());
let last = make_member::<T, I>(rank)?;
let last = make_member::<T, I>(rank);
let last_index = (0..=rank).map(|r| IdToIndex::<T, I>::get(r, &last).unwrap()).collect::<Vec<_>>();
let origin =
T::DemoteOrigin::try_successful_origin().map_err(|_| BenchmarkError::Weightless)?;
Expand All @@ -83,7 +83,7 @@ benchmarks_instance_pallet! {
promote_member {
let r in 0 .. 10;
let rank = r as u16;
let who = make_member::<T, I>(rank)?;
let who = make_member::<T, I>(rank);
let who_lookup = T::Lookup::unlookup(who.clone());
let origin =
T::PromoteOrigin::try_successful_origin().map_err(|_| BenchmarkError::Weightless)?;
Expand All @@ -97,10 +97,10 @@ benchmarks_instance_pallet! {
demote_member {
let r in 0 .. 10;
let rank = r as u16;
let first = make_member::<T, I>(rank)?;
let who = make_member::<T, I>(rank)?;
let first = make_member::<T, I>(rank);
let who = make_member::<T, I>(rank);
let who_lookup = T::Lookup::unlookup(who.clone());
let last = make_member::<T, I>(rank)?;
let last = make_member::<T, I>(rank);
let last_index = IdToIndex::<T, I>::get(rank, &last).unwrap();
let origin =
T::DemoteOrigin::try_successful_origin().map_err(|_| BenchmarkError::Weightless)?;
Expand All @@ -120,15 +120,15 @@ benchmarks_instance_pallet! {
let caller: T::AccountId = whitelisted_caller();
let caller_lookup = T::Lookup::unlookup(caller.clone());
assert_ok!(Pallet::<T, I>::add_member(
T::PromoteOrigin::try_successful_origin().map_err(|_| BenchmarkError::Weightless)?,
T::PromoteOrigin::try_successful_origin().unwrap(),
caller_lookup.clone(),
));
// Create a poll
let class = T::Polls::classes().into_iter().next().unwrap();
let rank = T::MinRankOfClass::convert(class.clone());
for _ in 0..rank {
assert_ok!(Pallet::<T, I>::promote_member(
T::PromoteOrigin::try_successful_origin().map_err(|_| BenchmarkError::Weightless)?,
T::PromoteOrigin::try_successful_origin().unwrap(),
caller_lookup.clone(),
));
}
Expand All @@ -154,7 +154,7 @@ benchmarks_instance_pallet! {

// Vote in the poll by each of `n` members
for i in 0..n {
let who = make_member::<T, I>(rank)?;
let who = make_member::<T, I>(rank);
assert_ok!(Pallet::<T, I>::vote(SystemOrigin::Signed(who).into(), poll, true));
}

Expand Down
Loading