-
Notifications
You must be signed in to change notification settings - Fork 716
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
feat!: Bump ICS to include inactive validators #3263
Conversation
* Upgrade ICS version * Change app wiring * Add migration * Fix linting * Add initialization for LastProviderConsensusValSet * Use CamelCase for const names * Remove unnecessary migration and parameter check * Adjust comment to mention LastProviderConsensusValidatorSet initialization * Remove panics and replace with error returns * Improve logging * Lint
This reverts commit 06f58d3.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
// in Interchain Security, after which the number of validators | ||
// participating in consensus on the Cosmos Hub will be governed by the | ||
// MaxProviderConsensusValidators parameter in the provider module. | ||
func SetMaxValidators(ctx sdk.Context, stakingKeeper stakingkeeper.Keeper) error { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this called Set...
while the previous one Initialize...
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Best reviewed by commit.
I looked directly at the files. I left some minor comments but otherwise, LGTM.
Reopening #3259 after it was merged by accident.
Upgrades ICS to current main (which includes inactive validators).
Best reviewed by commit.