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

Update benchmark's successful origin api #6598

Merged
merged 6 commits into from
Jan 29, 2023
Merged

Update benchmark's successful origin api #6598

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
a35c509
Update benchmark's successful origin api
muharem Jan 20, 2023
3ae43c0
Merge remote-tracking branch 'origin/master' into muharem-use-try-suc…
Jan 22, 2023
002b016
rustfmt
muharem Jan 22, 2023
ef72c96
unwrap for indirect origin dep
muharem Jan 23, 2023
b642aad
replace unwrap by expect with a message
muharem Jan 24, 2023
5b004d8
Merge remote-tracking branch 'origin/master' into muharem-use-try-suc…
Jan 29, 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
14 changes: 9 additions & 5 deletions runtime/common/src/auctions.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1730,7 +1730,7 @@ mod benchmarking {
use frame_system::RawOrigin;
use sp_runtime::{traits::Bounded, SaturatedConversion};

use frame_benchmarking::{account, benchmarks, whitelisted_caller};
use frame_benchmarking::{account, benchmarks, whitelisted_caller, BenchmarkError};

fn assert_last_event<T: Config>(generic_event: <T as Config>::RuntimeEvent) {
let events = frame_system::Pallet::<T>::events();
Expand Down Expand Up @@ -1786,7 +1786,8 @@ mod benchmarking {
new_auction {
let duration = T::BlockNumber::max_value();
let lease_period_index = LeasePeriodOf::<T>::max_value();
let origin = T::InitiateOrigin::successful_origin();
let origin =
T::InitiateOrigin::try_successful_origin().map_err(|_| BenchmarkError::Weightless)?;
}: _<T::RuntimeOrigin>(origin, duration, lease_period_index)
verify {
assert_last_event::<T>(Event::<T>::AuctionStarted {
Expand All @@ -1805,7 +1806,8 @@ mod benchmarking {
// Create a new auction
let duration = T::BlockNumber::max_value();
let lease_period_index = LeasePeriodOf::<T>::zero();
let origin = T::InitiateOrigin::successful_origin();
let origin = T::InitiateOrigin::try_successful_origin()
.expect("InitiateOrigin has no successful origin required for the benchmark");
Copy link
Member

Choose a reason for hiding this comment

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

Why do you sometimes expect and sometimes return an error?

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, there seem to be a lot of theses cases.

Copy link
Contributor Author

@muharem muharem Jan 27, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Here expect because InitiateOrigin is not an origin of bid extrinsic. So InitiateOrigin is a dependancy of the benchmark itself, its not an origin of the extrinsic. I do not wanna add an unexpected behaviour.
Here for example, its an origin of the extrinsic the bench written for, so its Weightless if no origin.

There are only these two cases in related PRs.

Copy link
Member

Choose a reason for hiding this comment

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

Ahh!

Copy link
Member

Choose a reason for hiding this comment

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

These benchmarks should probably be refactored in a way that an auction can be scheduled by calling some non-dispatchable function.

Copy link
Member

Choose a reason for hiding this comment

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

But yeah, not required by this pr

Auctions::<T>::new_auction(origin, duration, lease_period_index)?;

let para = ParaId::from(0);
Expand Down Expand Up @@ -1857,7 +1859,8 @@ mod benchmarking {
let duration: T::BlockNumber = lease_length / 2u32.into();
let lease_period_index = LeasePeriodOf::<T>::zero();
let now = frame_system::Pallet::<T>::block_number();
let origin = T::InitiateOrigin::successful_origin();
let origin = T::InitiateOrigin::try_successful_origin()
.expect("InitiateOrigin has no successful origin required for the benchmark");
Auctions::<T>::new_auction(origin, duration, lease_period_index)?;

fill_winners::<T>(lease_period_index);
Expand Down Expand Up @@ -1901,7 +1904,8 @@ mod benchmarking {
let duration: T::BlockNumber = lease_length / 2u32.into();
let lease_period_index = LeasePeriodOf::<T>::zero();
let now = frame_system::Pallet::<T>::block_number();
let origin = T::InitiateOrigin::successful_origin();
let origin = T::InitiateOrigin::try_successful_origin()
.expect("InitiateOrigin has no successful origin required for the benchmark");
Auctions::<T>::new_auction(origin, duration, lease_period_index)?;

fill_winners::<T>(lease_period_index);
Expand Down
14 changes: 9 additions & 5 deletions runtime/common/src/slots/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -987,7 +987,7 @@ mod benchmarking {
use frame_system::RawOrigin;
use sp_runtime::traits::{Bounded, One};

use frame_benchmarking::{account, benchmarks, whitelisted_caller};
use frame_benchmarking::{account, benchmarks, whitelisted_caller, BenchmarkError};

use crate::slots::Pallet as Slots;

Expand Down Expand Up @@ -1027,7 +1027,8 @@ mod benchmarking {
let amount = T::Currency::minimum_balance();
let period_begin = 69u32.into();
let period_count = 3u32.into();
let origin = T::ForceOrigin::successful_origin();
let origin =
T::ForceOrigin::try_successful_origin().map_err(|_| BenchmarkError::Weightless)?;
}: _<T::RuntimeOrigin>(origin, para, leaser.clone(), amount, period_begin, period_count)
verify {
assert_last_event::<T>(Event::<T>::Leased {
Expand Down Expand Up @@ -1061,7 +1062,8 @@ mod benchmarking {
// T parathread are upgrading to parachains
for (para, leaser) in paras_info {
let amount = T::Currency::minimum_balance();
let origin = T::ForceOrigin::successful_origin();
let origin = T::ForceOrigin::try_successful_origin()
.expect("ForceOrigin has no successful origin required for the benchmark");
Slots::<T>::force_lease(origin, para, leaser, amount, period_begin, period_count)?;
}

Expand Down Expand Up @@ -1112,7 +1114,8 @@ mod benchmarking {
// Average slot has 4 lease periods.
let period_count: LeasePeriodOf<T> = 4u32.into();
let period_begin = period_count * i.into();
let origin = T::ForceOrigin::successful_origin();
let origin = T::ForceOrigin::try_successful_origin()
.expect("ForceOrigin has no successful origin required for the benchmark");
Slots::<T>::force_lease(origin, para, leaser, amount, period_begin, period_count)?;
}

Expand All @@ -1121,7 +1124,8 @@ mod benchmarking {
assert_eq!(T::Currency::reserved_balance(&leaser), T::Currency::minimum_balance());
}

let origin = T::ForceOrigin::successful_origin();
let origin =
T::ForceOrigin::try_successful_origin().map_err(|_| BenchmarkError::Weightless)?;
}: _<T::RuntimeOrigin>(origin, para)
verify {
for i in 0 .. max_people {
Expand Down
12 changes: 8 additions & 4 deletions xcm/pallet-xcm/src/benchmarking.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,8 @@ type RuntimeOrigin<T> = <T as frame_system::Config>::RuntimeOrigin;

benchmarks! {
send {
let send_origin = T::SendXcmOrigin::successful_origin();
let send_origin =
T::SendXcmOrigin::try_successful_origin().map_err(|_| BenchmarkError::Weightless)?;
if T::SendXcmOrigin::try_origin(send_origin.clone()).is_err() {
return Err(BenchmarkError::Override(BenchmarkResult::from_weight(Weight::MAX)))
}
Expand All @@ -40,7 +41,8 @@ benchmarks! {

teleport_assets {
let asset: MultiAsset = (Here, 10).into();
let send_origin = T::ExecuteXcmOrigin::successful_origin();
let send_origin =
T::ExecuteXcmOrigin::try_successful_origin().map_err(|_| BenchmarkError::Weightless)?;
let origin_location = T::ExecuteXcmOrigin::try_origin(send_origin.clone())
.map_err(|_| BenchmarkError::Override(BenchmarkResult::from_weight(Weight::MAX)))?;
if !T::XcmTeleportFilter::contains(&(origin_location, vec![asset.clone()])) {
Expand All @@ -59,7 +61,8 @@ benchmarks! {

reserve_transfer_assets {
let asset: MultiAsset = (Here, 10).into();
let send_origin = T::ExecuteXcmOrigin::successful_origin();
let send_origin =
T::ExecuteXcmOrigin::try_successful_origin().map_err(|_| BenchmarkError::Weightless)?;
let origin_location = T::ExecuteXcmOrigin::try_origin(send_origin.clone())
.map_err(|_| BenchmarkError::Override(BenchmarkResult::from_weight(Weight::MAX)))?;
if !T::XcmReserveTransferFilter::contains(&(origin_location, vec![asset.clone()])) {
Expand All @@ -77,7 +80,8 @@ benchmarks! {
}: _<RuntimeOrigin<T>>(send_origin, Box::new(versioned_dest), Box::new(versioned_beneficiary), Box::new(versioned_assets), 0)

execute {
let execute_origin = T::ExecuteXcmOrigin::successful_origin();
let execute_origin =
T::ExecuteXcmOrigin::try_successful_origin().map_err(|_| BenchmarkError::Weightless)?;
let origin_location = T::ExecuteXcmOrigin::try_origin(execute_origin.clone())
.map_err(|_| BenchmarkError::Override(BenchmarkResult::from_weight(Weight::MAX)))?;
let msg = Xcm(vec![ClearOrigin]);
Expand Down