Add permissioned keys cli #2644
Conversation
WalkthroughThe changes introduce new command-line interface (CLI) functionalities for the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
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: 4
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
protocol/x/accountplus/client/cli/query.go(1 hunks)protocol/x/accountplus/client/cli/tx.go(1 hunks)protocol/x/accountplus/module.go(2 hunks)
🔇 Additional comments (2)
protocol/x/accountplus/module.go (2)
75-75: LGTM: CLI commands properly integrated
The integration of CLI commands follows the standard Cosmos SDK module patterns.
Also applies to: 81-81
Line range hint 1-112: Verify command naming and documentation consistency across the module
Let's ensure the command naming and documentation are consistent across the codebase.
✅ Verification successful
The previous results showed some command patterns, but we need a broader view of CLI commands across the codebase to verify consistency. This script will help us analyze the command naming patterns, documentation styles, and argument handling across all CLI files in the codebase.
Command naming and documentation patterns are consistent across the module
Based on the comprehensive analysis of CLI commands across the codebase:
-
The
accountplusmodule's command naming follows consistent patterns:- Query commands use the
get-prefix (e.g.,get-authenticator,get-all-authenticators) - Action commands use clear verbs (e.g.,
add-authenticator,remove-authenticator)
- Query commands use the
-
Documentation is complete and follows consistent patterns:
- All commands have clear
UseandShortdescriptions - Arguments are properly defined using
cobra.ExactArgs - Command structure aligns with other modules in the codebase (e.g., similar to affiliate, vault, and other modules)
- All commands have clear
-
The command argument handling is consistent:
- Uses
cobra.ExactArgswith appropriate argument counts - Follows the same pattern as other modules in the codebase
- Uses
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for consistent command naming and documentation patterns
echo "Checking command naming patterns..."
rg -A 2 'Use:.*authenticator'
echo "Checking for command documentation..."
rg 'Short:.*authenticator'
echo "Checking for similar commands in other modules..."
rg -g '*/client/cli/*.go' 'func Cmd.*Authenticator'
Length of output: 1866
Script:
#!/bin/bash
# Let's verify command naming patterns more comprehensively
echo "Checking command naming patterns in CLI files..."
fd -e go -p 'cli' --exec rg -A 1 'Use:.*".*"' {}
echo "Checking command documentation patterns..."
fd -e go -p 'cli' --exec rg -A 1 'Short:.*".*"' {}
echo "Checking command argument patterns..."
fd -e go -p 'cli' --exec rg 'Args:\s+cobra.ExactArgs' {}
Length of output: 20686
| func CmdAddAuthenticator() *cobra.Command { | ||
| cmd := &cobra.Command{ | ||
| Use: "add-authenticator account authenticator_type data", | ||
| Short: "Registers an authenticator", | ||
| Args: cobra.ExactArgs(3), | ||
| RunE: func(cmd *cobra.Command, args []string) error { | ||
| clientCtx, err := client.GetClientTxContext(cmd) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| msg := types.MsgAddAuthenticator{ | ||
| Sender: args[0], | ||
| AuthenticatorType: args[1], | ||
| Data: []byte(args[2]), | ||
| } | ||
| return tx.GenerateOrBroadcastTxCLI(clientCtx, cmd.Flags(), &msg) | ||
| }, | ||
| } | ||
| flags.AddTxFlagsToCmd(cmd) | ||
| return cmd | ||
| } |
There was a problem hiding this comment.
Add input validation for authenticator_type and data parameters
The command accepts raw input for authenticator_type and data without validation. This could lead to security issues if malformed or malicious data is provided.
Consider:
- Validating authenticator_type against a predefined list of allowed types
- Adding size limits and format validation for the data parameter
- Adding documentation about accepted formats
func CmdAddAuthenticator() *cobra.Command {
cmd := &cobra.Command{
Use: "add-authenticator account authenticator_type data",
- Short: "Registers an authenticator",
+ Short: "Registers an authenticator with the specified type and data",
+ Long: `Registers an authenticator for an account.
+Supported authenticator types: [list supported types]
+Data format: [specify format requirements]`,
Args: cobra.ExactArgs(3),
RunE: func(cmd *cobra.Command, args []string) error {
clientCtx, err := client.GetClientTxContext(cmd)
if err != nil {
return err
}
+ // Validate authenticator type
+ if !types.IsValidAuthenticatorType(args[1]) {
+ return fmt.Errorf("invalid authenticator type: %s", args[1])
+ }
+ // Validate data format and size
+ if err := types.ValidateAuthenticatorData(args[2]); err != nil {
+ return fmt.Errorf("invalid authenticator data: %v", err)
+ }
msg := types.MsgAddAuthenticator{
Sender: args[0],
AuthenticatorType: args[1],Committable suggestion skipped: line range outside the PR's diff.
| func CmdRemoveAuthenticator() *cobra.Command { | ||
| cmd := &cobra.Command{ | ||
| Use: "remove-authenticator [sender] [authenticator_id]", | ||
| Short: "Removes an authenticator", | ||
| Args: cobra.ExactArgs(2), | ||
| RunE: func(cmd *cobra.Command, args []string) error { | ||
| clientCtx, err := client.GetClientTxContext(cmd) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| id, err := cast.ToUint64E(args[1]) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| msg := types.MsgRemoveAuthenticator{ | ||
| Sender: args[0], | ||
| Id: id, | ||
| } | ||
| return tx.GenerateOrBroadcastTxCLI(clientCtx, cmd.Flags(), &msg) | ||
| }, | ||
| } | ||
| flags.AddTxFlagsToCmd(cmd) | ||
| return cmd | ||
| } |
There was a problem hiding this comment.
Add confirmation prompt for authenticator removal
Removing an authenticator is a sensitive operation that should require confirmation to prevent accidental removals.
Add a confirmation prompt using the SDK's input package:
func CmdRemoveAuthenticator() *cobra.Command {
cmd := &cobra.Command{
Use: "remove-authenticator [sender] [authenticator_id]",
- Short: "Removes an authenticator",
+ Short: "Removes an authenticator (requires confirmation)",
Args: cobra.ExactArgs(2),
RunE: func(cmd *cobra.Command, args []string) error {
clientCtx, err := client.GetClientTxContext(cmd)
if err != nil {
return err
}
id, err := cast.ToUint64E(args[1])
if err != nil {
return err
}
+ buf := bufio.NewReader(cmd.InOrStdin())
+ prompt := fmt.Sprintf("Are you sure you want to remove authenticator %d? (y/N): ", id)
+ if !input.GetConfirmation(prompt, buf) {
+ return fmt.Errorf("operation cancelled")
+ }
msg := types.MsgRemoveAuthenticator{Committable suggestion skipped: line range outside the PR's diff.
| func CmdQueryGetAuthenticator() *cobra.Command { | ||
| cmd := &cobra.Command{ | ||
| Use: "get-authenticator [account] [authenticator_id]", | ||
| Short: "Get authenticator", | ||
| Args: cobra.ExactArgs(3), | ||
| RunE: func(cmd *cobra.Command, args []string) error { | ||
| clientCtx, err := client.GetClientQueryContext(cmd) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| id, err := cast.ToUint64E(args[1]) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| queryClient := types.NewQueryClient(clientCtx) | ||
| res, err := queryClient.GetAuthenticator( | ||
| context.Background(), | ||
| &types.GetAuthenticatorRequest{ | ||
| Account: args[0], | ||
| AuthenticatorId: id, | ||
| }, | ||
| ) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| return clientCtx.PrintProto(res) | ||
| }, | ||
| } | ||
| return cmd | ||
| } |
There was a problem hiding this comment.
Fix argument count mismatch in get-authenticator command
The command's Use string indicates it needs 2 arguments, but Args: cobra.ExactArgs(3) requires 3 arguments.
Fix the argument count:
func CmdQueryGetAuthenticator() *cobra.Command {
cmd := &cobra.Command{
Use: "get-authenticator [account] [authenticator_id]",
Short: "Get authenticator",
- Args: cobra.ExactArgs(3),
+ Args: cobra.ExactArgs(2),📝 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 CmdQueryGetAuthenticator() *cobra.Command { | |
| cmd := &cobra.Command{ | |
| Use: "get-authenticator [account] [authenticator_id]", | |
| Short: "Get authenticator", | |
| Args: cobra.ExactArgs(3), | |
| RunE: func(cmd *cobra.Command, args []string) error { | |
| clientCtx, err := client.GetClientQueryContext(cmd) | |
| if err != nil { | |
| return err | |
| } | |
| id, err := cast.ToUint64E(args[1]) | |
| if err != nil { | |
| return err | |
| } | |
| queryClient := types.NewQueryClient(clientCtx) | |
| res, err := queryClient.GetAuthenticator( | |
| context.Background(), | |
| &types.GetAuthenticatorRequest{ | |
| Account: args[0], | |
| AuthenticatorId: id, | |
| }, | |
| ) | |
| if err != nil { | |
| return err | |
| } | |
| return clientCtx.PrintProto(res) | |
| }, | |
| } | |
| return cmd | |
| } | |
| func CmdQueryGetAuthenticator() *cobra.Command { | |
| cmd := &cobra.Command{ | |
| Use: "get-authenticator [account] [authenticator_id]", | |
| Short: "Get authenticator", | |
| Args: cobra.ExactArgs(2), | |
| RunE: func(cmd *cobra.Command, args []string) error { | |
| clientCtx, err := client.GetClientQueryContext(cmd) | |
| if err != nil { | |
| return err | |
| } | |
| id, err := cast.ToUint64E(args[1]) | |
| if err != nil { | |
| return err | |
| } | |
| queryClient := types.NewQueryClient(clientCtx) | |
| res, err := queryClient.GetAuthenticator( | |
| context.Background(), | |
| &types.GetAuthenticatorRequest{ | |
| Account: args[0], | |
| AuthenticatorId: id, | |
| }, | |
| ) | |
| if err != nil { | |
| return err | |
| } | |
| return clientCtx.PrintProto(res) | |
| }, | |
| } | |
| return cmd | |
| } |
| func CmdQueryGetAllAuthenticators() *cobra.Command { | ||
| cmd := &cobra.Command{ | ||
| Use: "get-all-authenticators [account]", | ||
| Short: "Get all authenticators for an account", | ||
| 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.GetAuthenticators( | ||
| context.Background(), | ||
| &types.GetAuthenticatorsRequest{ | ||
| Account: args[0], | ||
| }, | ||
| ) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| return clientCtx.PrintProto(res) | ||
| }, | ||
| } | ||
| return cmd | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add pagination support for get-all-authenticators command
For accounts with many authenticators, the response should be paginated to prevent overwhelming responses.
Add pagination flags and logic:
func CmdQueryGetAllAuthenticators() *cobra.Command {
cmd := &cobra.Command{
Use: "get-all-authenticators [account]",
Short: "Get all authenticators for an account",
Args: cobra.ExactArgs(1),
RunE: func(cmd *cobra.Command, args []string) error {
clientCtx, err := client.GetClientQueryContext(cmd)
if err != nil {
return err
}
+ pageReq, err := client.ReadPageRequest(cmd.Flags())
+ if err != nil {
+ return err
+ }
queryClient := types.NewQueryClient(clientCtx)
res, err := queryClient.GetAuthenticators(
context.Background(),
&types.GetAuthenticatorsRequest{
Account: args[0],
+ Pagination: pageReq,
},
)
if err != nil {
return err
}
return clientCtx.PrintProto(res)
},
}
+ flags.AddPaginationFlagsToCmd(cmd, "authenticators")
return cmd
}Committable suggestion skipped: line range outside the PR's diff.
|
@Mergifyio backport release/protocol/v8.x |
✅ Backports have been created
|
(cherry picked from commit 03c53ce)
Co-authored-by: jayy04 <[email protected]>
Changelist
[Describe or list the changes made in this PR]
Test Plan
[Describe how this PR was tested (if applicable)]
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
Bug Fixes
Documentation