add pagination for stakerinfos query, format #257
Conversation
WalkthroughThe pull request introduces various modifications across multiple files, primarily focusing on formatting improvements and the addition of new functionalities. Key changes include the introduction of pagination features in querying staker information, updates to the Makefile for Go and Protobuf file formatting, and the addition of new message types in several proto files. Overall, the changes enhance code readability and maintainability while ensuring that existing functionalities remain intact. Changes
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (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 using PR comments)
Other keywords and placeholders
CodeRabbit Configuration 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: 0
🧹 Outside diff range and nitpick comments (2)
x/oracle/keeper/native_token.go (1)
81-81: Rename unused parameter 'key' to '_' to suppress warningThe parameter
keyin thequery.Paginateanonymous function is unused, which may cause warnings from static analysis tools. Renaming it to_indicates that it is intentionally unused.Apply this diff to address the issue:
-resPage, err := query.Paginate(store, req.Pagination, func(key []byte, value []byte) error { +resPage, err := query.Paginate(store, req.Pagination, func(_ []byte, value []byte) error {🧰 Tools
🪛 golangci-lint (1.62.2)
[warning] 81-81: unused-parameter: parameter 'key' seems to be unused, consider removing or renaming it as _
(revive)
🪛 GitHub Check: Run golangci-lint
[warning] 81-81:
unused-parameter: parameter 'key' seems to be unused, consider removing or renaming it as _ (revive)proto/exocore/avs/v1/query.proto (1)
Line range hint
74-97: Update service method comments for consistencyThe Query service implementation looks good, but there are some inconsistencies in the method comments:
- Multiple methods use the same generic comment "Parameters queries the parameters of the module"
- Some method comments don't match their functionality
Consider updating the comments for better clarity:
service Query { - // Parameters queries the parameters of the module. + // QueryAVSInfo retrieves information about a specific AVS. rpc QueryAVSInfo(QueryAVSInfoReq) returns (QueryAVSInfoResponse) { option (google.api.http).get = "/exocore/avs/QueryAVSInfo"; } // TaskInfo queries the task information. rpc QueryAVSTaskInfo(QueryAVSTaskInfoReq) returns (TaskInfo) { option (google.api.http).get = "/exocore/avstask/v1/GetAVSTaskInfoReq"; } // QueryAVSAddrByChainID queries the avs address by chain id rpc QueryAVSAddrByChainID(QueryAVSAddrByChainIDReq) returns (QueryAVSAddrByChainIDResponse) { option (google.api.http).get = "/exocore/avs/QueryAVSAddrByChainID"; } - // Parameters queries the parameters of the module. + // QuerySubmitTaskResult retrieves the result of a submitted task. rpc QuerySubmitTaskResult(QuerySubmitTaskResultReq) returns (QuerySubmitTaskResultResponse) { option (google.api.http).get = "/exocore/avs/QuerySubmitTaskResult"; } - // Parameters queries the parameters of the module. + // QueryChallengeInfo retrieves information about a specific challenge. rpc QueryChallengeInfo(QueryChallengeInfoReq) returns (QueryChallengeInfoResponse) { option (google.api.http).get = "/exocore/avs/QueryChallengeInfo"; } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (2)
x/oracle/types/query.pb.gois excluded by!**/*.pb.gox/oracle/types/query.pb.gw.gois excluded by!**/*.pb.gw.go
📒 Files selected for processing (39)
Makefile(2 hunks)proto/evmos/erc20/v1/erc20.proto(1 hunks)proto/evmos/incentives/v1/incentives.proto(4 hunks)proto/exocore/appchain/common/v1/common.proto(4 hunks)proto/exocore/appchain/coordinator/v1/coordinator.proto(1 hunks)proto/exocore/appchain/coordinator/v1/params.proto(1 hunks)proto/exocore/appchain/coordinator/v1/query.proto(2 hunks)proto/exocore/appchain/coordinator/v1/tx.proto(2 hunks)proto/exocore/appchain/subscriber/v1/query.proto(2 hunks)proto/exocore/appchain/subscriber/v1/tx.proto(1 hunks)proto/exocore/assets/v1/genesis.proto(2 hunks)proto/exocore/assets/v1/tx.proto(9 hunks)proto/exocore/avs/v1/genesis.proto(2 hunks)proto/exocore/avs/v1/query.proto(2 hunks)proto/exocore/avs/v1/tx.proto(8 hunks)proto/exocore/delegation/v1/genesis.proto(3 hunks)proto/exocore/delegation/v1/query.proto(1 hunks)proto/exocore/dogfood/v1/params.proto(1 hunks)proto/exocore/dogfood/v1/query.proto(2 hunks)proto/exocore/dogfood/v1/tx.proto(1 hunks)proto/exocore/epochs/v1/epochs.proto(1 hunks)proto/exocore/feedistribution/v1/distribution.proto(1 hunks)proto/exocore/feedistribution/v1/genesis.proto(4 hunks)proto/exocore/feedistribution/v1/tx.proto(1 hunks)proto/exocore/operator/v1/genesis.proto(4 hunks)proto/exocore/operator/v1/query.proto(7 hunks)proto/exocore/operator/v1/tx.proto(6 hunks)proto/exocore/operator/v1/validator.proto(4 hunks)proto/exocore/oracle/v1/genesis.proto(1 hunks)proto/exocore/oracle/v1/native_token.proto(1 hunks)proto/exocore/oracle/v1/params.proto(2 hunks)proto/exocore/oracle/v1/query.proto(3 hunks)x/avs/types/genesis_test.go(6 hunks)x/oracle/client/cli/query_native_token.go(1 hunks)x/oracle/keeper/msg_server_update_params_test.go(1 hunks)x/oracle/keeper/native_token.go(3 hunks)x/oracle/keeper/query_native_token.go(1 hunks)x/oracle/types/errors.go(2 hunks)x/oracle/types/types.go(1 hunks)
✅ Files skipped from review due to trivial changes (26)
- proto/exocore/appchain/subscriber/v1/tx.proto
- proto/exocore/dogfood/v1/tx.proto
- proto/exocore/epochs/v1/epochs.proto
- proto/exocore/appchain/coordinator/v1/coordinator.proto
- x/oracle/types/types.go
- x/avs/types/genesis_test.go
- proto/exocore/feedistribution/v1/distribution.proto
- proto/exocore/appchain/subscriber/v1/query.proto
- proto/exocore/delegation/v1/genesis.proto
- proto/exocore/oracle/v1/native_token.proto
- proto/evmos/erc20/v1/erc20.proto
- proto/exocore/oracle/v1/params.proto
- proto/exocore/feedistribution/v1/tx.proto
- proto/exocore/delegation/v1/query.proto
- proto/exocore/oracle/v1/genesis.proto
- proto/exocore/appchain/coordinator/v1/tx.proto
- proto/exocore/operator/v1/validator.proto
- proto/exocore/assets/v1/genesis.proto
- proto/exocore/appchain/coordinator/v1/query.proto
- proto/evmos/incentives/v1/incentives.proto
- proto/exocore/appchain/coordinator/v1/params.proto
- proto/exocore/dogfood/v1/query.proto
- proto/exocore/appchain/common/v1/common.proto
- proto/exocore/operator/v1/genesis.proto
- proto/exocore/operator/v1/tx.proto
- proto/exocore/assets/v1/tx.proto
🧰 Additional context used
🪛 golangci-lint (1.62.2)
x/oracle/keeper/native_token.go
[warning] 81-81: unused-parameter: parameter 'key' seems to be unused, consider removing or renaming it as _
(revive)
🪛 GitHub Check: Run golangci-lint
x/oracle/keeper/native_token.go
[warning] 81-81:
unused-parameter: parameter 'key' seems to be unused, consider removing or renaming it as _ (revive)
🔇 Additional comments (20)
x/oracle/types/errors.go (1)
31-31: Addition of new error 'ErrInvalidPagination' for improved error handling
The new error ErrInvalidPagination enhances error reporting by providing a specific message when pagination parameters are invalid.
proto/exocore/dogfood/v1/params.proto (1)
Line range hint 30-34: Correctly specify field options for 'min_self_delegation'
The min_self_delegation field now includes appropriate options to define its scalar type, custom Go type, and nullability. This ensures proper serialization and type safety.
x/oracle/keeper/query_native_token.go (1)
27-27: Simplify 'StakerInfos' method by returning result directly
The StakerInfos method now directly returns the result of k.GetStakerInfos(ctx, req), simplifying the code by eliminating unnecessary variables.
proto/exocore/avs/v1/genesis.proto (2)
Line range hint 46-54: Addition of 'ChallengeInfo' message to store task challenge information
The new ChallengeInfo message is properly defined and enhances the genesis state by including task challenge data.
55-62: Addition of 'ChainIDInfo' message to store AVS chain ID information
The new ChainIDInfo message is correctly added, allowing the genesis state to include chain ID details for AVS.
x/oracle/client/cli/query_native_token.go (2)
29-35: LGTM: Pagination implementation follows best practices
The pagination implementation correctly follows Cosmos SDK patterns with proper error handling for the page limit.
38-39: LGTM: Request structure properly updated
The QueryStakerInfosRequest is correctly updated to include the pagination parameter.
proto/exocore/feedistribution/v1/genesis.proto (3)
Line range hint 47-57: LGTM: ValidatorCurrentRewardsList well structured
The message type is well-documented and follows proper protobuf conventions.
Line range hint 58-68: LGTM: ValidatorOutstandingRewardsList properly defined
The message structure maintains consistency with other validator-related messages.
Line range hint 69-78: LGTM: StakerOutstandingRewardsList follows conventions
The message type maintains consistency with the validator reward structures while appropriately handling staker-specific fields.
x/oracle/keeper/msg_server_update_params_test.go (1)
19-19: LGTM: Variable declaration improvement
The change from a global variable to a short declaration is a good improvement that maintains the same functionality while making the code more concise.
proto/exocore/oracle/v1/query.proto (1)
145-146: LGTM: Well-implemented pagination support
The addition of pagination fields follows the standard cosmos-sdk pattern for paginated queries, which is essential for handling large sets of staker information efficiently.
Let's verify that the pagination implementation is consistent across the codebase:
Also applies to: 152-154
proto/exocore/avs/v1/tx.proto (3)
46-54: LGTM: Improved field formatting
The formatting changes for avs_reward and avs_slash fields improve readability while maintaining the same functionality.
Line range hint 149-157: LGTM: Clear BLS public key structure
The BlsPubKeyInfo message is well-defined with appropriate fields for operator BLS public key management.
Let's verify the BLS key handling implementation:
✅ Verification successful
Based on the search results, I can see that the BLS public key handling is well-implemented in the codebase:
-
The
BlsPubKeyInfomessage is properly used in the keeper layer with validation:SetOperatorPubKeyvalidates the operator address and stores the keyGetOperatorPubKeysafely retrieves and unmarshals the keyGetAllBlsPubKeysprovides iteration capability with proper error handling
-
The implementation shows proper handling of BLS keys in tests:
- Keys are properly marshaled before storage
- Operator addresses are validated
- Error cases are handled
LGTM: BLS public key implementation is robust and well-structured
The codebase demonstrates proper validation, storage and retrieval of BLS public keys with appropriate error handling and test coverage.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check BLS key handling implementation
# Search for BLS key validation logic
rg -A 5 'BlsPubKeyInfo|pub_key'
# Check for proper key validation
ast-grep --pattern 'func $_(pubKey $types.BlsPubKeyInfo) error {
$$$
}'
Length of output: 52809
Line range hint 128-146: LGTM: Well-structured power tracking messages
The new OperatorActivePowerList and OperatorActivePowerInfo messages are well-designed for tracking operator power in tasks. Proper use of cosmos.Dec type for power values ensures accurate decimal handling.
Let's verify the integration of these new types:
✅ Verification successful
Let me gather more information about the usage of these types in the codebase.
Let me check the implementation details in the files where these types are used.
Power tracking messages are properly integrated and used in the codebase
The verification shows that:
- The types are correctly used in the keeper implementation (
impl_epoch_hook.go) for tracking operator power in tasks - Proper integration with cosmos-sdk's Dec type for accurate decimal handling is confirmed
- The implementation includes proper power calculations and error handling
- Generated protobuf code includes all necessary methods for marshaling/unmarshaling and size calculations
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify proper usage of new power tracking messages
# Check for handler implementations using these types
ast-grep --pattern 'func $_(ctx $_, $power *types.OperatorActivePowerList) {
$$$
}'
# Check for proper decimal handling
rg -A 3 'active_power.*Dec'
Length of output: 482
Script:
#!/bin/bash
# Search for imports and usages of OperatorActivePowerList
rg -l "OperatorActivePowerList"
# Search for any methods that work with these power tracking messages
ast-grep --pattern 'func $_ ($_ *OperatorActivePowerInfo) $_ {
$$$
}'
# Look for test files that validate these types
fd -e go -e proto test | xargs rg "OperatorActivePower"
Length of output: 244
Script:
#!/bin/bash
# Check the implementation in impl_epoch_hook.go
rg -A 10 "OperatorActivePowerList" x/avs/keeper/impl_epoch_hook.go
# Check for any methods that manipulate these power values
ast-grep --pattern 'func $_ ($_ $_, powerList *OperatorActivePowerList) $_ {
$$$
}'
# Look for any validation or conversion of power values
rg -A 5 "SelfActivePower" x/avs/types/tx.pb.go
Length of output: 1846
proto/exocore/operator/v1/query.proto (3)
Line range hint 369-379: LGTM: Proper implementation of pagination for validator queries
The QueryValidatorsRequest message correctly implements pagination using the standard cosmos pagination pattern with cosmos.base.query.v1beta1.PageRequest.
381-389: LGTM: Well-structured validator response message
The QueryValidatorsResponse message properly:
- Returns a list of validators with appropriate protobuf options
- Includes pagination response for client-side handling
281-337: LGTM: Consistent REST endpoint patterns
The RPC endpoint definitions follow consistent patterns:
- Resource-based paths (e.g.,
/operator/v1/...) - Clear parameter naming in URLs
- Proper use of HTTP GET methods
Makefile (2)
382-385: LGTM: Well-structured golang formatting target
The golang-format target correctly:
- Excludes vendor, git, and generated files
- Uses gofumpt for modern Go formatting
416-416: LGTM: Improved proto formatting using buf
Switching to buf format is a good improvement as it:
- Is the modern standard for proto formatting
- Provides more consistent results across different environments
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (2)
x/oracle/keeper/query_native_token_test.go (2)
18-23: Consider adding test cases for edge cases in the setup.The test setup creates 5 staker infos, but it would be beneficial to also test edge cases:
- Empty staker list
- Single staker
- Maximum page size
77-97: Consider enhancing test data variety.The test data generator creates very similar staker infos with predictable values. Consider adding more variety:
- Different balance amounts
- Multiple balance history entries
- Various validator pubkey combinations
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
x/oracle/keeper/native_token.go(3 hunks)x/oracle/keeper/query_native_token_test.go(1 hunks)
🔇 Additional comments (1)
x/oracle/keeper/native_token.go (1)
252-252: Verify the Append method's behavior for concurrent access.
The Append method is used to modify the staker info's balance list. Ensure this operation is safe for concurrent access.
✅ Verification successful
The Append operation is safe for concurrent access in this context
The Append method is used within the Cosmos SDK keeper pattern where state access is inherently safe from concurrent modifications because:
- The method is only called through keeper methods that receive a context (
sdk.Context) - All keeper operations are serialized through the ABCI application pipeline
- The implementation shows it's a simple slice operation that's only accessed through the keeper's state management methods
- No goroutines are used in the keeper implementation that would access this method concurrently
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for concurrent access patterns to the Append method
ast-grep --pattern 'func $_ {
$$$
stakerInfo.Append($_)
$$$
}'
Length of output: 70
Script:
#!/bin/bash
# Let's first find the definition of stakerInfo and Append method
rg -l "type.*StakerInfo"
# Then search for any concurrent access patterns or mutex usage
rg "sync\.Mutex|sync\.RWMutex"
# Also look for the implementation of Append method
ast-grep --pattern 'func ($_ *StakerInfo) Append($_) {
$$$
}'
Length of output: 1001
Script:
#!/bin/bash
# Let's check if this is used in any concurrent context by looking at the keeper implementation
rg -A 5 -B 5 "go "
# Also check if this is part of any concurrent operations in tests
rg -A 5 -B 5 "parallel|Parallel"
# Let's also see the keeper struct to understand if it's used in concurrent operations
ast-grep --pattern 'type Keeper struct {
$$$
}'
Length of output: 65946
Script:
#!/bin/bash
# Let's look at the StakerInfo struct and its usage
rg -A 5 "type StakerInfo struct"
# Check if there are any mutex protections in the native_token.go file
rg -A 5 -B 5 "sync\." x/oracle/keeper/native_token.go
# Look for any concurrent access patterns in the keeper
rg -A 5 "func \(k \*Keeper\)" x/oracle/keeper/native_token.go
Length of output: 760
Script:
#!/bin/bash
# Let's check the implementation of StakerInfo in the native_token.go file
rg -A 10 "StakerInfo" x/oracle/types/native_token.go
# Check if there are any concurrent operations in the keeper methods
rg -A 5 "func \(k Keeper\)" x/oracle/keeper/native_token.go
# Look for any goroutine usage in the oracle module
rg -A 5 "go func" x/oracle/
Length of output: 3966
| resPage, err := query.Paginate(store, req.Pagination, func(_ []byte, value []byte) error { | ||
| sInfo := types.StakerInfo{} | ||
| k.cdc.MustUnmarshal(iterator.Value(), &sInfo) | ||
| k.cdc.MustUnmarshal(value, &sInfo) | ||
| // keep only the latest effective-balance | ||
| if len(sInfo.BalanceList) > 0 { | ||
| sInfo.BalanceList = sInfo.BalanceList[len(sInfo.BalanceList)-1:] | ||
| } | ||
| // this is mainly used by price feeder, so we remove the stakerAddr to reduce the size of return value | ||
| sInfo.StakerAddr = "" | ||
| ret = append(ret, &sInfo) | ||
| retStakerInfos = append(retStakerInfos, &sInfo) | ||
| return nil | ||
| }) |
There was a problem hiding this comment.
Consider adding pagination limit validation.
The pagination implementation should validate the requested limit against a maximum allowed value to prevent potential resource exhaustion.
+ if req.Pagination != nil && req.Pagination.Limit > types.MaxPageLimit {
+ return nil, status.Errorf(codes.InvalidArgument, "pagination limit %d exceeds maximum allowed %d", req.Pagination.Limit, types.MaxPageLimit)
+ }
resPage, err := query.Paginate(store, req.Pagination, func(_ []byte, value []byte) error {Committable suggestion skipped: line range outside the PR's diff.
| // func (k Keeper) GetStakerInfos(ctx sdk.Context, assetID string) (ret []*types.StakerInfo) { | ||
| func (k Keeper) GetStakerInfos(ctx sdk.Context, req *types.QueryStakerInfosRequest) (*types.QueryStakerInfosResponse, error) { | ||
| store := prefix.NewStore(ctx.KVStore(k.storeKey), types.NativeTokenStakerKeyPrefix(req.AssetId)) | ||
| retStakerInfos := make([]*types.StakerInfo, 0) | ||
| resPage, err := query.Paginate(store, req.Pagination, func(_ []byte, value []byte) error { | ||
| sInfo := types.StakerInfo{} | ||
| k.cdc.MustUnmarshal(iterator.Value(), &sInfo) | ||
| k.cdc.MustUnmarshal(value, &sInfo) | ||
| // keep only the latest effective-balance | ||
| if len(sInfo.BalanceList) > 0 { | ||
| sInfo.BalanceList = sInfo.BalanceList[len(sInfo.BalanceList)-1:] | ||
| } | ||
| // this is mainly used by price feeder, so we remove the stakerAddr to reduce the size of return value | ||
| sInfo.StakerAddr = "" | ||
| ret = append(ret, &sInfo) | ||
| retStakerInfos = append(retStakerInfos, &sInfo) | ||
| return nil | ||
| }) | ||
| if err != nil { | ||
| return nil, status.Errorf(codes.InvalidArgument, "paginate: %v", err) | ||
| } | ||
| return ret | ||
| return &types.QueryStakerInfosResponse{ | ||
| StakerInfos: retStakerInfos, | ||
| Pagination: resPage, | ||
| }, nil |
There was a problem hiding this comment.
Consider adding error handling for nil request.
The function should validate the request parameter before proceeding with the pagination logic.
func (k Keeper) GetStakerInfos(ctx sdk.Context, req *types.QueryStakerInfosRequest) (*types.QueryStakerInfosResponse, error) {
+ if req == nil {
+ return nil, status.Error(codes.InvalidArgument, "empty request")
+ }
+ if req.AssetId == "" {
+ return nil, status.Error(codes.InvalidArgument, "empty asset id")
+ }
store := prefix.NewStore(ctx.KVStore(k.storeKey), types.NativeTokenStakerKeyPrefix(req.AssetId))📝 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.
| // func (k Keeper) GetStakerInfos(ctx sdk.Context, assetID string) (ret []*types.StakerInfo) { | |
| func (k Keeper) GetStakerInfos(ctx sdk.Context, req *types.QueryStakerInfosRequest) (*types.QueryStakerInfosResponse, error) { | |
| store := prefix.NewStore(ctx.KVStore(k.storeKey), types.NativeTokenStakerKeyPrefix(req.AssetId)) | |
| retStakerInfos := make([]*types.StakerInfo, 0) | |
| resPage, err := query.Paginate(store, req.Pagination, func(_ []byte, value []byte) error { | |
| sInfo := types.StakerInfo{} | |
| k.cdc.MustUnmarshal(iterator.Value(), &sInfo) | |
| k.cdc.MustUnmarshal(value, &sInfo) | |
| // keep only the latest effective-balance | |
| if len(sInfo.BalanceList) > 0 { | |
| sInfo.BalanceList = sInfo.BalanceList[len(sInfo.BalanceList)-1:] | |
| } | |
| // this is mainly used by price feeder, so we remove the stakerAddr to reduce the size of return value | |
| sInfo.StakerAddr = "" | |
| ret = append(ret, &sInfo) | |
| retStakerInfos = append(retStakerInfos, &sInfo) | |
| return nil | |
| }) | |
| if err != nil { | |
| return nil, status.Errorf(codes.InvalidArgument, "paginate: %v", err) | |
| } | |
| return ret | |
| return &types.QueryStakerInfosResponse{ | |
| StakerInfos: retStakerInfos, | |
| Pagination: resPage, | |
| }, nil | |
| func (k Keeper) GetStakerInfos(ctx sdk.Context, req *types.QueryStakerInfosRequest) (*types.QueryStakerInfosResponse, error) { | |
| if req == nil { | |
| return nil, status.Error(codes.InvalidArgument, "empty request") | |
| } | |
| if req.AssetId == "" { | |
| return nil, status.Error(codes.InvalidArgument, "empty asset id") | |
| } | |
| store := prefix.NewStore(ctx.KVStore(k.storeKey), types.NativeTokenStakerKeyPrefix(req.AssetId)) | |
| retStakerInfos := make([]*types.StakerInfo, 0) | |
| resPage, err := query.Paginate(store, req.Pagination, func(_ []byte, value []byte) error { | |
| sInfo := types.StakerInfo{} | |
| k.cdc.MustUnmarshal(value, &sInfo) | |
| // keep only the latest effective-balance | |
| if len(sInfo.BalanceList) > 0 { | |
| sInfo.BalanceList = sInfo.BalanceList[len(sInfo.BalanceList)-1:] | |
| } | |
| retStakerInfos = append(retStakerInfos, &sInfo) | |
| return nil | |
| }) | |
| if err != nil { | |
| return nil, status.Errorf(codes.InvalidArgument, "paginate: %v", err) | |
| } | |
| return &types.QueryStakerInfosResponse{ | |
| StakerInfos: retStakerInfos, | |
| Pagination: resPage, | |
| }, nil |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (4)
.protolint.yml (1)
72-74: Document the reasoning for removing line length limitsConsider adding a comment explaining why the line length limits were removed and how formatting consistency will be maintained using
buf formatinstead.# MAX_LINE_LENGTH rule option. + # Note: Line length limits are now handled by buf format tool instead # max_line_length: # Enforces a maximum line length # max_chars: 120x/oracle/keeper/query_native_token_test.go (3)
18-34: Consider parameterizing the assetID for broader test coverage.The test currently uses a hardcoded
NSTETHAssetIDMainnet. Consider parameterizing this value to test pagination with different asset types.-func TestQueryStakerInfosPaginated(t *testing.T) { - assetID := string(keeper.NSTETHAssetIDMainnet) +func TestQueryStakerInfosPaginated(t *testing.T) { + testCases := []struct { + name string + assetID string + }{ + {"NSTETH Mainnet", string(keeper.NSTETHAssetIDMainnet)}, + // Add more asset types as needed + } + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + keeper, ctx := keepertest.OracleKeeper(t) + // ... rest of the test
35-74: Add edge case tests for pagination.Consider adding test cases for:
- Empty result set
- Maximum limit scenarios
- Invalid pagination parameters
- Requesting more items than available
+ t.Run("EmptyResult", func(t *testing.T) { + emptyAssetID := "non_existent_asset" + resp, err := keeper.StakerInfos(wctx, request(emptyAssetID, nil, 0, 10, true)) + require.NoError(t, err) + require.Empty(t, resp.StakerInfos) + require.Equal(t, uint64(0), resp.Pagination.Total) + }) + + t.Run("MaxLimit", func(t *testing.T) { + resp, err := keeper.StakerInfos(wctx, request(assetID, nil, 0, 1000, false)) + require.NoError(t, err) + require.Len(t, resp.StakerInfos, len(msgs)) + })
76-96: Enhance test data generation flexibility.The helper function uses hardcoded values for balance info. Consider parameterizing these values to test different scenarios:
-func createNStakerInfos(keeper *keeper.Keeper, ctx sdk.Context, assetID string, n int) []*types.StakerInfo { +func createNStakerInfos(keeper *keeper.Keeper, ctx sdk.Context, assetID string, n int, opts ...StakerInfoOption) []*types.StakerInfo { + type options struct { + balanceAmount uint64 + roundID uint64 + action types.Action + } + defaultOpts := options{ + balanceAmount: 32, + roundID: 0, + action: types.Action_ACTION_DEPOSIT, + } + for _, opt := range opts { + opt(&defaultOpts) + } ret := make([]*types.StakerInfo, 0, n) for i := 0; i < n; i++ { ret = append(ret, &types.StakerInfo{ StakerAddr: fmt.Sprintf("Staker_%d", i), StakerIndex: int64(i), ValidatorPubkeyList: []string{strconv.Itoa(i + 1)}, BalanceList: []*types.BalanceInfo{ { - RoundID: 0, + RoundID: defaultOpts.roundID, Block: 0, Index: 0, - Balance: 32, - Change: types.Action_ACTION_DEPOSIT, + Balance: defaultOpts.balanceAmount, + Change: defaultOpts.action, }, }, })
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
.protolint.yml(2 hunks)x/oracle/keeper/query_native_token_test.go(1 hunks)
🔇 Additional comments (3)
.protolint.yml (1)
42-42: Verify the impact of removing MAX_LINE_LENGTH rule
The removal of line length constraints could affect code readability. Since you're using buf format, please verify that it adequately handles line wrapping.
✅ Verification successful
Line length constraints are generally well-maintained with few exceptions
The verification shows that most proto files maintain reasonable line lengths, with only a few exceptions exceeding 120 characters:
- proto/exocore/operator/v1/query.proto (3 lines: 134-138 chars)
- proto/exocore/delegation/v1/query.proto (1 line: 125 chars)
- proto/exocore/dogfood/v1/query.proto (2 lines: 121 chars)
Since buf format is being used and the results show only minimal line length violations (6 lines across all proto files), removing the MAX_LINE_LENGTH rule appears safe. The few longer lines are only marginally over the limit and don't significantly impact readability.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if any proto files have excessively long lines after buf format
for file in $(fd -e proto); do
echo "Checking $file..."
# Find lines longer than 120 chars (common reasonable limit)
awk 'length($0) > 120 { printf "Line %d: %d chars\n", NR, length($0) }' "$file"
done
Length of output: 27649
x/oracle/keeper/query_native_token_test.go (2)
1-16: LGTM!
The package declaration and imports are well-organized and include all necessary dependencies for testing pagination functionality.
35-46: Add assertions for the exact content of returned staker infos.
While the test verifies the subset relationship, it should also verify the exact content and order of returned staker infos for each pagination step.
|
resolve: #205 |
| return err | ||
| } | ||
| if pageReq.Limit > types.MaxPageLimit { | ||
| return types.ErrInvalidPagination.Wrapf("QueryStgakerInfos max page limitation is %d, got %d", types.MaxPageLimit, pageReq.Limit) |
There was a problem hiding this comment.
| return types.ErrInvalidPagination.Wrapf("QueryStgakerInfos max page limitation is %d, got %d", types.MaxPageLimit, pageReq.Limit) | |
| return types.ErrInvalidPagination.Wrapf("QueryStakerInfos max page limitation is %d, got %d", types.MaxPageLimit, pageReq.Limit) |
Description
Notable Changes
Closes #XXX
Summary by CodeRabbit
New Features
Improvements
Bug Fixes
Documentation