fix(textual): error if a formatted coin contains a comma #19265
There are no files selected for viewing
| 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. 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. 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. 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. 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 |
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -327,5 +327,10 @@ | |
| {"text":"", "error": true}, | ||
| {"text":"1COSM", "error": true}, | ||
| {"text":"1 COSM", "error": true}, | ||
| {"text":" 1 COSM", "error": true}, | ||
| { | ||
| "proto": {"amount": "10000000", "denom": "point, 222222 point"}, | ||
| "metadata": {"display": "POINT", "base": "point", "denom_units": [{"denom": "point", "exponent": 0}, {"denom": "POINT", "exponent": 0}]}, | ||
| "error": true | ||
| } | ||
| ] | ||
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