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

Max class voters for ranked collective vote tally #13313

Merged
merged 7 commits into from
May 17, 2023

Conversation

muharem
Copy link
Contributor

@muharem muharem commented Feb 5, 2023

The implementation of VoteTally trait, uses Rank type where Class is supposed.
For Kusama runtime, this is not an issue. The Rank and Class is the same u16 type and mapped 1:1.

But if,

  • the mapping is not 1:1, the max voters count might be wrong;
  • Class is any other type (e.g. enum), it wont compile.

// DONE tests

@muharem muharem added A0-please_review Pull request needs code review. B0-silent Changes should not be mentioned in any release notes D3-trivial 🧸 PR contains trivial changes in a runtime directory that do not require an audit labels Feb 5, 2023
@muharem muharem added the C1-low PR touches the given topic and has a low impact on builders. label Feb 5, 2023
@muharem muharem requested review from gavofyork, bkchr and ggwpez February 6, 2023 05:32
@muharem muharem requested a review from shawntabrizi February 21, 2023 02:40
Copy link
Member

@gavofyork gavofyork left a comment

Choose a reason for hiding this comment

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

Looks ok in general, but are Rank and Class not already defined as equivalent within ranked-collective's VoteTally impl because its Class generic parameter is provided as Rank?

@muharem
Copy link
Contributor Author

muharem commented Feb 25, 2023

Looks ok in general, but are Rank and Class not already defined as equivalent within ranked-collective's VoteTally impl because its Class generic parameter is provided as Rank?

I see it now.
It not simple to read it like that. Here for example we access Class type throughout Polling trait, signaling its generic for the pallet -

type MinRankOfClass: Convert<<Self::Polls as Polling<TallyOf<Self, I>>>::Class, Rank>;

The signatures of the VoteTally funcs impls becomes confusing to me, the name of the arg is class, the type is Rank, but docs will help -
fn rejection(class: Rank) -> Self {

I faced the issue, while trying to define the Class as enum.
We can go either way, generic class (like in the PR now) or class type is rank type, but I make it more explicit and add some docs.

@muharem
Copy link
Contributor Author

muharem commented Mar 29, 2023

bot rebase

@paritytech-processbot
Copy link

Rebased

@muharem muharem requested a review from gavofyork April 18, 2023 17:23
@muharem muharem requested review from a team and removed request for shawntabrizi May 16, 2023 08:47
@juangirini
Copy link
Contributor

bot rebase

@paritytech-processbot
Copy link

Rebased

@muharem muharem added D5-nicetohaveaudit ⚠️ PR contains trivial changes to logic that should be properly reviewed. and removed D3-trivial 🧸 PR contains trivial changes in a runtime directory that do not require an audit labels May 16, 2023
Copy link
Member

@ggwpez ggwpez left a comment

Choose a reason for hiding this comment

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

Just one comment.

Copy link
Member

@ggwpez ggwpez left a comment

Choose a reason for hiding this comment

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

Just one comment.

@muharem
Copy link
Contributor Author

muharem commented May 17, 2023

bot merge

@paritytech-processbot
Copy link

Waiting for commit status.

@paritytech-processbot paritytech-processbot bot merged commit 43a130c into master May 17, 2023
@paritytech-processbot paritytech-processbot bot deleted the muharem-referenda-max-voters branch May 17, 2023 10:58
gpestana pushed a commit that referenced this pull request May 27, 2023
* max class voters for vote tally

* fix move

* tests

* rename to GetMaxVoters

* saturating sub

---------

Co-authored-by: parity-processbot <>
nathanwhit pushed a commit to nathanwhit/substrate that referenced this pull request Jul 19, 2023
* max class voters for vote tally

* fix move

* tests

* rename to GetMaxVoters

* saturating sub

---------

Co-authored-by: parity-processbot <>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
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. D5-nicetohaveaudit ⚠️ PR contains trivial changes to logic that should be properly reviewed.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants