test(x/tx): add amino JSON pretty bytes demonstration test #21935
Conversation
kocubinski
requested review from
testinginprod,
aaronc and
a team
as code owners
📝 WalkthroughWalkthroughThe pull request includes changes to several files, primarily focusing on the removal of the Changes
Possibly related PRs
Suggested reviewers
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 using PR comments)
Other keywords and placeholders
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
x/tx/signing/aminojson/json_marshal_test.go (1)
437-442: Consider adding error handling for the custom encoding functionThe custom field encoding function for "hex" is concise and clear. However, it might be beneficial to add error handling for the
Fprintfcall, even though it's unlikely to fail for byte slices.Consider updating the custom encoding function as follows:
encoder.DefineFieldEncoding( "hex", func(enc *aminojson.Encoder, v protoreflect.Value, w io.Writer) error { - _, err := fmt.Fprintf(w, "\"%x\"", v.Bytes()) - return err + n, err := fmt.Fprintf(w, "\"%x\"", v.Bytes()) + if err != nil { + return err + } + if n == 0 { + return fmt.Errorf("no bytes written") + } + return nil })
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
📒 Files selected for processing (4)
- buf.work.yaml (0 hunks)
- x/tx/signing/aminojson/internal/testpb/test.proto (1 hunks)
- x/tx/signing/aminojson/internal/testpb/test.pulsar.go (16 hunks)
- x/tx/signing/aminojson/json_marshal_test.go (2 hunks)
💤 Files not reviewed due to no reviewable changes (1)
- buf.work.yaml
🧰 Additional context used
📓 Path-based instructions (2)
x/tx/signing/aminojson/internal/testpb/test.pulsar.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.x/tx/signing/aminojson/json_marshal_test.go (2)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Pattern
**/*_test.go: "Assess the unit test code assessing sufficient code coverage for the changes associated in the pull request"
🔇 Additional comments (18)
x/tx/signing/aminojson/internal/testpb/test.proto (3)
48-48: LGTM: New fieldpretty_bytesadded correctlyThe addition of the
pretty_bytesfield with theamino.encodingoption set to "hex" is well-implemented. This change aligns with the PR objective of demonstrating a "pretty" bytes encoder for arbitrary bytes. The field is correctly placed at the end of the message (field number 20), which maintains backward compatibility.
35-47: Formatting improvements enhance readabilityThe formatting changes in these lines improve the overall readability and consistency of the Protocol Buffer definition. The adjustments in spacing make the field definitions more uniform and easier to scan. These changes align with good coding practices for
.protofiles.
35-48: Changes align well with PR objectivesThe modifications in this file, particularly the addition of the
pretty_bytesfield and the formatting improvements, successfully address the PR's objective of demonstrating how to set up a "pretty" bytes encoder for arbitrary bytes. The use of theamino.encodingoption with "hex" value provides a clear example of how to achieve the desired encoding. These changes contribute to enhancing the functionality and readability of the amino JSON encoding process within the Cosmos SDK.x/tx/signing/aminojson/json_marshal_test.go (2)
419-447: LGTM: Well-structured test demonstrating custom byte encodingThis test function effectively demonstrates the use of custom byte encoding for the
ABitOfEverythingmessage. It compares the default encoding with a custom hex encoding, showcasing the flexibility of theaminojson.Encoder.The test structure follows Go best practices, using
requireassertions for clear error reporting. The use of SHA-256 for generating test bytes is a good choice for reproducibility.
443-446: LGTM: Effective comparison of legacy and custom encoding resultsThe comparison between legacy JSON and custom amino JSON encoding is well-implemented. The use of
require.NotEqualensures that the custom encoding produces different results from the legacy encoding, which is the expected behavior.The logging of hex-encoded bytes is helpful for debugging and understanding the output.
x/tx/signing/aminojson/internal/testpb/test.pulsar.go (13)
1758-1775: Field Descriptor forPrettyBytesAdded CorrectlyThe declaration and initialization of
fd_ABitOfEverything_pretty_bytesensure that thePrettyBytesfield is properly integrated into the protobuf reflection mechanism.Also applies to: 1797-1797
1961-1966: Inclusion ofPrettyBytesinRangeMethodThe
Rangemethod correctly handles thePrettyBytesfield, allowing it to be iterated over alongside other populated fields.
2014-2015: Accurate Implementation ofHasforPrettyBytesThe
Hasmethod correctly checks thePrettyBytesfield for non-zero length, ensuring accurate presence detection.
2064-2065: Proper Clearing ofPrettyBytesinClearMethodThe
Clearmethod appropriately setsPrettyBytestonil, effectively clearing its value.
2133-2135: Correct Retrieval inGetMethod forPrettyBytesThe
Getmethod accurately retrieves the value ofPrettyBytes, returning it as aprotoreflect.ValueOfBytes.
2190-2191: Appropriate Assignment inSetMethod forPrettyBytesThe
Setmethod correctly assigns the provided bytes value to thePrettyBytesfield.
2251-2252: Expected Panic inMutableMethod forPrettyBytesThe
Mutablemethod correctly panics when called for the non-compositePrettyBytesfield, consistent with the handling of similar scalar fields.
2300-2301:NewFieldMethod Returns Default forPrettyBytesThe
NewFieldmethod properly returns a defaultnilvalue for thePrettyBytesfield, aligning with expected behavior.
2426-2429: Inclusion ofPrettyBytesin Size CalculationThe size computation within
ProtoMethodsaccurately accounts for thePrettyBytesfield, ensuring correct message sizing.
2459-2467: Correct Serialization inMarshalMethod forPrettyBytesThe
Marshalmethod properly serializes thePrettyBytesfield, handling length encoding and field numbering correctly.
3018-3051: Accurate Deserialization inUnmarshalMethod forPrettyBytesThe
Unmarshalmethod correctly deserializes thePrettyBytesfield, including handling of length prefixes and data copying.
4265-4265: Addition ofPrettyBytesField toABitOfEverythingStructThe new
PrettyBytes []bytefield is correctly added to theABitOfEverythingstruct with appropriate protobuf tags and JSON annotations.
4400-4405: Correct Implementation ofGetPrettyBytesMethodThe
GetPrettyBytesaccessor method is properly implemented, providing safe access to thePrettyBytesfield.
julienrbrt
changed the title
Description
Related: #21850, #15740
This adds a test showing how a developer could set up a "pretty" bytes encoder for arbitrary bytes, which are now supported as a signer field. The default is
base64encoding, as this is theencoding/jsonstd implementation spec.Also fixed a problem in the buf work file, as this was causing the error below when regenerating test pbs.
Test output with the hex encoder:
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.
Please see Pull Request Reviewer section in the contributing guide for more information on how to review a pull request.
I have...
Summary by CodeRabbit
New Features
pretty_bytesto theABitOfEverythingmessage for enhanced data handling.pretty_bytesfield, allowing for hexadecimal string formatting.Bug Fixes
test.protofile.Tests
ABitOfEverythingmessage with the new field.