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

bounding staking: BoundedElectionProvider trait #12362

Merged
merged 13 commits into from
Sep 28, 2022

Conversation

Ank4n
Copy link
Contributor

@Ank4n Ank4n commented Sep 27, 2022

This PR is in preparation for bounding the staking pallet by MaxActiveValidators. paritytech/polkadot-sdk#255

This will split up the trait ElectionProvider into bounded and unbounded subtraits. The BoundedElectionProvider does not have any implementation yet. The follow up PR for this would be ElectionProviderMultipPhase implementing BoundedElectionProvider.

@github-actions github-actions bot added the A0-please_review Pull request needs code review. label Sep 27, 2022
@Ank4n Ank4n added B5-clientnoteworthy C1-low PR touches the given topic and has a low impact on builders. D3-trivial 🧸 PR contains trivial changes in a runtime directory that do not require an audit labels Sep 27, 2022
@Ank4n Ank4n added A3-in_progress Pull request is in progress. No review needed at this stage. and removed A0-please_review Pull request needs code review. labels Sep 27, 2022
@Ank4n Ank4n marked this pull request as draft September 27, 2022 10:19
@Ank4n Ank4n marked this pull request as ready for review September 27, 2022 13:11
@github-actions github-actions bot added A0-please_review Pull request needs code review. and removed A3-in_progress Pull request is in progress. No review needed at this stage. labels Sep 27, 2022
@kianenigma kianenigma changed the title Ankan max active validator bound bounding staking: BoundedElectionProvider trait Sep 27, 2022
Comment on lines 396 to 405
/// Elect a new set of winners, without specifying any bounds on the amount
/// of data fetched from [`ElectionProviderBase::DataProvider`].
/// An implementation could nonetheless impose its own custom limits.
///
/// The result is returned in a target major format, namely as *vector of
/// support*.
///
/// This should be implemented as a self-weighing function. The implementor
/// should register its appropriate weight at the end of execution with the
/// system pallet directly.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
/// Elect a new set of winners, without specifying any bounds on the amount
/// of data fetched from [`ElectionProviderBase::DataProvider`].
/// An implementation could nonetheless impose its own custom limits.
///
/// The result is returned in a target major format, namely as *vector of
/// support*.
///
/// This should be implemented as a self-weighing function. The implementor
/// should register its appropriate weight at the end of execution with the
/// system pallet directly.
/// Same a [`BoundedElectionProvider`], but no bounds are imposed on the number of winners.

To avoid duplicate facts.

Also, I would move both of these docs to the trait, and leave the function to have a very small, rather rudimentary document like:

/// Perform the actual election process.
fn elect() -> ..

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

LGTM, some docs could be improved.

@Ank4n Ank4n added B0-silent Changes should not be mentioned in any release notes and removed B5-clientnoteworthy labels Sep 27, 2022
Copy link
Contributor

@ruseinov ruseinov left a comment

Choose a reason for hiding this comment

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

looks broadly good

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.

Seems fine, did not look too closely.

@Ank4n
Copy link
Contributor Author

Ank4n commented Sep 28, 2022

bot merge

@paritytech-processbot paritytech-processbot bot merged commit e7f994d into master Sep 28, 2022
@paritytech-processbot paritytech-processbot bot deleted the ankan-max-active-validator-bound branch September 28, 2022 20:52
ordian added a commit that referenced this pull request Oct 5, 2022
* master: (42 commits)
  Adapt `pallet-contracts` to WeightV2 (#12421)
  Improved election pallet testing (#12327)
  Bump prost to 0.11+ (#12419)
  Use saturating add for alliance::disband witness data (#12418)
  [Fix] Rename VoterBagsList -> VoterList to match pdot (#12416)
  client/beefy: small code improvements (#12414)
  BEEFY: Simplify hashing for pallet-beefy-mmr (#12393)
  Add @koute to `docs/CODEOWNERS` and update stale paths (#12408)
  docs/CODEOWNERS: add @acatangiu as MMR owner (#12406)
  Remove unnecessary Clone trait bounds on CountedStorageMap (#12402)
  Fix `Weight::is_zero` (#12396)
  Beefy on-demand justifications as a custom RequestResponse protocol (#12124)
  Remove contracts RPCs (#12358)
  pallet-mmr: generate historical proofs (#12324)
  unsafe_pruning flag removed (#12385)
  Carry over where clauses defined in Config to Call and Hook (#12388)
  Properly set the max proof size weight on defaults and tests (#12383)
  BEEFY: impl TypeInfo for SignedCommitment (#12382)
  bounding staking: `BoundedElectionProvider` trait (#12362)
  New Pallet: Root offences (#11943)
  ...
ark0f pushed a commit to gear-tech/substrate that referenced this pull request Feb 27, 2023
* add a bounded election provider trait

* extract common trait election provider base

* fmt

* only bound the outer support vector

* fix tests

* docs

* fix rust docs

* fmt

* fix rustdocs

* docs

* improve docs

* small doc change
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. D3-trivial 🧸 PR contains trivial changes in a runtime directory that do not require an audit
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

4 participants