Skip to content

Conversation

@legobeat
Copy link
Contributor

Targeting #112

  • break out encoding to separate function
  • implementation closer to ERC spec code
  • functional approach

IMO this improves readability.

@legobeat legobeat requested a review from a team as a code owner July 11, 2023 03:48
@legobeat legobeat requested a review from mikesposito July 11, 2023 03:49
@legobeat legobeat requested a review from Gudahtt July 11, 2023 03:50
@legobeat legobeat requested a review from Mrtenz July 11, 2023 04:13
Copy link
Member

@Mrtenz Mrtenz left a comment

Choose a reason for hiding this comment

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

Can you add tests for this function? EIP-55 has some test cases which we can use.

@mikesposito
Copy link
Member

The only significant difference I can note is that we now calculate the entire hash and then we compare it, instead of early terminating the function on the first wrong char. But that isn't necessarily a bad thing

@Mrtenz
Copy link
Member

Mrtenz commented Jul 11, 2023

The only significant difference I can note is that we now calculate the entire hash and then we compare it, instead of early terminating the function on the first wrong char. But that isn't necessarily a bad thing

The difference in performance is probably negligible?

@mikesposito mikesposito merged commit 58ae2d2 into MetaMask:feat/is-valid-hex-address Jul 11, 2023
mikesposito added a commit that referenced this pull request Jul 11, 2023
* feat: add isValidHexAddress function

* fix: accept only prefixed addresses

* fix: remove case insensitive flag

* test: add test case for 0X

* feat: validate checksum addresses

* feat: break out address checksum encoding to erc55EncodeAddress function (#113)

* feat: break out address checksum encoding to erc55EncodeAddress function

* deps: ethereum-cryptography@2.0.0->2.1.0; dedupe @noble/hashes

* Update jsdoc

Co-authored-by: Maarten Zuidhoorn <maarten@zuidhoorn.com>

* refactor: rename erc55EncodeAddress function

* test: add test cases for getChecksumAddress

---------

Co-authored-by: Maarten Zuidhoorn <maarten@zuidhoorn.com>
Co-authored-by: Michele Esposito <michele@esposito.codes>

* docs: edit isValidHexAddress jsdoc description

* Update src/hex.ts

Co-authored-by: Mark Stacey <markjstacey@gmail.com>

* fix: add validation assertions

---------

Co-authored-by: legobeat <109787230+legobeat@users.noreply.github.com>
Co-authored-by: Maarten Zuidhoorn <maarten@zuidhoorn.com>
Co-authored-by: Mark Stacey <markjstacey@gmail.com>
@legobeat legobeat deleted the feat/is-valid-hex-address-encode branch July 11, 2023 23:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants