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
all tests fixed
  • Loading branch information
kianenigma committed Mar 6, 2019
commit baf8778a9feb6360c2e6632b2c5d1d2c7fd82f58
70 changes: 28 additions & 42 deletions srml/staking/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -575,7 +575,6 @@ impl<T: Trait> Module<T> {
let own_slash = own_slash - T::Currency::slash(v, own_slash).unwrap_or_default();
// The amount remaining that we can't slash from the validator, that must be taken from the nominators.
let rest_slash = slash - own_slash;
println!("Slashing!! {:?}", exposure);
if !rest_slash.is_zero() {
// The total to be slashed from the nominators.
let total = exposure.total - exposure.own;
Expand Down Expand Up @@ -624,7 +623,6 @@ impl<T: Trait> Module<T> {
}
safe_mul_rational(exposure.own)
};
println!("Rewarding {:?} by {:?}", who, validator_cut + off_the_table);
Self::make_payout(who, validator_cut + off_the_table);
}

Expand Down Expand Up @@ -711,6 +709,9 @@ impl<T: Trait> Module<T> {
exposure: Exposure { total: stash_balance, own: stash_balance, others: vec![] },
}
}).collect::<Vec<Candidate<T::AccountId, BalanceOf<T>>>>();

// Just to be used when we are below minimum validator count
let original_candidates = candidates.clone();

// 2- Collect the nominators with the associated votes.
// Also collect approval stake along the way.
Expand All @@ -731,11 +732,6 @@ impl<T: Trait> Module<T> {
load : Perquill::zero(),
}
}).collect::<Vec<Nominations<T::AccountId, BalanceOf<T>>>>();

println!("+++ Selecting candidates {} / {} ", candidates.len(), nominations.len());
println!("+++ candidates {:?} ", candidates);
println!("+++ nominaotions {:?} ", nominations);


// 3- optimization:
// candidates who have 0 stake => have no votes or all null-votes. best to kick them out not.
Expand Down Expand Up @@ -810,57 +806,47 @@ impl<T: Trait> Module<T> {
}
}
}
}
else { // end of `if candidates.len() > rounds`
} // if candidates.len() > rounds
else {
if candidates.len() > Self::minimum_validator_count() as usize {
// if we don't have enough candidates, just choose all that have some vote.
println!("++ Code yellow. Choosing all candidates");
elected_candidates = candidates;
}
else {
Copy link
Member

Choose a reason for hiding this comment

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

This else should be on the line above.

// if we have less than minimum, use the previous validator set.
println!("Code red. choosing previous candidates");
elected_candidates = <Validators<T>>::enumerate().map(|(who, _)| {
let exposure = Self::stakers(&who);
Candidate {
who,
approval_stake: BalanceOf::<T>::zero(), // don't care
score: Perquill::zero(), // don't care
exposure,
}
}).collect::<Vec<Candidate<T::AccountId, BalanceOf<T>>>>();
elected_candidates = original_candidates;
}
}

// 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.clone();

// 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);
if slash_count > 1 {
<SlashCount<T>>::insert(v, slash_count - 1);
}
}

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

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

slot_stake = min_candidate.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.

Suggested change
}
} else {

else {
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
else {

// This will only happen in the very first era.
// This will only happen in the very first era.
slot_stake = BalanceOf::<T>::zero();
}

// 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);
if slash_count > 1 {
<SlashCount<T>>::insert(v, slash_count - 1);
}
}

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

// 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.

);

<SlotStake<T>>::put(&slot_stake);
Copy link
Member

Choose a reason for hiding this comment

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

any reason why this was moved down here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

While automatic merges probably. Will move it back up in the final merge.

slot_stake
}
Expand Down
4 changes: 2 additions & 2 deletions srml/staking/src/mock.rs
Original file line number Diff line number Diff line change
Expand Up @@ -198,14 +198,14 @@ impl ExtBuilder {
(21, 20, balance_factor * 2000),
(31, 30, balance_factor * 3000),
(41, 40, balance_factor * 4000),
// nominators
// nominator
(101, 100, balance_factor * 500)
]
} else {
vec![
(11, 10, balance_factor * 1000),
(21, 20, balance_factor * 2000),
// nominators
// nominator
(101, 100, balance_factor * 500)
]
},
Expand Down
51 changes: 31 additions & 20 deletions srml/staking/src/tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -28,9 +28,6 @@ use srml_support::traits::Currency;
fn basic_setup_works() {
// Verifies initial conditions of mock
with_externalities(&mut ExtBuilder::default()
.nominate(true)
.sessions_per_era(1)
.session_length(1)
.build(),
|| {
assert_eq!(Staking::bonded(&11), Some(10)); // Account 11 is stashed and locked, and account 10 is the controller
Expand Down Expand Up @@ -290,7 +287,10 @@ fn rewards_should_work() {
// * rewards get recorded per session
// * rewards get paid per Era
// * Check that nominators are also rewarded
with_externalities(&mut ExtBuilder::default().nominate(true).build(),
with_externalities(&mut ExtBuilder::default()
.session_length(3)
.sessions_per_era(3)
.build(),
|| {
let delay = 2;
// this test is only in the scope of one era. Since this variable changes
Expand Down Expand Up @@ -440,7 +440,12 @@ fn staking_should_work() {
// * new validators can be added to the default set
// * 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).build(), || {
with_externalities(&mut ExtBuilder::default()
.session_length(1)
.sessions_per_era(3)
.nominate(false)
.build(),
|| {
assert_eq!(Staking::era_length(), 3);
// remember + compare this along with the test.
assert_eq!(Session::validators(), vec![100, 20, 10]);
Expand All @@ -457,8 +462,8 @@ fn staking_should_work() {

// --- Block 1:
System::set_block_number(1);


// add a new candidate for being a validator. account 3 controlled by 4.
assert_ok!(Staking::bond(Origin::signed(3), 4, 1500, RewardDestination::Controller)); // balance of 3 = 3000, stashed = 1500

Expand Down Expand Up @@ -618,10 +623,6 @@ fn nominating_and_rewards_should_work() {
.validator_pool(true)
.build(),
|| {
// roll the first era
System::set_block_number(1);
Session::check_rotate_session(System::block_number());

// initial validators
// note that since the test is `.nominate(false)` 100 is also treated initially as a validators
assert_eq!(Session::validators(), vec![100, 40, 30, 20, 10]);
Expand Down Expand Up @@ -651,9 +652,9 @@ fn nominating_and_rewards_should_work() {
assert_ok!(Staking::bond(Origin::signed(3), 4, 500, RewardDestination::Stash));
assert_ok!(Staking::nominate(Origin::signed(4), vec![10, 20, 40]));

System::set_block_number(2);
System::set_block_number(1);
Session::check_rotate_session(System::block_number());
assert_eq!(Staking::current_era(), 2);
assert_eq!(Staking::current_era(), 1);
// 10 and 20 have more votes, they will be chosen by phragmen.
assert_eq!(Session::validators(), vec![20, 10]);
// validators must have already received some rewards.
Expand Down Expand Up @@ -967,7 +968,10 @@ fn validator_payment_prefs_work() {
// Test that validator preferences are correctly honored
// Note: unstake threshold is being directly tested in slashing tests.
// This test will focus on validator payment.
with_externalities(&mut ExtBuilder::default().nominate(true).build(),
with_externalities(&mut ExtBuilder::default()
.session_length(3)
.sessions_per_era(3)
.build(),
|| {
let session_reward = 10;
let validator_cut = 5;
Expand Down Expand Up @@ -1382,7 +1386,8 @@ fn phragmen_poc_works() {
//
// NOTE: doesn't X/Y/Z's stash value make a difference here in phragmen?
with_externalities(&mut ExtBuilder::default()
.session_length(1).sessions_per_era(1).build(),
.nominate(false)
.build(),
|| {
// initial setup of 10 and 20, both validators + 100.
assert_eq!(Session::validators(), vec![100, 20, 10]);
Expand Down Expand Up @@ -1432,9 +1437,9 @@ fn phragmen_poc_works() {
// This is only because 30 has been bonded on the fly. 35 is the point, not 'own' Exposure.
assert_eq!(Staking::stakers(30).own, 0);
assert_eq!(Staking::stakers(30).total, 0 + 35);

assert_eq!(Staking::stakers(20).own, 2000);
assert_eq!(Staking::stakers(20).total, 2000 + 25);
// same as above. +25 is the point
assert_eq!(Staking::stakers(20).own, 2010);
assert_eq!(Staking::stakers(20).total, 2010 + 25);

// 30(Z) was supported by B-4 and C-6 with stake 20 and 15 respectively.
assert_eq!(Staking::stakers(30).others.iter().map(|e| e.value).collect::<Vec<BalanceOf<Test>>>(), vec![15, 20]);
Expand All @@ -1450,7 +1455,9 @@ fn phragmen_poc_works() {
fn switching_roles() {
// Show: It should be possible to switch between roles (nominator, validator, idle) with minimal overhead.
with_externalities(&mut ExtBuilder::default()
.session_length(1).build(),
.nominate(false)
.sessions_per_era(3)
.build(),
|| {
assert_eq!(Session::validators(), vec![100, 20, 10]);

Expand Down Expand Up @@ -1513,7 +1520,11 @@ fn switching_roles() {
#[test]
fn wrong_vote_is_null() {
with_externalities(&mut ExtBuilder::default()
.session_length(1).sessions_per_era(1).validator_pool(true).build(),
.session_length(1)
.sessions_per_era(1)
.nominate(false)
.validator_pool(true)
.build(),
|| {
// from the first era onward, only two will be chosen
assert_eq!(Session::validators(), vec![100, 40, 30, 20, 10]);
Expand Down