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

fix: return "0x00" for normalize(0) #306

Closed
wants to merge 2 commits into from
Closed

fix: return "0x00" for normalize(0) #306

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
8f9c3a2
fix: return "0x00" for normalize(0)
legobeat Apr 25, 2023
a78a5dd
Merge branch 'main' into fix-normalize-0x0
legobeat May 1, 2023
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
6 changes: 6 additions & 0 deletions src/utils.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -111,6 +111,12 @@ describe('normalize', function () {
expect(result).toBe(`0x${initial.toLowerCase()}`);
});

it('should normalize 0 to a byte-pair hex string', function () {
const initial = 0;
const result = normalize(initial);
expect(result).toBe('0x00');
});

it('should normalize an integer to a byte-pair hex string', function () {
const initial = 1;
const result = normalize(initial);
Expand Down
8 changes: 4 additions & 4 deletions src/utils.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -103,10 +103,6 @@ export function recoverPublicKey(
* @returns The normalized value.
*/
export function normalize(input: number | string): string | undefined {
if (!input) {
return undefined;
}

if (typeof input === 'number') {
if (input < 0) {
Copy link
Contributor Author

@legobeat legobeat Apr 25, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

@adonesky1 @kumavis This stood out to me. Do you recall the context on why this returns 0x for all negative numbers instead of throwing like it does on other invalid input?
1e4f176#diff-39b2554fd18da165b59a6351b1aafff3714e2a80c1435f2de9706355b4d32351R111

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, I guess the intention is to throw on negative numbers.

eth-sig-util/src/utils.test.ts

Line 120 in f33a032

// TODO: Add validation to disallow negative integers.

I can address that in a separate change (as it's a breaking one) after this one has been merged and released.

return '0x';
Expand All @@ -115,6 +111,10 @@ export function normalize(input: number | string): string | undefined {
input = bufferToHex(buffer);
}

if (!input) {
return undefined;
}

if (typeof input !== 'string') {
let msg = 'eth-sig-util.normalize() requires hex string or integer input.';
msg += ` received ${typeof input}: ${input as any as string}`;
Expand Down