Adding try_state hook for Tips pallet

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

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<Percent>;
 
-		/// 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<BalanceOf<Self, I>>;
 
@@ -465,6 +469,21 @@ pub mod pallet {
 			Ok(())
 		}
 	}
+
+	#[pallet::hooks]
+	impl<T: Config<I>, I: 'static> Hooks<BlockNumberFor<T>> for Pallet<T, I> {
+		fn integrity_test() {
+			assert!(
+				!T::TipReportDepositBase::get().is_zero(),
+				"`TipReportDepositBase` should not be zero",
+			);
+		}
+
+		#[cfg(feature = "try-runtime")]
+		fn try_state(_n: BlockNumberFor<T>) -> Result<(), TryRuntimeError> {
+			Self::do_try_state()
+		}
+	}
 }
 
 impl<T: Config<I>, I: 'static> Pallet<T, I> {
@@ -611,4 +630,42 @@ impl<T: Config<I>, I: 'static> Pallet<T, I> {
 			Tips::<T, I>::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::<T, I>::iter_keys().collect::<Vec<_>>();
+		let tips = Tips::<T, I>::iter_keys().collect::<Vec<_>>();
+
+		ensure!(
+			reasons.len() == tips.len(),
+			TryRuntimeError::Other("Equal length of entries in `Tips` and `Reasons` Storage")
+		);
+
+		for tip in Tips::<T, I>::iter_keys() {
+			let open_tip = Tips::<T, I>::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(())
+	}
 }

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<Instance1> 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<Instance1> 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<Test> {
 	System::events()
 		.into_iter()
@@ -239,7 +248,7 @@ fn last_event() -> TipEvent<Test> {
 
 #[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::<u128, u64, u64, H256> {
+			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::<Test>::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::<Test>::iter_keys().collect();
+
+		let mut open_tip = pallet_tips::Tips::<Test>::take(hash[0]).unwrap();
+
+		// Breaks invariant by changing value `open_tip.reason` in `Tips` Storage.
+		open_tip.reason = <Test as frame_system::Config>::Hashing::hash(&b"".to_vec());
+
+		pallet_tips::Tips::<Test>::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();
+	});
+}