chore(primitives): consolidated isValidHex usage #2031
Conversation
WalkthroughThe pull request introduces significant changes to the Changes
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 (
|
abi87
force-pushed
the
consilidate-isValidHex
branch
from
8f9f7a4
to
95a43f7
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2031 +/- ##
==========================================
- Coverage 22.42% 22.42% -0.01%
==========================================
Files 358 358
Lines 16012 16007 -5
Branches 12 12
==========================================
- Hits 3591 3589 -2
+ Misses 12272 12271 -1
+ Partials 149 147 -2
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range comments (2)
mod/primitives/pkg/encoding/hex/string.go (2)
Line range hint
58-65: LGTM! Consider adding a hex character validation.The updated
isValidHexfunction with generic type parameter is a good improvement. It enhances code reusability and type safety by handling both[]byteandstringtypes.Consider adding a check for valid hexadecimal characters after the prefix. This would make the function more robust:
func isValidHex[T []byte | string](str T) error { if len(str) == 0 { return ErrEmptyString } else if !has0xPrefix(str) { return ErrMissingPrefix } for _, c := range str[2:] { if !((c >= '0' && c <= '9') || (c >= 'a' && c <= 'f') || (c >= 'A' && c <= 'F')) { return errors.Wrapf(ErrInvalidString, "invalid hex character: %c", c) } } return nil }This addition ensures that all characters after the "0x" prefix are valid hexadecimal digits.
Line range hint
32-41: LGTM! Consider usingNewStringStrictfor consistency.The updated
UnmarshalTextmethod correctly uses the newisValidHexfunction for validation, which ensures consistency across the package. The error handling is appropriate and provides context.For even better consistency, consider using
NewStringStrictinstead of directly assigning the validated string:func (s *String) UnmarshalText(text []byte) error { str, err := NewStringStrict(text) if err != nil { return err } *s = str return nil }This change would ensure that all string creation goes through the same path, potentially catching any future invariants that might be added to
NewStringStrict.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
📒 Files selected for processing (2)
- mod/primitives/pkg/encoding/hex/format.go (2 hunks)
- mod/primitives/pkg/encoding/hex/string.go (2 hunks)
🔇 Additional comments (2)
mod/primitives/pkg/encoding/hex/string.go (1)
70-73: LGTM! Good simplification ofNewStringStrict.The refactored
NewStringStrictfunction is a clear improvement. It leverages the newisValidHexfunction for validation, which simplifies the code and improves maintainability. The error handling is consistent with the rest of the codebase.mod/primitives/pkg/encoding/hex/format.go (1)
61-71:⚠️ Potential issueClarify handling of missing prefix and empty input
In the
formatAndValidateTextfunction, whenisValidHex(input)returnsErrMissingPrefix, the function returnsnil, nilwith the comment// empty strings are allowed. This could conflate two cases:
- The input is empty.
- The input is non-empty but missing the
0xprefix.Returning
nil, nilin both cases might lead to unintended behavior. Consider explicitly checking for empty input and handling missing prefixes separately to avoid confusion and ensure correct processing.To verify how different inputs are handled, you can run the following script:
| if err := isValidHex(input); err != nil { | ||
| return *new(T), err |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Simplify zero value initialization for generic type T
In the formatAndValidateNumber function, using *new(T) to obtain a zero value of type T can be less readable and may cause confusion. Consider using var zero T for clarity and to align with Go conventions.
Apply this diff to improve readability:
if err := isValidHex(input); err != nil {
- return *new(T), err
+ var zero T
+ return zero, err
}📝 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.
| if err := isValidHex(input); err != nil { | |
| return *new(T), err | |
| if err := isValidHex(input); err != nil { | |
| var zero T | |
| return zero, err |
| if len(input) == 0 { | ||
| switch err := isValidHex(input); { | ||
| case err == nil: | ||
| input = input[2:] |
There was a problem hiding this comment.
can leave a comment to bypass nilaway given that isValidHex assumes that the string must be prefixed by 0x, so we know its safe to strip if err was nil
There was a problem hiding this comment.
Niaway is definitely allucinating here. Not sure how to suppress the error, since //nolint:nilawa does not seem to work
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
📒 Files selected for processing (2)
- mod/primitives/pkg/encoding/hex/format.go (2 hunks)
- mod/primitives/pkg/encoding/hex/string.go (2 hunks)
🔇 Additional comments (3)
mod/primitives/pkg/encoding/hex/format.go (2)
61-71: Improved error handling enhances code clarityRefactoring the error handling in
formatAndValidateTextusing a switch statement witherrors.Isimproves readability and maintainability. The logic correctly handles valid hex inputs, empty strings, and returns appropriate errors for invalid cases.
79-81: Use ofvar zero Timproves readabilityReplacing
*new(T)withvar zero Tfor zero value initialization enhances readability and aligns with Go conventions.mod/primitives/pkg/encoding/hex/string.go (1)
Line range hint
58-65: LGTM!The
isValidHexfunction has been correctly updated to accept a generic type parameterT []byte | string, allowing it to handle both[]byteandstringinputs seamlessly. The usage oflen(str)andhas0xPrefix(str)remains appropriate for both types.
| if len(input)%2 != 0 { | ||
| return nil, ErrOddLength | ||
| } | ||
| return input, nil | ||
| } | ||
|
|
||
| // formatAndValidateNumber checks the input text for a hex number. | ||
| func formatAndValidateNumber[T []byte | string](input T) (T, error) { | ||
| // realistically, this shouldn't rarely error if called on |
There was a problem hiding this comment.
Typographical error in comment
There's a typographical error in the comment which could cause confusion. Consider correcting it for clarity.
Suggested change:
-// realistically, this shouldn't rarely error if called on unwrapped hex.String
+// Realistically, this should rarely error if called on unwrapped hex.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.
| // realistically, this shouldn't rarely error if called on | |
| // Realistically, this should rarely error if called on unwrapped hex.String |
| if err := isValidHex(s); err != nil { | ||
| return "", err | ||
| } | ||
| return String(str), nil | ||
| return String(s), nil |
There was a problem hiding this comment.
Fix type conversion when returning String(s) with generic type T.
In NewStringStrict, s is of generic type T, which can be either []byte or string. Since String is defined as type String string, directly converting []byte to String without first converting it to string may lead to a compile-time error when s is a []byte. Ensure that s is converted to string before casting to String.
Apply the following diff to fix the type conversion:
func NewStringStrict[T []byte | string](s T) (String, error) {
if err := isValidHex(s); err != nil {
return "", err
}
- return String(s), nil
+ return String(string(s)), 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.
| if err := isValidHex(s); err != nil { | |
| return "", err | |
| } | |
| return String(str), nil | |
| return String(s), nil | |
| if err := isValidHex(s); err != nil { | |
| return "", err | |
| } | |
| return String(string(s)), nil |
|
Replaced by #2048 |
Conslidated
isValidHexusage via a generic function, instead of duplicating logicSummary by CodeRabbit
New Features
Bug Fixes
Refactor