-
Notifications
You must be signed in to change notification settings - Fork 3.6k
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
fix(textual): error if a formatted coin contains a comma #19265
Changes from 2 commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||
---|---|---|---|---|
|
@@ -31,6 +31,10 @@ Ref: https://keepachangelog.com/en/1.0.0/ | |||
|
||||
## [Unreleased] | ||||
|
||||
### Bug Fixes | ||||
|
||||
* [#19265](https://github.com/cosmos/cosmos-sdk/pull/19265) Reject denoms that contain a comma. | ||||
|
||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The improvement entry correctly documents the relocation of - * [#18857](https://github.com/cosmos/cosmos-sdk/pull/18857) Moved `FormatCoins` from `core/coins` to this package under `signing/textual`.
+ * [#18857](https://github.com/cosmos/cosmos-sdk/pull/18857) Moved `FormatCoins` from `core/coins` to this package under `signing/textual` for better modularity and clarity in coin formatting. Committable suggestion
Suggested change
|
||||
### Improvements | ||||
|
||||
* [#18857](https://github.com/cosmos/cosmos-sdk/pull/18857) Moved `FormatCoins` from `core/coins` to this package under `signing/textual`. | ||||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -284,6 +284,12 @@ func FormatCoins(coins []*basev1beta1.Coin, metadata []*bankv1beta1.Metadata) (s | |
if err != nil { | ||
return "", err | ||
} | ||
|
||
// If a coin contains a comma, return an error given that the output | ||
// could be misinterpreted by the user as 2 different coins. | ||
if strings.Contains(formatted[i], ",") { | ||
return "", fmt.Errorf("coin %s contains a comma", formatted[i]) | ||
} | ||
Comment on lines
+288
to
+292
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The addition of a check for commas in the formatted coin strings is a crucial update to prevent user confusion. However, consider adding a unit test to cover this new behavior, ensuring that coins containing commas indeed trigger an error as expected. This will help maintain the robustness of the Would you like me to help by adding a unit test for this scenario?
Comment on lines
+290
to
+292
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nit: curious if we can future proof this by using a regex instead, s.t. we fail if it doesn't match the regex expression. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We have a global regex that is configurable, I'm not sure how we should go about it |
||
} | ||
|
||
if len(coins) == 0 { | ||
Comment on lines
284
to
295
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Tip Codebase Verification Based on the output from the executed scripts, it appears that both the Analysis chain
Overall, the file's structure and the logic within the Scripts ExecutedThe following scripts were executed for the analysis: Script: #!/bin/bash
# Verify if documentation or changelog needs updates regarding the new error condition.
fd 'CHANGELOG.md' --exec grep -q 'FormatCoins' {} \; || echo "CHANGELOG.md needs an update for FormatCoins changes."
fd '\.md$' --exec grep -q 'FormatCoins' {} \; || echo "Documentation might need updates for FormatCoins changes."
Length of output: 344 |
||
|
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 changelog entry for the bug fix correctly links to the PR and succinctly describes the change. However, it could be more informative by briefly explaining the impact of the change, such as "to prevent misinterpretation of coin amounts."
Committable suggestion