Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

chore(primitives): consolidated isValidHex usage #2031

Closed
wants to merge 4 commits into from
Closed

chore(primitives): consolidated isValidHex usage #2031

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
95a43f7
consolidated isValidHex usage
abi87 Oct 1, 2024
de4268b
fixed special case
abi87 Oct 1, 2024
1b7d63b
Simplified zero value initialization for generic type T
abi87 Oct 1, 2024
9710e4d
Merge branch 'main' into consilidate-isValidHex
abi87 Oct 2, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
28 changes: 14 additions & 14 deletions mod/primitives/pkg/encoding/hex/format.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,8 @@

package hex

import "errors"

// has0xPrefix returns true if s has a 0x prefix.
func has0xPrefix[T []byte | string](s T) bool {
return len(s) >= 2 && s[0] == '0' && (s[1] == 'x' || s[1] == 'X')
Expand Down Expand Up @@ -56,28 +58,26 @@

// formatAndValidateText validates the input text for a hex string.
func formatAndValidateText(input []byte) ([]byte, error) {
if len(input) == 0 {
switch err := isValidHex(input); {
case err == nil:
input = input[2:]

Check failure on line 63 in mod/primitives/pkg/encoding/hex/format.go

View workflow job for this annotation

GitHub Actions / nilaway 

error: Potential nil panic detected. Observed nil flow from source to dereference point:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Niaway is definitely allucinating here. Not sure how to suppress the error, since //nolint:nilawa does not seem to work

if len(input)%2 != 0 {
return nil, ErrOddLength
}
return input, nil
case errors.Is(err, ErrEmptyString):
return nil, nil // empty strings are allowed
default:
return nil, err
}
if !has0xPrefix(input) {
return nil, ErrMissingPrefix
}
input = input[2:]
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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

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.

Suggested change
// realistically, this shouldn't rarely error if called on
// Realistically, this should rarely error if called on unwrapped hex.String

// unwrapped hex.String
if len(input) == 0 {
return *new(T), ErrEmptyString
}
if !has0xPrefix(input) {
return *new(T), ErrMissingPrefix
if err := isValidHex(input); err != nil {
return *new(T), err
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 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.

Suggested change
if err := isValidHex(input); err != nil {
return *new(T), err
if err := isValidHex(input); err != nil {
var zero T
return zero, err

}
input = input[2:]
if len(input) == 0 {
Expand Down
11 changes: 4 additions & 7 deletions mod/primitives/pkg/encoding/hex/string.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,7 +55,7 @@ func (s *String) UnmarshalText(text []byte) error {
return nil
}

func isValidHex(str string) error {
func isValidHex[T []byte | string](str T) error {
if len(str) == 0 {
return ErrEmptyString
} else if !has0xPrefix(str) {
Expand All @@ -67,13 +67,10 @@ func isValidHex(str string) error {
// NewStringStrict creates a hex string with 0x prefix. It errors if any of the
// string invariants are violated.
func NewStringStrict[T []byte | string](s T) (String, error) {
str := string(s)
if len(str) == 0 {
return "", ErrEmptyString
} else if !has0xPrefix(str) {
return "", ErrMissingPrefix
if err := isValidHex(s); err != nil {
return "", err
}
return String(str), nil
return String(s), nil
Comment on lines +70 to +73
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

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.

Suggested change
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

}

// FromBytes creates a hex string with 0x prefix.
Expand Down
Loading