Feat: Add val denylist #1268
Feat: Add val denylist #1268
Conversation
|
Warning Rate Limit Exceeded@tropicaldog has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 20 minutes and 36 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 updates introduce a deny list functionality for validators in an interchain staking system, allowing for better management and governance by enabling the addition or removal of validators from the deny list. This feature supports operations such as setting, retrieving, checking, and removing validators for a specific zone, and includes tests to ensure reliability and correctness. Changes
Assessment against linked issues
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 (
|
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1268 +/- ##
==========================================
+ Coverage 63.26% 63.33% +0.07%
==========================================
Files 170 171 +1
Lines 11269 11314 +45
==========================================
+ Hits 7129 7166 +37
- Misses 3415 3421 +6
- Partials 725 727 +2
Flags with carried forward coverage won't be shown. Click here to find out more.
|
| ) | ||
|
|
||
| // SetZoneValidatorToDenyList sets the zone validator deny list | ||
| func (k *Keeper) SetZoneValidatorToDenyList(ctx sdk.Context, chainID string, validator types.Validator) error { |
There was a problem hiding this comment.
Storing the entire validator would be super inefficient, storage wise. We probably only want to store []string (or maybe []bytes, as addressBytes has length 20, rather than 57-61 bytes for a bech32'd valoper), or perhaps a map[string]bool, which would involve storing more data, but able to look up the validator without iterator when we check whether a given val is on the list.
There was a problem hiding this comment.
I think we can improve this further by just passing in the valoper address string, rather than the validator object.
Similarly, on retrieval we can just return []string - the reason being that all uses of this besides tests don't actually care about the validator object.
Now store validator address bytes instead of serialized form of the Validator struct
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Status
Actionable comments generated: 5
Configuration used: CodeRabbit UI
Files ignored due to path filters (1)
x/interchainstaking/types/query.pb.gois excluded by:!**/*.pb.go
Files selected for processing (9)
- proto/quicksilver/interchainstaking/v1/query.proto (1 hunks)
- x/interchainstaking/keeper/deny_list.go (1 hunks)
- x/interchainstaking/keeper/deny_list_test.go (1 hunks)
- x/interchainstaking/keeper/grpc_query.go (1 hunks)
- x/interchainstaking/keeper/keeper.go (3 hunks)
- x/interchainstaking/types/deny_list.go (1 hunks)
- x/interchainstaking/types/deny_list_test.go (1 hunks)
- x/interchainstaking/types/error.go (1 hunks)
- x/interchainstaking/types/keys.go (2 hunks)
Files skipped from review due to trivial changes (1)
- x/interchainstaking/types/deny_list_test.go
Additional comments: 4
x/interchainstaking/types/error.go (1)
- 9-9: The addition of
ErrValidatorAlreadyInDenyListis appropriate and follows the existing pattern for error definitions. Good job maintaining consistency.x/interchainstaking/keeper/deny_list_test.go (1)
- 1-103: The tests provided for the deny list functionality cover basic operations such as adding, retrieving, and deleting validators. It's good practice to also include tests for error paths and edge cases to ensure the deny list behaves as expected under all conditions.
Consider expanding the test coverage to include scenarios such as attempting to add a validator that already exists in the deny list, or trying to remove a validator that isn't present.
proto/quicksilver/interchainstaking/v1/query.proto (1)
- 258-265: The introduction of
QueryDenyListRequestandQueryDenyListResponsemessages for querying the deny list is well-defined and follows protobuf best practices. These changes are crucial for supporting the new deny list functionality via gRPC queries.x/interchainstaking/keeper/keeper.go (1)
- 7-7: The addition of the
"slices"package is appropriate for the use ofslices.Containslater in the code. This is a standard library package introduced in Go 1.18, which provides generic functions for slice manipulation. Ensure that the Go version used in this project supports generics.
| func MustMarshalValidator(cdc codec.BinaryCodec, validator Validator) []byte { | ||
| return cdc.MustMarshal(&validator) | ||
| } | ||
|
|
||
| func UnmarshalValidator(cdc codec.BinaryCodec, value []byte) (Validator, error) { | ||
| if len(value) == 0 { | ||
| return Validator{}, errors.New("unable to unmarshal zero-length byte slice") | ||
| } | ||
| validator := Validator{} | ||
| err := cdc.Unmarshal(value, &validator) | ||
| return validator, err | ||
| } | ||
|
|
||
| func MustUnmarshalValidator(cdc codec.BinaryCodec, value []byte) Validator { | ||
| validator, err := UnmarshalValidator(cdc, value) | ||
| if err != nil { | ||
| panic(err) | ||
| } | ||
| return validator | ||
| } |
There was a problem hiding this comment.
The functions for marshaling and unmarshaling Validator objects are correctly implemented following Cosmos SDK conventions. However, it's important to ensure comprehensive test coverage for these critical serialization and deserialization operations.
Consider adding unit tests to cover these new functions and ensure they behave as expected under various conditions, including error scenarios.
| func (k *Keeper) SetZoneValidatorToDenyList(ctx sdk.Context, chainID string, validator types.Validator) error { | ||
| store := ctx.KVStore(k.storeKey) | ||
|
|
||
| key := types.GetDeniedValidatorKey(chainID, validator.ValoperAddress) | ||
| addrBytes, err := sdk.ValAddressFromBech32(validator.ValoperAddress) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| store.Set(key, addrBytes.Bytes()) | ||
| return nil | ||
| } | ||
|
|
||
| // GetZoneValidatorDenyList get the validator deny list of a specific zone | ||
| func (k *Keeper) GetZoneValidatorDenyList(ctx sdk.Context, chainID string) (denyList []string) { | ||
| k.IterateZoneDeniedValidator(ctx, chainID, func(validator string) bool { | ||
| denyList = append(denyList, validator) | ||
| return false | ||
| }) | ||
|
|
||
| return denyList | ||
| } | ||
|
|
||
| func (k *Keeper) GetDeniedValidatorInDenyList(ctx sdk.Context, chainID string, validatorAddress string) (types.Validator, bool) { | ||
| key := types.GetDeniedValidatorKey(chainID, validatorAddress) | ||
| store := ctx.KVStore(k.storeKey) | ||
| bz := store.Get(key) | ||
| if bz == nil { | ||
| return types.Validator{}, false | ||
| } | ||
|
|
||
| val, found := k.GetValidator(ctx, chainID, bz) | ||
| if !found { | ||
| return types.Validator{}, false | ||
| } | ||
|
|
||
| return val, true | ||
| } | ||
|
|
||
| // RemoveValidatorFromDenyList removes a validator from the deny list. Panic if the validator is not in the deny list | ||
| func (k *Keeper) RemoveValidatorFromDenyList(ctx sdk.Context, chainID string, validator types.Validator) { | ||
| store := ctx.KVStore(k.storeKey) | ||
| key := types.GetDeniedValidatorKey(chainID, validator.ValoperAddress) | ||
| store.Delete(key) | ||
| } |
There was a problem hiding this comment.
The implementation for managing the deny list, including adding, retrieving, and removing validators, is efficient and addresses previous concerns about storage inefficiency by using sdk.ValAddressFromBech32. However, it's crucial to ensure error paths are also covered by tests to maintain robustness.
Consider adding tests to cover error scenarios, such as invalid validator addresses or issues during the retrieval process.
x/interchainstaking/types/keys.go
Outdated
| func GetZoneValidatorAddrsByConsAddrKey(chainID string) []byte { | ||
| return append(KeyPrefixValidatorAddrsByConsAddr, []byte(chainID)...) | ||
| } | ||
|
|
||
| // GetDeniedValidatorKey gets the validator deny list key prefix for a given chain. | ||
| func GetDeniedValidatorKey(chainID string, validatorAddress string) []byte { | ||
| return append(KeyPrefixDeniedValidator, append([]byte(chainID), []byte(validatorAddress)...)...) | ||
| } | ||
|
|
||
| // GetZoneValidatorDenyListKey gets the validator deny list key prefix for a given chain. | ||
| func GetZoneDeniedValidatorKey(chainID string) []byte { | ||
| return append(KeyPrefixDeniedValidator, []byte(chainID)...) | ||
| } |
There was a problem hiding this comment.
📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [66-178]
The addition of GetDeniedValidatorKey and GetZoneDeniedValidatorKey functions for managing storage keys related to the deny list is correctly implemented, following the established patterns for key management in the module. However, it's important to ensure these key generation functions are covered by tests to verify their correctness and prevent potential issues with key collisions or misinterpretations.
Consider adding tests to cover these new key management functions, ensuring they generate the expected keys under various conditions.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (1)
- x/interchainstaking/keeper/intent_test.go (3 hunks)
Files skipped from review as they are similar to previous changes (1)
- x/interchainstaking/keeper/intent_test.go
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (2)
- x/interchainstaking/keeper/grpc_query_test.go (1 hunks)
- x/interchainstaking/keeper/intent_test.go (2 hunks)
Files skipped from review as they are similar to previous changes (1)
- x/interchainstaking/keeper/intent_test.go
Additional comments: 1
x/interchainstaking/keeper/grpc_query_test.go (1)
- 1350-1403: The test function
TestKeeper_ZoneValidatorDenyListprovides a good foundation for testing the deny list functionality. However, there are a few suggestions for improvement:
- Error Handling for "zone not found": The test case named "zone not found" sets
wantErrtofalse, which might be misleading since the expectation for a zone not found scenario could logically be an error or a specific response indicating the absence of the zone. Consider clarifying the expected behavior in this scenario.- Setup Repetition: The
suite.SetupTest()andsuite.setupTestZones()are called at the beginning of each test case run. If these methods are idempotent and necessary for each test case, this is fine. However, if they are resetting the state and not required to be run before each case, consider moving them to a setup block that runs once before all test cases to improve test efficiency.- Asserting on Specific Error Messages: When expecting errors, it's a good practice to assert on specific error messages or types. This ensures that the test fails for the right reason and not due to some unrelated error.
- Use of
suite.Emptyandsuite.NotNil: While checking thedenyListcontent, usingsuite.Emptyandsuite.NotNilis good. However, for a more robust test, consider also checking the length ofdenyList.Validatorsdirectly whentc.expectedLengthis not 1 to ensure the test is explicitly checking the expected conditions.Consider these suggestions to enhance the test's clarity, robustness, and efficiency.
There was a problem hiding this comment.
Some comments here to make things a little more efficient.
We should create the key from the validator address bytes, not the bech32 string
We should just pass the string around, not the validator itself.
There is. no need to look up the validator - we just care about the address.
| ) | ||
|
|
||
| // SetZoneValidatorToDenyList sets the zone validator deny list | ||
| func (k *Keeper) SetZoneValidatorToDenyList(ctx sdk.Context, chainID string, validator types.Validator) error { |
There was a problem hiding this comment.
I think we can improve this further by just passing in the valoper address string, rather than the validator object.
Similarly, on retrieval we can just return []string - the reason being that all uses of this besides tests don't actually care about the validator object.
…using sdk.ValAddress as argument
There was a problem hiding this comment.
Review Status
Actionable comments generated: 1
Configuration used: CodeRabbit UI
Files selected for processing (4)
- x/interchainstaking/keeper/deny_list.go (1 hunks)
- x/interchainstaking/keeper/deny_list_test.go (1 hunks)
- x/interchainstaking/keeper/grpc_query_test.go (1 hunks)
- x/interchainstaking/types/keys.go (2 hunks)
Files skipped from review as they are similar to previous changes (3)
- x/interchainstaking/keeper/deny_list.go
- x/interchainstaking/keeper/grpc_query_test.go
- x/interchainstaking/types/keys.go
Additional comments: 1
x/interchainstaking/keeper/deny_list_test.go (1)
- 8-101: The test cases for storing, getting, and deleting validators from the deny list are well-structured and cover the essential functionalities. However, consider adding assertions for error conditions and edge cases, such as attempting to delete a validator that is not in the deny list or adding a validator that is already in the deny list. This will ensure the robustness of the deny list functionality under various conditions.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (1)
- x/interchainstaking/keeper/deny_list_test.go (1 hunks)
Files skipped from review as they are similar to previous changes (1)
- x/interchainstaking/keeper/deny_list_test.go
1. Summary
Fixes #1131
2.Type of change
3. Implementation details
4. How to test/use
5. Checklist
6. Limitations (optional)
7. Future Work (optional)
Summary by CodeRabbit