Skip to content
This repository has been archived by the owner on Nov 15, 2023. It is now read-only.

Adding try_state hook(tips & vesting) #13515

Open
wants to merge 19 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 8 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 23 additions & 0 deletions frame/tips/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -452,6 +452,14 @@ pub mod pallet {
Ok(())
}
}

#[pallet::hooks]
impl<T: Config<I>, I: 'static> Hooks<BlockNumberFor<T>> for Pallet<T, I> {
#[cfg(feature = "try-runtime")]
fn try_state(_n: BlockNumberFor<T>) -> Result<(), &'static str> {
Self::do_try_state()
}
}
}

impl<T: Config<I>, I: 'static> Pallet<T, I> {
Expand Down Expand Up @@ -598,4 +606,19 @@ 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 or after each state transition of this pallet.
///
/// ## Invariants:
///
/// * `Reasons` and `Tips` must have the same length of keys
#[cfg(any(feature = "try-runtime", test))]
pub fn do_try_state() -> Result<(), &'static str> {
let reasons = Reasons::<T, I>::iter_keys().collect::<Vec<_>>();
let tips = Tips::<T, I>::iter_keys().collect::<Vec<_>>();
assert_eq!(reasons.len(), tips.len());
Doordashcon marked this conversation as resolved.
Show resolved Hide resolved
Ok(())
}
}
13 changes: 13 additions & 0 deletions frame/tips/src/tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -239,6 +239,7 @@ fn tip_new_cannot_be_used_twice() {
Tips::tip_new(RuntimeOrigin::signed(11), b"awesome.dot".to_vec(), 3, 10),
Error::<Test>::AlreadyKnown
);
Tips::do_try_state().unwrap();
});
}

Expand Down Expand Up @@ -266,6 +267,7 @@ fn report_awesome_and_tip_works() {
assert_eq!(Balances::reserved_balance(0), 0);
assert_eq!(Balances::free_balance(0), 102);
assert_eq!(Balances::free_balance(3), 8);
Tips::do_try_state().unwrap();
});
}

Expand All @@ -284,6 +286,7 @@ fn report_awesome_from_beneficiary_and_tip_works() {
assert_ok!(Tips::close_tip(RuntimeOrigin::signed(100), h.into()));
assert_eq!(Balances::reserved_balance(0), 0);
assert_eq!(Balances::free_balance(0), 110);
Tips::do_try_state().unwrap();
});
}

Expand Down Expand Up @@ -322,6 +325,8 @@ fn close_tip_works() {
Tips::close_tip(RuntimeOrigin::signed(100), h.into()),
Error::<Test>::UnknownTip
);

Tips::do_try_state().unwrap();
});
}

Expand Down Expand Up @@ -353,6 +358,8 @@ fn slash_tip_works() {
// tipper slashed
assert_eq!(Balances::reserved_balance(0), 0);
assert_eq!(Balances::free_balance(0), 88);

Tips::do_try_state().unwrap();
});
}

Expand Down Expand Up @@ -387,6 +394,8 @@ fn retract_tip_works() {
Tips::close_tip(RuntimeOrigin::signed(10), h.into()),
Error::<Test>::UnknownTip
);

Tips::do_try_state().unwrap();
});
}

Expand All @@ -401,6 +410,7 @@ fn tip_median_calculation_works() {
System::set_block_number(2);
assert_ok!(Tips::close_tip(RuntimeOrigin::signed(0), h.into()));
assert_eq!(Balances::free_balance(3), 10);
Tips::do_try_state().unwrap();
});
}

Expand All @@ -420,6 +430,7 @@ fn tip_changing_works() {
System::set_block_number(2);
assert_ok!(Tips::close_tip(RuntimeOrigin::signed(0), h.into()));
assert_eq!(Balances::free_balance(3), 10);
Tips::do_try_state().unwrap();
});
}

Expand Down Expand Up @@ -617,5 +628,7 @@ fn report_awesome_and_tip_works_second_instance() {
assert_eq!(Balances::free_balance(&Treasury::account_id()), 101);
// Treasury 2 gave the funds
assert_eq!(Balances::free_balance(&Treasury1::account_id()), 191);

Tips::do_try_state().unwrap();
});
}
33 changes: 33 additions & 0 deletions frame/vesting/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -192,6 +192,11 @@ pub mod pallet {
fn integrity_test() {
assert!(T::MAX_VESTING_SCHEDULES > 0, "`MaxVestingSchedules` must ge greater than 0");
}

#[cfg(feature = "try-runtime")]
fn try_state(_n: BlockNumberFor<T>) -> Result<(), &'static str> {
Self::do_try_state()
}
}

/// Information regarding the vesting of a given account.
Expand Down Expand Up @@ -648,6 +653,34 @@ impl<T: Config> Pallet<T> {

Ok((schedules, locked_now))
}

/// Ensure the correctness of the state of this pallet.
///
/// This should be valid before or after each state transition of this pallet.
///
/// ## Invariants:
///
/// * The `per_block` payments left of every account currently vesting
/// * is equal to the total balance currently locked.
#[cfg(any(feature = "try-runtime", test))]
pub fn do_try_state() -> Result<(), &'static str> {
Doordashcon marked this conversation as resolved.
Show resolved Hide resolved
Vesting::<T>::iter().for_each(|(_, d)| {
let vesting = d.to_vec();
let mut total_per_block: BalanceOf<T> = Zero::zero();
let mut total_locked_now: BalanceOf<T> = Zero::zero();
liamaharon marked this conversation as resolved.
Show resolved Hide resolved
for info in vesting.iter() {
// get the number of remaining vesting blocks<>balance
Copy link
Contributor

@liamaharon liamaharon Apr 21, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't understand this comment

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the number of blocks remaining are converted to balance

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please update this comment with this clarification

let payments_left: BalanceOf<T> =
info.ending_block_as_balance::<T::BlockNumberToBalance>();
Doordashcon marked this conversation as resolved.
Show resolved Hide resolved
total_per_block += info.per_block().saturating_mul(payments_left);
total_locked_now += info
.locked_at::<T::BlockNumberToBalance>(<frame_system::Pallet<T>>::block_number());
}
let one_extra = total_per_block.saturating_sub(total_locked_now);
assert_eq!(total_per_block, total_locked_now.saturating_add(one_extra));
Doordashcon marked this conversation as resolved.
Show resolved Hide resolved
});
Ok(())
}
}

impl<T: Config> VestingSchedule<T::AccountId> for Pallet<T>
Expand Down
5 changes: 4 additions & 1 deletion frame/vesting/src/mock.rs
Original file line number Diff line number Diff line change
Expand Up @@ -153,7 +153,10 @@ impl ExtBuilder {
.assimilate_storage(&mut t)
.unwrap();
let mut ext = sp_io::TestExternalities::new(t);
ext.execute_with(|| System::set_block_number(1));
ext.execute_with(|| {
System::set_block_number(1);
Vesting::do_try_state().unwrap();
});
Copy link
Contributor

@liamaharon liamaharon Apr 21, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this doesn't really test much because it runs before there's any state set up to test?

I suggest that you

  1. test that the try-state hooks will detect the issues that they're supposed to, check out how that's done here: https://github.com/paritytech/substrate/blob/master/frame/bags-list/src/list/tests.rs#L353-L378 (also do this with tips)
  2. pepper assert_ok!(vesting::do_try_state());s into existing tests, like you did with tips

Copy link
Contributor Author

@Doordashcon Doordashcon May 4, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But i still think it's all the same and there's no need to pepper existing tests, the current setup in vesting tests the genesis state.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let me know if I'm missing something, but I think we should test all functionality not just the genesis state?

Like bags-list there should be tests ensuring that the try-state hooks actually detect the issues that they're supposed to: https://github.com/paritytech/substrate/blob/master/frame/bags-list/src/list/tests.rs#L353-L378

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hello @liamaharon , I have made updates to this. Please review

Copy link
Contributor

@liamaharon liamaharon May 16, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add tests ensuring that the try-state hooks detects the issues they are intended to. They’d have picked up a bug in the assert (see other comment)

ext
}
}