From ad1876bf94840c4470037b1b67fc72ce2254c9f1 Mon Sep 17 00:00:00 2001 From: Shawn Tabrizi Date: Mon, 23 Mar 2020 13:19:15 +0100 Subject: [PATCH] Switch Treasury to `on_initialize`, Tips `KeepAlive` (#5352) * Switch Treasury to `on_initialize` * Fix bench compile --- substrate/frame/treasury/src/benchmarking.rs | 8 +++---- substrate/frame/treasury/src/lib.rs | 12 +++++----- substrate/frame/treasury/src/tests.rs | 24 +++++++++++--------- 3 files changed, 23 insertions(+), 21 deletions(-) diff --git a/substrate/frame/treasury/src/benchmarking.rs b/substrate/frame/treasury/src/benchmarking.rs index 08318bdd6c..fb16af740a 100644 --- a/substrate/frame/treasury/src/benchmarking.rs +++ b/substrate/frame/treasury/src/benchmarking.rs @@ -20,7 +20,7 @@ use super::*; use frame_system::RawOrigin; use frame_benchmarking::{benchmarks, account}; -use sp_runtime::traits::OnFinalize; +use sp_runtime::traits::OnInitialize; use crate::Module as Treasury; @@ -82,7 +82,7 @@ fn create_tips(t: u32, hash: T::Hash, value: BalanceOf) -> Result<( Ok(()) } -// Create proposals that are approved for use in `on_finalize`. +// Create proposals that are approved for use in `on_initialize`. fn create_approved_proposals(n: u32) -> Result<(), &'static str> { for i in 0 .. n { let (caller, value, lookup) = setup_proposal::(i); @@ -199,13 +199,13 @@ benchmarks! { let caller = account("caller", t, SEED); }: _(RawOrigin::Signed(caller), hash) - on_finalize { + on_initialize { let p in 0 .. 100; let pot_account = Treasury::::account_id(); let value = T::Currency::minimum_balance().saturating_mul(1_000_000_000.into()); let _ = T::Currency::make_free_balance_be(&pot_account, value); create_approved_proposals::(p)?; }: { - Treasury::::on_finalize(T::BlockNumber::zero()); + Treasury::::on_initialize(T::BlockNumber::zero()); } } diff --git a/substrate/frame/treasury/src/lib.rs b/substrate/frame/treasury/src/lib.rs index d7562d0767..2c808e0659 100644 --- a/substrate/frame/treasury/src/lib.rs +++ b/substrate/frame/treasury/src/lib.rs @@ -92,7 +92,7 @@ use serde::{Serialize, Deserialize}; use sp_std::prelude::*; use frame_support::{decl_module, decl_storage, decl_event, ensure, print, decl_error, Parameter}; use frame_support::traits::{ - Currency, ExistenceRequirement, Get, Imbalance, OnUnbalanced, ExistenceRequirement::AllowDeath, + Currency, Get, Imbalance, OnUnbalanced, ExistenceRequirement::KeepAlive, ReservableCurrency, WithdrawReason }; use sp_runtime::{Permill, ModuleId, Percent, RuntimeDebug, traits::{ @@ -553,7 +553,7 @@ decl_module! { Self::payout_tip(tip); } - fn on_finalize(n: T::BlockNumber) { + fn on_initialize(n: T::BlockNumber) { // Check to see if we should spend some funds! if (n % T::SpendPeriod::get()).is_zero() { Self::spend_funds(); @@ -631,7 +631,7 @@ impl Module { Self::retain_active_tips(&mut tips); tips.sort_by_key(|i| i.1); let treasury = Self::account_id(); - let max_payout = T::Currency::free_balance(&treasury); + let max_payout = Self::pot(); let mut payout = tips[tips.len() / 2].1.min(max_payout); if let Some((finder, deposit)) = tip.finder { let _ = T::Currency::unreserve(&finder, deposit); @@ -641,11 +641,11 @@ impl Module { payout -= finders_fee; // this should go through given we checked it's at most the free balance, but still // we only make a best-effort. - let _ = T::Currency::transfer(&treasury, &finder, finders_fee, AllowDeath); + let _ = T::Currency::transfer(&treasury, &finder, finders_fee, KeepAlive); } } // same as above: best-effort only. - let _ = T::Currency::transfer(&treasury, &tip.who, payout, AllowDeath); + let _ = T::Currency::transfer(&treasury, &tip.who, payout, KeepAlive); } // Spend some money! @@ -697,7 +697,7 @@ impl Module { &Self::account_id(), imbalance, WithdrawReason::Transfer.into(), - ExistenceRequirement::KeepAlive + KeepAlive ) { print("Inconsistent state - couldn't settle imbalance for funds spent by treasury"); // Nothing else to do here. diff --git a/substrate/frame/treasury/src/tests.rs b/substrate/frame/treasury/src/tests.rs index a5acb2efe5..d7e710639d 100644 --- a/substrate/frame/treasury/src/tests.rs +++ b/substrate/frame/treasury/src/tests.rs @@ -6,7 +6,7 @@ use sp_core::H256; use sp_runtime::{ Perbill, testing::Header, - traits::{BlakeTwo256, OnFinalize, IdentityLookup, BadOrigin}, + traits::{BlakeTwo256, OnInitialize, IdentityLookup, BadOrigin}, }; impl_outer_origin! { @@ -60,6 +60,8 @@ impl Contains for TenToFourteen { fn sorted_members() -> Vec { vec![10, 11, 12, 13, 14] } + #[cfg(feature = "runtime-benchmarks")] + fn add(_: &u64) { unimplemented!() } } parameter_types! { pub const ProposalBond: Permill = Permill::from_percent(5); @@ -287,7 +289,7 @@ fn accepted_spend_proposal_ignored_outside_spend_period() { assert_ok!(Treasury::propose_spend(Origin::signed(0), 100, 3)); assert_ok!(Treasury::approve_proposal(Origin::ROOT, 0)); - >::on_finalize(1); + >::on_initialize(1); assert_eq!(Balances::free_balance(3), 0); assert_eq!(Treasury::pot(), 100); }); @@ -300,7 +302,7 @@ fn unused_pot_should_diminish() { Balances::make_free_balance_be(&Treasury::account_id(), 101); assert_eq!(Balances::total_issuance(), init_total_issuance + 100); - >::on_finalize(2); + >::on_initialize(2); assert_eq!(Treasury::pot(), 50); assert_eq!(Balances::total_issuance(), init_total_issuance + 50); }); @@ -314,7 +316,7 @@ fn rejected_spend_proposal_ignored_on_spend_period() { assert_ok!(Treasury::propose_spend(Origin::signed(0), 100, 3)); assert_ok!(Treasury::reject_proposal(Origin::ROOT, 0)); - >::on_finalize(2); + >::on_initialize(2); assert_eq!(Balances::free_balance(3), 0); assert_eq!(Treasury::pot(), 50); }); @@ -365,7 +367,7 @@ fn accepted_spend_proposal_enacted_on_spend_period() { assert_ok!(Treasury::propose_spend(Origin::signed(0), 100, 3)); assert_ok!(Treasury::approve_proposal(Origin::ROOT, 0)); - >::on_finalize(2); + >::on_initialize(2); assert_eq!(Balances::free_balance(3), 100); assert_eq!(Treasury::pot(), 0); }); @@ -380,11 +382,11 @@ fn pot_underflow_should_not_diminish() { assert_ok!(Treasury::propose_spend(Origin::signed(0), 150, 3)); assert_ok!(Treasury::approve_proposal(Origin::ROOT, 0)); - >::on_finalize(2); + >::on_initialize(2); assert_eq!(Treasury::pot(), 100); // Pot hasn't changed let _ = Balances::deposit_into_existing(&Treasury::account_id(), 100).unwrap(); - >::on_finalize(4); + >::on_initialize(4); assert_eq!(Balances::free_balance(3), 150); // Fund has been spent assert_eq!(Treasury::pot(), 25); // Pot has finally changed }); @@ -402,13 +404,13 @@ fn treasury_account_doesnt_get_deleted() { assert_ok!(Treasury::propose_spend(Origin::signed(0), treasury_balance, 3)); assert_ok!(Treasury::approve_proposal(Origin::ROOT, 0)); - >::on_finalize(2); + >::on_initialize(2); assert_eq!(Treasury::pot(), 100); // Pot hasn't changed assert_ok!(Treasury::propose_spend(Origin::signed(0), Treasury::pot(), 3)); assert_ok!(Treasury::approve_proposal(Origin::ROOT, 1)); - >::on_finalize(4); + >::on_initialize(4); assert_eq!(Treasury::pot(), 0); // Pot is emptied assert_eq!(Balances::free_balance(Treasury::account_id()), 1); // but the account is still there }); @@ -433,7 +435,7 @@ fn inexistent_account_works() { assert_ok!(Treasury::approve_proposal(Origin::ROOT, 0)); assert_ok!(Treasury::propose_spend(Origin::signed(0), 1, 3)); assert_ok!(Treasury::approve_proposal(Origin::ROOT, 1)); - >::on_finalize(2); + >::on_initialize(2); assert_eq!(Treasury::pot(), 0); // Pot hasn't changed assert_eq!(Balances::free_balance(3), 0); // Balance of `3` hasn't changed @@ -441,7 +443,7 @@ fn inexistent_account_works() { assert_eq!(Treasury::pot(), 99); // Pot now contains funds assert_eq!(Balances::free_balance(Treasury::account_id()), 100); // Account does exist - >::on_finalize(4); + >::on_initialize(4); assert_eq!(Treasury::pot(), 0); // Pot has changed assert_eq!(Balances::free_balance(3), 99); // Balance of `3` has changed