From b4acc403e85f40ee68f73b7bc087c3bc6583ccab Mon Sep 17 00:00:00 2001 From: doordashcon Date: Thu, 4 Jan 2024 05:10:07 +0100 Subject: [PATCH 1/8] include invariant --- substrate/frame/vesting/src/lib.rs | 121 ++++++++++++++++++++++++++++ substrate/frame/vesting/src/mock.rs | 5 +- 2 files changed, 125 insertions(+), 1 deletion(-) diff --git a/substrate/frame/vesting/src/lib.rs b/substrate/frame/vesting/src/lib.rs index 4101caded4180..7d309c80cc338 100644 --- a/substrate/frame/vesting/src/lib.rs +++ b/substrate/frame/vesting/src/lib.rs @@ -78,6 +78,9 @@ use sp_runtime::{ }; use sp_std::{fmt::Debug, marker::PhantomData, prelude::*}; +#[cfg(any(feature = "try-runtime", test))] +use sp_runtime::{SaturatedConversion, TryRuntimeError}; + pub use pallet::*; pub use vesting_info::*; pub use weights::WeightInfo; @@ -196,6 +199,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) -> Result<(), TryRuntimeError> { + Self::do_try_state() + } } /// Information regarding the vesting of a given account. @@ -678,6 +686,119 @@ impl Pallet { } } +/// Ensure the correctness of the state of this pallet. +/// +/// The following expectations must always apply. +/// +/// ## Expectations: +/// +/// `handle_before_schedule_starts`: +/// * the locked amount of a vesting schedule must be equal to the +/// product of the duration(`schedules_left` - `starting_block`) and the per block amount when +/// the locked amount is divisible by the per block amount. +/// * However, If the locked amount is not divisible by the per block amount, the final vesting blo +/// (`schedules_left` - 1), the vesting balance should be equal to the remainder. +/// +/// `handle_during_schedule`: +/// * the amount `still_vesting` must be equal to the product of the remaining blocks to +/// vest(`schedules_left` - `current_block`) +/// and per block amount when the locked amount is divisible by the per block amount. +/// * However, If the locked amount is not divisible by the per block amount, then at the final +/// vesting block of the +/// current schedule (`schedules_left` - 1), the vesting balance should be equal to the +/// remainder. +#[cfg(any(feature = "try-runtime", test))] +impl Pallet { + pub fn do_try_state() -> Result<(), TryRuntimeError> { + for (_, d) in Vesting::::iter() { + let infos = d.to_vec(); + + for info in infos.iter() { + let schedules_left: BalanceOf = + info.ending_block_as_balance::(); + let starting_block = T::BlockNumberToBalance::convert(info.starting_block()); + let current_block_to_balance = + T::BlockNumberToBalance::convert(>::block_number()); + + if current_block_to_balance < starting_block { + Self::handle_before_schedule_starts(info, starting_block, schedules_left)?; + } else { + Self::handle_during_schedule(info, current_block_to_balance, schedules_left)?; + } + } + } + Ok(()) + } + + fn handle_before_schedule_starts( + info: &VestingInfo, BlockNumberFor>, + starting_block: BalanceOf, + schedules_left: BalanceOf, + ) -> Result<(), TryRuntimeError> { + let count = schedules_left.saturating_sub(starting_block); + + if (info.locked() % info.per_block()).is_zero() { + ensure!( + info.locked_at::(frame_system::Pallet::::block_number()) == (count * info.per_block()), + TryRuntimeError::Other("Before schedule starts, the vesting balance should be equal to the total per block releases") + ); + } else { + let re = info.locked() % info.per_block(); + + let final_vest_block = + schedules_left.saturating_sub(One::one()).saturated_into::(); + + let final_vest_amount = + info.locked_at::(final_vest_block.saturated_into()); + + ensure!(final_vest_amount == re, TryRuntimeError::Other("Before schedule starts, the final vest amount should be equal to the remainder")); + + let no_schedules_left = schedules_left.saturated_into::(); + + let no_locks = + info.locked_at::(no_schedules_left.saturated_into()); + + ensure!( + no_locks == Zero::zero(), + TryRuntimeError::Other("After all schedules, all amounts should be unlocked") + ); + } + + Ok(()) + } + + fn handle_during_schedule( + info: &VestingInfo, BlockNumberFor>, + current_block_to_balance: BalanceOf, + schedules_left: BalanceOf, + ) -> Result<(), TryRuntimeError> { + let current_block = frame_system::Pallet::::block_number(); + + let still_vesting = info.locked_at::(current_block); + + if (info.locked() % info.per_block()).is_zero() { + ensure!( + still_vesting == (schedules_left.saturating_sub(current_block_to_balance) * info.per_block()), + TryRuntimeError::Other("during schedules, the vesting balance should be equal to the total per block releases") + ); + } else { + let re = info.locked() % info.per_block(); + + if current_block_to_balance == schedules_left.saturating_sub(One::one()) { + ensure!(still_vesting == re, TryRuntimeError::Other("At the final vesting block, the vesting balance should be equal to the remainder")); + } + + if current_block_to_balance == schedules_left { + ensure!( + still_vesting == Zero::zero(), + TryRuntimeError::Other("Schedule ended, no more vesting balance") + ); + } + } + Ok(()) + } +} + impl VestingSchedule for Pallet where BalanceOf: MaybeSerializeDeserialize + Debug, diff --git a/substrate/frame/vesting/src/mock.rs b/substrate/frame/vesting/src/mock.rs index 3af4a9c962d1b..0701fcf6c38ff 100644 --- a/substrate/frame/vesting/src/mock.rs +++ b/substrate/frame/vesting/src/mock.rs @@ -151,7 +151,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(); + }); ext } } From 7fca502ec11525d4b3c791a17614c1e16d9415cd Mon Sep 17 00:00:00 2001 From: doordashcon Date: Sat, 6 Jan 2024 04:16:00 +0100 Subject: [PATCH 2/8] test --- substrate/frame/vesting/src/tests.rs | 103 +++++++++++++++++++++++++++ 1 file changed, 103 insertions(+) diff --git a/substrate/frame/vesting/src/tests.rs b/substrate/frame/vesting/src/tests.rs index 2e1e41fc9578f..6ba20a0f5514f 100644 --- a/substrate/frame/vesting/src/tests.rs +++ b/substrate/frame/vesting/src/tests.rs @@ -38,6 +38,7 @@ where { // Its ok for this to fail because the user may already have no schedules. let _result = Vesting::vest(Some(account).into()); + assert_ok!(Vesting::do_try_state()); assert!(!>::contains_key(account)); } @@ -78,6 +79,7 @@ fn check_vesting_status() { System::set_block_number(10); assert_eq!(System::block_number(), 10); + assert_ok!(Vesting::do_try_state()); // Account 1 has fully vested by block 10 assert_eq!(Vesting::vesting_balance(&1), Some(0)); @@ -88,6 +90,7 @@ fn check_vesting_status() { System::set_block_number(30); assert_eq!(System::block_number(), 30); + assert_ok!(Vesting::do_try_state()); assert_eq!(Vesting::vesting_balance(&1), Some(0)); // Account 1 is still fully vested, and not negative assert_eq!(Vesting::vesting_balance(&2), Some(0)); // Account 2 has fully vested by block 30 @@ -124,6 +127,7 @@ fn check_vesting_status_for_multi_schedule_account() { 0, ); assert_ok!(Vesting::vested_transfer(Some(4).into(), 2, sched1)); + assert_ok!(Vesting::do_try_state()); // Free balance is equal to the two existing schedules total amount. let free_balance = Balances::free_balance(&2); assert_eq!(free_balance, ED * (10 + 20)); @@ -139,8 +143,10 @@ fn check_vesting_status_for_multi_schedule_account() { 5, ); assert_ok!(Vesting::vested_transfer(Some(4).into(), 2, sched2)); + assert_ok!(Vesting::do_try_state()); System::set_block_number(9); + assert_ok!(Vesting::do_try_state()); // Free balance is equal to the 3 existing schedules total amount. let free_balance = Balances::free_balance(&2); assert_eq!(free_balance, ED * (10 + 20 + 30)); @@ -158,6 +164,7 @@ fn check_vesting_status_for_multi_schedule_account() { free_balance - sched1.locked() - sched2.per_block() * 15 - sched0.per_block() * 10 ) ); + assert_ok!(Vesting::do_try_state()); System::set_block_number(30); // At block #30 sched0 and sched1 are fully unlocked while sched2 is partially unlocked. @@ -165,10 +172,12 @@ fn check_vesting_status_for_multi_schedule_account() { Vesting::vesting_balance(&2), Some(free_balance - sched1.locked() - sched2.per_block() * 25 - sched0.locked()) ); + assert_ok!(Vesting::do_try_state()); // At block #35 sched2 fully unlocks and thus all schedules funds are unlocked. System::set_block_number(35); assert_eq!(Vesting::vesting_balance(&2), Some(0)); + assert_ok!(Vesting::do_try_state()); // Since we have not called any extrinsics that would unlock funds the schedules // are still in storage, assert_eq!(Vesting::vesting(&2).unwrap(), vec![sched0, sched1, sched2]); @@ -197,6 +206,7 @@ fn vested_balance_should_transfer() { // Account 1 has only 5 units vested at block 1 (plus 50 unvested) assert_eq!(Vesting::vesting_balance(&1), Some(45)); assert_ok!(Vesting::vest(Some(1).into())); + assert_ok!(Vesting::do_try_state()); assert_ok!(Balances::transfer_allow_death(Some(1).into(), 2, 55)); }); } @@ -206,6 +216,7 @@ fn vested_balance_should_transfer_with_multi_sched() { ExtBuilder::default().existential_deposit(ED).build().execute_with(|| { let sched0 = VestingInfo::new(5 * ED, 128, 0); assert_ok!(Vesting::vested_transfer(Some(13).into(), 1, sched0)); + assert_ok!(Vesting::do_try_state()); // Total 10*ED locked for all the schedules. assert_eq!(Vesting::vesting(&1).unwrap(), vec![sched0, sched0]); @@ -215,6 +226,7 @@ fn vested_balance_should_transfer_with_multi_sched() { // Account 1 has only 256 units unlocking at block 1 (plus 1280 already fee). assert_eq!(Vesting::vesting_balance(&1), Some(2304)); assert_ok!(Vesting::vest(Some(1).into())); + assert_ok!(Vesting::do_try_state()); assert_ok!(Balances::transfer_allow_death(Some(1).into(), 2, 1536)); }); } @@ -235,6 +247,7 @@ fn vested_balance_should_transfer_using_vest_other() { // Account 1 has only 5 units vested at block 1 (plus 50 unvested) assert_eq!(Vesting::vesting_balance(&1), Some(45)); assert_ok!(Vesting::vest_other(Some(2).into(), 1)); + assert_ok!(Vesting::do_try_state()); assert_ok!(Balances::transfer_allow_death(Some(1).into(), 2, 55)); }); } @@ -253,6 +266,7 @@ fn vested_balance_should_transfer_using_vest_other_with_multi_sched() { // Account 1 has only 256 units unlocking at block 1 (plus 1280 already free). assert_eq!(Vesting::vesting_balance(&1), Some(2304)); assert_ok!(Vesting::vest_other(Some(2).into(), 1)); + assert_ok!(Vesting::do_try_state()); assert_ok!(Balances::transfer_allow_death(Some(1).into(), 2, 1536)); }); } @@ -280,11 +294,13 @@ fn extra_balance_should_transfer() { // Account 1 has only 5 units vested at block 1 (plus 150 unvested) assert_eq!(Vesting::vesting_balance(&1), Some(45)); assert_ok!(Vesting::vest(Some(1).into())); + assert_ok!(Vesting::do_try_state()); assert_ok!(Balances::transfer_allow_death(Some(1).into(), 3, 155)); // Account 1 can send extra units gained // Account 2 has no units vested at block 1, but gained 100 assert_eq!(Vesting::vesting_balance(&2), Some(200)); assert_ok!(Vesting::vest(Some(2).into())); + assert_ok!(Vesting::do_try_state()); assert_ok!(Balances::transfer_allow_death(Some(2).into(), 3, 100)); // Account 2 can send extra // units gained }); @@ -328,6 +344,7 @@ fn vested_transfer_works() { 10, ); assert_ok!(Vesting::vested_transfer(Some(3).into(), 4, new_vesting_schedule)); + assert_ok!(Vesting::do_try_state()); // Now account 4 should have vesting. assert_eq!(Vesting::vesting(&4).unwrap(), vec![new_vesting_schedule]); // Ensure the transfer happened correctly. @@ -340,12 +357,14 @@ fn vested_transfer_works() { System::set_block_number(20); assert_eq!(System::block_number(), 20); + assert_ok!(Vesting::do_try_state()); // Account 4 has 5 * 64 units vested by block 20. assert_eq!(Vesting::vesting_balance(&4), Some(10 * 64)); System::set_block_number(30); assert_eq!(System::block_number(), 30); + assert_ok!(Vesting::do_try_state()); // Account 4 has fully vested, assert_eq!(Vesting::vesting_balance(&4), Some(0)); @@ -481,12 +500,14 @@ fn force_vested_transfer_works() { System::set_block_number(20); assert_eq!(System::block_number(), 20); + assert_ok!(Vesting::do_try_state()); // Account 4 has 5 * 64 units vested by block 20. assert_eq!(Vesting::vesting_balance(&4), Some(10 * 64)); System::set_block_number(30); assert_eq!(System::block_number(), 30); + assert_ok!(Vesting::do_try_state()); // Account 4 has fully vested, assert_eq!(Vesting::vesting_balance(&4), Some(0)); @@ -580,6 +601,7 @@ fn force_vested_transfer_allows_max_schedules() { // Account 4 has fully vested when all the schedules end, System::set_block_number(::MinVestedTransfer::get() + 10); assert_eq!(Vesting::vesting_balance(&4), Some(0)); + assert_ok!(Vesting::do_try_state()); // and after unlocking its schedules are removed from storage. vest_and_assert_no_vesting::(4); }); @@ -599,9 +621,11 @@ fn merge_schedules_that_have_not_started() { // Add a schedule that is identical to the one that already exists. assert_ok!(Vesting::vested_transfer(Some(3).into(), 2, sched0)); + assert_ok!(Vesting::do_try_state()); assert_eq!(Vesting::vesting(&2).unwrap(), vec![sched0, sched0]); assert_eq!(Balances::usable_balance(&2), 0); assert_ok!(Vesting::merge_schedules(Some(2).into(), 0, 1)); + assert_ok!(Vesting::do_try_state()); // Since we merged identical schedules, the new schedule finishes at the same // time as the original, just with double the amount. @@ -634,17 +658,20 @@ fn merge_ongoing_schedules() { sched0.starting_block() + 5, // Start at block 15. ); assert_ok!(Vesting::vested_transfer(Some(4).into(), 2, sched1)); + assert_ok!(Vesting::do_try_state()); assert_eq!(Vesting::vesting(&2).unwrap(), vec![sched0, sched1]); // Got to half way through the second schedule where both schedules are actively vesting. let cur_block = 20; System::set_block_number(cur_block); + assert_ok!(Vesting::do_try_state()); // Account 2 has no usable balances prior to the merge because they have not unlocked // with `vest` yet. assert_eq!(Balances::usable_balance(&2), 0); assert_ok!(Vesting::merge_schedules(Some(2).into(), 0, 1)); + assert_ok!(Vesting::do_try_state()); // Merging schedules un-vests all pre-existing schedules prior to merging, which is // reflected in account 2's updated usable balance. @@ -670,6 +697,7 @@ fn merge_ongoing_schedules() { // And just to double check, we assert the new merged schedule we be cleaned up as expected. System::set_block_number(30); + assert_ok!(Vesting::do_try_state()); vest_and_assert_no_vesting::(2); }); } @@ -703,11 +731,15 @@ fn merging_shifts_other_schedules_index() { let cur_block = 1; assert_eq!(System::block_number(), cur_block); + assert_ok!(Vesting::do_try_state()); // Transfer the above 3 schedules to account 3. assert_ok!(Vesting::vested_transfer(Some(4).into(), 3, sched0)); + assert_ok!(Vesting::do_try_state()); assert_ok!(Vesting::vested_transfer(Some(4).into(), 3, sched1)); + assert_ok!(Vesting::do_try_state()); assert_ok!(Vesting::vested_transfer(Some(4).into(), 3, sched2)); + assert_ok!(Vesting::do_try_state()); // With no schedules vested or merged they are in the order they are created assert_eq!(Vesting::vesting(&3).unwrap(), vec![sched0, sched1, sched2]); @@ -715,6 +747,7 @@ fn merging_shifts_other_schedules_index() { assert_eq!(usable_balance, Balances::usable_balance(&3)); assert_ok!(Vesting::merge_schedules(Some(3).into(), 0, 2)); + assert_ok!(Vesting::do_try_state()); // Create the merged schedule of sched0 & sched2. // The merged schedule will have the max possible starting block, @@ -755,6 +788,7 @@ fn merge_ongoing_and_yet_to_be_started_schedules() { (sched0.starting_block() + sched0.ending_block_as_balance::()) / 2; assert_eq!(cur_block, 20); System::set_block_number(cur_block); + assert_ok!(Vesting::do_try_state()); // Prior to vesting there is no usable balance. let mut usable_balance = 0; @@ -778,9 +812,11 @@ fn merge_ongoing_and_yet_to_be_started_schedules() { cur_block + 1, ); assert_ok!(Vesting::vested_transfer(Some(4).into(), 2, sched1)); + assert_ok!(Vesting::do_try_state()); // Merge the schedules before sched1 starts. assert_ok!(Vesting::merge_schedules(Some(2).into(), 0, 1)); + assert_ok!(Vesting::do_try_state()); // After merging, the usable balance only changes by the amount sched0 vested since we // last called `vest` (which is just 1 block). The usable balance is not affected by // sched1 because it has not started yet. @@ -823,6 +859,7 @@ fn merge_finished_and_ongoing_schedules() { 10, ); assert_ok!(Vesting::vested_transfer(Some(4).into(), 2, sched1)); + assert_ok!(Vesting::do_try_state()); // Transfer a 3rd schedule, so we can demonstrate how schedule indices change. // (We are not merging this schedule.) @@ -832,6 +869,7 @@ fn merge_finished_and_ongoing_schedules() { 10, ); assert_ok!(Vesting::vested_transfer(Some(3).into(), 2, sched2)); + assert_ok!(Vesting::do_try_state()); // The schedules are in expected order prior to merging. assert_eq!(Vesting::vesting(&2).unwrap(), vec![sched0, sched1, sched2]); @@ -840,11 +878,13 @@ fn merge_finished_and_ongoing_schedules() { let cur_block = sched0.ending_block_as_balance::(); System::set_block_number(cur_block); assert_eq!(System::block_number(), 30); + assert_ok!(Vesting::do_try_state()); // Prior to `merge_schedules` and with no vest/vest_other called the user has no usable // balance. assert_eq!(Balances::usable_balance(&2), 0); assert_ok!(Vesting::merge_schedules(Some(2).into(), 0, 1)); + assert_ok!(Vesting::do_try_state()); // sched2 is now the first, since sched0 & sched1 get filtered out while "merging". // sched1 gets treated like the new merged schedule by getting pushed onto back @@ -886,6 +926,7 @@ fn merge_finishing_schedules_does_not_create_a_new_one() { 10, ); assert_ok!(Vesting::vested_transfer(Some(3).into(), 2, sched1)); + assert_ok!(Vesting::do_try_state()); assert_eq!(Vesting::vesting(&2).unwrap(), vec![sched0, sched1]); let all_scheds_end = sched0 @@ -894,6 +935,7 @@ fn merge_finishing_schedules_does_not_create_a_new_one() { assert_eq!(all_scheds_end, 40); System::set_block_number(all_scheds_end); + assert_ok!(Vesting::do_try_state()); // Prior to merge_schedules and with no vest/vest_other called the user has no usable // balance. @@ -901,6 +943,7 @@ fn merge_finishing_schedules_does_not_create_a_new_one() { // Merge schedule 0 and 1. assert_ok!(Vesting::merge_schedules(Some(2).into(), 0, 1)); + assert_ok!(Vesting::do_try_state()); // The user no longer has any more vesting schedules because they both ended at the // block they where merged, assert!(!>::contains_key(&2)); @@ -927,6 +970,7 @@ fn merge_finished_and_yet_to_be_started_schedules() { 35, ); assert_ok!(Vesting::vested_transfer(Some(13).into(), 2, sched1)); + assert_ok!(Vesting::do_try_state()); assert_eq!(Vesting::vesting(&2).unwrap(), vec![sched0, sched1]); let sched2 = VestingInfo::new( @@ -936,9 +980,11 @@ fn merge_finished_and_yet_to_be_started_schedules() { ); // Add a 3rd schedule to demonstrate how sched1 shifts. assert_ok!(Vesting::vested_transfer(Some(13).into(), 2, sched2)); + assert_ok!(Vesting::do_try_state()); assert_eq!(Vesting::vesting(&2).unwrap(), vec![sched0, sched1, sched2]); System::set_block_number(30); + assert_ok!(Vesting::do_try_state()); // At block 30, sched0 has finished unlocking while sched1 and sched2 are still fully // locked, @@ -948,6 +994,7 @@ fn merge_finished_and_yet_to_be_started_schedules() { // Merge schedule 0 and 1. assert_ok!(Vesting::merge_schedules(Some(2).into(), 0, 1)); + assert_ok!(Vesting::do_try_state()); // sched0 is removed since it finished, and sched1 is removed and then pushed on the back // because it is treated as the merged schedule @@ -1170,6 +1217,7 @@ fn remove_vesting_schedule() { 10, ); assert_ok!(Vesting::vested_transfer(Some(3).into(), 4, new_vesting_schedule)); + assert_ok!(Vesting::do_try_state()); // Now account 4 should have vesting. assert_eq!(Vesting::vesting(&4).unwrap(), vec![new_vesting_schedule]); // Account 4 has 5 * 256 locked. @@ -1191,3 +1239,58 @@ fn remove_vesting_schedule() { ); }); } + +#[test] +fn play_out_all_schedules() { + let vesting_config = vec![ + // 5 * existential deposit locked. + (1, 0, 10, 5 * ED), + // 1 * existential deposit locked. + (2, 10, 20, 19 * ED), + // 2 * existential deposit locked. + (2, 10, 0, 18 * ED), + // 1 * existential deposit locked. + (12, 0, 20, 9 * ED), + // 2 * existential deposit locked. + (12, 10, 0, 8 * ED), + // 3 * existential deposit locked. + (12, 10, 20, 7 * ED), + ]; + ExtBuilder::default() + .existential_deposit(ED) + .vesting_genesis_config(vesting_config) + .build() + .execute_with(|| { + let user1_sched1 = VestingInfo::new(5 * ED, 128, 0u64); + assert_eq!(Vesting::vesting(&1).unwrap(), vec![user1_sched1]); + + let user2_sched1 = VestingInfo::new(1 * ED, 12, 10u64); + let user2_sched2 = VestingInfo::new(2 * ED, 512, 10u64); + assert_eq!(Vesting::vesting(&2).unwrap(), vec![user2_sched1, user2_sched2]); + + let user12_sched1 = VestingInfo::new(1 * ED, 12, 0u64); + let user12_sched2 = VestingInfo::new(2 * ED, 512, 10u64); + let user12_sched3 = VestingInfo::new(3 * ED, 38, 10u64); + assert_eq!( + Vesting::vesting(&12).unwrap(), + vec![user12_sched1, user12_sched2, user12_sched3] + ); + + // get the max ending block + let mut v: Vec = Vec::new(); + + v.push(user1_sched1.ending_block_as_balance::()); + v.push(user2_sched1.ending_block_as_balance::()); + v.push(user2_sched2.ending_block_as_balance::()); + v.push(user12_sched1.ending_block_as_balance::()); + v.push(user12_sched2.ending_block_as_balance::()); + v.push(user12_sched3.ending_block_as_balance::()); + + // Loop through all schedules + let max_value = v.iter().max().unwrap_or(&380); + for i in 0..*max_value { + System::set_block_number(i); + assert_ok!(Vesting::do_try_state()); + } + }) +} From 5456eb272a42ed27bb8e5cb01e1245c536a3c216 Mon Sep 17 00:00:00 2001 From: doordashcon Date: Sat, 6 Jan 2024 04:24:29 +0100 Subject: [PATCH 3/8] unvested amount instead of vested balance --- substrate/frame/vesting/src/lib.rs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/substrate/frame/vesting/src/lib.rs b/substrate/frame/vesting/src/lib.rs index 7d309c80cc338..0cf671f7c56fc 100644 --- a/substrate/frame/vesting/src/lib.rs +++ b/substrate/frame/vesting/src/lib.rs @@ -696,8 +696,8 @@ impl Pallet { /// * the locked amount of a vesting schedule must be equal to the /// product of the duration(`schedules_left` - `starting_block`) and the per block amount when /// the locked amount is divisible by the per block amount. -/// * However, If the locked amount is not divisible by the per block amount, the final vesting blo -/// (`schedules_left` - 1), the vesting balance should be equal to the remainder. +/// * However, If the locked amount is not divisible by the per block amount, the final vesting block +/// (`schedules_left` - 1), the unvested amount should be equal to the remainder. /// /// `handle_during_schedule`: /// * the amount `still_vesting` must be equal to the product of the remaining blocks to @@ -705,7 +705,7 @@ impl Pallet { /// and per block amount when the locked amount is divisible by the per block amount. /// * However, If the locked amount is not divisible by the per block amount, then at the final /// vesting block of the -/// current schedule (`schedules_left` - 1), the vesting balance should be equal to the +/// current schedule (`schedules_left` - 1), the unvested amount should be equal to the /// remainder. #[cfg(any(feature = "try-runtime", test))] impl Pallet { From a716860996e158075a5aefa3736e2a5d3341e3d3 Mon Sep 17 00:00:00 2001 From: doordashcon Date: Tue, 9 Jan 2024 06:01:06 +0100 Subject: [PATCH 4/8] prdoc added --- prdoc/pr_2847.prdoc | 9 +++++++++ substrate/frame/vesting/src/lib.rs | 12 +++++++----- 2 files changed, 16 insertions(+), 5 deletions(-) create mode 100644 prdoc/pr_2847.prdoc diff --git a/prdoc/pr_2847.prdoc b/prdoc/pr_2847.prdoc new file mode 100644 index 0000000000000..0d3580b60d052 --- /dev/null +++ b/prdoc/pr_2847.prdoc @@ -0,0 +1,9 @@ +title: Adding `try-state` hook to vesting pallet + +doc: + - audience: Runtime User + description: | + At the blocks before vesting begins, the locked amount of a vesting schedule must be product of the duration and the release per block amount when the locked amount is divisible by the per block amount, else the final vesting block should be equal to the unvested amount. This should also hold true during vesting shechules. + +crates: +- name: pallet-vesting diff --git a/substrate/frame/vesting/src/lib.rs b/substrate/frame/vesting/src/lib.rs index 0cf671f7c56fc..1e7b7646165f6 100644 --- a/substrate/frame/vesting/src/lib.rs +++ b/substrate/frame/vesting/src/lib.rs @@ -696,7 +696,8 @@ impl Pallet { /// * the locked amount of a vesting schedule must be equal to the /// product of the duration(`schedules_left` - `starting_block`) and the per block amount when /// the locked amount is divisible by the per block amount. -/// * However, If the locked amount is not divisible by the per block amount, the final vesting block +/// * However, If the locked amount is not divisible by the per block amount, the final vesting +/// block /// (`schedules_left` - 1), the unvested amount should be equal to the remainder. /// /// `handle_during_schedule`: @@ -717,8 +718,9 @@ impl Pallet { let schedules_left: BalanceOf = info.ending_block_as_balance::(); let starting_block = T::BlockNumberToBalance::convert(info.starting_block()); - let current_block_to_balance = - T::BlockNumberToBalance::convert(>::block_number()); + let current_block_to_balance = T::BlockNumberToBalance::convert( + T::BlockNumberProvider::current_block_number(), + ); if current_block_to_balance < starting_block { Self::handle_before_schedule_starts(info, starting_block, schedules_left)?; @@ -739,7 +741,7 @@ impl Pallet { if (info.locked() % info.per_block()).is_zero() { ensure!( - info.locked_at::(frame_system::Pallet::::block_number()) == (count * info.per_block()), + info.locked_at::(T::BlockNumberProvider::current_block_number()) == (count * info.per_block()), TryRuntimeError::Other("Before schedule starts, the vesting balance should be equal to the total per block releases") ); } else { @@ -772,7 +774,7 @@ impl Pallet { current_block_to_balance: BalanceOf, schedules_left: BalanceOf, ) -> Result<(), TryRuntimeError> { - let current_block = frame_system::Pallet::::block_number(); + let current_block = T::BlockNumberProvider::current_block_number(); let still_vesting = info.locked_at::(current_block); From 3a9a68daa4cb166b26842184f4ebbfef7e8eab2c Mon Sep 17 00:00:00 2001 From: doordashcon Date: Tue, 9 Jan 2024 06:10:20 +0100 Subject: [PATCH 5/8] update prdoc --- prdoc/pr_2847.prdoc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/prdoc/pr_2847.prdoc b/prdoc/pr_2847.prdoc index 0d3580b60d052..0f46c8b6bb276 100644 --- a/prdoc/pr_2847.prdoc +++ b/prdoc/pr_2847.prdoc @@ -3,7 +3,7 @@ title: Adding `try-state` hook to vesting pallet doc: - audience: Runtime User description: | - At the blocks before vesting begins, the locked amount of a vesting schedule must be product of the duration and the release per block amount when the locked amount is divisible by the per block amount, else the final vesting block should be equal to the unvested amount. This should also hold true during vesting shechules. + At the blocks before vesting begins, the locked amount of a vesting schedule must be equal to the product of the duration and the release per block amount when the locked amount is divisible by the per block amount, else the final vesting block should be equal to the unvested amount. This should also hold true during vesting schedules. crates: - name: pallet-vesting From 3e0784c65aa385b9617d89d235b8c1f5af1c8d9a Mon Sep 17 00:00:00 2001 From: doordashcon Date: Mon, 15 Jan 2024 06:09:25 +0100 Subject: [PATCH 6/8] wraps at 100chars, build_and_execute --- substrate/frame/vesting/src/lib.rs | 30 ++++--- substrate/frame/vesting/src/mock.rs | 8 +- substrate/frame/vesting/src/tests.rs | 118 +++++++++------------------ 3 files changed, 60 insertions(+), 96 deletions(-) diff --git a/substrate/frame/vesting/src/lib.rs b/substrate/frame/vesting/src/lib.rs index 1e7b7646165f6..e6836ff64a880 100644 --- a/substrate/frame/vesting/src/lib.rs +++ b/substrate/frame/vesting/src/lib.rs @@ -422,7 +422,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; @@ -530,7 +530,7 @@ impl Pallet { // Validate user inputs. ensure!(schedule.locked() >= T::MinVestedTransfer::get(), Error::::AmountLow); if !schedule.is_valid() { - return Err(Error::::InvalidScheduleParams.into()) + return Err(Error::::InvalidScheduleParams.into()); }; let target = T::Lookup::lookup(target)?; let source = T::Lookup::lookup(source)?; @@ -693,21 +693,19 @@ impl Pallet { /// ## Expectations: /// /// `handle_before_schedule_starts`: -/// * the locked amount of a vesting schedule must be equal to the -/// product of the duration(`schedules_left` - `starting_block`) and the per block amount when -/// the locked amount is divisible by the per block amount. +/// * the locked amount of a vesting schedule must be equal to the product of the duration +/// (`schedules_left` - `starting_block`) and the per block amount when the locked amount is +/// divisible by the per block amount. /// * However, If the locked amount is not divisible by the per block amount, the final vesting -/// block -/// (`schedules_left` - 1), the unvested amount should be equal to the remainder. +/// block (`schedules_left` - 1), the unvested amount should be equal to the remainder. /// /// `handle_during_schedule`: -/// * the amount `still_vesting` must be equal to the product of the remaining blocks to -/// vest(`schedules_left` - `current_block`) -/// and per block amount when the locked amount is divisible by the per block amount. +/// * the amount `still_vesting` must be equal to the product of the remaining blocks to vest +/// (`schedules_left` - `current_block`) and per block amount when the locked amount is divisible +/// by the per block amount. /// * However, If the locked amount is not divisible by the per block amount, then at the final -/// vesting block of the -/// current schedule (`schedules_left` - 1), the unvested amount should be equal to the -/// remainder. +/// vesting block of the current schedule (`schedules_left` - 1), the unvested amount should be +/// equal to the remainder. #[cfg(any(feature = "try-runtime", test))] impl Pallet { pub fn do_try_state() -> Result<(), TryRuntimeError> { @@ -840,13 +838,13 @@ where starting_block: BlockNumberFor, ) -> 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::::InvalidScheduleParams.into()) + return Err(Error::::InvalidScheduleParams.into()); }; let mut schedules = Self::vesting(who).unwrap_or_default(); @@ -874,7 +872,7 @@ where ) -> DispatchResult { // Check for `per_block` or `locked` of 0. if !VestingInfo::new(locked, per_block, starting_block).is_valid() { - return Err(Error::::InvalidScheduleParams.into()) + return Err(Error::::InvalidScheduleParams.into()); } ensure!( diff --git a/substrate/frame/vesting/src/mock.rs b/substrate/frame/vesting/src/mock.rs index 0701fcf6c38ff..234607de5f866 100644 --- a/substrate/frame/vesting/src/mock.rs +++ b/substrate/frame/vesting/src/mock.rs @@ -153,8 +153,14 @@ impl ExtBuilder { let mut ext = sp_io::TestExternalities::new(t); ext.execute_with(|| { System::set_block_number(1); - Vesting::do_try_state().unwrap(); }); ext } + + pub fn build_and_execute(self, test: impl FnOnce() -> ()) { + self.build().execute_with(|| { + test(); + Vesting::do_try_state().unwrap(); + }) + } } diff --git a/substrate/frame/vesting/src/tests.rs b/substrate/frame/vesting/src/tests.rs index 6ba20a0f5514f..3bb197a9bb467 100644 --- a/substrate/frame/vesting/src/tests.rs +++ b/substrate/frame/vesting/src/tests.rs @@ -38,13 +38,12 @@ where { // Its ok for this to fail because the user may already have no schedules. let _result = Vesting::vest(Some(account).into()); - assert_ok!(Vesting::do_try_state()); assert!(!>::contains_key(account)); } #[test] fn check_vesting_status() { - ExtBuilder::default().existential_deposit(ED).build().execute_with(|| { + ExtBuilder::default().existential_deposit(ED).build_and_execute(|| { let user1_free_balance = Balances::free_balance(&1); let user2_free_balance = Balances::free_balance(&2); let user12_free_balance = Balances::free_balance(&12); @@ -79,7 +78,6 @@ fn check_vesting_status() { System::set_block_number(10); assert_eq!(System::block_number(), 10); - assert_ok!(Vesting::do_try_state()); // Account 1 has fully vested by block 10 assert_eq!(Vesting::vesting_balance(&1), Some(0)); @@ -90,7 +88,6 @@ fn check_vesting_status() { System::set_block_number(30); assert_eq!(System::block_number(), 30); - assert_ok!(Vesting::do_try_state()); assert_eq!(Vesting::vesting_balance(&1), Some(0)); // Account 1 is still fully vested, and not negative assert_eq!(Vesting::vesting_balance(&2), Some(0)); // Account 2 has fully vested by block 30 @@ -105,7 +102,7 @@ fn check_vesting_status() { #[test] fn check_vesting_status_for_multi_schedule_account() { - ExtBuilder::default().existential_deposit(ED).build().execute_with(|| { + ExtBuilder::default().existential_deposit(ED).build_and_execute(|| { assert_eq!(System::block_number(), 1); let sched0 = VestingInfo::new( ED * 20, @@ -127,7 +124,7 @@ fn check_vesting_status_for_multi_schedule_account() { 0, ); assert_ok!(Vesting::vested_transfer(Some(4).into(), 2, sched1)); - assert_ok!(Vesting::do_try_state()); + // Free balance is equal to the two existing schedules total amount. let free_balance = Balances::free_balance(&2); assert_eq!(free_balance, ED * (10 + 20)); @@ -143,10 +140,9 @@ fn check_vesting_status_for_multi_schedule_account() { 5, ); assert_ok!(Vesting::vested_transfer(Some(4).into(), 2, sched2)); - assert_ok!(Vesting::do_try_state()); System::set_block_number(9); - assert_ok!(Vesting::do_try_state()); + // Free balance is equal to the 3 existing schedules total amount. let free_balance = Balances::free_balance(&2); assert_eq!(free_balance, ED * (10 + 20 + 30)); @@ -164,7 +160,6 @@ fn check_vesting_status_for_multi_schedule_account() { free_balance - sched1.locked() - sched2.per_block() * 15 - sched0.per_block() * 10 ) ); - assert_ok!(Vesting::do_try_state()); System::set_block_number(30); // At block #30 sched0 and sched1 are fully unlocked while sched2 is partially unlocked. @@ -172,12 +167,11 @@ fn check_vesting_status_for_multi_schedule_account() { Vesting::vesting_balance(&2), Some(free_balance - sched1.locked() - sched2.per_block() * 25 - sched0.locked()) ); - assert_ok!(Vesting::do_try_state()); // At block #35 sched2 fully unlocks and thus all schedules funds are unlocked. System::set_block_number(35); assert_eq!(Vesting::vesting_balance(&2), Some(0)); - assert_ok!(Vesting::do_try_state()); + // Since we have not called any extrinsics that would unlock funds the schedules // are still in storage, assert_eq!(Vesting::vesting(&2).unwrap(), vec![sched0, sched1, sched2]); @@ -188,7 +182,7 @@ fn check_vesting_status_for_multi_schedule_account() { #[test] fn unvested_balance_should_not_transfer() { - ExtBuilder::default().existential_deposit(10).build().execute_with(|| { + ExtBuilder::default().existential_deposit(10).build_and_execute(|| { let user1_free_balance = Balances::free_balance(&1); assert_eq!(user1_free_balance, 100); // Account 1 has free balance // Account 1 has only 5 units vested at block 1 (plus 50 unvested) @@ -200,23 +194,22 @@ fn unvested_balance_should_not_transfer() { #[test] fn vested_balance_should_transfer() { - ExtBuilder::default().existential_deposit(10).build().execute_with(|| { + ExtBuilder::default().existential_deposit(10).build_and_execute(|| { let user1_free_balance = Balances::free_balance(&1); assert_eq!(user1_free_balance, 100); // Account 1 has free balance // Account 1 has only 5 units vested at block 1 (plus 50 unvested) assert_eq!(Vesting::vesting_balance(&1), Some(45)); assert_ok!(Vesting::vest(Some(1).into())); - assert_ok!(Vesting::do_try_state()); assert_ok!(Balances::transfer_allow_death(Some(1).into(), 2, 55)); }); } #[test] fn vested_balance_should_transfer_with_multi_sched() { - ExtBuilder::default().existential_deposit(ED).build().execute_with(|| { + ExtBuilder::default().existential_deposit(ED).build_and_execute(|| { let sched0 = VestingInfo::new(5 * ED, 128, 0); assert_ok!(Vesting::vested_transfer(Some(13).into(), 1, sched0)); - assert_ok!(Vesting::do_try_state()); + // Total 10*ED locked for all the schedules. assert_eq!(Vesting::vesting(&1).unwrap(), vec![sched0, sched0]); @@ -226,14 +219,13 @@ fn vested_balance_should_transfer_with_multi_sched() { // Account 1 has only 256 units unlocking at block 1 (plus 1280 already fee). assert_eq!(Vesting::vesting_balance(&1), Some(2304)); assert_ok!(Vesting::vest(Some(1).into())); - assert_ok!(Vesting::do_try_state()); assert_ok!(Balances::transfer_allow_death(Some(1).into(), 2, 1536)); }); } #[test] fn non_vested_cannot_vest() { - ExtBuilder::default().existential_deposit(ED).build().execute_with(|| { + ExtBuilder::default().existential_deposit(ED).build_and_execute(|| { assert!(!>::contains_key(4)); assert_noop!(Vesting::vest(Some(4).into()), Error::::NotVesting); }); @@ -241,20 +233,19 @@ fn non_vested_cannot_vest() { #[test] fn vested_balance_should_transfer_using_vest_other() { - ExtBuilder::default().existential_deposit(10).build().execute_with(|| { + ExtBuilder::default().existential_deposit(10).build_and_execute(|| { let user1_free_balance = Balances::free_balance(&1); assert_eq!(user1_free_balance, 100); // Account 1 has free balance // Account 1 has only 5 units vested at block 1 (plus 50 unvested) assert_eq!(Vesting::vesting_balance(&1), Some(45)); assert_ok!(Vesting::vest_other(Some(2).into(), 1)); - assert_ok!(Vesting::do_try_state()); assert_ok!(Balances::transfer_allow_death(Some(1).into(), 2, 55)); }); } #[test] fn vested_balance_should_transfer_using_vest_other_with_multi_sched() { - ExtBuilder::default().existential_deposit(ED).build().execute_with(|| { + ExtBuilder::default().existential_deposit(ED).build_and_execute(|| { let sched0 = VestingInfo::new(5 * ED, 128, 0); assert_ok!(Vesting::vested_transfer(Some(13).into(), 1, sched0)); // Total of 10*ED of locked for all the schedules. @@ -266,14 +257,13 @@ fn vested_balance_should_transfer_using_vest_other_with_multi_sched() { // Account 1 has only 256 units unlocking at block 1 (plus 1280 already free). assert_eq!(Vesting::vesting_balance(&1), Some(2304)); assert_ok!(Vesting::vest_other(Some(2).into(), 1)); - assert_ok!(Vesting::do_try_state()); assert_ok!(Balances::transfer_allow_death(Some(1).into(), 2, 1536)); }); } #[test] fn non_vested_cannot_vest_other() { - ExtBuilder::default().existential_deposit(ED).build().execute_with(|| { + ExtBuilder::default().existential_deposit(ED).build_and_execute(|| { assert!(!>::contains_key(4)); assert_noop!(Vesting::vest_other(Some(3).into(), 4), Error::::NotVesting); }); @@ -281,7 +271,7 @@ fn non_vested_cannot_vest_other() { #[test] fn extra_balance_should_transfer() { - ExtBuilder::default().existential_deposit(10).build().execute_with(|| { + ExtBuilder::default().existential_deposit(10).build_and_execute(|| { assert_ok!(Balances::transfer_allow_death(Some(3).into(), 1, 100)); assert_ok!(Balances::transfer_allow_death(Some(3).into(), 2, 100)); @@ -294,13 +284,11 @@ fn extra_balance_should_transfer() { // Account 1 has only 5 units vested at block 1 (plus 150 unvested) assert_eq!(Vesting::vesting_balance(&1), Some(45)); assert_ok!(Vesting::vest(Some(1).into())); - assert_ok!(Vesting::do_try_state()); assert_ok!(Balances::transfer_allow_death(Some(1).into(), 3, 155)); // Account 1 can send extra units gained // Account 2 has no units vested at block 1, but gained 100 assert_eq!(Vesting::vesting_balance(&2), Some(200)); assert_ok!(Vesting::vest(Some(2).into())); - assert_ok!(Vesting::do_try_state()); assert_ok!(Balances::transfer_allow_death(Some(2).into(), 3, 100)); // Account 2 can send extra // units gained }); @@ -308,7 +296,7 @@ fn extra_balance_should_transfer() { #[test] fn liquid_funds_should_transfer_with_delayed_vesting() { - ExtBuilder::default().existential_deposit(256).build().execute_with(|| { + ExtBuilder::default().existential_deposit(256).build_and_execute(|| { let user12_free_balance = Balances::free_balance(&12); assert_eq!(user12_free_balance, 2560); // Account 12 has free balance @@ -330,7 +318,7 @@ fn liquid_funds_should_transfer_with_delayed_vesting() { #[test] fn vested_transfer_works() { - ExtBuilder::default().existential_deposit(256).build().execute_with(|| { + ExtBuilder::default().existential_deposit(256).build_and_execute(|| { let user3_free_balance = Balances::free_balance(&3); let user4_free_balance = Balances::free_balance(&4); assert_eq!(user3_free_balance, 256 * 30); @@ -344,7 +332,7 @@ fn vested_transfer_works() { 10, ); assert_ok!(Vesting::vested_transfer(Some(3).into(), 4, new_vesting_schedule)); - assert_ok!(Vesting::do_try_state()); + // Now account 4 should have vesting. assert_eq!(Vesting::vesting(&4).unwrap(), vec![new_vesting_schedule]); // Ensure the transfer happened correctly. @@ -357,14 +345,12 @@ fn vested_transfer_works() { System::set_block_number(20); assert_eq!(System::block_number(), 20); - assert_ok!(Vesting::do_try_state()); // Account 4 has 5 * 64 units vested by block 20. assert_eq!(Vesting::vesting_balance(&4), Some(10 * 64)); System::set_block_number(30); assert_eq!(System::block_number(), 30); - assert_ok!(Vesting::do_try_state()); // Account 4 has fully vested, assert_eq!(Vesting::vesting_balance(&4), Some(0)); @@ -375,7 +361,7 @@ fn vested_transfer_works() { #[test] fn vested_transfer_correctly_fails() { - ExtBuilder::default().existential_deposit(ED).build().execute_with(|| { + ExtBuilder::default().existential_deposit(ED).build_and_execute(|| { let user2_free_balance = Balances::free_balance(&2); let user4_free_balance = Balances::free_balance(&4); assert_eq!(user2_free_balance, ED * 20); @@ -422,7 +408,7 @@ fn vested_transfer_correctly_fails() { #[test] fn vested_transfer_allows_max_schedules() { - ExtBuilder::default().existential_deposit(ED).build().execute_with(|| { + ExtBuilder::default().existential_deposit(ED).build_and_execute(|| { let mut user_4_free_balance = Balances::free_balance(&4); let max_schedules = ::MAX_VESTING_SCHEDULES; let sched = VestingInfo::new( @@ -463,7 +449,7 @@ fn vested_transfer_allows_max_schedules() { #[test] fn force_vested_transfer_works() { - ExtBuilder::default().existential_deposit(ED).build().execute_with(|| { + ExtBuilder::default().existential_deposit(ED).build_and_execute(|| { let user3_free_balance = Balances::free_balance(&3); let user4_free_balance = Balances::free_balance(&4); assert_eq!(user3_free_balance, ED * 30); @@ -500,14 +486,12 @@ fn force_vested_transfer_works() { System::set_block_number(20); assert_eq!(System::block_number(), 20); - assert_ok!(Vesting::do_try_state()); // Account 4 has 5 * 64 units vested by block 20. assert_eq!(Vesting::vesting_balance(&4), Some(10 * 64)); System::set_block_number(30); assert_eq!(System::block_number(), 30); - assert_ok!(Vesting::do_try_state()); // Account 4 has fully vested, assert_eq!(Vesting::vesting_balance(&4), Some(0)); @@ -518,7 +502,7 @@ fn force_vested_transfer_works() { #[test] fn force_vested_transfer_correctly_fails() { - ExtBuilder::default().existential_deposit(ED).build().execute_with(|| { + ExtBuilder::default().existential_deposit(ED).build_and_execute(|| { let user2_free_balance = Balances::free_balance(&2); let user4_free_balance = Balances::free_balance(&4); assert_eq!(user2_free_balance, ED * 20); @@ -569,7 +553,7 @@ fn force_vested_transfer_correctly_fails() { #[test] fn force_vested_transfer_allows_max_schedules() { - ExtBuilder::default().existential_deposit(ED).build().execute_with(|| { + ExtBuilder::default().existential_deposit(ED).build_and_execute(|| { let mut user_4_free_balance = Balances::free_balance(&4); let max_schedules = ::MAX_VESTING_SCHEDULES; let sched = VestingInfo::new( @@ -601,7 +585,7 @@ fn force_vested_transfer_allows_max_schedules() { // Account 4 has fully vested when all the schedules end, System::set_block_number(::MinVestedTransfer::get() + 10); assert_eq!(Vesting::vesting_balance(&4), Some(0)); - assert_ok!(Vesting::do_try_state()); + // and after unlocking its schedules are removed from storage. vest_and_assert_no_vesting::(4); }); @@ -609,7 +593,7 @@ fn force_vested_transfer_allows_max_schedules() { #[test] fn merge_schedules_that_have_not_started() { - ExtBuilder::default().existential_deposit(ED).build().execute_with(|| { + ExtBuilder::default().existential_deposit(ED).build_and_execute(|| { // Account 2 should already have a vesting schedule. let sched0 = VestingInfo::new( ED * 20, @@ -643,7 +627,7 @@ fn merge_schedules_that_have_not_started() { #[test] fn merge_ongoing_schedules() { // Merging two schedules that have started will vest both before merging. - ExtBuilder::default().existential_deposit(ED).build().execute_with(|| { + ExtBuilder::default().existential_deposit(ED).build_and_execute(|| { // Account 2 should already have a vesting schedule. let sched0 = VestingInfo::new( ED * 20, @@ -658,20 +642,17 @@ fn merge_ongoing_schedules() { sched0.starting_block() + 5, // Start at block 15. ); assert_ok!(Vesting::vested_transfer(Some(4).into(), 2, sched1)); - assert_ok!(Vesting::do_try_state()); assert_eq!(Vesting::vesting(&2).unwrap(), vec![sched0, sched1]); // Got to half way through the second schedule where both schedules are actively vesting. let cur_block = 20; System::set_block_number(cur_block); - assert_ok!(Vesting::do_try_state()); // Account 2 has no usable balances prior to the merge because they have not unlocked // with `vest` yet. assert_eq!(Balances::usable_balance(&2), 0); assert_ok!(Vesting::merge_schedules(Some(2).into(), 0, 1)); - assert_ok!(Vesting::do_try_state()); // Merging schedules un-vests all pre-existing schedules prior to merging, which is // reflected in account 2's updated usable balance. @@ -697,7 +678,6 @@ fn merge_ongoing_schedules() { // And just to double check, we assert the new merged schedule we be cleaned up as expected. System::set_block_number(30); - assert_ok!(Vesting::do_try_state()); vest_and_assert_no_vesting::(2); }); } @@ -706,7 +686,7 @@ fn merge_ongoing_schedules() { fn merging_shifts_other_schedules_index() { // Schedules being merged are filtered out, schedules to the right of any merged // schedule shift left and the merged schedule is always last. - ExtBuilder::default().existential_deposit(ED).build().execute_with(|| { + ExtBuilder::default().existential_deposit(ED).build_and_execute(|| { let sched0 = VestingInfo::new( ED * 10, ED, // Vesting over 10 blocks. @@ -731,15 +711,11 @@ fn merging_shifts_other_schedules_index() { let cur_block = 1; assert_eq!(System::block_number(), cur_block); - assert_ok!(Vesting::do_try_state()); // Transfer the above 3 schedules to account 3. assert_ok!(Vesting::vested_transfer(Some(4).into(), 3, sched0)); - assert_ok!(Vesting::do_try_state()); assert_ok!(Vesting::vested_transfer(Some(4).into(), 3, sched1)); - assert_ok!(Vesting::do_try_state()); assert_ok!(Vesting::vested_transfer(Some(4).into(), 3, sched2)); - assert_ok!(Vesting::do_try_state()); // With no schedules vested or merged they are in the order they are created assert_eq!(Vesting::vesting(&3).unwrap(), vec![sched0, sched1, sched2]); @@ -747,7 +723,6 @@ fn merging_shifts_other_schedules_index() { assert_eq!(usable_balance, Balances::usable_balance(&3)); assert_ok!(Vesting::merge_schedules(Some(3).into(), 0, 2)); - assert_ok!(Vesting::do_try_state()); // Create the merged schedule of sched0 & sched2. // The merged schedule will have the max possible starting block, @@ -774,7 +749,7 @@ fn merging_shifts_other_schedules_index() { fn merge_ongoing_and_yet_to_be_started_schedules() { // Merge an ongoing schedule that has had `vest` called and a schedule that has not already // started. - ExtBuilder::default().existential_deposit(ED).build().execute_with(|| { + ExtBuilder::default().existential_deposit(ED).build_and_execute(|| { // Account 2 should already have a vesting schedule. let sched0 = VestingInfo::new( ED * 20, @@ -788,7 +763,6 @@ fn merge_ongoing_and_yet_to_be_started_schedules() { (sched0.starting_block() + sched0.ending_block_as_balance::()) / 2; assert_eq!(cur_block, 20); System::set_block_number(cur_block); - assert_ok!(Vesting::do_try_state()); // Prior to vesting there is no usable balance. let mut usable_balance = 0; @@ -812,11 +786,10 @@ fn merge_ongoing_and_yet_to_be_started_schedules() { cur_block + 1, ); assert_ok!(Vesting::vested_transfer(Some(4).into(), 2, sched1)); - assert_ok!(Vesting::do_try_state()); // Merge the schedules before sched1 starts. assert_ok!(Vesting::merge_schedules(Some(2).into(), 0, 1)); - assert_ok!(Vesting::do_try_state()); + // After merging, the usable balance only changes by the amount sched0 vested since we // last called `vest` (which is just 1 block). The usable balance is not affected by // sched1 because it has not started yet. @@ -844,7 +817,7 @@ fn merge_ongoing_and_yet_to_be_started_schedules() { fn merge_finished_and_ongoing_schedules() { // If a schedule finishes by the current block we treat the ongoing schedule, // without any alterations, as the merged one. - ExtBuilder::default().existential_deposit(ED).build().execute_with(|| { + ExtBuilder::default().existential_deposit(ED).build_and_execute(|| { // Account 2 should already have a vesting schedule. let sched0 = VestingInfo::new( ED * 20, @@ -859,7 +832,6 @@ fn merge_finished_and_ongoing_schedules() { 10, ); assert_ok!(Vesting::vested_transfer(Some(4).into(), 2, sched1)); - assert_ok!(Vesting::do_try_state()); // Transfer a 3rd schedule, so we can demonstrate how schedule indices change. // (We are not merging this schedule.) @@ -869,7 +841,6 @@ fn merge_finished_and_ongoing_schedules() { 10, ); assert_ok!(Vesting::vested_transfer(Some(3).into(), 2, sched2)); - assert_ok!(Vesting::do_try_state()); // The schedules are in expected order prior to merging. assert_eq!(Vesting::vesting(&2).unwrap(), vec![sched0, sched1, sched2]); @@ -878,13 +849,11 @@ fn merge_finished_and_ongoing_schedules() { let cur_block = sched0.ending_block_as_balance::(); System::set_block_number(cur_block); assert_eq!(System::block_number(), 30); - assert_ok!(Vesting::do_try_state()); // Prior to `merge_schedules` and with no vest/vest_other called the user has no usable // balance. assert_eq!(Balances::usable_balance(&2), 0); assert_ok!(Vesting::merge_schedules(Some(2).into(), 0, 1)); - assert_ok!(Vesting::do_try_state()); // sched2 is now the first, since sched0 & sched1 get filtered out while "merging". // sched1 gets treated like the new merged schedule by getting pushed onto back @@ -910,7 +879,7 @@ fn merge_finished_and_ongoing_schedules() { #[test] fn merge_finishing_schedules_does_not_create_a_new_one() { // If both schedules finish by the current block we don't create new one - ExtBuilder::default().existential_deposit(ED).build().execute_with(|| { + ExtBuilder::default().existential_deposit(ED).build_and_execute(|| { // Account 2 should already have a vesting schedule. let sched0 = VestingInfo::new( ED * 20, @@ -926,7 +895,6 @@ fn merge_finishing_schedules_does_not_create_a_new_one() { 10, ); assert_ok!(Vesting::vested_transfer(Some(3).into(), 2, sched1)); - assert_ok!(Vesting::do_try_state()); assert_eq!(Vesting::vesting(&2).unwrap(), vec![sched0, sched1]); let all_scheds_end = sched0 @@ -935,7 +903,6 @@ fn merge_finishing_schedules_does_not_create_a_new_one() { assert_eq!(all_scheds_end, 40); System::set_block_number(all_scheds_end); - assert_ok!(Vesting::do_try_state()); // Prior to merge_schedules and with no vest/vest_other called the user has no usable // balance. @@ -943,7 +910,6 @@ fn merge_finishing_schedules_does_not_create_a_new_one() { // Merge schedule 0 and 1. assert_ok!(Vesting::merge_schedules(Some(2).into(), 0, 1)); - assert_ok!(Vesting::do_try_state()); // The user no longer has any more vesting schedules because they both ended at the // block they where merged, assert!(!>::contains_key(&2)); @@ -955,7 +921,7 @@ fn merge_finishing_schedules_does_not_create_a_new_one() { #[test] fn merge_finished_and_yet_to_be_started_schedules() { - ExtBuilder::default().existential_deposit(ED).build().execute_with(|| { + ExtBuilder::default().existential_deposit(ED).build_and_execute(|| { // Account 2 should already have a vesting schedule. let sched0 = VestingInfo::new( ED * 20, @@ -970,7 +936,6 @@ fn merge_finished_and_yet_to_be_started_schedules() { 35, ); assert_ok!(Vesting::vested_transfer(Some(13).into(), 2, sched1)); - assert_ok!(Vesting::do_try_state()); assert_eq!(Vesting::vesting(&2).unwrap(), vec![sched0, sched1]); let sched2 = VestingInfo::new( @@ -980,11 +945,9 @@ fn merge_finished_and_yet_to_be_started_schedules() { ); // Add a 3rd schedule to demonstrate how sched1 shifts. assert_ok!(Vesting::vested_transfer(Some(13).into(), 2, sched2)); - assert_ok!(Vesting::do_try_state()); assert_eq!(Vesting::vesting(&2).unwrap(), vec![sched0, sched1, sched2]); System::set_block_number(30); - assert_ok!(Vesting::do_try_state()); // At block 30, sched0 has finished unlocking while sched1 and sched2 are still fully // locked, @@ -994,7 +957,6 @@ fn merge_finished_and_yet_to_be_started_schedules() { // Merge schedule 0 and 1. assert_ok!(Vesting::merge_schedules(Some(2).into(), 0, 1)); - assert_ok!(Vesting::do_try_state()); // sched0 is removed since it finished, and sched1 is removed and then pushed on the back // because it is treated as the merged schedule @@ -1007,7 +969,7 @@ fn merge_finished_and_yet_to_be_started_schedules() { #[test] fn merge_schedules_throws_proper_errors() { - ExtBuilder::default().existential_deposit(ED).build().execute_with(|| { + ExtBuilder::default().existential_deposit(ED).build_and_execute(|| { // Account 2 should already have a vesting schedule. let sched0 = VestingInfo::new( ED * 20, @@ -1058,8 +1020,7 @@ fn generates_multiple_schedules_from_genesis_config() { ExtBuilder::default() .existential_deposit(ED) .vesting_genesis_config(vesting_config) - .build() - .execute_with(|| { + .build_and_execute(|| { let user1_sched1 = VestingInfo::new(5 * ED, 128, 0u64); assert_eq!(Vesting::vesting(&1).unwrap(), vec![user1_sched1]); @@ -1092,14 +1053,14 @@ fn multiple_schedules_from_genesis_config_errors() { #[test] fn build_genesis_has_storage_version_v1() { - ExtBuilder::default().existential_deposit(ED).build().execute_with(|| { + ExtBuilder::default().existential_deposit(ED).build_and_execute(|| { assert_eq!(StorageVersion::::get(), Releases::V1); }); } #[test] fn merge_vesting_handles_per_block_0() { - ExtBuilder::default().existential_deposit(ED).build().execute_with(|| { + ExtBuilder::default().existential_deposit(ED).build_and_execute(|| { let sched0 = VestingInfo::new( ED, 0, // Vesting over 256 blocks. 1, @@ -1178,7 +1139,7 @@ fn per_block_works() { // When an accounts free balance + schedule.locked is less than ED, the vested transfer will fail. #[test] fn vested_transfer_less_than_existential_deposit_fails() { - ExtBuilder::default().existential_deposit(4 * ED).build().execute_with(|| { + ExtBuilder::default().existential_deposit(4 * ED).build_and_execute(|| { // MinVestedTransfer is less the ED. assert!( ::Currency::minimum_balance() > @@ -1205,7 +1166,7 @@ fn vested_transfer_less_than_existential_deposit_fails() { #[test] fn remove_vesting_schedule() { - ExtBuilder::default().existential_deposit(ED).build().execute_with(|| { + ExtBuilder::default().existential_deposit(ED).build_and_execute(|| { assert_eq!(Balances::free_balance(&3), 256 * 30); assert_eq!(Balances::free_balance(&4), 256 * 40); // Account 4 should not have any vesting yet. @@ -1217,7 +1178,7 @@ fn remove_vesting_schedule() { 10, ); assert_ok!(Vesting::vested_transfer(Some(3).into(), 4, new_vesting_schedule)); - assert_ok!(Vesting::do_try_state()); + // Now account 4 should have vesting. assert_eq!(Vesting::vesting(&4).unwrap(), vec![new_vesting_schedule]); // Account 4 has 5 * 256 locked. @@ -1259,8 +1220,7 @@ fn play_out_all_schedules() { ExtBuilder::default() .existential_deposit(ED) .vesting_genesis_config(vesting_config) - .build() - .execute_with(|| { + .build_and_execute(|| { let user1_sched1 = VestingInfo::new(5 * ED, 128, 0u64); assert_eq!(Vesting::vesting(&1).unwrap(), vec![user1_sched1]); From c1d29431708dcfb9c1f0c73d3d54d67ac3b6f480 Mon Sep 17 00:00:00 2001 From: doordashcon Date: Fri, 2 Feb 2024 16:56:06 +0100 Subject: [PATCH 7/8] T::Currency::free_balance --- substrate/frame/vesting/src/lib.rs | 86 ++++++++++++++++++++---------- 1 file changed, 59 insertions(+), 27 deletions(-) diff --git a/substrate/frame/vesting/src/lib.rs b/substrate/frame/vesting/src/lib.rs index e6836ff64a880..24019b6742cea 100644 --- a/substrate/frame/vesting/src/lib.rs +++ b/substrate/frame/vesting/src/lib.rs @@ -422,7 +422,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; @@ -530,7 +530,7 @@ impl Pallet { // Validate user inputs. ensure!(schedule.locked() >= T::MinVestedTransfer::get(), Error::::AmountLow); if !schedule.is_valid() { - return Err(Error::::InvalidScheduleParams.into()); + return Err(Error::::InvalidScheduleParams.into()) }; let target = T::Lookup::lookup(target)?; let source = T::Lookup::lookup(source)?; @@ -709,21 +709,50 @@ impl Pallet { #[cfg(any(feature = "try-runtime", test))] impl Pallet { pub fn do_try_state() -> Result<(), TryRuntimeError> { - for (_, d) in Vesting::::iter() { - let infos = d.to_vec(); - - for info in infos.iter() { - let schedules_left: BalanceOf = - info.ending_block_as_balance::(); - let starting_block = T::BlockNumberToBalance::convert(info.starting_block()); - let current_block_to_balance = T::BlockNumberToBalance::convert( - T::BlockNumberProvider::current_block_number(), - ); + for who in Vesting::::iter_keys() { + if let Some(infos) = Vesting::::get(who.clone()) { + // Accumulate the total locked + let mut total_locked_amount: BalanceOf = Zero::zero(); + + for info in infos.iter() { + let schedules_left: BalanceOf = + info.ending_block_as_balance::(); + let starting_block = T::BlockNumberToBalance::convert(info.starting_block()); + let current_block_to_balance = T::BlockNumberToBalance::convert( + T::BlockNumberProvider::current_block_number(), + ); + + if current_block_to_balance < starting_block { + // handle the case when vesting has not started for this schedule + match Self::handle_before_schedule_starts( + info, + starting_block, + schedules_left, + ) { + Ok(schedule_locked_amount) => + total_locked_amount += schedule_locked_amount, + Err(e) => return Err(e), + } + } else { + match Self::handle_during_schedule( + info, + current_block_to_balance, + schedules_left, + ) { + Ok(schedule_locked_amount) => + total_locked_amount += schedule_locked_amount, + Err(e) => return Err(e), + } + } + } - if current_block_to_balance < starting_block { - Self::handle_before_schedule_starts(info, starting_block, schedules_left)?; + if let Some(vesting_balance) = Self::vesting_balance(&who) { + ensure!( + vesting_balance == total_locked_amount, + TryRuntimeError::Other("inconsistent locked amount") + ); } else { - Self::handle_during_schedule(info, current_block_to_balance, schedules_left)?; + TryRuntimeError::Other("Account has no schedules"); } } } @@ -734,14 +763,18 @@ impl Pallet { info: &VestingInfo, BlockNumberFor>, starting_block: BalanceOf, schedules_left: BalanceOf, - ) -> Result<(), TryRuntimeError> { + ) -> Result, TryRuntimeError> { let count = schedules_left.saturating_sub(starting_block); + let still_vesting = info + .locked_at::(T::BlockNumberProvider::current_block_number()); + if (info.locked() % info.per_block()).is_zero() { ensure!( - info.locked_at::(T::BlockNumberProvider::current_block_number()) == (count * info.per_block()), + still_vesting == (count * info.per_block()), TryRuntimeError::Other("Before schedule starts, the vesting balance should be equal to the total per block releases") ); + return Ok(still_vesting) } else { let re = info.locked() % info.per_block(); @@ -762,25 +795,24 @@ impl Pallet { no_locks == Zero::zero(), TryRuntimeError::Other("After all schedules, all amounts should be unlocked") ); + return Ok(still_vesting) } - - Ok(()) } fn handle_during_schedule( info: &VestingInfo, BlockNumberFor>, current_block_to_balance: BalanceOf, schedules_left: BalanceOf, - ) -> Result<(), TryRuntimeError> { - let current_block = T::BlockNumberProvider::current_block_number(); - - let still_vesting = info.locked_at::(current_block); + ) -> Result, TryRuntimeError> { + let still_vesting = info + .locked_at::(T::BlockNumberProvider::current_block_number()); if (info.locked() % info.per_block()).is_zero() { ensure!( still_vesting == (schedules_left.saturating_sub(current_block_to_balance) * info.per_block()), TryRuntimeError::Other("during schedules, the vesting balance should be equal to the total per block releases") ); + return Ok(still_vesting) } else { let re = info.locked() % info.per_block(); @@ -794,8 +826,8 @@ impl Pallet { TryRuntimeError::Other("Schedule ended, no more vesting balance") ); } + return Ok(still_vesting) } - Ok(()) } } @@ -838,13 +870,13 @@ where starting_block: BlockNumberFor, ) -> 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::::InvalidScheduleParams.into()); + return Err(Error::::InvalidScheduleParams.into()) }; let mut schedules = Self::vesting(who).unwrap_or_default(); @@ -872,7 +904,7 @@ where ) -> DispatchResult { // Check for `per_block` or `locked` of 0. if !VestingInfo::new(locked, per_block, starting_block).is_valid() { - return Err(Error::::InvalidScheduleParams.into()); + return Err(Error::::InvalidScheduleParams.into()) } ensure!( From 3e05336f05a3d76302f904488a281322de008b43 Mon Sep 17 00:00:00 2001 From: doordashcon Date: Fri, 9 Feb 2024 07:19:41 +0100 Subject: [PATCH 8/8] remove no effect --- substrate/frame/vesting/src/lib.rs | 2 -- 1 file changed, 2 deletions(-) diff --git a/substrate/frame/vesting/src/lib.rs b/substrate/frame/vesting/src/lib.rs index 24019b6742cea..d03c98d636ec0 100644 --- a/substrate/frame/vesting/src/lib.rs +++ b/substrate/frame/vesting/src/lib.rs @@ -751,8 +751,6 @@ impl Pallet { vesting_balance == total_locked_amount, TryRuntimeError::Other("inconsistent locked amount") ); - } else { - TryRuntimeError::Other("Account has no schedules"); } } }