WIP Make ChangeMembers trait fallible #5985

apopiak · 2020-05-12T09:39:20Z

This PR changes the ChangeMembers trait to be fallible.
change_members, change_members_sorted and set_members_sorted are no longer guaranteed to set all members successfully.
Instead they return the new number of members after the operation. There is also a new ensure_can_change_members function that is meant to allow checking whether a member change will execute without errors.

Preparation for #5802 as it uses the member limit to estimate weights in pallet-collective (an implementor of ChangeMembers).

✄ -----------------------------------------------------------------------------

You labeled the PR with appropriate labels if you have permissions to do so.
You mentioned a related issue if this PR related to it, e.g. Fixes #228 or Related #1337.
You asked any particular reviewers to review. If you aren't sure, start with GH suggestions.
You bumped the runtime version if there are breaking changes in the runtime.
Has the PR altered the external API or interfaces used by Polkadot? Do you have the corresponding Polkadot PR ready?

…ust usage

parity-cla-bot · 2020-05-12T09:39:23Z

It looks like @apopiak signed our Contributor License Agreement. 👍

Many thanks,

Parity Technologies CLA Bot

frame/elections-phragmen/src/lib.rs

frame/support/src/traits.rs

shawntabrizi · 2020-05-12T10:03:07Z

frame/scored-pool/src/lib.rs

@@ -407,16 +407,20 @@ impl<T: Trait<I>, I: Instance> Module<T, I> {
 		new_members.sort();

 		let old_members = <Members<T, I>>::get();
+		// TODO: What to do in case of error?


regarding changes to this pallet. If you look more closely at the pallet code, you will see there is a storage item MemberCount which determines the number of members selected from the pool.

If I understand the end goals here correctly, you will make it so there is an upper limit to the number of members that can exist in a collective. So you should probably bring that information about the collective upper limit into this pallet, and choose the smaller of the two numbers when creating a membership group... or just make sure that if you send N members from this pallet to membership, that the top M members are selected or something like this.

We will have to expose MaxMembers through ChangeMembers.
And while looking into this I also found that we need to adjust InitializeMembers.
This is snowballing 🏔️

shawntabrizi · 2020-05-12T10:04:58Z

frame/scored-pool/src/lib.rs

-			ChangeReceiver::MembershipChanged =>
-				T::MembershipChanged::set_members_sorted(
+			ChangeReceiver::MembershipChanged => {
+				// TODO: Is this safe?


This is probably not safe as is because members here are sorted by account id, not score. So if there is truncation happening downstream, it will pick the wrong set of members.

shawntabrizi · 2020-05-12T10:05:39Z

It seems like this PR should also introduce the member limit to collective

bkchr

It does not make any sense to write let _ = for all these lengths. They are dropped automatically.

frame/elections-phragmen/src/lib.rs

frame/membership/src/lib.rs

frame/scored-pool/src/lib.rs

apopiak · 2020-05-12T13:17:29Z

It seems like this PR should also introduce the member limit to collective

I added the member limit.
Do note that the benchmarks are only fixed to compile and run without errors. The actual adjustments are in #5802

apopiak · 2020-05-12T13:24:54Z

It would be great to get some feedback on the API design here.
I'm working around the constraint of not making change_members (and similar) fallible by just returning a Result there.
My current compromise is to return the new length of members after the change (which can indicate a partial update if members were dropped) as well as providing the ensure_can_change_members function that checks execution beforehand.

2 key questions:

Should it be ensure_can_change_members (and returning a result) or can_change_members (and returning a bool)?
Should it return the actual length (as is implemented now) and should it be marked as #[must_use]? (to point people towards the fact that it's not "fire and forget")

shawntabrizi · 2020-05-12T13:43:10Z

@apopiak my take on your questions:

Using result seems fine. There are just as many apis in substrate that returns result versus returns bool it seems. So dont get hung up on this :)
You should not force the user to use your variable. It is perfectly within reason for people to read the docs to understand how the function works and how to use it. In terms of what is returned, you should return the actual list of members. Then if the programmer has any follow up code, they know exactly which members were placed into storage.

Co-authored-by: Bastian Köcher <[email protected]>

kianenigma · 2020-05-13T07:24:32Z

frame/support/src/traits.rs

+	///
+	/// If this returns `Err`, calls to `change_members`, `change_members_sorted` and 
+	/// `set_members_sorted` are not guaranteed to have the desired effect.
+	fn ensure_can_change_members(_new: &[AccountId], _old: &[AccountId]) -> DispatchResult {


I don't have much to say agains the core of the change itself, but enforcing this trait, which by itself has nothing to do with dispatches, to return a DispatchResult is a mistake. While I can guess that it makes the code on the pallets slightly easier, I think it makes a whole lot more sense if this returns normal Result, or anything else which is generic, and then you can do a simple map_err() on your pallet.

I guess we might use the same approach for other traits in support as well.. which is also wrong IMO. Either way, take it with a grain of salt, but I think it is a valid point.

That is a good point, thanks.

frame/support/src/traits.rs

Co-authored-by: Kian Paimani <[email protected]>

apopiak · 2020-05-13T10:00:33Z

This is out of scope for the polkadot release and will be de-prioritized to ship the weights stuff.

…ate into apopiak-change-members

bkchr · 2020-06-04T22:00:11Z

What is the status of this pr?

apopiak · 2020-06-05T15:29:20Z

What is the status of this pr?

It is basically defunct. I split it off because it was out of scope of the PR that spawned it (the collective weights stuff) and figured that we would want to come back to it (the topic of how to adjust ChangeMembers being fallible) at some point.

I'm busy with the Edgeware migration at the moment and won't be working on it, so feel free to close. I think it might be good to keep the branch, but up to how we handle stale branches at Parity (which I don't know).

apopiak added 2 commits May 12, 2020 11:23

add ensure_can_change_members function to ChangeMembers trait and adj…

4aac2f0

…ust usage

adjust comments and return length

3a9fabb

apopiak added the A0-please_review Pull request needs code review. label May 12, 2020

apopiak requested a review from kianenigma as a code owner May 12, 2020 09:39

paritytech-ci added the B2-breaksapi label May 12, 2020

apopiak mentioned this pull request May 12, 2020

Collective weights #5802

Merged

2 tasks

bump runtime spec version

880d4bb

apopiak requested review from bkchr and shawntabrizi May 12, 2020 09:46

shawntabrizi reviewed May 12, 2020

View reviewed changes

frame/elections-phragmen/src/lib.rs Outdated Show resolved Hide resolved

apopiak requested a review from gavofyork May 12, 2020 09:48

shawntabrizi reviewed May 12, 2020

View reviewed changes

frame/support/src/traits.rs Show resolved Hide resolved

shawntabrizi reviewed May 12, 2020

View reviewed changes

bkchr reviewed May 12, 2020

View reviewed changes

frame/elections-phragmen/src/lib.rs Outdated Show resolved Hide resolved

frame/elections-phragmen/src/lib.rs Outdated Show resolved Hide resolved

frame/membership/src/lib.rs Outdated Show resolved Hide resolved

frame/scored-pool/src/lib.rs Outdated Show resolved Hide resolved

apopiak added 2 commits May 12, 2020 15:14

remove todo, usage is safe

787c0ef

add MaxMembers limit to collective

542088e

apopiak and others added 2 commits May 12, 2020 17:26

return the new members from change_members instead of the length

a5fc3ac

remove unnecessary let

b547837

Co-authored-by: Bastian Köcher <[email protected]>

kianenigma reviewed May 13, 2020

View reviewed changes

frame/support/src/traits.rs Outdated Show resolved Hide resolved

Update frame/support/src/traits.rs

eae912d

Co-authored-by: Kian Paimani <[email protected]>

apopiak marked this pull request as draft May 13, 2020 10:00

add some comments on safety, add an ensure check

eed63fe

Merge branch 'apopiak-change-members' of github.com:paritytech/substr…

bfe5153

…ate into apopiak-change-members

apopiak changed the title ~~Make ChangeMembers trait fallible~~ WIP Make ChangeMembers trait fallible May 14, 2020

bkchr closed this Jun 5, 2020

bkchr deleted the apopiak-change-members branch June 5, 2020 20:03

apopiak mentioned this pull request Aug 18, 2020

Companion to #5985 paritytech/polkadot#1094

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

WIP Make ChangeMembers trait fallible #5985

WIP Make ChangeMembers trait fallible #5985

apopiak commented May 12, 2020 •

edited

Loading

parity-cla-bot commented May 12, 2020

shawntabrizi May 12, 2020

apopiak May 12, 2020

shawntabrizi May 12, 2020

shawntabrizi commented May 12, 2020

bkchr left a comment

apopiak commented May 12, 2020

apopiak commented May 12, 2020

shawntabrizi commented May 12, 2020 •

edited

Loading

kianenigma May 13, 2020

apopiak May 13, 2020

apopiak commented May 13, 2020

bkchr commented Jun 4, 2020

apopiak commented Jun 5, 2020

WIP Make ChangeMembers trait fallible #5985

WIP Make ChangeMembers trait fallible #5985

Conversation

apopiak commented May 12, 2020 • edited Loading

parity-cla-bot commented May 12, 2020

shawntabrizi May 12, 2020

Choose a reason for hiding this comment

apopiak May 12, 2020

Choose a reason for hiding this comment

shawntabrizi May 12, 2020

Choose a reason for hiding this comment

shawntabrizi commented May 12, 2020

bkchr left a comment

Choose a reason for hiding this comment

apopiak commented May 12, 2020

apopiak commented May 12, 2020

shawntabrizi commented May 12, 2020 • edited Loading

kianenigma May 13, 2020

Choose a reason for hiding this comment

apopiak May 13, 2020

Choose a reason for hiding this comment

apopiak commented May 13, 2020

bkchr commented Jun 4, 2020

apopiak commented Jun 5, 2020

apopiak commented May 12, 2020 •

edited

Loading

shawntabrizi commented May 12, 2020 •

edited

Loading