diff --git a/prdoc/pr_1871.prdoc b/prdoc/pr_1871.prdoc new file mode 100644 index 000000000000..d1850509afbb --- /dev/null +++ b/prdoc/pr_1871.prdoc @@ -0,0 +1,12 @@ +title: Adding `try-state` hook to tips pallet + +doc: + - audience: Runtime User + description: | + Enforces the following invariants; + 1. The number of entries in Tips should be equal to Reasons. + 2. If OpenTip.finders_fee is true, then OpenTip.deposit should be greater than zero. + 3. Reasons exists for each Tip[OpenTip.reason], implying equal length of storage. + +crates: +- name: pallet-tips diff --git a/substrate/frame/tips/src/lib.rs b/substrate/frame/tips/src/lib.rs index 8764486d5f4f..4c7cfc3028a9 100644 --- a/substrate/frame/tips/src/lib.rs +++ b/substrate/frame/tips/src/lib.rs @@ -68,6 +68,7 @@ use sp_std::prelude::*; use codec::{Decode, Encode}; use frame_support::{ + ensure, traits::{ ContainsLengthBound, Currency, EnsureOrigin, ExistenceRequirement::KeepAlive, Get, OnUnbalanced, ReservableCurrency, SortedMembers, @@ -76,6 +77,9 @@ use frame_support::{ }; use frame_system::pallet_prelude::BlockNumberFor; +#[cfg(any(feature = "try-runtime", test))] +use sp_runtime::TryRuntimeError; + pub use pallet::*; pub use weights::WeightInfo; @@ -150,7 +154,7 @@ pub mod pallet { #[pallet::constant] type TipFindersFee: Get; - /// The amount held on deposit for placing a tip report. + /// The non-zero amount held on deposit for placing a tip report. #[pallet::constant] type TipReportDepositBase: Get>; @@ -465,6 +469,21 @@ pub mod pallet { Ok(()) } } + + #[pallet::hooks] + impl, I: 'static> Hooks> for Pallet { + fn integrity_test() { + assert!( + !T::TipReportDepositBase::get().is_zero(), + "`TipReportDepositBase` should not be zero", + ); + } + + #[cfg(feature = "try-runtime")] + fn try_state(_n: BlockNumberFor) -> Result<(), TryRuntimeError> { + Self::do_try_state() + } + } } impl, I: 'static> Pallet { @@ -611,4 +630,42 @@ impl, I: 'static> Pallet { Tips::::insert(hash, new_tip) } } + + /// Ensure the correctness of the state of this pallet. + /// + /// This should be valid before and after each state transition of this pallet. + /// + /// ## Invariants: + /// 1. The number of entries in `Tips` should be equal to `Reasons`. + /// 2. Reasons exists for each Tip[`OpenTip.reason`]. + /// 3. If `OpenTip.finders_fee` is true, then OpenTip.deposit should be greater than zero. + #[cfg(any(feature = "try-runtime", test))] + pub fn do_try_state() -> Result<(), TryRuntimeError> { + let reasons = Reasons::::iter_keys().collect::>(); + let tips = Tips::::iter_keys().collect::>(); + + ensure!( + reasons.len() == tips.len(), + TryRuntimeError::Other("Equal length of entries in `Tips` and `Reasons` Storage") + ); + + for tip in Tips::::iter_keys() { + let open_tip = Tips::::get(&tip).expect("All map keys are valid; qed"); + + if open_tip.finders_fee { + ensure!( + !open_tip.deposit.is_zero(), + TryRuntimeError::Other( + "Tips with `finders_fee` should have non-zero `deposit`." + ) + ) + } + + ensure!( + reasons.contains(&open_tip.reason), + TryRuntimeError::Other("no reason for this tip") + ); + } + Ok(()) + } } diff --git a/substrate/frame/tips/src/tests.rs b/substrate/frame/tips/src/tests.rs index d5c8e17a3969..63ac8e37cddd 100644 --- a/substrate/frame/tips/src/tests.rs +++ b/substrate/frame/tips/src/tests.rs @@ -31,7 +31,7 @@ use frame_support::{ storage::StoragePrefixedMap, traits::{ tokens::{PayFromAccount, UnityAssetBalanceConversion}, - ConstU32, ConstU64, SortedMembers, StorageVersion, + ConstU32, ConstU64, IntegrityTest, SortedMembers, StorageVersion, }, PalletId, }; @@ -189,13 +189,14 @@ impl pallet_treasury::Config for Test { parameter_types! { pub const TipFindersFee: Percent = Percent::from_percent(20); + pub static TipReportDepositBase: u64 = 1; } impl Config for Test { type MaximumReasonLength = ConstU32<16384>; type Tippers = TenToFourteen; type TipCountdown = ConstU64<1>; type TipFindersFee = TipFindersFee; - type TipReportDepositBase = ConstU64<1>; + type TipReportDepositBase = TipReportDepositBase; type DataDepositPerByte = ConstU64<1>; type MaxTipAmount = ConstU64<10_000_000>; type RuntimeEvent = RuntimeEvent; @@ -207,7 +208,7 @@ impl Config for Test { type Tippers = TenToFourteen; type TipCountdown = ConstU64<1>; type TipFindersFee = TipFindersFee; - type TipReportDepositBase = ConstU64<1>; + type TipReportDepositBase = TipReportDepositBase; type DataDepositPerByte = ConstU64<1>; type MaxTipAmount = ConstU64<10_000_000>; type RuntimeEvent = RuntimeEvent; @@ -228,6 +229,14 @@ pub fn new_test_ext() -> sp_io::TestExternalities { ext } +/// Run the function pointer inside externalities and asserts the try_state hook at the end. +pub fn build_and_execute(test: impl FnOnce() -> ()) { + new_test_ext().execute_with(|| { + test(); + Tips::do_try_state().expect("All invariants must hold after a test"); + }); +} + fn last_event() -> TipEvent { System::events() .into_iter() @@ -239,7 +248,7 @@ fn last_event() -> TipEvent { #[test] fn genesis_config_works() { - new_test_ext().execute_with(|| { + build_and_execute(|| { assert_eq!(Treasury::pot(), 0); assert_eq!(Treasury::proposal_count(), 0); }); @@ -251,7 +260,7 @@ fn tip_hash() -> H256 { #[test] fn tip_new_cannot_be_used_twice() { - new_test_ext().execute_with(|| { + build_and_execute(|| { Balances::make_free_balance_be(&Treasury::account_id(), 101); assert_ok!(Tips::tip_new(RuntimeOrigin::signed(10), b"awesome.dot".to_vec(), 3, 10)); assert_noop!( @@ -263,7 +272,7 @@ fn tip_new_cannot_be_used_twice() { #[test] fn report_awesome_and_tip_works() { - new_test_ext().execute_with(|| { + build_and_execute(|| { Balances::make_free_balance_be(&Treasury::account_id(), 101); assert_ok!(Tips::report_awesome(RuntimeOrigin::signed(0), b"awesome.dot".to_vec(), 3)); assert_eq!(Balances::reserved_balance(0), 12); @@ -290,7 +299,7 @@ fn report_awesome_and_tip_works() { #[test] fn report_awesome_from_beneficiary_and_tip_works() { - new_test_ext().execute_with(|| { + build_and_execute(|| { Balances::make_free_balance_be(&Treasury::account_id(), 101); assert_ok!(Tips::report_awesome(RuntimeOrigin::signed(0), b"awesome.dot".to_vec(), 0)); assert_eq!(Balances::reserved_balance(0), 12); @@ -308,7 +317,7 @@ fn report_awesome_from_beneficiary_and_tip_works() { #[test] fn close_tip_works() { - new_test_ext().execute_with(|| { + build_and_execute(|| { System::set_block_number(1); Balances::make_free_balance_be(&Treasury::account_id(), 101); @@ -346,7 +355,7 @@ fn close_tip_works() { #[test] fn slash_tip_works() { - new_test_ext().execute_with(|| { + build_and_execute(|| { System::set_block_number(1); Balances::make_free_balance_be(&Treasury::account_id(), 101); assert_eq!(Treasury::pot(), 100); @@ -377,7 +386,7 @@ fn slash_tip_works() { #[test] fn retract_tip_works() { - new_test_ext().execute_with(|| { + build_and_execute(|| { // with report awesome Balances::make_free_balance_be(&Treasury::account_id(), 101); assert_ok!(Tips::report_awesome(RuntimeOrigin::signed(0), b"awesome.dot".to_vec(), 3)); @@ -411,7 +420,7 @@ fn retract_tip_works() { #[test] fn tip_median_calculation_works() { - new_test_ext().execute_with(|| { + build_and_execute(|| { Balances::make_free_balance_be(&Treasury::account_id(), 101); assert_ok!(Tips::tip_new(RuntimeOrigin::signed(10), b"awesome.dot".to_vec(), 3, 0)); let h = tip_hash(); @@ -425,7 +434,7 @@ fn tip_median_calculation_works() { #[test] fn tip_large_should_fail() { - new_test_ext().execute_with(|| { + build_and_execute(|| { Balances::make_free_balance_be(&Treasury::account_id(), 101); assert_ok!(Tips::tip_new(RuntimeOrigin::signed(10), b"awesome.dot".to_vec(), 3, 0)); let h = tip_hash(); @@ -442,7 +451,7 @@ fn tip_large_should_fail() { #[test] fn tip_changing_works() { - new_test_ext().execute_with(|| { + build_and_execute(|| { Balances::make_free_balance_be(&Treasury::account_id(), 101); assert_ok!(Tips::tip_new(RuntimeOrigin::signed(10), b"awesome.dot".to_vec(), 3, 10000)); let h = tip_hash(); @@ -627,7 +636,7 @@ fn genesis_funding_works() { #[test] fn report_awesome_and_tip_works_second_instance() { - new_test_ext().execute_with(|| { + build_and_execute(|| { Balances::make_free_balance_be(&Treasury::account_id(), 101); Balances::make_free_balance_be(&Treasury1::account_id(), 201); assert_eq!(Balances::free_balance(&Treasury::account_id()), 101); @@ -657,3 +666,88 @@ fn report_awesome_and_tip_works_second_instance() { assert_eq!(Balances::free_balance(&Treasury1::account_id()), 191); }); } + +#[test] +fn equal_entries_invariant() { + new_test_ext().execute_with(|| { + use frame_support::pallet_prelude::DispatchError::Other; + + Balances::make_free_balance_be(&Treasury::account_id(), 101); + + assert_ok!(Tips::report_awesome(RuntimeOrigin::signed(0), b"awesome.dot".to_vec(), 3)); + + let reason1 = BlakeTwo256::hash(b"reason1"); + let hash1 = BlakeTwo256::hash_of(&(reason1, 10u64)); + + let tip = OpenTip:: { + reason: reason1, + who: 10, + finder: 20, + deposit: 30, + closes: Some(13), + tips: vec![(40, 50), (60, 70)], + finders_fee: true, + }; + + // Breaks invariant by adding an entry to only `Tips` Storage. + pallet_tips::Tips::::insert(hash1, tip); + + // Invariant violated + assert_eq!( + Tips::do_try_state(), + Err(Other("Equal length of entries in `Tips` and `Reasons` Storage")) + ); + }) +} + +#[test] +fn finders_fee_invariant() { + new_test_ext().execute_with(|| { + use frame_support::pallet_prelude::DispatchError::Other; + + // Breaks invariant by having a zero deposit. + TipReportDepositBase::set(0); + + Balances::make_free_balance_be(&Treasury::account_id(), 101); + + assert_ok!(Tips::report_awesome(RuntimeOrigin::signed(0), b"".to_vec(), 3)); + + // Invariant violated + assert_eq!( + Tips::do_try_state(), + Err(Other("Tips with `finders_fee` should have non-zero `deposit`.")) + ); + }) +} + +#[test] +fn reasons_invariant() { + new_test_ext().execute_with(|| { + use frame_support::pallet_prelude::DispatchError::Other; + + Balances::make_free_balance_be(&Treasury::account_id(), 101); + + assert_ok!(Tips::report_awesome(RuntimeOrigin::signed(0), b"awesome.dot".to_vec(), 0)); + + let hash: Vec<_> = pallet_tips::Tips::::iter_keys().collect(); + + let mut open_tip = pallet_tips::Tips::::take(hash[0]).unwrap(); + + // Breaks invariant by changing value `open_tip.reason` in `Tips` Storage. + open_tip.reason = ::Hashing::hash(&b"".to_vec()); + + pallet_tips::Tips::::insert(hash[0], open_tip); + + // Invariant violated + assert_eq!(Tips::do_try_state(), Err(Other("no reason for this tip"))); + }) +} + +#[test] +#[should_panic = "`TipReportDepositBase` should not be zero"] +fn zero_base_deposit_prohibited() { + new_test_ext().execute_with(|| { + TipReportDepositBase::set(0); + Tips::integrity_test(); + }); +}