fix: Use canonical hexadecimal strings for Eip155 address encoding #2120
Conversation
WalkthroughThe changes in this pull request include updates to the Changes
Assessment against linked issues
Possibly related PRs
Suggested reviewers
Poem
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
CodeRabbit Configuration File (
|
CalicoNino
changed the title
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2120 +/- ##
==========================================
- Coverage 64.80% 64.79% -0.01%
==========================================
Files 273 273
Lines 21638 21636 -2
==========================================
- Hits 14022 14020 -2
Misses 6644 6644
Partials 972 972
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
CHANGELOG.md(2 hunks)eth/eip55.go(1 hunks)eth/eip55_test.go(3 hunks)x/evm/keeper/grpc_query.go(1 hunks)
🔇 Additional comments (5)
x/evm/keeper/grpc_query.go (1)
653-653: Comment Update is Appropriate
The updated comment accurately reflects the function's operation.
eth/eip55_test.go (3)
119-127: Update Expected JSON Values in Tests
The changes to expectedJson in TestProtobufEncoding should align with the serialization methods. If the serialization is corrected as per the earlier comments, ensure that the expected JSON values in the tests are updated accordingly.
143-143: Review Assertion for Marshaled Bytes
The assertion comparing the marshaled bytes may not be valid if the serialization method is corrected. Please verify that the test accurately reflects the expected output after fixing the serialization methods.
151-152: Correct Size Method Assertions
The assertions for the Size method should be updated to reflect the corrected size calculation. Ensure that the tests validate the size based on the raw byte length of the address.
CHANGELOG.md (1)
48-48: Changelog Entry Added for EIP155 Address Encoding Fix
The changelog entry correctly documents the fix related to the use of canonical hexadecimal strings for EIP155 address encoding.
| } | ||
|
|
||
| // MarshalTo serializes a EIP55Addr directly into a pre-allocated byte slice ("data"). | ||
| // MarshalTo implements the gogo proto custom type interface. | ||
| // Implements the gogo proto custom type interface. | ||
| // Ref: https://github.com/cosmos/gogoproto/blob/v1.5.0/custom_types.md | ||
| func (h *EIP55Addr) MarshalTo(data []byte) (n int, err error) { | ||
| copy(data, h.Bytes()) | ||
| copy(data, []byte(h.Address.Hex())) |
There was a problem hiding this comment.
Incorrect Serialization in MarshalTo Method
The MarshalTo method should copy the raw bytes of the address into the data slice to conform with protobuf serialization requirements. Copying the hexadecimal string representation ([]byte(h.Address.Hex())) is inappropriate and may cause serialization issues.
Suggested fix:
func (h *EIP55Addr) MarshalTo(data []byte) (n int, err error) {
- copy(data, []byte(h.Address.Hex()))
+ copy(data, h.Address.Bytes())
return h.Size(), nil
}📝 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. Thoroughly test & benchmark the code to ensure it meets the requirements.
| copy(data, []byte(h.Address.Hex())) | |
| copy(data, h.Address.Bytes()) |
| @@ -36,53 +35,49 @@ | |||
| // Marshal implements the gogo proto custom type interface. | |||
| // Ref: https://github.com/cosmos/gogoproto/blob/v1.5.0/custom_types.md | |||
| func (h EIP55Addr) Marshal() ([]byte, error) { | |||
| return h.Bytes(), nil | |||
| return []byte(h.Address.Hex()), nil | |||
There was a problem hiding this comment.
Incorrect Protobuf Serialization Format in Marshal Method
The Marshal method should return the raw bytes of the address to conform with protobuf serialization expectations. Returning the hexadecimal string representation ([]byte(h.Address.Hex())) may lead to serialization issues.
Suggested fix:
func (h EIP55Addr) Marshal() ([]byte, error) {
- return []byte(h.Address.Hex()), nil
+ return h.Address.Bytes(), nil
}📝 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. Thoroughly test & benchmark the code to ensure it meets the requirements.
| return []byte(h.Address.Hex()), nil | |
| return h.Address.Bytes(), nil |
| } | ||
|
|
||
| // MarshalJSON returns the [EIP55Addr] as JSON bytes. | ||
| // Implements the gogo proto custom type interface. | ||
| // Ref: https://github.com/cosmos/gogoproto/blob/v1.5.0/custom_types.md | ||
| func (h EIP55Addr) MarshalJSON() ([]byte, error) { | ||
| return json.Marshal(h.String()) | ||
| return []byte(h.String()), nil |
There was a problem hiding this comment.
Invalid JSON Encoding in MarshalJSON Method
Directly returning []byte(h.String()) bypasses JSON encoding and may result in invalid JSON output. The MarshalJSON method should use json.Marshal(h.String()) to ensure proper JSON formatting and escaping.
Proposed fix:
func (h EIP55Addr) MarshalJSON() ([]byte, error) {
- return []byte(h.String()), nil
+ return json.Marshal(h.String())
}📝 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. Thoroughly test & benchmark the code to ensure it meets the requirements.
| return []byte(h.String()), nil | |
| return json.Marshal(h.String()) |
| addr, err := NewEIP55AddrFromStr(string(data)) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| *h = addr |
There was a problem hiding this comment.
Incorrect Deserialization in Unmarshal Method
The Unmarshal method should reconstruct the address from the raw bytes provided, not from a string representation. Converting data to a string and parsing it may result in incorrect deserialization, especially if the original data is not a valid string.
Suggested fix:
func (h *EIP55Addr) Unmarshal(data []byte) error {
- addr, err := NewEIP55AddrFromStr(string(data))
- if err != nil {
- return err
- }
- *h = addr
+ if len(data) != gethcommon.AddressLength {
+ return fmt.Errorf("invalid EIP55Addr length: %d", len(data))
+ }
+ h.Address = gethcommon.BytesToAddress(data)
return nil
}📝 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. Thoroughly test & benchmark the code to ensure it meets the requirements.
| addr, err := NewEIP55AddrFromStr(string(data)) | |
| if err != nil { | |
| return err | |
| } | |
| *h = addr | |
| if len(data) != gethcommon.AddressLength { | |
| return fmt.Errorf("invalid EIP55Addr length: %d", len(data)) | |
| } | |
| h.Address = gethcommon.BytesToAddress(data) | |
| return nil |
| } | ||
|
|
||
| addr, err := NewEIP55AddrFromStr(*text) | ||
| addr, err := NewEIP55AddrFromStr(string(bz)) |
There was a problem hiding this comment.
Incorrect JSON Decoding in UnmarshalJSON Method
The UnmarshalJSON method should use json.Unmarshal to correctly parse the JSON-encoded data. Directly converting bz to a string and parsing it may fail to handle JSON encoding properly, especially with quotation marks.
Proposed fix:
func (h *EIP55Addr) UnmarshalJSON(bz []byte) error {
- addr, err := NewEIP55AddrFromStr(string(bz))
+ var addrStr string
+ if err := json.Unmarshal(bz, &addrStr); err != nil {
+ return err
+ }
+ addr, err := NewEIP55AddrFromStr(addrStr)
if err != nil {
return err
}
*h = addr
return nil
}Committable suggestion skipped: line range outside the PR's diff.
CalicoNino
changed the title
Purpose / Abstract
Summary by CodeRabbit
Release Notes
Documentation
New Features
Bug Fixes
Tests