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 all commits
Commits
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
20 changes: 10 additions & 10 deletions frame/staking/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -187,7 +187,7 @@
//! The validator and its nominator split their reward as following:
//!
//! The validator can declare an amount, named
//! [`validator_payment`](./struct.ValidatorPrefs.html#structfield.validator_payment), that does not
//! [`commission`](./struct.ValidatorPrefs.html#structfield.commission), that does not
//! get shared with the nominators at each reward payout through its
//! [`ValidatorPrefs`](./struct.ValidatorPrefs.html). This value gets deducted from the total reward
//! that is paid to the validator and its nominators. The remaining portion is split among the
Expand Down Expand Up @@ -347,19 +347,19 @@ impl Default for RewardDestination {
}
}

/// Preference of what happens on a slash event.
/// Preference of what happens regarding validation.
#[derive(PartialEq, Eq, Clone, Encode, Decode, RuntimeDebug)]
pub struct ValidatorPrefs<Balance: HasCompact> {
pub struct ValidatorPrefs {
/// Reward that validator takes up-front; only the rest is split between themselves and
/// nominators.
#[codec(compact)]
pub validator_payment: Balance,
pub commission: Perbill,
}

impl<B: Default + HasCompact + Copy> Default for ValidatorPrefs<B> {
impl Default for ValidatorPrefs {
fn default() -> Self {
ValidatorPrefs {
validator_payment: Default::default(),
commission: Default::default(),
}
}
}
Expand Down Expand Up @@ -657,7 +657,7 @@ decl_storage! {
pub Payee get(fn payee): map T::AccountId => RewardDestination;

/// The map from (wannabe) validator stash key to the preferences of that validator.
pub Validators get(fn validators): linked_map T::AccountId => ValidatorPrefs<BalanceOf<T>>;
pub Validators get(fn validators): linked_map T::AccountId => ValidatorPrefs;

/// The map from nominator stash key to the set of stash keys of all validators to nominate.
///
Expand Down Expand Up @@ -982,7 +982,7 @@ decl_module! {
/// - Writes are limited to the `origin` account key.
/// # </weight>
#[weight = SimpleDispatchInfo::FixedNormal(750_000)]
fn validate(origin, prefs: ValidatorPrefs<BalanceOf<T>>) {
fn validate(origin, prefs: ValidatorPrefs) {
Self::ensure_storage_upgraded();

let controller = ensure_signed(origin)?;
Expand Down Expand Up @@ -1252,8 +1252,8 @@ impl<T: Trait> Module<T> {
/// nominators' balance, pro-rata based on their exposure, after having removed the validator's
/// pre-payout cut.
fn reward_validator(stash: &T::AccountId, reward: BalanceOf<T>) -> PositiveImbalanceOf<T> {
let off_the_table = reward.min(Self::validators(stash).validator_payment);
let reward = reward - off_the_table;
let off_the_table = Self::validators(stash).commission * reward;
let reward = reward.saturating_sub(off_the_table);
Copy link
Contributor

Choose a reason for hiding this comment

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

We can't really end up with anything greater than reward, can we? Just making sure, I'm in favour of the change anyway, just to be safe.

Copy link
Member Author

Choose a reason for hiding this comment

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

i don't think so since Perbill can only go up to 1, but if i didn't do it then an auditor would surely pick up on it and request the change anyway :-)

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes Perbill is between 0 and 1

Copy link
Contributor

@tomusdrw tomusdrw Nov 28, 2019

Choose a reason for hiding this comment

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

A nitpicky auditor would suggest to replace the multiplication above with saturating_mul as well, just in case Perbill goes over 1 🤣

Copy link
Contributor

Choose a reason for hiding this comment

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

unfortunately Perbill doesn't implement saturating_mul with balance only saturing_mul with another Perbill hehe

let mut imbalance = <PositiveImbalanceOf<T>>::zero();
let validator_cut = if reward.is_zero() {
Zero::zero()
Expand Down
7 changes: 3 additions & 4 deletions frame/staking/src/tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -965,7 +965,6 @@ fn validator_payment_prefs_work() {
// This test will focus on validator payment.
ExtBuilder::default().build().execute_with(|| {
// Initial config
let validator_cut = 5;
let stash_initial_balance = Balances::total_balance(&11);

// check the balance of a validator accounts.
Expand All @@ -983,7 +982,7 @@ fn validator_payment_prefs_work() {
});
<Payee<Test>>::insert(&2, RewardDestination::Stash);
<Validators<Test>>::insert(&11, ValidatorPrefs {
validator_payment: validator_cut
commission: Perbill::from_percent(50),
});

// Compute total payout now for whole duration as other parameter won't change
Expand All @@ -994,9 +993,9 @@ fn validator_payment_prefs_work() {
start_era(1);

// whats left to be shared is the sum of 3 rounds minus the validator's cut.
let shared_cut = total_payout_0 - validator_cut;
let shared_cut = total_payout_0 / 2;
// Validator's payee is Staked account, 11, reward will be paid here.
assert_eq!(Balances::total_balance(&11), stash_initial_balance + shared_cut / 2 + validator_cut);
assert_eq!(Balances::total_balance(&11), stash_initial_balance + shared_cut / 2 + shared_cut);
// Controller account will not get any reward.
assert_eq!(Balances::total_balance(&10), 1);
// Rest of the reward will be shared and paid to the nominator in stake.
Expand Down