feat: add a query to retrieve validator set that was last sent to the consumer chain #1867
Conversation
github-actions
bot
added
the
C:x/provider
insumity
force-pushed
the
insumity/new-query
branch
2 times, most recently
from
64991d4
to
f7cd09b
Compare
| @@ -103,3 +105,147 @@ func TestQueryOldestUnconfirmedVsc(t *testing.T) { | |||
| } | |||
| require.Equal(t, expectedResult, response.VscSendTimestamp) | |||
| } | |||
|
|
|||
| func TestQueryConsumerChainOptedInValidators(t *testing.T) { | |||
There was a problem hiding this comment.
Although not specific to this PR, I added some basic tests for some of the other queries.
| string chain_id = 1; | ||
| } | ||
|
|
||
| message QueryConsumerChainConsumerValidator { |
There was a problem hiding this comment.
We could potentially have used ConsumerValidator from here instead of creating a new message. However, ConsumerValidator has the provider_address as a bytes so it would not parse in a nice way when showing it as part of the response.
|
Warning Rate Limit Exceeded@insumity has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 16 minutes and 6 seconds before requesting another review. How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. WalkthroughThe update introduces a new query, Changes
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Out of diff range and nitpick comments (1)
docs/docs/validators/partial-set-security-for-validators.md (1)
163-167: Clarify the potential delay of theVSCPacket.The warning about the potential delay of the
VSCPacketis crucial for understanding the temporal discrepancies in validator sets. It might be helpful to provide an example or a more detailed explanation of how such delays can occur and their typical duration or impact.
| ### How to retrieve all the opted-in validators on a consumer chain? | ||
| With the following query: | ||
| ```bash | ||
| interchain-security-pd query provider consumer-opted-in-validators <consumer-chain-id> | ||
| ``` | ||
| we can see all the opted-in validators on `consumer-chain-id` that were manually or automatically opted in. | ||
|
|
||
| ### How to retrieve all the consumer validators on a consumer chain? | ||
| With the following query: | ||
| ```bash | ||
| interchain-security-pd query provider consumer-validators <consumer-chain-id> | ||
| ``` | ||
| we can see all the _consumer validators_ (i.e., validator set) of `consumer-chain-id`. The consumer validators are the | ||
| ones that are currently (or in the future, see warning) validate the consumer chain. A _consumer validator_ is an opted-in | ||
| validator but not vice versa. For example, an opted-in validator `V` might not be a consumer validator because `V` is | ||
| denylisted or because `V` is removed due to a validator-set cap. | ||
|
|
||
| :::warning | ||
| The returned consumer validators from this query do not necessarily correspond to the validator set that is | ||
| validating the consumer chain at this exact moment. This is because the `VSCPacket` sent to a consumer chain might be | ||
| delayed and hence this query might return the validator set that the consumer chain would have at some future | ||
| point in time. | ||
| ::: |
There was a problem hiding this comment.
Clarify the distinction between "consumer validators" and "opted-in validators".
The new section introduces important concepts but could benefit from clearer definitions to avoid confusion. Consider adding a brief explanation or a diagram to visually distinguish between "consumer validators" and "opted-in validators". This will enhance understanding, especially for new users.
| :::warning | ||
| The returned consumer validators from this query do not necessarily correspond to the validator set that is | ||
| validating the consumer chain at this exact moment. This is because the `VSCPacket` sent to a consumer chain might be | ||
| delayed and hence this query might return the validator set that the consumer chain would have at some future |
There was a problem hiding this comment.
Remove redundancy in the phrase.
- at some future point in time.
+ in the future.This simplification removes redundancy without losing any meaning, making the sentence clearer and more concise.
Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| delayed and hence this query might return the validator set that the consumer chain would have at some future | |
| delayed and hence this query might return the validator set that the consumer chain would have in the future |
insumity
force-pushed
the
insumity/new-query
branch
from
beb5b47
to
edef130
Compare
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
| func TestQueryConsumerChainOptedInValidators(t *testing.T) { | ||
| chainID := "chainID" | ||
|
|
||
| pk, ctx, ctrl, _ := testkeeper.GetProviderKeeperAndCtx(t, testkeeper.NewInMemKeeperParams(t)) | ||
| defer ctrl.Finish() | ||
|
|
||
| req := types.QueryConsumerChainOptedInValidatorsRequest{ | ||
| ChainId: chainID, | ||
| } | ||
|
|
||
| // error returned from not yet proposed or not yet registered chain | ||
| _, err := pk.QueryConsumerChainOptedInValidators(ctx, &req) | ||
| require.Error(t, err) | ||
|
|
||
| pk.SetProposedConsumerChain(ctx, chainID, 1) | ||
|
|
||
| providerAddr1 := types.NewProviderConsAddress([]byte("providerAddr1")) | ||
| providerAddr2 := types.NewProviderConsAddress([]byte("providerAddr2")) | ||
| expectedResponse := types.QueryConsumerChainOptedInValidatorsResponse{ | ||
| ValidatorsProviderAddresses: []string{providerAddr1.String(), providerAddr2.String()}, | ||
| } | ||
|
|
||
| pk.SetOptedIn(ctx, chainID, providerAddr1) | ||
| pk.SetOptedIn(ctx, chainID, providerAddr2) | ||
| res, err := pk.QueryConsumerChainOptedInValidators(ctx, &req) | ||
| require.NoError(t, err) | ||
| require.Equal(t, &expectedResponse, res) | ||
| } |
There was a problem hiding this comment.
The tests for QueryConsumerChainOptedInValidators are well-implemented. Consider adding a test case for a scenario where the chain is valid but has no opted-in validators, to ensure the function handles empty validator lists correctly.
Would you like me to help implement this additional test case?
| func TestQueryConsumerValidators(t *testing.T) { | ||
| chainID := "chainID" | ||
|
|
||
| pk, ctx, ctrl, _ := testkeeper.GetProviderKeeperAndCtx(t, testkeeper.NewInMemKeeperParams(t)) | ||
| defer ctrl.Finish() | ||
|
|
||
| req := types.QueryConsumerValidatorsRequest{ | ||
| ChainId: chainID, | ||
| } | ||
|
|
||
| // error returned from not-started chain | ||
| _, err := pk.QueryConsumerValidators(ctx, &req) | ||
| require.Error(t, err) | ||
|
|
||
| providerAddr1 := types.NewProviderConsAddress([]byte("providerAddr1")) | ||
| consumerKey1 := cryptotestutil.NewCryptoIdentityFromIntSeed(1).TMProtoCryptoPublicKey() | ||
| consumerValidator1 := types.ConsumerValidator{ProviderConsAddr: providerAddr1.ToSdkConsAddr(), Power: 1, ConsumerPublicKey: &consumerKey1} | ||
|
|
||
| providerAddr2 := types.NewProviderConsAddress([]byte("providerAddr2")) | ||
| consumerKey2 := cryptotestutil.NewCryptoIdentityFromIntSeed(2).TMProtoCryptoPublicKey() | ||
| consumerValidator2 := types.ConsumerValidator{ProviderConsAddr: providerAddr2.ToSdkConsAddr(), Power: 2, ConsumerPublicKey: &consumerKey2} | ||
|
|
||
| expectedResponse := types.QueryConsumerValidatorsResponse{ | ||
| Validators: []*types.QueryConsumerValidatorsValidator{ | ||
| {providerAddr1.String(), &consumerKey1, 1}, | ||
| {providerAddr2.String(), &consumerKey2, 2}, | ||
| }, | ||
| } | ||
|
|
||
| // set up the client id so the chain looks like it "started" | ||
| pk.SetConsumerClientId(ctx, chainID, "clientID") | ||
| pk.SetConsumerValSet(ctx, chainID, []types.ConsumerValidator{consumerValidator1, consumerValidator2}) | ||
|
|
||
| res, err := pk.QueryConsumerValidators(ctx, &req) | ||
| require.NoError(t, err) | ||
| require.Equal(t, &expectedResponse, res) | ||
| } |
There was a problem hiding this comment.
The tests for QueryConsumerValidators cover error scenarios and a valid request. It's good to see thorough testing of edge cases. Ensure that all possible error types returned by the keeper are tested to improve coverage.
If needed, I can assist in adding these additional error case tests.
… consumer chain (#1867) * init commit * took into account comments * add docs * Update docs/docs/validators/partial-set-security-for-validators.md Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> --------- Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> (cherry picked from commit e1b191a)
… consumer chain (#1867) * init commit * took into account comments * add docs * Update docs/docs/validators/partial-set-security-for-validators.md Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> --------- Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> (cherry picked from commit e1b191a)
… consumer chain (backport #1867) (#1889) feat: add a query to retrieve validator set that was last sent to the consumer chain (#1867) * init commit * took into account comments * add docs * Update docs/docs/validators/partial-set-security-for-validators.md Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> --------- Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> (cherry picked from commit e1b191a) Co-authored-by: insumity <karolos@informal.systems>
… consumer chain (backport #1867) (#1890) feat: add a query to retrieve validator set that was last sent to the consumer chain (#1867) * init commit * took into account comments * add docs * Update docs/docs/validators/partial-set-security-for-validators.md Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> --------- Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> (cherry picked from commit e1b191a) Co-authored-by: insumity <karolos@informal.systems>
Description
Closes: #XXXX
This PR introduces a query to retrieve the validator set that was last sent to the consumer chain.
Note that this does not necessarily mean that the consumer chain is using this validator set because a
VSCPacketcould be delayed, etc.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 the change is state-machine breakingCHANGELOG.mdReviewers 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...
!the type prefix if the change is state-machine breakingSummary by CodeRabbit
New Features
Documentation