Fix `isHex` and `isHexStrict` for some edge cases and enrich their tests #5655

Muhammad-Altabba · 2022-11-25T11:36:17Z

Description

Fixes #5616 (in addition to the added comment #5616 (comment))

Type of change

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to not work as expected)

Checklist for 1.x:

Checklist for 4.x:

render · 2022-11-25T11:36:23Z

Your Render PR Server URL is https://atweb3js-docs-pr-5655.onrender.com.

Follow its progress at https://dashboard.render.com/static/srv-ce0ahd1a6gdpfmh1ol20.

…-isHexStrict-and-modify-test-cases

spacesailor24

In 1.x, 0x and -0x return true for both isHex and isHexStrict

'' also returns true for isHex in 1.x, I would consider making this breaking change (to return false instead), however I'm not sure how many of our users are relying on this behaviour

spacesailor24 · 2022-11-29T03:05:43Z

1.x also returns false for -123 as well...

but true for 123

luu-alex · 2022-11-29T08:33:38Z

I think it'll be fine to stray away from these 1.x changes, as these results are a bit misleading and include these changes in the breaking changes

packages/web3-validator/src/validation/string.ts

spacesailor24 · 2022-11-29T17:45:02Z

@Muhammad-Altabba @luu-alex

These are the regex tests from 1.x:

isHexStrict: /^(-)?0x[0-9a-f]*$/i
isHex: ^(-0x|0x)?[0-9a-f]*$/i

Because of (-)?0x in isHexStrict check, I would assume that -123 being false and 123 being true for isHex is erroneous, and a breaking change would make sense (both should return true).

I'm not so sure we should make the breaking change for 0x and -0x, I think these should return true as this has the highest probability of breaking user code out of all the mentioned edge cases. As an example, even in our own code do we allow certain properties such as value, to, and input/data to be 0x:

Not that it matters too much, but this is what Ethers is returning

luu-alex · 2022-11-29T20:57:06Z

@spacesailor24 agreed, I think 0x should be okay to include but -0x probably not. If we include these breaking changes apart of the 4.x utils migration doc so that theres awareness around this for the users.

spacesailor24 · 2022-11-29T23:29:33Z

@luu-alex Yea -0x doesn't make sense. Even if we include changes in the migration docs, it would still serve our community the best to make as little changes as possible, especially changes that might require users to make many changes across their projects (and imo, any breaking changes that involve transactions shouldn't be taken lightly)

…-test-cases

Muhammad-Altabba · 2022-12-02T10:59:58Z

@spacesailor24
Actually the returned value when passing 0x to isHex and isHexStrict was never mentioned to be changed in this MR. As there is no behavior change for this case. Additionally, there is a test case added to ensure this: https://github.com/web3/web3.js/pull/5655/files#diff-d8d5a378a9499635d2f6cca11a00d76024d6a3e6afde5a6db676f824563ed75dR135 And so there is no code that would be broken for this case 😄

The only additional changes in this MR (in addition to allow accepting -123) are to fix the issues mentioned in #5616 (comment)

both isHex and isHexStrict returns true for -0x, which does not make sense.
isHexreturns true for empty strings ''.

However, it seems that changing the scope of this issue created some confusion, sorry for that. And also seems to better be discussed separately. So, I will revert back to the original scope (allowing -123) and we could open a different issue to discuss the other points.

I would like also to comment on the outputs of ether.js.
It possibly returns false for -123 by mistake because it copied that behavior from this repository (v0.x and v1.x) as it is far older 😄 . And possibly it was kept like that as this function is mostly an internal function. And for that, there were no complains also on their or this repo from any user.

And for returning false for both 0x and 0x123. From some point of view, the hex number is an abstract form of number representation that could represent any data. And so even negative numbers could be represented by something like 0xf123 for example. But also there could be someone who could write -0x456 and expect it to be a valid negative hex.
And if out try to convert -123 to Hex at: https://www.rapidtables.com/convert/number/decimal-to-hex.html it would give different values for what they called Hex number and Hex signed 2'd complement`:

For this, I did not change the behavior of returning true for -0x123. But I probably understand why ether.js returns false for that.
However, I found returning true for -0x as non-sense. But I could be wrong. And I will leave this to a possible future discussion 😄

spacesailor24 · 2022-12-02T19:13:22Z

Okay, so I think the consensus is:

-0x returning false
'' returning false
123 and -123 returning true

luu-alex · 2022-12-02T19:34:10Z

im good with that,

Muhammad-Altabba · 2022-12-02T20:20:08Z

Okay, so I think the consensus is:

-0x returning false

'' returning false

123 and -123 returning true

Thanks @spacesailor24 . This is great if we all agree on that. As this is what is included in this MR 👍 . And so, could you, please, approve this MR, if you do not have other concerns?
If you still have some douts. It would be OK and we can discuss them here or in another issue 😄

…s' of https://github.com/ChainSafe/web3.js into fix/5616/fix-isHex-and-isHexStrict-and-modify-test-cases

Muhammad-Altabba · 2022-12-05T11:00:29Z

I think it'll be fine to stray away from these 1.x changes, as these results are a bit misleading and include these changes in the breaking changes

Thanks for the important suggestion.
Done.

* Update CHANGELOG version headers * Contract call with tuple is missing param names (#5613) * call special output type * fix * fix: enable outputs to have param names (#5624) * hot fix * add type if only one param * overloaded inputs types * fix resolver tests * add type tests * simplify types * revert registry unit test * test firefox * revert firefox test * update changelogs Co-authored-by: Marin Petrunić <[email protected]> * Fix `isHex` and `isHexStrict` for some edge cases and enrich their tests (#5655) * Change `isHex` to return true for negative numbers (for example `-123`) * Change both `isHex` and `isHexStrict` to return `false` for `-0x` * Change `isHex` to return `false` for empty strings ''. * Remove erroneous set provider code for Contract constructor (#5669) * Remove erroneous set provider code for Contract constructor. Add Contract constructor provider set test * Update CHANGELOGs * Debugging failing integration tests * Apply suggestions from code review * Use getSystemTestProvider instead of hardcoded string * Refactor ternaries in Contract constructor to if statements * Correct CHANGELOG entries * Remove unneeded checks in if statements * Test with injected external providers (#5652) * fix: sending tx with injected provider (#5651) Co-authored-by: Marin Petrunic <[email protected]> * adding a test for using `ganache` provider * enable the jsonrpc `id` to optionally be incremented starting from a number (Inspired by: #5373 (comment) and needed as a work around for blockchainsllc/in3#46) * test with `in3` as a provider & skip `in3` test if it takes too long * increase integration test timeout at web3 package * add a test for using `hardhat` provider * improve how legacy providers, such as hardhat, is used * implement `isPromise` that works in the browsers * refactor external providers tests * update CHANGELOG.md * Use Error ABI to parse errors when sending a transaction (#5662) * use Error ABI when sending a transaction * update CHANGELOG.md for #5587 * Remove merge conflict marker Co-authored-by: Oleksii Kosynskyi <[email protected]> Co-authored-by: Marin Petrunić <[email protected]> Co-authored-by: Muhammad Altabba <[email protected]> Co-authored-by: jdevcs <[email protected]>

* Version bump and build for v4.0.1-alpha.2 * Release/4.0.1 alpha.2 changelog update 3 (#5689) * Update CHANGELOG version headers * Update CHANGELOGs * Revert package version bump for web3-packagetemplate * Release/4.0.1 alpha.2 merge conflicts (#5692) * Update CHANGELOG version headers * Contract call with tuple is missing param names (#5613) * call special output type * fix * fix: enable outputs to have param names (#5624) * hot fix * add type if only one param * overloaded inputs types * fix resolver tests * add type tests * simplify types * revert registry unit test * test firefox * revert firefox test * update changelogs Co-authored-by: Marin Petrunić <[email protected]> * Fix `isHex` and `isHexStrict` for some edge cases and enrich their tests (#5655) * Change `isHex` to return true for negative numbers (for example `-123`) * Change both `isHex` and `isHexStrict` to return `false` for `-0x` * Change `isHex` to return `false` for empty strings ''. * Remove erroneous set provider code for Contract constructor (#5669) * Remove erroneous set provider code for Contract constructor. Add Contract constructor provider set test * Update CHANGELOGs * Debugging failing integration tests * Apply suggestions from code review * Use getSystemTestProvider instead of hardcoded string * Refactor ternaries in Contract constructor to if statements * Correct CHANGELOG entries * Remove unneeded checks in if statements * Test with injected external providers (#5652) * fix: sending tx with injected provider (#5651) Co-authored-by: Marin Petrunic <[email protected]> * adding a test for using `ganache` provider * enable the jsonrpc `id` to optionally be incremented starting from a number (Inspired by: #5373 (comment) and needed as a work around for blockchainsllc/in3#46) * test with `in3` as a provider & skip `in3` test if it takes too long * increase integration test timeout at web3 package * add a test for using `hardhat` provider * improve how legacy providers, such as hardhat, is used * implement `isPromise` that works in the browsers * refactor external providers tests * update CHANGELOG.md * Use Error ABI to parse errors when sending a transaction (#5662) * use Error ABI when sending a transaction * update CHANGELOG.md for #5587 * Remove merge conflict marker Co-authored-by: Oleksii Kosynskyi <[email protected]> Co-authored-by: Marin Petrunić <[email protected]> Co-authored-by: Muhammad Altabba <[email protected]> Co-authored-by: jdevcs <[email protected]> * Release patch/4.0.1 alpha.2/solve merge conflicts (#5697) * Update CHANGELOG version headers * Remove merge conflict markers * Remove double CHANGELOG entry Co-authored-by: Oleksii Kosynskyi <[email protected]> Co-authored-by: Marin Petrunić <[email protected]> Co-authored-by: Muhammad Altabba <[email protected]> Co-authored-by: jdevcs <[email protected]>

fix- isHex and isHexStrict and modify test cases

9595ebb

Muhammad-Altabba added 4 commits November 25, 2022 15:28

accept 0x for bytes

e352058

modify isHexStrict and isHex to accept '' and '0x'

1d1a15d

allow '' to pass hex check

c0eff62

update CHANGELOG.md for #5373

0dfd5ab

Muhammad-Altabba changed the title ~~[DRAFT] fix isHex and isHexStrict and modify test cases~~ Fix isHex and isHexStrict and modify test cases Nov 28, 2022

Muhammad-Altabba changed the title ~~Fix isHex and isHexStrict and modify test cases~~ Fix isHex and isHexStrict for some edge cases and enrich their tests Nov 28, 2022

Muhammad-Altabba marked this pull request as ready for review November 28, 2022 18:09

Muhammad-Altabba requested review from avkos, jdevcs, luu-alex, nikoulai and spacesailor24 November 28, 2022 18:12

luu-alex approved these changes Nov 28, 2022

View reviewed changes

Merge remote-tracking branch 'origin/4.x' into fix/5616/fix-isHex-and…

6e5f1da

…-isHexStrict-and-modify-test-cases

avkos approved these changes Nov 28, 2022

View reviewed changes

spacesailor24 suggested changes Nov 29, 2022

View reviewed changes

luu-alex self-requested a review November 29, 2022 05:20

nikoulai approved these changes Nov 29, 2022

View reviewed changes

packages/web3-validator/src/validation/string.ts Show resolved Hide resolved

Merge branch '4.x' into fix/5616/fix-isHex-and-isHexStrict-and-modify…

980aed9

…-test-cases

Muhammad-Altabba marked this pull request as draft December 2, 2022 15:48

Muhammad-Altabba requested a review from spacesailor24 December 2, 2022 20:20

luu-alex approved these changes Dec 2, 2022

View reviewed changes

spacesailor24 approved these changes Dec 2, 2022

View reviewed changes

Muhammad-Altabba marked this pull request as ready for review December 3, 2022 00:24

Muhammad-Altabba added 3 commits December 5, 2022 11:53

add isHex and isHexStrict changes to the migration guide

19eb749

Merge branch 'fix/5616/fix-isHex-and-isHexStrict-and-modify-test-case…

9428210

…s' of https://github.com/ChainSafe/web3.js into fix/5616/fix-isHex-and-isHexStrict-and-modify-test-cases

add 0X to the valid hex cases at tests

b69122b

jdevcs approved these changes Dec 5, 2022

View reviewed changes

Muhammad-Altabba merged commit a37124b into 4.x Dec 5, 2022

Muhammad-Altabba deleted the fix/5616/fix-isHex-and-isHexStrict-and-modify-test-cases branch December 5, 2022 15:52

Fix isHex and isHexStrict for some edge cases and enrich their tests #5655

Fix isHex and isHexStrict for some edge cases and enrich their tests #5655

Uh oh!

Conversation

Muhammad-Altabba commented Nov 25, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Type of change

Checklist for 1.x:

Checklist for 4.x:

Uh oh!

render bot commented Nov 25, 2022

Uh oh!

spacesailor24 left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

spacesailor24 commented Nov 29, 2022

Uh oh!

luu-alex commented Nov 29, 2022

Uh oh!

Uh oh!

spacesailor24 commented Nov 29, 2022

Uh oh!

luu-alex commented Nov 29, 2022

Uh oh!

spacesailor24 commented Nov 29, 2022

Uh oh!

Muhammad-Altabba commented Dec 2, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

spacesailor24 commented Dec 2, 2022

Uh oh!

luu-alex commented Dec 2, 2022

Uh oh!

Muhammad-Altabba commented Dec 2, 2022

Uh oh!

Muhammad-Altabba commented Dec 5, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

Fix `isHex` and `isHexStrict` for some edge cases and enrich their tests #5655

Fix `isHex` and `isHexStrict` for some edge cases and enrich their tests #5655

Muhammad-Altabba commented Nov 25, 2022 •

edited

Loading

spacesailor24 left a comment •

edited

Loading

Muhammad-Altabba commented Dec 2, 2022 •

edited

Loading