From 9cbae9db93e5a4ce7fa8c61393d42aa2d0428cd9 Mon Sep 17 00:00:00 2001 From: thiolliere Date: Thu, 27 Jun 2019 14:59:12 +0200 Subject: [PATCH] address comments --- srml/staking/src/lib.rs | 64 +++++++++++++++++++--------------------- srml/staking/src/mock.rs | 1 - 2 files changed, 31 insertions(+), 34 deletions(-) diff --git a/srml/staking/src/lib.rs b/srml/staking/src/lib.rs index 70abab132aa14..e4629b3021614 100644 --- a/srml/staking/src/lib.rs +++ b/srml/staking/src/lib.rs @@ -305,6 +305,15 @@ const STAKING_ID: LockIdentifier = *b"staking "; /// Counter for the number of eras that have passed. pub type EraIndex = u32; +/// Reward of an era. +#[derive(Encode, Decode, Default)] +pub struct EraRewards { + total: u32, + /// Reward at one index correspond to reward for validator in current_elected of this index. + /// Thus this reward vec is only valid for one elected set. + rewards: Vec, +} + /// Indicates the initial status of the staker. #[cfg_attr(feature = "std", derive(Debug, Serialize, Deserialize))] pub enum StakerStatus { @@ -483,8 +492,6 @@ decl_storage! { /// Minimum number of staking participants before emergency conditions are imposed. pub MinimumValidatorCount get(minimum_validator_count) config(): u32 = DEFAULT_MINIMUM_VALIDATOR_COUNT; - /// Maximum reward, per validator, that is provided per acceptable session. - pub SessionReward get(session_reward) config(): Perbill = Perbill::from_parts(60); /// Slash, per validator that is taken for the first time they are found to be offline. pub OfflineSlash get(offline_slash) config(): Perbill = Perbill::from_millionths(1000); /// Number of instances of offline reports before slashing begins for validators. @@ -531,14 +538,11 @@ decl_storage! { /// The current era index. pub CurrentEra get(current_era) config(): EraIndex; - /// The accumulated reward for the current era. Reset to zero at the beginning of the era and - /// increased for every successfully finished session. + /// The start of the current era. pub CurrentEraStart get(current_era_start): T::Moment; - /// A vec of validator to reward and their associated number of points. - /// A validator can appear in many elements of this vec. - // TODO TODO: should we use such a structure ? aggregate with an existing one ? use a map ? - pub CurrentEraRewards: Vec<(T::AccountId, u32)>; + /// Rewards for the current era. Using indices of current elected set. + pub CurrentEraRewards: EraRewards; /// The amount of balance actively at stake for each validator slot, currently. /// @@ -905,8 +909,18 @@ decl_module! { /// /// At the end of the era each the total payout will be distributed among validator /// relatively to their points. - fn add_reward_points_to_validators(add: Vec<(T::AccountId, u32)>) { - >::append(&add[..]).unwrap(); + fn add_reward_points_to_validator(validator: T::AccountId, points: u32) { + >::current_elected().iter() + .position(|elected| *elected == validator) + .map(|index| { + >::mutate(|rewards| { + if let Some(new_total) = rewards.total.checked_add(points) { + rewards.total = new_total; + rewards.rewards.resize(index + 1, 0); + rewards.rewards[index] += points; // Addition is less than total + } + }) + }); } } } @@ -1018,8 +1032,6 @@ impl Module { /// Session has just ended. Provide the validator set for the next session if it's an era-end. fn new_session(session_index: SessionIndex) -> Option> { - // TODO TODO: previous implementation was accumulating reward here, should we accumulate - // duration in a way ? if >::take() || session_index % T::SessionsPerEra::get() == 0 { Self::new_era() } else { @@ -1045,34 +1057,21 @@ impl Module { let validator_len: BalanceOf = (validators.len() as u32).into(); let total_rewarded_stake = Self::slot_stake() * validator_len; - let mut total_points = 0u32; - let mut points_for_validator = BTreeMap::new(); - for (validator, reward) in rewards { - // If the total number of points exceed u32::MAX then they are dismissed - // TODO TODO: if this is a problem we can use u64 - if let Some(p) = total_points.checked_add(reward) { - total_points = p; - - // Cannot overflow as inferior to total_points - *points_for_validator.entry(validator).or_insert(0) += reward; - } - } - - // TODO TODO: does this should be generic ? let total_payout = inflation::compute_total_payout( total_rewarded_stake.clone(), T::Currency::total_issuance(), >::from( - // Era of duration more than u32::MAX is rewarded as u32::max_value. + // Era of duration more than u32::MAX is rewarded as u32::MAX. TryInto::::try_into(era_duration).unwrap_or(u32::max_value()) ), ); let mut total_imbalance = >::zero(); - for v in validators.iter() { - if let Some(&points) = points_for_validator.get(v) { - let reward = multiply_by_fraction(total_payout, points, total_points); + let total_points = rewards.total; + for (v, points) in validators.iter().zip(rewards.rewards.into_iter()) { + if points != 0 { + let reward = multiply_by_rational(total_payout, points, total_points); total_imbalance.subsume(Self::reward_validator(v, reward)); } } @@ -1289,7 +1288,7 @@ impl OnFreeBalanceZero for Module { // This is guarantee not to overflow on whatever values. // `num` must be inferior to `den` otherwise it will be reduce to `den`. -fn multiply_by_fraction(value: N, num: u32, den: u32) -> N +fn multiply_by_rational(value: N, num: u32, den: u32) -> N where N: SimpleArithmetic + Clone { let num = num.min(den); @@ -1303,8 +1302,7 @@ fn multiply_by_fraction(value: N, num: u32, den: u32) -> N let rem_u32 = rem.saturated_into::(); // Multiplication fits into u64 as both term are u32 - let rem_part = rem_u32 as u64 * num as u64 - / den as u64; + let rem_part = rem_u32 as u64 * num as u64 / den as u64; // Result fits into u32 as num < total_points (rem_part as u32).into() diff --git a/srml/staking/src/mock.rs b/srml/staking/src/mock.rs index ea4fcbd8f9f01..6e50fec8db5e9 100644 --- a/srml/staking/src/mock.rs +++ b/srml/staking/src/mock.rs @@ -235,7 +235,6 @@ impl ExtBuilder { ], validator_count: self.validator_count, minimum_validator_count: self.minimum_validator_count, - session_reward: Perbill::from_millionths((1000000 * self.reward / balance_factor) as u32), offline_slash: Perbill::from_percent(5), current_session_reward: self.reward, offline_slash_grace: 0,