fix(textual): error if a formatted coin contains a comma #19265
Conversation
WalkthroughThe recent update introduces a significant change to the Changes
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 as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
This comment has been minimized.
This comment has been minimized.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
| // 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]) | ||
| } |
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 FormatCoins function.
Would you like me to help by adding a unit test for this scenario?
| 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]) | ||
| } | ||
| } | ||
|
|
||
| if len(coins) == 0 { |
There was a problem hiding this comment.
Tip
Codebase Verification
Based on the output from the executed scripts, it appears that both the CHANGELOG.md and the documentation might require updates to reflect the changes made to the FormatCoins function. This suggests that the original review comment about ensuring documentation and changelog entries are updated is valid and actionable.
Analysis chain
📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [1-292]
Overall, the file's structure and the logic within the FormatCoins function appear sound and well-organized. The changes made align with the PR's objectives to enhance clarity in coin formatting. It's important to ensure that all related documentation and changelog entries are updated to reflect this new error condition for future reference and SDK users' awareness.
Scripts Executed
The 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
| if strings.Contains(formatted[i], ",") { | ||
| return "", fmt.Errorf("coin %s contains a comma", formatted[i]) | ||
| } |
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
| @@ -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.
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."
- * [#19265](https://github.com/cosmos/cosmos-sdk/pull/19265) Reject denoms that contain a comma.
+ * [#19265](https://github.com/cosmos/cosmos-sdk/pull/19265) Reject denoms that contain a comma to prevent misinterpretation of coin amounts.Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| * [#19265](https://github.com/cosmos/cosmos-sdk/pull/19265) Reject denoms that contain a comma. | |
| * [#19265](https://github.com/cosmos/cosmos-sdk/pull/19265) Reject denoms that contain a comma to prevent misinterpretation of coin amounts. |
| ### Bug Fixes | ||
|
|
||
| * [#19265](https://github.com/cosmos/cosmos-sdk/pull/19265) Reject denoms that contain a comma. | ||
|
|
There was a problem hiding this comment.
The improvement entry correctly documents the relocation of FormatCoins. However, it's missing a brief rationale for why this change is beneficial, which could help in understanding the context of the improvement.
- * [#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
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
Description
Currently if a coin is
amount: 123 denom: ATOM, 123 STAKE, it will be displayed to the user in a screen as123 ATOM, 123 STAKE. Now with this fix, the value renderer will error if it sees a comma.This is not an issue for x/bank transactions given that we check with a regex those denoms, but it could be an issue for any other module that does not check for denoms when handling coins.
There could be other cases in which a
;could be passed and the user mistakenly takes it as a separator, but I don't think we should make the checks in the formatter that extensiveAuthor 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.
I have...