feat(client/v2)!: dynamic prompt #22775
Conversation
JulianToledano
requested review from
a team,
kocubinski,
akhilkumarpilli,
julienrbrt and
sontrinh16
as code owners
📝 Walkthrough📝 WalkthroughWalkthroughThis pull request introduces a comprehensive set of features and modifications to the Changes
Suggested labels
Suggested reviewers
Possibly related PRs
📜 Recent review detailsConfiguration used: .coderabbit.yml 📒 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
Documentation and Community
|
This comment has been minimized.
This comment has been minimized.
![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
🧹 Outside diff range and nitpick comments (14)
client/v2/autocli/prompt/struct.go (2)
84-84: Avoid suppressing linter warnings with// nolintcommentsUsing
// nolintcomments can hide potential issues and is discouraged. Sincestrings.Titleis deprecated, it's better to refactor the code to address the warning rather than suppressing it.
31-37: Simplify reflection logic for improved readabilityThe reflection handling in lines 31-37 is complex and may affect maintainability. Consider adding comments to explain each step or refactor to simplify the logic, making the code more understandable for future contributors.
client/v2/autocli/prompt/struct_test.go (1)
23-45: Enhance unit test coverage with additional test casesCurrently,
TestPromptStructcovers a basic scenario. To ensure robust testing:
- Add test cases with unsupported field types to verify they are skipped appropriately.
- Include inputs that should produce errors, such as invalid integers.
- Test with nested structs and nil pointer fields.
- Verify behavior with slices containing multiple values.
x/group/client/cli/prompt.go (3)
39-39: Consider refactoring parameters into a struct for clarityThe
Promptfunction now accepts multipleaddress.Codecparameters (addressCodec,validatorAddressCodec,consensusAddressCodec). Passing several related parameters can make the function signature lengthy and harder to maintain. Consider grouping these codecs into a single struct or interface to enhance readability and maintainability.
82-91: Evaluate the necessity of marshaling betweenprotoandgogoprotoThe code serializes the
resultmessage usingproto.Marshaland then unmarshals it withgogoproto.Unmarshalintop.Msgto retain the@typeinformation. This conversion between different protobuf implementations may introduce unnecessary complexity and potential performance overhead. Consider exploring if there's a way to preserve the type information without this conversion, perhaps by standardizing on one protobuf library or adjusting the serialization approach.
Line range hint
53-66: Useprompt.ValidateAddressfor consistent address validationThe validation for addresses is currently using
client.ValidatePromptAddress. Since thepromptpackage introduces a newValidateAddressfunction inclient/v2/internal/prompt/validation.go, consider using it to maintain consistency and leverage the updated validation logic throughout the codebase.x/gov/client/cli/prompt.go (2)
71-71: Refactor parameters into a struct to simplify the function signatureThe
Promptfunction now includes multipleaddress.Codecparameters. To improve code readability and ease of maintenance, consider grouping these parameters into a struct or interface, especially if they are frequently used together.
111-121: Review the need for marshaling between different protobuf implementationsSimilar to changes in other files, there's a conversion from
prototogogoprototo retain the@typeinformation. This adds complexity and may affect performance. Consider whether it's necessary to convert between these protobuf implementations or if there's an alternative method to maintain the type information without this overhead.client/v2/autocli/prompt/message.go (3)
197-204: Remove commented-out code to keep the codebase cleanLines 198~ to 203~ contain commented-out code. Keeping such code can clutter the codebase and lead to confusion. If this code is no longer needed, consider removing it to maintain clarity.
🧰 Tools
🪛 golangci-lint (1.62.2)
200-200: commentFormatting: put a space between
//and comment text(gocritic)
248-262: Improve the continuation prompt for better user experienceThe continuation prompt in
promptMessageListasks "Add another item?" with options "No" and "Yes". As per the TODO comment, switching to a simple yes/no prompt could enhance the user experience by making it more intuitive. Consider implementing a direct yes/no prompt to streamline user interaction.
147-150: Handle unimplemented field kinds explicitlyIn the
valueOffunction, unhandled field kinds result in returning an emptyprotoreflect.Valuewithout an error. This silent failure could lead to unexpected behavior. Consider handling additional field kinds or returning an informative error when encountering unimplemented types to improve robustness.client/v2/internal/prompt/validation.go (1)
46-46: Adjust the error message for generic address validationThe error message "invalid consensus address" may not be appropriate, as the
ValidateAddressfunction validates addresses of various types based on the provided codec. Consider changing the error message to "invalid address" or including the specific address type for clarity.client/v2/autocli/prompt/util.go (1)
50-68: ExtendSetDefaultsto handle additional field typesThe
SetDefaultsfunction currently handlesstring,int64,int32, andbooltypes. If there are other field types that could benefit from default values, consider extending the function to handle them. This will make the function more robust and versatile for various protobuf messages.client/v2/autocli/prompt/message_test.go (1)
16-24: Consider improving readability of the padding calculationThe padding logic handles a known promptui issue correctly, but the magic number
4096could be made more maintainable.Consider this improvement:
func getReader(inputs []string) io.ReadCloser { + const paddingBlockSize = 4096 var paddedInputs []string for _, input := range inputs { - padding := strings.Repeat("a", 4096-1-len(input)%4096) + remainingBytes := paddingBlockSize - 1 - len(input)%paddingBlockSize + padding := strings.Repeat("a", remainingBytes) paddedInputs = append(paddedInputs, input+"\n"+padding) } return io.NopCloser(strings.NewReader(strings.Join(paddedInputs, ""))) }
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
📒 Files selected for processing (11)
client/v2/autocli/prompt/message.go(1 hunks)client/v2/autocli/prompt/message_test.go(1 hunks)client/v2/autocli/prompt/struct.go(1 hunks)client/v2/autocli/prompt/struct_test.go(1 hunks)client/v2/autocli/prompt/util.go(1 hunks)client/v2/internal/prompt/validation.go(2 hunks)x/gov/client/cli/prompt.go(8 hunks)x/gov/client/cli/prompt_test.go(0 hunks)x/gov/go.mod(2 hunks)x/group/client/cli/prompt.go(7 hunks)x/group/go.mod(1 hunks)
💤 Files with no reviewable changes (1)
- x/gov/client/cli/prompt_test.go
🧰 Additional context used
📓 Path-based instructions (8)
client/v2/internal/prompt/validation.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
client/v2/autocli/prompt/message.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
client/v2/autocli/prompt/struct.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
x/gov/client/cli/prompt.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
x/group/client/cli/prompt.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
client/v2/autocli/prompt/message_test.go (2)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Pattern **/*_test.go: "Assess the unit test code assessing sufficient code coverage for the changes associated in the pull request"
client/v2/autocli/prompt/struct_test.go (2)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Pattern **/*_test.go: "Assess the unit test code assessing sufficient code coverage for the changes associated in the pull request"
client/v2/autocli/prompt/util.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
🪛 golangci-lint (1.62.2)
client/v2/autocli/prompt/message.go
200-200: commentFormatting: put a space between // and comment text
(gocritic)
🔇 Additional comments (2)
x/gov/go.mod (1)
7-7: Verify client/v2 module versioning
The pseudo-version v2.0.0-00010101000000-000000000000 indicates a pre-release or development version. Ensure this is intentional and consider using a stable version when available.
Also applies to: 185-185
x/group/go.mod (1)
187-187: LGTM!
The changes are consistent with the modifications in x/gov/go.mod.
|
|
||
| // create prompts | ||
| prompt := promptui.Prompt{ | ||
| Label: fmt.Sprintf("Enter %s %s", promptPrefix, strings.Title(fieldName)), // nolint:staticcheck // strings.Title has a better API |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Replace deprecated strings.Title with cases.Title for proper title casing
The function strings.Title is deprecated and should be avoided. Instead, use cases.Title from the golang.org/x/text/cases package for accurate Unicode-aware casing.
Apply this diff to update the code:
+import (
+ "golang.org/x/text/cases"
+ "golang.org/x/text/language"
+ // other imports
+)
-Label: fmt.Sprintf("Enter %s %s", promptPrefix, strings.Title(fieldName)), // nolint:staticcheck // strings.Title has a better API
+caser := cases.Title(language.Und)
+Label: fmt.Sprintf("Enter %s %s", promptPrefix, caser.String(fieldName)),Committable suggestion skipped: line range outside the PR's diff.
| func TestPromptMessage(t *testing.T) { | ||
| tests := []struct { | ||
| name string | ||
| msg protoreflect.Message | ||
| inputs []string | ||
| }{ | ||
| { | ||
| name: "testPb", | ||
| inputs: []string{ | ||
| "1", "2", "string", "bytes", "10101010", "0", "234234", "3", "4", "5", "true", "ENUM_ONE", | ||
| "bar", "6", "10000", "stake", "cosmos10d07y265gmmuvt4z0w9aw880jnsr700j6zn9kn", | ||
| "bytes", "6", "7", "false", "false", "true,false,true", "1,2,3", "hello,hola,ciao", "ENUM_ONE,ENUM_TWO", | ||
| "10239", "0", "No", "bar", "343", "No", "134", "positional2", "23455", "stake", "No", "deprecate", | ||
| "shorthand", "false", "cosmosvaloper1tnh2q55v8wyygtt9srz5safamzdengsn9dsd7z", | ||
| }, | ||
| msg: (&testpb.MsgRequest{}).ProtoReflect(), | ||
| }, | ||
| } | ||
| for _, tt := range tests { | ||
| t.Run(tt.name, func(t *testing.T) { | ||
| // https://github.com/manifoldco/promptui/issues/63#issuecomment-621118463 | ||
| var paddedInputs []string | ||
| for _, input := range tt.inputs { | ||
| padding := strings.Repeat("a", 4096-1-len(input)%4096) | ||
| paddedInputs = append(paddedInputs, input+"\n"+padding) | ||
| } | ||
| reader := io.NopCloser(strings.NewReader(strings.Join(paddedInputs, ""))) | ||
|
|
||
| got, err := promptMessage(address2.NewBech32Codec("cosmos"), address2.NewBech32Codec("cosmosvaloper"), address2.NewBech32Codec("cosmosvalcons"), "prefix", reader, tt.msg) | ||
| require.NoError(t, err) | ||
| require.NotNil(t, got) | ||
| }) | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance test coverage
The current test only covers the happy path. Consider adding:
- Error cases (invalid inputs, wrong formats)
- Validation of the resulting message content
- Edge cases (empty inputs, maximum values)
Would you like me to help generate additional test cases?
🛠️ Refactor suggestion
Refactor test implementation
The test has two issues that should be addressed:
- The padding logic is duplicated instead of using the
getReaderfunction - The test inputs lack documentation explaining their purpose
Apply these improvements:
func TestPromptMessage(t *testing.T) {
tests := []struct {
name string
msg protoreflect.Message
inputs []string
}{
{
name: "testPb",
+ // Each input corresponds to a field in MsgRequest:
+ // 1-12: Basic types (numbers, strings, bools, enums)
+ // 13-17: Complex types (addresses, coins)
+ // 18-40: Array and repeated fields
inputs: []string{
"1", "2", "string", "bytes", "10101010", "0", "234234", "3", "4", "5", "true", "ENUM_ONE",
"bar", "6", "10000", "stake", "cosmos10d07y265gmmuvt4z0w9aw880jnsr700j6zn9kn",
"bytes", "6", "7", "false", "false", "true,false,true", "1,2,3", "hello,hola,ciao", "ENUM_ONE,ENUM_TWO",
"10239", "0", "No", "bar", "343", "No", "134", "positional2", "23455", "stake", "No", "deprecate",
"shorthand", "false", "cosmosvaloper1tnh2q55v8wyygtt9srz5safamzdengsn9dsd7z",
},
msg: (&testpb.MsgRequest{}).ProtoReflect(),
},
}
for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
- var paddedInputs []string
- for _, input := range tt.inputs {
- padding := strings.Repeat("a", 4096-1-len(input)%4096)
- paddedInputs = append(paddedInputs, input+"\n"+padding)
- }
- reader := io.NopCloser(strings.NewReader(strings.Join(paddedInputs, "")))
+ reader := getReader(tt.inputs)
got, err := promptMessage(address2.NewBech32Codec("cosmos"), address2.NewBech32Codec("cosmosvaloper"), address2.NewBech32Codec("cosmosvalcons"), "prefix", reader, tt.msg)
require.NoError(t, err)
require.NotNil(t, got)
})
}
}Committable suggestion skipped: line range outside the PR's diff.
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
There was a problem hiding this comment.
tACK!
I really love this rewrite! It looks way better and is way less hacky 😬 .
PromptStruct now doesn't have any kind of default (like we had for authority), but I think it is fine, as messages does it and gov proposal is indeed a message!
Awesome that now it sets the inner field as well! That makes the proposal UX way better! Can't wait to see it enabled as a flag / option on autocli directly :) (point 2. of #18310 :) )
We should add a changelog and let's backport! Left a comment about moving prompt validation and delete the one from client/ but this can be done in a follow-up if you want!
There was a problem hiding this comment.
Let's delete the ones that aren't used, and move the ones we need in gov public. autocli/prompt may be fine.
The goal is that we can then completely delete client/prompt_validation.go
There was a problem hiding this comment.
Actually, maybe let's just define in gov the ones we need. This way we don't add anything for validation public. Less to maintain. So we can delete client/prompt_validation.go and the one that aren't used in prompt/internal in client/v2
julienrbrt
added
the
backport/v0.52.x
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (16)
client/v2/autocli/prompt/message.go (1)
151-154: Correct the function comment to match the function name.The comment above the
promptListfunction incorrectly refers tovalueOfinstead ofpromptList. Please update the comment to accurately reflect the functionality ofpromptList.Apply this diff to correct the comment:
-// valueOf prompts the user for a comma-separated list of values for a repeated field. +// promptList prompts the user for a comma-separated list of values for a repeated field.client/v2/autocli/prompt/validation.go (2)
3-8: Format imports according to Go standards.The import statements are not properly grouped and ordered. According to Go conventions, imports should be grouped in the following order: standard library packages, third-party packages, and local packages, separated by blank lines.
Apply this diff to organize the imports:
import ( + "errors" + "fmt" + "cosmossdk.io/core/address" - "errors" - "fmt" )
5-5: Rungofumptto format the code consistently.The code is not formatted according to
gofumptstandards. Runninggofumpt -extrawill ensure consistent formatting across the codebase.# Run gofumpt with the extra rules: gofumpt -w -extra validation.goclient/v2/autocli/prompt/validation_test.go (3)
21-49: Add test cases for invalid addresses inTestValidateAddress.The current test cases only cover valid addresses. Consider adding test cases with invalid addresses to ensure that
ValidateAddresscorrectly identifies and rejects them.Example of adding test cases with invalid addresses:
{ name: "invalid address - empty string", ac: address2.NewBech32Codec("cosmos"), addr: "", expectErr: true, }, { name: "invalid address - malformed string", ac: address2.NewBech32Codec("cosmos"), addr: "invalidaddress", expectErr: true, },Modify the test loop to handle expected errors:
for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { err := ValidateAddress(tt.ac)(tt.addr) if tt.expectErr { require.Error(t, err) } else { require.NoError(t, err) } }) }
3-8: Format imports according to Go standards.The import statements are not properly grouped and ordered. According to Go conventions, imports should be grouped as standard library packages, third-party packages, and local packages, separated by blank lines.
Apply this diff to organize the imports:
import ( + "testing" + + "github.com/stretchr/testify/require" + "cosmossdk.io/core/address" - "github.com/stretchr/testify/require" - - "testing" )🧰 Tools
🪛 golangci-lint (1.62.2)
6-6: File is not
gci-ed with --skip-generated -s standard -s default -s prefix(cosmossdk.io) -s prefix(github.com/cosmos/cosmos-sdk) --custom-order(gci)
5-5: File is not
gofumpt-ed with-extra(gofumpt)
5-5: Rungofumptto format the code consistently.The code is not formatted according to
gofumptstandards. Runninggofumpt -extrawill enforce consistent formatting across the codebase.# Run gofumpt with the extra rules: gofumpt -w -extra validation_test.go🧰 Tools
🪛 golangci-lint (1.62.2)
5-5: File is not
gofumpt-ed with-extra(gofumpt)
x/group/client/cli/prompt.go (1)
14-15: Organize imports according to Go conventions.The import statements are not grouped correctly. Imports should be organized into standard library packages, third-party packages, and local packages, with blank lines separating each group.
Apply this diff to reorganize the imports:
import ( "encoding/json" "fmt" "os" "sort" + gogoproto "github.com/cosmos/gogoproto/proto" + "github.com/spf13/cobra" + + "cosmossdk.io/client/v2/autocli/prompt" + "cosmossdk.io/core/address" + govcli "cosmossdk.io/x/gov/client/cli" + govtypes "cosmossdk.io/x/gov/types" + "github.com/cosmos/cosmos-sdk/client" "github.com/cosmos/cosmos-sdk/client/flags" "github.com/cosmos/cosmos-sdk/codec" sdk "github.com/cosmos/cosmos-sdk/types" )x/gov/client/cli/util.go (1)
222-229: Consider handling whitespace in empty validation.The function correctly validates for empty strings, but consider trimming whitespace to catch inputs that only contain spaces.
func ValidatePromptNotEmpty(input string) error { - if input == "" { + if strings.TrimSpace(input) == "" { return errors.New("input cannot be empty") } return nil }x/gov/CHANGELOG.md (1)
64-64: Enhance changelog entry with migration details.While the entry correctly documents the breaking change, it would be helpful to include details about how users should migrate from the old implementation to the new one.
x/gov/client/cli/prompt.go (3)
71-71: Consider splitting address codec parameters.The method signature is becoming long with multiple address codecs. Consider creating a struct to hold these codecs for better maintainability.
+type AddressCodecs struct { + Base address.Codec + Validator address.Codec + Consensus address.Codec +} -func (p *proposalType) Prompt(cdc codec.Codec, skipMetadata bool, addressCodec, validatorAddressCodec, consensusAddressCodec address.Codec) (*proposal, types.ProposalMetadata, error) { +func (p *proposalType) Prompt(cdc codec.Codec, skipMetadata bool, codecs AddressCodecs) (*proposal, types.ProposalMetadata, error) {
111-120: Consider encapsulating proto message conversion.The conversion between proto implementations could be moved to a helper function for better readability and reusability.
+func convertToGogoProto(msg proto.Message, target gogoproto.Message) error { + resultBytes, err := proto.Marshal(msg) + if err != nil { + return fmt.Errorf("failed to marshal message: %w", err) + } + return gogoproto.Unmarshal(resultBytes, target) +} -// message must be converted to gogoproto so @type is not lost -resultBytes, err := proto.Marshal(result.Interface()) -if err != nil { - return nil, metadata, fmt.Errorf("failed to marshal proposal message: %w", err) -} - -err = gogoproto.Unmarshal(resultBytes, p.Msg) +err = convertToGogoProto(result.Interface(), p.Msg)
Line range hint
206-208: Replace panic with proper error handling.Instead of panicking when GetMsgFromTypeURL fails, consider returning an error to maintain consistent error handling throughout the codebase.
- if err != nil { - // should never happen - panic(err) - } + if err != nil { + return fmt.Errorf("failed to get message from type URL %s: %w", proposal.MsgType, err) + }x/gov/go.mod (1)
7-7: Consider the implications of using a beta dependencyThe module depends on
client/v2 v2.0.0-beta.6. Since this is a beta version, ensure that:
- Breaking changes are expected and documented
- There's a plan to upgrade to the stable version when available
x/gov/client/cli/util_test.go (1)
725-730: Consider adding more test cases for coin validationWhile the basic cases are covered, consider adding tests for:
- Multiple denominations (e.g., "100stake,200token")
- Zero amounts (e.g., "0stake")
- Invalid amount formats (e.g., "stake100")
- Special characters in denominations
func TestValidatePromptCoins(t *testing.T) { require := require.New(t) require.NoError(ValidatePromptCoins("100stake")) + require.NoError(ValidatePromptCoins("100stake,200token")) + require.NoError(ValidatePromptCoins("0stake")) require.ErrorContains(ValidatePromptCoins("foo"), "invalid coins") + require.ErrorContains(ValidatePromptCoins("stake100"), "invalid coins") + require.ErrorContains(ValidatePromptCoins("100stake$"), "invalid coins") }CHANGELOG.md (2)
Line range hint
1-1: Add title to the CHANGELOG fileThe CHANGELOG file should start with a clear title like "# Changelog" to follow standard markdown practices.
+ # Changelog
Line range hint
14-14: Standardize version header formattingVersion headers have inconsistent formatting - some use links while others don't. Standardize all version headers to use markdown links to the release tags.
- ## v0.47.0 - 2023-03-14 + ## [v0.47.0](https://github.com/cosmos/cosmos-sdk/releases/tag/v0.47.0) - 2023-03-14Also applies to: 62-62, 1084-1084
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
📒 Files selected for processing (23)
CHANGELOG.md(1 hunks)client/prompt_validation.go(0 hunks)client/prompt_validation_test.go(0 hunks)client/v2/CHANGELOG.md(1 hunks)client/v2/autocli/prompt/message.go(1 hunks)client/v2/autocli/prompt/struct.go(1 hunks)client/v2/autocli/prompt/validation.go(1 hunks)client/v2/autocli/prompt/validation_test.go(1 hunks)client/v2/go.mod(1 hunks)client/v2/internal/prompt/validation.go(0 hunks)client/v2/internal/prompt/validation_test.go(0 hunks)simapp/go.mod(1 hunks)simapp/v2/go.mod(1 hunks)tests/go.mod(1 hunks)x/feegrant/go.mod(3 hunks)x/gov/CHANGELOG.md(1 hunks)x/gov/client/cli/prompt.go(8 hunks)x/gov/client/cli/util.go(1 hunks)x/gov/client/cli/util_test.go(1 hunks)x/gov/go.mod(3 hunks)x/group/client/cli/prompt.go(7 hunks)x/group/go.mod(4 hunks)x/upgrade/go.mod(3 hunks)
💤 Files with no reviewable changes (4)
- client/v2/internal/prompt/validation.go
- client/v2/internal/prompt/validation_test.go
- client/prompt_validation_test.go
- client/prompt_validation.go
🚧 Files skipped from review as they are similar to previous changes (1)
- client/v2/autocli/prompt/struct.go
🧰 Additional context used
📓 Path-based instructions (11)
client/v2/autocli/prompt/validation_test.go (2)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Pattern **/*_test.go: "Assess the unit test code assessing sufficient code coverage for the changes associated in the pull request"
client/v2/autocli/prompt/validation.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
x/gov/client/cli/util_test.go (2)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Pattern **/*_test.go: "Assess the unit test code assessing sufficient code coverage for the changes associated in the pull request"
tests/go.mod (1)
Pattern tests/**/*: "Assess the integration and e2e test code assessing sufficient code coverage for the changes associated in the pull request"
client/v2/autocli/prompt/message.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
client/v2/CHANGELOG.md (1)
Pattern **/*.md: "Assess the documentation for misspellings, grammatical errors, missing documentation and correctness"
x/gov/client/cli/util.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
x/gov/client/cli/prompt.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
x/group/client/cli/prompt.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
x/gov/CHANGELOG.md (1)
Pattern **/*.md: "Assess the documentation for misspellings, grammatical errors, missing documentation and correctness"
CHANGELOG.md (1)
Pattern **/*.md: "Assess the documentation for misspellings, grammatical errors, missing documentation and correctness"
🪛 golangci-lint (1.62.2)
client/v2/autocli/prompt/validation_test.go
6-6: File is not gci-ed with --skip-generated -s standard -s default -s prefix(cosmossdk.io) -s prefix(github.com/cosmos/cosmos-sdk) --custom-order
(gci)
5-5: File is not gofumpt-ed with -extra
(gofumpt)
🔇 Additional comments (14)
client/v2/CHANGELOG.md (1)
48-48: Change log entry is accurate and well-formatted.
The added changelog entry correctly reflects the introduced feature and follows the project's changelog format.
x/group/client/cli/prompt.go (1)
137-140:
Avoid using panic; handle errors gracefully.
Using panic for error handling is discouraged as it can cause the application to crash unexpectedly. Instead, return the error to allow the caller to handle it appropriately.
Apply this diff to replace the panic with an error return:
proposal.Msg, err = sdk.GetMsgFromTypeURL(clientCtx.Codec, result)
if err != nil {
- // should never happen
- panic(err)
+ return nil, govtypes.ProposalMetadata{}, fmt.Errorf("failed to get message from type URL: %w", err)
}Likely invalid or redundant comment.
x/gov/client/cli/util.go (1)
213-220: LGTM! Well-structured validation function.
The function correctly validates coin inputs using ParseCoinsNormalized and provides clear error messages.
x/gov/client/cli/prompt.go (1)
Line range hint 141-160: LGTM! Clean implementation of metadata prompting.
The function has been simplified and makes good use of the new prompt package. Error handling is clear and comprehensive.
client/v2/go.mod (1)
121-121: Verify security status of promptui dependency
The promptui v0.9.0 is relatively old. Let's verify if this version has any known security vulnerabilities.
✅ Verification successful
promptui v0.9.0 appears to be secure and up-to-date
The verification shows that:
- No known security vulnerabilities are reported for the promptui package
- v0.9.0 is actually the latest available version of the package
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for security advisories for promptui
gh api graphql -f query='
{
securityVulnerabilities(first: 5, ecosystem: GO, package: "github.com/manifoldco/promptui") {
nodes {
advisory {
summary
severity
publishedAt
}
vulnerableVersionRange
firstPatchedVersion {
identifier
}
}
}
}'
# Check latest version
curl -s "https://api.github.com/repos/manifoldco/promptui/tags" | jq '.[0].name'
Length of output: 451
x/feegrant/go.mod (1)
39-39: Verify the necessity of client/v2 indirect dependency
The module has gained an indirect dependency on client/v2. This is likely propagated through the gov module update.
✅ Verification successful
Let me gather more specific information about the dependency chain.
The indirect dependency on client/v2 is correctly propagated
The indirect dependency is necessary as:
- x/feegrant has test files that import x/gov (
tx_test.go) - x/gov directly depends on client/v2 in its go.mod
- Therefore, the indirect dependency in x/feegrant's go.mod is correctly tracking the transitive dependency through x/gov
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check which direct dependencies require client/v2
rg -l "cosmossdk.io/client/v2" --type go | grep -v "go.mod"
Length of output: 1310
Script:
#!/bin/bash
# Check if x/feegrant imports client/v2 directly or indirectly through gov
rg -l "cosmossdk.io/client/v2" x/feegrant --type go
# Check if x/feegrant imports x/gov
rg -l "cosmossdk.io/x/gov" x/feegrant --type go
# Check go.mod in x/gov to see if it directly depends on client/v2
cat x/gov/go.mod | grep "cosmossdk.io/client/v2"
Length of output: 262
x/group/go.mod (2)
123-123: LGTM: Indirect dependency addition.
The addition of promptui as an indirect dependency is appropriate for handling interactive CLI prompts.
7-7: Verify the beta dependency impact.
The addition of client/v2 v2.0.0-beta.6 and update of the gov module version are consistent with other modules, but using a beta version could introduce breaking changes.
Also applies to: 19-19
x/upgrade/go.mod (1)
13-13: LGTM: Consistent dependency versions.
The gov module version update and client/v2 addition match the versions used in other modules, maintaining consistency across the codebase.
Also applies to: 43-43
tests/go.mod (1)
71-71: LGTM: Test dependencies aligned with main modules.
The client/v2 version in tests matches the version used in the main modules, ensuring consistent testing environment.
simapp/go.mod (1)
7-7: LGTM: Version update for client/v2
The update to cosmossdk.io/client/v2 v2.0.0-beta.6 aligns with the PR's focus on client/v2 features and matches the version updates in related modules.
simapp/v2/go.mod (1)
7-7: LGTM: Consistent version update
The update to cosmossdk.io/client/v2 v2.0.0-beta.6 maintains version consistency with other modules in the codebase.
x/gov/client/cli/util_test.go (1)
718-723: LGTM: Well-structured validation test
The test function provides good coverage for empty input validation with both positive and negative cases.
CHANGELOG.md (1)
Line range hint 1-1200: LGTM on overall changelog structure and content
The changelog follows good practices by:
- Organizing changes by version
- Categorizing changes (Features, Improvements, Bug Fixes, etc.)
- Including PR references
- Providing detailed descriptions of changes
- Highlighting breaking changes
| // TODO: add more kinds | ||
| // skip any other types | ||
| return protoreflect.Value{}, nil |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Handle additional protobuf field kinds in valueOf function.
Currently, the valueOf function does not handle certain field kinds such as floats. Consider implementing support for additional protobuf field kinds to ensure all message fields can be appropriately parsed.
You can add cases for FloatKind and DoubleKind in the switch statement:
func valueOf(field protoreflect.FieldDescriptor, result string) (protoreflect.Value, error) {
switch field.Kind() {
// existing cases...
+ case protoreflect.FloatKind, protoreflect.DoubleKind:
+ resultFloat, err := strconv.ParseFloat(result, 64)
+ if err != nil {
+ return protoreflect.Value{}, fmt.Errorf("invalid value for float: %w", err)
+ }
+ return protoreflect.ValueOfFloat64(resultFloat), nil
// existing cases...
}
}Committable suggestion skipped: line range outside the PR's diff.
| import ( | ||
| "fmt" | ||
| "io" | ||
| "reflect" |
Check notice
Code scanning / CodeQL
Sensitive package import Note
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (19)
client/v2/autocli/prompt/struct.go (4)
13-15: Enhance function documentation with examples and error cases.Consider expanding the documentation to include:
- Usage examples
- Possible error cases
- Limitations on supported field types
- Behavior with nested structs
Example documentation:
// PromptStruct prompts for values of a struct's fields interactively. // It returns the populated struct and any error encountered. // // Supported field types: // - String and int (both value and pointer types) // - Nested structs (recursively handled) // - Slices of strings and ints // // Example usage: // // type Config struct { // Name string // Port int // Options *struct { // Verbose bool // } // } // // config := Config{} // result, err := PromptStruct("config", config) // // Returns error if: // - User cancels the prompt // - Invalid integer input is provided // - IO operations fail
96-100: Refactor duplicate integer parsing logicThe integer parsing logic is duplicated across three locations. Consider extracting it into a helper function.
+func parseAndValidateInt(s string) (int64, error) { + result, err := strconv.ParseInt(s, 10, 0) + if err != nil { + return 0, fmt.Errorf("invalid value for int: %w", err) + } + return result, nil +} -resultInt, err := strconv.ParseInt(result, 10, 0) -if err != nil { - return data, fmt.Errorf("invalid value for int: %w", err) -} +resultInt, err := parseAndValidateInt(result) +if err != nil { + return data, err +}Also applies to: 106-112, 120-126
88-90: Improve error message consistencyError messages should be consistent and provide context about the field being processed.
-return data, fmt.Errorf("failed to prompt for %s: %w", fieldName, err) +return data, fmt.Errorf("failed to prompt for field %s.%s: %w", promptPrefix, fieldName, err)
28-134: Consider adding validation hooksThe current implementation only validates for non-empty input. Consider adding support for custom validation functions that can be passed as options.
Example interface:
type PromptOptions struct { Validator func(string) error Default string Help string } func PromptStruct[T any](promptPrefix string, data T, opts map[string]PromptOptions) (T, error)client/v2/autocli/prompt/validation.go (1)
19-29: Consider enhancing error message for invalid addressesThe error message could be more descriptive by including the actual validation error from the codec.
Consider this improvement:
func ValidateAddress(ac address.Codec) func(string) error { return func(i string) error { if _, err := ac.StringToBytes(i); err != nil { - return fmt.Errorf("invalid address") + return fmt.Errorf("invalid address: %w", err) } return nil } }x/group/client/cli/prompt.go (3)
Line range hint
53-64: Consider extracting error messages as constantsWhile the implementation is correct, consider extracting the error messages as constants to maintain consistency and ease future updates.
+const ( + errGroupPolicyAddress = "failed to set group policy address: %w" + errProposerAddress = "failed to set proposer address: %w" +)
71-91: Consider extracting error variables and adding commentsThe message handling logic is complex and could benefit from:
- Extracted error variables
- Comments explaining the proto/gogoproto conversion necessity
+// errMsgNotFound is returned when the proposal message type is not found +var errMsgNotFound = errors.New("failed to find proposal msg") +// convertProtoMessage converts the proto message to gogoproto format +// This is necessary to preserve the @type field during marshaling func convertProtoMessage(msg proto.Message, target gogoproto.Message) error { resultBytes, err := proto.Marshal(msg) if err != nil { return fmt.Errorf("failed to marshal proposal message: %w", err) } return gogoproto.Unmarshal(resultBytes, target) }
Line range hint
130-142: Consider extracting magic strings and improving error handlingThe implementation could benefit from:
- Extracted string constants for proposal types
- More specific error messages for different failure scenarios
+const ( + errProposalTypePrompt = "failed to prompt proposal types: %w" + errMsgTypeNotFound = "failed to get message from type URL: %w" +)x/gov/client/cli/util.go (1)
213-229: Consider enhancing error messagesThe validation functions are well-implemented, but consider:
- More descriptive error messages for
ValidatePromptCoins- Adding validation for maximum length in
ValidatePromptNotEmpty- return fmt.Errorf("invalid coins: %w", err) + return fmt.Errorf("invalid coin format (expected format: '1atom,1stake'): %w", err) - return errors.New("input cannot be empty") + return errors.New("input cannot be empty or contain only whitespace")x/gov/client/cli/prompt.go (2)
94-120: Consider enhancing error handling in proto conversion chainWhile the proto message handling is correct, the conversion chain between proto implementations could benefit from more granular error handling. Consider wrapping each conversion step with specific error context.
- resultBytes, err := proto.Marshal(result.Interface()) + resultBytes, err := proto.Marshal(result.Interface()) + if err != nil { + return nil, metadata, fmt.Errorf("failed to marshal proto message: %w", err) + } + + msg := p.Msg + if msg == nil { + return nil, metadata, fmt.Errorf("destination message is nil") + } - err = gogoproto.Unmarshal(resultBytes, p.Msg) + err = gogoproto.Unmarshal(resultBytes, msg)
Line range hint
241-242: Consider implementing yes/no prompt improvementThe TODO comment suggests using a yes/no prompt instead of interactive selection. This could indeed provide a better user experience for simple binary choices.
Would you like me to implement a simple yes/no prompt utility function to replace the current selection?
client/v2/autocli/prompt/message.go (3)
61-63: Consider adding field-specific validationThe comment suggests that not all fields need to be non-empty, but there's no field-specific validation logic. Consider implementing optional/required field validation based on protobuf field options.
145-146: Complete implementation for remaining protobuf typesThe TODO indicates that some protobuf types are not yet handled. Consider implementing support for:
- FloatKind
- DoubleKind
- GroupKind
Would you like me to provide implementations for these additional types?
154-171: Consider enhancing list input UXThe current comma-separated input for lists could be improved. Consider:
- Supporting different delimiters
- Allowing multi-line input for better readability
- Adding input validation preview
+const ( + defaultListDelimiter = "," + alternateDelimiter = "\n" +)x/gov/client/cli/util_test.go (2)
718-723: Consider expanding test cases for ValidatePromptNotEmpty.The test covers basic functionality but could be enhanced with additional test cases:
- Whitespace-only input
- Very long input strings
- Special characters
- Unicode characters
func TestValidatePromptNotEmpty(t *testing.T) { require := require.New(t) - - require.NoError(ValidatePromptNotEmpty("foo")) - require.ErrorContains(ValidatePromptNotEmpty(""), "input cannot be empty") + + testCases := []struct { + name string + input string + wantErr string + }{ + {"valid input", "foo", ""}, + {"empty string", "", "input cannot be empty"}, + {"whitespace only", " ", "input cannot be empty"}, + {"special chars", "!@#$%^", ""}, + {"unicode chars", "你好", ""}, + {"very long string", strings.Repeat("a", 1000), ""}, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + err := ValidatePromptNotEmpty(tc.input) + if tc.wantErr != "" { + require.ErrorContains(err, tc.wantErr) + } else { + require.NoError(err) + } + }) + } }
725-730: Consider expanding test cases for ValidatePromptCoins.The test covers basic functionality but could be enhanced with additional test cases:
- Multiple denominations
- Zero amounts
- Maximum amounts
- Invalid denominations
- Invalid amount formats
func TestValidatePromptCoins(t *testing.T) { require := require.New(t) - - require.NoError(ValidatePromptCoins("100stake")) - require.ErrorContains(ValidatePromptCoins("foo"), "invalid coins") + + testCases := []struct { + name string + input string + wantErr string + }{ + {"valid single coin", "100stake", ""}, + {"valid multiple coins", "100stake,200token", ""}, + {"zero amount", "0stake", ""}, + {"max uint64", "18446744073709551615stake", ""}, + {"invalid amount", "foo", "invalid coins"}, + {"invalid denomination", "100st@ke", "invalid coins"}, + {"empty string", "", "invalid coins"}, + {"missing amount", "stake", "invalid coins"}, + {"missing denomination", "100", "invalid coins"}, + {"invalid format", "100,stake", "invalid coins"}, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + err := ValidatePromptCoins(tc.input) + if tc.wantErr != "" { + require.ErrorContains(err, tc.wantErr) + } else { + require.NoError(err) + } + }) + } }CHANGELOG.md (3)
Line range hint
13-15: Consider adding version release dates consistentlyWhile some versions include release dates, others don't. Consider adding dates consistently for all versions to help track the timeline of changes.
Line range hint
2892-2892: Remove redundant backticks at file endThere are redundant backticks at the end of the file that should be removed.
-```
Line range hint
1-2892: Consider splitting changelog by major versionsGiven the size of this changelog, consider splitting it into separate files by major version numbers (e.g., CHANGELOG_v0.47.md, CHANGELOG_v0.46.md etc.) while keeping a summary in the main CHANGELOG.md. This would improve readability and maintenance.
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
📒 Files selected for processing (23)
CHANGELOG.md(1 hunks)client/prompt_validation.go(0 hunks)client/prompt_validation_test.go(0 hunks)client/v2/CHANGELOG.md(1 hunks)client/v2/autocli/prompt/message.go(1 hunks)client/v2/autocli/prompt/struct.go(1 hunks)client/v2/autocli/prompt/validation.go(1 hunks)client/v2/autocli/prompt/validation_test.go(1 hunks)client/v2/go.mod(1 hunks)client/v2/internal/prompt/validation.go(0 hunks)client/v2/internal/prompt/validation_test.go(0 hunks)simapp/go.mod(1 hunks)simapp/v2/go.mod(1 hunks)tests/go.mod(1 hunks)x/feegrant/go.mod(3 hunks)x/gov/CHANGELOG.md(1 hunks)x/gov/client/cli/prompt.go(8 hunks)x/gov/client/cli/util.go(1 hunks)x/gov/client/cli/util_test.go(1 hunks)x/gov/go.mod(3 hunks)x/group/client/cli/prompt.go(7 hunks)x/group/go.mod(4 hunks)x/upgrade/go.mod(3 hunks)
💤 Files with no reviewable changes (4)
- client/v2/internal/prompt/validation.go
- client/v2/internal/prompt/validation_test.go
- client/prompt_validation_test.go
- client/prompt_validation.go
🚧 Files skipped from review as they are similar to previous changes (1)
- x/group/go.mod
🧰 Additional context used
📓 Path-based instructions (12)
client/v2/autocli/prompt/validation.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
client/v2/autocli/prompt/validation_test.go (2)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Pattern **/*_test.go: "Assess the unit test code assessing sufficient code coverage for the changes associated in the pull request"
tests/go.mod (1)
Pattern tests/**/*: "Assess the integration and e2e test code assessing sufficient code coverage for the changes associated in the pull request"
x/gov/client/cli/util_test.go (2)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Pattern **/*_test.go: "Assess the unit test code assessing sufficient code coverage for the changes associated in the pull request"
x/gov/client/cli/util.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
client/v2/autocli/prompt/message.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
client/v2/CHANGELOG.md (1)
Pattern **/*.md: "Assess the documentation for misspellings, grammatical errors, missing documentation and correctness"
x/gov/client/cli/prompt.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
x/group/client/cli/prompt.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
x/gov/CHANGELOG.md (1)
Pattern **/*.md: "Assess the documentation for misspellings, grammatical errors, missing documentation and correctness"
CHANGELOG.md (1)
Pattern **/*.md: "Assess the documentation for misspellings, grammatical errors, missing documentation and correctness"
client/v2/autocli/prompt/struct.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
🪛 golangci-lint (1.62.2)
client/v2/autocli/prompt/validation_test.go
6-6: File is not gci-ed with --skip-generated -s standard -s default -s prefix(cosmossdk.io) -s prefix(github.com/cosmos/cosmos-sdk) --custom-order
(gci)
5-5: File is not gofumpt-ed with -extra
(gofumpt)
🪛 GitHub Check: CodeQL
client/v2/autocli/prompt/struct.go
[notice] 6-6: Sensitive package import
Certain system packages contain functions which may be a possible source of non-determinism
🔇 Additional comments (20)
client/v2/autocli/prompt/struct.go (1)
82-82: Replace deprecated strings.Title with cases.Title
This issue was previously identified and still needs to be addressed.
client/v2/autocli/prompt/validation.go (1)
11-17: LGTM! Simple and effective empty string validation.
The implementation is clean and follows Go error handling patterns.
client/v2/autocli/prompt/validation_test.go (1)
14-19: LGTM! Good coverage of empty string validation.
The test covers both valid and invalid cases effectively.
client/v2/CHANGELOG.md (1)
48-48: LGTM! Well-formatted changelog entry.
The entry follows the Keep a Changelog format and clearly describes the new feature.
x/group/client/cli/prompt.go (2)
9-14: LGTM: Clean import organization and struct enhancement
The addition of protobuf-related imports and the new MsgType field in proposalType struct align well with the migration to use the new prompt package.
Also applies to: 33-35
39-39: LGTM: Enhanced address validation support
The updated method signature now properly handles different address types through dedicated codecs.
x/gov/CHANGELOG.md (1)
64-64: LGTM: Clear and well-categorized changelog entry
The changelog entry correctly documents the breaking change and its implications.
x/gov/client/cli/prompt.go (2)
71-71: LGTM: Enhanced address validation with type-safe codecs
The updated signature with separate address codecs for different address types (validator, consensus) improves type safety and validation.
Line range hint 141-160: Ensure consistent validation across all metadata fields
While the function has been simplified, consider implementing consistent validation rules for all metadata fields to maintain data quality.
client/v2/go.mod (1)
121-121: LGTM: Correctly updated promptui dependency
The dependency on github.com/manifoldco/promptui is now correctly marked as direct, reflecting its usage in the new prompt package.
client/v2/autocli/prompt/message.go (1)
16-23: LGTM: Well-structured message prompting implementation
The public API is clean and well-documented, with good separation of concerns between the public and internal functions.
CHANGELOG.md (1)
Line range hint 1-1: LGTM: Well-structured changelog format
The changelog follows best practices with clear sections for breaking changes, improvements, bug fixes and features. The hierarchical organization makes it easy to navigate.
x/gov/go.mod (2)
185-185: LGTM: Replace directive is correctly configured
The replace directive for client/v2 is properly set up for local development.
7-7: Verify compatibility with beta dependencies
The addition of client/v2 and its related prompt dependencies aligns with the PR's objective. However, using a beta version (v2.0.0-beta.6) could introduce stability concerns.
Also applies to: 19-19, 26-26
✅ Verification successful
Beta dependency usage appears stable and well-integrated
The client/v2 package is extensively used across the codebase in critical components including CLI, transaction handling, and autocli features. The dependency is properly managed with a replace directive, indicating it's being developed in sync with the main codebase. Multiple modules (gov, upgrade, group, feegrant) are consistently using this version, suggesting a coordinated upgrade approach.
- The package is used in 35+ Go files across core functionalities
- 8 different modules depend on this version
- Replace directive ensures version consistency:
cosmossdk.io/client/v2 => ../../client/v2
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if there are any known issues with the beta version
gh issue list --repo cosmos/cosmos-sdk --label "client/v2" --state open
Length of output: 71
Script:
#!/bin/bash
# Let's check the go.mod file to understand the dependency usage and any replace directives
cat x/gov/go.mod
# Search for actual usage of the client/v2 package
rg -l "cosmossdk.io/client/v2" --type go
# Check if there are any other modules using this dependency
fd -e mod -x grep -l "cosmossdk.io/client/v2" {}
Length of output: 10511
x/feegrant/go.mod (2)
186-186: LGTM: Replace directive is correctly configured
The replace directive for client/v2 is properly set up for local development.
15-15: Verify gov module version compatibility
The update to the gov module version and addition of client/v2 as an indirect dependency appears correct. However, we should verify the compatibility between these versions.
Also applies to: 39-39
✅ Verification successful
Based on the analysis of the shell script outputs, I can now generate the final response:
Gov module version update is safe to use
The feegrant module only uses gov for message type strings in allowed message filtering, specifically "/cosmos.gov.v1.MsgSubmitProposal" and "/cosmos.gov.v1beta1.MsgVote". This is a stable API surface that doesn't require tight version coupling.
- The gov dependency is used in test setup and message filtering only
- No direct API dependencies that could cause compatibility issues
- The version update (912390d) is related to store implementation changes and doesn't affect the gov module's public API
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for any breaking changes in the gov module
gh pr list --repo cosmos/cosmos-sdk --label "x/gov" --state closed --search "merged:>2023-09-25"
Length of output: 96
Script:
#!/bin/bash
# Let's check the go.mod files to verify version compatibility
echo "=== Checking x/feegrant/go.mod ==="
cat x/feegrant/go.mod
echo -e "\n=== Checking x/gov/go.mod ==="
cat x/gov/go.mod
echo -e "\n=== Checking for gov module usage in feegrant ==="
rg "gov\." x/feegrant/ -l
Length of output: 18564
Script:
#!/bin/bash
# Let's check how gov is used in feegrant to understand the dependency
echo "=== Checking gov usage in feegrant files ==="
rg "gov\." x/feegrant/ -A 3 -B 3
# Let's also check if there are any breaking changes in the specific gov version
echo -e "\n=== Checking git commit for gov version ==="
git show 912390d5fc4a --oneline
Length of output: 20344
x/upgrade/go.mod (2)
13-13: LGTM: Dependencies are consistently updated
The gov module version update and addition of client/v2 as an indirect dependency maintain consistency with other modules.
Also applies to: 43-43
210-210: LGTM: Replace directive is correctly configured
The replace directive for client/v2 is properly set up for local development.
simapp/go.mod (1)
7-7: Version update maintains consistency
The update to v2.0.0-beta.6 maintains version consistency with other modules.
simapp/v2/go.mod (1)
7-7: Version alignment with v2 architecture
The update to v2.0.0-beta.6 aligns with the v2 architecture of the simapp module.
Let's verify the beta version's stability:
✅ Verification successful
Beta version is stable for client/v2 usage
The TODOs found in the client/v2 module are not critical for its stability:
-
Most TODOs are related to future improvements and cleanup:
- Removing SDK dependencies in favor of internal implementations
- Adding more message kinds in prompts
- Improving UX for prompts
-
The module is actively maintained and used in production, as evidenced by its inclusion in the simapp v2 architecture.
-
No experimental or unstable features were found in the codebase.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any recent changes or issues in the client/v2 module
# that might affect stability
# Look for any TODO, FIXME, or experimental features
rg -g "client/v2/**" "TODO|FIXME|experimental"
Length of output: 683
Script:
#!/bin/bash
# Let's check if these TODOs are critical by examining their context
rg -g 'client/v2/**' -B 2 -A 2 'TODO|FIXME|experimental'
# Also check for any breaking changes or stability warnings in go.mod and changelog
fd "CHANGELOG" --exec cat {} \; || true
cat client/v2/go.mod || true
Length of output: 81605
| case reflect.Slice: | ||
| switch v.Field(i).Type().Elem().Kind() { | ||
| case reflect.String: | ||
| v.Field(i).Set(reflect.ValueOf([]string{result})) | ||
| case reflect.Int: | ||
| resultInt, err := strconv.ParseInt(result, 10, 0) | ||
| if err != nil { | ||
| return data, fmt.Errorf("invalid value for int: %w", err) | ||
| } | ||
|
|
||
| v.Field(i).Set(reflect.ValueOf([]int{int(resultInt)})) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance slice handling functionality
Current implementation only supports single-element slices. Consider:
- Adding support for comma-separated values
- Implementing a repeated prompt for multiple values
Example implementation for string slices:
case reflect.Slice:
var values []string
for {
result, err := prompt.Run()
if err != nil {
if err == promptui.ErrInterrupt {
break
}
return data, fmt.Errorf("failed to prompt for %s: %w", fieldName, err)
}
if result == "" {
break
}
values = append(values, result)
}
v.Field(i).Set(reflect.ValueOf(values))| func TestValidateAddress(t *testing.T) { | ||
| tests := []struct { | ||
| name string | ||
| ac address.Codec | ||
| addr string | ||
| }{ | ||
| { | ||
| name: "address", | ||
| ac: address2.NewBech32Codec("cosmos"), | ||
| addr: "cosmos129lxcu2n3hx54fdxlwsahqkjr3sp32cxm00zlm", | ||
| }, | ||
| { | ||
| name: "validator address", | ||
| ac: address2.NewBech32Codec("cosmosvaloper"), | ||
| addr: "cosmosvaloper1tnh2q55v8wyygtt9srz5safamzdengsn9dsd7z", | ||
| }, | ||
| { | ||
| name: "consensus address", | ||
| ac: address2.NewBech32Codec("cosmosvalcons"), | ||
| addr: "cosmosvalcons136uu5rj23kdr3jjcmjt7aw5qpugjjat2klgrus", | ||
| }, | ||
| } | ||
| for _, tt := range tests { | ||
| t.Run(tt.name, func(t *testing.T) { | ||
| err := ValidateAddress(tt.ac)(tt.addr) | ||
| require.NoError(t, err) | ||
| }) | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add negative test cases for address validation
While the test covers various valid address types, it's missing test cases for invalid addresses.
Consider adding these test cases:
func TestValidateAddress(t *testing.T) {
tests := []struct {
name string
ac address.Codec
addr string
+ wantErr bool
}{
{
name: "address",
ac: address2.NewBech32Codec("cosmos"),
addr: "cosmos129lxcu2n3hx54fdxlwsahqkjr3sp32cxm00zlm",
},
+ {
+ name: "invalid prefix",
+ ac: address2.NewBech32Codec("cosmos"),
+ addr: "osmo1abc...",
+ wantErr: true,
+ },
+ {
+ name: "invalid checksum",
+ ac: address2.NewBech32Codec("cosmos"),
+ addr: "cosmos129lxcu2n3hx54fdxlwsahqkjr3sp32cxm00zln",
+ wantErr: true,
+ },
// existing 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.wantErr {
+ require.Error(t, err)
+ } else {
+ require.NoError(t, err)
+ }
})
}
}Committable suggestion skipped: line range outside the PR's diff.
| @@ -68,7 +68,7 @@ require ( | |||
| cloud.google.com/go/compute/metadata v0.5.0 // indirect | |||
| cloud.google.com/go/iam v1.1.13 // indirect | |||
| cloud.google.com/go/storage v1.43.0 // indirect | |||
| cosmossdk.io/client/v2 v2.0.0-20230630094428-02b760776860 // indirect | |||
| cosmossdk.io/client/v2 v2.0.0-beta.6 // indirect | |||
There was a problem hiding this comment.
💡 Codebase verification
Inconsistent version found in tools/hubl/go.mod
The codebase shows an inconsistent version of cosmossdk.io/client/v2 in tools/hubl/go.mod which uses v2.0.0-beta.1.0.20240118210941-3897926e722e while all other modules use v2.0.0-beta.6. This inconsistency should be addressed.
tools/hubl/go.mod: Update the version tov2.0.0-beta.6to maintain consistency across the codebase
🔗 Analysis chain
Dependency update aligns with breaking changes
The update to v2.0.0-beta.6 is consistent with the breaking changes indicated in the PR title.
Let's verify the version consistency across the codebase:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any inconsistent versions of cosmossdk.io/client/v2
# across all go.mod files in the repository
# Search for all go.mod files and check client/v2 versions
fd --type f "go.mod" --exec grep -l "cosmossdk.io/client/v2" {} \; | \
xargs grep "cosmossdk.io/client/v2"
Length of output: 1079
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (4)
client/v2/autocli/prompt/validation_test.go (1)
50-55: Enhance URL validation test coverageConsider adding more edge cases to strengthen the test suite:
func TestValidatePromptURL(t *testing.T) { tests := []struct { name string url string wantErr bool }{ { name: "valid https", url: "https://example.com", }, + { + name: "missing protocol", + url: "example.com", + wantErr: true, + }, + { + name: "invalid protocol", + url: "ftp://example.com", + wantErr: true, + }, + { + name: "valid with path and query", + url: "https://example.com/path?query=value", + }, } // ... test execution }client/v2/autocli/prompt/validation.go (1)
21-30: Enhance error message with underlying causeThe error message could be more helpful by including the original error details from the codec.
func ValidateAddress(ac address.Codec) func(string) error { return func(i string) error { if _, err := ac.StringToBytes(i); err != nil { - return fmt.Errorf("invalid address") + return fmt.Errorf("invalid address: %w", err) } return nil } }client/v2/autocli/prompt/message.go (2)
214-259: Consider alternative input methods for list entriesThe current implementation prompts for each item individually and uses an interactive select for continuation. As noted in the TODO comment, this might not be the most efficient approach, especially for large lists.
Consider:
- Accepting a JSON array input for bulk entry
- Using a simple y/n prompt instead of interactive select
- Adding a "done" keyword in the input to indicate completion
- continuePrompt := promptui.Select{ - Label: "Add another item?", - Items: []string{"No", "Yes"}, - Stdin: stdIn, - } + continuePrompt := promptui.Prompt{ + Label: "Add another item? (y/n)", + Validate: validateYesNo, + Stdin: stdIn, + }
92-96: Enhance error context in valueOf error handlingWhen valueOf returns an error, the context of which field caused the error is lost.
v, err := valueOf(field, result) if err != nil { - return msg, err + return msg, fmt.Errorf("failed to parse value for field %s: %w", field.Name(), err) }
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
📒 Files selected for processing (4)
client/prompt_validation_test.go(0 hunks)client/v2/autocli/prompt/message.go(1 hunks)client/v2/autocli/prompt/validation.go(2 hunks)client/v2/autocli/prompt/validation_test.go(1 hunks)
💤 Files with no reviewable changes (1)
- client/prompt_validation_test.go
🧰 Additional context used
📓 Path-based instructions (3)
client/v2/autocli/prompt/validation.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
client/v2/autocli/prompt/validation_test.go (2)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Pattern **/*_test.go: "Assess the unit test code assessing sufficient code coverage for the changes associated in the pull request"
client/v2/autocli/prompt/message.go (1)
Pattern **/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
🔇 Additional comments (6)
client/v2/autocli/prompt/validation_test.go (3)
1-12: LGTM: Clean package structure and imports
The package declaration and imports are well-organized and all dependencies are properly utilized.
13-18: LGTM: Comprehensive empty string validation test
The test effectively covers both valid and invalid cases with specific error message validation.
20-48: 🛠️ Refactor suggestion
Add negative test cases for address validation
While the test covers various valid address types, it's missing test cases for invalid addresses such as:
- Addresses with invalid prefixes
- Addresses with invalid checksums
- Empty addresses
- Malformed addresses
This will ensure the validation function properly handles error cases.
client/v2/autocli/prompt/validation.go (2)
Line range hint 12-19: LGTM: Clear and focused validation function
The function correctly validates non-empty input with appropriate error messaging.
Line range hint 33-41: LGTM: Proper URL validation with error wrapping
The function correctly validates URLs and provides descriptive error messages.
client/v2/autocli/prompt/message.go (1)
102-149:
Add support for missing protobuf field types
The function currently lacks support for several protobuf field types, particularly floating-point types (FloatKind, DoubleKind). Additionally, the default case silently ignores unsupported types instead of returning an error.
Consider also improving the default case:
default:
- // TODO: add more kinds
- // skip any other types
- return protoreflect.Value{}, nil
+ return protoreflect.Value{}, fmt.Errorf("unsupported field type %s", field.Kind())
|
@JulianToledano could you fix the conflicts? |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (4)
CHANGELOG.md (4)
Line range hint
1-1: Add title and description at the top of changelogThe changelog should start with a title and brief description of its purpose. This helps readers understand the document's scope.
+# Changelog + +All notable changes to the Cosmos SDK will be documented in this file. + # Changelog
Line range hint
15-20: Standardize version header formatVersion headers should consistently include both version number and release date. Some entries are missing dates.
-## [v0.47.14](https://github.com/cosmos/cosmos-sdk/releases/tag/v0.47.14) - 2024-09-20 +## [v0.47.14] - 2024-09-20 + +[Release](https://github.com/cosmos/cosmos-sdk/releases/tag/v0.47.14)
Line range hint
2512-2513: Add section for migration guidesConsider adding a dedicated section linking to migration guides for major version upgrades, making it easier for users to find upgrade instructions.
## Previous Releases + +## Migration Guides + +- [v0.47.x → v0.50.x](https://docs.cosmos.network/v0.50/migrations/upgrading) +- [v0.46.x → v0.47.x](https://docs.cosmos.network/v0.47/migrations/upgrading)
Line range hint
2514-2516: Remove redundant markdown code blockThe empty markdown code block at the end of the file serves no purpose and should be removed.
[CHANGELOG of previous versions](https://github.com/cosmos/cosmos-sdk/blob/c17c3caab86a1426a1eef4541e8203f5f54a1a54/CHANGELOG.md#v0391---2020-08-11) (pre Stargate). - -```
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
📒 Files selected for processing (6)
CHANGELOG.md(1 hunks)client/v2/CHANGELOG.md(1 hunks)client/v2/go.mod(1 hunks)simapp/go.mod(1 hunks)simapp/v2/go.mod(1 hunks)tests/go.mod(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- tests/go.mod
- simapp/go.mod
- client/v2/go.mod
- simapp/v2/go.mod
🧰 Additional context used
📓 Path-based instructions (2)
client/v2/CHANGELOG.md (1)
Pattern **/*.md: "Assess the documentation for misspellings, grammatical errors, missing documentation and correctness"
CHANGELOG.md (1)
Pattern **/*.md: "Assess the documentation for misspellings, grammatical errors, missing documentation and correctness"
🔇 Additional comments (1)
client/v2/CHANGELOG.md (1)
48-48: LGTM! The changelog entry is well-formatted and accurately describes the feature.
The entry follows the Keep a Changelog format, is placed in the correct section, and properly documents the addition of interactive autocli prompt functionality with appropriate issue reference.
(cherry picked from commit 5b12426) # Conflicts: # CHANGELOG.md # simapp/go.mod # simapp/v2/go.mod # tests/go.mod # x/feegrant/go.mod # x/gov/go.mod # x/group/go.mod # x/upgrade/go.mod
Co-authored-by: Julián Toledano <[email protected]> Co-authored-by: Julien Robert <[email protected]>
Description
Closes:
#17222
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...
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Improvements
Breaking Changes