Refactor CurrencyToVote (#6896)

* Refactor CurrencyToVote to avoid calls to total_issuance.

* Update frame/support/src/traits.rs

Co-authored-by: Guillaume Thiolliere <[email protected]>

* Some grumbles

* Fix last grumbles.

* Fix comment

* Final fix

Co-authored-by: Guillaume Thiolliere <[email protected]>

Author: kianenigma

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<u64, u64> for CurrencyToVoteHandler {
-	fn convert(x: u64) -> u64 {
-		x
-	}
-}
-impl Convert<u128, u64> for CurrencyToVoteHandler {
-	fn convert(x: u128) -> u64 {
-		x.saturated_into()
-	}
-}
-
 #[derive(Clone, Eq, PartialEq, Debug)]
 pub struct Test;
 
@@ -177,7 +164,7 @@ impl<C> frame_system::offchain::SendTransactionTypes<C> for Test where
 impl pallet_staking::Trait for Test {
 	type Currency = Balances;
 	type UnixTime = pallet_timestamp::Module<Self>;
-	type CurrencyToVote = CurrencyToVoteHandler;
+	type CurrencyToVote = frame_support::traits::SaturatingCurrencyToVote;
 	type RewardRemainder = ();
 	type Event = ();
 	type Slash = ();

src/lib.rs

@@ -299,7 +299,7 @@ use frame_support::{
 	},
 	traits::{
 		Currency, LockIdentifier, LockableCurrency, WithdrawReasons, OnUnbalanced, Imbalance, Get,
-		UnixTime, EstimateNextNewSession, EnsureOrigin,
+		UnixTime, EstimateNextNewSession, EnsureOrigin, CurrencyToVote,
 	}
 };
 use pallet_session::historical;
@@ -811,7 +811,7 @@ pub trait Trait: frame_system::Trait + SendTransactionTypes<Call<Self>> {
 	/// [`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<BalanceOf<Self>, VoteWeight> + Convert<u128, BalanceOf<Self>>;
+	type CurrencyToVote: CurrencyToVote<BalanceOf<Self>>;
 
 	/// Tokens have been minted and are unused for validator-reward.
 	/// See [Era payout](./index.html#era-payout).
@@ -2192,11 +2192,22 @@ impl<T: Trait> Module<T> {
 		Self::bonded(stash).and_then(Self::ledger).map(|l| l.active).unwrap_or_default()
 	}
 
-	/// internal impl of [`slashable_balance_of`] that returns [`VoteWeight`].
-	pub fn slashable_balance_of_vote_weight(stash: &T::AccountId) -> VoteWeight {
-		<T::CurrencyToVote as Convert<BalanceOf<T>, VoteWeight>>::convert(
-			Self::slashable_balance_of(stash)
-		)
+	/// Internal impl of [`slashable_balance_of`] that returns [`VoteWeight`].
+	pub fn slashable_balance_of_vote_weight(stash: &T::AccountId, issuance: BalanceOf<T>) -> 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. But it is
+	/// important to be only used while the total issuance is not changing.
+	pub fn slashable_balance_of_fn() -> Box<dyn Fn(&T::AccountId) -> VoteWeight> {
+		// NOTE: changing this to unboxed `impl Fn(..)` return type and the module will still
+		// compile, while some types in mock fail to resolve.
+		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.
@@ -2601,7 +2612,7 @@ impl<T: Trait> Module<T> {
 		// convert into staked assignments.
 		let staked_assignments = sp_npos_elections::assignment_ratio_to_staked(
 			assignments,
-			Self::slashable_balance_of_vote_weight,
+			Self::slashable_balance_of_fn(),
 		);
 
 		// build the support map thereof in order to evaluate.
@@ -2852,7 +2863,7 @@ impl<T: Trait> Module<T> {
 	/// `PrimitiveElectionResult` into `ElectionResult`.
 	///
 	/// No storage item is updated.
-	fn do_on_chain_phragmen() -> Option<ElectionResult<T::AccountId, BalanceOf<T>>> {
+	pub fn do_on_chain_phragmen() -> Option<ElectionResult<T::AccountId, BalanceOf<T>>> {
 		if let Some(phragmen_result) = Self::do_phragmen::<ChainAccuracy>(0) {
 			let elected_stashes = phragmen_result.winners.iter()
 				.map(|(s, _)| s.clone())
@@ -2861,7 +2872,7 @@ impl<T: Trait> Module<T> {
 
 			let staked_assignments = sp_npos_elections::assignment_ratio_to_staked(
 				assignments,
-				Self::slashable_balance_of_vote_weight,
+				Self::slashable_balance_of_fn(),
 			);
 
 			let supports = build_support_map::<T::AccountId>(
@@ -2903,16 +2914,16 @@ impl<T: Trait> Module<T> {
 	/// Self votes are added and nominations before the most recent slashing span are ignored.
 	///
 	/// No storage item is updated.
-	pub fn do_phragmen<Accuracy: PerThing>(
-		iterations: usize,
-	) -> Option<PrimitiveElectionResult<T::AccountId, Accuracy>>
+	pub fn do_phragmen<Accuracy: PerThing>(iterations: usize)
+	-> Option<PrimitiveElectionResult<T::AccountId, Accuracy>>
 		where ExtendedBalance: From<InnerOf<Accuracy>>
 	{
+		let weight_of = Self::slashable_balance_of_fn();
 		let mut all_nominators: Vec<(T::AccountId, VoteWeight, Vec<T::AccountId>)> = Vec::new();
 		let mut all_validators = Vec::new();
 		for (validator, _) in <Validators<T>>::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);
 		}
@@ -2932,7 +2943,7 @@ impl<T: Trait> Module<T> {
 			(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)
 		}));
 
@@ -2956,8 +2967,8 @@ impl<T: Trait> Module<T> {
 	fn collect_exposure(
 		supports: SupportMap<T::AccountId>,
 	) -> Vec<(T::AccountId, Exposure<T::AccountId, BalanceOf<T>>)> {
-		let to_balance = |e: ExtendedBalance|
-			<T::CurrencyToVote as Convert<ExtendedBalance, BalanceOf<T>>>::convert(e);
+		let total_issuance = T::Currency::total_issuance();
+		let to_currency = |e: ExtendedBalance| T::CurrencyToVote::to_currency(e, total_issuance);
 
 		supports.into_iter().map(|(validator, support)| {
 			// build `struct exposure` from `support`
@@ -2966,7 +2977,7 @@ impl<T: Trait> Module<T> {
 			let mut total: BalanceOf<T> = 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);

src/mock.rs

@@ -27,18 +27,14 @@ use frame_support::{
 use sp_core::H256;
 use sp_io;
 use sp_npos_elections::{
-	build_support_map, evaluate_support, reduce, ElectionScore, ExtendedBalance, StakedAssignment,
+	build_support_map, evaluate_support, reduce, ExtendedBalance, StakedAssignment, ElectionScore,
 };
 use sp_runtime::{
 	curve::PiecewiseLinear,
 	testing::{Header, TestXt, UintAuthorityId},
-	traits::{Convert, IdentityLookup, SaturatedConversion, Zero},
-	Perbill,
-};
-use sp_staking::{
-	offence::{OffenceDetails, OnOffenceHandler},
-	SessionIndex,
+	traits::{IdentityLookup, Zero},
 };
+use sp_staking::offence::{OffenceDetails, OnOffenceHandler};
 use std::{cell::RefCell, collections::HashSet};
 
 pub const INIT_TIMESTAMP: u64 = 30_000;
@@ -49,19 +45,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<Balance, u64> for CurrencyToVoteHandler {
-	fn convert(x: Balance) -> u64 {
-		x.saturated_into()
-	}
-}
-impl Convert<u128, Balance> for CurrencyToVoteHandler {
-	fn convert(x: u128) -> Balance {
-		x
-	}
-}
-
 thread_local! {
 	static SESSION: RefCell<(Vec<AccountId>, HashSet<AccountId>)> = RefCell::new(Default::default());
 	static SESSION_PER_ERA: RefCell<SessionIndex> = RefCell::new(3);
@@ -319,7 +302,7 @@ impl OnUnbalanced<NegativeImbalanceOf<Test>> for RewardRemainderMock {
 impl Trait for Test {
 	type Currency = Balances;
 	type UnixTime = Timestamp;
-	type CurrencyToVote = CurrencyToVoteHandler;
+	type CurrencyToVote = frame_support::traits::SaturatingCurrencyToVote;
 	type RewardRemainder = RewardRemainderMock;
 	type Event = MetaEvent;
 	type Slash = ();
@@ -926,7 +909,7 @@ pub(crate) fn prepare_submission_with(
 
 	let mut staked = sp_npos_elections::assignment_ratio_to_staked(
 		assignments,
-		Staking::slashable_balance_of_vote_weight,
+		Staking::slashable_balance_of_fn(),
 	);
 
 	// apply custom tweaks. awesome for testing.
@@ -964,7 +947,7 @@ pub(crate) fn prepare_submission_with(
 	let score = if compute_real_score {
 		let staked = sp_npos_elections::assignment_ratio_to_staked(
 			assignments_reduced.clone(),
-			Staking::slashable_balance_of_vote_weight,
+			Staking::slashable_balance_of_fn(),
 		);
 
 		let support_map = build_support_map::<AccountId>(
@@ -978,10 +961,8 @@ pub(crate) fn prepare_submission_with(
 
 	let compact =
 		CompactAssignments::from_assignment(assignments_reduced, nominator_index, validator_index)
-			.map_err(|e| { println!("error in compact: {:?}", e); e })
 			.expect("Failed to create compact");
 
-
 	// winner ids to index
 	let winners = winners.into_iter().map(|w| validator_index(&w).unwrap()).collect::<Vec<_>>();
 

src/offchain_election.rs

@@ -268,13 +268,9 @@ where
 	match compact.len().checked_sub(maximum_allowed_voters as usize) {
 		Some(to_remove) if to_remove > 0 => {
 			// grab all voters and sort them by least stake.
+			let balance_of = <Module<T>>::slashable_balance_of_fn();
 			let mut voters_sorted = <Nominators<T>>::iter()
-				.map(|(who, _)| {
-					(
-						who.clone(),
-						<Module<T>>::slashable_balance_of_vote_weight(&who),
-					)
-				})
+				.map(|(who, _)| (who.clone(), balance_of(&who)))
 				.collect::<Vec<_>>();
 			voters_sorted.sort_by_key(|(_, y)| *y);
 
@@ -378,7 +374,7 @@ where
 	// convert into absolute value and to obtain the reduced version.
 	let mut staked = sp_npos_elections::assignment_ratio_to_staked(
 		assignments,
-		<Module<T>>::slashable_balance_of_vote_weight,
+		<Module<T>>::slashable_balance_of_fn(),
 	);
 
 	// reduce
@@ -423,8 +419,8 @@ where
 		let compact = compact.clone();
 		let assignments = compact.into_assignment(nominator_at, validator_at).unwrap();
 		let staked = sp_npos_elections::assignment_ratio_to_staked(
-			assignments,
-			<Module<T>>::slashable_balance_of_vote_weight,
+			assignments.clone(),
+			<Module<T>>::slashable_balance_of_fn(),
 		);
 
 		let support_map = build_support_map::<T::AccountId>(&winners, &staked)

src/testing_utils.rs

@@ -193,9 +193,10 @@ pub fn get_weak_solution<T: Trait>(
 			who: w.clone(),
 			distribution: vec![(
 				w.clone(),
-				<T::CurrencyToVote as Convert<BalanceOf<T>, u64>>::convert(
-					<Module<T>>::slashable_balance_of(&w),
-				) as ExtendedBalance,
+				<Module<T>>::slashable_balance_of_vote_weight(
+					&w,
+					T::Currency::total_issuance(),
+				).into(),
 			)],
 		})
 	});
@@ -220,11 +221,6 @@ pub fn get_weak_solution<T: Trait>(
 			.position(|x| x == a)
 			.and_then(|i| <usize as TryInto<ValidatorIndex>>::try_into(i).ok())
 	};
-	let stake_of = |who: &T::AccountId| -> VoteWeight {
-		<T::CurrencyToVote as Convert<BalanceOf<T>, u64>>::convert(
-			<Module<T>>::slashable_balance_of(who),
-		)
-	};
 
 	// convert back to ratio assignment. This takes less space.
 	let low_accuracy_assignment = assignment_staked_to_ratio_normalized(staked_assignments)
@@ -234,7 +230,7 @@ pub fn get_weak_solution<T: Trait>(
 	let score = {
 		let staked = assignment_ratio_to_staked::<_, OffchainAccuracy, _>(
 			low_accuracy_assignment.clone(),
-			stake_of
+			<Module<T>>::slashable_balance_of_fn(),
 		);
 
 		let support_map = build_support_map::<T::AccountId>(
@@ -325,7 +321,7 @@ pub fn get_single_winner_solution<T: Trait>(
 
 	let stake = <Staking<T>>::slashable_balance_of(&winner);
 	let stake =
-		<T::CurrencyToVote as Convert<BalanceOf<T>, VoteWeight>>::convert(stake) as ExtendedBalance;
+		<T::CurrencyToVote>::to_vote(stake, T::Currency::total_issuance()) as ExtendedBalance;
 
 	let val_index = val_index as ValidatorIndex;
 	let nom_index = nom_index as NominatorIndex;