-
Notifications
You must be signed in to change notification settings - Fork 511
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.
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
fix: throw error if hexToBytes
or hexToString
is provided a string that is not in hex
#2657
Conversation
@@ -15,25 +15,6 @@ | |||
* Eliminates 4 runtime dependencies: `base-x`, `base64-js`, `buffer`, and `ieee754`. | |||
* Execute test in a browser in addition to node | |||
|
|||
## 5.0.0 Beta 1 (2023-11-30) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This still feels relevant. There were releases. It also shows we released betas.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's not useful in the changelog IMO - it's already in the release list.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I personally find negative value in having beta info in changelogs, even when looking at other packages. I think it just clutters up the file. But if you disagree I'm happy to roll back this part of the change.
}) | ||
|
||
it('bytesToHex - bad hex', () => { | ||
expect(() => hexToBytes('hello')).toThrow(new Error('Invalid hex string')) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this be a special Error that can be more easily detected?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is in the unit tests, so I don't expect a special error code to be useful here. On the other hand, I feel the main code base could make use of a special error code / error status message
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah that is what I mean.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I checked what errors were used in the rest of the package and it was just Error
, so I just copied the norm
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hmm. Assigning special error codes (for all errors) might be a significant deviation from the intent of the PR. I'm okay if it's not included as a part of this change list
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Apart from the missing test case (I've tagged you on the comment), I'm okay with the rest of the PR.
I don't have a strong opinion on removing the beta changelist information. I'm weakly in favor of removing the information
@@ -1,5 +1,7 @@ | |||
import { concatBytes } from '@noble/hashes/utils' | |||
|
|||
export const HEX_REGEX = /^[A-F0-9]*$/iu |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we want to change from *
to +
to prevent empty strings? @mvadari
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I purposefully kept support for empty strings - I don't see any reason why these functions shouldn't support empty strings, which would convert to an empty bytestring or empty ASCII string.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, but in what cases would an empty string be a legitimate input?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One example: your app has a field that accepts memos and runs the result through convertHexToString
…g that is not in hex (XRPLF#2657) * better error handling * fix browser tests * add shared variable * re-add test case
…g that is not in hex (XRPLF#2657) * better error handling * fix browser tests * add shared variable * re-add test case
High Level Overview of Change
Title says it all.
Context of Change
The current methods return an empty bytes object/string if they are not provided a string in hex, which caused an issue.
Type of Change
Did you update
HISTORY.md
?I also cleaned up the other
HISTORY.md
files.Test Plan
Added tests, they pass.