pallet-vesting: Support multiple, merge-able vesting schedules (#9202)

* Support multiple, mergable vesting schedules

* Update node runtime

* Remove some TODO design questions and put them as commennts

* Update frame/vesting/src/benchmarking.rs

* Syntax and comment clean up

* Create filter enum for removing schedules

* Dry vesting calls with do_vest

* Improve old benchmarks to account for max schedules

* Update WeightInfo trait and make dummy fns

* Add merge_schedule weights

* Explicitly test multiple vesting scheudles

* Make new vesting tests more more clear

* Apply suggestions from code review

* Update remove_vesting_schedule to error with no index

* Try reduce spacing diff

* Apply suggestions from code review

* Use get on vesting for bounds check; check origin first

* No filter tuple; various simplifications

* unwrap or default when getting user schedules

* spaces be gone

* ReadMe fixes

* Update frame/vesting/src/lib.rs

Co-authored-by: Peter Goodspeed-Niklaus <coriolinus@users.noreply.github.com>

* address some comments for docs

* merge sched docs

* Apply suggestions from code review

Co-authored-by: Shawn Tabrizi <shawntabrizi@gmail.com>

* log error when trying to push to vesting vec

* use let Some, not is_some

* remove_vesting_schedule u32, not optin

* new not try_new, create validate builder; VestingInfo

* Merge prep: break out tests and mock

* Add files forgot to include in merge

* revert some accidental changes to merged files

* Revert remaining accidental file changes

* More revert of accidental file change

* Try to reduce diff on tests

* namespace Vesting; check key when key should not exist;

* ending_block throws error on per_block of 0

* Try improve merge vesting info comment

* Update frame/vesting/src/lib.rs

Co-authored-by: Kian Paimani <5588131+kianenigma@users.noreply.github.com>

* add validate + correct; handle duration > blocknumber

* Move vesting_info module to its own file

* Seperate Vesting/locks updates from writing

* Add can_add_vesting schedule

* Adjust min vested transfer to be greater than all ED

* Initial integrity test impl

* merge_finished_and_yet_to_be_started_schedules

* Make sure to assert storage items are cleaned up

* Migration initial impl (not tested)

* Correct try-runtime hooks

* Apply suggestions from code review

Co-authored-by: Shawn Tabrizi <shawntabrizi@gmail.com>

* header

* WIP: improve benchmarks

* Benchmarking working

* benchmarking: step over max schedules

* cargo run --release --features=runtime-benchmarks --manifest-path=bin/node/cli/Cargo.toml -- benchmark --chain=dev --steps=50 --repeat=20 --pallet=pallet_vesting --extrinsic=* --execution=wasm --wasm-execution=compiled --heap-pages=4096 --output=./frame/vesting/src/weights.rs --template=./.maintain/frame-weight-template.hbs

* Simplify APIs by accepting vec; convert to bounded on write

* Test:  build_genesis_has_storage_version_v1

* Test more error cases

* Hack to get polkadot weights to work; should revert later

* Improve benchmarking; works on polkadot

* cargo run --release --features=runtime-benchmarks --manifest-path=bin/node/cli/Cargo.toml -- benchmark --chain=dev --steps=50 --repeat=20 --pallet=pallet_vesting --extrinsic=* --execution=wasm --wasm-execution=compiled --heap-pages=4096 --output=./frame/vesting/src/weights.rs --template=./.maintain/frame-weight-template.hbs

* WIP override storage

* Set storage not working example

* Remove unused tests

* VestingInfo: make public, derive MaxEndcodedLen

* Rename ending_block to ending_block_as_balance

* Superificial improvements

* Check for end block infinite, not just duration

* More superficial update

* Update tests

* Test vest with multi schedule

* Don't use half max balance in benchmarks

* Use debug_assert when locked is unexpected 0

* Implement exec_action

* Simplify per_block calc in vesting_info

* VestingInfo.validate in add_vesting_schedule & can_add_vesting_schedule

* Simplify post migrate check

* Remove merge event

* Minor benchmarking updates

* Remove VestingInfo.correct

* per_block accesor max with 1

* Improve comment

* Remoe debug

* Fix add schedule comment

* Apply suggestions from code review

Co-authored-by: Peter Goodspeed-Niklaus <coriolinus@users.noreply.github.com>

* no ref for should_remove param

* Remove unused vestingaction derive

* Asserts to show balance unlock in merge benchmark

* Remove unused imports

* trivial

* Fix benchmark asserts to handle non-multiple of 20 locked

* Add generate_storage_info

* migration :facepalm

* Remove per_block 0 logic

* Update frame/vesting/src/lib.rs

* Do not check for ending later than greatest block

* Apply suggestions from code review

* Benchmarks: simplify vesting schedule creation

* Add log back for migration

* Add note in ext docs explaining that all schedules will vest

* Make integrity test work

* Improve integrity test

* Remove unnescary type param from VestingInfo::new

* Remove unnescary resut for ending_block_as_balance

* Remove T param from ending_block_as_balance

* Reduce visibility of raw_per_block

* Remove unused type param for validate

* update old comment

* Make log a dep; log warn in migrate

* VestingInfo.validate returns Err(()), no T type param

* Try improve report_schedule_updates

* is_valid, not validate

* revert node runtime reorg;

* change schedule validity check to just warning

* Simplify merge_vesting_info return type

* Apply suggestions from code review

* Apply suggestions from code review

Co-authored-by: Kian Paimani <5588131+kianenigma@users.noreply.github.com>

* Add warning for migration

* Fix indentation

* Delete duplicate warnings

* Reduce diff in node runtime

* Fix benchmark build

* Upgrade cargo.toml to use 4.0.0-dev

* Cleanup

* MaxVestingSchedulesGetter initial impl

* MinVestedTransfer getter inintial impl

* Test MaxVestingSchedules & MinVestedTransfer getters; use getters in benchmarks

* Run cargo fmt

* Revert MinVestedTransfer & MaxVestingSchedules getters; Add integrity test

* Make MAX_VESTING_SCHEDULES a const

* fmt

* WIP: benchmark improvements

* Finish benchmark update

* Add test for transfer to account with less than ed

* Rm min_new_account_transfer; move sp-io to dev-dep

* Reduce cargo.toml diff

* Explain MAX_VESTING_SCHEDULES choice

* Fix after merge

* Try fix CI complaints

* cargo run --quiet --release --features=runtime-benchmarks --manifest-path=bin/node/cli/Cargo.toml -- benchmark --chain=dev --steps=50 --repeat=20 --pallet=pallet_vesting --extrinsic=* --execution=wasm --wasm-execution=compiled --heap-pages=4096 --output=./frame/vesting/src/weights.rs --template=./.maintain/frame-weight-template.hbs

* cargo run --quiet --release --features=runtime-benchmarks --manifest-path=bin/node/cli/Cargo.toml -- benchmark --chain=dev --steps=50 --repeat=20 --pallet=pallet_vesting --extrinsic=* --execution=wasm --wasm-execution=compiled --heap-pages=4096 --output=./frame/vesting/src/weights.rs --template=./.maintain/frame-weight-template.hbs

* fmt

* trigger

* fmt

Co-authored-by: Parity Bot <admin@parity.io>
Co-authored-by: Peter Goodspeed-Niklaus <coriolinus@users.noreply.github.com>
Co-authored-by: Shawn Tabrizi <shawntabrizi@gmail.com>
Co-authored-by: Kian Paimani <5588131+kianenigma@users.noreply.github.com>
Co-authored-by: kianenigma <kian@parity.io>

substrate/frame/vesting/src/benchmarking.rs

@@ -19,12 +19,12 @@
 
 #![cfg(feature = "runtime-benchmarks")]
 
-use super::*;
-
 use frame_benchmarking::{account, benchmarks, impl_benchmark_test_suite, whitelisted_caller};
+use frame_support::assert_ok;
 use frame_system::{Pallet as System, RawOrigin};
-use sp_runtime::traits::Bounded;
+use sp_runtime::traits::{Bounded, CheckedDiv, CheckedMul};
 
+use super::*;
 use crate::Pallet as Vesting;
 
 const SEED: u32 = 0;
@@ -35,42 +35,63 @@ type BalanceOf<T> =
 fn add_locks<T: Config>(who: &T::AccountId, n: u8) {
 	for id in 0..n {
 		let lock_id = [id; 8];
-		let locked = 100u32;
+		let locked = 256u32;
 		let reasons = WithdrawReasons::TRANSFER | WithdrawReasons::RESERVE;
 		T::Currency::set_lock(lock_id, who, locked.into(), reasons);
 	}
 }
 
-fn add_vesting_schedule<T: Config>(who: &T::AccountId) -> Result<(), &'static str> {
-	let locked = 100u32;
-	let per_block = 10u32;
+fn add_vesting_schedules<T: Config>(
+	target: <T::Lookup as StaticLookup>::Source,
+	n: u32,
+) -> Result<BalanceOf<T>, &'static str> {
+	let min_transfer = T::MinVestedTransfer::get();
+	let locked = min_transfer.checked_mul(&20u32.into()).unwrap();
+	// Schedule has a duration of 20.
+	let per_block = min_transfer;
 	let starting_block = 1u32;
 
-	System::<T>::set_block_number(0u32.into());
+	let source: T::AccountId = account("source", 0, SEED);
+	let source_lookup: <T::Lookup as StaticLookup>::Source = T::Lookup::unlookup(source.clone());
+	T::Currency::make_free_balance_be(&source, BalanceOf::<T>::max_value());
 
-	// Add schedule to avoid `NotVesting` error.
-	Vesting::<T>::add_vesting_schedule(
-		&who,
-		locked.into(),
-		per_block.into(),
-		starting_block.into(),
-	)?;
-	Ok(())
+	System::<T>::set_block_number(T::BlockNumber::zero());
+
+	let mut total_locked: BalanceOf<T> = Zero::zero();
+	for _ in 0..n {
+		total_locked += locked;
+
+		let schedule = VestingInfo::new(locked, per_block, starting_block.into());
+		assert_ok!(Vesting::<T>::do_vested_transfer(
+			source_lookup.clone(),
+			target.clone(),
+			schedule
+		));
+
+		// Top up to guarantee we can always transfer another schedule.
+		T::Currency::make_free_balance_be(&source, BalanceOf::<T>::max_value());
+	}
+
+	Ok(total_locked.into())
 }
 
 benchmarks! {
 	vest_locked {
-		let l in 0 .. MaxLocksOf::<T>::get();
+		let l in 0 .. MaxLocksOf::<T>::get() - 1;
+		let s in 1 .. T::MAX_VESTING_SCHEDULES;
+
+		let caller: T::AccountId = whitelisted_caller();
+		let caller_lookup: <T::Lookup as StaticLookup>::Source = T::Lookup::unlookup(caller.clone());
+		T::Currency::make_free_balance_be(&caller, T::Currency::minimum_balance());
 
-		let caller = whitelisted_caller();
-		T::Currency::make_free_balance_be(&caller, BalanceOf::<T>::max_value());
 		add_locks::<T>(&caller, l as u8);
-		add_vesting_schedule::<T>(&caller)?;
+		let expected_balance = add_vesting_schedules::<T>(caller_lookup, s)?;
+
 		// At block zero, everything is vested.
-		System::<T>::set_block_number(T::BlockNumber::zero());
+		assert_eq!(System::<T>::block_number(), T::BlockNumber::zero());
 		assert_eq!(
 			Vesting::<T>::vesting_balance(&caller),
-			Some(100u32.into()),
+			Some(expected_balance.into()),
 			"Vesting schedule not added",
 		);
 	}: vest(RawOrigin::Signed(caller.clone()))
@@ -78,20 +99,24 @@ benchmarks! {
 		// Nothing happened since everything is still vested.
 		assert_eq!(
 			Vesting::<T>::vesting_balance(&caller),
-			Some(100u32.into()),
+			Some(expected_balance.into()),
 			"Vesting schedule was removed",
 		);
 	}
 
 	vest_unlocked {
-		let l in 0 .. MaxLocksOf::<T>::get();
+		let l in 0 .. MaxLocksOf::<T>::get() - 1;
+		let s in 1 .. T::MAX_VESTING_SCHEDULES;
+
+		let caller: T::AccountId = whitelisted_caller();
+		let caller_lookup: <T::Lookup as StaticLookup>::Source = T::Lookup::unlookup(caller.clone());
+		T::Currency::make_free_balance_be(&caller, T::Currency::minimum_balance());
 
-		let caller = whitelisted_caller();
-		T::Currency::make_free_balance_be(&caller, BalanceOf::<T>::max_value());
 		add_locks::<T>(&caller, l as u8);
-		add_vesting_schedule::<T>(&caller)?;
-		// At block 20, everything is unvested.
-		System::<T>::set_block_number(20u32.into());
+		add_vesting_schedules::<T>(caller_lookup, s)?;
+
+		// At block 21, everything is unlocked.
+		System::<T>::set_block_number(21u32.into());
 		assert_eq!(
 			Vesting::<T>::vesting_balance(&caller),
 			Some(BalanceOf::<T>::zero()),
@@ -108,18 +133,20 @@ benchmarks! {
 	}
 
 	vest_other_locked {
-		let l in 0 .. MaxLocksOf::<T>::get();
+		let l in 0 .. MaxLocksOf::<T>::get() - 1;
+		let s in 1 .. T::MAX_VESTING_SCHEDULES;
 
 		let other: T::AccountId = account("other", 0, SEED);
 		let other_lookup: <T::Lookup as StaticLookup>::Source = T::Lookup::unlookup(other.clone());
-		T::Currency::make_free_balance_be(&other, BalanceOf::<T>::max_value());
+
 		add_locks::<T>(&other, l as u8);
-		add_vesting_schedule::<T>(&other)?;
+		let expected_balance = add_vesting_schedules::<T>(other_lookup.clone(), s)?;
+
 		// At block zero, everything is vested.
-		System::<T>::set_block_number(T::BlockNumber::zero());
+		assert_eq!(System::<T>::block_number(), T::BlockNumber::zero());
 		assert_eq!(
 			Vesting::<T>::vesting_balance(&other),
-			Some(100u32.into()),
+			Some(expected_balance),
 			"Vesting schedule not added",
 		);
 
@@ -129,21 +156,23 @@ benchmarks! {
 		// Nothing happened since everything is still vested.
 		assert_eq!(
 			Vesting::<T>::vesting_balance(&other),
-			Some(100u32.into()),
+			Some(expected_balance.into()),
 			"Vesting schedule was removed",
 		);
 	}
 
 	vest_other_unlocked {
-		let l in 0 .. MaxLocksOf::<T>::get();
+		let l in 0 .. MaxLocksOf::<T>::get() - 1;
+		let s in 1 .. T::MAX_VESTING_SCHEDULES;
 
 		let other: T::AccountId = account("other", 0, SEED);
 		let other_lookup: <T::Lookup as StaticLookup>::Source = T::Lookup::unlookup(other.clone());
-		T::Currency::make_free_balance_be(&other, BalanceOf::<T>::max_value());
+
 		add_locks::<T>(&other, l as u8);
-		add_vesting_schedule::<T>(&other)?;
-		// At block 20, everything is unvested.
-		System::<T>::set_block_number(20u32.into());
+		add_vesting_schedules::<T>(other_lookup.clone(), s)?;
+		// At block 21 everything is unlocked.
+		System::<T>::set_block_number(21u32.into());
+
 		assert_eq!(
 			Vesting::<T>::vesting_balance(&other),
 			Some(BalanceOf::<T>::zero()),
@@ -153,7 +182,7 @@ benchmarks! {
 		let caller: T::AccountId = whitelisted_caller();
 	}: vest_other(RawOrigin::Signed(caller.clone()), other_lookup)
 	verify {
-		// Vesting schedule is removed!
+		// Vesting schedule is removed.
 		assert_eq!(
 			Vesting::<T>::vesting_balance(&other),
 			None,
@@ -162,65 +191,187 @@ benchmarks! {
 	}
 
 	vested_transfer {
-		let l in 0 .. MaxLocksOf::<T>::get();
+		let l in 0 .. MaxLocksOf::<T>::get() - 1;
+		let s in 0 .. T::MAX_VESTING_SCHEDULES - 1;
 
 		let caller: T::AccountId = whitelisted_caller();
 		T::Currency::make_free_balance_be(&caller, BalanceOf::<T>::max_value());
+
 		let target: T::AccountId = account("target", 0, SEED);
 		let target_lookup: <T::Lookup as StaticLookup>::Source = T::Lookup::unlookup(target.clone());
 		// Give target existing locks
 		add_locks::<T>(&target, l as u8);
+		// Add one vesting schedules.
+		let mut expected_balance = add_vesting_schedules::<T>(target_lookup.clone(), s)?;
 
 		let transfer_amount = T::MinVestedTransfer::get();
+		let per_block = transfer_amount.checked_div(&20u32.into()).unwrap();
+		expected_balance += transfer_amount;
 
-		let vesting_schedule = VestingInfo {
-			locked: transfer_amount,
-			per_block: 10u32.into(),
-			starting_block: 1u32.into(),
-		};
+		let vesting_schedule = VestingInfo::new(
+			transfer_amount,
+			per_block,
+			1u32.into(),
+		);
 	}: _(RawOrigin::Signed(caller), target_lookup, vesting_schedule)
 	verify {
 		assert_eq!(
-			T::MinVestedTransfer::get(),
+			expected_balance,
 			T::Currency::free_balance(&target),
 			"Transfer didn't happen",
 		);
 		assert_eq!(
 			Vesting::<T>::vesting_balance(&target),
-			Some(T::MinVestedTransfer::get()),
-			"Lock not created",
+			Some(expected_balance),
+			"Lock not correctly updated",
 		);
 	}
 
 	force_vested_transfer {
-		let l in 0 .. MaxLocksOf::<T>::get();
+		let l in 0 .. MaxLocksOf::<T>::get() - 1;
+		let s in 0 .. T::MAX_VESTING_SCHEDULES - 1;
 
 		let source: T::AccountId = account("source", 0, SEED);
 		let source_lookup: <T::Lookup as StaticLookup>::Source = T::Lookup::unlookup(source.clone());
 		T::Currency::make_free_balance_be(&source, BalanceOf::<T>::max_value());
+
 		let target: T::AccountId = account("target", 0, SEED);
 		let target_lookup: <T::Lookup as StaticLookup>::Source = T::Lookup::unlookup(target.clone());
 		// Give target existing locks
 		add_locks::<T>(&target, l as u8);
+		// Add one less than max vesting schedules
+		let mut expected_balance = add_vesting_schedules::<T>(target_lookup.clone(), s)?;
 
 		let transfer_amount = T::MinVestedTransfer::get();
+		let per_block = transfer_amount.checked_div(&20u32.into()).unwrap();
+		expected_balance += transfer_amount;
 
-		let vesting_schedule = VestingInfo {
-			locked: transfer_amount,
-			per_block: 10u32.into(),
-			starting_block: 1u32.into(),
-		};
+		let vesting_schedule = VestingInfo::new(
+			transfer_amount,
+			per_block,
+			1u32.into(),
+		);
 	}: _(RawOrigin::Root, source_lookup, target_lookup, vesting_schedule)
 	verify {
 		assert_eq!(
-			T::MinVestedTransfer::get(),
+			expected_balance,
 			T::Currency::free_balance(&target),
 			"Transfer didn't happen",
 		);
 		assert_eq!(
 			Vesting::<T>::vesting_balance(&target),
-			Some(T::MinVestedTransfer::get()),
-			"Lock not created",
+			Some(expected_balance.into()),
+				"Lock not correctly updated",
+			);
+		}
+
+	not_unlocking_merge_schedules {
+		let l in 0 .. MaxLocksOf::<T>::get() - 1;
+		let s in 2 .. T::MAX_VESTING_SCHEDULES;
+
+		let caller: T::AccountId = account("caller", 0, SEED);
+		let caller_lookup: <T::Lookup as StaticLookup>::Source = T::Lookup::unlookup(caller.clone());
+		// Give target existing locks.
+		add_locks::<T>(&caller, l as u8);
+		// Add max vesting schedules.
+		let expected_balance = add_vesting_schedules::<T>(caller_lookup.clone(), s)?;
+
+		// Schedules are not vesting at block 0.
+		assert_eq!(System::<T>::block_number(), T::BlockNumber::zero());
+		assert_eq!(
+			Vesting::<T>::vesting_balance(&caller),
+			Some(expected_balance),
+			"Vesting balance should equal sum locked of all schedules",
+		);
+		assert_eq!(
+			Vesting::<T>::vesting(&caller).unwrap().len(),
+			s as usize,
+			"There should be exactly max vesting schedules"
+		);
+	}: merge_schedules(RawOrigin::Signed(caller.clone()), 0, s - 1)
+	verify {
+		let expected_schedule = VestingInfo::new(
+			T::MinVestedTransfer::get() * 20u32.into() * 2u32.into(),
+			T::MinVestedTransfer::get() * 2u32.into(),
+			1u32.into(),
+		);
+		let expected_index = (s - 2) as usize;
+		assert_eq!(
+			Vesting::<T>::vesting(&caller).unwrap()[expected_index],
+			expected_schedule
+		);
+		assert_eq!(
+			Vesting::<T>::vesting_balance(&caller),
+			Some(expected_balance),
+			"Vesting balance should equal total locked of all schedules",
+		);
+		assert_eq!(
+			Vesting::<T>::vesting(&caller).unwrap().len(),
+			(s - 1) as usize,
+			"Schedule count should reduce by 1"
+		);
+	}
+
+	unlocking_merge_schedules {
+		let l in 0 .. MaxLocksOf::<T>::get() - 1;
+		let s in 2 .. T::MAX_VESTING_SCHEDULES;
+
+		// Destination used just for currency transfers in asserts.
+		let test_dest: T::AccountId = account("test_dest", 0, SEED);
+
+		let caller: T::AccountId = account("caller", 0, SEED);
+		let caller_lookup: <T::Lookup as StaticLookup>::Source = T::Lookup::unlookup(caller.clone());
+		// Give target other locks.
+		add_locks::<T>(&caller, l as u8);
+		// Add max vesting schedules.
+		let total_transferred = add_vesting_schedules::<T>(caller_lookup.clone(), s)?;
+
+		// Go to about half way through all the schedules duration. (They all start at 1, and have a duration of 20 or 21).
+		System::<T>::set_block_number(11u32.into());
+		// We expect half the original locked balance (+ any remainder that vests on the last block).
+		let expected_balance = total_transferred / 2u32.into();
+		assert_eq!(
+			Vesting::<T>::vesting_balance(&caller),
+			Some(expected_balance),
+			"Vesting balance should reflect that we are half way through all schedules duration",
+		);
+		assert_eq!(
+			Vesting::<T>::vesting(&caller).unwrap().len(),
+			s as usize,
+			"There should be exactly max vesting schedules"
+		);
+		// The balance is not actually transferable because it has not been unlocked.
+		assert!(T::Currency::transfer(&caller, &test_dest, expected_balance, ExistenceRequirement::AllowDeath).is_err());
+	}: merge_schedules(RawOrigin::Signed(caller.clone()), 0, s - 1)
+	verify {
+		let expected_schedule = VestingInfo::new(
+			T::MinVestedTransfer::get() * 2u32.into() * 10u32.into(),
+			T::MinVestedTransfer::get() * 2u32.into(),
+			11u32.into(),
+		);
+		let expected_index = (s - 2) as usize;
+		assert_eq!(
+			Vesting::<T>::vesting(&caller).unwrap()[expected_index],
+			expected_schedule,
+			"New schedule is properly created and placed"
+		);
+		assert_eq!(
+			Vesting::<T>::vesting(&caller).unwrap()[expected_index],
+			expected_schedule
+		);
+		assert_eq!(
+			Vesting::<T>::vesting_balance(&caller),
+			Some(expected_balance),
+			"Vesting balance should equal half total locked of all schedules",
+		);
+		assert_eq!(
+			Vesting::<T>::vesting(&caller).unwrap().len(),
+			(s - 1) as usize,
+			"Schedule count should reduce by 1"
+		);
+		// Since merge unlocks all schedules we can now transfer the balance.
+		assert_ok!(
+			T::Currency::transfer(&caller, &test_dest, expected_balance, ExistenceRequirement::AllowDeath)
 		);
 	}
 }