From 088d977e6195bbec37e5eb90ee6a7426dd552d22 Mon Sep 17 00:00:00 2001 From: doordashcon Date: Sat, 14 Oct 2023 03:36:28 +0100 Subject: [PATCH 01/10] finder_fee_invariant --- substrate/frame/tips/src/lib.rs | 33 +++++++++++++++++++++++++++++++ substrate/frame/tips/src/tests.rs | 30 +++++++++++++++++++--------- 2 files changed, 54 insertions(+), 9 deletions(-) diff --git a/substrate/frame/tips/src/lib.rs b/substrate/frame/tips/src/lib.rs index 8764486d5f4f..5deacea8d824 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; @@ -465,6 +469,14 @@ pub mod pallet { Ok(()) } } + + #[pallet::hooks] + impl, I: 'static> Hooks> for Pallet { + #[cfg(feature = "try-runtime")] + fn try_state(_n: BlockNumberFor) -> Result<(), TryRuntimeError> { + Self::do_try_state() + } + } } impl, I: 'static> Pallet { @@ -611,4 +623,25 @@ impl, I: 'static> Pallet { Tips::::insert(hash, new_tip) } } + + /// Ensure the correctness of the state of this pallet. + /// + /// This should be valid before or after each state transition of this pallet. + /// + /// ## Invariants: + /// + /// If `OpenTip.finders_fee` is true, then OpenTip.deposit should be greater that zero. + #[cfg(any(feature = "try-runtime", test))] + pub fn do_try_state() -> Result<(), TryRuntimeError> { + for tips in Tips::::iter_keys() { + let open_tip = Tips::::get(&tips).unwrap(); + if open_tip.finders_fee { + ensure!( + !open_tip.deposit.is_zero(), + TryRuntimeError::Other("finder's fee not present") + ) + } + } + Ok(()) + } } diff --git a/substrate/frame/tips/src/tests.rs b/substrate/frame/tips/src/tests.rs index 8fe111afc26a..c6d651c7f9d7 100644 --- a/substrate/frame/tips/src/tests.rs +++ b/substrate/frame/tips/src/tests.rs @@ -192,7 +192,7 @@ impl Config for Test { type Tippers = TenToFourteen; type TipCountdown = ConstU64<1>; type TipFindersFee = TipFindersFee; - type TipReportDepositBase = ConstU64<1>; + type TipReportDepositBase = ConstU64<0>; type DataDepositPerByte = ConstU64<1>; type MaxTipAmount = ConstU64<10_000_000>; type RuntimeEvent = RuntimeEvent; @@ -263,8 +263,8 @@ fn report_awesome_and_tip_works() { new_test_ext().execute_with(|| { 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); - assert_eq!(Balances::free_balance(0), 88); + assert_eq!(Balances::reserved_balance(0), 11); + assert_eq!(Balances::free_balance(0), 89); // other reports don't count. assert_noop!( @@ -290,8 +290,8 @@ fn report_awesome_from_beneficiary_and_tip_works() { new_test_ext().execute_with(|| { 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); - assert_eq!(Balances::free_balance(0), 88); + assert_eq!(Balances::reserved_balance(0), 11); + assert_eq!(Balances::free_balance(0), 89); let h = BlakeTwo256::hash_of(&(BlakeTwo256::hash(b"awesome.dot"), 0u128)); assert_ok!(Tips::tip(RuntimeOrigin::signed(10), h, 10)); assert_ok!(Tips::tip(RuntimeOrigin::signed(11), h, 10)); @@ -353,8 +353,8 @@ fn slash_tip_works() { assert_ok!(Tips::report_awesome(RuntimeOrigin::signed(0), b"awesome.dot".to_vec(), 3)); - assert_eq!(Balances::reserved_balance(0), 12); - assert_eq!(Balances::free_balance(0), 88); + assert_eq!(Balances::reserved_balance(0), 11); + assert_eq!(Balances::free_balance(0), 89); let h = tip_hash(); assert_eq!(last_event(), TipEvent::NewTip { tip_hash: h }); @@ -364,11 +364,11 @@ fn slash_tip_works() { // can remove from root. assert_ok!(Tips::slash_tip(RuntimeOrigin::root(), h)); - assert_eq!(last_event(), TipEvent::TipSlashed { tip_hash: h, finder: 0, deposit: 12 }); + assert_eq!(last_event(), TipEvent::TipSlashed { tip_hash: h, finder: 0, deposit: 11 }); // tipper slashed assert_eq!(Balances::reserved_balance(0), 0); - assert_eq!(Balances::free_balance(0), 88); + assert_eq!(Balances::free_balance(0), 89); }); } @@ -654,3 +654,15 @@ fn report_awesome_and_tip_works_second_instance() { assert_eq!(Balances::free_balance(&Treasury1::account_id()), 191); }); } + +#[test] +fn try_state_invariant_works() { + 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"".to_vec(), 3)); + + assert_eq!(Tips::do_try_state(), Err(Other("finder's fee not present"))) + }) +} From cf20cec00a13146359f4e99c2387738c40d556b8 Mon Sep 17 00:00:00 2001 From: doordashcon Date: Fri, 20 Oct 2023 23:19:48 +0100 Subject: [PATCH 02/10] 2 more invariants + integrity test --- substrate/frame/tips/src/lib.rs | 40 +++++++++---- substrate/frame/tips/src/tests.rs | 99 +++++++++++++++++++++++++++---- 2 files changed, 117 insertions(+), 22 deletions(-) diff --git a/substrate/frame/tips/src/lib.rs b/substrate/frame/tips/src/lib.rs index 5deacea8d824..705adb64cca4 100644 --- a/substrate/frame/tips/src/lib.rs +++ b/substrate/frame/tips/src/lib.rs @@ -264,8 +264,8 @@ pub mod pallet { let hash = T::Hashing::hash_of(&(&reason_hash, &who)); ensure!(!Tips::::contains_key(&hash), Error::::AlreadyKnown); - let deposit = T::TipReportDepositBase::get() + - T::DataDepositPerByte::get() * (reason.len() as u32).into(); + let deposit = T::TipReportDepositBase::get() + + T::DataDepositPerByte::get() * (reason.len() as u32).into(); T::Currency::reserve(&finder, deposit)?; Reasons::::insert(&reason_hash, &reason); @@ -472,6 +472,13 @@ pub mod pallet { #[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() @@ -525,9 +532,9 @@ impl, I: 'static> Pallet { Some(m) => { member = members_iter.next(); if m < a { - continue + continue; } else { - break true + break true; } }, } @@ -626,21 +633,34 @@ impl, I: 'static> Pallet { /// Ensure the correctness of the state of this pallet. /// - /// This should be valid before or after each state transition of this pallet. + /// This should be valid before and after each state transition of this pallet. /// /// ## Invariants: - /// - /// If `OpenTip.finders_fee` is true, then OpenTip.deposit should be greater that zero. + /// 1. Reasons exists for each Tip[`OpenTip.reason`], this implies equal lenth of entries in storage. + /// 2. If `OpenTip.finders_fee` is true, then OpenTip.deposit should be greater that zero. #[cfg(any(feature = "try-runtime", test))] pub fn do_try_state() -> Result<(), TryRuntimeError> { - for tips in Tips::::iter_keys() { - let open_tip = Tips::::get(&tips).unwrap(); + 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("finder's fee not present") + 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 c6d651c7f9d7..f9f56a02b0fc 100644 --- a/substrate/frame/tips/src/tests.rs +++ b/substrate/frame/tips/src/tests.rs @@ -29,6 +29,7 @@ use sp_storage::Storage; use frame_support::{ assert_noop, assert_ok, parameter_types, storage::StoragePrefixedMap, + traits::IntegrityTest, traits::{ tokens::{PayFromAccount, UnityAssetBalanceConversion}, ConstU32, ConstU64, SortedMembers, StorageVersion, @@ -186,13 +187,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<0>; + type TipReportDepositBase = TipReportDepositBase; type DataDepositPerByte = ConstU64<1>; type MaxTipAmount = ConstU64<10_000_000>; type RuntimeEvent = RuntimeEvent; @@ -204,7 +206,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; @@ -263,8 +265,8 @@ fn report_awesome_and_tip_works() { new_test_ext().execute_with(|| { 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), 11); - assert_eq!(Balances::free_balance(0), 89); + assert_eq!(Balances::reserved_balance(0), 12); + assert_eq!(Balances::free_balance(0), 88); // other reports don't count. assert_noop!( @@ -290,8 +292,8 @@ fn report_awesome_from_beneficiary_and_tip_works() { new_test_ext().execute_with(|| { 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), 11); - assert_eq!(Balances::free_balance(0), 89); + assert_eq!(Balances::reserved_balance(0), 12); + assert_eq!(Balances::free_balance(0), 88); let h = BlakeTwo256::hash_of(&(BlakeTwo256::hash(b"awesome.dot"), 0u128)); assert_ok!(Tips::tip(RuntimeOrigin::signed(10), h, 10)); assert_ok!(Tips::tip(RuntimeOrigin::signed(11), h, 10)); @@ -353,8 +355,8 @@ fn slash_tip_works() { assert_ok!(Tips::report_awesome(RuntimeOrigin::signed(0), b"awesome.dot".to_vec(), 3)); - assert_eq!(Balances::reserved_balance(0), 11); - assert_eq!(Balances::free_balance(0), 89); + assert_eq!(Balances::reserved_balance(0), 12); + assert_eq!(Balances::free_balance(0), 88); let h = tip_hash(); assert_eq!(last_event(), TipEvent::NewTip { tip_hash: h }); @@ -364,11 +366,11 @@ fn slash_tip_works() { // can remove from root. assert_ok!(Tips::slash_tip(RuntimeOrigin::root(), h)); - assert_eq!(last_event(), TipEvent::TipSlashed { tip_hash: h, finder: 0, deposit: 11 }); + assert_eq!(last_event(), TipEvent::TipSlashed { tip_hash: h, finder: 0, deposit: 12 }); // tipper slashed assert_eq!(Balances::reserved_balance(0), 0); - assert_eq!(Balances::free_balance(0), 89); + assert_eq!(Balances::free_balance(0), 88); }); } @@ -656,13 +658,86 @@ fn report_awesome_and_tip_works_second_instance() { } #[test] -fn try_state_invariant_works() { +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)); - assert_eq!(Tips::do_try_state(), Err(Other("finder's fee not present"))) + // 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(); + }); +} From b009d4720faccf50b58261d2ff4ef9f4e8c6153a Mon Sep 17 00:00:00 2001 From: doordashcon Date: Wed, 25 Oct 2023 20:49:57 +0100 Subject: [PATCH 03/10] Doc fix, 1 more invariant --- substrate/frame/tips/src/lib.rs | 7 ++++--- substrate/frame/tips/src/tests.rs | 2 +- 2 files changed, 5 insertions(+), 4 deletions(-) diff --git a/substrate/frame/tips/src/lib.rs b/substrate/frame/tips/src/lib.rs index 705adb64cca4..04e2ad470723 100644 --- a/substrate/frame/tips/src/lib.rs +++ b/substrate/frame/tips/src/lib.rs @@ -636,14 +636,15 @@ impl, I: 'static> Pallet { /// This should be valid before and after each state transition of this pallet. /// /// ## Invariants: - /// 1. Reasons exists for each Tip[`OpenTip.reason`], this implies equal lenth of entries in storage. - /// 2. If `OpenTip.finders_fee` is true, then OpenTip.deposit should be greater that zero. + /// 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")); + 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"); diff --git a/substrate/frame/tips/src/tests.rs b/substrate/frame/tips/src/tests.rs index f9f56a02b0fc..70416534a934 100644 --- a/substrate/frame/tips/src/tests.rs +++ b/substrate/frame/tips/src/tests.rs @@ -685,7 +685,7 @@ fn equal_entries_invariant() { // Invariant violated assert_eq!( Tips::do_try_state(), - Err(Other("Equal length of entries in 'Tips' and 'Reasons` Storage")) + Err(Other("Equal length of entries in `Tips` and `Reasons` Storage")) ); }) } From d1d33cf1471db66f3900e6b58d5c25d3e9abf919 Mon Sep 17 00:00:00 2001 From: doordashcon Date: Thu, 26 Oct 2023 09:44:01 +0100 Subject: [PATCH 04/10] cargo fmt --- substrate/frame/tips/src/lib.rs | 15 +++++++++------ substrate/frame/tips/src/tests.rs | 3 +-- 2 files changed, 10 insertions(+), 8 deletions(-) diff --git a/substrate/frame/tips/src/lib.rs b/substrate/frame/tips/src/lib.rs index 04e2ad470723..0063f1ef515d 100644 --- a/substrate/frame/tips/src/lib.rs +++ b/substrate/frame/tips/src/lib.rs @@ -264,8 +264,8 @@ pub mod pallet { let hash = T::Hashing::hash_of(&(&reason_hash, &who)); ensure!(!Tips::::contains_key(&hash), Error::::AlreadyKnown); - let deposit = T::TipReportDepositBase::get() - + T::DataDepositPerByte::get() * (reason.len() as u32).into(); + let deposit = T::TipReportDepositBase::get() + + T::DataDepositPerByte::get() * (reason.len() as u32).into(); T::Currency::reserve(&finder, deposit)?; Reasons::::insert(&reason_hash, &reason); @@ -532,9 +532,9 @@ impl, I: 'static> Pallet { Some(m) => { member = members_iter.next(); if m < a { - continue; + continue } else { - break true; + break true } }, } @@ -638,13 +638,16 @@ impl, I: 'static> 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. + /// 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")); + 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"); diff --git a/substrate/frame/tips/src/tests.rs b/substrate/frame/tips/src/tests.rs index 70416534a934..746db4905ecf 100644 --- a/substrate/frame/tips/src/tests.rs +++ b/substrate/frame/tips/src/tests.rs @@ -29,10 +29,9 @@ use sp_storage::Storage; use frame_support::{ assert_noop, assert_ok, parameter_types, storage::StoragePrefixedMap, - traits::IntegrityTest, traits::{ tokens::{PayFromAccount, UnityAssetBalanceConversion}, - ConstU32, ConstU64, SortedMembers, StorageVersion, + ConstU32, ConstU64, IntegrityTest, SortedMembers, StorageVersion, }, PalletId, }; From afb4f6dadbc4f3042d3410af97758faf6bbd2e09 Mon Sep 17 00:00:00 2001 From: Jesse Chejieh Date: Tue, 21 Nov 2023 00:30:31 +0100 Subject: [PATCH 05/10] explicit error message MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-authored-by: Gonçalo Pestana --- substrate/frame/tips/src/lib.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/substrate/frame/tips/src/lib.rs b/substrate/frame/tips/src/lib.rs index 0063f1ef515d..2af673490055 100644 --- a/substrate/frame/tips/src/lib.rs +++ b/substrate/frame/tips/src/lib.rs @@ -646,7 +646,7 @@ impl, I: 'static> Pallet { ensure!( reasons.len() == tips.len(), - TryRuntimeError::Other("Equal length of entries in `Tips` and `Reasons` Storage") + TryRuntimeError::Other("Number of `Tips` and of `Reasons` is not the same.") ); for tip in Tips::::iter_keys() { From 8a200aa15e69f50cb1e8e1fe117578c7cf035e4d Mon Sep 17 00:00:00 2001 From: doordashcon Date: Mon, 1 Jan 2024 07:31:47 +0100 Subject: [PATCH 06/10] fix test --- substrate/frame/tips/src/lib.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/substrate/frame/tips/src/lib.rs b/substrate/frame/tips/src/lib.rs index 2af673490055..0063f1ef515d 100644 --- a/substrate/frame/tips/src/lib.rs +++ b/substrate/frame/tips/src/lib.rs @@ -646,7 +646,7 @@ impl, I: 'static> Pallet { ensure!( reasons.len() == tips.len(), - TryRuntimeError::Other("Number of `Tips` and of `Reasons` is not the same.") + TryRuntimeError::Other("Equal length of entries in `Tips` and `Reasons` Storage") ); for tip in Tips::::iter_keys() { From 435b1e451d979ec466b060b2f686b9f91b915a96 Mon Sep 17 00:00:00 2001 From: doordashcon Date: Tue, 9 Jan 2024 06:27:24 +0100 Subject: [PATCH 07/10] add pr docs --- prdoc/pr_1871.prdoc | 12 ++++++++++++ 1 file changed, 12 insertions(+) create mode 100644 prdoc/pr_1871.prdoc diff --git a/prdoc/pr_1871.prdoc b/prdoc/pr_1871.prdoc new file mode 100644 index 000000000000..3d1b1fae7e95 --- /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 From f1e5b6d680ae5c44409bf35417dff76f8adc3139 Mon Sep 17 00:00:00 2001 From: Doordashcon Date: Thu, 11 Jan 2024 05:46:09 +0100 Subject: [PATCH 08/10] fix spacing prdoc Co-authored-by: Oliver Tale-Yazdi --- prdoc/pr_1871.prdoc | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/prdoc/pr_1871.prdoc b/prdoc/pr_1871.prdoc index 3d1b1fae7e95..d1850509afbb 100644 --- a/prdoc/pr_1871.prdoc +++ b/prdoc/pr_1871.prdoc @@ -5,8 +5,8 @@ doc: 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. + 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 From 71c6f329a1b683ceb21e5046160fb9b37b7b9281 Mon Sep 17 00:00:00 2001 From: doordashcon Date: Wed, 17 Jan 2024 17:35:05 +0100 Subject: [PATCH 09/10] build_and_execute + doc update --- substrate/frame/tips/src/lib.rs | 2 +- substrate/frame/tips/src/tests.rs | 29 +++++++++++++++++++---------- 2 files changed, 20 insertions(+), 11 deletions(-) diff --git a/substrate/frame/tips/src/lib.rs b/substrate/frame/tips/src/lib.rs index 0063f1ef515d..4c7cfc3028a9 100644 --- a/substrate/frame/tips/src/lib.rs +++ b/substrate/frame/tips/src/lib.rs @@ -154,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>; diff --git a/substrate/frame/tips/src/tests.rs b/substrate/frame/tips/src/tests.rs index de9e223449cf..2631d72d39a3 100644 --- a/substrate/frame/tips/src/tests.rs +++ b/substrate/frame/tips/src/tests.rs @@ -229,6 +229,15 @@ 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() @@ -240,7 +249,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); }); @@ -252,7 +261,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!( @@ -264,7 +273,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); @@ -291,7 +300,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); @@ -309,7 +318,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); @@ -347,7 +356,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); @@ -378,7 +387,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)); @@ -412,7 +421,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(); @@ -426,7 +435,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(); @@ -443,7 +452,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(); From 204237f7844cb780d2b0784e20b25d77b430e071 Mon Sep 17 00:00:00 2001 From: doordashcon Date: Wed, 17 Jan 2024 18:25:15 +0100 Subject: [PATCH 10/10] build_and_execute + 1 --- substrate/frame/tips/src/tests.rs | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/substrate/frame/tips/src/tests.rs b/substrate/frame/tips/src/tests.rs index 2631d72d39a3..63ac8e37cddd 100644 --- a/substrate/frame/tips/src/tests.rs +++ b/substrate/frame/tips/src/tests.rs @@ -230,8 +230,7 @@ pub fn new_test_ext() -> sp_io::TestExternalities { } /// Run the function pointer inside externalities and asserts the try_state hook at the end. -pub fn build_and_execute(test: impl FnOnce() -> ()) -{ +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"); @@ -637,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);