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

Adding try_state hook(tips & vesting) #13515

Show file tree
Hide file tree
Changes from 10 commits
Commits
Show all changes
19 commits
Select commit Hold shift + click to select a range
fcaf225
vesting
Doordashcon Mar 2, 2023
70e6c00
Tips
Doordashcon Mar 11, 2023
7f01672
documentation, revert society
Doordashcon Mar 20, 2023
34800d8
revert society
Doordashcon Mar 20, 2023
e3194a1
cargo fmt
Doordashcon Mar 20, 2023
bed7f69
attribute fix
Doordashcon Apr 6, 2023
4b30985
Merge branch 'paritytech:master' into ddc-TS-1
Doordashcon Apr 8, 2023
2d742fa
Merge branch 'paritytech:master' into ddc-TS-1
Doordashcon Apr 17, 2023
786f375
improve vesting docs & Tips try-state
Doordashcon May 4, 2023
d9e0f3a
pepper conditional
Doordashcon May 15, 2023
834a28c
Merge branch 'paritytech:master' into ddc-TS-1
Doordashcon Jun 28, 2023
42410fe
WIP
Doordashcon Jul 30, 2023
6a5a764
vesting docs, tips result
Doordashcon Aug 4, 2023
3c539a9
Merge branch 'paritytech:master' into ddc-TS-1
Doordashcon Aug 4, 2023
8eb23d0
oversight print macro
Doordashcon Aug 4, 2023
dc502ee
feature flag SaturatedConversion
Doordashcon Aug 6, 2023
52d40d4
fully Q
Doordashcon Aug 6, 2023
fda9110
ensure
Doordashcon Aug 8, 2023
b235a18
slit vesting logic into seperate fn
Doordashcon Aug 24, 2023
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
36 changes: 32 additions & 4 deletions frame/tips/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -253,8 +253,8 @@ pub mod pallet {
let hash = T::Hashing::hash_of(&(&reason_hash, &who));
ensure!(!Tips::<T, I>::contains_key(&hash), Error::<T, I>::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::<T, I>::insert(&reason_hash, &reason);
Expand Down 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 @@ -500,9 +508,9 @@ impl<T: Config<I>, I: 'static> Pallet<T, I> {
Some(m) => {
member = members_iter.next();
if m < a {
continue
continue;
} else {
break true
break true;
}
},
}
Expand Down Expand Up @@ -598,4 +606,24 @@ 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:
///
/// * There should be a corresponding `OpenTip.reason` for each key in `Reasons`.
#[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<_>>();

for reason in reasons {
for tip in &tips {
assert_eq!(reason, Tips::<T, I>::get(tip).unwrap().reason);
}
}
Ok(())
}
}
11 changes: 11 additions & 0 deletions frame/tips/src/tests.rs
View file
Open in desktop
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 All @@ -262,6 +263,7 @@ fn report_awesome_and_tip_works() {
assert_ok!(Tips::tip(RuntimeOrigin::signed(12), h, 10));
assert_noop!(Tips::tip(RuntimeOrigin::signed(9), h, 10), BadOrigin);
System::set_block_number(2);
Tips::do_try_state().unwrap();
assert_ok!(Tips::close_tip(RuntimeOrigin::signed(100), h.into()));
assert_eq!(Balances::reserved_balance(0), 0);
assert_eq!(Balances::free_balance(0), 102);
Expand All @@ -281,6 +283,7 @@ fn report_awesome_from_beneficiary_and_tip_works() {
assert_ok!(Tips::tip(RuntimeOrigin::signed(11), h, 10));
assert_ok!(Tips::tip(RuntimeOrigin::signed(12), h, 10));
System::set_block_number(2);
Tips::do_try_state().unwrap();
assert_ok!(Tips::close_tip(RuntimeOrigin::signed(100), h.into()));
assert_eq!(Balances::reserved_balance(0), 0);
assert_eq!(Balances::free_balance(0), 110);
Expand Down Expand Up @@ -312,6 +315,7 @@ fn close_tip_works() {
assert_noop!(Tips::close_tip(RuntimeOrigin::signed(0), h.into()), Error::<Test>::Premature);

System::set_block_number(2);
Tips::do_try_state().unwrap();
assert_noop!(Tips::close_tip(RuntimeOrigin::none(), h.into()), BadOrigin);
assert_ok!(Tips::close_tip(RuntimeOrigin::signed(0), h.into()));
assert_eq!(Balances::free_balance(3), 10);
Expand Down Expand Up @@ -347,6 +351,7 @@ fn slash_tip_works() {
assert_noop!(Tips::slash_tip(RuntimeOrigin::signed(0), h), BadOrigin);

// can remove from root.
Tips::do_try_state().unwrap();
assert_ok!(Tips::slash_tip(RuntimeOrigin::root(), h));
assert_eq!(last_event(), TipEvent::TipSlashed { tip_hash: h, finder: 0, deposit: 12 });

Expand All @@ -366,6 +371,7 @@ fn retract_tip_works() {
assert_ok!(Tips::tip(RuntimeOrigin::signed(10), h, 10));
assert_ok!(Tips::tip(RuntimeOrigin::signed(11), h, 10));
assert_ok!(Tips::tip(RuntimeOrigin::signed(12), h, 10));
Tips::do_try_state().unwrap();
assert_noop!(Tips::retract_tip(RuntimeOrigin::signed(10), h), Error::<Test>::NotFinder);
assert_ok!(Tips::retract_tip(RuntimeOrigin::signed(0), h));
System::set_block_number(2);
Expand All @@ -380,6 +386,7 @@ fn retract_tip_works() {
let h = tip_hash();
assert_ok!(Tips::tip(RuntimeOrigin::signed(11), h, 10));
assert_ok!(Tips::tip(RuntimeOrigin::signed(12), h, 10));
Tips::do_try_state().unwrap();
assert_noop!(Tips::retract_tip(RuntimeOrigin::signed(0), h), Error::<Test>::NotFinder);
assert_ok!(Tips::retract_tip(RuntimeOrigin::signed(10), h));
System::set_block_number(2);
Expand All @@ -399,6 +406,7 @@ fn tip_median_calculation_works() {
assert_ok!(Tips::tip(RuntimeOrigin::signed(11), h, 10));
assert_ok!(Tips::tip(RuntimeOrigin::signed(12), h, 1000000));
System::set_block_number(2);
Tips::do_try_state().unwrap();
assert_ok!(Tips::close_tip(RuntimeOrigin::signed(0), h.into()));
assert_eq!(Balances::free_balance(3), 10);
});
Expand All @@ -418,6 +426,7 @@ fn tip_changing_works() {
assert_ok!(Tips::tip(RuntimeOrigin::signed(11), h, 100));
assert_ok!(Tips::tip(RuntimeOrigin::signed(10), h, 10));
System::set_block_number(2);
Tips::do_try_state().unwrap();
assert_ok!(Tips::close_tip(RuntimeOrigin::signed(0), h.into()));
assert_eq!(Balances::free_balance(3), 10);
});
Expand Down Expand Up @@ -617,5 +626,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();
});
}
55 changes: 48 additions & 7 deletions frame/vesting/src/lib.rs
View file
Open in desktop
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 @@ -416,7 +421,7 @@ pub mod pallet {
) -> DispatchResult {
let who = ensure_signed(origin)?;
if schedule1_index == schedule2_index {
return Ok(())
return Ok(());
};
let schedule1_index = schedule1_index as usize;
let schedule2_index = schedule2_index as usize;
Expand Down Expand Up @@ -494,7 +499,7 @@ impl<T: Config> Pallet<T> {
// Validate user inputs.
ensure!(schedule.locked() >= T::MinVestedTransfer::get(), Error::<T>::AmountLow);
if !schedule.is_valid() {
return Err(Error::<T>::InvalidScheduleParams.into())
return Err(Error::<T>::InvalidScheduleParams.into());
};
let target = T::Lookup::lookup(target)?;
let source = T::Lookup::lookup(source)?;
Expand Down Expand Up @@ -642,12 +647,48 @@ impl<T: Config> Pallet<T> {
};

debug_assert!(
locked_now > Zero::zero() && schedules.len() > 0 ||
locked_now == Zero::zero() && schedules.len() == 0
locked_now > Zero::zero() && schedules.len() > 0
|| locked_now == Zero::zero() && schedules.len() == 0
);

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:
///
/// For each account currently vesting:
/// * The `per_block` amount left to be claimed is equal to the
/// total balance currently locked.
Copy link
Contributor

@liamaharon liamaharon May 16, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Outdated variable name in comment

///
/// *`total_to_vest` the total amount left to vest over all remaining blocks.
/// *`total_locked` the total amount locked.
///
/// * `one_extra` accounts for the extra block that
/// will be added to the vesting schedule if `per_block` is bigger than `locked`.
Copy link
Contributor

@liamaharon liamaharon May 16, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Please add documentation for variables the line above where the variable is declared, rather than in the doc comment

#[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_to_vest: BalanceOf<T> = Zero::zero();
let mut total_locked: BalanceOf<T> = Zero::zero();
for info in vesting.iter() {
// get the number of remaining vesting blocks<>balance
Copy link
Contributor

@liamaharon liamaharon Apr 21, 2023 •
edited
Loading

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 amount_left: BalanceOf<T> =
info.ending_block_as_balance::<T::BlockNumberToBalance>();
total_to_vest += info.per_block().saturating_mul(amount_left);
// get the total block<>balance still locked.
Copy link
Contributor

@liamaharon liamaharon May 16, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Could you please replace this block<>balance shorthand throughout the comments with just plain English

total_locked += info
.locked_at::<T::BlockNumberToBalance>(<frame_system::Pallet<T>>::block_number());
}
let one_extra = total_to_vest.saturating_sub(total_locked);
assert_eq!(total_to_vest, total_locked.saturating_add(one_extra));
Copy link
Contributor

@liamaharon liamaharon May 16, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

So what this is checking is:

if one_extra = total_to_vest - total_locked does total_to_vest = one_extra + total_locked?

or more abstractly

if x = y - z, then does y = x + z?

of course it will always be true, you're asserting that 1 == 1.

});
Ok(())
}
}

impl<T: Config> VestingSchedule<T::AccountId> for Pallet<T>
Expand Down Expand Up @@ -689,13 +730,13 @@ where
starting_block: T::BlockNumber,
) -> DispatchResult {
if locked.is_zero() {
return Ok(())
return Ok(());
}

let vesting_schedule = VestingInfo::new(locked, per_block, starting_block);
// Check for `per_block` or `locked` of 0.
if !vesting_schedule.is_valid() {
return Err(Error::<T>::InvalidScheduleParams.into())
return Err(Error::<T>::InvalidScheduleParams.into());
};

let mut schedules = Self::vesting(who).unwrap_or_default();
Expand Down Expand Up @@ -723,7 +764,7 @@ where
) -> DispatchResult {
// Check for `per_block` or `locked` of 0.
if !VestingInfo::new(locked, per_block, starting_block).is_valid() {
return Err(Error::<T>::InvalidScheduleParams.into())
return Err(Error::<T>::InvalidScheduleParams.into());
}

ensure!(
Expand Down
5 changes: 4 additions & 1 deletion frame/vesting/src/mock.rs
View file
Open in desktop
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 •
edited
Loading

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 •
edited
Loading

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 •
edited
Loading

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
}
}
Loading