Skip to content

Commit

Permalink
Adding try_state hook for Tips pallet (#1871)
Browse files Browse the repository at this point in the history 
Part of #239.

Invariant

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.

polkadot address: 12zsKEDVcHpKEWb99iFt3xrTCQQXZMu477nJQsTBBrof5k2h

---------

Co-authored-by: Gonçalo Pestana <g6pestana@gmail.com>
Co-authored-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>
  • Loading branch information
@Doordashcon @gpestana @ggwpez
3 people authored Jan 20, 2024
1 parent 84ff0a9 commit 3b7a8c7
Show file tree
Hide file tree
Showing 3 changed files with 178 additions and 15 deletions.
12 changes: 12 additions & 0 deletions prdoc/pr_1871.prdoc
View file
Original file line number Diff line number Diff line change
@@ -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
59 changes: 58 additions & 1 deletion substrate/frame/tips/src/lib.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand All @@ -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;

Expand Down Expand Up @@ -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>>;

Expand Down Expand Up @@ -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> {
Expand Down Expand Up @@ -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(())
}
}
122 changes: 108 additions & 14 deletions substrate/frame/tips/src/tests.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@ use frame_support::{
storage::StoragePrefixedMap,
traits::{
tokens::{PayFromAccount, UnityAssetBalanceConversion},
ConstU32, ConstU64, SortedMembers, StorageVersion,
ConstU32, ConstU64, IntegrityTest, SortedMembers, StorageVersion,
},
PalletId,
};
Expand Down Expand Up @@ -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;
Expand All @@ -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;
Expand All @@ -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()
Expand All @@ -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);
});
Expand All @@ -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!(
Expand All @@ -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);
Expand All @@ -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);
Expand All @@ -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);
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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));
Expand Down Expand Up @@ -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();
Expand All @@ -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();
Expand All @@ -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();
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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();
});
}

0 comments on commit 3b7a8c7

Please sign in to comment.