Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat(client/v2): support gov proposals #18461
feat(client/v2): support gov proposals #18461
Changes from 11 commits
8028339
19c4dff
77c298d
e42c5f9
833da56
98e7cae
e5cb363
59acfa5
f4504e9
6e38eb8
17cbc97
e329e20
75863fa
7838a80
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The new validation functions introduced in the
prompt
package are well-defined and follow Go's idiomatic error handling. Each function provides a clear error message specific to the validation failure, which is good for debugging and user feedback. The use of%w
infmt.Errorf
allows for error wrapping, which is a best practice for error handling in Go as it enables errors to be unwrapped and inspected if needed.However, it's important to ensure that the error messages are consistent and user-friendly since they might be displayed to the end-user. For example, the message "invalid coins" in
ValidatePromptCoins
could be more descriptive, explaining what makes the coins invalid.Additionally, consider adding unit tests for these functions to ensure they handle edge cases correctly, such as inputs with leading or trailing spaces, or unusual characters in URLs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The test
TestValidatePromptURL
is checking for valid URLs. It would be beneficial to include more test cases to cover different types of URLs, including those with query parameters, fragments, and different schemes (http, https).There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
#17222
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@julienrbrt I have reviewed the issue #17222 on the Cosmos SDK repository. If there are specific actions or changes you would like me to take in relation to this issue, please let me know, and I will be happy to assist.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The test
TestValidatePromptCoins
checks for valid coin input. It would be beneficial to include more test cases to cover various valid and invalid coin formats, such as multiple denominations, invalid characters, and missing amounts.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
#17222
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@julienrbrt I've reviewed the issue you linked, and it seems to be related to the Cosmos SDK's governance capabilities. However, could you please clarify how this issue relates to the current pull request or the test cases for
TestValidatePromptCoins
? If there are specific test scenarios derived from this issue that you would like to see included, please let me know, and I'll work on adding those to the test suite.