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.

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

Add pallet_external_validators, remove ValidatorManager #722

Merged
merged 42 commits into from
Nov 4, 2024

Conversation

tmpolaczyk
Copy link
Collaborator

@tmpolaczyk tmpolaczyk commented Oct 16, 2024

Add pallet_external_validators, remove pallet ValidatorManager.

Contains a migration to populate initial validators and remove all storage from ValidatorManager pallet.

Main storage items:

  • WhitelistedValidators: Fixed validators set by root/governance. Have priority over the external validators.
  • ExternalValidators: Validators set using storage proofs from another blockchain. Can be disabled by setting
    SkipExternalValidators to true.

Validators only change once per era. By default the era changes after a fixed number of sessions (currently 1 era = 6 sessions), but new eras can be forced or disabled using a root extrinsic.

Copy link
Contributor

github-actions bot commented Oct 17, 2024

WASM runtime size check:

Compared to target branch

dancebox runtime: 1412 KB (no changes) ✅

flashbox runtime: 832 KB (no changes) ✅

dancelight runtime: 1988 KB (+1988 KB) ⚠️

container chain template simple runtime: 1096 KB (-4212 KB) ✅

container chain template frontier runtime: 1388 KB (-5136 KB) ✅

Copy link
Contributor

github-actions bot commented Oct 17, 2024

Coverage Report

(master)

@@                      Coverage Diff                       @@
##           master   tomasz-external-validators      +/-   ##
==============================================================
+ Coverage   65.25%                       65.31%   +0.06%     
+ Files         302                          304       +2     
+ Lines       52728                        53181     +453     
==============================================================
+ Hits        34403                        34733     +330     
+ Misses      18325                        18448     +123     
Files Changed Coverage
/pallets/invulnerables/src/lib.rs 87.74% (+1.25%)
/primitives/traits/src/lib.rs 65.57% (+0.08%)
/runtime/common/src/migrations.rs 90.32% (+0.57%)
/runtime/dancebox/src/weights/pallet_treasury.rs 60.29% (+39.70%)
/solo-chains/runtime/dancelight/src/genesis_config_presets.rs 58.95% (+0.59%)
/solo-chains/runtime/dancelight/src/tests/common/mod.rs 96.95% (+0.04%)
/solo-chains/runtime/dancelight/src/tests/migrations_test.rs 97.47% (-2.53%)

Coverage generated Thu Oct 31 17:27:48 UTC 2024

@tmpolaczyk tmpolaczyk added B7-runtimenoteworthy Changes should be noted in any runtime-upgrade release notes breaking Needs to be mentioned in breaking changes D5-nicetohaveaudit⚠️ PR contains trivial changes to logic that should be properly reviewed. labels Oct 17, 2024
@tmpolaczyk tmpolaczyk marked this pull request as ready for review October 17, 2024 11:14
@girazoki
Copy link
Collaborator

girazoki commented Oct 21, 2024

A couple of things we need here:

  • we need to add a certain eraLength concept and increment the era when N number of sessions have elapsed (similar to how polkadot does it in pallet-staking)
  • we need a hook OnEraStart that we can implement other pallets to perform when an era starts
  • similarly, we need a hook called OnEraEnd

Cargo.toml Show resolved Hide resolved
Copy link
Collaborator

@girazoki girazoki left a comment

Choose a reason for hiding this comment

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

Overall the design of the pallet looks good to me, I just have some comments related to things I would change in the pallet plus testing, but overall looks good

Copy link
Collaborator

@girazoki girazoki left a comment

Choose a reason for hiding this comment

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

LGTM

@tmpolaczyk tmpolaczyk merged commit 575f7b9 into master Nov 4, 2024
42 checks passed
@tmpolaczyk tmpolaczyk deleted the tomasz-external-validators branch November 4, 2024 09:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
B7-runtimenoteworthy Changes should be noted in any runtime-upgrade release notes breaking Needs to be mentioned in breaking changes 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