feat(accountplus): Add query for accountplus account state #2659
Conversation
WalkthroughThis pull request introduces a new Changes
Possibly related PRs
Poem
Tip CodeRabbit's docstrings feature is now available as part of our Early Access Program! Simply use the command 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 (
|
| @@ -114,3 +114,30 @@ func CmdQueryGetAllAuthenticators() *cobra.Command { | |||
| flags.AddQueryFlagsToCmd(cmd) | |||
| return cmd | |||
| } | |||
|
|
|||
| func CmdQueryAccountState() *cobra.Command { | |||
There was a problem hiding this comment.
think you need to add this to the GetQueryCmd above
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
proto/dydxprotocol/accountplus/query.proto (1)
40-45: Fix comment prefix in AccountStateResponseThe comment incorrectly uses "Get" prefix while the RPC method is named "AccountState".
-// AccountStateResponse is request type for the Query/AccountState RPC method. +// AccountStateRequest is request type for the Query/AccountState RPC method. -// GetAccountStateResponse is response type for the Query/GetAccountState RPC +// AccountStateResponse is response type for the Query/AccountState RPC // method.protocol/x/accountplus/client/cli/query.go (1)
120-122: Fix typo in command description.There's a typo in the Short description: "Geta account state" should be "Get account state".
- Short: "Geta account state for an address", + Short: "Get account state for an address",indexer/packages/v4-protos/src/codegen/dydxprotocol/accountplus/query.ts (1)
6-41: Fix incorrect comment placement and content.There are several issues with the comments:
- The comment above
AccountStateRequestincorrectly states it's a response type- The same incorrect comment is repeated multiple times
- Comments for
AccountStateResponsemention "GetAccountState" instead of "AccountState"Apply these documentation fixes:
-/** AccountStateResponse is request type for the Query/AccountState RPC method. */ +/** AccountStateRequest is request type for the Query/AccountState RPC method. */ export interface AccountStateRequest { - /** AccountStateResponse is request type for the Query/AccountState RPC method. */ address: string; } -/** AccountStateResponse is request type for the Query/AccountState RPC method. */ +/** AccountStateRequestSDKType is the SDK type for AccountStateRequest */ export interface AccountStateRequestSDKType { - /** AccountStateResponse is request type for the Query/AccountState RPC method. */ address: string; } /** - * GetAccountStateResponse is response type for the Query/GetAccountState RPC + * AccountStateResponse is response type for the Query/AccountState RPC * method. */
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
protocol/x/accountplus/types/query.pb.gois excluded by!**/*.pb.goprotocol/x/accountplus/types/query.pb.gw.gois excluded by!**/*.pb.gw.go
📒 Files selected for processing (6)
indexer/packages/v4-protos/src/codegen/dydxprotocol/accountplus/query.lcd.ts(3 hunks)indexer/packages/v4-protos/src/codegen/dydxprotocol/accountplus/query.rpc.Query.ts(5 hunks)indexer/packages/v4-protos/src/codegen/dydxprotocol/accountplus/query.ts(2 hunks)proto/dydxprotocol/accountplus/query.proto(2 hunks)protocol/x/accountplus/client/cli/query.go(1 hunks)protocol/x/accountplus/keeper/query.go(1 hunks)
🔇 Additional comments (6)
indexer/packages/v4-protos/src/codegen/dydxprotocol/accountplus/query.lcd.ts (1)
2-2: LGTM! Implementation follows established patterns
The LCD client implementation for the account state query is consistent with existing patterns:
- Proper type imports
- Constructor binding
- Endpoint construction matches proto definition
Also applies to: 15-15, 38-44
proto/dydxprotocol/accountplus/query.proto (1)
6-6: LGTM! RPC definition follows best practices
The AccountState RPC method is well-defined with:
- Clear description
- Proper HTTP mapping
- Consistent path structure
Also applies to: 32-37
protocol/x/accountplus/keeper/query.go (1)
71-92: Verify handling of non-existent accounts
The implementation has two potential issues:
- Line 87: The found status from GetAccountState is ignored, which could be useful for distinguishing between non-existent accounts and accounts with empty state
- Line 90: Returning a pointer to accountState could result in nil if the account doesn't exist
Consider either:
- Including the found status in the response, or
- Returning a clear error for non-existent accounts
✅ Verification successful
Let me try a different search pattern to find the relevant usage of GetAccountState.
Let me try one more search to find the implementation of GetAccountState in the keeper.
Based on the implementation found, I can now provide a final response:
Current handling of non-existent accounts is appropriate
The implementation is correct as is, because:
- The underlying
GetAccountStatemethod in the keeper properly handles non-existent accounts by returning(AccountState{}, false) - Returning a pointer to an empty AccountState in the query response is a valid pattern for representing non-existent accounts, as it's consistent with protobuf's handling of optional fields
- The found status can be safely ignored since an empty/zero-valued AccountState already indicates the account doesn't exist
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if GetAccountState is used elsewhere to understand standard error handling
ast-grep --pattern 'GetAccountState($_, $_)'
Length of output: 31298
Script:
#!/bin/bash
# Search for GetAccountState method definition and usage
ast-grep --pattern 'GetAccountState($_, $_)'
rg -A 5 'GetAccountState'
Length of output: 37870
Script:
#!/bin/bash
# Search for GetAccountState implementation
rg -A 10 "func.*GetAccountState"
Length of output: 1492
indexer/packages/v4-protos/src/codegen/dydxprotocol/accountplus/query.rpc.Query.ts (2)
16-18: LGTM! Documentation and method signature are well-defined.
The new accountState method in the Query interface follows the established pattern and includes clear documentation.
49-53: LGTM! Implementation follows the standard RPC pattern.
The implementation correctly:
- Encodes the request
- Makes the RPC call
- Decodes the response
indexer/packages/v4-protos/src/codegen/dydxprotocol/accountplus/query.ts (1)
105-193: LGTM! Encoding and decoding implementation is correct.
The implementation follows the protobuf encoding/decoding pattern consistently:
- Proper handling of optional fields
- Correct wire format encoding
- Proper error handling for unknown fields
|
|
||
| func CmdQueryAccountState() *cobra.Command { | ||
| cmd := &cobra.Command{ | ||
| Use: "account-state [address]", | ||
| Short: "Geta account state for an address", | ||
| Args: cobra.ExactArgs(1), | ||
| RunE: func(cmd *cobra.Command, args []string) error { | ||
| clientCtx, err := client.GetClientQueryContext(cmd) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| queryClient := types.NewQueryClient(clientCtx) | ||
| res, err := queryClient.AccountState( | ||
| context.Background(), | ||
| &types.AccountStateRequest{ | ||
| Address: args[0], | ||
| }, | ||
| ) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| return clientCtx.PrintProto(res) | ||
| }, | ||
| } | ||
| flags.AddQueryFlagsToCmd(cmd) | ||
| return cmd | ||
| } |
There was a problem hiding this comment.
Add command to the GetQueryCmd function.
The new command needs to be added to the command list in the GetQueryCmd function to make it available in the CLI.
Add the following line to the cmd.AddCommand call in GetQueryCmd:
cmd.AddCommand(
CmdQueryParam(),
CmdQueryGetAuthenticator(),
CmdQueryGetAllAuthenticators(),
+ CmdQueryAccountState(),
)Committable suggestion skipped: line range outside the PR's diff.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
proto/dydxprotocol/accountplus/query.proto (2)
32-37: LGTM! Consider adding more detailed documentation.The RPC method is well-defined with a proper REST endpoint mapping. Consider adding more detailed documentation about the expected behavior when the account doesn't exist.
40-45: Consider adding field validation for the address.The message definitions are correct. Consider adding field validation for the address using protobuf field options (e.g., regex pattern for bech32 addresses).
- string address = 1; + string address = 1 [(gogoproto.moretags) = "validate:\"bech32\""];protocol/x/accountplus/keeper/query.go (2)
76-78: Consider using a more specific error message.The error message could be more descriptive to help clients understand what was missing.
- return nil, status.Error(codes.InvalidArgument, "empty request") + return nil, status.Error(codes.InvalidArgument, "account state request cannot be nil")
85-91: Consider documenting the behavior for non-existent accounts.The code correctly handles non-existent accounts by returning an empty state, but this behavior should be documented in the comments.
- // GetAccountState returns `empty, false` AccountState if the account does not exist. + // GetAccountState returns `empty, false` AccountState if the account does not exist. + // Note: We return the empty state to clients when the account doesn't exist instead of an error.indexer/packages/v4-protos/src/codegen/dydxprotocol/accountplus/query.ts (1)
8-41: Consider adding JSDoc comments for better documentation.The type definitions are correct but would benefit from JSDoc comments describing the purpose and usage of each interface.
+/** + * Request type for querying an account's state by address. + * @property address - The bech32-encoded address of the account + */ export interface AccountStateRequest { address: string; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
protocol/x/accountplus/types/query.pb.gois excluded by!**/*.pb.go
📒 Files selected for processing (4)
indexer/packages/v4-protos/src/codegen/dydxprotocol/accountplus/query.ts(2 hunks)proto/dydxprotocol/accountplus/query.proto(2 hunks)protocol/x/accountplus/client/cli/query.go(2 hunks)protocol/x/accountplus/keeper/query.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- protocol/x/accountplus/client/cli/query.go
🔇 Additional comments (4)
proto/dydxprotocol/accountplus/query.proto (1)
6-6: LGTM!
The import for accountplus.proto is correctly added to access the AccountState type.
protocol/x/accountplus/keeper/query.go (1)
80-83: LGTM!
The address validation is properly implemented with appropriate error handling.
indexer/packages/v4-protos/src/codegen/dydxprotocol/accountplus/query.ts (2)
105-109: LGTM!
The base object creation functions properly initialize default values for both request and response types.
Also applies to: 150-154
111-193: LGTM!
The encode/decode methods are correctly implemented with proper handling of optional fields and protobuf wire format.
|
@Mergifyio backport release/protocol/v8.x |
|
@Mergifyio backport release/protocol/v7.x |
✅ Backports have been created
|
✅ Backports have been created
|
(cherry picked from commit 5fd53fc) # Conflicts: # protocol/x/accountplus/client/cli/query.go
(cherry picked from commit 5fd53fc)
…2659) (#2661) Co-authored-by: Teddy Ding <[email protected]>
|
@Mergifyio backport jy/v8_upgrade_test |
✅ Backports have been created
|
Changelist
Add query for
x/accountplusaccount stateTest Plan
Testing on localnet
Author/Reviewer Checklist
state-breakinglabel.indexer-postgres-breakinglabel.PrepareProposalorProcessProposal, manually add the labelproposal-breaking.feature:[feature-name].backport/[branch-name].refactor,chore,bug.Summary by CodeRabbit
New Features
Keeperstruct.LCDQueryClientwith an asynchronous method to query account states.Bug Fixes
Documentation