Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Removes the Default implementation for RewardDestination #2402

Merged
merged 27 commits into from
Jan 27, 2024
Merged

Removes the Default implementation for RewardDestination #2402

merged 27 commits into from
Jan 27, 2024

Conversation

gpestana
Copy link
Contributor

@gpestana gpestana commented Nov 19, 2023 •
edited
Loading

This PR removes current default for RewardDestination, which may cause confusion since a ledger should not have a default reward destination: either it has a reward destination, or something is wrong. It also changes the Payee's reward destination in storage from ValueQuery to OptionQuery.

In addition, it adds a try_state check to make sure each bonded ledger have a valid reward destination.

Closes #2063

@gpestana gpestana added T1-FRAME This PR/Issue is related to core FRAME, the framework. T2-pallets This PR/Issue is related to a particular pallet. D0-easy Can be fixed primarily by duplicating and adapting code by an intermediate coder. labels Nov 19, 2023
@gpestana gpestana self-assigned this Nov 19, 2023
@gpestana gpestana requested review from a team November 19, 2023 20:38
@gpestana gpestana changed the title Deprecate RewardDestination default Removes the Default implementation for RewardDestination Nov 19, 2023
georgepisaltu
georgepisaltu reviewed Nov 20, 2023
View reviewed changes
Copy link
Contributor

@georgepisaltu georgepisaltu left a comment

Choose a reason for hiding this comment

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

I agree with the idea there shouldn't be a default reward destination, however removing it creates some problems, see below.

substrate/frame/staking/src/pallet/mod.rs Show resolved Hide resolved
@gpestana gpestana marked this pull request as draft November 20, 2023 13:53
Ank4n
Ank4n approved these changes Nov 27, 2023
View reviewed changes
Copy link
Contributor

@Ank4n Ank4n left a comment

Choose a reason for hiding this comment

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

LGTM

@gpestana
Copy link
Contributor Author

bot fmt

@gpestana gpestana marked this pull request as ready for review November 27, 2023 15:08
@gpestana
Copy link
Contributor Author

bot fmt

rossbulat
rossbulat approved these changes Nov 28, 2023
View reviewed changes
Copy link

@rossbulat rossbulat left a comment

Choose a reason for hiding this comment

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

Noticed a couple of grammatical errors, LGTM otherwise

georgepisaltu
georgepisaltu approved these changes Nov 28, 2023
View reviewed changes
Copy link
Contributor

@georgepisaltu georgepisaltu left a comment

Choose a reason for hiding this comment

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

A couple of nits but otherwise LGTM

substrate/frame/staking/src/pallet/impls.rs Outdated Show resolved Hide resolved
substrate/frame/staking/src/pallet/impls.rs Outdated Show resolved Hide resolved
gpestana and others added 2 commits November 28, 2023 11:35
@gpestana
Update substrate/frame/staking/src/tests.rs
f7469d0 
Co-authored-by: Ross Bulat <ross@parity.io>
@gpestana
Update substrate/frame/staking/src/pallet/impls.rs
a81406b 
Co-authored-by: Ross Bulat <ross@parity.io>
@gpestana
Copy link
Contributor Author

bot fmt

auto-merge was automatically disabled December 13, 2023 19:17

Merge queue setting changed

kianenigma
kianenigma reviewed Jan 15, 2024
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.

Don't see why this is not merged?

@gpestana
Copy link
Contributor Author

bot rfmt

@gpestana
Copy link
Contributor Author

bot clean

@gpestana
Copy link
Contributor Author

bot fmt

@command-bot
Copy link

command-bot bot commented Jan 15, 2024 •
edited
Loading

@gpestana https://gitlab.parity.io/parity/mirrors/polkadot-sdk/-/jobs/4924667 was started for your command "$PIPELINE_SCRIPTS_DIR/commands/fmt/fmt.sh". Check out https://gitlab.parity.io/parity/mirrors/polkadot-sdk/-/pipelines?page=1&scope=all&username=group_605_bot to know what else is being executed currently.

Comment bot cancel 2-1f4a240e-aad8-4137-998e-2ccf04f27b4c to cancel this command or bot cancel to cancel all commands in this pull request.

@command-bot
Copy link

command-bot bot commented Jan 15, 2024

@gpestana Command "$PIPELINE_SCRIPTS_DIR/commands/fmt/fmt.sh" has finished. Result: https://gitlab.parity.io/parity/mirrors/polkadot-sdk/-/jobs/4924667 has finished. If any artifacts were generated, you can download them from https://gitlab.parity.io/parity/mirrors/polkadot-sdk/-/jobs/4924667/artifacts/download.

@paritytech-cicd-pr
Copy link

The CI pipeline was cancelled due to failure one of the required jobs.
Job name: test-linux-stable 2/3
Logs: https://gitlab.parity.io/parity/mirrors/polkadot-sdk/-/jobs/5039379

@gpestana gpestana added this pull request to the merge queue Jan 27, 2024
Merged via the queue into master with commit a9992db Jan 27, 2024
122 of 124 checks passed
@gpestana gpestana deleted the gpestana/2380-deprecate_default branch January 27, 2024 16:46
al3mart pushed a commit that referenced this pull request Jan 29, 2024
@gpestana @al3mart
Removes the Default implementation for RewardDestination (#2402)
272af77 
This PR removes current default for `RewardDestination`, which may cause
confusion since a ledger should not have a default reward destination:
either it has a reward destination, or something is wrong. It also
changes the `Payee`'s reward destination in storage from `ValueQuery` to
`OptionQuery`.

In addition, it adds a `try_state` check to make sure each bonded ledger
have a valid reward destination.

Closes #2063

---------

Co-authored-by: command-bot <>
Co-authored-by: Ross Bulat <ross@parity.io>
@github-actions github-actions bot mentioned this pull request Mar 13, 2024
Closed
bgallois pushed a commit to duniter/duniter-polkadot-sdk that referenced this pull request Mar 25, 2024
@gpestana
Removes the Default implementation for RewardDestination (parityt…
c02ac37 
…ech#2402)

This PR removes current default for `RewardDestination`, which may cause
confusion since a ledger should not have a default reward destination:
either it has a reward destination, or something is wrong. It also
changes the `Payee`'s reward destination in storage from `ValueQuery` to
`OptionQuery`.

In addition, it adds a `try_state` check to make sure each bonded ledger
have a valid reward destination.

Closes paritytech#2063

---------

Co-authored-by: command-bot <>
Co-authored-by: Ross Bulat <ross@parity.io>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@georgepisaltu georgepisaltu georgepisaltu approved these changes

@kianenigma kianenigma kianenigma left review comments

@lexnv lexnv lexnv approved these changes

@rossbulat rossbulat rossbulat approved these changes

@Ank4n Ank4n Ank4n approved these changes

@bkchr bkchr bkchr approved these changes

Assignees

@gpestana gpestana

Labels
D0-easy Can be fixed primarily by duplicating and adapting code by an intermediate coder. R0-silent Changes should not be mentioned in any release notes T1-FRAME This PR/Issue is related to core FRAME, the framework. T2-pallets This PR/Issue is related to a particular pallet.
Projects
(Nominated) Proof of Stake
Status: ✅ Done
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Remove impl Default for RewardDestination
8 participants
@gpestana @paritytech-cicd-pr @kianenigma @bkchr @Ank4n @rossbulat @georgepisaltu @lexnv