Skip to content
This repository has been archived by the owner on Nov 15, 2023. It is now read-only.

staking: Fix Reward usage #10887

Merged
merged 9 commits into from
Apr 21, 2022
Merged

staking: Fix Reward usage #10887

merged 9 commits into from
Apr 21, 2022

Conversation

emostov
Copy link
Contributor

@emostov emostov commented Feb 19, 2022

Supersedes: #10885

@emostov emostov requested a review from kianenigma as a code owner February 19, 2022 23:59
@github-actions github-actions bot added the A0-please_review Pull request needs code review. label Feb 19, 2022
emostov
emostov commented Feb 20, 2022
View reviewed changes
frame/staking/src/tests.rs Outdated
@@ -3363,10 +3378,13 @@ fn test_payout_stakers() {
Staking::reward_by_ids(vec![(11, 1)]);

// compute and ensure the reward amount is greater than zero.
let _ = current_total_payout_for_duration(reward_time_per_era());
let payout = current_total_payout_for_duration(reward_time_per_era());
let actual_paid_out = payout_exposure_part.mul_ceil(payout);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am not sure why only .mul_ceil worked here; .mul ends up being 1 less which I don't understand because we use normal multiplication here:

substrate/frame/staking/src/pallet/impls.rs

Lines 186 to 187 in f72fe05

let nominator_reward: BalanceOf<T> =
nominator_exposure_part * validator_leftover_payout;

Copy link
Member

Choose a reason for hiding this comment

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

I feel we should figure this out before we merge :/

Copy link
Contributor Author

Choose a reason for hiding this comment

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

002dafc

kianenigma
kianenigma approved these changes Feb 20, 2022
View reviewed changes
Copy link
Contributor

@kianenigma kianenigma left a comment

Choose a reason for hiding this comment

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

Looks good to me as long as the default behavior can be preserved.

@emostov emostov requested a review from shawntabrizi February 20, 2022 21:44
@kianenigma kianenigma mentioned this pull request Feb 22, 2022
Closed
@emostov emostov added B0-silent Changes should not be mentioned in any release notes C1-low PR touches the given topic and has a low impact on builders. D9-needsaudit 👮 PR contains changes to fund-managing logic that should be properly reviewed and externally audited labels Mar 2, 2022
@kianenigma
Copy link
Contributor

ping.

@kianenigma kianenigma requested review from KiChjang, ggwpez and bkchr April 10, 2022 15:23
@shawntabrizi
Copy link
Member

bot rebase

@paritytech-processbot
Copy link

Rebasing

@shawntabrizi
Copy link
Member

bot merge

@paritytech-processbot
Copy link

Waiting for commit status.

@paritytech-processbot
Copy link

Merge cancelled due to error. Error: Statuses failed for c07685a

@emostov
Copy link
Contributor Author

emostov commented Apr 21, 2022

bot merge

@paritytech-processbot paritytech-processbot bot merged commit f160775 into master Apr 21, 2022
@paritytech-processbot paritytech-processbot bot deleted the zeke-make-reward-work branch April 21, 2022 13:50
@github-actions github-actions bot mentioned this pull request May 31, 2022
Closed
@jakoblell jakoblell added D1-audited 👍 PR contains changes to fund-managing logic that has been properly reviewed and externally audited and removed D9-needsaudit 👮 PR contains changes to fund-managing logic that should be properly reviewed and externally audited labels Jun 7, 2022
godcodehunter pushed a commit to sensoriumxr/substrate that referenced this pull request Jun 22, 2022
@emostov @godcodehunter
staking: Fix Reward usage (paritytech#10887)
383baf3 
* staking: Fix `Reward` usage

* Some small fixes

* Check on_unbalanced was called

* Improve tests

* Add not for Reward; FMT

* 🤦

Co-authored-by: parity-processbot <>
DaviRain-Su pushed a commit to octopus-network/substrate that referenced this pull request Aug 23, 2022
@emostov @DaviRain-Su
staking: Fix Reward usage (paritytech#10887)
77d36dd 
* staking: Fix `Reward` usage

* Some small fixes

* Check on_unbalanced was called

* Improve tests

* Add not for Reward; FMT

* 🤦

Co-authored-by: parity-processbot <>
ark0f pushed a commit to gear-tech/substrate that referenced this pull request Feb 27, 2023
@emostov @ark0f
staking: Fix Reward usage (paritytech#10887)
fa2adff 
* staking: Fix `Reward` usage

* Some small fixes

* Check on_unbalanced was called

* Improve tests

* Add not for Reward; FMT

* 🤦

Co-authored-by: parity-processbot <>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@kianenigma kianenigma kianenigma approved these changes

@KiChjang KiChjang KiChjang approved these changes

@shawntabrizi shawntabrizi shawntabrizi approved these changes

@ggwpez ggwpez Awaiting requested review from ggwpez

@bkchr bkchr Awaiting requested review from bkchr

Assignees
No one assigned
Labels
A0-please_review Pull request needs code review. B0-silent Changes should not be mentioned in any release notes C1-low PR touches the given topic and has a low impact on builders. D1-audited 👍 PR contains changes to fund-managing logic that has been properly reviewed and externally audited
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@emostov @kianenigma @shawntabrizi @KiChjang @jakoblell