Skip to content
This repository was archived by the owner on Nov 15, 2023. It is now read-only.
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
27 commits
Select commit Hold shift + click to select a range
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
cleanup
  • Loading branch information
kianenigma committed Mar 6, 2019
commit cfaf7b95eea7eb6c96c0e556e027e0fd40227fb2
16 changes: 9 additions & 7 deletions srml/staking/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -738,7 +738,7 @@ impl<T: Trait> Module<T> {
let mut candidates = candidates.into_iter().filter(|c| c.approval_stake > BalanceOf::<T>::zero())
.collect::<Vec<Candidate<T::AccountId, BalanceOf<T>>>>();

// If we have more candidates then needed, run phragmen.
// 4- If we have more candidates then needed, run phragmen.
if candidates.len() > rounds {
// Main election loop
for _round in 0..rounds {
Expand Down Expand Up @@ -771,7 +771,7 @@ impl<T: Trait> Module<T> {
let (winner_index, _) = candidates.iter().enumerate().min_by_key(|&(_i, c)| c.score.extract())
.expect("candidates length is checked to be >0; qed");

// update nominator and vote load
// loop 3: update nominator and vote load
let winner = candidates.remove(winner_index);
for nominator_idx in 0..nominations.len() {
for vote_idx in 0..nominations[nominator_idx].nominees.len() {
Expand All @@ -785,10 +785,12 @@ impl<T: Trait> Module<T> {
}
}
}

elected_candidates.push(winner);

} // end of all rounds

// Update backing stake of candidates and nominators
// 4.1- Update backing stake of candidates and nominators
for nomination in &mut nominations {
for vote in &mut nomination.nominees {
// if the target of this vote is among the winners, otherwise let go.
Expand Down Expand Up @@ -818,7 +820,7 @@ impl<T: Trait> Module<T> {
}
}

// Figure out the minimum stake behind a slot.
// 5- Figure out the minimum stake behind a slot.
let slot_stake;
if let Some(min_candidate) = elected_candidates.iter().min_by_key(|c| c.exposure.total) {
Copy link
Member

Choose a reason for hiding this comment

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

Okay that can be simplified to

let slot_stake = elected_candidates.iter().min_by_key(|c| c.exposure.total).map(|c| c.exposure.total).unwrap_or_default();

slot_stake = min_candidate.exposure.total;
Expand All @@ -828,7 +830,7 @@ impl<T: Trait> Module<T> {
slot_stake = BalanceOf::<T>::zero();
}

// Clear Stakers and reduce their slash_count.
// 6- Clear Stakers and reduce their slash_count.
for v in <session::Module<T>>::validators().iter() {
<Stakers<T>>::remove(v);
let slash_count = <SlashCount<T>>::take(v);
Expand All @@ -837,12 +839,12 @@ impl<T: Trait> Module<T> {
}
}

// Populate Stakers.
// 7- Populate Stakers.
for candidate in &elected_candidates {
<Stakers<T>>::insert(candidate.who.clone(), candidate.exposure.clone());
}

// Set the new validator set.
// 8- Set the new validator set.
<session::Module<T>>::set_validators(
&elected_candidates.into_iter().map(|i| i.who).collect::<Vec<_>>()
Copy link
Member

Choose a reason for hiding this comment

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

You just iterated this list above, maybe merge both iterations?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

With the new function decomposition its not the same + this is passing the whole Vec inside, I don't get exactly what you mean.

Copy link
Member

Choose a reason for hiding this comment

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

If you refactored and saying that my comment does not apply anymore, what should I do without seeing the new code?(nevertheless, you were iterating this list twice and that does not need to be done)

Copy link
Member

Choose a reason for hiding this comment

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

Just for reference, you now iterate the same vector 3 times :D
This could all be done in one iteration.

);
Expand Down
53 changes: 8 additions & 45 deletions srml/staking/src/tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -191,11 +191,6 @@ fn offline_grace_should_delay_slashing() {
});
}

#[test]
fn reporting_misbehaviors_work() {
// TODO: Does this code exist?
// WONTFIX.
}

#[test]
fn max_unstake_threshold_works() {
Expand Down Expand Up @@ -251,12 +246,7 @@ fn max_unstake_threshold_works() {
#[test]
fn slashing_does_not_cause_underflow() {
// Tests that slashing more than a user has does not underflow
with_externalities(&mut ExtBuilder::default()
.sessions_per_era(1)
.session_length(1)
.nominate(true)
.build(),
|| {
with_externalities(&mut ExtBuilder::default().build(), || {
// One user with less than `max_value` will test underflow does not occur
Balances::set_free_balance(&10, 1);

Expand Down Expand Up @@ -441,7 +431,6 @@ fn staking_should_work() {
// * new ones will be chosen per era
// * either one can unlock the stash and back-down from being a validator via `chill`ing.
with_externalities(&mut ExtBuilder::default()
.session_length(1)
.sessions_per_era(3)
.nominate(false)
.build(),
Expand Down Expand Up @@ -527,11 +516,6 @@ fn staking_should_work() {
});
}

#[test]
fn no_one_nominates_does_not_panic() {
// TODO: Test the behavior when no one is nominating.
}

#[test]
fn less_than_needed_candidates_works() {
// Test the situation where the number of validators are less than `ValidatorCount` but more than <MinValidators>
Expand Down Expand Up @@ -699,9 +683,7 @@ fn nominating_and_rewards_should_work() {
#[test]
fn nominators_also_get_slashed() {
// A nominator should be slashed if the validator they nominated is slashed
with_externalities(&mut ExtBuilder::default()
.session_length(1).sessions_per_era(1).build(),
|| {
with_externalities(&mut ExtBuilder::default().build(), || {
assert_eq!(Staking::era_length(), 1);
assert_eq!(Staking::validator_count(), 2);
// slash happens immediately.
Expand Down Expand Up @@ -750,7 +732,8 @@ fn double_staking_should_fail() {
// * an account already bonded as stash cannot nominate.
// * an account already bonded as controller can nominate.
with_externalities(&mut ExtBuilder::default()
.session_length(1).sessions_per_era(2).build(),
.sessions_per_era(2)
.build(),
|| {
let arbitrary_value = 5;
System::set_block_number(1);
Expand All @@ -770,9 +753,7 @@ fn double_staking_should_fail() {
#[test]
fn session_and_eras_work() {
with_externalities(&mut ExtBuilder::default()
.session_length(1)
.sessions_per_era(2)
.nominate(true)
.reward(10)
.build(),
|| {
Expand Down Expand Up @@ -864,12 +845,7 @@ fn cannot_transfer_staked_balance() {
#[test]
fn cannot_reserve_staked_balance() {
// Checks that a bonded account cannot reserve balance from free balance
with_externalities(&mut ExtBuilder::default()
.session_length(1)
.sessions_per_era(1)
.nominate(true)
.build(),
|| {
with_externalities(&mut ExtBuilder::default().build(), || {
System::set_block_number(1);
Session::check_rotate_session(System::block_number());

Expand All @@ -892,12 +868,7 @@ fn cannot_reserve_staked_balance() {
#[test]
fn reward_destination_works() {
// Rewards go to the correct destination as determined in Payee
with_externalities(&mut ExtBuilder::default()
.sessions_per_era(1)
.nominate(true)
.session_length(1)
.build(),
|| {
with_externalities(&mut ExtBuilder::default().build(), || {
// Check that account 10 is a validator
assert!(<Validators<Test>>::exists(10));
// Check the balance of the validator account
Expand Down Expand Up @@ -1087,10 +1058,7 @@ fn bond_extra_and_withdraw_unbonded_works() {
// * it can unbond a portion of its funds from the stash account.
// * Once the unbonding period is done, it can actually take the funds out of the stash.
with_externalities(&mut ExtBuilder::default()
.nominate(true)
.sessions_per_era(1)
.session_length(1)
.reward(10) // it is default, just for verbosity
.reward(10) // it is the default, just for verbosity
.build(),
|| {
// Set payee to controller. avoids confusion
Expand Down Expand Up @@ -1187,12 +1155,7 @@ fn slot_stake_is_least_staked_validator_and_limits_maximum_punishment() {
// Test that slot_stake is the maximum punishment that can happen to a validator
// Note that rewardDestination is the stash account by default
// Note that unlike reward slash will affect free_balance, not the stash account.
with_externalities(&mut ExtBuilder::default()
.session_length(1)
.sessions_per_era(1)
.nominate(true)
.build(),
|| {
with_externalities(&mut ExtBuilder::default().build(), || {
// Confirm validator count is 2
assert_eq!(Staking::validator_count(), 2);
// Confirm account 10 and 20 are validators
Expand Down