fix!: change UX in key assignment #1998
Conversation
github-actions
bot
added
C:Testing
WalkthroughWalkthroughThe changes enhance the key assignment process by preventing validators from reusing the same consumer key. This involves returning an error when a validator attempts to reuse a key, improving security and avoiding duplication issues. Modifications span across error handling in test cases, updating JSON test data, refining key assignment logic, and adjusting test descriptions to reflect the new error scenarios. Changes
Recent review detailsConfiguration used: .coderabbit.yml Files selected for processing (15)
Additional context usedPath-based instructions (6)
Additional comments not posted (23)
Warning Review ran into problemsProblems (1)
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 Configuration File (
|
| @@ -54,7 +54,7 @@ func (k msgServer) AssignConsumerKey(goCtx context.Context, msg *types.MsgAssign | |||
| k.Logger(ctx).Info("assigned consumer key", | |||
| "consumer chainID", msg.ChainId, | |||
| "validator operator addr", msg.ProviderAddr, | |||
| "consumer tm pubkey", consumerTMPublicKey.String(), | |||
| "consumer public key", msg.ConsumerKey, | |||
There was a problem hiding this comment.
Before the public key was logged as something similar to this:
5:16PM INF assigned consumer key consumer chainID=neutron-1 consumer tm pubkey="ed25519:\"\\242\\263!y\\256\\304\\246\\023\\031\\360\\323\\215\\337\\263\\020\\271\\203A\\253\\211hk\\205\\356'\\234\\310W>B7\\213\" " module=x/ibc-provider validator operator addr=cosmosvaloper1jmykcq8gylmy5tgqtel4xj4q62fdt49sl584xd
We fix this so we get the public key logged as:
`{"@type":"/cosmos.crypto.ed25519.PubKey","key":"Ui5Gf1+mtWUdH8u3xlmzdKID+F3PK0sfXZ73GZ6q6is="}
Thanks to @MSalopek for pointing this out!
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Outside diff range and nitpick comments (1)
x/ccv/provider/keeper/msg_server.go (1)
[!TIP]
Codebase VerificationEnsure robust handling of previously assigned consumer keys.
The
AssignConsumerKeyfunction does not currently check if a consumer key has already been assigned. This could lead to key reuse, which is not handled by the current implementation. Please add a check to verify if the consumer key is already in use and handle this scenario appropriately.
- File:
x/ccv/provider/keeper/msg_server.go- Lines: 32-69
Analysis chain
Line range hint
39-64: Ensure robust handling of previously assigned consumer keys.Please verify that the implementation correctly identifies and handles cases where a consumer key is reused. This is crucial for meeting the new requirements stated in the PR.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the handling of previously assigned consumer keys. # Test: Search for the function implementation and check for error handling related to key reuse. ast-grep --lang go --pattern $'func (k msgServer) AssignConsumerKey($_, $_) ($_,$_){ $$$ if $_ { return nil, $_ } $$$ }'Length of output: 133
Script:
#!/bin/bash # Description: Locate and extract the implementation of the AssignConsumerKey function to verify error handling for key reuse. # Locate the file containing the AssignConsumerKey function. file_path=$(fd -IH 'msg_server.go') # Extract the implementation of the AssignConsumerKey function. ast-grep --lang go --pattern 'func (k $_) AssignConsumerKey($_, $_) ($_,$_){ $$$ }' $file_pathLength of output: 2865
insumity
force-pushed
the
insumity/fix-key-assignment
branch
from
a15827c
to
58e6bd2
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Outside diff range, codebase verification and nitpick comments (11)
x/ccv/provider/handler_test.go (1)
Line range hint
131-152: Redundant test case: "fail: consumer key in use by the same validator".The test case "fail: consumer key in use by the same validator" is redundant as it is similar to the test case "fail: consumer key in use by other validator". Consider removing it to avoid redundancy.
- { - name: "fail: consumer key in use by the same validator", - setup: func(ctx sdk.Context, - k keeper.Keeper, mocks testkeeper.MockedKeepers, - ) { - k.SetPendingConsumerAdditionProp(ctx, &providertypes.ConsumerAdditionProposal{ - ChainId: "chainid", - }) - // Use the consumer key already - k.SetValidatorByConsumerAddr(ctx, "chainid", consumerConsAddr, providerConsAddr) - - gomock.InOrder( - mocks.MockStakingKeeper.EXPECT().GetValidator( - ctx, providerCryptoId.SDKValOpAddress(), - ).Return(providerCryptoId.SDKStakingValidator(), nil).Times(1), - mocks.MockStakingKeeper.EXPECT().GetValidatorByConsAddr(ctx, - consumerConsAddr.ToSdkConsAddr(), - ).Return(stakingtypes.Validator{}, stakingtypes.ErrNoValidatorFound), - ) - }, - expError: true, - chainID: "chainid", - },tests/integration/key_assignment.go (4)
124-124: Clarify test case description.The description "double same-key assignment in same block by same val" could be clearer. Consider rephrasing for better readability.
- "double same-key assignment in same block by same val" + "double key assignment in the same block by the same validator"
127-127: Ensure consistent test case descriptions.The description "double key assignment in same block by same val" should be consistent with the previous suggestion for clarity.
- "double key assignment in same block by same val" + "double key assignment in the same block by the same validator"
194-194: Clarify test case description.The description "double same-key assignment in different blocks by same val" could be clearer. Consider rephrasing for better readability.
- "double same-key assignment in different blocks by same val" + "double key assignment in different blocks by the same validator"
197-197: Ensure consistent test case descriptions.The description "double key assignment in different blocks by same val" should be consistent with the previous suggestion for clarity.
- "double key assignment in different blocks by same val" + "double key assignment in different blocks by the same validator"tests/e2e/steps_start_chains.go (4)
89-89: Clarify comment for failure scenario.The comment "op should fail - key already assigned by the same validator" is clear but could be more descriptive.
- // op should fail - key already assigned by the same validator + // Operation should fail because the key has already been assigned by the same validator
95-96: Ensure error message consistency.The
ExpectedErrormessage "a validator has or had assigned this consumer key already" should be consistent with other similar error messages.- ExpectedError: "a validator has or had assigned this consumer key already", + ExpectedError: "a validator has already assigned this consumer key or had assigned it before",
109-109: Clarify comment for failure scenario.The comment "op should fail - key already assigned by another validator" is clear but could be more descriptive.
- // op should fail - key already assigned by another validator + // Operation should fail because the key has already been assigned by another validator
109-109: Ensure error message consistency.The
ExpectedErrormessage "a validator has or had assigned this consumer key already" should be consistent with other similar error messages.- ExpectedError: "a validator has or had assigned this consumer key already", + ExpectedError: "a validator has already assigned this consumer key or had assigned it before",tests/e2e/tracehandler_testdata/consumer-double-sign.json (2)
148-149: Ensure error message consistency.The
ExpectedErrormessage "a validator has or had assigned this consumer key already" should be consistent with other similar error messages.- "ExpectedError": "a validator has or had assigned this consumer key already" + "ExpectedError": "a validator has already assigned this consumer key or had assigned it before"
161-161: Ensure error message consistency.The
ExpectedErrormessage "a validator has or had assigned this consumer key already" should be consistent with other similar error messages.- "ExpectedError": "a validator has or had assigned this consumer key already" + "ExpectedError": "a validator has already assigned this consumer key or had assigned it before"
| name: "fail: consumer key in use by other validator", | ||
| setup: func(ctx sdk.Context, | ||
| k keeper.Keeper, mocks testkeeper.MockedKeepers, | ||
| ) { | ||
| k.SetPendingConsumerAdditionProp(ctx, &providertypes.ConsumerAdditionProposal{ | ||
| ChainId: "chainid", | ||
| }) | ||
|
|
||
| // Use the consumer key already used by some other validator | ||
| k.SetValidatorByConsumerAddr(ctx, "chainid", consumerConsAddr, providerConsAddr2) | ||
|
|
||
| gomock.InOrder( | ||
| mocks.MockStakingKeeper.EXPECT().GetValidator( | ||
| ctx, providerCryptoId.SDKValOpAddress(), | ||
| // validator should not be missing | ||
| ).Return(providerCryptoId.SDKStakingValidator(), nil).Times(1), | ||
| mocks.MockStakingKeeper.EXPECT().GetValidatorByConsAddr(ctx, | ||
| consumerConsAddr.ToSdkConsAddr(), | ||
| // return false - no other validator uses the consumer key to validate *on the provider* | ||
| ).Return(stakingtypes.Validator{}, stakingtypes.ErrNoValidatorFound), | ||
| ) | ||
| }, |
There was a problem hiding this comment.
Duplicate test case: "fail: consumer key in use by other validator".
The test case "fail: consumer key in use by other validator" appears twice. Remove the duplicate to avoid redundancy.
- {
- name: "fail: consumer key in use by other validator",
- setup: func(ctx sdk.Context,
- k keeper.Keeper, mocks testkeeper.MockedKeepers,
- ) {
- k.SetPendingConsumerAdditionProp(ctx, &providertypes.ConsumerAdditionProposal{
- ChainId: "chainid",
- })
- // Use the consumer key already used by some other validator
- k.SetValidatorByConsumerAddr(ctx, "chainid", consumerConsAddr, providerConsAddr2)
-
- gomock.InOrder(
- mocks.MockStakingKeeper.EXPECT().GetValidator(
- ctx, providerCryptoId.SDKValOpAddress(),
- // validator should not be missing
- ).Return(providerCryptoId.SDKStakingValidator(), nil).Times(1),
- mocks.MockStakingKeeper.EXPECT().GetValidatorByConsAddr(ctx,
- consumerConsAddr.ToSdkConsAddr(),
- // return false - no other validator uses the consumer key to validate *on the provider*
- ).Return(stakingtypes.Validator{}, stakingtypes.ErrNoValidatorFound),
- )
- },
- expError: true,
- chainID: "chainid",
- },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. Thoroughly test & benchmark the code to ensure it meets the requirements.
| name: "fail: consumer key in use by other validator", | |
| setup: func(ctx sdk.Context, | |
| k keeper.Keeper, mocks testkeeper.MockedKeepers, | |
| ) { | |
| k.SetPendingConsumerAdditionProp(ctx, &providertypes.ConsumerAdditionProposal{ | |
| ChainId: "chainid", | |
| }) | |
| // Use the consumer key already used by some other validator | |
| k.SetValidatorByConsumerAddr(ctx, "chainid", consumerConsAddr, providerConsAddr2) | |
| gomock.InOrder( | |
| mocks.MockStakingKeeper.EXPECT().GetValidator( | |
| ctx, providerCryptoId.SDKValOpAddress(), | |
| // validator should not be missing | |
| ).Return(providerCryptoId.SDKStakingValidator(), nil).Times(1), | |
| mocks.MockStakingKeeper.EXPECT().GetValidatorByConsAddr(ctx, | |
| consumerConsAddr.ToSdkConsAddr(), | |
| // return false - no other validator uses the consumer key to validate *on the provider* | |
| ).Return(stakingtypes.Validator{}, stakingtypes.ErrNoValidatorFound), | |
| ) | |
| }, |
fix!: change UX in key assignment (#1998) * init * added nit changes * added CHANGELOGs * fixed E2E tests error message * brought one more test from #1732 * fixed logging * fixed event key * fixed CHANGELOGs * rebase (cherry picked from commit fde751e) Co-authored-by: insumity <[email protected]>
* init * added nit changes * added CHANGELOGs * fixed E2E tests error message * brought one more test from #1732 * fixed logging * fixed event key * fixed CHANGELOGs * rebase
Description
Modifies the user experience (UX) of key assignment by reverting #1732. Note that this is not a full revert of #1732 because #1732 added tests that made sense to keep here as well.
Before, when a validator tried to re-use a previously assigned consumer key that they used, we would return success and the assignment would be a no-op. With this PR, we return an error instead.
The reason for this change was that we had a validator re-assigning an old consumer key they used but because the assignment was a no-op, the old consumer key was not set. As a result, the validator was confused on why the assignment was successful but the consumer key on the consumer chain remained unchanged.
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
Bug Fixes
Tests
Documentation