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

Generate storage info for pallet authority_discovery #9428

Merged
20 commits merged into from
Aug 30, 2021
Merged

Generate storage info for pallet authority_discovery #9428

20 commits merged into from
Aug 30, 2021

Conversation

georgesdib
Copy link
Contributor

@georgesdib georgesdib commented Jul 24, 2021

Fixes part of paritytech/polkadot-sdk#323

Refactor Authority Discovery to use WeakBoundedVec instead of Vec.

Polkadot companion paritytech/polkadot#3517

@cla-bot-2021
Copy link

cla-bot-2021 bot commented Jul 24, 2021

User @georgesdib, please sign the CLA here.

@shawntabrizi shawntabrizi added B7-runtimenoteworthy C1-low PR touches the given topic and has a low impact on builders. A0-please_review Pull request needs code review. D5-nicetohaveaudit ⚠️ PR contains trivial changes to logic that should be properly reviewed. labels Aug 1, 2021
frame/aura/src/lib.rs Outdated Show resolved Hide resolved
Georges Dib added 2 commits August 1, 2021 18:45
Can happen if vec size is too large, so truncate the vec in that case
@gui1117
Copy link
Contributor

gui1117 commented Aug 20, 2021

aura refactoring is also done in this PR #9371

Maybe this PR can only do authority_discovery

@georgesdib georgesdib changed the title Update Vec to BoundedVec Generate storage info for pallet authority_discovery Aug 22, 2021
@georgesdib
Copy link
Contributor Author

aura refactoring is also done in this PR #9371

Maybe this PR can only do authority_discovery

Removed any code change in pallet aura to reflect that fact.

Copy link
Contributor

@gui1117 gui1117 left a comment

Choose a reason for hiding this comment

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

I think it is better to use WeakBoundedVec:

  • a correctly configured chain should never reach the bound
  • an incorrectly configured chain probably prefer a slight underestimate of PoV with a warning than a truncation of authorities which can have some unexpected result.

frame/authority-discovery/src/lib.rs Outdated Show resolved Hide resolved
frame/authority-discovery/src/lib.rs Outdated Show resolved Hide resolved
More robust implementation following @thiolliere recommendation.
Copy link
Contributor

@gui1117 gui1117 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, thanks

@KiChjang KiChjang added A0-please_review Pull request needs code review. and removed A0-please_review Pull request needs code review. labels Aug 30, 2021
@KiChjang
Copy link
Contributor

bot merge

@ghost
Copy link

ghost commented Aug 30, 2021

Trying merge.

This pull request was closed.
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. 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.

4 participants