From c3419ddbfdb269381634625ad4bd0818b80ed183 Mon Sep 17 00:00:00 2001 From: kianenigma Date: Fri, 14 Aug 2020 17:29:50 +0200 Subject: [PATCH 1/6] Refactor CurrencyToVote to avoid calls to total_issuance. --- Cargo.lock | 4 +- bin/node/runtime/src/impls.rs | 16 ------ bin/node/runtime/src/lib.rs | 6 +-- frame/babe/src/mock.rs | 18 +------ frame/elections-phragmen/src/lib.rs | 37 +++++-------- frame/grandpa/src/mock.rs | 18 +------ frame/offences/benchmarking/src/mock.rs | 15 +----- frame/session/benchmarking/src/mock.rs | 16 +----- frame/staking/fuzzer/src/mock.rs | 15 +----- frame/staking/src/lib.rs | 40 +++++++++----- frame/staking/src/mock.rs | 30 +++-------- frame/staking/src/offchain_election.rs | 4 +- frame/staking/src/testing_utils.rs | 17 +++--- frame/support/src/traits.rs | 69 +++++++++++++++++++++++- primitives/npos-elections/src/helpers.rs | 4 +- 15 files changed, 135 insertions(+), 174 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index dc664f97ddeb5..3ec07bb080ad3 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -8624,8 +8624,8 @@ name = "substrate-test-utils-derive" version = "0.8.0-rc5" dependencies = [ "proc-macro-crate", - "quote 1.0.6", - "syn 1.0.33", + "quote", + "syn", ] [[package]] diff --git a/bin/node/runtime/src/impls.rs b/bin/node/runtime/src/impls.rs index 039093ddee697..cd692de250447 100644 --- a/bin/node/runtime/src/impls.rs +++ b/bin/node/runtime/src/impls.rs @@ -29,22 +29,6 @@ impl OnUnbalanced for Author { } } -/// Struct that handles the conversion of Balance -> `u64`. This is used for staking's election -/// calculation. -pub struct CurrencyToVoteHandler; - -impl CurrencyToVoteHandler { - fn factor() -> Balance { (Balances::total_issuance() / u64::max_value() as Balance).max(1) } -} - -impl Convert for CurrencyToVoteHandler { - fn convert(x: Balance) -> u64 { (x / Self::factor()) as u64 } -} - -impl Convert for CurrencyToVoteHandler { - fn convert(x: u128) -> Balance { x * Self::factor() } -} - #[cfg(test)] mod multiplier_tests { use super::*; diff --git a/bin/node/runtime/src/lib.rs b/bin/node/runtime/src/lib.rs index acc1b07281834..9128868900693 100644 --- a/bin/node/runtime/src/lib.rs +++ b/bin/node/runtime/src/lib.rs @@ -78,7 +78,7 @@ pub use pallet_staking::StakerStatus; /// Implementations of some helper traits passed into runtime modules as associated types. pub mod impls; -use impls::{CurrencyToVoteHandler, Author}; +use impls::Author; /// Constant values used within the runtime. pub mod constants; @@ -423,7 +423,7 @@ parameter_types! { impl pallet_staking::Trait for Runtime { type Currency = Balances; type UnixTime = Timestamp; - type CurrencyToVote = CurrencyToVoteHandler; + type CurrencyToVote = frame_support::traits::U128CurrencyToVote; type RewardRemainder = Treasury; type Event = Event; type Slash = Treasury; // send the slashed funds to the treasury. @@ -533,7 +533,7 @@ impl pallet_elections_phragmen::Trait for Runtime { // NOTE: this implies that council's genesis members cannot be set directly and must come from // this module. type InitializeMembers = Council; - type CurrencyToVote = CurrencyToVoteHandler; + type CurrencyToVote = frame_support::traits::U128CurrencyToVote; type CandidacyBond = CandidacyBond; type VotingBond = VotingBond; type LoserCandidate = (); diff --git a/frame/babe/src/mock.rs b/frame/babe/src/mock.rs index 8a0356d8da7e8..99b58eb9ba096 100644 --- a/frame/babe/src/mock.rs +++ b/frame/babe/src/mock.rs @@ -23,7 +23,7 @@ use sp_runtime::{ Perbill, impl_opaque_keys, curve::PiecewiseLinear, testing::{Digest, DigestItem, Header, TestXt,}, - traits::{Convert, Header as _, IdentityLookup, OpaqueKeys, SaturatedConversion}, + traits::{Header as _, IdentityLookup, OpaqueKeys}, }; use frame_system::InitKind; use frame_support::{ @@ -182,23 +182,9 @@ parameter_types! { pub const StakingUnsignedPriority: u64 = u64::max_value() / 2; } -pub struct CurrencyToVoteHandler; - -impl Convert for CurrencyToVoteHandler { - fn convert(x: u128) -> u128 { - x - } -} - -impl Convert for CurrencyToVoteHandler { - fn convert(x: u128) -> u64 { - x.saturated_into() - } -} - impl pallet_staking::Trait for Test { type RewardRemainder = (); - type CurrencyToVote = CurrencyToVoteHandler; + type CurrencyToVote = frame_support::traits::IdentityCurrencyToVote; type Event = (); type Currency = Balances; type Slash = (); diff --git a/frame/elections-phragmen/src/lib.rs b/frame/elections-phragmen/src/lib.rs index 50c5de9bc0de4..797d7a5fefb30 100644 --- a/frame/elections-phragmen/src/lib.rs +++ b/frame/elections-phragmen/src/lib.rs @@ -87,7 +87,7 @@ use codec::{Encode, Decode}; use sp_std::prelude::*; use sp_runtime::{ DispatchError, RuntimeDebug, Perbill, - traits::{Zero, StaticLookup, Convert}, + traits::{Zero, StaticLookup}, }; use frame_support::{ decl_storage, decl_event, ensure, decl_module, decl_error, @@ -97,10 +97,10 @@ use frame_support::{ traits::{ Currency, Get, LockableCurrency, LockIdentifier, ReservableCurrency, WithdrawReasons, ChangeMembers, OnUnbalanced, WithdrawReason, Contains, BalanceStatus, InitializeMembers, - ContainsLengthBound, + ContainsLengthBound, CurrencyToVote, } }; -use sp_npos_elections::{build_support_map, ExtendedBalance, VoteWeight, ElectionResult}; +use sp_npos_elections::{build_support_map, VoteWeight, ElectionResult}; use frame_system::{ensure_signed, ensure_root}; mod benchmarking; @@ -189,7 +189,7 @@ pub trait Trait: frame_system::Trait { /// Convert a balance into a number used for election calculation. /// This must fit into a `u64` but is allowed to be sensibly lossy. - type CurrencyToVote: Convert, VoteWeight> + Convert>; + type CurrencyToVote: CurrencyToVote>; /// How much should be locked up in order to submit one's candidacy. type CandidacyBond: Get>; @@ -893,19 +893,17 @@ impl Module { // previous runners_up are also always candidates for the next round. candidates.append(&mut Self::runners_up_ids()); + // helper closures to deal with balance/stake. - let to_votes = |b: BalanceOf| -> VoteWeight { - , VoteWeight>>::convert(b) - }; - let to_balance = |e: ExtendedBalance| -> BalanceOf { - >>::convert(e) - }; + let total_issuance = T::Currency::total_issuance(); let stake_of = |who: &T::AccountId| -> VoteWeight { - to_votes(Self::locked_stake_of(who)) + ::to_vote(Self::locked_stake_of(who), total_issuance) }; let voters_and_votes = Voting::::iter() - .map(|(voter, (stake, targets))| { (voter, to_votes(stake), targets) }) + .map(|(voter, (stake, targets))| + (voter, ::to_vote(stake, total_issuance), targets) + ) .collect::>(); let maybe_phragmen_result = sp_npos_elections::seq_phragmen::( num_to_elect, @@ -953,7 +951,7 @@ impl Module { "entire new_set was given to build_support_map; en entry must be \ created for each item; qed" ); - (m.clone(), to_balance(support.total)) + (m.clone(), ::to_currency(support.total, total_issuance)) }) .collect::)>>(); @@ -1220,17 +1218,6 @@ mod tests { } } - /// Simple structure that exposes how u64 currency can be represented as... u64. - pub struct CurrencyToVoteHandler; - impl Convert for CurrencyToVoteHandler { - fn convert(x: u64) -> u64 { x } - } - impl Convert for CurrencyToVoteHandler { - fn convert(x: u128) -> u64 { - x as u64 - } - } - parameter_types!{ pub const ElectionsPhragmenModuleId: LockIdentifier = *b"phrelect"; } @@ -1239,7 +1226,7 @@ mod tests { type ModuleId = ElectionsPhragmenModuleId; type Event = Event; type Currency = Balances; - type CurrencyToVote = CurrencyToVoteHandler; + type CurrencyToVote = frame_support::traits::IdentityCurrencyToVote; type ChangeMembers = TestChangeMembers; type InitializeMembers = (); type CandidacyBond = CandidacyBond; diff --git a/frame/grandpa/src/mock.rs b/frame/grandpa/src/mock.rs index 684712df7d078..03b0cbd3966e1 100644 --- a/frame/grandpa/src/mock.rs +++ b/frame/grandpa/src/mock.rs @@ -36,7 +36,7 @@ use sp_runtime::{ curve::PiecewiseLinear, impl_opaque_keys, testing::{Header, TestXt, UintAuthorityId}, - traits::{Convert, IdentityLookup, OpaqueKeys, SaturatedConversion}, + traits::{IdentityLookup, OpaqueKeys}, DigestItem, Perbill, }; use sp_staking::SessionIndex; @@ -204,23 +204,9 @@ parameter_types! { pub const StakingUnsignedPriority: u64 = u64::max_value() / 2; } -pub struct CurrencyToVoteHandler; - -impl Convert for CurrencyToVoteHandler { - fn convert(x: u128) -> u128 { - x - } -} - -impl Convert for CurrencyToVoteHandler { - fn convert(x: u128) -> u64 { - x.saturated_into() - } -} - impl staking::Trait for Test { type RewardRemainder = (); - type CurrencyToVote = CurrencyToVoteHandler; + type CurrencyToVote = frame_support::traits::IdentityCurrencyToVote; type Event = TestEvent; type Currency = Balances; type Slash = (); diff --git a/frame/offences/benchmarking/src/mock.rs b/frame/offences/benchmarking/src/mock.rs index ad6e8a14d5622..dfdadf55eef74 100644 --- a/frame/offences/benchmarking/src/mock.rs +++ b/frame/offences/benchmarking/src/mock.rs @@ -26,7 +26,6 @@ use frame_support::{ }; use frame_system as system; use sp_runtime::{ - SaturatedConversion, traits::{IdentityLookup, Block as BlockT}, testing::{Header, UintAuthorityId}, }; @@ -149,22 +148,10 @@ parameter_types! { pub type Extrinsic = sp_runtime::testing::TestXt; -pub struct CurrencyToVoteHandler; -impl Convert for CurrencyToVoteHandler { - fn convert(x: u64) -> u64 { - x - } -} -impl Convert for CurrencyToVoteHandler { - fn convert(x: u128) -> u64 { - x.saturated_into() - } -} - impl pallet_staking::Trait for Test { type Currency = Balances; type UnixTime = pallet_timestamp::Module; - type CurrencyToVote = CurrencyToVoteHandler; + type CurrencyToVote = frame_support::traits::IdentityCurrencyToVote; type RewardRemainder = (); type Event = Event; type Slash = (); diff --git a/frame/session/benchmarking/src/mock.rs b/frame/session/benchmarking/src/mock.rs index d4eac4247734f..9023dd765554c 100644 --- a/frame/session/benchmarking/src/mock.rs +++ b/frame/session/benchmarking/src/mock.rs @@ -19,7 +19,7 @@ #![cfg(test)] -use sp_runtime::traits::{Convert, SaturatedConversion, IdentityLookup}; +use sp_runtime::traits::IdentityLookup; use frame_support::{impl_outer_origin, impl_outer_dispatch, parameter_types}; type AccountId = u64; @@ -42,18 +42,6 @@ impl_outer_dispatch! { } } -pub struct CurrencyToVoteHandler; -impl Convert for CurrencyToVoteHandler { - fn convert(x: u64) -> u64 { - x - } -} -impl Convert for CurrencyToVoteHandler { - fn convert(x: u128) -> u64 { - x.saturated_into() - } -} - #[derive(Clone, Eq, PartialEq, Debug)] pub struct Test; @@ -171,7 +159,7 @@ impl frame_system::offchain::SendTransactionTypes for Test where impl pallet_staking::Trait for Test { type Currency = Balances; type UnixTime = pallet_timestamp::Module; - type CurrencyToVote = CurrencyToVoteHandler; + type CurrencyToVote = frame_support::traits::IdentityCurrencyToVote; type RewardRemainder = (); type Event = (); type Slash = (); diff --git a/frame/staking/fuzzer/src/mock.rs b/frame/staking/fuzzer/src/mock.rs index 1f5b29b56b6b6..a0f7c98a37f13 100644 --- a/frame/staking/fuzzer/src/mock.rs +++ b/frame/staking/fuzzer/src/mock.rs @@ -17,7 +17,6 @@ //! Mock file for staking fuzzing. -use sp_runtime::traits::{Convert, SaturatedConversion}; use frame_support::{impl_outer_origin, impl_outer_dispatch, parameter_types}; type AccountId = u64; @@ -41,18 +40,6 @@ impl_outer_dispatch! { } } -pub struct CurrencyToVoteHandler; -impl Convert for CurrencyToVoteHandler { - fn convert(x: u64) -> u64 { - x - } -} -impl Convert for CurrencyToVoteHandler { - fn convert(x: u128) -> u64 { - x.saturated_into() - } -} - #[derive(Clone, Eq, PartialEq, Debug)] pub struct Test; @@ -176,7 +163,7 @@ impl frame_system::offchain::SendTransactionTypes for Test where impl pallet_staking::Trait for Test { type Currency = Balances; type UnixTime = pallet_timestamp::Module; - type CurrencyToVote = CurrencyToVoteHandler; + type CurrencyToVote = frame_support::traits::IdentityCurrencyToVote; type RewardRemainder = (); type Event = (); type Slash = (); diff --git a/frame/staking/src/lib.rs b/frame/staking/src/lib.rs index 858bb279a8571..d3fd3772e0c5d 100644 --- a/frame/staking/src/lib.rs +++ b/frame/staking/src/lib.rs @@ -298,7 +298,7 @@ use frame_support::{ }, traits::{ Currency, LockIdentifier, LockableCurrency, WithdrawReasons, OnUnbalanced, Imbalance, Get, - UnixTime, EstimateNextNewSession, EnsureOrigin, + UnixTime, EstimateNextNewSession, EnsureOrigin, CurrencyToVote, } }; use pallet_session::historical; @@ -907,7 +907,7 @@ pub trait Trait: frame_system::Trait + SendTransactionTypes> { /// [`sp_npos_elections`] crate which accepts u64 numbers and does operations in 128. /// Consequently, the backward convert is used convert the u128s from sp-elections back to a /// [`BalanceOf`]. - type CurrencyToVote: Convert, VoteWeight> + Convert>; + type CurrencyToVote: CurrencyToVote>; /// Tokens have been minted and are unused for validator-reward. /// See [Era payout](./index.html#era-payout). @@ -2299,11 +2299,21 @@ impl Module { Self::bonded(stash).and_then(Self::ledger).map(|l| l.active).unwrap_or_default() } - /// internal impl of [`slashable_balance_of`] that returns [`VoteWeight`]. - fn slashable_balance_of_vote_weight(stash: &T::AccountId) -> VoteWeight { - , VoteWeight>>::convert( - Self::slashable_balance_of(stash) - ) + /// Internal impl of [`slashable_balance_of`] that returns [`VoteWeight`]. + fn slashable_balance_of_vote_weight(stash: &T::AccountId, issuance: BalanceOf) -> VoteWeight { + T::CurrencyToVote::to_vote(Self::slashable_balance_of(stash), issuance) + } + + /// Returns a closure around `slashable_balance_of_vote_weight` that can be passed around. + /// + /// This prevents call sites from repeatedly requesting `total_issuance` from backend. + pub(crate) fn weight_of_fn() -> Box VoteWeight> { + // TODO: changing this to unboxed `impl Fn(..)` return type and the module will still + // compile, while some types in mock fail to resolve, all in all, wtf is all I can say. + let issuance = T::Currency::total_issuance(); + Box::new(move |who: &T::AccountId| -> VoteWeight { + Self::slashable_balance_of_vote_weight(who, issuance) + }) } /// Dump the list of validators and nominators into vectors and keep them on-chain. @@ -2702,7 +2712,7 @@ impl Module { // convert into staked assignments. let staked_assignments = sp_npos_elections::assignment_ratio_to_staked( assignments, - Self::slashable_balance_of_vote_weight, + Self::weight_of_fn(), ); // build the support map thereof in order to evaluate. @@ -2972,7 +2982,7 @@ impl Module { let staked_assignments = sp_npos_elections::assignment_ratio_to_staked( assignments, - Self::slashable_balance_of_vote_weight, + Self::weight_of_fn(), ); let (supports, _) = build_support_map::( @@ -3008,11 +3018,12 @@ impl Module { /// /// No storage item is updated. fn do_phragmen() -> Option> { + let weight_of = Self::weight_of_fn(); let mut all_nominators: Vec<(T::AccountId, VoteWeight, Vec)> = Vec::new(); let mut all_validators = Vec::new(); for (validator, _) in >::iter() { // append self vote - let self_vote = (validator.clone(), Self::slashable_balance_of_vote_weight(&validator), vec![validator.clone()]); + let self_vote = (validator.clone(), weight_of(&validator), vec![validator.clone()]); all_nominators.push(self_vote); all_validators.push(validator); } @@ -3032,7 +3043,7 @@ impl Module { (nominator, targets) }); all_nominators.extend(nominator_votes.map(|(n, ns)| { - let s = Self::slashable_balance_of_vote_weight(&n); + let s = weight_of(&n); (n, s, ns) })); @@ -3046,8 +3057,9 @@ impl Module { /// Consume a set of [`Supports`] from [`sp_npos_elections`] and collect them into a [`Exposure`] fn collect_exposure(supports: SupportMap) -> Vec<(T::AccountId, Exposure>)> { - let to_balance = |e: ExtendedBalance| - >>::convert(e); + let total_issuance = T::Currency::total_issuance(); + let to_currency = |e: ExtendedBalance| + ::to_currency(e, total_issuance); supports.into_iter().map(|(validator, support)| { // build `struct exposure` from `support` @@ -3056,7 +3068,7 @@ impl Module { let mut total: BalanceOf = Zero::zero(); support.voters .into_iter() - .map(|(nominator, weight)| (nominator, to_balance(weight))) + .map(|(nominator, weight)| (nominator, to_currency(weight))) .for_each(|(nominator, stake)| { if nominator == validator { own = own.saturating_add(stake); diff --git a/frame/staking/src/mock.rs b/frame/staking/src/mock.rs index 84201827704e1..f10deff66edf9 100644 --- a/frame/staking/src/mock.rs +++ b/frame/staking/src/mock.rs @@ -20,7 +20,7 @@ use std::{collections::HashSet, cell::RefCell}; use sp_runtime::Perbill; use sp_runtime::curve::PiecewiseLinear; -use sp_runtime::traits::{IdentityLookup, Convert, SaturatedConversion, Zero}; +use sp_runtime::traits::{IdentityLookup, Zero}; use sp_runtime::testing::{Header, UintAuthorityId, TestXt}; use sp_staking::{SessionIndex, offence::{OffenceDetails, OnOffenceHandler}}; use sp_core::H256; @@ -33,7 +33,6 @@ use frame_support::{ use sp_io; use sp_npos_elections::{ build_support_map, evaluate_support, reduce, ExtendedBalance, StakedAssignment, ElectionScore, - VoteWeight, }; use crate::*; @@ -45,19 +44,6 @@ pub(crate) type AccountIndex = u64; pub(crate) type BlockNumber = u64; pub(crate) type Balance = u128; -/// Simple structure that exposes how u64 currency can be represented as... u64. -pub struct CurrencyToVoteHandler; -impl Convert for CurrencyToVoteHandler { - fn convert(x: Balance) -> u64 { - x.saturated_into() - } -} -impl Convert for CurrencyToVoteHandler { - fn convert(x: u128) -> Balance { - x - } -} - thread_local! { static SESSION: RefCell<(Vec, HashSet)> = RefCell::new(Default::default()); static SESSION_PER_ERA: RefCell = RefCell::new(3); @@ -312,7 +298,7 @@ impl OnUnbalanced> for RewardRemainderMock { impl Trait for Test { type Currency = Balances; type UnixTime = Timestamp; - type CurrencyToVote = CurrencyToVoteHandler; + type CurrencyToVote = frame_support::traits::IdentityCurrencyToVote; type RewardRemainder = RewardRemainderMock; type Event = MetaEvent; type Slash = (); @@ -911,13 +897,11 @@ pub(crate) fn prepare_submission_with( } = Staking::do_phragmen::().unwrap(); let winners = sp_npos_elections::to_without_backing(winners); - let stake_of = |who: &AccountId| -> VoteWeight { - >::convert( - Staking::slashable_balance_of(&who) - ) - }; + let mut staked = sp_npos_elections::assignment_ratio_to_staked( + assignments, + Staking::weight_of_fn(), + ); - let mut staked = sp_npos_elections::assignment_ratio_to_staked(assignments, stake_of); let (mut support_map, _) = build_support_map::(&winners, &staked); if iterations > 0 { @@ -964,7 +948,7 @@ pub(crate) fn prepare_submission_with( let score = { let staked = sp_npos_elections::assignment_ratio_to_staked( assignments_reduced.clone(), - Staking::slashable_balance_of_vote_weight, + Staking::weight_of_fn(), ); let (support_map, _) = build_support_map::( diff --git a/frame/staking/src/offchain_election.rs b/frame/staking/src/offchain_election.rs index 79f3a5c2d94fe..aa8f0740f2a1c 100644 --- a/frame/staking/src/offchain_election.rs +++ b/frame/staking/src/offchain_election.rs @@ -174,7 +174,7 @@ pub fn prepare_submission( // convert into absolute value and to obtain the reduced version. let mut staked = sp_npos_elections::assignment_ratio_to_staked( assignments, - >::slashable_balance_of_vote_weight, + >::weight_of_fn(), ); let (mut support_map, _) = build_support_map::(&winners, &staked); @@ -217,7 +217,7 @@ pub fn prepare_submission( let score = { let staked = sp_npos_elections::assignment_ratio_to_staked( low_accuracy_assignment.clone(), - >::slashable_balance_of_vote_weight, + >::weight_of_fn(), ); let (support_map, _) = build_support_map::(&winners, &staked); diff --git a/frame/staking/src/testing_utils.rs b/frame/staking/src/testing_utils.rs index 02acd135e63e4..3f826e0e399e9 100644 --- a/frame/staking/src/testing_utils.rs +++ b/frame/staking/src/testing_utils.rs @@ -182,9 +182,10 @@ pub fn get_weak_solution( who: w.clone(), distribution: vec![( w.clone(), - , u64>>::convert( - >::slashable_balance_of(&w), - ) as ExtendedBalance, + >::slashable_balance_of_vote_weight( + &w, + T::Currency::total_issuance(), + ).into(), )], }) }); @@ -209,11 +210,6 @@ pub fn get_weak_solution( .position(|x| x == a) .and_then(|i| >::try_into(i).ok()) }; - let stake_of = |who: &T::AccountId| -> VoteWeight { - , u64>>::convert( - >::slashable_balance_of(who), - ) - }; // convert back to ratio assignment. This takes less space. let low_accuracy_assignment = assignment_staked_to_ratio_normalized(staked_assignments) @@ -223,7 +219,7 @@ pub fn get_weak_solution( let score = { let staked = assignment_ratio_to_staked::<_, OffchainAccuracy, _>( low_accuracy_assignment.clone(), - stake_of + >::weight_of_fn(), ); let (support_map, _) = @@ -284,8 +280,7 @@ pub fn get_single_winner_solution( let nom_index = snapshot_nominators.iter().position(|x| *x == winner).ok_or("not a nominator")?; let stake = >::slashable_balance_of(&winner); - let stake = , VoteWeight>>::convert(stake) - as ExtendedBalance; + let stake = ::to_vote(stake, T::Currency::total_issuance()) as ExtendedBalance; let val_index = val_index as ValidatorIndex; let nom_index = nom_index as NominatorIndex; diff --git a/frame/support/src/traits.rs b/frame/support/src/traits.rs index 72a3850d2d37e..2f28f5bc772fa 100644 --- a/frame/support/src/traits.rs +++ b/frame/support/src/traits.rs @@ -23,9 +23,10 @@ use sp_std::{prelude::*, result, marker::PhantomData, ops::Div, fmt::Debug}; use codec::{FullCodec, Codec, Encode, Decode, EncodeLike}; use sp_core::u32_trait::Value as U32; use sp_runtime::{ - RuntimeDebug, ConsensusEngineId, DispatchResult, DispatchError, traits::{ + RuntimeDebug, ConsensusEngineId, DispatchResult, DispatchError, + traits::{ MaybeSerializeDeserialize, AtLeast32Bit, Saturating, TrailingZeroInput, Bounded, Zero, - BadOrigin, AtLeast32BitUnsigned + BadOrigin, AtLeast32BitUnsigned, UniqueSaturatedFrom, UniqueSaturatedInto, }, }; use crate::dispatch::Parameter; @@ -1664,6 +1665,70 @@ pub trait Instance: 'static { const PREFIX: &'static str ; } +/// A trait similar to `Convert` to convert values from `B` (i.e. the `Balance` type of the chain) +/// into u64, which is the standard numeric types used in election and other places where complex +/// calculation over balance type is needed. +/// +/// Total issuance of the chain is passed in, but an implementation may or may not use it. +pub trait CurrencyToVote { + /// Convert balance to u64. + fn to_vote(value: B, issuance: B) -> u64; + + /// Convert u128 to balance. + fn to_currency(value: u128, issuance: B) -> B; +} + +/// An implementation of `CurrencyToVote` tailored for chain's that have a balance type of u128. +/// +/// The factor is the `(total_issuance / u64::max()).max(1)`, represented as u64. Let's look at the +/// important cases: +/// +/// If the chain's total issuance is less than u64::max(), this will always be 1, which means that +/// the factor will not have any effect. In this case, any account's balance is also less. Thus, +/// both of the conversions are basically an `as`; Any balance can fit in u64. +/// +/// If the chain's total issuance is more than 2*u64::max(), then a factor might be multiplied and +/// divided upon conversion. +/// +/// # Warning +/// +/// This will have undefined behavior if the chain's balance type is u32. +pub struct U128CurrencyToVote; + +impl U128CurrencyToVote { + fn factor(issuance: u128) -> u128 { + (issuance / u64::max_value() as u128).max(1) + } +} + +impl CurrencyToVote for U128CurrencyToVote { + fn to_vote(value: u128, issuance: u128) -> u64 { + (value / Self::factor(issuance)) as u64 + } + + fn to_currency(value: u128, issuance: u128) -> u128 { + value.saturating_mul(Self::factor(issuance)) + } +} + + +/// A naive implementation of `CurrencyConvert` that simply saturates all conversions. +/// +/// # Warning +/// +/// This is designed to be used mostly for testing. Use with care, and think about the consequences. +pub struct IdentityCurrencyToVote; + +impl + UniqueSaturatedFrom> CurrencyToVote for IdentityCurrencyToVote { + fn to_vote(value: B, _: B) -> u64 { + value.unique_saturated_into() + } + + fn to_currency(value: u128, _: B) -> B { + B::unique_saturated_from(value) + } +} + #[cfg(test)] mod tests { use super::*; diff --git a/primitives/npos-elections/src/helpers.rs b/primitives/npos-elections/src/helpers.rs index 063eac70c57fd..d3777669b8bb0 100644 --- a/primitives/npos-elections/src/helpers.rs +++ b/primitives/npos-elections/src/helpers.rs @@ -25,7 +25,7 @@ use sp_std::prelude::*; /// /// Note that this will NOT attempt at normalizing the result. pub fn assignment_ratio_to_staked( - ratio: Vec>, + ratios: Vec>, stake_of: FS, ) -> Vec> where @@ -33,7 +33,7 @@ where P: sp_std::ops::Mul, ExtendedBalance: From>, { - ratio + ratios .into_iter() .map(|a| { let stake = stake_of(&a.who); From a68c4a16379d997d3c7d416f4a45ba04c2abe0c7 Mon Sep 17 00:00:00 2001 From: Kian Paimani <5588131+kianenigma@users.noreply.github.com> Date: Mon, 17 Aug 2020 16:20:00 +0200 Subject: [PATCH 2/6] Update frame/support/src/traits.rs Co-authored-by: Guillaume Thiolliere --- frame/support/src/traits.rs | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/frame/support/src/traits.rs b/frame/support/src/traits.rs index 2f28f5bc772fa..e70d48ae52a10 100644 --- a/frame/support/src/traits.rs +++ b/frame/support/src/traits.rs @@ -1665,11 +1665,11 @@ pub trait Instance: 'static { const PREFIX: &'static str ; } -/// A trait similar to `Convert` to convert values from `B` (i.e. the `Balance` type of the chain) -/// into u64, which is the standard numeric types used in election and other places where complex -/// calculation over balance type is needed. +/// A trait similar to `Convert` to convert values from `B` an abstract balance type +/// into u64 and back from u128. (This conversion is used in election and other places where complex +/// calculation over balance type is needed) /// -/// Total issuance of the chain is passed in, but an implementation may or may not use it. +/// Total issuance of the currency is passed in, but an implementation of this trait may or may not use it. pub trait CurrencyToVote { /// Convert balance to u64. fn to_vote(value: B, issuance: B) -> u64; From 78993cb9ce02838c04390e5187a92f60ed231194 Mon Sep 17 00:00:00 2001 From: kianenigma Date: Mon, 17 Aug 2020 16:22:17 +0200 Subject: [PATCH 3/6] Some grumbles --- bin/node/runtime/src/lib.rs | 7 +++++-- frame/babe/src/mock.rs | 2 +- frame/elections-phragmen/src/lib.rs | 2 +- frame/grandpa/src/mock.rs | 2 +- frame/offences/benchmarking/src/mock.rs | 2 +- frame/session/benchmarking/src/mock.rs | 2 +- frame/staking/fuzzer/src/mock.rs | 2 +- frame/staking/src/mock.rs | 2 +- frame/support/src/traits.rs | 8 ++------ 9 files changed, 14 insertions(+), 15 deletions(-) diff --git a/bin/node/runtime/src/lib.rs b/bin/node/runtime/src/lib.rs index 9128868900693..7aa69ce8aa3b0 100644 --- a/bin/node/runtime/src/lib.rs +++ b/bin/node/runtime/src/lib.rs @@ -30,7 +30,10 @@ use frame_support::{ Weight, IdentityFee, constants::{BlockExecutionWeight, ExtrinsicBaseWeight, RocksDbWeight, WEIGHT_PER_SECOND}, }, - traits::{Currency, Imbalance, KeyOwnerProofSystem, OnUnbalanced, Randomness, LockIdentifier}, + traits::{ + Currency, Imbalance, KeyOwnerProofSystem, OnUnbalanced, Randomness, LockIdentifier, + U128CurrencyToVote, + }, }; use frame_system::{EnsureRoot, EnsureOneOf}; use frame_support::traits::InstanceFilter; @@ -533,7 +536,7 @@ impl pallet_elections_phragmen::Trait for Runtime { // NOTE: this implies that council's genesis members cannot be set directly and must come from // this module. type InitializeMembers = Council; - type CurrencyToVote = frame_support::traits::U128CurrencyToVote; + type CurrencyToVote = U128CurrencyToVote; type CandidacyBond = CandidacyBond; type VotingBond = VotingBond; type LoserCandidate = (); diff --git a/frame/babe/src/mock.rs b/frame/babe/src/mock.rs index 99b58eb9ba096..3bd0dda3b6aa0 100644 --- a/frame/babe/src/mock.rs +++ b/frame/babe/src/mock.rs @@ -184,7 +184,7 @@ parameter_types! { impl pallet_staking::Trait for Test { type RewardRemainder = (); - type CurrencyToVote = frame_support::traits::IdentityCurrencyToVote; + type CurrencyToVote = frame_support::traits::SaturatingCurrencyToVote; type Event = (); type Currency = Balances; type Slash = (); diff --git a/frame/elections-phragmen/src/lib.rs b/frame/elections-phragmen/src/lib.rs index 797d7a5fefb30..434632248a6bd 100644 --- a/frame/elections-phragmen/src/lib.rs +++ b/frame/elections-phragmen/src/lib.rs @@ -1226,7 +1226,7 @@ mod tests { type ModuleId = ElectionsPhragmenModuleId; type Event = Event; type Currency = Balances; - type CurrencyToVote = frame_support::traits::IdentityCurrencyToVote; + type CurrencyToVote = frame_support::traits::SaturatingCurrencyToVote; type ChangeMembers = TestChangeMembers; type InitializeMembers = (); type CandidacyBond = CandidacyBond; diff --git a/frame/grandpa/src/mock.rs b/frame/grandpa/src/mock.rs index 03b0cbd3966e1..068b32fd2f8eb 100644 --- a/frame/grandpa/src/mock.rs +++ b/frame/grandpa/src/mock.rs @@ -206,7 +206,7 @@ parameter_types! { impl staking::Trait for Test { type RewardRemainder = (); - type CurrencyToVote = frame_support::traits::IdentityCurrencyToVote; + type CurrencyToVote = frame_support::traits::SaturatingCurrencyToVote; type Event = TestEvent; type Currency = Balances; type Slash = (); diff --git a/frame/offences/benchmarking/src/mock.rs b/frame/offences/benchmarking/src/mock.rs index dfdadf55eef74..301d9a1caa226 100644 --- a/frame/offences/benchmarking/src/mock.rs +++ b/frame/offences/benchmarking/src/mock.rs @@ -151,7 +151,7 @@ pub type Extrinsic = sp_runtime::testing::TestXt; impl pallet_staking::Trait for Test { type Currency = Balances; type UnixTime = pallet_timestamp::Module; - type CurrencyToVote = frame_support::traits::IdentityCurrencyToVote; + type CurrencyToVote = frame_support::traits::SaturatingCurrencyToVote; type RewardRemainder = (); type Event = Event; type Slash = (); diff --git a/frame/session/benchmarking/src/mock.rs b/frame/session/benchmarking/src/mock.rs index 9023dd765554c..902f9a206d11e 100644 --- a/frame/session/benchmarking/src/mock.rs +++ b/frame/session/benchmarking/src/mock.rs @@ -159,7 +159,7 @@ impl frame_system::offchain::SendTransactionTypes for Test where impl pallet_staking::Trait for Test { type Currency = Balances; type UnixTime = pallet_timestamp::Module; - type CurrencyToVote = frame_support::traits::IdentityCurrencyToVote; + type CurrencyToVote = frame_support::traits::SaturatingCurrencyToVote; type RewardRemainder = (); type Event = (); type Slash = (); diff --git a/frame/staking/fuzzer/src/mock.rs b/frame/staking/fuzzer/src/mock.rs index a0f7c98a37f13..6246fca5ecbbc 100644 --- a/frame/staking/fuzzer/src/mock.rs +++ b/frame/staking/fuzzer/src/mock.rs @@ -163,7 +163,7 @@ impl frame_system::offchain::SendTransactionTypes for Test where impl pallet_staking::Trait for Test { type Currency = Balances; type UnixTime = pallet_timestamp::Module; - type CurrencyToVote = frame_support::traits::IdentityCurrencyToVote; + type CurrencyToVote = frame_support::traits::SaturatingCurrencyToVote; type RewardRemainder = (); type Event = (); type Slash = (); diff --git a/frame/staking/src/mock.rs b/frame/staking/src/mock.rs index 4bbf027757e22..6f6731ac198aa 100644 --- a/frame/staking/src/mock.rs +++ b/frame/staking/src/mock.rs @@ -298,7 +298,7 @@ impl OnUnbalanced> for RewardRemainderMock { impl Trait for Test { type Currency = Balances; type UnixTime = Timestamp; - type CurrencyToVote = frame_support::traits::IdentityCurrencyToVote; + type CurrencyToVote = frame_support::traits::SaturatingCurrencyToVote; type RewardRemainder = RewardRemainderMock; type Event = MetaEvent; type Slash = (); diff --git a/frame/support/src/traits.rs b/frame/support/src/traits.rs index 2f28f5bc772fa..22eecd39773a4 100644 --- a/frame/support/src/traits.rs +++ b/frame/support/src/traits.rs @@ -1689,10 +1689,6 @@ pub trait CurrencyToVote { /// /// If the chain's total issuance is more than 2*u64::max(), then a factor might be multiplied and /// divided upon conversion. -/// -/// # Warning -/// -/// This will have undefined behavior if the chain's balance type is u32. pub struct U128CurrencyToVote; impl U128CurrencyToVote { @@ -1717,9 +1713,9 @@ impl CurrencyToVote for U128CurrencyToVote { /// # Warning /// /// This is designed to be used mostly for testing. Use with care, and think about the consequences. -pub struct IdentityCurrencyToVote; +pub struct SaturatingCurrencyToVote; -impl + UniqueSaturatedFrom> CurrencyToVote for IdentityCurrencyToVote { +impl + UniqueSaturatedFrom> CurrencyToVote for SaturatingCurrencyToVote { fn to_vote(value: B, _: B) -> u64 { value.unique_saturated_into() } From f501eeda9c334339f5a5f9944e3eeb3ab7079abb Mon Sep 17 00:00:00 2001 From: kianenigma Date: Tue, 18 Aug 2020 14:43:25 +0200 Subject: [PATCH 4/6] Fix last grumbles. --- bin/node/runtime/src/impls.rs | 5 +---- frame/support/src/traits.rs | 3 ++- 2 files changed, 3 insertions(+), 5 deletions(-) diff --git a/bin/node/runtime/src/impls.rs b/bin/node/runtime/src/impls.rs index cd692de250447..16666997b3a55 100644 --- a/bin/node/runtime/src/impls.rs +++ b/bin/node/runtime/src/impls.rs @@ -17,8 +17,6 @@ //! Some configurable implementations as associated type for the substrate runtime. -use node_primitives::Balance; -use sp_runtime::traits::Convert; use frame_support::traits::{OnUnbalanced, Currency}; use crate::{Balances, Authorship, NegativeImbalance}; @@ -31,8 +29,7 @@ impl OnUnbalanced for Author { #[cfg(test)] mod multiplier_tests { - use super::*; - use sp_runtime::{assert_eq_error_rate, FixedPointNumber}; + use sp_runtime::{assert_eq_error_rate, FixedPointNumber, traits::Convert}; use pallet_transaction_payment::{Multiplier, TargetedFeeAdjustment}; use crate::{ diff --git a/frame/support/src/traits.rs b/frame/support/src/traits.rs index 66e7524a9e928..709d58077cb10 100644 --- a/frame/support/src/traits.rs +++ b/frame/support/src/traits.rs @@ -27,6 +27,7 @@ use sp_runtime::{ traits::{ MaybeSerializeDeserialize, AtLeast32Bit, Saturating, TrailingZeroInput, Bounded, Zero, BadOrigin, AtLeast32BitUnsigned, UniqueSaturatedFrom, UniqueSaturatedInto, + SaturatedConversion, }, }; use crate::dispatch::Parameter; @@ -1699,7 +1700,7 @@ impl U128CurrencyToVote { impl CurrencyToVote for U128CurrencyToVote { fn to_vote(value: u128, issuance: u128) -> u64 { - (value / Self::factor(issuance)) as u64 + (value / Self::factor(issuance)).saturated_into() } fn to_currency(value: u128, issuance: u128) -> u128 { From 63b154ff11b92d542e15809e5aa114cb0f9a4088 Mon Sep 17 00:00:00 2001 From: kianenigma Date: Thu, 20 Aug 2020 15:06:16 +0200 Subject: [PATCH 5/6] Fix comment --- frame/support/src/traits.rs | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/frame/support/src/traits.rs b/frame/support/src/traits.rs index 709d58077cb10..093124a7d42fd 100644 --- a/frame/support/src/traits.rs +++ b/frame/support/src/traits.rs @@ -1670,7 +1670,14 @@ pub trait Instance: 'static { /// into u64 and back from u128. (This conversion is used in election and other places where complex /// calculation over balance type is needed) /// -/// Total issuance of the currency is passed in, but an implementation of this trait may or may not use it. +/// Total issuance of the currency is passed in, but an implementation of this trait may or may not +/// use it. +/// +/// # WARNING +/// +/// the total issuance being passed in implies that the implementation must be aware of the fact +/// that its values can affect the outcome. This implies that if the vote value is dependent on the +/// total issuance, it should never ber written to storage for later re-use. pub trait CurrencyToVote { /// Convert balance to u64. fn to_vote(value: B, issuance: B) -> u64; From e0f5dcb7b0765c40b15479eee96a68425c4123c7 Mon Sep 17 00:00:00 2001 From: kianenigma Date: Thu, 8 Oct 2020 09:40:01 +0200 Subject: [PATCH 6/6] Final fix --- bin/node/runtime/src/lib.rs | 2 +- frame/elections-phragmen/src/lib.rs | 41 +++++++++++++---------------- 2 files changed, 20 insertions(+), 23 deletions(-) diff --git a/bin/node/runtime/src/lib.rs b/bin/node/runtime/src/lib.rs index d2d5cbcd3137b..75439fe948bdf 100644 --- a/bin/node/runtime/src/lib.rs +++ b/bin/node/runtime/src/lib.rs @@ -451,7 +451,7 @@ parameter_types! { impl pallet_staking::Trait for Runtime { type Currency = Balances; type UnixTime = Timestamp; - type CurrencyToVote = frame_support::traits::U128CurrencyToVote; + type CurrencyToVote = U128CurrencyToVote; type RewardRemainder = Treasury; type Event = Event; type Slash = Treasury; // send the slashed funds to the treasury. diff --git a/frame/elections-phragmen/src/lib.rs b/frame/elections-phragmen/src/lib.rs index 468692b57d545..964cf6daf2cee 100644 --- a/frame/elections-phragmen/src/lib.rs +++ b/frame/elections-phragmen/src/lib.rs @@ -83,25 +83,26 @@ #![cfg_attr(not(feature = "std"), no_std)] -use codec::{Encode, Decode}; -use sp_std::prelude::*; -use sp_runtime::{ - DispatchError, RuntimeDebug, Perbill, - traits::{Zero, StaticLookup, Saturating}, -}; +use codec::{Decode, Encode}; use frame_support::{ - decl_storage, decl_event, ensure, decl_module, decl_error, - weights::Weight, - storage::{StorageMap, IterableStorageMap}, + decl_error, decl_event, decl_module, decl_storage, dispatch::{DispatchResultWithPostInfo, WithPostDispatchInfo}, + ensure, + storage::{IterableStorageMap, StorageMap}, traits::{ - Currency, Get, LockableCurrency, LockIdentifier, ReservableCurrency, WithdrawReasons, - ChangeMembers, OnUnbalanced, WithdrawReason, Contains, InitializeMembers, BalanceStatus, - ContainsLengthBound, CurrencyToVote, - } + BalanceStatus, ChangeMembers, Contains, ContainsLengthBound, Currency, CurrencyToVote, Get, + InitializeMembers, LockIdentifier, LockableCurrency, OnUnbalanced, ReservableCurrency, + WithdrawReason, WithdrawReasons, + }, + weights::Weight, +}; +use frame_system::{ensure_root, ensure_signed}; +use sp_npos_elections::{ElectionResult, ExtendedBalance}; +use sp_runtime::{ + traits::{Saturating, StaticLookup, Zero}, + DispatchError, Perbill, RuntimeDebug, }; -use sp_npos_elections::{ExtendedBalance, VoteWeight, ElectionResult}; -use frame_system::{ensure_signed, ensure_root}; +use sp_std::prelude::*; mod benchmarking; mod default_weights; @@ -872,16 +873,12 @@ impl Module { // helper closures to deal with balance/stake. let total_issuance = T::Currency::total_issuance(); - let to_votes = |b: BalanceOf| -> VoteWeight { - T::CurrencyToVote::to_vote(b, total_issuance) - }; - let to_balance = |e: ExtendedBalance| -> BalanceOf { - T::CurrencyToVote::to_currency(e, total_issuance) - }; + let to_votes = |b: BalanceOf| T::CurrencyToVote::to_vote(b, total_issuance); + let to_balance = |e: ExtendedBalance| T::CurrencyToVote::to_currency(e, total_issuance); // used for prime election. let voters_and_stakes = Voting::::iter() - .map(|(voter, (stake, votes))| { (voter, stake, votes) }) + .map(|(voter, (stake, votes))| (voter, stake, votes)) .collect::>(); // used for phragmen. let voters_and_votes = voters_and_stakes.iter()