Skip to content
This repository has been archived by the owner on Nov 15, 2023. It is now read-only.

Make Staking pallet using a proper Time module. #4662

Merged
merged 15 commits into from
Mar 26, 2020
1 change: 1 addition & 0 deletions Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion bin/node/runtime/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -283,7 +283,7 @@ parameter_types! {

impl pallet_staking::Trait for Runtime {
type Currency = Balances;
type Time = Timestamp;
type UnixTime = Timestamp;
type CurrencyToVote = CurrencyToVoteHandler;
type RewardRemainder = Treasury;
type Event = Event;
Expand Down
1 change: 1 addition & 0 deletions frame/staking/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@ pallet-staking-reward-curve = { version = "2.0.0-alpha.5", path = "../staking/r
substrate-test-utils = { version = "2.0.0-alpha.5", path = "../../test-utils" }
frame-benchmarking = { version = "2.0.0-alpha.5", path = "../benchmarking" }
rand_chacha = { version = "0.2" }
hex = "0.4"

[features]
default = ["std"]
Expand Down
45 changes: 27 additions & 18 deletions frame/staking/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -262,11 +262,12 @@ pub mod inflation;
use sp_std::{prelude::*, result, collections::btree_map::BTreeMap};
use codec::{HasCompact, Encode, Decode};
use frame_support::{
decl_module, decl_event, decl_storage, ensure, decl_error, weights::SimpleDispatchInfo,
decl_module, decl_event, decl_storage, ensure, decl_error,
dispatch::DispatchResult, storage::IterableStorageMap, traits::{
Currency, LockIdentifier, LockableCurrency, WithdrawReasons, OnUnbalanced, Imbalance, Get,
Time
}
UnixTime
},
weights::{SimpleDispatchInfo, Weight},
};
use pallet_session::historical::SessionManager;
use sp_runtime::{
Expand Down Expand Up @@ -300,14 +301,14 @@ pub type RewardPoint = u32;

/// Information regarding the active era (era in used in session).
#[derive(Encode, Decode, RuntimeDebug)]
pub struct ActiveEraInfo<Moment> {
pub struct ActiveEraInfo {
/// Index of era.
index: EraIndex,
/// Moment of start
/// Moment of start expresed as millisecond from `$UNIX_EPOCH`.
///
/// Start can be none if start hasn't been set for the era yet,
/// Start is set on the first on_finalize of the era to guarantee usage of `Time`.
start: Option<Moment>,
start: Option<u64>,
}

/// Reward points of an era. Used to split era total payout between validators.
Expand Down Expand Up @@ -564,7 +565,6 @@ type PositiveImbalanceOf<T> =
<<T as Trait>::Currency as Currency<<T as frame_system::Trait>::AccountId>>::PositiveImbalance;
type NegativeImbalanceOf<T> =
<<T as Trait>::Currency as Currency<<T as frame_system::Trait>::AccountId>>::NegativeImbalance;
type MomentOf<T> = <<T as Trait>::Time as Time>::Moment;

/// Means for interacting with a specialized version of the `session` trait.
///
Expand Down Expand Up @@ -613,7 +613,7 @@ pub trait Trait: frame_system::Trait {
///
/// It is guaranteed to start being called from the first `on_finalize`. Thus value at genesis
/// is not used.
type Time: Time;
type UnixTime: UnixTime;

/// Convert a balance into a number used for election calculation.
/// This must fit into a `u64` but is allowed to be sensibly lossy.
Expand Down Expand Up @@ -686,11 +686,12 @@ impl Default for Forcing {
enum Releases {
V1_0_0Ancient,
V2_0_0,
V3_0_0,
}

impl Default for Releases {
fn default() -> Self {
Releases::V2_0_0
Releases::V3_0_0
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure what to think about this default, because if we add staking in a running chain (so without executing GenesisConfig) then we must insert the storage version in the runtime upgrade.
I think a staking module without any storage version must be considered invalid now.

otherwise if a chain introduce staking module without inserting the storage version then the storage version depends on when it has been inserted, and storage version could be V2 or V3 there is no way to know it.

}
}

Expand Down Expand Up @@ -746,7 +747,7 @@ decl_storage! {
///
/// The active era is the era currently rewarded.
/// Validator set of this era must be equal to `SessionInterface::validators`.
pub ActiveEra get(fn active_era): Option<ActiveEraInfo<MomentOf<T>>>;
pub ActiveEra get(fn active_era): Option<ActiveEraInfo>;

/// The session index at which the era start for the last `HISTORY_DEPTH` eras
pub ErasStartSessionIndex get(fn eras_start_session_index):
Expand Down Expand Up @@ -850,8 +851,8 @@ decl_storage! {

/// Storage version of the pallet.
///
/// This is set to v2.0.0 for new networks.
StorageVersion build(|_: &GenesisConfig<T>| Releases::V2_0_0): Releases;
/// This is set to v3.0.0 for new networks.
StorageVersion build(|_: &GenesisConfig<T>| Releases::V3_0_0): Releases;
}
add_extra_genesis {
config(stakers):
Expand Down Expand Up @@ -959,12 +960,20 @@ decl_module! {
// Set the start of the first era.
if let Some(mut active_era) = Self::active_era() {
if active_era.start.is_none() {
active_era.start = Some(T::Time::now());
<ActiveEra<T>>::put(active_era);
let now_as_millis_u64 = T::UnixTime::now().as_millis().saturated_into::<u64>();
active_era.start = Some(now_as_millis_u64);
ActiveEra::put(active_era);
}
}
}

fn on_runtime_upgrade() -> Weight {
// For Kusama the type hasn't actually changed as Moment was u64 and was the number of
// millisecond since unix epoch.
StorageVersion::put(Releases::V3_0_0);
0
}

/// Take the origin account as a stash and lock up `value` of its balance. `controller` will
/// be the account that controls it.
///
Expand Down Expand Up @@ -1696,7 +1705,7 @@ impl<T: Trait> Module<T> {
/// * reset `active_era.start`,
/// * update `BondedEras` and apply slashes.
fn start_era(start_session: SessionIndex) {
let active_era = <ActiveEra<T>>::mutate(|active_era| {
let active_era = ActiveEra::mutate(|active_era| {
let new_index = active_era.as_ref().map(|info| info.index + 1).unwrap_or(0);
*active_era = Some(ActiveEraInfo {
index: new_index,
Expand Down Expand Up @@ -1734,12 +1743,12 @@ impl<T: Trait> Module<T> {
}

/// Compute payout for era.
fn end_era(active_era: ActiveEraInfo<MomentOf<T>>, _session_index: SessionIndex) {
fn end_era(active_era: ActiveEraInfo, _session_index: SessionIndex) {
// Note: active_era_start can be None if end era is called during genesis config.
if let Some(active_era_start) = active_era.start {
let now = T::Time::now();
let now_as_millis_u64 = T::UnixTime::now().as_millis().saturated_into::<u64>();

let era_duration = now - active_era_start;
let era_duration = now_as_millis_u64 - active_era_start;
let (total_payout, _max_payout) = inflation::compute_total_payout(
&T::RewardCurve::get(),
Self::eras_total_stake(&active_era.index),
Expand Down
4 changes: 2 additions & 2 deletions frame/staking/src/mock.rs
Original file line number Diff line number Diff line change
Expand Up @@ -201,8 +201,8 @@ parameter_types! {
pub const MaxNominatorRewardedPerValidator: u32 = 64;
}
impl Trait for Test {
type Currency = pallet_balances::Module<Self>;
type Time = pallet_timestamp::Module<Self>;
type Currency = Balances;
type UnixTime = Timestamp;
type CurrencyToVote = CurrencyToVoteHandler;
type RewardRemainder = ();
type Event = ();
Expand Down
11 changes: 9 additions & 2 deletions frame/staking/src/tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -21,9 +21,8 @@ use mock::*;
use sp_runtime::{assert_eq_error_rate, traits::BadOrigin};
use sp_staking::offence::OffenceDetails;
use frame_support::{
assert_ok, assert_noop,
assert_ok, assert_noop, StorageMap,
traits::{Currency, ReservableCurrency, OnInitialize},
StorageMap,
};
use pallet_balances::Error as BalancesError;
use substrate_test_utils::assert_eq_uvec;
Expand Down Expand Up @@ -3039,3 +3038,11 @@ fn set_history_depth_works() {
assert!(!<Staking as Store>::ErasTotalStake::contains_key(10 - 5));
});
}

#[test]
fn assert_migration_is_noop() {
let kusama_active_era = "4a0200000190e2721171010000";
let era = ActiveEraInfo::decode(&mut &hex::decode(kusama_era_start).unwrap()[..]).unwrap();
assert_eq!(era.index, 586);
assert_eq!(era.start, Some(1585135674000));
}
6 changes: 6 additions & 0 deletions frame/support/src/traits.rs
Original file line number Diff line number Diff line change
Expand Up @@ -858,6 +858,12 @@ pub trait Time {
fn now() -> Self::Moment;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now you are mixing some time related functionality (year()) with a moment that can be anything.

I still think we should keep this Time trait simple and say that now() will return a moment that increases between blocks, but without specifying what it is. It could also be the moment from the genesis of the chain.

I would add another trait UnixTime that clearly says that now() returns the Duration from the unix epoch. I would directly return core::time::Duration. This already brings all the functions for converting between the different time formats.

The timestamp module would implement both of these traits and return the same value for both now() calls, but as different types. Staking would require that Time implements UnixTime.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, I completely agree

}

/// Trait to deal with unix time.
pub trait UnixTime {
/// Return duration since `SystemTime::UNIX_EPOCH`.
fn now() -> core::time::Duration;
}

impl WithdrawReasons {
/// Choose all variants except for `one`.
///
Expand Down
25 changes: 22 additions & 3 deletions frame/timestamp/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -95,15 +95,17 @@ mod benchmarking;

use sp_std::{result, cmp};
use sp_inherents::{ProvideInherent, InherentData, InherentIdentifier};
use frame_support::{Parameter, decl_storage, decl_module};
use frame_support::traits::{Time, Get};
use frame_support::{
Parameter, decl_storage, decl_module, debug,
traits::{Time, UnixTime, Get},
weights::SimpleDispatchInfo,
};
use sp_runtime::{
RuntimeString,
traits::{
AtLeast32Bit, Zero, SaturatedConversion, Scale
}
};
use frame_support::weights::SimpleDispatchInfo;
use frame_system::ensure_none;
use sp_timestamp::{
InherentError, INHERENT_IDENTIFIER, InherentType,
Expand Down Expand Up @@ -239,6 +241,23 @@ impl<T: Trait> Time for Module<T> {
}
}

/// Before the timestamp inherent is applied, it returns the time of previous block.
///
/// On genesis the time returned is not valid.
impl<T: Trait> UnixTime for Module<T> {
fn now() -> core::time::Duration {
// now is duration since unix epoch in millisecond as documented in
// `sp_timestamp::InherentDataProvider`.
let now = Self::now();
if now == T::Moment::zero() {
debug::error!(
"`pallet_timestamp::UnixTime::now` is called at genesis, invalid value returned: 0"
);
}
gui1117 marked this conversation as resolved.
Show resolved Hide resolved
core::time::Duration::from_millis(now.saturated_into::<u64>())
}
}

#[cfg(test)]
mod tests {
use super::*;
Expand Down
1 change: 1 addition & 0 deletions primitives/timestamp/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -77,6 +77,7 @@ impl TimestampInherentData for InherentData {
}
}

/// Provide duration since unix epoch in millisecond for timestamp inherent.
#[cfg(feature = "std")]
pub struct InherentDataProvider;

Expand Down