From 420d610b46b9819128a4c58b45a65d75fa54342b Mon Sep 17 00:00:00 2001
From: Xiliang Chen <xlchen1291@gmail.com>
Date: Tue, 30 Jun 2020 17:16:19 +1200
Subject: [PATCH] fix orml-vesting (#223)
* fix vesting
* add tests
---
traits/src/currency.rs | 5 +-
vesting/src/lib.rs | 63 +++++++++++++++++++------
vesting/src/mock.rs | 2 +
vesting/src/tests.rs | 103 +++++++++++++++++++++++++++--------------
4 files changed, 120 insertions(+), 53 deletions(-)
diff --git a/traits/src/currency.rs b/traits/src/currency.rs
index 83f73f3..b51c118 100644
--- a/traits/src/currency.rs
+++ b/traits/src/currency.rs
@@ -289,14 +289,11 @@ pub trait BasicReservableCurrency<AccountId>: BasicCurrency<AccountId> {
) -> result::Result<Self::Balance, DispatchError>;
}
+#[impl_trait_for_tuples::impl_for_tuples(30)]
pub trait OnDustRemoval<CurrencyId, Balance> {
fn on_dust_removal(currency_id: CurrencyId, balance: Balance);
}
-impl<CurrencyId, Balance> OnDustRemoval<CurrencyId, Balance> for () {
- fn on_dust_removal(_: CurrencyId, _: Balance) {}
-}
-
#[impl_trait_for_tuples::impl_for_tuples(30)]
pub trait OnReceived<AccountId, CurrencyId, Balance> {
fn on_received(account: &AccountId, currency: CurrencyId, amount: Balance);
diff --git a/vesting/src/lib.rs b/vesting/src/lib.rs
index 69d2748..e5b40c0 100644
--- a/vesting/src/lib.rs
+++ b/vesting/src/lib.rs
@@ -16,7 +16,7 @@
//!
//! ### Dispatchable Functions
//!
-//! - `add_vesting_schedule` - Add a new vesting schedule for an account.
+//! - `vested_transfer` - Add a new vesting schedule for an account.
//! - `claim` - Claim unlocked balances.
//! - `update_vesting_schedules` - Update all vesting schedules under an account, `root` origin required.
@@ -37,13 +37,16 @@ use sp_std::{
// #3295 https://github.com/paritytech/substrate/issues/3295
use frame_system::{self as system, ensure_root, ensure_signed};
use sp_runtime::{
- traits::{AtLeast32Bit, CheckedAdd, StaticLookup, Zero},
+ traits::{AtLeast32Bit, CheckedAdd, Saturating, StaticLookup, Zero},
DispatchResult, RuntimeDebug,
};
mod mock;
mod tests;
+/// The maximum number of vesting schedules an account can have.
+pub const MAX_VESTINGS: usize = 20;
+
/// The vesting schedule.
///
/// Benefits would be granted gradually, `per_period` amount every `period` of blocks
@@ -60,6 +63,7 @@ pub struct VestingSchedule<BlockNumber, Balance: HasCompact> {
impl<BlockNumber: AtLeast32Bit + Copy, Balance: AtLeast32Bit + Copy> VestingSchedule<BlockNumber, Balance> {
/// Returns the end of all periods, `None` if calculation overflows.
pub fn end(&self) -> Option<BlockNumber> {
+ // period * period_count + start
self.period
.checked_mul(&self.period_count.into())?
.checked_add(&self.start)
@@ -75,6 +79,9 @@ impl<BlockNumber: AtLeast32Bit + Copy, Balance: AtLeast32Bit + Copy> VestingSche
/// Note this func assumes schedule is a valid one(non-zero period and non-overflow total amount),
/// and it should be guaranteed by callers.
pub fn locked_amount(&self, time: BlockNumber) -> Balance {
+ // full = (time - start) / period
+ // unrealized = period_count - full
+ // per_period * unrealized
let full = time
.saturating_sub(self.start)
.checked_div(&self.period)
@@ -99,6 +106,9 @@ pub type ScheduledItem<T> = (
pub trait Trait: frame_system::Trait {
type Event: From<Event<Self>> + Into<<Self as frame_system::Trait>::Event>;
type Currency: LockableCurrency<Self::AccountId, Moment = Self::BlockNumber>;
+
+ /// The minimum amount transferred to call `vested_transfer`.
+ type MinVestedTransfer: Get<BalanceOf<Self>>;
}
decl_storage! {
@@ -140,6 +150,8 @@ decl_error! {
ZeroVestingPeriodCount,
NumOverflow,
InsufficientBalanceToLock,
+ TooManyVestingSchedules,
+ AmountLow,
}
}
@@ -147,6 +159,9 @@ decl_module! {
pub struct Module<T: Trait> for enum Call where origin: T::Origin {
type Error = Error<T>;
+ /// The minimum amount to be transferred to create a new vesting schedule.
+ const MinVestedTransfer: BalanceOf<T> = T::MinVestedTransfer::get();
+
fn deposit_event() = default;
/// # <weight>
@@ -176,14 +191,14 @@ decl_module! {
/// Base Weight: 47.26 µs
/// # </weight>
#[weight = 48 * WEIGHT_PER_MICROS + T::DbWeight::get().reads_writes(4, 4)]
- pub fn add_vesting_schedule(
+ pub fn vested_transfer(
origin,
dest: <T::Lookup as StaticLookup>::Source,
schedule: VestingScheduleOf<T>,
) {
let from = ensure_signed(origin)?;
let to = T::Lookup::lookup(dest)?;
- Self::do_add_vesting_schedule(&from, &to, schedule.clone())?;
+ Self::do_vested_transfer(&from, &to, schedule.clone())?;
Self::deposit_event(RawEvent::VestingScheduleAdded(from, to, schedule));
}
@@ -229,24 +244,40 @@ impl<T: Trait> Module<T> {
/// Returns locked balance based on current block number.
fn locked_balance(who: &T::AccountId) -> BalanceOf<T> {
let now = <frame_system::Module<T>>::block_number();
- Self::vesting_schedules(who)
- .iter()
- .fold(Zero::zero(), |acc, s| acc + s.locked_amount(now))
+ <VestingSchedules<T>>::mutate_exists(who, |maybe_schedules| {
+ let total = if let Some(schedules) = maybe_schedules.as_mut() {
+ let mut total: BalanceOf<T> = Zero::zero();
+ schedules.retain(|s| {
+ let amount = s.locked_amount(now);
+ total = total.saturating_add(amount);
+ !amount.is_zero()
+ });
+ total
+ } else {
+ Zero::zero()
+ };
+ if total.is_zero() {
+ *maybe_schedules = None;
+ }
+ total
+ })
}
- fn do_add_vesting_schedule(
- from: &T::AccountId,
- to: &T::AccountId,
- schedule: VestingScheduleOf<T>,
- ) -> DispatchResult {
+ fn do_vested_transfer(from: &T::AccountId, to: &T::AccountId, schedule: VestingScheduleOf<T>) -> DispatchResult {
let schedule_amount = Self::ensure_valid_vesting_schedule(&schedule)?;
+
+ ensure!(
+ <VestingSchedules<T>>::decode_len(to).unwrap_or(0) < MAX_VESTINGS,
+ Error::<T>::TooManyVestingSchedules
+ );
+
let total_amount = Self::locked_balance(to)
.checked_add(&schedule_amount)
.ok_or(Error::<T>::NumOverflow)?;
T::Currency::transfer(from, to, schedule_amount, ExistenceRequirement::AllowDeath)?;
T::Currency::set_lock(VESTING_LOCK_ID, to, total_amount, WithdrawReasons::all());
- <VestingSchedules<T>>::mutate(to, |v| (*v).push(schedule));
+ <VestingSchedules<T>>::append(to, schedule);
Ok(())
}
@@ -276,6 +307,10 @@ impl<T: Trait> Module<T> {
ensure!(!schedule.period_count.is_zero(), Error::<T>::ZeroVestingPeriodCount);
ensure!(schedule.end().is_some(), Error::<T>::NumOverflow);
- schedule.total_amount().ok_or(Error::<T>::NumOverflow)
+ let total = schedule.total_amount().ok_or(Error::<T>::NumOverflow)?;
+
+ ensure!(total >= T::MinVestedTransfer::get(), Error::<T>::AmountLow);
+
+ Ok(total)
}
}
diff --git a/vesting/src/mock.rs b/vesting/src/mock.rs
index 953f3eb..9b4d0c8 100644
--- a/vesting/src/mock.rs
+++ b/vesting/src/mock.rs
@@ -67,6 +67,7 @@ type Balance = u64;
parameter_types! {
pub const ExistentialDeposit: u64 = 1;
+ pub const MinVestedTransfer: u64 = 5;
}
impl pallet_balances::Trait for Runtime {
@@ -81,6 +82,7 @@ pub type PalletBalances = pallet_balances::Module<Runtime>;
impl Trait for Runtime {
type Event = TestEvent;
type Currency = PalletBalances;
+ type MinVestedTransfer = MinVestedTransfer;
}
pub type Vesting = Module<Runtime>;
diff --git a/vesting/src/tests.rs b/vesting/src/tests.rs
index ecf65b2..25a5c34 100644
--- a/vesting/src/tests.rs
+++ b/vesting/src/tests.rs
@@ -8,7 +8,7 @@ use mock::{ExtBuilder, Origin, PalletBalances, Runtime, System, TestEvent, Vesti
use pallet_balances::{BalanceLock, Reasons};
#[test]
-fn add_vesting_schedule_works() {
+fn vested_transfer_works() {
ExtBuilder::default().one_hundred_for_alice().build().execute_with(|| {
System::set_block_number(1);
@@ -18,11 +18,7 @@ fn add_vesting_schedule_works() {
period_count: 1u32,
per_period: 100u64,
};
- assert_ok!(Vesting::add_vesting_schedule(
- Origin::signed(ALICE),
- BOB,
- schedule.clone()
- ));
+ assert_ok!(Vesting::vested_transfer(Origin::signed(ALICE), BOB, schedule.clone()));
assert_eq!(Vesting::vesting_schedules(&BOB), vec![schedule.clone()]);
let vested_event = TestEvent::vesting(RawEvent::VestingScheduleAdded(ALICE, BOB, schedule));
@@ -39,7 +35,7 @@ fn add_new_vesting_schedule_merges_with_current_locked_balance_and_until() {
period_count: 2u32,
per_period: 10u64,
};
- assert_ok!(Vesting::add_vesting_schedule(Origin::signed(ALICE), BOB, schedule));
+ assert_ok!(Vesting::vested_transfer(Origin::signed(ALICE), BOB, schedule));
System::set_block_number(12);
@@ -49,11 +45,7 @@ fn add_new_vesting_schedule_merges_with_current_locked_balance_and_until() {
period_count: 1u32,
per_period: 7u64,
};
- assert_ok!(Vesting::add_vesting_schedule(
- Origin::signed(ALICE),
- BOB,
- another_schedule
- ));
+ assert_ok!(Vesting::vested_transfer(Origin::signed(ALICE), BOB, another_schedule));
assert_eq!(
PalletBalances::locks(&BOB).pop(),
@@ -75,17 +67,13 @@ fn cannot_use_fund_if_not_claimed() {
period_count: 1u32,
per_period: 50u64,
};
- assert_ok!(Vesting::add_vesting_schedule(
- Origin::signed(ALICE),
- BOB,
- schedule.clone()
- ));
+ assert_ok!(Vesting::vested_transfer(Origin::signed(ALICE), BOB, schedule.clone()));
assert!(PalletBalances::ensure_can_withdraw(&BOB, 1, WithdrawReason::Transfer.into(), 49).is_err());
});
}
#[test]
-fn add_vesting_schedule_fails_if_zero_period_or_count() {
+fn vested_transfer_fails_if_zero_period_or_count() {
ExtBuilder::default().one_hundred_for_alice().build().execute_with(|| {
let schedule = VestingSchedule {
start: 1u64,
@@ -94,7 +82,7 @@ fn add_vesting_schedule_fails_if_zero_period_or_count() {
per_period: 100u64,
};
assert_err!(
- Vesting::add_vesting_schedule(Origin::signed(ALICE), BOB, schedule.clone()),
+ Vesting::vested_transfer(Origin::signed(ALICE), BOB, schedule.clone()),
Error::<Runtime>::ZeroVestingPeriod
);
@@ -105,14 +93,14 @@ fn add_vesting_schedule_fails_if_zero_period_or_count() {
per_period: 100u64,
};
assert_err!(
- Vesting::add_vesting_schedule(Origin::signed(ALICE), BOB, schedule.clone()),
+ Vesting::vested_transfer(Origin::signed(ALICE), BOB, schedule.clone()),
Error::<Runtime>::ZeroVestingPeriodCount
);
});
}
#[test]
-fn add_vesting_schedule_fails_if_transfer_err() {
+fn vested_transfer_fails_if_transfer_err() {
ExtBuilder::default().one_hundred_for_alice().build().execute_with(|| {
let schedule = VestingSchedule {
start: 1u64,
@@ -121,14 +109,14 @@ fn add_vesting_schedule_fails_if_transfer_err() {
per_period: 100u64,
};
assert_err!(
- Vesting::add_vesting_schedule(Origin::signed(BOB), ALICE, schedule.clone()),
+ Vesting::vested_transfer(Origin::signed(BOB), ALICE, schedule.clone()),
pallet_balances::Error::<Runtime, _>::InsufficientBalance,
);
});
}
#[test]
-fn add_vesting_schedule_fails_if_overflow() {
+fn vested_transfer_fails_if_overflow() {
ExtBuilder::default().one_hundred_for_alice().build().execute_with(|| {
let schedule = VestingSchedule {
start: 1u64,
@@ -137,7 +125,7 @@ fn add_vesting_schedule_fails_if_overflow() {
per_period: u64::max_value(),
};
assert_err!(
- Vesting::add_vesting_schedule(Origin::signed(ALICE), BOB, schedule),
+ Vesting::vested_transfer(Origin::signed(ALICE), BOB, schedule),
Error::<Runtime>::NumOverflow
);
@@ -148,7 +136,7 @@ fn add_vesting_schedule_fails_if_overflow() {
per_period: 1u64,
};
assert_err!(
- Vesting::add_vesting_schedule(Origin::signed(ALICE), BOB, another_schedule),
+ Vesting::vested_transfer(Origin::signed(ALICE), BOB, another_schedule),
Error::<Runtime>::NumOverflow
);
});
@@ -163,11 +151,7 @@ fn claim_works() {
period_count: 2u32,
per_period: 10u64,
};
- assert_ok!(Vesting::add_vesting_schedule(
- Origin::signed(ALICE),
- BOB,
- schedule.clone()
- ));
+ assert_ok!(Vesting::vested_transfer(Origin::signed(ALICE), BOB, schedule.clone()));
System::set_block_number(11);
// remain locked if not claimed
@@ -199,11 +183,7 @@ fn update_vesting_schedules_works() {
period_count: 2u32,
per_period: 10u64,
};
- assert_ok!(Vesting::add_vesting_schedule(
- Origin::signed(ALICE),
- BOB,
- schedule.clone()
- ));
+ assert_ok!(Vesting::vested_transfer(Origin::signed(ALICE), BOB, schedule.clone()));
let updated_schedule = VestingSchedule {
start: 0u64,
@@ -234,3 +214,56 @@ fn update_vesting_schedules_fails_if_unexpected_existing_locks() {
PalletBalances::set_lock(*b"prelocks", &BOB, 0u64, WithdrawReasons::all());
});
}
+
+#[test]
+fn vested_transfer_check_for_min() {
+ ExtBuilder::default().one_hundred_for_alice().build().execute_with(|| {
+ let schedule = VestingSchedule {
+ start: 1u64,
+ period: 1u64,
+ period_count: 1u32,
+ per_period: 3u64,
+ };
+ assert_err!(
+ Vesting::vested_transfer(Origin::signed(BOB), ALICE, schedule.clone()),
+ Error::<Runtime>::AmountLow
+ );
+ });
+}
+
+#[test]
+fn multiple_vesting_schedule_claim_works() {
+ ExtBuilder::default().one_hundred_for_alice().build().execute_with(|| {
+ let schedule = VestingSchedule {
+ start: 0u64,
+ period: 10u64,
+ period_count: 2u32,
+ per_period: 10u64,
+ };
+ assert_ok!(Vesting::vested_transfer(Origin::signed(ALICE), BOB, schedule.clone()));
+
+ let schedule2 = VestingSchedule {
+ start: 0u64,
+ period: 10u64,
+ period_count: 3u32,
+ per_period: 10u64,
+ };
+ assert_ok!(Vesting::vested_transfer(Origin::signed(ALICE), BOB, schedule2.clone()));
+
+ assert_eq!(Vesting::vesting_schedules(&BOB), vec![schedule, schedule2.clone()]);
+
+ System::set_block_number(21);
+
+ assert_ok!(Vesting::claim(Origin::signed(BOB)));
+
+ assert_eq!(Vesting::vesting_schedules(&BOB), vec![schedule2]);
+
+ System::set_block_number(31);
+
+ assert_ok!(Vesting::claim(Origin::signed(BOB)));
+
+ assert_eq!(VestingSchedules::<Runtime>::contains_key(&BOB), false);
+
+ assert_eq!(PalletBalances::locks(&BOB), vec![]);
+ });
+}
--
GitLab