diff --git a/prdoc/pr_3049.prdoc b/prdoc/pr_3049.prdoc new file mode 100644 index 000000000000..9cead8e2a4e5 --- /dev/null +++ b/prdoc/pr_3049.prdoc @@ -0,0 +1,11 @@ +title: "Fix treasury benchmarks when `SpendOrigin` being `None`" + +doc: + - audience: Runtime Dev + description: | + Fix treasury benchmarks when `SpendOrigin` not returning any succesful origin. + This is for example the case when `SpendOrigin` is set to `NeverOrigin`. + +crates: + - name: pallet-treasury + bump: patch diff --git a/substrate/frame/treasury/src/benchmarking.rs b/substrate/frame/treasury/src/benchmarking.rs index e63febb5798b..0bac78503f41 100644 --- a/substrate/frame/treasury/src/benchmarking.rs +++ b/substrate/frame/treasury/src/benchmarking.rs @@ -26,7 +26,7 @@ use frame_benchmarking::{ v2::*, }; use frame_support::{ - ensure, + assert_err, assert_ok, ensure, traits::{ tokens::{ConversionFromAssetBalance, PaymentStatus}, EnsureOrigin, OnInitialize, @@ -73,12 +73,19 @@ fn setup_proposal, I: 'static>( // Create proposals that are approved for use in `on_initialize`. fn create_approved_proposals, I: 'static>(n: u32) -> Result<(), &'static str> { - let origin = T::SpendOrigin::try_successful_origin().map_err(|_| BenchmarkError::Weightless)?; + let spender = T::SpendOrigin::try_successful_origin(); + for i in 0..n { let (_, value, lookup) = setup_proposal::(i); - Treasury::::spend_local(origin.clone(), value, lookup)?; + + if let Ok(origin) = &spender { + Treasury::::spend_local(origin.clone(), value, lookup)?; + } + } + + if spender.is_ok() { + ensure!(Approvals::::get().len() == n as usize, "Not all approved"); } - ensure!(Approvals::::get().len() == n as usize, "Not all approved"); Ok(()) } @@ -106,8 +113,8 @@ fn create_spend_arguments, I: 'static>( mod benchmarks { use super::*; - // This benchmark is short-circuited if `SpendOrigin` cannot provide - // a successful origin, in which case `spend` is un-callable and can use weight=0. + /// This benchmark is short-circuited if `SpendOrigin` cannot provide + /// a successful origin, in which case `spend` is un-callable and can use weight=0. #[benchmark] fn spend_local() -> Result<(), BenchmarkError> { let (_, value, beneficiary_lookup) = setup_proposal::(SEED); @@ -155,6 +162,8 @@ mod benchmarks { Ok(()) } + /// This benchmark is short-circuited if `SpendOrigin` cannot provide + /// a successful origin, in which case `spend` is un-callable and can use weight=0. #[benchmark] fn spend() -> Result<(), BenchmarkError> { let origin = @@ -190,85 +199,135 @@ mod benchmarks { #[benchmark] fn payout() -> Result<(), BenchmarkError> { - let origin = T::SpendOrigin::try_successful_origin().map_err(|_| "No origin")?; let (asset_kind, amount, beneficiary, beneficiary_lookup) = create_spend_arguments::(SEED); T::BalanceConverter::ensure_successful(asset_kind.clone()); - Treasury::::spend( - origin, - Box::new(asset_kind.clone()), - amount, - Box::new(beneficiary_lookup), - None, - )?; + + let spend_exists = if let Ok(origin) = T::SpendOrigin::try_successful_origin() { + Treasury::::spend( + origin, + Box::new(asset_kind.clone()), + amount, + Box::new(beneficiary_lookup), + None, + )?; + + true + } else { + false + }; + T::Paymaster::ensure_successful(&beneficiary, asset_kind, amount); let caller: T::AccountId = account("caller", 0, SEED); - #[extrinsic_call] - _(RawOrigin::Signed(caller.clone()), 0u32); - - let id = match Spends::::get(0).unwrap().status { - PaymentState::Attempted { id, .. } => { - assert_ne!(T::Paymaster::check_payment(id), PaymentStatus::Failure); - id - }, - _ => panic!("No payout attempt made"), - }; - assert_last_event::(Event::Paid { index: 0, payment_id: id }.into()); - assert!(Treasury::::payout(RawOrigin::Signed(caller).into(), 0u32).is_err()); + #[block] + { + let res = Treasury::::payout(RawOrigin::Signed(caller.clone()).into(), 0u32); + + if spend_exists { + assert_ok!(res); + } else { + assert_err!(res, crate::Error::::InvalidIndex); + } + } + + if spend_exists { + let id = match Spends::::get(0).unwrap().status { + PaymentState::Attempted { id, .. } => { + assert_ne!(T::Paymaster::check_payment(id), PaymentStatus::Failure); + id + }, + _ => panic!("No payout attempt made"), + }; + assert_last_event::(Event::Paid { index: 0, payment_id: id }.into()); + assert!(Treasury::::payout(RawOrigin::Signed(caller).into(), 0u32).is_err()); + } + Ok(()) } #[benchmark] fn check_status() -> Result<(), BenchmarkError> { - let origin = T::SpendOrigin::try_successful_origin().map_err(|_| "No origin")?; let (asset_kind, amount, beneficiary, beneficiary_lookup) = create_spend_arguments::(SEED); + T::BalanceConverter::ensure_successful(asset_kind.clone()); - Treasury::::spend( - origin, - Box::new(asset_kind.clone()), - amount, - Box::new(beneficiary_lookup), - None, - )?; - T::Paymaster::ensure_successful(&beneficiary, asset_kind, amount); + T::Paymaster::ensure_successful(&beneficiary, asset_kind.clone(), amount); let caller: T::AccountId = account("caller", 0, SEED); - Treasury::::payout(RawOrigin::Signed(caller.clone()).into(), 0u32)?; - match Spends::::get(0).unwrap().status { - PaymentState::Attempted { id, .. } => { - T::Paymaster::ensure_concluded(id); - }, - _ => panic!("No payout attempt made"), + + let spend_exists = if let Ok(origin) = T::SpendOrigin::try_successful_origin() { + Treasury::::spend( + origin, + Box::new(asset_kind), + amount, + Box::new(beneficiary_lookup), + None, + )?; + + Treasury::::payout(RawOrigin::Signed(caller.clone()).into(), 0u32)?; + match Spends::::get(0).unwrap().status { + PaymentState::Attempted { id, .. } => { + T::Paymaster::ensure_concluded(id); + }, + _ => panic!("No payout attempt made"), + }; + + true + } else { + false }; - #[extrinsic_call] - _(RawOrigin::Signed(caller.clone()), 0u32); + #[block] + { + let res = + Treasury::::check_status(RawOrigin::Signed(caller.clone()).into(), 0u32); + + if spend_exists { + assert_ok!(res); + } else { + assert_err!(res, crate::Error::::InvalidIndex); + } + } if let Some(s) = Spends::::get(0) { assert!(!matches!(s.status, PaymentState::Attempted { .. })); } + Ok(()) } #[benchmark] fn void_spend() -> Result<(), BenchmarkError> { - let origin = T::SpendOrigin::try_successful_origin().map_err(|_| "No origin")?; let (asset_kind, amount, _, beneficiary_lookup) = create_spend_arguments::(SEED); T::BalanceConverter::ensure_successful(asset_kind.clone()); - Treasury::::spend( - origin, - Box::new(asset_kind.clone()), - amount, - Box::new(beneficiary_lookup), - None, - )?; - assert!(Spends::::get(0).is_some()); + let spend_exists = if let Ok(origin) = T::SpendOrigin::try_successful_origin() { + Treasury::::spend( + origin, + Box::new(asset_kind.clone()), + amount, + Box::new(beneficiary_lookup), + None, + )?; + assert!(Spends::::get(0).is_some()); + + true + } else { + false + }; + let origin = T::RejectOrigin::try_successful_origin().map_err(|_| BenchmarkError::Weightless)?; - #[extrinsic_call] - _(origin as T::RuntimeOrigin, 0u32); + #[block] + { + let res = Treasury::::void_spend(origin as T::RuntimeOrigin, 0u32); + + if spend_exists { + assert_ok!(res); + } else { + assert_err!(res, crate::Error::::InvalidIndex); + } + } assert!(Spends::::get(0).is_none()); Ok(()) @@ -279,4 +338,15 @@ mod benchmarks { crate::tests::ExtBuilder::default().build(), crate::tests::Test ); + + mod no_spend_origin_tests { + use super::*; + + impl_benchmark_test_suite!( + Treasury, + crate::tests::ExtBuilder::default().spend_origin_succesful_origin_err().build(), + crate::tests::Test, + benchmarks_path = benchmarking + ); + } } diff --git a/substrate/frame/treasury/src/tests.rs b/substrate/frame/treasury/src/tests.rs index a895ea8151af..106bfb530a88 100644 --- a/substrate/frame/treasury/src/tests.rs +++ b/substrate/frame/treasury/src/tests.rs @@ -77,6 +77,9 @@ thread_local! { pub static PAID: RefCell> = RefCell::new(BTreeMap::new()); pub static STATUS: RefCell> = RefCell::new(BTreeMap::new()); pub static LAST_ID: RefCell = RefCell::new(0u64); + + #[cfg(feature = "runtime-benchmarks")] + pub static TEST_SPEND_ORIGIN_TRY_SUCCESFUL_ORIGIN_ERR: RefCell = RefCell::new(false); } /// paid balance for a given account and asset ids @@ -131,6 +134,7 @@ parameter_types! { pub TreasuryAccount: u128 = Treasury::account_id(); pub const SpendPayoutPeriod: u64 = 5; } + pub struct TestSpendOrigin; impl frame_support::traits::EnsureOrigin for TestSpendOrigin { type Success = u64; @@ -147,7 +151,11 @@ impl frame_support::traits::EnsureOrigin for TestSpendOrigin { } #[cfg(feature = "runtime-benchmarks")] fn try_successful_origin() -> Result { - Ok(RuntimeOrigin::root()) + if TEST_SPEND_ORIGIN_TRY_SUCCESFUL_ORIGIN_ERR.with(|i| *i.borrow()) { + Err(()) + } else { + Ok(frame_system::RawOrigin::Root.into()) + } } } @@ -187,11 +195,20 @@ pub struct ExtBuilder {} impl Default for ExtBuilder { fn default() -> Self { + #[cfg(feature = "runtime-benchmarks")] + TEST_SPEND_ORIGIN_TRY_SUCCESFUL_ORIGIN_ERR.with(|i| *i.borrow_mut() = false); + Self {} } } impl ExtBuilder { + #[cfg(feature = "runtime-benchmarks")] + pub fn spend_origin_succesful_origin_err(self) -> Self { + TEST_SPEND_ORIGIN_TRY_SUCCESFUL_ORIGIN_ERR.with(|i| *i.borrow_mut() = true); + self + } + pub fn build(self) -> sp_io::TestExternalities { let mut t = frame_system::GenesisConfig::::default().build_storage().unwrap(); pallet_balances::GenesisConfig:: {