feat(RPC): refine some RPCs regarding asset and delegation #316
Conversation
WalkthroughThis pull request introduces multiple updates in data handling and querying for staker balances and delegation information. In the assets module, balance fields are now converted using Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant GRPCServer
participant StakerAssetKeeper
Client->>GRPCServer: QueryStakerBalance(request)
GRPCServer->>StakerAssetKeeper: GetStakerBalanceByAsset(ctx, lower(stakerId), lower(assetId))
StakerAssetKeeper-->>GRPCServer: balance data
GRPCServer->>Client: QueryStakerBalanceResponse(balance data)
sequenceDiagram
participant Client
participant GRPCServer
participant DelegationKeeper
Client->>GRPCServer: QuerySingleDelegationInfo(request)
GRPCServer->>DelegationKeeper: GetSingleDelegationInfo(ctx, stakerId, assetId)
DelegationKeeper-->>GRPCServer: delegation info wrapped in SingleDelegationInfo
GRPCServer->>Client: SingleDelegationInfoResponse(SingleDelegationInfo)
Suggested labels
Suggested reviewers
Poem
✨ Finishing Touches
🪧 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,
adu-web3,
MaxMustermann2 and
leonz789
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (3)
x/assets/types/query.pb.gois excluded by!**/*.pb.gox/assets/types/query.pb.gw.gois excluded by!**/*.pb.gw.gox/delegation/types/query.pb.gois excluded by!**/*.pb.go
📒 Files selected for processing (14)
precompiles/assets/query.go(1 hunks)precompiles/avs/query_test.go(1 hunks)proto/imuachain/assets/v1/query.proto(2 hunks)proto/imuachain/delegation/v1/query.proto(2 hunks)testutil/batch/tx_check.go(1 hunks)x/assets/keeper/grpc_query.go(1 hunks)x/assets/keeper/staker_asset.go(3 hunks)x/assets/types/general.go(1 hunks)x/delegation/keeper/delegation_state.go(1 hunks)x/delegation/keeper/grpc_query.go(1 hunks)x/oracle/keeper/feedermanagement/aggregator_test.go(0 hunks)x/oracle/keeper/feedermanagement/data_elaborate_test.go(3 hunks)x/oracle/keeper/feedermanagement/helper_test.go(1 hunks)x/oracle/keeper/prices_test.go(0 hunks)
💤 Files with no reviewable changes (2)
- x/oracle/keeper/prices_test.go
- x/oracle/keeper/feedermanagement/aggregator_test.go
✅ Files skipped from review due to trivial changes (3)
- x/oracle/keeper/feedermanagement/helper_test.go
- precompiles/avs/query_test.go
- x/oracle/keeper/feedermanagement/data_elaborate_test.go
🧰 Additional context used
🪛 GitHub Check: proto-lint
proto/imuachain/assets/v1/query.proto
[failure] 119-119:
Field name "Withdrawable" should be lower_snake_case, such as "withdrawable".
[failure] 125-125:
Field name "Delegated" should be lower_snake_case, such as "delegated".
[failure] 131-131:
Field name "PendingUndelegated" should be lower_snake_case, such as "pending_undelegated".
[failure] 137-137:
Field name "TotalDeposited" should be lower_snake_case, such as "total_deposited".
🪛 GitHub Check: proto-break-check
proto/imuachain/delegation/v1/query.proto
[failure] 76-76:
Previously present message "QueryDelegationInfoResponse.DelegationInfosEntry" was deleted from file.
[failure] 78-78:
Field "1" on message "QueryDelegationInfoResponse" changed type from "imuachain.delegation.v1.QueryDelegationInfoResponse.DelegationInfosEntry" to "imuachain.delegation.v1.DelegationInfoWithOperator".
[failure] 95-95:
Field "1" with name "single_delegation_info" on message "SingleDelegationInfoResponse" changed option "json_name" from "delegationAmounts" to "singleDelegationInfo".
[failure] 95-95:
Field "1" on message "SingleDelegationInfoResponse" changed type from "imuachain.delegation.v1.DelegationAmounts" to "imuachain.delegation.v1.SingleDelegationInfo".
[failure] 95-95:
Field "1" on message "SingleDelegationInfoResponse" changed name from "delegation_amounts" to "single_delegation_info".
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: test-unit-e2e
- GitHub Check: test-unit-cover
- GitHub Check: Analyze (go)
- GitHub Check: goreleaser
🔇 Additional comments (13)
proto/imuachain/delegation/v1/query.proto (2)
54-64: Review the new SingleDelegationInfo message structure.The new message type provides a cleaner encapsulation of delegation information by combining
DelegationAmountswithmax_undelegatable_amount. The field definitions and type annotations look correct.
66-72: Review the new DelegationInfoWithOperator message structure.The message effectively combines operator information with delegation details, improving data organization.
x/delegation/keeper/grpc_query.go (1)
24-29: Implementation aligns with new proto definitions.The response construction correctly uses the new
SingleDelegationInfostructure. The error handling and input validation remain robust.x/delegation/keeper/delegation_state.go (1)
209-226: Implementation aligns with new response structure.The code correctly initializes and populates the new slice-based structure for delegation information. The error handling and business logic remain intact.
testutil/batch/tx_check.go (1)
58-58: Implementation updated to use new response structure.The code correctly accesses the max undelegatable amount through the new nested structure.
precompiles/assets/query.go (1)
170-174: LGTM! Consistent type conversion for balance fields.The changes ensure that all balance fields are consistently converted to
BigInt(), improving type safety and alignment with the updatedStakerBalancestruct.x/assets/keeper/grpc_query.go (1)
114-123: LGTM! Clean implementation of the new RPC.The implementation follows best practices:
- Proper error handling
- Case-insensitive ID comparison
- Consistent with other query methods in the file
x/assets/types/general.go (1)
100-108:✅ Verification successful
Verify the impact of removing the StakerBalance struct.
The struct has been moved to proto definitions. Let's verify that all references have been updated.
🏁 Script executed:
#!/bin/bash # Description: Search for any remaining references to the old StakerBalance struct. # Search for direct struct usage rg -A 5 'type StakerBalance struct' # Search for field references with old naming rg -A 5 'StakerID.*AssetID.*Balance.*Withdrawable.*Delegated.*PendingUndelegated.*TotalDeposited'Length of output: 1400
StakerBalance Struct Update Verified
The search confirms that the old definition inx/assets/types/general.gois now commented out and no active code references its fields. The updated definitions—one inprecompiles/assets/abi_types.goand the proto-generated version inx/assets/types/query.pb.go—properly replace the removed struct. If the commented-out code inx/assets/types/general.gois no longer needed, please consider removing it to clean up the codebase.x/assets/keeper/staker_asset.go (2)
100-106: LGTM! Simplified delegation info handling.The changes improve code readability by:
- Directly using
MaxUndelegatableAmountfrom delegation info- Simplifying the logic for updating total deposit and pending undelegation amounts
209-215: LGTM! Updated field names to match proto definitions.Field names have been updated from camelCase to PascalCase to match the proto definitions:
StakerID→StakerIdAssetID→AssetIdproto/imuachain/assets/v1/query.proto (3)
144-150: ApproveQueryStakerBalanceRequestMessageThe
QueryStakerBalanceRequestmessage is succinct and uses appropriate, clearly named fields for querying a staker's balance by asset.
152-157: ApproveQueryStakerBalanceResponseMessageThe
QueryStakerBalanceResponsemessage encapsulates the staker balance using the newly definedStakerBalancemessage neatly. Its structure meets the RPC response requirements.
205-209: Approve New RPC MethodQueryStakerBalanceThe new RPC method
QueryStakerBalanceis implemented with clean syntax and proper RESTful HTTP mapping. It aligns with the PR objectives by providing the required functionality to query staker balances based on the specified asset and staker id.
| message QueryDelegationInfoResponse { | ||
| // delegation_infos is the delegation information for each operator. | ||
| map<string, DelegationAmounts> delegation_infos = 1; | ||
| repeated DelegationInfoWithOperator delegation_infos = 1; | ||
| } |
There was a problem hiding this comment.
Breaking change: QueryDelegationInfoResponse structure modified.
The response structure has changed from a map to a repeated field, which is a breaking change that could affect existing clients.
Consider:
- Implementing a migration strategy for existing clients
- Versioning the API (e.g., v2) to maintain backward compatibility
- Documenting the breaking changes in release notes
🧰 Tools
🪛 GitHub Check: proto-break-check
[failure] 76-76:
Previously present message "QueryDelegationInfoResponse.DelegationInfosEntry" was deleted from file.
[failure] 78-78:
Field "1" on message "QueryDelegationInfoResponse" changed type from "imuachain.delegation.v1.QueryDelegationInfoResponse.DelegationInfosEntry" to "imuachain.delegation.v1.DelegationInfoWithOperator".
| // SingleDelegationInfoResponse is the response to QuerySingleDelegationInfo | ||
| message SingleDelegationInfoResponse { | ||
| // delegation_amounts is the delegation info recorded in the KVStore | ||
| DelegationAmounts delegation_amounts = 1; | ||
| // max_undelegatable_amount is the maximum amount that can be undelegated | ||
| string max_undelegatable_amount = 2 [ | ||
| (cosmos_proto.scalar) = "cosmos.Int", | ||
| (gogoproto.customtype) = "github.com/cosmos/cosmos-sdk/types.Int", | ||
| (gogoproto.nullable) = false | ||
| ]; | ||
| // single_delegation_info | ||
| SingleDelegationInfo single_delegation_info = 1 ; | ||
| } |
There was a problem hiding this comment.
Breaking change: SingleDelegationInfoResponse structure modified.
The response structure has changed significantly, which is a breaking change that could affect existing clients.
Consider:
- Implementing a migration strategy for existing clients
- Versioning the API (e.g., v2) to maintain backward compatibility
- Documenting the breaking changes in release notes
🧰 Tools
🪛 GitHub Check: proto-break-check
[failure] 93-93:
Previously present field "2" with name "max_undelegatable_amount" on message "SingleDelegationInfoResponse" was deleted.
[failure] 95-95:
Field "1" with name "single_delegation_info" on message "SingleDelegationInfoResponse" changed option "json_name" from "delegationAmounts" to "singleDelegationInfo".
[failure] 95-95:
Field "1" on message "SingleDelegationInfoResponse" changed type from "imuachain.delegation.v1.DelegationAmounts" to "imuachain.delegation.v1.SingleDelegationInfo".
[failure] 95-95:
Field "1" on message "SingleDelegationInfoResponse" changed name from "delegation_amounts" to "single_delegation_info".
TimmyExogenous
force-pushed
the
assets-rpc
branch
from
c02c014 to
078f7a7
Compare
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's limits when posting large numbers of comments. If you are seeing this consistently, please check "Code review limits" under "Moderation" settings.
Actionable comments posted: 2
🧹 Nitpick comments (4)
client/docs/swagger-ui/swagger.json (3)
18313-18417: New Endpoint Definition – Validate Auto-generated Fields
The new GET endpoint/imuachain/assets/v1/staker_balance/{staker_id}/{asset_id}is defined with a clear response schema. However, note that thebalancefield (line 18335–18338) is reporting a pipeline diff error (failure at line 18336). Please verify that the generated swagger output remains stable when runningmake proto-swagger-genand that naming (e.g. use of uppercase inWithdrawable,Delegated, etc.) follows project conventions.🧰 Tools
🪛 GitHub Actions: Check Proto Swagger Generation
[error] 18336-18336: Changes detected after running 'make proto-swagger-gen'.
21018-21058: Delegation Infos Schema Enhancement
The update converts thedelegation_infosfield to an array and introduces a nestedmax_undelegatable_amountinside each delegation object. The detailed descriptions and titles clarify the purpose of each field. Please double-check that these properties align exactly with the proto definitions and internal documentation standards.
21260-21289: SingleDelegationInfoResponse Schema Update
The schema forsingle_delegation_infonow includes the additionalmax_undelegatable_amountfield alongside the existing delegation amounts. Ensure that the title and description are consistent with other similar structures, and verify that this structure matches the proto-generated expectations.proto/imuachain/assets/v1/query.proto (1)
102-142: Message StakerBalance – Confirm Field Names & Update Comment for Consistency
The definition of the new message is clear and uses lower_snake_case for all fields as required. One minor nitpick: the comment on line 103 states “pendingUndelegated” while the field name ispending_undelegated. For consistency and clarity, please update the comment to match the field name.
🛑 Comments failed to post (2)
client/docs/swagger-ui/swagger.json (2)
41301-41344:
⚠️ Potential issueQueryStakerBalanceResponse Schema – Pipeline Verification Needed
The newimuachain.assets.v1.QueryStakerBalanceResponseobject is well defined; however, thebalanceproperty at line 41319–41322 (specifically line 41320) is triggering a pipeline error. Please re-runmake proto-swagger-gento ensure that this field’s title and type accurately reflect the intended output. Adjust the naming or formatting if necessary to achieve a stable proto-swagger output.🧰 Tools
🪛 GitHub Actions: Check Proto Swagger Generation
[error] 41320-41320: Changes detected after running 'make proto-swagger-gen'.
41359-41398:
⚠️ Potential issueStakerBalance Schema Addition – Confirm Consistency
The introduction of theimuachain.assets.v1.StakerBalanceschema is clear and detailed. Note that thebalancefield (line 41373–41376) has also been flagged by the pipeline (error at line 41374). Please verify that the field definition (including its title and type) exactly matches the auto-generated proto output and standard conventions used across the API.🧰 Tools
🪛 GitHub Actions: Check Proto Swagger Generation
[error] 41374-41374: Changes detected after running 'make proto-swagger-gen'.
TimmyExogenous
force-pushed
the
assets-rpc
branch
from
078f7a7 to
b1d2db9
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
proto/imuachain/delegation/v1/query.proto (2)
76-79:⚠️ Potential issueBreaking Change: Modified 'QueryDelegationInfoResponse' Structure
The fielddelegation_infosnow uses a repeatedDelegationInfoAndOperatorrather than a map-based structure. This is a notable breaking change that can affect existing client integrations. Please ensure that migration guidelines and proper API versioning are documented.🧰 Tools
🪛 GitHub Check: proto-break-check
[failure] 76-76:
Previously present message "QueryDelegationInfoResponse.DelegationInfosEntry" was deleted from file.
[failure] 78-78:
Field "1" on message "QueryDelegationInfoResponse" changed type from "imuachain.delegation.v1.QueryDelegationInfoResponse.DelegationInfosEntry" to "imuachain.delegation.v1.DelegationInfoAndOperator".
92-96:⚠️ Potential issueBreaking Change: Updated 'SingleDelegationInfoResponse' Structure
TheSingleDelegationInfoResponsenow wraps the delegation details inside a single fieldsingle_delegation_infoof typeSingleDelegationInfo, replacing the previous composite structure. This change requires updates on the client side; ensure that clear migration steps and versioning documentation are provided.🧰 Tools
🪛 GitHub Check: proto-break-check
[failure] 93-93:
Previously present field "2" with name "max_undelegatable_amount" on message "SingleDelegationInfoResponse" was deleted.
[failure] 95-95:
Field "1" with name "single_delegation_info" on message "SingleDelegationInfoResponse" changed option "json_name" from "delegationAmounts" to "singleDelegationInfo".
[failure] 95-95:
Field "1" on message "SingleDelegationInfoResponse" changed type from "imuachain.delegation.v1.DelegationAmounts" to "imuachain.delegation.v1.SingleDelegationInfo".
[failure] 95-95:
Field "1" on message "SingleDelegationInfoResponse" changed name from "delegation_amounts" to "single_delegation_info".
🧹 Nitpick comments (4)
x/assets/keeper/grpc_query.go (2)
114-123: Add documentation comments for the new query method.Consider adding documentation comments to describe the purpose, parameters, and return values of the
QueryStakerBalancemethod, following the pattern used for other query methods in this file.Apply this diff to add documentation:
+// QueryStakerBalance retrieves the balance of a staker for a specific asset +// Parameters: +// - ctx: The context for the query +// - req: The request containing stakerId and assetId +// Returns the staker's balance and any error encountered func (k Keeper) QueryStakerBalance(ctx context.Context, req *assetstype.QueryStakerBalanceRequest) (*assetstype.QueryStakerBalanceResponse, error) {
114-123: Consider adding input validation.The method could benefit from validating the
stakerIdandassetIdparameters before making the call toGetStakerBalanceByAsset. This would help catch invalid inputs early and provide better error messages.Apply this diff to add input validation:
func (k Keeper) QueryStakerBalance(ctx context.Context, req *assetstype.QueryStakerBalanceRequest) (*assetstype.QueryStakerBalanceResponse, error) { + if req.StakerId == "" { + return nil, status.Error(codes.InvalidArgument, "staker id cannot be empty") + } + if req.AssetId == "" { + return nil, status.Error(codes.InvalidArgument, "asset id cannot be empty") + } c := sdk.UnwrapSDKContext(ctx) stakerBalance, err := k.GetStakerBalanceByAsset(c, strings.ToLower(req.StakerId), strings.ToLower(req.AssetId))client/docs/swagger-ui/swagger.json (2)
41301-41344: New QueryStakerBalanceResponse Type Definition
Introducing the"imuachain.assets.v1.QueryStakerBalanceResponse"type with an embeddedstaker_balanceproperty is well structured. The schema is consistent with the earlier endpoint definition and ensures clear documentation of response properties.Suggestion: Consider reviewing the use of the
"title"attribute versus"description"to ensure consistency in property documentation.
41359-41398: StakerBalance Type Enhancement
The new"imuachain.assets.v1.StakerBalance"type effectively encapsulates the balance details for a staker. All fields—such asstaker_id,asset_id,balance,withdrawable,delegated,pending_undelegated, andtotal_deposited—are clearly described.Suggestion: Validate that field naming and descriptions here match those used elsewhere in your API for a consistent client experience.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (3)
x/assets/types/query.pb.gois excluded by!**/*.pb.gox/assets/types/query.pb.gw.gois excluded by!**/*.pb.gw.gox/delegation/types/query.pb.gois excluded by!**/*.pb.go
📒 Files selected for processing (15)
client/docs/swagger-ui/swagger.json(8 hunks)precompiles/assets/query.go(1 hunks)precompiles/avs/query_test.go(1 hunks)proto/imuachain/assets/v1/query.proto(2 hunks)proto/imuachain/delegation/v1/query.proto(2 hunks)testutil/batch/tx_check.go(1 hunks)x/assets/keeper/grpc_query.go(1 hunks)x/assets/keeper/staker_asset.go(3 hunks)x/assets/types/general.go(1 hunks)x/delegation/keeper/delegation_state.go(1 hunks)x/delegation/keeper/grpc_query.go(1 hunks)x/oracle/keeper/feedermanagement/aggregator_test.go(0 hunks)x/oracle/keeper/feedermanagement/data_elaborate_test.go(3 hunks)x/oracle/keeper/feedermanagement/helper_test.go(1 hunks)x/oracle/keeper/prices_test.go(0 hunks)
💤 Files with no reviewable changes (2)
- x/oracle/keeper/prices_test.go
- x/oracle/keeper/feedermanagement/aggregator_test.go
🚧 Files skipped from review as they are similar to previous changes (9)
- testutil/batch/tx_check.go
- precompiles/avs/query_test.go
- x/assets/types/general.go
- x/delegation/keeper/grpc_query.go
- x/delegation/keeper/delegation_state.go
- x/oracle/keeper/feedermanagement/helper_test.go
- x/oracle/keeper/feedermanagement/data_elaborate_test.go
- precompiles/assets/query.go
- x/assets/keeper/staker_asset.go
🧰 Additional context used
🪛 GitHub Actions: Check Proto Swagger Generation
client/docs/swagger-ui/swagger.json
[error] 21052-21052: Changes detected after running 'make proto-swagger-gen'.
[error] 44056-44056: Changes detected after running 'make proto-swagger-gen'.
[error] 44170-44170: Changes detected after running 'make proto-swagger-gen'.
🪛 GitHub Check: proto-break-check
proto/imuachain/delegation/v1/query.proto
[failure] 76-76:
Previously present message "QueryDelegationInfoResponse.DelegationInfosEntry" was deleted from file.
[failure] 78-78:
Field "1" on message "QueryDelegationInfoResponse" changed type from "imuachain.delegation.v1.QueryDelegationInfoResponse.DelegationInfosEntry" to "imuachain.delegation.v1.DelegationInfoAndOperator".
[failure] 95-95:
Field "1" with name "single_delegation_info" on message "SingleDelegationInfoResponse" changed option "json_name" from "delegationAmounts" to "singleDelegationInfo".
[failure] 95-95:
Field "1" on message "SingleDelegationInfoResponse" changed type from "imuachain.delegation.v1.DelegationAmounts" to "imuachain.delegation.v1.SingleDelegationInfo".
[failure] 95-95:
Field "1" on message "SingleDelegationInfoResponse" changed name from "delegation_amounts" to "single_delegation_info".
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: test-unit-cover
- GitHub Check: test-unit-e2e
- GitHub Check: goreleaser
- GitHub Check: Analyze (go)
🔇 Additional comments (11)
x/assets/keeper/grpc_query.go (1)
114-123: Implementation looks good!The core implementation follows the established patterns in the codebase, properly handles error cases, and maintains consistency with string case normalization.
client/docs/swagger-ui/swagger.json (6)
18313-18416: New API Endpoint for QueryStakerBalance Added
The new GET endpoint/imuachain/assets/v1/staker_balance/{staker_id}/{asset_id}is defined with clear parameters, response schema, summary, and operationId. The response properties (staker_id,asset_id,balance,withdrawable,delegated,pending_undelegated, andtotal_deposited) are well documented.
21018-21058: Delegation Info Structure Updated
The revised structure changesdelegation_infosfrom an object to an array and enriches each item with anoperatorfield and a nesteddelegation_infoobject that includesdelegation_amountsand the newmax_undelegatable_amountproperty. The descriptions are detailed and provide good context for each field.Action: Please verify that these changes completely align with the output of
make proto-swagger-gen(note: pipeline error detected near line 21052) and that the JSON schema remains consistent with your protobuf definitions.🧰 Tools
🪛 GitHub Actions: Check Proto Swagger Generation
[error] 21052-21052: Changes detected after running 'make proto-swagger-gen'.
21260-21289: Refinement of Single Delegation Info Schema
The schema forsingle_delegation_infonow clearly separates the delegation amounts (includingundelegatable_share) and adds themax_undelegatable_amountfield. The comprehensive descriptions help clarify edge cases such as the initial delegation scenario.Action: Double-check that these naming conventions and descriptions are uniformly used in both the proto definitions and the swagger output.
44056-44094: Update to DelegationInfoWithOperator Schema
The updated schema for"imuachain.delegation.v1.DelegationInfoWithOperator"now explicitly documents both the operator address and the embedded delegation information. However, note that the build pipeline flagged a change at line 44056.Action: Re-run
make proto-swagger-genand ensure that the auto‐generated output reflects these changes so that no discrepancies persist in your repository.🧰 Tools
🪛 GitHub Actions: Check Proto Swagger Generation
[error] 44056-44056: Changes detected after running 'make proto-swagger-gen'.
44136-44179: Refined QueryDelegationInfoResponse Structure
Thedelegation_infosarray has been updated to include each delegation’s operator and delegation info—with the newmax_undelegatable_amountfield. The detailed descriptions enhance clarity.Action: Check that the formatting and field ordering, especially around line 44170 (as noted by the pipeline error), are consistent with the proto-generated swagger output.
🧰 Tools
🪛 GitHub Actions: Check Proto Swagger Generation
[error] 44170-44170: Changes detected after running 'make proto-swagger-gen'.
44193-44236: Enhanced SingleDelegationInfoResponse Definition
The schema for"imuachain.delegation.v1.SingleDelegationInfoResponse"now incorporates a nestedsingle_delegation_infoobject that includes all delegation amount details and themax_undelegatable_amountfield. The added titles and descriptions improve readability.Action: Ensure that these modifications align with the auto-generated swagger (verify with
make proto-swagger-gen) and maintain consistency with the corresponding protobuf definitions.proto/imuachain/assets/v1/query.proto (4)
102-142: Verify Field Naming and Structure inStakerBalanceMessageThe new
StakerBalancemessage is introduced with comprehensive inline comments and uses lower_snake_case consistently for all field names. This effectively addresses the previously noted naming convention issues.
144-150: Ensure Correct Definition ofQueryStakerBalanceRequestThe
QueryStakerBalanceRequestmessage is properly defined with thestaker_idandasset_idfields, following the project's naming conventions and maintaining clarity.
152-156: Review Definition ofQueryStakerBalanceResponseThe
QueryStakerBalanceResponsemessage succinctly encapsulates theStakerBalanceobject, matching the design intent and ensuring consistency with similar response structures elsewhere in the proto.
205-209: Validation of New RPC Method:QueryStakerBalanceThe new RPC method
QueryStakerBalanceis correctly added to theQueryservice. It includes the module query safety flag and an HTTP GET annotation with a RESTful endpoint format (/imuachain/assets/v1/staker_balance/{staker_id}/{asset_id}), ensuring it aligns well with the overall API design.
| // DelegationInfoAndOperator includes the SingleDelegationInfo and operator address | ||
| message DelegationInfoAndOperator { | ||
| // operator is the operator address of this delegation | ||
| string operator = 1; | ||
| // delegation_info is the delegation information | ||
| SingleDelegationInfo delegation_info = 2; | ||
| } |
There was a problem hiding this comment.
Enhancement: New Message 'DelegationInfoAndOperator' Introduced
This new message cleanly encapsulates the operator address along with the delegation details by embedding SingleDelegationInfo. Confirm that all consuming clients are updated to use this combined structure and that the API documentation highlights this breaking change.
TimmyExogenous
force-pushed
the
assets-rpc
branch
from
b1d2db9 to
22ae0d5
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (2)
x/delegation/keeper/delegation_state.go (2)
207-233: Well-structured changes to GetDelegationInfo!The refactoring from a map to a slice of
DelegationInfoAndOperatorimproves the response structure and adds valuable information about maximum undelegatable amounts. The error handling is robust and the code is well-organized.Consider pre-allocating the slice with a reasonable capacity to avoid potential reallocations:
- ret.DelegationInfos = make([]*delegationtype.DelegationInfoAndOperator, 0) + ret.DelegationInfos = make([]*delegationtype.DelegationInfoAndOperator, 0, 10) // or another reasonable initial capacity
173-174: Consider implementing storage optimization TODOs.There are two related TODOs about optimizing storage by cleaning up zero-value states:
- Deletion of states when both share and WaitUndelegationAmount are zero
- Deletion of items with zero share when pendingUndelegation is also zero
These optimizations could help reduce chain state bloat.
Would you like me to help implement these storage optimizations? I can propose a solution that:
- Adds a helper method to check if a delegation state is "empty"
- Implements the cleanup logic in relevant methods
- Adds tests to verify the cleanup behavior
Also applies to: 359-360
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (3)
x/assets/types/query.pb.gois excluded by!**/*.pb.gox/assets/types/query.pb.gw.gois excluded by!**/*.pb.gw.gox/delegation/types/query.pb.gois excluded by!**/*.pb.go
📒 Files selected for processing (15)
client/docs/swagger-ui/swagger.json(8 hunks)precompiles/assets/query.go(1 hunks)precompiles/avs/query_test.go(1 hunks)proto/imuachain/assets/v1/query.proto(2 hunks)proto/imuachain/delegation/v1/query.proto(2 hunks)testutil/batch/tx_check.go(1 hunks)x/assets/keeper/grpc_query.go(1 hunks)x/assets/keeper/staker_asset.go(3 hunks)x/assets/types/general.go(1 hunks)x/delegation/keeper/delegation_state.go(1 hunks)x/delegation/keeper/grpc_query.go(1 hunks)x/oracle/keeper/feedermanagement/aggregator_test.go(0 hunks)x/oracle/keeper/feedermanagement/data_elaborate_test.go(3 hunks)x/oracle/keeper/feedermanagement/helper_test.go(1 hunks)x/oracle/keeper/prices_test.go(0 hunks)
💤 Files with no reviewable changes (2)
- x/oracle/keeper/feedermanagement/aggregator_test.go
- x/oracle/keeper/prices_test.go
🚧 Files skipped from review as they are similar to previous changes (9)
- x/assets/types/general.go
- x/assets/keeper/grpc_query.go
- precompiles/avs/query_test.go
- x/delegation/keeper/grpc_query.go
- testutil/batch/tx_check.go
- x/assets/keeper/staker_asset.go
- x/oracle/keeper/feedermanagement/data_elaborate_test.go
- precompiles/assets/query.go
- x/oracle/keeper/feedermanagement/helper_test.go
🧰 Additional context used
🪛 GitHub Check: proto-break-check
proto/imuachain/delegation/v1/query.proto
[failure] 76-76:
Previously present message "QueryDelegationInfoResponse.DelegationInfosEntry" was deleted from file.
[failure] 78-78:
Field "1" on message "QueryDelegationInfoResponse" changed type from "imuachain.delegation.v1.QueryDelegationInfoResponse.DelegationInfosEntry" to "imuachain.delegation.v1.DelegationInfoAndOperator".
[failure] 95-95:
Field "1" with name "single_delegation_info" on message "SingleDelegationInfoResponse" changed option "json_name" from "delegationAmounts" to "singleDelegationInfo".
[failure] 95-95:
Field "1" on message "SingleDelegationInfoResponse" changed type from "imuachain.delegation.v1.DelegationAmounts" to "imuachain.delegation.v1.SingleDelegationInfo".
[failure] 95-95:
Field "1" on message "SingleDelegationInfoResponse" changed name from "delegation_amounts" to "single_delegation_info".
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: test-unit-e2e
- GitHub Check: test-unit-cover
- GitHub Check: goreleaser
- GitHub Check: Analyze (go)
🔇 Additional comments (13)
client/docs/swagger-ui/swagger.json (8)
18313-18416: New Endpoint for Staker Balance Query
The new endpoint/imuachain/assets/v1/staker_balance/{staker_id}/{asset_id}is well-defined with clear parameter definitions and a detailed response schema. The use of precise titles and descriptions for properties such asstaker_id,asset_id,balance,withdrawable,delegated,pending_undelegated, andtotal_depositedhelps ensure clarity. Consider including example responses in the documentation if your guidelines permit.
21018-21059: Refactoring Delegation Infos Structure
Changingdelegation_infosfrom an object/map to an array improves data iterability and consistency with collection usage. The nested structure now clearly incorporates theoperatoranddelegation_info(with its detailed subproperties), and the addition ofmax_undelegatable_amountstrengthens the information provided.
21260-21290: Enhancement of SingleDelegationInfoResponse Schema
The update to includemax_undelegatable_amountinsidesingle_delegation_infoenriches the response's semantic detail by explicitly specifying the limits on undelegation. This structured approach matches the enhancements made elsewhere in the delegation schema.
41301-41344: New QueryStakerBalanceResponse Definition
The newly addedimuachain.assets.v1.QueryStakerBalanceResponseis well constructed. Its properties—such asstaker_balance(which nests fields likestaker_id,asset_id,balance, etc.)—are clearly defined with appropriate titles and descriptions. This ensures that API consumers understand the response composition.
41359-41398: Addition of StakerBalance Type
The introduction of theimuachain.assets.v1.StakerBalancetype neatly encapsulates the details related to a staker's asset balance. All properties are consistent with naming conventions and include concise descriptions, which will aid clients in correctly interpreting the data.
44056-44095: Introducing DelegationInfoAndOperator Structure
The new typeimuachain.delegation.v1.DelegationInfoAndOperatoreffectively combines the delegation information with its associated operator address. This structure now incorporates detailed delegation properties—including the recently addedmax_undelegatable_amount—which enhances clarity and ensures that consumers have access to all relevant details in one go.
44136-44180: Refinement of QueryDelegationInfoResponse
Switching thedelegation_infosproperty to an array format is a sound design choice, especially when handling multiple delegation entries. The nested objects now clearly define both the operator and detailed delegation info (includingmax_undelegatable_amount), making the response easier to parse and more consistent with modern API practices.
44193-44236: Enhancements in SingleDelegationInfo and Its Response Schema
The updates to bothSingleDelegationInfoandSingleDelegationInfoResponseeffectively integrate themax_undelegatable_amountfield. These changes bring enhanced expressiveness to the delegation data by explicitly outlining the limits on undelegation, with clear and thorough descriptions that help developers understand the nuances—for instance, the handling of initial delegation cases.proto/imuachain/assets/v1/query.proto (4)
102-142: Naming Conventions and Structure inStakerBalanceMessage
The newly addedStakerBalancemessage correctly uses lower_snake_case for all field names (e.g.staker_id,asset_id,balance,withdrawable,delegated,pending_undelegated, andtotal_deposited). This adheres to the project's naming conventions and addresses previous concerns.
144-150: Validation ofQueryStakerBalanceRequestStructure
The request message properly includesstaker_idandasset_idas required parameters. This structure is clean and concise.
152-156: Review ofQueryStakerBalanceResponseMessage
The response message encapsulates a singleStakerBalanceunder the fieldstaker_balance. The design is clear and aligns with the intended purpose of the RPC.
205-209: Addition ofQueryStakerBalanceRPC Method
The new RPC method is well-integrated into theQueryservice with appropriate HTTP mapping (/imuachain/assets/v1/staker_balance/{staker_id}/{asset_id}) and module query safety enabled. Ensure that the corresponding keeper implementation correctly handles this new request.proto/imuachain/delegation/v1/query.proto (1)
66-72: Addition ofDelegationInfoAndOperatorMessage
This message pairs the operator address with the newly structured delegation information, enhancing clarity in responses. Verify that all integration points consuming this message type are updated accordingly.
| func (k *Keeper) GetDelegationInfo(ctx sdk.Context, stakerID, assetID string) (*delegationtype.QueryDelegationInfoResponse, error) { | ||
| var ret delegationtype.QueryDelegationInfoResponse |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add input parameter validation.
The GetDelegationInfo method should validate its input parameters stakerID and assetID to ensure they are not empty or malformed before processing.
Add input validation at the start of the method:
func (k *Keeper) GetDelegationInfo(ctx sdk.Context, stakerID, assetID string) (*delegationtype.QueryDelegationInfoResponse, error) {
+ if stakerID == "" {
+ return nil, errorsmod.Wrap(delegationtype.ErrInvalidInput, "staker ID cannot be empty")
+ }
+ if assetID == "" {
+ return nil, errorsmod.Wrap(delegationtype.ErrInvalidInput, "asset ID cannot be empty")
+ }
var ret delegationtype.QueryDelegationInfoResponse
ret.DelegationInfos = make([]*delegationtype.DelegationInfoAndOperator, 0)📝 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) GetDelegationInfo(ctx sdk.Context, stakerID, assetID string) (*delegationtype.QueryDelegationInfoResponse, error) { | |
| var ret delegationtype.QueryDelegationInfoResponse | |
| func (k *Keeper) GetDelegationInfo(ctx sdk.Context, stakerID, assetID string) (*delegationtype.QueryDelegationInfoResponse, error) { | |
| if stakerID == "" { | |
| return nil, errorsmod.Wrap(delegationtype.ErrInvalidInput, "staker ID cannot be empty") | |
| } | |
| if assetID == "" { | |
| return nil, errorsmod.Wrap(delegationtype.ErrInvalidInput, "asset ID cannot be empty") | |
| } | |
| var ret delegationtype.QueryDelegationInfoResponse | |
| ret.DelegationInfos = make([]*delegationtype.DelegationInfoAndOperator, 0) | |
| // ... rest of the code ... | |
| } |
| // SingleDelegationInfo includes the share, amount and pending undelegation amount | ||
| message SingleDelegationInfo { | ||
| // delegation_amounts is the delegation info recorded in the KVStore | ||
| DelegationAmounts delegation_amounts = 1 ; | ||
| // max_undelegatable_amount is the maximum amount that can be undelegated | ||
| string max_undelegatable_amount = 2 [ | ||
| (cosmos_proto.scalar) = "cosmos.Int", | ||
| (gogoproto.customtype) = "github.com/cosmos/cosmos-sdk/types.Int", | ||
| (gogoproto.nullable) = false | ||
| ]; | ||
| } |
There was a problem hiding this comment.
Introduction of SingleDelegationInfo Message
The new SingleDelegationInfo message consolidates the delegation details by encapsulating both delegation_amounts and max_undelegatable_amount. Note that this change represents a breaking update compared to the previous structure. Please ensure that migration documentation and communication are provided for affected clients.
| message QueryDelegationInfoResponse { | ||
| // delegation_infos is the delegation information for each operator. | ||
| map<string, DelegationAmounts> delegation_infos = 1; | ||
| repeated DelegationInfoAndOperator delegation_infos = 1; | ||
| } |
There was a problem hiding this comment.
Modification of QueryDelegationInfoResponse Structure
Replacing the former map-based delegation information with a repeated field of DelegationInfoAndOperator enforces deterministic ordering. However, this is a breaking change. It is critical to provide migration guidance and update client documentation on this change.
🧰 Tools
🪛 GitHub Check: proto-break-check
[failure] 76-76:
Previously present message "QueryDelegationInfoResponse.DelegationInfosEntry" was deleted from file.
[failure] 78-78:
Field "1" on message "QueryDelegationInfoResponse" changed type from "imuachain.delegation.v1.QueryDelegationInfoResponse.DelegationInfosEntry" to "imuachain.delegation.v1.DelegationInfoAndOperator".
| message SingleDelegationInfoResponse { | ||
| // delegation_amounts is the delegation info recorded in the KVStore | ||
| DelegationAmounts delegation_amounts = 1; | ||
| // max_undelegatable_amount is the maximum amount that can be undelegated | ||
| string max_undelegatable_amount = 2 [ | ||
| (cosmos_proto.scalar) = "cosmos.Int", | ||
| (gogoproto.customtype) = "github.com/cosmos/cosmos-sdk/types.Int", | ||
| (gogoproto.nullable) = false | ||
| ]; | ||
| // single_delegation_info | ||
| SingleDelegationInfo single_delegation_info = 1 ; | ||
| } |
There was a problem hiding this comment.
Refinement in SingleDelegationInfoResponse
The response now returns a unified SingleDelegationInfo instead of separate fields for delegation amounts and the maximum undelegatable amount. This simplification, while beneficial, is a breaking change. Ensure that clients are notified and migration paths are documented.
🧰 Tools
🪛 GitHub Check: proto-break-check
[failure] 93-93:
Previously present field "2" with name "max_undelegatable_amount" on message "SingleDelegationInfoResponse" was deleted.
[failure] 95-95:
Field "1" with name "single_delegation_info" on message "SingleDelegationInfoResponse" changed option "json_name" from "delegationAmounts" to "singleDelegationInfo".
[failure] 95-95:
Field "1" on message "SingleDelegationInfoResponse" changed type from "imuachain.delegation.v1.DelegationAmounts" to "imuachain.delegation.v1.SingleDelegationInfo".
[failure] 95-95:
Field "1" on message "SingleDelegationInfoResponse" changed name from "delegation_amounts" to "single_delegation_info".
| // this amount is post-slashing, as explained above. | ||
| info.TotalDepositAmount = info.TotalDepositAmount.Add(undelegatableTokens).Add(record.WaitUndelegationAmount) | ||
| info.PendingUndelegationAmount = info.PendingUndelegationAmount.Add(record.WaitUndelegationAmount) | ||
| info.TotalDepositAmount = info.TotalDepositAmount.Add(delegationInfo.DelegationInfo.MaxUndelegatableAmount).Add(delegationInfo.DelegationInfo.DelegationAmounts.WaitUndelegationAmount) |
There was a problem hiding this comment.
this TotalDepositAmount seems referring to the total balance of the staker, not the real total deposit amount, since total_deposit_amount suggests adding the amount of every deposit and getting the final result, which should not change with the delegation(slash) and undelegation process
Description
close #issue312
This PR addresses the issue 312.
GetStakerBalanceByAssetmax_undelegatable_amountto the response of RPCQueryDelegationInfoQueryDelegationInfoSummary by CodeRabbit
New Features
Refactor
Tests