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

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

Closed
wants to merge 2 commits into from
Closed

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

wants to merge 2 commits into from

Conversation

legobeat
Copy link
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 (!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.

@legobeat legobeat requested review from kumavis and adonesky1 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
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 Mrtenz and Gudahtt April 27, 2023 11:11
Gudahtt
Gudahtt previously approved these changes Apr 27, 2023
View reviewed changes
Copy link
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
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
Member

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

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

@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
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
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
Member

Gudahtt commented Jun 29, 2023

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

@Gudahtt
Copy link
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

@Gudahtt Gudahtt Gudahtt approved these changes

@mcmire mcmire mcmire 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