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.

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

Add address related utils #112

Merged
merged 9 commits into from
Jul 11, 2023
Merged

Add address related utils #112

merged 9 commits into from
Jul 11, 2023

Conversation

mikesposito
Copy link
Member

@mikesposito mikesposito commented Jul 6, 2023

This PR ports the isValidHexAddress function from @metamask/controller-utils, without using the ethereumjs-utils package to check the string.

The function can be used to check if a string is a valid prefixed hex address, all lower-cased or a valid checksum

Changes

  • ADDED: isValidHexAddress has been added to check the validity of an hex address

  • ADDED: getChecksumAddress has been added to calculate the ERC-55 mixed-case checksum of an hex address

  • ADDED: isValidChecksumAddress has been added to check the validity of an ERC-55 mixed-case checksum address

@mikesposito mikesposito requested a review from a team as a code owner July 6, 2023 12:39
src/hex.ts Outdated Show resolved Hide resolved
src/hex.ts Show resolved Hide resolved
src/hex.ts Outdated Show resolved Hide resolved
Gudahtt
Gudahtt previously approved these changes Jul 6, 2023
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!

Gudahtt
Gudahtt previously approved these changes Jul 6, 2023
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.

This does not validate the checksum validity of the address. Ideally that would be part of this function as well.

I wrote a function for this some time ago, that doesn't use ethereumjs or other libraries (except for a Keccak-256 library, of course), if that helps?

@Gudahtt
Copy link
Member

Gudahtt commented Jul 6, 2023

Hmm. Validating the checksum does seem valuable, though less so for lower-cased addresses. We still use lower-cased addresses in most places (though not all).

Perhaps we could add a variant of this that includes checksum validation? Or we could update this one to only validate the checksum if upper-case characters are detected.

@Mrtenz
Copy link
Member

Mrtenz commented Jul 6, 2023

Validating the checksum if the address has uppercase characters makes sense to me.

@mcmire
Copy link
Contributor

mcmire commented Jul 6, 2023

Sorry if I'm missing something, but why can't we validate the checksum if the address has lowercase characters?

@Gudahtt
Copy link
Member

Gudahtt commented Jul 6, 2023

The case of each character is the checksum; if the address is lower-cased, there is no checksum to validate.

See here for further details: https://eips.ethereum.org/EIPS/eip-55

@socket-security
Copy link

socket-security bot commented Jul 10, 2023

No top level dependency changes detected. Learn more about Socket for GitHub ↗︎

src/hex.ts Outdated Show resolved Hide resolved
src/hex.ts Outdated Show resolved Hide resolved
src/hex.ts Outdated Show resolved Hide resolved
src/hex.ts Outdated Show resolved Hide resolved
legobeat and others added 2 commits July 11, 2023 17:43
…ion (#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>
src/hex.ts Outdated Show resolved Hide resolved
Gudahtt
Gudahtt previously approved these changes Jul 11, 2023
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!

Co-authored-by: Mark Stacey <markjstacey@gmail.com>
src/hex.ts Outdated Show resolved Hide resolved
* @returns The address encoded according to ERC-55.
* @see https://eips.ethereum.org/EIPS/eip-55
*/
export function getChecksumAddress(address: Hex) {
Copy link
Member

Choose a reason for hiding this comment

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

I wonder if we should do more validation here, such as checking if the provided address is 20 bytes long.

Copy link
Member Author

Choose a reason for hiding this comment

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

I just added an assertion to check the string we receive here

@mikesposito mikesposito changed the title Add isValidHexAddress function Add address related utils Jul 11, 2023
@mikesposito mikesposito merged commit f1024ca into main Jul 11, 2023
16 checks passed
@mikesposito mikesposito deleted the feat/is-valid-hex-address branch July 11, 2023 17:39
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.

5 participants