feat(delegation): query maximum undelegation amount #285
Conversation
WalkthroughThis pull request updates the API specification, protocol definitions, tests, and keeper logic for delegation information. The OpenAPI documentation and proto files now include a new response type that groups delegation amounts and the maximum undelegatable amount. The gRPC query methods and helper functions have been refactored to handle lowercasing for consistent identifier processing. Additionally, some test utilities have been simplified by removing extra logic and redundant queries, and asset-related constants are now imported from a dedicated package. Changes
Sequence Diagram(s)sequenceDiagram
participant C as Client
participant G as gRPC Endpoint
participant K as Keeper
participant A as AssetsKeeper
C->>G: QuerySingleDelegationInfo(request with IDs)
G->>K: GetSingleDelegationInfo(lowercased IDs)
K->>K: Retrieve delegationAmounts
K->>K: Call UndelegatableAmount(...)
K->>A: Request asset state for operator
A-->>K: Return asset state data
K->>G: Construct SingleDelegationInfoResponse with delegationAmounts and maxUndelegatableAmount
G->>C: Return constructed response
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
✨ Finishing Touches
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 (
|
TimmyExogenous
requested review from
bwhour,
cloud8little,
MaxMustermann2 and
leonz789
TimmyExogenous
changed the title
![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
🧹 Nitpick comments (2)
x/delegation/keeper/grpc_query.go (1)
12-26: LGTM! Consider enhancing error messages.The implementation correctly retrieves delegation information and calculates the maximum undelegatable amount. The error handling is appropriate with early returns.
Consider adding more context to error messages:
- return nil, err + return nil, fmt.Errorf("failed to get delegation info: %w", err) - return nil, err + return nil, fmt.Errorf("failed to calculate undelegatable amount: %w", err)client/docs/swagger-ui/swagger.json (1)
20834-20836: Maintain consistent terminology.The description uses "unbonded" while the field names use "undelegation". Consider using consistent terminology throughout the API documentation.
- "description": "wait_undelegation_amount is the amount that is waiting to be unbonded." + "description": "wait_undelegation_amount is the amount that is waiting to be undelegated."
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (4)
x/delegation/types/genesis.pb.gois excluded by!**/*.pb.gox/delegation/types/query.pb.gois excluded by!**/*.pb.gox/dogfood/types/query.pb.gois excluded by!**/*.pb.gox/dogfood/types/query.pb.gw.gois excluded by!**/*.pb.gw.go
📒 Files selected for processing (7)
client/docs/swagger-ui/swagger.json(2 hunks)proto/exocore/delegation/v1/genesis.proto(1 hunks)proto/exocore/delegation/v1/query.proto(4 hunks)testutil/batch/tx_check.go(1 hunks)x/delegation/keeper/delegation_op_test.go(1 hunks)x/delegation/keeper/delegation_state.go(3 hunks)x/delegation/keeper/grpc_query.go(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- x/delegation/keeper/delegation_op_test.go
🧰 Additional context used
🪛 GitHub Check: break-check
proto/exocore/delegation/v1/query.proto
[failure] 182-182:
RPC "QuerySingleDelegationInfo" on service "Query" changed response type from "exocore.delegation.v1.DelegationAmounts" to "exocore.delegation.v1.SingleDelegationInfoResponse".
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: goreleaser
- GitHub Check: Analyze (go)
🔇 Additional comments (9)
testutil/batch/tx_check.go (1)
58-58: LGTM! Code simplified correctly.The method now correctly uses the
MaxUndelegatableAmountfrom the new response structure, removing duplicate calculation logic.x/delegation/keeper/delegation_state.go (2)
75-87: LGTM! Well-structured new method.The
UndelegatableAmountmethod is well-implemented with proper validation and error handling.
Line range hint
97-101: Good refactoring to reduce code duplication.The changes to use
UndelegatableAmountin both methods improve code maintainability by centralizing the calculation logic.Also applies to: 113-117
client/docs/swagger-ui/swagger.json (3)
20827-20847: Well-structured response schema with comprehensive documentation.The response schema is well-organized with:
- Clear separation of concerns between delegation amounts and maximum undelegatable amount
- Detailed documentation of the mathematical formulas for share calculations
- Proper type definitions and descriptions for all fields
43425-43449: Well-defined schema with proper documentation.The SingleDelegationInfoResponse schema is well-structured and properly documented. The schema definition aligns perfectly with the endpoint specification and PR objectives.
Note: The same terminology inconsistency between "unbonded" and "undelegation" exists in this section. Please update it for consistency when addressing the previous comment.
20819-20847: Verify integration with proto and keeper changes.The OpenAPI changes look well-integrated with the mentioned proto and keeper changes. Let's verify the alignment of field names and types across the codebase.
Also applies to: 43425-43449
✅ Verification successful
OpenAPI changes correctly aligned with proto and keeper implementations ✅
The field names, types, and documentation in the OpenAPI specification perfectly match the proto definitions and keeper implementation. The
max_undelegatable_amountfield is properly exposed as a string type (matching cosmos.Int), and the nesteddelegation_amountsstructure is accurately represented.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify field names and types are consistent across proto and OpenAPI specs # Check proto definitions echo "Checking proto definitions for field alignment..." rg -A 10 "message SingleDelegationInfoResponse" "proto/" # Check keeper implementation echo "Checking keeper implementation..." rg -A 10 "SingleDelegationInfoResponse" "x/delegation/keeper/"Length of output: 2831
Script:
#!/bin/bash # Check OpenAPI spec for SingleDelegationInfoResponse echo "Checking OpenAPI spec..." rg -A 15 '"SingleDelegationInfoResponse"' client/docs/swagger-ui/swagger.jsonLength of output: 138
Script:
#!/bin/bash # Check specific lines in swagger.json echo "Checking specific lines in swagger.json..." sed -n '20819,20847p' client/docs/swagger-ui/swagger.json # Search for similar patterns in case of different naming echo -e "\nChecking for similar response patterns..." rg -i "delegation.*info.*response" client/docs/swagger-ui/swagger.jsonLength of output: 2917
proto/exocore/delegation/v1/genesis.proto (1)
45-45: Verify the removal of nullable optionThe removal of
[(gogoproto.nullable) = false]from thestakersfield appears unrelated to the PR's objective of adding maximum undelegatable amount information. This change could affect backward compatibility and data serialization.Consider keeping the nullable option to maintain consistent behavior with other repeated fields in the file that are marked as non-nullable (e.g.,
associations,delegation_states).- repeated string stakers = 2; + repeated string stakers = 2 [(gogoproto.nullable) = false];proto/exocore/delegation/v1/query.proto (2)
72-82: LGTM! Well-structured response messageThe new
SingleDelegationInfoResponsemessage is well-designed with:
- Clear field names and descriptions
- Proper use of cosmos scalar types
- Consistent nullable options
180-185: Breaking change in RPC response typeThe change in response type from
DelegationAmountstoSingleDelegationInfoResponseis a breaking change that requires careful migration planning.Please ensure that:
- All clients of this RPC endpoint are updated to handle the new response type
- The breaking change is documented in the changelog
- Consider version bumping according to semantic versioning
🧰 Tools
🪛 GitHub Check: break-check
[failure] 182-182:
RPC "QuerySingleDelegationInfo" on service "Query" changed response type from "exocore.delegation.v1.DelegationAmounts" to "exocore.delegation.v1.SingleDelegationInfoResponse".
|
There is delegation record for the staker, but the grpc query( |
TimmyExogenous
force-pushed
the
max-undelegation-amount
branch
from
3ffdd00 to
0026a73
Compare
The issue in the gRPCs of the assets, delegation, and operator modules has been fixed in the last commit. |
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (2)
x/operator/keeper/grpc_query.go (2)
350-357:⚠️ Potential issueEnsure consistent case handling for AVS addresses.
The removal of
strings.ToLowerforreq.AvsinQuerySnapshotHelperis inconsistent with the case-insensitive handling added in other files, particularly inGetSnapshotHelperwhich now converts the AVS address to lowercase.Apply this diff to maintain consistency:
- snapshotHelper, err := k.GetSnapshotHelper(ctx, req.Avs) + snapshotHelper, err := k.GetSnapshotHelper(ctx, strings.ToLower(req.Avs))
359-369:⚠️ Potential issueEnsure consistent case handling for AVS addresses.
The removal of
strings.ToLowerforreq.AvsinQuerySpecifiedSnapshotis inconsistent with the case-insensitive handling pattern.Apply this diff to maintain consistency:
- findHeight, snapshot, err := k.LoadVotingPowerSnapshot(ctx, req.Avs, req.Height) + findHeight, snapshot, err := k.LoadVotingPowerSnapshot(ctx, strings.ToLower(req.Avs), req.Height)
🧹 Nitpick comments (3)
x/delegation/keeper/delegation_state.go (3)
75-87: Add input validation for assetID and amounts parameters.The method should validate all input parameters to ensure robustness:
- Check if
assetIDis empty- Validate that
amountsis not nilfunc (k Keeper) UndelegatableAmount(ctx sdk.Context, assetID, operator string, amounts *delegationtype.DelegationAmounts) (amount sdkmath.Int, err error) { + if assetID == "" { + return sdkmath.ZeroInt(), errorsmod.Wrap(delegationtype.ErrInvalidInput, "assetID cannot be empty") + } + if amounts == nil { + return sdkmath.ZeroInt(), errorsmod.Wrap(delegationtype.ErrInvalidInput, "amounts cannot be nil") + } opAccAddr := sdk.MustAccAddressFromBech32(operator)
89-106: Add input validation for stakerID and assetID parameters.The method should validate input parameters before processing:
func (k Keeper) TotalDelegatedAmountForStakerAsset(ctx sdk.Context, stakerID string, assetID string) (amount sdkmath.Int, err error) { + if stakerID == "" { + return sdkmath.ZeroInt(), errorsmod.Wrap(delegationtype.ErrInvalidInput, "stakerID cannot be empty") + } + if assetID == "" { + return sdkmath.ZeroInt(), errorsmod.Wrap(delegationtype.ErrInvalidInput, "assetID cannot be empty") + } amount = sdkmath.ZeroInt()
108-125: Add input validation and optimize map initialization.Consider these improvements:
- Add input parameter validation
- Add capacity hint for the map based on the expected number of operators
func (k *Keeper) AllDelegatedInfoForStakerAsset(ctx sdk.Context, stakerID string, assetID string) (map[string]sdkmath.Int, error) { + if stakerID == "" { + return nil, errorsmod.Wrap(delegationtype.ErrInvalidInput, "stakerID cannot be empty") + } + if assetID == "" { + return nil, errorsmod.Wrap(delegationtype.ErrInvalidInput, "assetID cannot be empty") + } - ret := make(map[string]sdkmath.Int) + // Initialize with capacity hint based on average number of operators per staker + ret := make(map[string]sdkmath.Int, 10) // Adjust capacity based on your use case
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
x/delegation/types/query.pb.gois excluded by!**/*.pb.go
📒 Files selected for processing (9)
client/docs/swagger-ui/swagger.json(2 hunks)proto/exocore/delegation/v1/query.proto(4 hunks)testutil/batch/tx_check.go(1 hunks)x/assets/keeper/grpc_query.go(5 hunks)x/delegation/keeper/delegation_op_test.go(6 hunks)x/delegation/keeper/delegation_state.go(3 hunks)x/delegation/keeper/grpc_query.go(4 hunks)x/operator/keeper/grpc_query.go(2 hunks)x/operator/keeper/voting_power_snapshot.go(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- testutil/batch/tx_check.go
- x/delegation/keeper/delegation_op_test.go
🧰 Additional context used
🪛 GitHub Check: break-check
proto/exocore/delegation/v1/query.proto
[failure] 182-182:
RPC "QuerySingleDelegationInfo" on service "Query" changed response type from "exocore.delegation.v1.DelegationAmounts" to "exocore.delegation.v1.SingleDelegationInfoResponse".
🔇 Additional comments (17)
x/delegation/keeper/grpc_query.go (6)
13-28: LGTM! Enhanced query response with maximum undelegatable amount.The implementation correctly:
- Converts identifiers to lowercase for case-insensitive handling
- Calculates the maximum undelegatable amount
- Returns both delegation amounts and maximum undelegatable amount in the response
30-33: LGTM! Added case-insensitive handling for identifiers.Consistent case handling using
strings.ToLowerforStakerIdandAssetId.
35-44: LGTM! Added case-insensitive handling for undelegations query.Consistent case handling using
strings.ToLowerforStakerIdandAssetId.
57-65: LGTM! Added case-insensitive handling for undelegation hold count.Consistent case handling using
strings.ToLowerforStakerIdandAssetId.
67-76: LGTM! Added case-insensitive handling for associated operator query.Consistent case handling using
strings.ToLowerforStakerId.
89-99: LGTM! Added case-insensitive handling for delegated stakers query.Consistent case handling using
strings.ToLowerforAssetId.x/assets/keeper/grpc_query.go (4)
58-62: LGTM! Added case-insensitive handling for staking asset info query.Consistent case handling using
strings.ToLowerforAssetId.
74-82: LGTM! Added case-insensitive handling for staker asset infos query.Consistent case handling using
strings.ToLowerforStakerId.
84-88: LGTM! Added case-insensitive handling for staker specified asset amount query.Consistent case handling using
strings.ToLowerfor bothStakerIdandAssetId.
104-112: LGTM! Added case-insensitive handling for operator specified asset amount query.Consistent case handling using
strings.ToLowerforAssetId.x/operator/keeper/voting_power_snapshot.go (1)
229-241: LGTM! Added case-insensitive handling for snapshot helper.Consistent case handling using
strings.ToLowerfor AVS address when accessing store.client/docs/swagger-ui/swagger.json (3)
20819-20819: LGTM! Clear and descriptive endpoint summary.The updated summary accurately reflects the enhanced functionality of the endpoint, which now includes both delegation information and maximum undelegatable amount.
20827-20841: LGTM! Well-structured response schema with clear documentation.The response schema has been thoughtfully organized:
- Groups related fields under
delegation_amounts- Includes detailed descriptions for complex calculations
- Clearly documents the maximum undelegatable amount
Also applies to: 20842-20845, 20847-20847
43425-43449: LGTM! Consistent type definition.The
SingleDelegationInfoResponsetype definition matches the endpoint response schema, maintaining consistency across the API specification.proto/exocore/delegation/v1/query.proto (3)
61-62: LGTM! Clear documentation update.The message description accurately reflects the enhanced functionality to include maximum undelegatable amount information.
72-82: Breaking change: New response type for enhanced delegation info.The new
SingleDelegationInfoResponsemessage is well-structured and properly annotated. However, this is a breaking change that requires careful consideration during deployment.Please ensure that:
- All clients are updated to handle the new response type
- The API version is incremented appropriately
- Migration documentation is updated to reflect this breaking change
180-185:✅ Verification successful
LGTM! RPC method updated to support maximum undelegatable amount.
The changes align perfectly with the PR objectives to enhance the
QuerySingleDelegationInfoRPC method. The implementation addresses the issue reported by cloud8little in the PR comments regarding delegation record retrieval.
🏁 Script executed:
#!/bin/bash # Description: Verify that the implementation in the keeper matches the proto definition # Check if the keeper implementation returns both delegation amounts and max undelegatable amount rg -A 10 "func.*QuerySingleDelegationInfo.*Context.*SingleDelegationInfoReq.*SingleDelegationInfoResponse"Length of output: 3307
LGTM! Keeper implementation verified for maximum undelegatable amount.
The proto and keeper changes have been confirmed:
- The keeper in
x/delegation/keeper/grpc_query.gocorrectly callsGetSingleDelegationInfoand then calculates the undelegatable amount viaUndelegatableAmount.- The RPC method remains consistent with the proto definition, preserving backward compatibility.
No further modifications are necessary.
🧰 Tools
🪛 GitHub Check: break-check
[failure] 182-182:
RPC "QuerySingleDelegationInfo" on service "Query" changed response type from "exocore.delegation.v1.DelegationAmounts" to "exocore.delegation.v1.SingleDelegationInfoResponse".
Description
This PR returns the maximum undelegatable amount in the RPC
QuerySingleDelegationInfo.Summary by CodeRabbit
New Features
Documentation