From 79671a4132b76f5eb69b22f3348da0856dc49af0 Mon Sep 17 00:00:00 2001 From: Shawn Tabrizi Date: Fri, 6 Sep 2024 14:35:25 -0400 Subject: [PATCH 01/16] introduce vested transfer trait --- .../support/src/traits/tokens/currency.rs | 4 ++- .../src/traits/tokens/currency/lockable.rs | 33 +++++++++++++++++++ 2 files changed, 36 insertions(+), 1 deletion(-) diff --git a/substrate/frame/support/src/traits/tokens/currency.rs b/substrate/frame/support/src/traits/tokens/currency.rs index b3db4c98001d..ea2c66a32cb0 100644 --- a/substrate/frame/support/src/traits/tokens/currency.rs +++ b/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 { diff --git a/substrate/frame/support/src/traits/tokens/currency/lockable.rs b/substrate/frame/support/src/traits/tokens/currency/lockable.rs index 51a48dd15ce8..df170fa18c24 100644 --- a/substrate/frame/support/src/traits/tokens/currency/lockable.rs +++ b/substrate/frame/support/src/traits/tokens/currency/lockable.rs @@ -112,3 +112,36 @@ pub trait VestingSchedule { /// 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 { + /// The quantity used to denote time; usually just a `BlockNumber`. + type Moment; + + /// The currency that this schedule applies to. + type Currency: Currency; + + // Execute a vested transfer from `source` to `target` with the given `schedule`. + fn vested_transfer( + source: &AccountId, + target: &AccountId, + locked: >::Balance, + per_block: >::Balance, + starting_block: Self::Moment, + ) -> DispatchResult; +} + +impl VestedTransfer for () { + type Moment = (); + type Currency = (); + + fn vested_transfer( + _source: &AccountId, + _target: &AccountId, + _locked: >::Balance, + _per_block: >::Balance, + _starting_block: Self::Moment, + ) -> DispatchResult { + Err(sp_runtime::DispatchError::Unavailable.into()) + } +} From d47c24024eaf41187773d4fdbac801f73bb729f6 Mon Sep 17 00:00:00 2001 From: Shawn Tabrizi Date: Fri, 6 Sep 2024 14:59:20 -0400 Subject: [PATCH 02/16] impl VestedTransfer to vesting pallet --- substrate/frame/support/src/traits.rs | 3 +- substrate/frame/vesting/src/lib.rs | 45 +++++++++++++++++---------- 2 files changed, 31 insertions(+), 17 deletions(-) diff --git a/substrate/frame/support/src/traits.rs b/substrate/frame/support/src/traits.rs index f635ed32a124..79c2f1e88492 100644 --- a/substrate/frame/support/src/traits.rs +++ b/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}, diff --git a/substrate/frame/vesting/src/lib.rs b/substrate/frame/vesting/src/lib.rs index bfc10efeed79..0f71681cb369 100644 --- a/substrate/frame/vesting/src/lib.rs +++ b/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, }; @@ -534,17 +534,10 @@ impl Pallet { if !schedule.is_valid() { return Err(Error::::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, - schedule.locked(), - schedule.per_block(), - schedule.starting_block(), - )?; - T::Currency::transfer( &source, &target, @@ -552,14 +545,14 @@ impl Pallet { ExistenceRequirement::AllowDeath, )?; - // We can't let this fail because the currency transfer has already happened. - let res = Self::add_vesting_schedule( + // If adding this vesting schedule fails, all storage changes are undone due to FRAME's + // default transactional layers. + Self::add_vesting_schedule( &target, schedule.locked(), schedule.per_block(), schedule.starting_block(), - ); - debug_assert!(res.is_ok(), "Failed to add a schedule when we had to succeed."); + )?; Ok(()) } @@ -751,8 +744,7 @@ where Ok(()) } - // Ensure we can call `add_vesting_schedule` without error. This should always - // be called prior to `add_vesting_schedule`. + /// Checks if `add_vesting_schedule` would work against `who`. fn can_add_vesting_schedule( who: &T::AccountId, locked: BalanceOf, @@ -784,3 +776,24 @@ where Ok(()) } } + +impl VestedTransfer for Pallet +where + BalanceOf: MaybeSerializeDeserialize + Debug, +{ + type Currency = T::Currency; + type Moment = BlockNumberFor; + + fn vested_transfer( + source: &T::AccountId, + target: &T::AccountId, + locked: BalanceOf, + per_block: BalanceOf, + starting_block: BlockNumberFor, + ) -> DispatchResult { + let schedule = VestingInfo::new(locked, per_block, starting_block); + let source = ::unlookup(source.clone()); + let target = ::unlookup(target.clone()); + Self::do_vested_transfer(source, target, schedule) + } +} From 75da32e97f5b3bb7693fbeffe0118f0562a6a578 Mon Sep 17 00:00:00 2001 From: Shawn Tabrizi Date: Fri, 6 Sep 2024 15:03:31 -0400 Subject: [PATCH 03/16] fix test comment formatting --- substrate/frame/vesting/src/tests.rs | 21 +++++++++++++-------- 1 file changed, 13 insertions(+), 8 deletions(-) diff --git a/substrate/frame/vesting/src/tests.rs b/substrate/frame/vesting/src/tests.rs index 004da0dfbfa1..01b2facab35a 100644 --- a/substrate/frame/vesting/src/tests.rs +++ b/substrate/frame/vesting/src/tests.rs @@ -280,13 +280,14 @@ 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!(Balances::transfer_allow_death(Some(1).into(), 3, 155)); // Account 1 can send extra units gained + // Account 1 can send extra units gained + assert_ok!(Balances::transfer_allow_death(Some(1).into(), 3, 155)); // 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!(Balances::transfer_allow_death(Some(2).into(), 3, 100)); // Account 2 can send extra - // units gained + // Account 2 can send extra units gained + assert_ok!(Balances::transfer_allow_death(Some(2).into(), 3, 100)); }); } @@ -295,14 +296,16 @@ fn liquid_funds_should_transfer_with_delayed_vesting() { ExtBuilder::default().existential_deposit(256).build().execute_with(|| { let user12_free_balance = Balances::free_balance(&12); - assert_eq!(user12_free_balance, 2560); // Account 12 has free balance - // Account 12 has liquid funds + // Account 12 has free balance + assert_eq!(user12_free_balance, 2560); + // Account 12 has liquid funds assert_eq!(Vesting::vesting_balance(&12), Some(user12_free_balance - 256 * 5)); // Account 12 has delayed vesting let user12_vesting_schedule = VestingInfo::new( 256 * 5, - 64, // Vesting over 20 blocks + // Vesting over 20 blocks + 64, 10, ); assert_eq!(VestingStorage::::get(&12).unwrap(), vec![user12_vesting_schedule]); @@ -630,8 +633,10 @@ fn merge_ongoing_schedules() { let sched1 = VestingInfo::new( ED * 10, - ED, // Vest over 10 blocks. - sched0.starting_block() + 5, // Start at block 15. + // Vest over 10 blocks. + ED, + // Start at block 15. + sched0.starting_block() + 5, ); assert_ok!(Vesting::vested_transfer(Some(4).into(), 2, sched1)); assert_eq!(VestingStorage::::get(&2).unwrap(), vec![sched0, sched1]); From 7e825db29c352f3d5eb4210b56ed06e16208fc18 Mon Sep 17 00:00:00 2001 From: Shawn Tabrizi Date: Fri, 6 Sep 2024 15:33:36 -0400 Subject: [PATCH 04/16] Create pr_5630.prdoc --- prdoc/pr_5630.prdoc | 15 +++++++++++++++ 1 file changed, 15 insertions(+) create mode 100644 prdoc/pr_5630.prdoc diff --git a/prdoc/pr_5630.prdoc b/prdoc/pr_5630.prdoc new file mode 100644 index 000000000000..bafb9e746d40 --- /dev/null +++ b/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 From 24b2832a07981874fac8efe080b6027fdd4c273b Mon Sep 17 00:00:00 2001 From: Shawn Tabrizi Date: Fri, 6 Sep 2024 15:42:08 -0400 Subject: [PATCH 05/16] better docs --- .../src/traits/tokens/currency/lockable.rs | 18 +++++++++++++++++- 1 file changed, 17 insertions(+), 1 deletion(-) diff --git a/substrate/frame/support/src/traits/tokens/currency/lockable.rs b/substrate/frame/support/src/traits/tokens/currency/lockable.rs index df170fa18c24..d99993808b15 100644 --- a/substrate/frame/support/src/traits/tokens/currency/lockable.rs +++ b/substrate/frame/support/src/traits/tokens/currency/lockable.rs @@ -121,7 +121,21 @@ pub trait VestedTransfer { /// The currency that this schedule applies to. type Currency: Currency; - // Execute a vested transfer from `source` to `target` with the given `schedule`. + // 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, @@ -131,6 +145,8 @@ pub trait VestedTransfer { ) -> DispatchResult; } +// An no-op implementation of `VestedTransfer` for pallets that require this trait, but users may +// not want to implement this functionality. impl VestedTransfer for () { type Moment = (); type Currency = (); From e784ecb6e302f971bb4c148941bda6ebf844748e Mon Sep 17 00:00:00 2001 From: Shawn Tabrizi Date: Fri, 6 Sep 2024 20:46:26 -0400 Subject: [PATCH 06/16] fix apis --- substrate/frame/vesting/src/lib.rs | 26 +++++++++----------------- 1 file changed, 9 insertions(+), 17 deletions(-) diff --git a/substrate/frame/vesting/src/lib.rs b/substrate/frame/vesting/src/lib.rs index 0f71681cb369..e62bb3cfe17e 100644 --- a/substrate/frame/vesting/src/lib.rs +++ b/substrate/frame/vesting/src/lib.rs @@ -351,8 +351,8 @@ pub mod pallet { schedule: VestingInfo, BlockNumberFor>, ) -> DispatchResult { let transactor = ensure_signed(origin)?; - let transactor = ::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, BlockNumberFor>, ) -> 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,8 +527,8 @@ impl Pallet { // Execute a vested transfer from `source` to `target` with the given `schedule`. fn do_vested_transfer( - source: AccountIdLookupOf, - target: AccountIdLookupOf, + source: &T::AccountId, + target: &T::AccountId, schedule: VestingInfo, BlockNumberFor>, ) -> DispatchResult { // Validate user inputs. @@ -535,20 +537,12 @@ impl Pallet { return Err(Error::::InvalidScheduleParams.into()) }; - let target = T::Lookup::lookup(target)?; - let source = T::Lookup::lookup(source)?; - - T::Currency::transfer( - &source, - &target, - schedule.locked(), - ExistenceRequirement::AllowDeath, - )?; + T::Currency::transfer(source, target, schedule.locked(), ExistenceRequirement::AllowDeath)?; // If adding this vesting schedule fails, all storage changes are undone due to FRAME's // default transactional layers. Self::add_vesting_schedule( - &target, + target, schedule.locked(), schedule.per_block(), schedule.starting_block(), @@ -792,8 +786,6 @@ where starting_block: BlockNumberFor, ) -> DispatchResult { let schedule = VestingInfo::new(locked, per_block, starting_block); - let source = ::unlookup(source.clone()); - let target = ::unlookup(target.clone()); Self::do_vested_transfer(source, target, schedule) } } From 65ae2c258b88231b5ed4bc0d39367171f65d06a6 Mon Sep 17 00:00:00 2001 From: Shawn Tabrizi Date: Sun, 8 Sep 2024 14:58:07 -0400 Subject: [PATCH 07/16] Update substrate/frame/support/src/traits/tokens/currency/lockable.rs Co-authored-by: Guillaume Thiolliere --- .../src/traits/tokens/currency/lockable.rs | 30 +++++++++---------- 1 file changed, 15 insertions(+), 15 deletions(-) diff --git a/substrate/frame/support/src/traits/tokens/currency/lockable.rs b/substrate/frame/support/src/traits/tokens/currency/lockable.rs index d99993808b15..04579b0ff585 100644 --- a/substrate/frame/support/src/traits/tokens/currency/lockable.rs +++ b/substrate/frame/support/src/traits/tokens/currency/lockable.rs @@ -121,21 +121,21 @@ pub trait VestedTransfer { /// The currency that this schedule applies to. type Currency: Currency; - // 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. + /// 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, From 39be304cbd647a9829ef545ae82f9a5a744a0b8c Mon Sep 17 00:00:00 2001 From: Shawn Tabrizi Date: Wed, 11 Sep 2024 13:01:44 -0400 Subject: [PATCH 08/16] add transactional layer within vested transfer trait --- substrate/frame/vesting/src/lib.rs | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/substrate/frame/vesting/src/lib.rs b/substrate/frame/vesting/src/lib.rs index e62bb3cfe17e..ce31619fbfc7 100644 --- a/substrate/frame/vesting/src/lib.rs +++ b/substrate/frame/vesting/src/lib.rs @@ -785,7 +785,15 @@ where per_block: BalanceOf, starting_block: BlockNumberFor, ) -> DispatchResult { + use frame_support::storage::{with_transaction, TransactionOutcome}; let schedule = VestingInfo::new(locked, per_block, starting_block); - Self::do_vested_transfer(source, target, schedule) + with_transaction(|| -> TransactionOutcome { + let result = Self::do_vested_transfer(source, target, schedule); + + match &result { + Ok(()) => TransactionOutcome::Commit(result), + _ => TransactionOutcome::Rollback(result), + } + }) } } From c690ab83260f8840130ecf32ac769b968f8e9e05 Mon Sep 17 00:00:00 2001 From: Shawn Tabrizi Date: Wed, 11 Sep 2024 13:17:54 -0400 Subject: [PATCH 09/16] add tests --- substrate/frame/vesting/src/lib.rs | 8 ++++++ substrate/frame/vesting/src/tests.rs | 40 ++++++++++++++++++++++++++++ 2 files changed, 48 insertions(+) diff --git a/substrate/frame/vesting/src/lib.rs b/substrate/frame/vesting/src/lib.rs index ce31619fbfc7..8880835cbd59 100644 --- a/substrate/frame/vesting/src/lib.rs +++ b/substrate/frame/vesting/src/lib.rs @@ -537,6 +537,14 @@ impl Pallet { return Err(Error::::InvalidScheduleParams.into()) }; + // Check we can add to this account prior to any storage writes. + Self::can_add_vesting_schedule( + target, + schedule.locked(), + schedule.per_block(), + schedule.starting_block(), + )?; + T::Currency::transfer(source, target, schedule.locked(), ExistenceRequirement::AllowDeath)?; // If adding this vesting schedule fails, all storage changes are undone due to FRAME's diff --git a/substrate/frame/vesting/src/tests.rs b/substrate/frame/vesting/src/tests.rs index 01b2facab35a..54b86447d923 100644 --- a/substrate/frame/vesting/src/tests.rs +++ b/substrate/frame/vesting/src/tests.rs @@ -1196,3 +1196,43 @@ fn remove_vesting_schedule() { ); }); } + +#[test] +fn vested_transfer_impl_works() { + ExtBuilder::default().existential_deposit(ED).build().execute_with(|| { + assert_eq!(Balances::free_balance(&3), 256 * 30); + assert_eq!(Balances::free_balance(&4), 256 * 40); + // Account 4 should not have any vesting yet. + assert_eq!(VestingStorage::::get(&4), None); + + // Basic working scenario + assert_ok!(>::vested_transfer( + &3, + &4, + ED * 5, + ED * 5 / 20, + 10 + )); + // Now account 4 should have vesting. + let new_vesting_schedule = VestingInfo::new( + ED * 5, + (ED * 5) / 20, // Vesting over 20 blocks + 10, + ); + assert_eq!(VestingStorage::::get(&4).unwrap(), vec![new_vesting_schedule]); + // Account 4 has 5 * 256 locked. + assert_eq!(Vesting::vesting_balance(&4), Some(256 * 5)); + + // If the transfer fails (because they don't have enough balance), no storage is changed. + assert_noop!( + >::vested_transfer(&3, &4, ED * 9999, ED * 5 / 20, 10), + TokenError::FundsUnavailable + ); + + // If applying the vesting schedule fails (per block is 0), no storage is changed. + assert_noop!( + >::vested_transfer(&3, &4, ED * 5, 0, 10), + Error::::InvalidScheduleParams + ); + }); +} From 011c6ae41da82f47713f35bc67c520fbb468f787 Mon Sep 17 00:00:00 2001 From: Shawn Tabrizi Date: Wed, 11 Sep 2024 13:20:45 -0400 Subject: [PATCH 10/16] Update lib.rs --- substrate/frame/vesting/src/lib.rs | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/substrate/frame/vesting/src/lib.rs b/substrate/frame/vesting/src/lib.rs index 8880835cbd59..bd37b9affa1d 100644 --- a/substrate/frame/vesting/src/lib.rs +++ b/substrate/frame/vesting/src/lib.rs @@ -547,8 +547,7 @@ impl Pallet { T::Currency::transfer(source, target, schedule.locked(), ExistenceRequirement::AllowDeath)?; - // If adding this vesting schedule fails, all storage changes are undone due to FRAME's - // default transactional layers. + // We can't let this fail because the currency transfer has already happened. Self::add_vesting_schedule( target, schedule.locked(), @@ -746,7 +745,8 @@ where Ok(()) } - /// Checks if `add_vesting_schedule` would work against `who`. + /// 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, @@ -779,6 +779,8 @@ where } } +/// An implementation that allows the Vesting Pallet to handle a vested transfer +/// on behalf of another Pallet. impl VestedTransfer for Pallet where BalanceOf: MaybeSerializeDeserialize + Debug, From 3760a948499086876d674c6034b0cd07b11d8ad4 Mon Sep 17 00:00:00 2001 From: Shawn Tabrizi Date: Wed, 11 Sep 2024 14:43:38 -0400 Subject: [PATCH 11/16] fix benchmarks --- substrate/frame/vesting/src/benchmarking.rs | 29 ++++++++------------- 1 file changed, 11 insertions(+), 18 deletions(-) diff --git a/substrate/frame/vesting/src/benchmarking.rs b/substrate/frame/vesting/src/benchmarking.rs index 68214c4f47cc..34dad4a4f1c5 100644 --- a/substrate/frame/vesting/src/benchmarking.rs +++ b/substrate/frame/vesting/src/benchmarking.rs @@ -42,7 +42,7 @@ fn add_locks(who: &T::AccountId, n: u8) { } fn add_vesting_schedules( - target: AccountIdLookupOf, + target: &T::AccountId, n: u32, ) -> Result, &'static str> { let min_transfer = T::MinVestedTransfer::get(); @@ -52,7 +52,6 @@ fn add_vesting_schedules( 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::::max_value()); T::BlockNumberProvider::set_block_number(BlockNumberFor::::zero()); @@ -62,11 +61,7 @@ fn add_vesting_schedules( total_locked += locked; let schedule = VestingInfo::new(locked, per_block, starting_block.into()); - assert_ok!(Vesting::::do_vested_transfer( - source_lookup.clone(), - target.clone(), - schedule - )); + assert_ok!(Vesting::::do_vested_transfer(&source, target, schedule)); // Top up to guarantee we can always transfer another schedule. T::Currency::make_free_balance_be(&source, BalanceOf::::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::(&caller, l as u8); - let expected_balance = add_vesting_schedules::(caller_lookup, s)?; + let expected_balance = add_vesting_schedules::(&caller, s)?; // At block zero, everything is vested. assert_eq!(System::::block_number(), BlockNumberFor::::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::(&caller, l as u8); - add_vesting_schedules::(caller_lookup, s)?; + add_vesting_schedules::(&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::(&other, l as u8); - let expected_balance = add_vesting_schedules::(other_lookup.clone(), s)?; + let expected_balance = add_vesting_schedules::(&other, s)?; // At block zero, everything is vested. assert_eq!(System::::block_number(), BlockNumberFor::::zero()); @@ -171,7 +164,7 @@ benchmarks! { T::Currency::make_free_balance_be(&other, T::Currency::minimum_balance()); add_locks::(&other, l as u8); - add_vesting_schedules::(other_lookup.clone(), s)?; + add_vesting_schedules::(&other, s)?; // At block 21 everything is unlocked. T::BlockNumberProvider::set_block_number(21u32.into()); @@ -206,7 +199,7 @@ benchmarks! { add_locks::(&target, l as u8); // Add one vesting schedules. let orig_balance = T::Currency::free_balance(&target); - let mut expected_balance = add_vesting_schedules::(target_lookup.clone(), s)?; + let mut expected_balance = add_vesting_schedules::(&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::(&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::(target_lookup.clone(), s)?; + let mut expected_balance = add_vesting_schedules::(&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::(&caller, l as u8); // Add max vesting schedules. - let expected_balance = add_vesting_schedules::(caller_lookup, s)?; + let expected_balance = add_vesting_schedules::(&caller, s)?; // Schedules are not vesting at block 0. assert_eq!(System::::block_number(), BlockNumberFor::::zero()); @@ -332,7 +325,7 @@ benchmarks! { T::Currency::make_free_balance_be(&caller, T::Currency::minimum_balance()); add_locks::(&caller, l as u8); // Add max vesting schedules. - let total_transferred = add_vesting_schedules::(caller_lookup, s)?; + let total_transferred = add_vesting_schedules::(&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::(&target, l as u8); - let _ = add_vesting_schedules::(target_lookup.clone(), s)?; + let _ = add_vesting_schedules::(&target, s)?; // The last vesting schedule. let schedule_index = s - 1; From 084347487534c6ead38110c97fac3967a8cf2a77 Mon Sep 17 00:00:00 2001 From: Shawn Tabrizi Date: Wed, 11 Sep 2024 14:44:41 -0400 Subject: [PATCH 12/16] remove let _ --- substrate/frame/vesting/src/benchmarking.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/substrate/frame/vesting/src/benchmarking.rs b/substrate/frame/vesting/src/benchmarking.rs index 34dad4a4f1c5..736dd6eac1a8 100644 --- a/substrate/frame/vesting/src/benchmarking.rs +++ b/substrate/frame/vesting/src/benchmarking.rs @@ -390,7 +390,7 @@ force_remove_vesting_schedule { // Give target existing locks. add_locks::(&target, l as u8); - let _ = add_vesting_schedules::(&target, s)?; + add_vesting_schedules::(&target, s)?; // The last vesting schedule. let schedule_index = s - 1; From cfe000f8cd0f13b6608f27ff24325833a4f25e13 Mon Sep 17 00:00:00 2001 From: Shawn Tabrizi Date: Sat, 14 Sep 2024 14:08:52 -0400 Subject: [PATCH 13/16] Update substrate/frame/vesting/src/lib.rs Co-authored-by: Guillaume Thiolliere --- substrate/frame/vesting/src/lib.rs | 2 ++ 1 file changed, 2 insertions(+) diff --git a/substrate/frame/vesting/src/lib.rs b/substrate/frame/vesting/src/lib.rs index bd37b9affa1d..83981c6abb11 100644 --- a/substrate/frame/vesting/src/lib.rs +++ b/substrate/frame/vesting/src/lib.rs @@ -548,6 +548,8 @@ impl Pallet { 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. Self::add_vesting_schedule( target, schedule.locked(), From 2a66893dde76155ae8d0e07c7ff787e7ef987929 Mon Sep 17 00:00:00 2001 From: Shawn Tabrizi Date: Sat, 14 Sep 2024 14:13:18 -0400 Subject: [PATCH 14/16] add back debug assert --- substrate/frame/vesting/src/lib.rs | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/substrate/frame/vesting/src/lib.rs b/substrate/frame/vesting/src/lib.rs index 83981c6abb11..a897270f2542 100644 --- a/substrate/frame/vesting/src/lib.rs +++ b/substrate/frame/vesting/src/lib.rs @@ -550,12 +550,13 @@ impl Pallet { // 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. - Self::add_vesting_schedule( + let res = Self::add_vesting_schedule( target, schedule.locked(), schedule.per_block(), schedule.starting_block(), )?; + debug_assert!(res.is_ok(), "Failed to add a schedule when we had to succeed."); Ok(()) } From e5ecc12a5884836f9f62f18bd52b4b6c553c7506 Mon Sep 17 00:00:00 2001 From: Shawn Tabrizi Date: Sat, 14 Sep 2024 14:14:04 -0400 Subject: [PATCH 15/16] Update substrate/frame/vesting/src/lib.rs --- substrate/frame/vesting/src/lib.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/substrate/frame/vesting/src/lib.rs b/substrate/frame/vesting/src/lib.rs index a897270f2542..15f8d397f81c 100644 --- a/substrate/frame/vesting/src/lib.rs +++ b/substrate/frame/vesting/src/lib.rs @@ -555,7 +555,7 @@ impl Pallet { schedule.locked(), schedule.per_block(), schedule.starting_block(), - )?; + ); debug_assert!(res.is_ok(), "Failed to add a schedule when we had to succeed."); Ok(()) From a66f794d97e3a4bce5e3e570ce87d6ad59833c14 Mon Sep 17 00:00:00 2001 From: Shawn Tabrizi Date: Thu, 3 Oct 2024 00:52:19 -0400 Subject: [PATCH 16/16] Update lockable.rs --- .../support/src/traits/tokens/currency/lockable.rs | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/substrate/frame/support/src/traits/tokens/currency/lockable.rs b/substrate/frame/support/src/traits/tokens/currency/lockable.rs index 04579b0ff585..4ec45c908e68 100644 --- a/substrate/frame/support/src/traits/tokens/currency/lockable.rs +++ b/substrate/frame/support/src/traits/tokens/currency/lockable.rs @@ -146,10 +146,14 @@ pub trait VestedTransfer { } // An no-op implementation of `VestedTransfer` for pallets that require this trait, but users may -// not want to implement this functionality. -impl VestedTransfer for () { +// not want to implement this functionality +pub struct NoVestedTransfers { + phantom: core::marker::PhantomData, +} + +impl> VestedTransfer for NoVestedTransfers { type Moment = (); - type Currency = (); + type Currency = C; fn vested_transfer( _source: &AccountId,