feat!: Bump ICS to include inactive validators #3259
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
p-offtermatt
requested review from
zmanian,
crodriguezvega,
jackzampolin and
a team
as code owners
8 tasks
p-offtermatt
commented
Aug 7, 2024
| appKeepers.ProviderModule = icsprovider.NewAppModule(&appKeepers.ProviderKeeper, appKeepers.GetSubspace(providertypes.ModuleName)) | ||
| // gov depends on provider, so needs to be set after | ||
| govConfig := govtypes.DefaultConfig() | ||
| // set the MaxMetadataLen for proposals to the same value as it was pre-sdk v0.47.x |
There was a problem hiding this comment.
@insumity notice that this is already present before my changes
just in general, this is there to just get the same behaviour in terms of metadata length that Gaia has always had; it's just that in 47, something changed that makes it necessary to specify this now
p-offtermatt
commented
Aug 7, 2024
app/upgrades/v20/upgrades.go
Outdated
| func InitializeMaxValidatorsForExistingConsumers(ctx sdk.Context, providerKeeper providerkeeper.Keeper) { | ||
| maxVals := providerKeeper.GetParams(ctx).MaxProviderConsensusValidators | ||
| for _, chainID := range providerKeeper.GetAllRegisteredConsumerChainIDs(ctx) { | ||
| providerKeeper.SetValidatorSetCap(ctx, chainID, uint32(maxVals)) |
There was a problem hiding this comment.
proposed chains would not have their validator cap set to 180.
but all chains we have on the horizon are opt-in chains that specify a validator set cap, anyways.
if this is not ultimately the case, then we need to adjust this.
p-offtermatt
commented
Aug 7, 2024
| @@ -28,7 +28,7 @@ require ( | |||
| github.com/cosmos/ibc-apps/modules/rate-limiting/v8 v8.0.0 | |||
| github.com/cosmos/ibc-go/modules/capability v1.0.0 | |||
| github.com/cosmos/ibc-go/v8 v8.4.0 | |||
| github.com/cosmos/interchain-security/v5 v5.1.1 | |||
| github.com/cosmos/interchain-security/v5 v5.0.0-20240806104629-29327696b8e6 | |||
There was a problem hiding this comment.
This looks like a regression, but this is just temporary and stems from upgrading to a commit on main instead of teh v5.1 branch
This will be updated when we move to the actual ICS release
p-offtermatt
commented
Aug 7, 2024
app/upgrades/v20/upgrades.go
Outdated
|
|
||
| // InitializeLastProviderConsensusValidatorSet initializes the last provider consensus validator set | ||
| // by setting it to the first 180 validators from the current validator set of the staking module. | ||
| func InitializeLastProviderConsensusValidatorSet(ctx sdk.Context, providerKeeper providerkeeper.Keeper, stakingKeeper stakingkeeper.Keeper) { |
There was a problem hiding this comment.
Thanks @insumity, yes the LastProviderConsensusValSet should be initialized here. Nice catch
mpoke
reviewed
Aug 7, 2024
mpoke
approved these changes
Aug 7, 2024
insumity
reviewed
Aug 8, 2024
insumity
approved these changes
Aug 8, 2024
p-offtermatt
added a commit
that referenced
this pull request
Aug 9, 2024
This reverts commit 06f58d3.
p-offtermatt
added a commit
that referenced
this pull request
Aug 12, 2024
This reverts commit 06f58d3.
mpoke
added a commit
that referenced
this pull request
Aug 14, 2024
* replace gaia/v19 with gaia/v20 * add v20 upgrade handler * update old gaia in upgrade-test * Upgrade ICS version * Change app wiring * Add migration * Fix linting * Revert changes * fix linter * feat!: Bump ICS to include inactive validators (#3259) * 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 * Revert "feat!: Bump ICS to include inactive validators (#3259)" This reverts commit 06f58d3. * tests: update config cmd usage in upgrade test --------- Co-authored-by: Philip Offtermatt <p.offtermatt@gmail.com> Co-authored-by: Philip Offtermatt <57488781+p-offtermatt@users.noreply.github.com> Co-authored-by: MSalopek <matija.salopek994@gmail.com>
mpoke
added a commit
that referenced
this pull request
Aug 14, 2024
* replace gaia/v19 with gaia/v20 * add v20 upgrade handler * update old gaia in upgrade-test * Upgrade ICS version * Change app wiring * Add migration * Fix linting * Revert changes * Upgrade ICS version * Change app wiring * Add migration * Fix linting * Add initialization for LastProviderConsensusValSet * Use CamelCase for const names * fix linter * Remove unnecessary migration and parameter check * Adjust comment to mention LastProviderConsensusValidatorSet initialization * Remove panics and replace with error returns * Improve logging * Lint * feat!: Bump ICS to include inactive validators (#3259) * 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 * Revert "feat!: Bump ICS to include inactive validators (#3259)" This reverts commit 06f58d3. * add changelog entries * Update app/upgrades/v20/upgrades.go --------- Co-authored-by: mpoke <marius.poke@posteo.de>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
Closes: #XXXX
Upgrades ICS to current main (which includes inactive validators).
Best reviewed by commit.
Author Checklist
All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.
I have...
!to the type prefix if API, client, or state breaking change (i.e., requires minor or major version bump).changelog(for details, see contributing guidelines)Reviewers Checklist
All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items.
I have...
!in the type prefix if API or client breaking change