feat(client): add dynamic prompt (partial cherry-pick #22775) #24378
Conversation
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
📝 WalkthroughWalkthroughThis pull request introduces interactive command-line prompt functionality to improve user input handling for protobuf messages and structs. It adds new helper and utility functions for field prompting, validation, and defaulting, along with comprehensive tests. Additionally, it defines new Protocol Buffers services and messages, and refactors existing CLI proposal functionalities to use the new prompt utilities. Deprecated methods and tests have been removed, and the codebase is reorganized to avoid circular dependencies while enhancing user experience. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant CLI
participant PromptFunction
participant HelperFunction
participant Validator
User->>CLI: Initiate message prompt
CLI->>PromptFunction: Call PromptMessage()
PromptFunction->>HelperFunction: Iterate over message fields
HelperFunction->>Validator: Validate field input (e.g., addresses, types)
Validator-->>HelperFunction: Return validation result
HelperFunction-->>PromptFunction: Return valid field value
PromptFunction-->>CLI: Return constructed protobuf message
CLI-->>User: Display populated message or error feedback
sequenceDiagram
participant User
participant CLI
participant LookupRegistry
participant ProposalPrompter
User->>CLI: Start drafting proposal
CLI->>LookupRegistry: Lookup message type using MsgType
LookupRegistry-->>CLI: Return message type
CLI->>ProposalPrompter: Call PromptMessage() with proper codecs
ProposalPrompter->>User: Request input for proposal fields
User-->>ProposalPrompter: Provide field inputs
ProposalPrompter-->>CLI: Return populated proposal message
CLI-->>User: Proceed with proposal submission
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 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: 0
♻️ Duplicate comments (1)
client/prompt/struct.go (1)
8-8: Duplicate comment on potential non-determinism with “os” import.A previous review comment warned that importing system packages like “os” can introduce non-determinism. While this is standard for interactive CLI usage, ensure that you’re comfortable with any nondeterministic behavior inherent to reading from standard input.
🧹 Nitpick comments (14)
client/prompt/validation_test.go (1)
20-48: LGTM! Comprehensive address validation testing.The test uses a table-driven approach to verify that different types of addresses (regular, validator, consensus) are correctly validated with their corresponding codecs.
Consider adding negative test cases to verify that invalid addresses properly return errors.
func TestValidateAddress(t *testing.T) { tests := []struct { name string ac address.Codec addr string + expectError bool }{ { name: "address", ac: address2.NewBech32Codec("cosmos"), addr: "cosmos129lxcu2n3hx54fdxlwsahqkjr3sp32cxm00zlm", + expectError: false, }, + { + name: "invalid address", + ac: address2.NewBech32Codec("cosmos"), + addr: "invalid_address", + expectError: true, + }, // ... other test cases } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { err := ValidateAddress(tt.ac)(tt.addr) - require.NoError(t, err) + if tt.expectError { + require.Error(t, err) + } else { + require.NoError(t, err) + } }) } }client/prompt/struct_test.go (2)
25-27: Consider adding test coverage for invalid or edge-case user inputs.These doc comments explain the test objectives well, but the current test only verifies a pre-filled struct with no actual user input. You might want to add tests for scenarios such as invalid integer parsing, empty required fields, or multi-line input to ensure thorough coverage.
28-62: Looks good overall; consider expanding scenarios.The test effectively checks that default/prefilled values remain intact when no user input is provided. Consider adding broader test scenarios—such as interactive input, negative testing, or empty slices—to further validate the robustness of
promptStruct.client/prompt/struct.go (3)
75-77: Consider gracefully handling unsupported slice element types.Currently, slices are skipped if they don’t contain strings or ints. It may be helpful to warn the user or return an error in such cases rather than silently ignoring them.
95-98: Potentially inconsistent behavior for empty input in test scenarios.When
stdInis non-nil and returns an empty string plus EOF, the code proceeds to the next field without re-prompting. This differs from the case when reading from real stdin, which tries again if input is empty. Confusion or unexpected results might arise in automated tests unless documented or aligned.
117-155: Multiple entries in slices may be desired.Currently, each slice field is populated with only one element from a single line of user input. Depending on your needs, consider extending this logic to allow multiple entries and dynamic expansions from user input (e.g., prompting until the user types an empty line).
client/prompt/util.go (1)
86-119: Consider extending SetDefaults to support more protobuf field types.The SetDefaults function provides a useful mechanism for setting default values on protobuf messages. Currently, it handles string, int64, int32, and bool types.
Consider extending the implementation to support more protobuf field types such as uint32, uint64, float32, float64, bytes, and possibly nested messages. This would make the function more versatile for complex protobuf messages.
func SetDefaults(msg protoreflect.Message, defaults map[string]interface{}) { fields := msg.Descriptor().Fields() for i := 0; i < fields.Len(); i++ { field := fields.Get(i) fieldName := string(field.Name()) if v, ok := defaults[fieldName]; ok { // Get the field's kind fieldKind := field.Kind() switch v.(type) { case string: if fieldKind == protoreflect.StringKind { msg.Set(field, protoreflect.ValueOf(v)) } case int64: if fieldKind == protoreflect.Int64Kind { msg.Set(field, protoreflect.ValueOf(v)) } case int32: if fieldKind == protoreflect.Int32Kind { msg.Set(field, protoreflect.ValueOf(v)) } case bool: if fieldKind == protoreflect.BoolKind { msg.Set(field, protoreflect.ValueOf(v)) } + case uint32: + if fieldKind == protoreflect.Uint32Kind { + msg.Set(field, protoreflect.ValueOf(v)) + } + case uint64: + if fieldKind == protoreflect.Uint64Kind { + msg.Set(field, protoreflect.ValueOf(v)) + } + case float32: + if fieldKind == protoreflect.FloatKind { + msg.Set(field, protoreflect.ValueOf(v)) + } + case float64: + if fieldKind == protoreflect.DoubleKind { + msg.Set(field, protoreflect.ValueOf(v)) + } + case []byte: + if fieldKind == protoreflect.BytesKind { + msg.Set(field, protoreflect.ValueOf(v)) + } } } } }x/gov/client/cli/prompt.go (4)
57-60: Deprecation notice is well-defined.
Marking the oldPromptfunction as deprecated and returning an error clarifies intended usage. This helps guide users towardclient/promptfunctionality. Remember to remove it in the future when you confirm no references remain.You could optionally mark it with a Go doc comment like
// Deprecated: ...to ensure IDEs and other tools highlight this more explicitly.
88-89: Prompting for deposit is consistent.
Usingprompt.PromptString("Enter proposal deposit", prompt.ValidatePromptCoins)is clear and consistent with other prompt usage. Consider adding a short help text or example deposit format (e.g., "100stake") for clarity in interactive mode.
122-133: Bridging from google proto to gogoproto.
The sequence ofproto.Marshal→gogoproto.Unmarshalis effective, but ensure these message types are wire-compatible. A mismatch can cause subtle data corruption.You could add a quick check to confirm the message type matches the expected wire type for
p.Msgbefore unmarshal.
203-207: Listing available Msg implementations.
Providing a dynamic list fromInterfaceRegistry.ListImplementations(sdk.MsgInterfaceProtoName)is a safe way to guide the user. Consider whether you want to filter out commonly irrelevant or low-level message types to reduce clutter.client/prompt/message.go (3)
27-140: Robust interactive prompt for message fields.
This core logic properly distinguishes between scalar fields, repeated fields, and nested messages. Consider these edge cases:
• Cyclical references in proto definitions could lead to infinite recursion.
• Fields of map type are not yet handled.
• Thei--approach for re-prompting on validation errors can result in indefinite retries. Usually this is acceptable, but be aware of potential user confusion if validation never passes.A possible enhancement is to track the number of retries or to provide a clear “Abort” path for repeated failures.
142-194: Handles scalar field conversion well.
This switch covers string, various int types, bool, bytes, and enums. Be mindful that float/double and other well-known types (e.g.,Duration,Timestamp) aren’t yet supported.Add a TODO or handle float/double conversions if your use cases require it.
250-346: Nested and repeated message fields are well-handled.
Recursively invokingpromptMessagefor message kinds and implementing a repeated “continue adding?” flow is user-friendly. Just be aware of potential indefinite recursion if the user keeps adding nested messages in certain proto designs.If you foresee large or deeply nested messages, consider a more structured UI flow or partial expansions to mitigate complexity.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (18)
client/prompt/internal/testpb/msg.pulsar.gois excluded by!**/*.pulsar.goclient/prompt/internal/testpb/msg_grpc.pb.gois excluded by!**/*.pb.go,!**/*.pb.goclient/prompt/internal/testpb/query.pulsar.gois excluded by!**/*.pulsar.goclient/prompt/internal/testpb/query_grpc.pb.gois excluded by!**/*.pb.go,!**/*.pb.goclient/v2/go.modis excluded by!**/*.modclient/v2/go.sumis excluded by!**/*.sum,!**/*.sumgo.modis excluded by!**/*.modgo.sumis excluded by!**/*.sum,!**/*.sumsimapp/go.modis excluded by!**/*.modsimapp/go.sumis excluded by!**/*.sum,!**/*.sumtests/go.modis excluded by!**/*.modtests/go.sumis excluded by!**/*.sum,!**/*.sumtests/systemtests/go.sumis excluded by!**/*.sum,!**/*.sumx/evidence/go.sumis excluded by!**/*.sum,!**/*.sumx/feegrant/go.modis excluded by!**/*.modx/feegrant/go.sumis excluded by!**/*.sum,!**/*.sumx/upgrade/go.modis excluded by!**/*.modx/upgrade/go.sumis excluded by!**/*.sum,!**/*.sum
📒 Files selected for processing (15)
CHANGELOG.md(1 hunks)client/prompt/internal/helpers.go(1 hunks)client/prompt/internal/testpb/msg.proto(1 hunks)client/prompt/internal/testpb/query.proto(1 hunks)client/prompt/message.go(1 hunks)client/prompt/message_test.go(1 hunks)client/prompt/struct.go(1 hunks)client/prompt/struct_test.go(1 hunks)client/prompt/util.go(1 hunks)client/prompt/validation.go(3 hunks)client/prompt/validation_test.go(1 hunks)client/prompt_validation_test.go(0 hunks)x/gov/client/cli/prompt.go(8 hunks)x/gov/client/cli/prompt_test.go(0 hunks)x/group/client/cli/prompt.go(6 hunks)
💤 Files with no reviewable changes (2)
- client/prompt_validation_test.go
- x/gov/client/cli/prompt_test.go
🧰 Additional context used
🧬 Code Definitions (2)
client/prompt/validation_test.go (1)
client/prompt/validation.go (3)
ValidatePromptNotEmpty(14-20)ValidateAddress(23-31)ValidatePromptURL(34-41)
client/prompt/message.go (2)
client/prompt/validation.go (2)
ValidatePromptNotEmpty(14-20)ValidateAddress(23-31)client/prompt/internal/helpers.go (5)
GetSignerFieldName(28-35)GetScalarType(19-23)AddressStringScalarType(12-12)ValidatorAddressStringScalarType(13-13)ConsensusAddressStringScalarType(14-14)
🪛 Buf (1.47.2)
client/prompt/internal/testpb/query.proto
7-7: import "cosmos_proto/cosmos.proto": file does not exist
(COMPILE)
client/prompt/internal/testpb/msg.proto
7-7: import "cosmos_proto/cosmos.proto": file does not exist
(COMPILE)
⏰ Context from checks skipped due to timeout of 90000ms (18)
- GitHub Check: test-system-legacy
- GitHub Check: test-system
- GitHub Check: tests (03)
- GitHub Check: tests (02)
- GitHub Check: tests (01)
- GitHub Check: tests (00)
- GitHub Check: test-x-upgrade
- GitHub Check: test-x-feegrant
- GitHub Check: test-x-evidence
- GitHub Check: test-sim-nondeterminism
- GitHub Check: test-integration
- GitHub Check: test-e2e
- GitHub Check: build (arm64)
- GitHub Check: build (amd64)
- GitHub Check: Analyze
- GitHub Check: golangci-lint
- GitHub Check: Gosec
- GitHub Check: Summary
🔇 Additional comments (24)
CHANGELOG.md (1)
77-77: LGTM! Clear documentation for the new feature.The changelog entry clearly describes the new interactive autocli prompt functionality that has been added, which includes message field prompting, validation helpers, and default value support.
client/prompt/message_test.go (2)
15-19: LGTM! Good utility function for test input simulation.The
getReaderhelper function provides a clean way to simulate user input for testing by converting a slice of strings into anio.ReadCloser.
21-58: LGTM! Comprehensive test coverage for message prompting.The test function effectively verifies that the
promptMessagefunction can handle a variety of inputs when populating protobuf messages. The test structure is clear and can be easily extended with additional test cases if needed.Consider adding more test cases with different message types or edge cases in the future to strengthen test coverage.
client/prompt/validation_test.go (2)
13-18: LGTM! Good test coverage for empty validation.The test properly verifies both the successful and error cases for the
ValidatePromptNotEmptyfunction.
50-55: LGTM! URL validation testing is complete.The test properly covers both valid and invalid URL cases for the
ValidatePromptURLfunction.client/prompt/validation.go (2)
22-31: Well-designed validation function for addresses.The new
ValidateAddressfunction provides a flexible way to validate different types of addresses using the provided address codec. This is an improvement over the deprecated function as it can handle any type of address with the appropriate codec.
43-46: Appropriate deprecation notice.Good practice to mark the function as deprecated with a clear comment explaining why it's deprecated and when it will be removed. This helps users migrate to the new
ValidateAddressfunction.client/prompt/internal/testpb/query.proto (2)
7-7: Verify the existence of imported file.Static analysis indicates that
cosmos_proto/cosmos.protomay not exist in the codebase. Ensure it's included or otherwise accessible, since this file is required for compiling the proto definitions referencing cosmos scalar annotations.🧰 Tools
🪛 Buf (1.47.2)
7-7: import "cosmos_proto/cosmos.proto": file does not exist
(COMPILE)
55-61: Confirm negative enum value usage under proto3.The negative enumeration value
ENUM_NEG_THREE = -3;is unusual. Double-check that this is valid and supported in your environment, as some code generators may not allow negative enum values in proto3.client/prompt/internal/helpers.go (1)
1-36: Well-structured helper functions for protobuf descriptor access.This new internal package provides well-documented utility functions to extract scalar types and signer field information from protobuf descriptors, which supports the PR objective of avoiding circular dependencies. The constants for address types and helper functions are clearly documented with their purpose.
x/group/client/cli/prompt.go (3)
31-35: Improved proposalType struct with explicit MsgType field.The addition of the MsgType field to the proposalType struct enhances clarity by explicitly separating the message type identifier from the message instance itself. This separation aligns with the PR's goal of improving the CLI prompting functionality.
39-102: Enhanced prompt implementation with better type handling.The updated Prompt method now properly handles address validation using the new prompt package and correctly manages the conversion between different protobuf implementations. The approach of using protoregistry to find message types by URL is more robust than the previous implementation.
I particularly like the explicit error handling at each step of the process, which will make debugging easier.
The conversion between proto and gogoproto (lines 87-96) is a potential area for subtle bugs. Can you verify this works correctly with different message types by testing the command with various proposal types?
122-143: Improved selection UI with new prompt package.The implementation now uses the new prompt.Select function instead of the previous selection mechanism, providing a consistent user experience across the SDK. The approach of listing all available message types and allowing selection by number or text is user-friendly.
client/prompt/util.go (2)
14-59: Well-implemented selection prompt with robust input handling.The Select function and its helper parseSelection provide a clean interface for user selection with good error handling and validation. The implementation correctly handles both numeric selection and exact text matching, creating a flexible user experience.
61-84: Clean implementation of string input with validation.The PromptString function provides a simple yet effective way to get validated string input from users. The loop for retrying on invalid input ensures robustness.
client/prompt/internal/testpb/msg.proto (2)
1-62: Well-structured test proto file with comprehensive field types.This test proto file provides excellent coverage of different field types and scenarios that the prompt system needs to handle. The inclusion of various field types (primitives, complex types, custom messages), annotations, and special cases (deprecated fields, hidden fields) will ensure thorough testing of the prompt functionality.
🧰 Tools
🪛 Buf (1.47.2)
7-7: import "cosmos_proto/cosmos.proto": file does not exist
(COMPILE)
7-7:❓ Verification inconclusive
Verify the cosmos_proto import path.
The static analysis tool flagged that the import "cosmos_proto/cosmos.proto" file does not exist. This might cause build issues.
Please verify that the import path is correct. You might need to update it to the correct location of the cosmos_proto package.
🏁 Script executed:
#!/bin/bash # Check if cosmos_proto/cosmos.proto can be found in the project find . -name "cosmos.proto" | grep -i cosmos_protoLength of output: 50
Action Required: Verify the Validity of the
cosmos_protoImport PathOur initial search for a file matching
"cosmos_proto/cosmos.proto"did not return any results. This could mean either that the file is missing from the repository, it exists under a different location than expected, or it is meant to be provided as part of an external dependency. Please verify the following:
- Confirm whether the
cosmos_protopackage is expected to be part of your repository or managed externally (e.g., via a package manager).- If the package should be available internally, check if it resides under a different directory or path.
- Update the import in
client/prompt/internal/testpb/msg.protoaccordingly if the correct path differs from"cosmos_proto/cosmos.proto".🧰 Tools
🪛 Buf (1.47.2)
7-7: import "cosmos_proto/cosmos.proto": file does not exist
(COMPILE)
x/gov/client/cli/prompt.go (5)
5-5: Imports look fine.
These newly introduced imports correctly bridge between gogoproto and google proto types, and facilitate the usage of the newclient/promptpackage. No immediate concerns with dependency conflicts or version mismatches.Also applies to: 11-14, 18-18, 20-20, 22-22
99-117: Ensure authority field compatibility.
Retrieving the proposal message viaFindMessageByURLand setting a default"authority"field is a neat approach. However, confirm that every msg type includes anauthorityfield—otherwise, your default might silently fail or overwrite an unrelated field.Would you like to integrate a check that verifies the presence of the
authorityfield before setting defaults?
189-190: Good use of prompt.Select for proposal type.
Selecting from a known list of types is straightforward. The subsequent loop matching the selection is clear.
222-224: Skip metadata flag usage appears correct.
The combination ofskip-metadatalogic withproposal.Promptproperly bypasses metadata prompts. Double-check that no necessary metadata is lost when skipping.
71-75:✅ Verification successful
Recreating address codecs from global SDK config.
Re-initializing codecs here seems logical to avoid potential stale references. However, confirm that related modules or test setups do not also reconfigure them, risking inconsistent setups.
🏁 Script executed:
#!/bin/bash # Description: Search for other references to address.NewBech32Codec usage # to verify no conflicting re-initializations exist. rg -A 3 "NewBech32Codec"Length of output: 72491
Global SDK-Based Codec Reinitialization Validated
The instantiation of address codecs in this file—from the SDK global config—ensures that stale references are avoided. An extensive search across the codebase shows that while many test files and modules create codecs directly with fixed prefixes, there is no evidence of conflicting reconfigurations that would lead to inconsistent setups. This approach in the gov client (x/gov/client/cli/prompt.go) is consistent with the design intent of using the current SDK config for dynamic prefix assignment.client/prompt/message.go (2)
18-25: Simple pass-through to promptMessage.
This top-levelPromptMessagefunction is clean and self-explanatory. Good separation of concerns by delegating the main logic topromptMessage.
196-248: Comma-separated approach for repeated fields.
The approach works for simple or short lists. For large or multiline inputs, this might limit usability. Currently, the code gracefully ignores empty items.
| "fmt" | ||
| "io" | ||
| "os" | ||
| "reflect" |
Check notice
Code scanning / CodeQL
Sensitive package import Note
|
overall looks good - how can we test this? |
Easiest way is to run Previously, the prompt will not prompt you the inner message, or the complexer structure but would have just skipped it. |
|
@julienrbrt conflicts |
Description
Partial cherry-pick of #22775
Instead of adding the new prompt logic to client/v2, it is added in client directly.
bf7194f
This was due to avoid circular dependencies (otherwise we'd need to import client/v2 in the SDK).
Additionally breaking changes have been reverted and unused APIs are simply deprecated.
Lastly, removed the promptui lib bf7194f, as it is quite buggy (data races, double inputs, etc..).
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 changeCHANGELOG.mdReviewers 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.
Please see Pull Request Reviewer section in the contributing guide for more information on how to review a pull request.
I have...