-
Notifications
You must be signed in to change notification settings - Fork 119
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: Add support for priority validators #2101
base: main
Are you sure you want to change the base?
Conversation
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.
This looks great so far. I left a few comments
.changelog/unreleased/api-breaking/provider/xxxx-priority-validators.md
Outdated
Show resolved
Hide resolved
.changelog/unreleased/features/provider/xxxx-priority-validators.md
Outdated
Show resolved
Hide resolved
@@ -218,6 +218,12 @@ func MigrateLaunchedConsumerChains(ctx sdk.Context, store storetypes.KVStore, pk | |||
return err | |||
} | |||
|
|||
// chainId -> Prioritylist | |||
err = rekeyChainIdAndConsAddrKey(store, providertypes.PrioritylistKeyPrefix(), chainId, consumerId) |
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.
No need to add this here I think, since this is for a migration that will happen before we have the priority vals feature
@mpoke could you double-check that that's true? I'm not very on top of the ICS release schedule atm
…rs.md Co-authored-by: Philip Offtermatt <[email protected]>
Co-authored-by: Philip Offtermatt <[email protected]>
Co-authored-by: Philip Offtermatt <[email protected]>
Co-authored-by: Philip Offtermatt <[email protected]>
…dators.md Co-authored-by: Philip Offtermatt <[email protected]>
…terchain-security into ph/priority-validators
@@ -0,0 +1,2 @@ | |||
- Allow consumer chains to specify a list of priority validators that are included in the validator set before other validators are considered |
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.
Rename the file (here and below) so they do not have xxx
. Probably add a verb there as well to make it clear from the name what this does (e.g., introduce-priority...
).
@@ -1840,6 +1840,190 @@ func stepsValidatorsDenylistedChain() []Step { | |||
return s | |||
} | |||
|
|||
// stepsValidatorsAllowlistedChain starts a provider chain and an Opt-In chain with an prioritylist |
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.
Comment name does not match function name.
ChainID("consu"): ChainState{ | ||
ValPowers: &map[ValidatorID]uint{ | ||
ValidatorID("alice"): 100, | ||
ValidatorID("bob"): 200, |
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.
Above it's written:
// Bob is not in the validator set due to ValidatorSetCap and priority list
so why is "bob"
validating here?
"denylist": [], | ||
"min_stake": "0", | ||
"allow_inactive_vals": false, | ||
"prioritylist": [] |
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.
Fix indentation here and below.
@@ -89,7 +89,7 @@ func (k Keeper) UpdateMinimumPowerInTopN(ctx sdk.Context, consumerId string, old | |||
|
|||
// CapValidatorSet caps the provided `validators` if chain with `consumerId` is an Opt In chain with a validator-set cap. | |||
// If cap is `k`, `CapValidatorSet` returns the first `k` validators from `validators` with the highest power. | |||
func (k Keeper) CapValidatorSet( | |||
func (k Keeper) CapValidatorSet2( |
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.
Use a better name to express what this does. Also, the function name does not match comment.
Description
Closes: #2030
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...
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...