feat: delegations status filter #386
Conversation
|
Important Review skippedReview was skipped as selected files did not have any reviewable changes. 💤 Files selected but had no reviewable changes (1)
⛔ Files ignored due to path filters (1)
You can disable this status message by setting the 📝 WalkthroughWalkthroughThis pull request introduces a new optional field, Changes
Sequence Diagram(s)sequenceDiagram
participant D as Delegator
participant Q as Query Handler
participant V as Status Validator (isValidStatus)
participant C as Validator Cache (context)
participant F as Filter Function (delegationStatusFilterFunc)
participant T as Balance Calculator (totalDelegationBalance)
D->>Q: Send query with status parameter
Q->>V: Validate status
alt Valid status
V-->>Q: Status OK
Q->>C: Initialize validator cache in context
Q->>F: Filter delegations by status using cache
alt Request Total Balance
Q->>T: Calculate balance based on filtered delegations
T-->>Q: Return computed balance
end
Q-->>D: Return filtered delegations or balance
else Invalid status
V-->>Q: Return error
Q-->>D: Return error response
end
Poem
🪧 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 (
|
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.Scanned FilesNone |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #386 +/- ##
==========================================
+ Coverage 40.07% 40.14% +0.07%
==========================================
Files 294 294
Lines 27511 27562 +51
==========================================
+ Hits 11025 11066 +41
- Misses 14785 14791 +6
- Partials 1701 1705 +4
🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
x/mstaking/keeper/grpc_query.go (1)
19-21: Consider using a typed context key for safety.Currently, the code uses
iotafor the context key. For clarity and to avoid potential collisions with other packages, you can define a private type and constant, for example:-const ( - contextKeyValidators = iota -) +type contextKey int +const contextKeyValidators contextKey = iota
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Lite
📒 Files selected for processing (1)
x/mstaking/keeper/grpc_query.go(8 hunks)
🧰 Additional context used
🧬 Code Definitions (1)
x/mstaking/keeper/grpc_query.go (2)
x/ibc/perm/keeper/keeper.go (1)
Keeper(21-29)x/mstaking/types/validator.go (1)
Validators(77-80)
🔇 Additional comments (13)
x/mstaking/keeper/grpc_query.go (13)
37-39: Looks good!The additional status validation check aligns well with the new
isValidStatusfunction.
239-243: Validation approach is consistent.Enforcing valid statuses here helps ensure error messages are surfaced early.
253-256: Validator caching initialization is clear.Storing a local map of validators in the context is a good approach to reduce redundant store lookups.
257-259: Delegation filtering integration looks correct.This call to
delegationStatusFilterFuncproperly plugs into the pagination filter to handle status logic.
406-409: Repeated validation check is consistent.Continuing the same pattern of ensuring
req.Statusis acceptable keeps the behavior uniform.
425-427: Helper function usage is clean.Extracting the delegation balance calculation into
totalDelegationBalanceimproves readability.
431-431: Return statement is straightforward.Returning the computed balance in a typed response aligns with the design.
544-567: Caching validators inlookupValidatoris efficient.This function neatly checks the context cache before falling back to the store, avoiding duplicate lookups.
569-570: Refined usage oflookupValidator.Switching from direct store calls to
lookupValidatorkeeps caching consistent and reduces repeated logic.
608-615: Redelegation logic uses cache effectively.Calling
lookupValidatorfor the destination validator, then generating a truncated balance, is in line with the rest of the module’s design.
637-650: Filtering closure is well-organized.Providing a closure for
CollectionFilteredPaginateoffers a flexible, pluggable approach to status-based filtering.
652-663: Aggregating balances intotalDelegationBalance.Iterating delegations and summing upon success is straightforward. This helps keep the calling method concise.
665-677: Delegation balance function is concise.Skipping shares when the validator status doesn't match requested criteria is an efficient way to avoid extra computation.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
x/mstaking/keeper/grpc_query.go (1)
684-689: 🛠️ Refactor suggestionEnsure case handling consistency within
isValidStatus.Currently,
isValidStatuschecks for an exact string match, but other parts of the code (e.g.,delegationStatusFilterFunc) usestrings.EqualFoldfor status comparison.To unify behavior, consider allowing case-insensitive checks:
func isValidStatus(s string) bool { - if s != "" && !(s == types.Bonded.String() || s == types.Unbonded.String() || s == types.Unbonding.String()) { - return false - } - return true + if s == "" { + return true + } + s = strings.ToLower(s) + return s == strings.ToLower(types.Bonded.String()) || + s == strings.ToLower(types.Unbonded.String()) || + s == strings.ToLower(types.Unbonding.String()) }
🧹 Nitpick comments (1)
x/mstaking/keeper/grpc_query.go (1)
549-572: Consider optimizing validator cache lookup.The validator cache lookup implementation is sound, but there's a small optimization opportunity.
The function accesses the context twice to get the same validators map - once for lookup and once for update. Consider retrieving the map only once:
func lookupValidator(ctx context.Context, k *Keeper, valAddrStr string) (types.Validator, error) { - if validators, ok := sdk.UnwrapSDKContext(ctx).Value(contextKeyValidators).(map[string]types.Validator); ok { + sdkCtx := sdk.UnwrapSDKContext(ctx) + if validators, ok := sdkCtx.Value(contextKeyValidators).(map[string]types.Validator); ok { if val, ok := validators[valAddrStr]; ok { return val, nil } + + // Continue using validators for store lookup case + valAddr, err := k.validatorAddressCodec.StringToBytes(valAddrStr) + if err != nil { + return types.Validator{}, err + } + + val, err := k.Validators.Get(ctx, valAddr) + if err != nil { + return types.Validator{}, err + } + + validators[valAddrStr] = val + return val, nil } valAddr, err := k.validatorAddressCodec.StringToBytes(valAddrStr) if err != nil { return types.Validator{}, err } val, err := k.Validators.Get(ctx, valAddr) if err != nil { return types.Validator{}, err } - if validators, ok := sdk.UnwrapSDKContext(ctx).Value(contextKeyValidators).(map[string]types.Validator); ok { - validators[valAddrStr] = val - } return val, nil }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Lite
⛔ Files ignored due to path filters (2)
x/mstaking/types/query.pb.gois excluded by!**/*.pb.go,!**/*.pb.gox/mstaking/types/query.pb.gw.gois excluded by!**/*.pb.gw.go,!**/*.pb.gw.go
📒 Files selected for processing (3)
proto/initia/mstaking/v1/query.proto(2 hunks)x/mstaking/keeper/grpc_query.go(8 hunks)x/mstaking/keeper/grpc_query_test.go(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- x/mstaking/keeper/grpc_query_test.go
- proto/initia/mstaking/v1/query.proto
🔇 Additional comments (11)
x/mstaking/keeper/grpc_query.go (11)
19-21: LGTM: Context key for validator cache.The constant
contextKeyValidatorsis appropriately defined usingiotaas a unique key for storing validator cache in the context.
239-242: LGTM: Status validation in DelegatorDelegations.Good addition of status validation before processing delegations. This ensures that invalid status values are rejected with a proper error message.
253-255: LGTM: Validator caching implementation.Creating a validator cache and storing it in the context is a good optimization that will reduce repeated validator lookups when processing multiple delegations.
257-263: LGTM: Using filtered pagination with status filter.The approach of using
CollectionFilteredPaginatewith a dedicated filter function is clean and maintainable.
411-414: LGTM: Status validation in DelegatorTotalDelegationBalance.Consistent validation of status parameters across query methods ensures a uniform API experience.
430-434: LGTM: Using helper for total delegation balance calculation.The new approach using a dedicated helper function improves code organization and reusability.
574-592: LGTM: Using cached validator in delegation response.The function appropriately uses the new
lookupValidatorhelper to efficiently retrieve validators for delegation responses.
609-640: LGTM: Using cached validator in redelegation responses.Consistent use of the validator cache mechanism across all response generation functions improves overall performance.
642-655: LGTM: Status filtering implementation.The filter function correctly handles empty status (return all delegations) and performs case-insensitive matching with
strings.EqualFold.
657-668: LGTM: Total delegation balance calculation.The function correctly handles all edge cases, including nil balances from filtered delegations that don't match the status criteria.
670-682: LGTM: Single delegation balance calculation with status filtering.The function correctly returns nil for delegations that don't match the status filter, which is then properly handled by the calling function.
Description
introduce status filter on delegator delegations and total delegation balance query
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...
!in the type prefix if API or client breaking changeReviewers 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...