Skip to content

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

Closed
legobeat wants to merge 2 commits intoMetaMask:mainfrom
legobeat:fix-normalize-0x0
Closed

fix: return "0x00" for normalize(0)#306
legobeat wants to merge 2 commits intoMetaMask:mainfrom
legobeat:fix-normalize-0x0

Conversation

@legobeat
Copy link
Copy Markdown
Contributor

@legobeat legobeat commented Apr 25, 2023 •
edited
Loading

The normalize function returns 0x${number} for all numbers except 0, for which it returns undefined. This changes it to return 0x00 for 0.

legobeat
legobeat commented Apr 25, 2023
View reviewed changes
src/utils.ts
}

if (typeof input === 'number') {
if (input < 0) {
Copy link
Copy Markdown
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
Copy Markdown
Contributor Author

@legobeat legobeat Apr 25, 2023

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.

@legobeat legobeat requested review from adonesky1 and kumavis April 25, 2023 02:32
@legobeat legobeat marked this pull request as ready for review April 25, 2023 02:32
@legobeat legobeat requested a review from a team as a code owner April 25, 2023 02:32
@legobeat legobeat force-pushed the fix-normalize-0x0 branch from b29aaf8 to 7e736dc Compare April 25, 2023 02:35
@legobeat legobeat changed the title fix: return "0x0" for normalize(0) fix: return "0x00" for normalize(0) Apr 25, 2023
kumavis
kumavis previously approved these changes Apr 26, 2023
View reviewed changes
Copy link
Copy Markdown
Member

@kumavis kumavis left a comment

Choose a reason for hiding this comment

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

good

this is a breaking change

@legobeat legobeat requested review from Gudahtt and Mrtenz April 27, 2023 11:11
Gudahtt
Gudahtt previously approved these changes Apr 27, 2023
View reviewed changes
Copy link
Copy Markdown
Member

@Gudahtt Gudahtt left a comment

Choose a reason for hiding this comment

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

LGTM!

@legobeat legobeat dismissed stale reviews from Gudahtt and kumavis via 8f9c3a2 April 29, 2023 00:42
@legobeat legobeat force-pushed the fix-normalize-0x0 branch from 7e736dc to 8f9c3a2 Compare April 29, 2023 00:42
@legobeat legobeat requested review from Gudahtt and kumavis April 29, 2023 00:43
@legobeat
Copy link
Copy Markdown
Contributor Author

legobeat commented May 1, 2023 •
edited
Loading

Not certain if this should be merged and released before or after #273.

@mikesposito
Copy link
Copy Markdown
Member

mikesposito commented Jun 22, 2023

I didn't notice this PR before opening this one: #315
@legobeat I am thinking if maybe #315 could solve the problem you are trying to fix?

@legobeat legobeat mentioned this pull request Jun 23, 2023
Merged
@mcmire
Copy link
Copy Markdown

mcmire commented Jun 23, 2023 •
edited
Loading

@mikesposito @legobeat This seems complementary to #315. There would be conflicts whether we merge one first or the other, but I don't see a reason why we couldn't have both. Do you?

mcmire
mcmire approved these changes Jun 23, 2023
View reviewed changes
Copy link
Copy Markdown

@mcmire mcmire left a comment

Choose a reason for hiding this comment

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

Makes sense.

@mikesposito
Copy link
Copy Markdown
Member

mikesposito commented Jun 26, 2023 •
edited
Loading

@mcmire We can definitely have both, I was just pointing out that after #315 has been merged, the early function termination does not affect 0 as input anymore, so from this PR we could also just add the test

@mikesposito mikesposito mentioned this pull request Jun 26, 2023
Merged
@Gudahtt
Copy link
Copy Markdown
Member

Gudahtt commented Jun 29, 2023

I don't think the functional change made in this PR is relevant anymore. The change in #315 is functionally equivalent in the case where zero is passed in, though broader in that it also affects the case where an empty string is passed in.

Agreed that the test would be useful to include though!

@Gudahtt Gudahtt mentioned this pull request Jun 29, 2023
Merged
@Gudahtt
Copy link
Copy Markdown
Member

Gudahtt commented Jun 29, 2023

I have created a separate PR to add just the test: #317

@Gudahtt
Copy link
Copy Markdown
Member

Gudahtt commented Jun 29, 2023

Superseded by #315 and #317

@Gudahtt Gudahtt closed this Jun 29, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@mcmire mcmire mcmire approved these changes

@Gudahtt Gudahtt Gudahtt approved these changes

@adonesky1 adonesky1 Awaiting requested review from adonesky1

@Mrtenz Mrtenz Awaiting requested review from Mrtenz

@kumavis kumavis Awaiting requested review from kumavis

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@legobeat @mikesposito @mcmire @Gudahtt @kumavis