Introduce and Implement VestedTransfer Trait (#5630)

This PR introduces a `VestedTransfer` Trait, which handles making a
transfer while also applying a vesting schedule to that balance.

This can be used in pallets like the Treasury pallet, where now we can
easily introduce a `vested_spend` extrinsic as an alternative to giving
all funds up front.

We implement `()` for the `VestedTransfer` trait, which just returns an
error, and allows anyone to opt out from needing to use or implement
this trait.

This PR also updates the logic of `do_vested_transfer` to remove the
"pre-check" which was needed before we had a default transactional layer
in FRAME.

Finally, I also fixed up some bad formatting in the test.rs file.

---------

Co-authored-by: Guillaume Thiolliere <[email protected]>
Co-authored-by: Bastian Köcher <[email protected]>

prdoc/pr_5630.prdoc

@@ -0,0 +1,15 @@
+# Schema: Polkadot SDK PRDoc Schema (prdoc) v1.0.0
+# See doc at https://raw.githubusercontent.com/paritytech/polkadot-sdk/master/prdoc/schema_user.json
+
+title: Introduce and Implement the `VestedTransfer` Trait
+
+doc:
+  - audience: Runtime Dev
+    description: |
+      This PR introduces a new trait `VestedTransfer` which is implemented by `pallet_vesting`. With this, other pallets can easily introduce vested transfers into their logic.
+
+crates:
+  - name: frame-support
+    bump: minor
+  - name: pallet-vesting
+    bump: minor

substrate/frame/support/src/traits.rs

@@ -23,7 +23,8 @@ pub mod tokens;
 pub use tokens::{
 	currency::{
 		ActiveIssuanceOf, Currency, InspectLockableCurrency, LockIdentifier, LockableCurrency,
-		NamedReservableCurrency, ReservableCurrency, TotalIssuanceOf, VestingSchedule,
+		NamedReservableCurrency, ReservableCurrency, TotalIssuanceOf, VestedTransfer,
+		VestingSchedule,
 	},
 	fungible, fungibles,
 	imbalance::{Imbalance, OnUnbalanced, SignedImbalance},

substrate/frame/support/src/traits/tokens/currency.rs

@@ -30,7 +30,9 @@ use sp_runtime::{traits::MaybeSerializeDeserialize, DispatchError};
 mod reservable;
 pub use reservable::{NamedReservableCurrency, ReservableCurrency};
 mod lockable;
-pub use lockable::{InspectLockableCurrency, LockIdentifier, LockableCurrency, VestingSchedule};
+pub use lockable::{
+	InspectLockableCurrency, LockIdentifier, LockableCurrency, VestedTransfer, VestingSchedule,
+};
 
 /// Abstraction over a fungible assets system.
 pub trait Currency<AccountId> {

substrate/frame/support/src/traits/tokens/currency/lockable.rs

@@ -112,3 +112,56 @@ pub trait VestingSchedule<AccountId> {
 	/// NOTE: This doesn't alter the free balance of the account.
 	fn remove_vesting_schedule(who: &AccountId, schedule_index: u32) -> DispatchResult;
 }
+
+/// A vested transfer over a currency. This allows a transferred amount to vest over time.
+pub trait VestedTransfer<AccountId> {
+	/// The quantity used to denote time; usually just a `BlockNumber`.
+	type Moment;
+
+	/// The currency that this schedule applies to.
+	type Currency: Currency<AccountId>;
+
+	/// Execute a vested transfer from `source` to `target` with the given schedule:
+	/// 	- `locked`: The amount to be transferred and for the vesting schedule to apply to.
+	/// 	- `per_block`: The amount to be unlocked each block. (linear vesting)
+	/// 	- `starting_block`: The block where the vesting should start. This block can be in the past
+	///    or future, and should adjust when the tokens become available to the user.
+	///
+	/// Example: Assume we are on block 100. If `locked` amount is 100, and `per_block` is 1:
+	/// 	- If `starting_block` is 0, then the whole 100 tokens will be available right away as the
+	///    vesting schedule started in the past and has fully completed.
+	/// 	- If `starting_block` is 50, then 50 tokens are made available right away, and 50 more
+	///    tokens will unlock one token at a time until block 150.
+	/// 	- If `starting_block` is 100, then each block, 1 token will be unlocked until the whole
+	///    balance is unlocked at block 200.
+	/// 	- If `starting_block` is 200, then the 100 token balance will be completely locked until
+	///    block 200, and then start to unlock one token at a time until block 300.
+	fn vested_transfer(
+		source: &AccountId,
+		target: &AccountId,
+		locked: <Self::Currency as Currency<AccountId>>::Balance,
+		per_block: <Self::Currency as Currency<AccountId>>::Balance,
+		starting_block: Self::Moment,
+	) -> DispatchResult;
+}
+
+// An no-op implementation of `VestedTransfer` for pallets that require this trait, but users may
+// not want to implement this functionality
+pub struct NoVestedTransfers<C> {
+	phantom: core::marker::PhantomData<C>,
+}
+
+impl<AccountId, C: Currency<AccountId>> VestedTransfer<AccountId> for NoVestedTransfers<C> {
+	type Moment = ();
+	type Currency = C;
+
+	fn vested_transfer(
+		_source: &AccountId,
+		_target: &AccountId,
+		_locked: <Self::Currency as Currency<AccountId>>::Balance,
+		_per_block: <Self::Currency as Currency<AccountId>>::Balance,
+		_starting_block: Self::Moment,
+	) -> DispatchResult {
+		Err(sp_runtime::DispatchError::Unavailable.into())
+	}
+}

substrate/frame/vesting/src/benchmarking.rs

@@ -42,7 +42,7 @@ fn add_locks<T: Config>(who: &T::AccountId, n: u8) {
 }
 
 fn add_vesting_schedules<T: Config>(
-	target: AccountIdLookupOf<T>,
+	target: &T::AccountId,
 	n: u32,
 ) -> Result<BalanceOf<T>, &'static str> {
 	let min_transfer = T::MinVestedTransfer::get();
@@ -52,7 +52,6 @@ fn add_vesting_schedules<T: Config>(
 	let starting_block = 1u32;
 
 	let source: T::AccountId = account("source", 0, SEED);
-	let source_lookup = T::Lookup::unlookup(source.clone());
 	T::Currency::make_free_balance_be(&source, BalanceOf::<T>::max_value());
 
 	T::BlockNumberProvider::set_block_number(BlockNumberFor::<T>::zero());
@@ -62,11 +61,7 @@ fn add_vesting_schedules<T: Config>(
 		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
-		));
+		assert_ok!(Vesting::<T>::do_vested_transfer(&source, target, schedule));
 
 		// Top up to guarantee we can always transfer another schedule.
 		T::Currency::make_free_balance_be(&source, BalanceOf::<T>::max_value());
@@ -81,11 +76,10 @@ benchmarks! {
 		let s in 1 .. T::MAX_VESTING_SCHEDULES;
 
 		let caller: T::AccountId = whitelisted_caller();
-		let caller_lookup = T::Lookup::unlookup(caller.clone());
 		T::Currency::make_free_balance_be(&caller, T::Currency::minimum_balance());
 
 		add_locks::<T>(&caller, l as u8);
-		let expected_balance = add_vesting_schedules::<T>(caller_lookup, s)?;
+		let expected_balance = add_vesting_schedules::<T>(&caller, s)?;
 
 		// At block zero, everything is vested.
 		assert_eq!(System::<T>::block_number(), BlockNumberFor::<T>::zero());
@@ -109,11 +103,10 @@ benchmarks! {
 		let s in 1 .. T::MAX_VESTING_SCHEDULES;
 
 		let caller: T::AccountId = whitelisted_caller();
-		let caller_lookup = T::Lookup::unlookup(caller.clone());
 		T::Currency::make_free_balance_be(&caller, T::Currency::minimum_balance());
 
 		add_locks::<T>(&caller, l as u8);
-		add_vesting_schedules::<T>(caller_lookup, s)?;
+		add_vesting_schedules::<T>(&caller, s)?;
 
 		// At block 21, everything is unlocked.
 		T::BlockNumberProvider::set_block_number(21u32.into());
@@ -141,7 +134,7 @@ benchmarks! {
 
 		T::Currency::make_free_balance_be(&other, T::Currency::minimum_balance());
 		add_locks::<T>(&other, l as u8);
-		let expected_balance = add_vesting_schedules::<T>(other_lookup.clone(), s)?;
+		let expected_balance = add_vesting_schedules::<T>(&other, s)?;
 
 		// At block zero, everything is vested.
 		assert_eq!(System::<T>::block_number(), BlockNumberFor::<T>::zero());
@@ -171,7 +164,7 @@ benchmarks! {
 
 		T::Currency::make_free_balance_be(&other, T::Currency::minimum_balance());
 		add_locks::<T>(&other, l as u8);
-		add_vesting_schedules::<T>(other_lookup.clone(), s)?;
+		add_vesting_schedules::<T>(&other, s)?;
 		// At block 21 everything is unlocked.
 		T::BlockNumberProvider::set_block_number(21u32.into());
 
@@ -206,7 +199,7 @@ benchmarks! {
 		add_locks::<T>(&target, l as u8);
 		// Add one vesting schedules.
 		let orig_balance = T::Currency::free_balance(&target);
-		let mut expected_balance = add_vesting_schedules::<T>(target_lookup.clone(), s)?;
+		let mut expected_balance = add_vesting_schedules::<T>(&target, s)?;
 
 		let transfer_amount = T::MinVestedTransfer::get();
 		let per_block = transfer_amount.checked_div(&20u32.into()).unwrap();
@@ -246,7 +239,7 @@ benchmarks! {
 		add_locks::<T>(&target, l as u8);
 		// Add one less than max vesting schedules
 		let orig_balance = T::Currency::free_balance(&target);
-		let mut expected_balance = add_vesting_schedules::<T>(target_lookup.clone(), s)?;
+		let mut expected_balance = add_vesting_schedules::<T>(&target, s)?;
 
 		let transfer_amount = T::MinVestedTransfer::get();
 		let per_block = transfer_amount.checked_div(&20u32.into()).unwrap();
@@ -281,7 +274,7 @@ benchmarks! {
 		T::Currency::make_free_balance_be(&caller, T::Currency::minimum_balance());
 		add_locks::<T>(&caller, l as u8);
 		// Add max vesting schedules.
-		let expected_balance = add_vesting_schedules::<T>(caller_lookup, s)?;
+		let expected_balance = add_vesting_schedules::<T>(&caller, s)?;
 
 		// Schedules are not vesting at block 0.
 		assert_eq!(System::<T>::block_number(), BlockNumberFor::<T>::zero());
@@ -332,7 +325,7 @@ benchmarks! {
 		T::Currency::make_free_balance_be(&caller, T::Currency::minimum_balance());
 		add_locks::<T>(&caller, l as u8);
 		// Add max vesting schedules.
-		let total_transferred = add_vesting_schedules::<T>(caller_lookup, s)?;
+		let total_transferred = add_vesting_schedules::<T>(&caller, s)?;
 
 		// Go to about half way through all the schedules duration. (They all start at 1, and have a duration of 20 or 21).
 		T::BlockNumberProvider::set_block_number(11u32.into());
@@ -397,7 +390,7 @@ force_remove_vesting_schedule {
 
 		// Give target existing locks.
 		add_locks::<T>(&target, l as u8);
-		let _ = add_vesting_schedules::<T>(target_lookup.clone(), s)?;
+		add_vesting_schedules::<T>(&target, s)?;
 
 		// The last vesting schedule.
 		let schedule_index = s - 1;

substrate/frame/vesting/src/lib.rs

@@ -66,8 +66,8 @@ use frame_support::{
 	ensure,
 	storage::bounded_vec::BoundedVec,
 	traits::{
-		Currency, ExistenceRequirement, Get, LockIdentifier, LockableCurrency, VestingSchedule,
-		WithdrawReasons,
+		Currency, ExistenceRequirement, Get, LockIdentifier, LockableCurrency, VestedTransfer,
+		VestingSchedule, WithdrawReasons,
 	},
 	weights::Weight,
 };
@@ -351,8 +351,8 @@ pub mod pallet {
 			schedule: VestingInfo<BalanceOf<T>, BlockNumberFor<T>>,
 		) -> DispatchResult {
 			let transactor = ensure_signed(origin)?;
-			let transactor = <T::Lookup as StaticLookup>::unlookup(transactor);
-			Self::do_vested_transfer(transactor, target, schedule)
+			let target = T::Lookup::lookup(target)?;
+			Self::do_vested_transfer(&transactor, &target, schedule)
 		}
 
 		/// Force a vested transfer.
@@ -380,7 +380,9 @@ pub mod pallet {
 			schedule: VestingInfo<BalanceOf<T>, BlockNumberFor<T>>,
 		) -> DispatchResult {
 			ensure_root(origin)?;
-			Self::do_vested_transfer(source, target, schedule)
+			let target = T::Lookup::lookup(target)?;
+			let source = T::Lookup::lookup(source)?;
+			Self::do_vested_transfer(&source, &target, schedule)
 		}
 
 		/// Merge two vesting schedules together, creating a new vesting schedule that unlocks over
@@ -525,36 +527,31 @@ impl<T: Config> Pallet<T> {
 
 	// Execute a vested transfer from `source` to `target` with the given `schedule`.
 	fn do_vested_transfer(
-		source: AccountIdLookupOf<T>,
-		target: AccountIdLookupOf<T>,
+		source: &T::AccountId,
+		target: &T::AccountId,
 		schedule: VestingInfo<BalanceOf<T>, BlockNumberFor<T>>,
 	) -> DispatchResult {
 		// Validate user inputs.
 		ensure!(schedule.locked() >= T::MinVestedTransfer::get(), Error::<T>::AmountLow);
 		if !schedule.is_valid() {
 			return Err(Error::<T>::InvalidScheduleParams.into())
 		};
-		let target = T::Lookup::lookup(target)?;
-		let source = T::Lookup::lookup(source)?;
 
 		// Check we can add to this account prior to any storage writes.
 		Self::can_add_vesting_schedule(
-			&target,
+			target,
 			schedule.locked(),
 			schedule.per_block(),
 			schedule.starting_block(),
 		)?;
 
-		T::Currency::transfer(
-			&source,
-			&target,
-			schedule.locked(),
-			ExistenceRequirement::AllowDeath,
-		)?;
+		T::Currency::transfer(source, target, schedule.locked(), ExistenceRequirement::AllowDeath)?;
 
 		// We can't let this fail because the currency transfer has already happened.
+		// Must be successful as it has been checked before.
+		// Better to return error on failure anyway.
 		let res = Self::add_vesting_schedule(
-			&target,
+			target,
 			schedule.locked(),
 			schedule.per_block(),
 			schedule.starting_block(),
@@ -751,8 +748,8 @@ where
 		Ok(())
 	}
 
-	// Ensure we can call `add_vesting_schedule` without error. This should always
-	// be called prior to `add_vesting_schedule`.
+	/// Ensure we can call `add_vesting_schedule` without error. This should always
+	/// be called prior to `add_vesting_schedule`.
 	fn can_add_vesting_schedule(
 		who: &T::AccountId,
 		locked: BalanceOf<T>,
@@ -784,3 +781,32 @@ where
 		Ok(())
 	}
 }
+
+/// An implementation that allows the Vesting Pallet to handle a vested transfer
+/// on behalf of another Pallet.
+impl<T: Config> VestedTransfer<T::AccountId> for Pallet<T>
+where
+	BalanceOf<T>: MaybeSerializeDeserialize + Debug,
+{
+	type Currency = T::Currency;
+	type Moment = BlockNumberFor<T>;
+
+	fn vested_transfer(
+		source: &T::AccountId,
+		target: &T::AccountId,
+		locked: BalanceOf<T>,
+		per_block: BalanceOf<T>,
+		starting_block: BlockNumberFor<T>,
+	) -> DispatchResult {
+		use frame_support::storage::{with_transaction, TransactionOutcome};
+		let schedule = VestingInfo::new(locked, per_block, starting_block);
+		with_transaction(|| -> TransactionOutcome<DispatchResult> {
+			let result = Self::do_vested_transfer(source, target, schedule);
+
+			match &result {
+				Ok(()) => TransactionOutcome::Commit(result),
+				_ => TransactionOutcome::Rollback(result),
+			}
+		})
+	}
+}