feat(x/tx): add enum as string encoder option#19618
Conversation
|
Warning Rate Limit Exceeded@julienrbrt has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 14 minutes and 57 seconds before requesting another review. How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. WalkthroughThe overarching change involves enhancing the encoding process to allow enums to be represented as strings in JSON outputs, addressing issues related to readability and data representation consistency. This update includes fixing issues with denomination strings containing commas and restructuring coin formatting functionalities. The modifications span across various components, introducing an Changes
Assessment against linked issues
The modifications appear to directly address the primary concerns highlighted in the linked issue, especially regarding the readability of enums in JSON outputs. However, there is a slight ambiguity regarding the exact compatibility with previous versions' enum representations, which could benefit from further clarification to ensure complete alignment with the objectives. 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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Status
Actionable comments generated: 2
Configuration used: .coderabbit.yml
Files selected for processing (5)
- client/v2/autocli/query.go (1 hunks)
- x/tx/CHANGELOG.md (1 hunks)
- x/tx/signing/aminojson/encoder.go (2 hunks)
- x/tx/signing/aminojson/json_marshal.go (9 hunks)
- x/tx/signing/aminojson/json_marshal_test.go (1 hunks)
Additional comments: 8
x/tx/signing/aminojson/encoder.go (2)
- 78-78: Adding a
nilargument as a placeholder to themarshalListfunction call. Ensure that themarshalListfunction is designed to handlenilappropriately in this context. Ifnilrepresents a specific condition or default behavior, this should be clearly documented in themarshalListfunction's comments.- 164-164: Passing
pubkeysFieldas an argument to themarshalListfunction. This change seems to be intended to provide additional context or customization for the marshaling process. Verify that themarshalListfunction utilizes this argument effectively and that this change aligns with the intended functionality. Additionally, ensure that this does not introduce any unintended side effects, especially in scenarios wherepubkeysFieldmight not be set or could be different from expected.client/v2/autocli/query.go (1)
- 114-114: The addition of
EnumAsString: truetoEncoderOptionsin theBuildQueryMethodCommandfunction is a significant change aimed at improving the readability of query responses by encoding enums as strings. This change aligns with the PR's objectives to enhance user experience. Ensure that all relevant parts of the codebase that rely on enum encoding are tested with this new behavior to prevent any unintended consequences. Additionally, consider documenting this behavior in relevant user-facing documentation to inform users about how enums will be represented in query responses.x/tx/CHANGELOG.md (1)
- 44-44: The documentation of the bug fix related to rejecting denoms containing a comma is clear and concise, placed correctly under the "Bug Fixes" section. This entry helps users understand the specific issue addressed by the PR. Ensure that the PR link provided is accurate and leads to the correct issue or PR addressing this bug fix.
x/tx/signing/aminojson/json_marshal.go (4)
- 31-32: The addition of the
EnumAsStringfield inEncoderOptionsis correctly implemented. This field allows users to specify whether enums should be encoded as strings, aligning with the PR's objective to improve the readability of query responses.- 50-50: The
enumsAsStringfield added to theEncoderstruct correctly reflects theEnumAsStringoption fromEncoderOptions. This field is essential for controlling the encoding behavior of enums throughout the encoding process.- 84-84: The assignment of the
enumsAsStringfield fromEncoderOptionsto theEncoderstruct in theNewEncoderfunction is correctly implemented. This ensures that the encoder respects theEnumAsStringoption when encoding enums.- 222-241: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [196-237]
The logic in the
marshalmethod to encode enums as strings based on theEnumAsStringoption is correctly implemented. It checks ifenumsAsStringis true and if the field descriptor is not nil before attempting to encode the enum as a string. If the descriptor is found, it writes the enum's name as a string; otherwise, it falls back to encoding the enum as an integer. This approach is consistent with the PR's goal to enhance readability by allowing enums to be encoded as strings.
| ### Features | ||
|
|
||
| * [#19265](https://github.com/cosmos/cosmos-sdk/pull/19265) Reject denoms that contain a comma. | ||
| * * [#](https://github.com/cosmos/cosmos-sdk/pull/) Add enum as string option to encoder. |
There was a problem hiding this comment.
The addition of the feature to add an enum as a string option to the encoder is accurately documented under the "Features" section. This entry clearly communicates the enhancement made to the SDK, aligning with the PR's objectives. Ensure that the placeholder link [#](https://github.com/cosmos/cosmos-sdk/pull/) is replaced with the actual PR link for proper referencing.
- * * [#](https://github.com/cosmos/cosmos-sdk/pull/) Add enum as string option to encoder.
+ * * [#19618](https://github.com/cosmos/cosmos-sdk/pull/19618) Add enum as string option to encoder.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.
| * * [#](https://github.com/cosmos/cosmos-sdk/pull/) Add enum as string option to encoder. | |
| * * [#19618](https://github.com/cosmos/cosmos-sdk/pull/19618) Add enum as string option to encoder. |
There was a problem hiding this comment.
Review Status
Actionable comments generated: 2
Configuration used: .coderabbit.yml
Files selected for processing (4)
- client/v2/CHANGELOG.md (1 hunks)
- x/tx/CHANGELOG.md (1 hunks)
- x/tx/signing/aminojson/encoder.go (2 hunks)
- x/tx/signing/aminojson/json_marshal.go (9 hunks)
Files skipped from review as they are similar to previous changes (3)
- x/tx/CHANGELOG.md
- x/tx/signing/aminojson/encoder.go
- x/tx/signing/aminojson/json_marshal.go
Additional comments: 2
client/v2/CHANGELOG.md (2)
- 49-49: The entry for PR #19216 clearly communicates the improvement of using the TxConfig provided in the context directly. This change promotes better consistency and reliability in transaction configuration, aligning well with the PR objectives and changelog best practices.
- 49-49: The "API Breaking Changes" section effectively communicates the removal of address codecs from
autocli.AppOptionsandflag.Builder, a significant change that developers need to be aware of. This entry is clear, accurate, and aligns well with the principles of a good changelog by informing developers of changes that may require adjustments in their implementations.
|
When constructing an amino JSON encoder for signing should we ensure that this new option is always set to false? cosmos-sdk/x/tx/signing/aminojson/aminojson.go Lines 43 to 50 in aa9ff3d |
kocubinski
left a comment
kocubinski
left a comment
There was a problem hiding this comment.
Marshaling mechanics look good, but maybe we should ensure this option is false in aminojson.NewSignModeHandler. toggling on/off will produce different sign bytes.
|
That makes sense, I'll update to explicitly setting it to false there. |
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: .coderabbit.yml
Files selected for processing (3)
- x/tx/CHANGELOG.md (2 hunks)
- x/tx/signing/aminojson/aminojson.go (1 hunks)
- x/tx/signing/aminojson/json_marshal.go (9 hunks)
Files skipped from review as they are similar to previous changes (1)
- x/tx/signing/aminojson/json_marshal.go
Additional comments: 4
x/tx/signing/aminojson/aminojson.go (1)
- 47-47: The addition of
EnumAsString: falseexplicitly sets enums to be encoded as numbers by default. This change is clear and aligns with the PR's objectives. However, it would be beneficial to add a comment explaining why this default is chosen, especially since the PR aims to enhance readability by allowing enums to be encoded as strings. This will help future maintainers understand the decision-making process.x/tx/CHANGELOG.md (3)
- 38-38: The changelog entry clearly documents the addition of the enum as string encoding option, including a reference to the PR. This entry is well-formatted and adheres to the changelog's guiding principles.
- 42-42: The changelog entry accurately documents the restructuring of
FormatCoinsfromcore/coinstosigning/textual. This entry is clear and provides the necessary context for understanding the improvement.- 46-46: The changelog entry concisely documents the bug fix related to rejecting denominations that contain a comma. This entry is clear and accurately reflects the nature of the bug fix.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: .coderabbit.yml
Files selected for processing (4)
- x/tx/go.mod (2 hunks)
- x/tx/go.sum (1 hunks)
- x/tx/signing/textual/internal/testdata/e2e.json (3 hunks)
- x/tx/signing/textual/internal/testdata/tx.json (4 hunks)
Additional comments: 10
x/tx/go.mod (1)
- 38-40: The comment added provides clear context on why specific versions of dependencies are retained, referencing an unresolved issue. This is a good practice for maintaining clarity in decision-making regarding dependency management.
x/tx/go.sum (2)
- 1-2: The updates to module versions for
cosmossdk.io/apiandcosmossdk.io/coreare reflected correctly in the checksums, ensuring integrity and consistency with thego.modfile.- 1-1: The removal of references to
buf.build/gen/go/cosmos/gogo-proto/protocolbuffers/goandbuf.build/gen/go/tendermint/tendermint/protocolbuffers/golikely indicates a cleanup or update in dependency management, which is a positive change for maintaining a clean and current dependency graph.x/tx/signing/textual/internal/testdata/tx.json (4)
- 145-145: The change from a placeholder or previous enum value to "VOTE_OPTION_YES" aligns with the PR's objective to improve readability and clarity in query responses by using string representations of enums. This change is consistent and correctly implemented within the context of the provided JSON data.
- 196-196: The update to "VOTE_OPTION_YES" in this instance further supports the PR's goal of enhancing the user experience by providing more descriptive and human-readable enum values in query responses. This modification is accurately reflected in the JSON structure.
- 229-229: Similar to the previous comments, changing the enum value to "VOTE_OPTION_YES" here is in line with the PR's intention to use string representations for enums. This change is correctly applied and contributes to the overall improvement of query response readability.
- 342-342: This final instance of updating the enum value to "VOTE_OPTION_YES" is consistent with the PR's objectives and the changes made in other parts of the JSON file. It correctly implements the desired enhancement of using string values for enums to improve readability.
x/tx/signing/textual/internal/testdata/e2e.json (3)
- 216-216: The change from "VOTE_OPTION_ONE" to "VOTE_OPTION_YES" in the
MsgVotemessage aligns with the PR's objective to enhance readability and clarity in query responses. This update is consistent with the intention to use string representations for enum values, making the output more understandable for users.- 329-329: The inclusion of detailed metadata within the
MsgVotemessage, ending with a single ampersand "@", is an interesting choice. While it serves as a placeholder or example, ensure that such content is appropriate and intended for demonstration purposes. If this is meant to simulate real-world metadata, it's effectively demonstrating the capability to handle long strings and special characters.- 377-377: The
cborfield contains a long string representing CBOR-encoded data. While the review focuses on JSON changes, it's important to ensure that any updates to the JSON structure are reflected in the corresponding CBOR representation if necessary. This might require additional verification or testing to ensure the CBOR data remains accurate and consistent with the JSON structure changes.
3 participants
Description
Closes: #19456
The backport will only add the option to client/v2 encoder options.
After this PR I'll tag x/tx.
ref: #19530
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.
I have...
Summary by CodeRabbit
FormatCoinsfor enhanced efficiency.