Skip to content

Add address related utils - #112

Merged
mikesposito merged 9 commits into
mainfrom
feat/is-valid-hex-address
Jul 11, 2023
Merged

Add address related utils#112
mikesposito merged 9 commits into
mainfrom
feat/is-valid-hex-address

Conversation

@mikesposito

@mikespositomikesposito commented Jul 6, 2023•
edited
Loading

Copy link
Copy Markdown
Member

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 ownerJuly 6, 2023 12:39
@mikesposito
mikespositoforce-pushed the feat/is-valid-hex-address branch from 432aad3 to 4d8a802CompareJuly 6, 2023 12:43
Gudahtt
Gudahtt reviewed Jul 6, 2023
View reviewed changes
Comment threadsrc/hex.ts Outdated
mikesposito
mikesposito commented Jul 6, 2023
View reviewed changes
Comment threadsrc/hex.ts
Gudahtt
Gudahtt reviewed Jul 6, 2023
View reviewed changes
Comment threadsrc/hex.ts Outdated
Gudahtt
Gudahtt previously approved these changes Jul 6, 2023
View reviewed changes

@GudahttGudahtt left a comment

Copy link
Copy Markdown
Member

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
View reviewed changes
Mrtenz
Mrtenz reviewed Jul 6, 2023
View reviewed changes

@MrtenzMrtenz left a comment •
edited
Loading

Copy link
Copy Markdown
Member

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

Gudahtt commented Jul 6, 2023

Copy link
Copy Markdown
Member

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

Mrtenz commented Jul 6, 2023

Copy link
Copy Markdown
Member

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

@mcmire

mcmire commented Jul 6, 2023

Copy link
Copy Markdown
Contributor

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

@Gudahtt

Gudahtt commented Jul 6, 2023

Copy link
Copy Markdown
Member

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

@mikespositomikesposito mentioned this pull request Jul 10, 2023
Merged
3 tasks
@mikesposito
mikespositoforce-pushed the feat/is-valid-hex-address branch from f2f5c16 to 3245089CompareJuly 10, 2023 16:30
@socket-security

socket-securityBot commented Jul 10, 2023•
edited
Loading

Copy link
Copy Markdown

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

Gudahtt
Gudahtt reviewed Jul 10, 2023
View reviewed changes
Comment threadsrc/hex.ts Outdated
Gudahtt
Gudahtt reviewed Jul 10, 2023
View reviewed changes
Comment threadsrc/hex.ts Outdated
Gudahtt
Gudahtt reviewed Jul 10, 2023
View reviewed changes
Comment threadsrc/hex.ts Outdated
legobeat
legobeat reviewed Jul 11, 2023
View reviewed changes
Comment threadsrc/hex.ts Outdated
@legobeatlegobeat mentioned this pull request Jul 11, 2023
Merged
legobeatand others added 2 commits July 11, 2023 17:43
@legobeat@Mrtenz@mikesposito
feat: break out address checksum encoding to erc55EncodeAddress funct…
58ae2d2
…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>
@mikesposito
docs: edit isValidHexAddress jsdoc description
38444a9
Gudahtt
Gudahtt reviewed Jul 11, 2023
View reviewed changes
Comment threadsrc/hex.ts Outdated
Gudahtt
Gudahtt previously approved these changes Jul 11, 2023
View reviewed changes

@GudahttGudahtt left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM!

@mikesposito@Gudahtt
Update src/hex.ts
4a49c16
Co-authored-by: Mark Stacey <markjstacey@gmail.com>
Mrtenz
Mrtenz reviewed Jul 11, 2023
View reviewed changes
Comment threadsrc/hex.ts Outdated
Comment threadsrc/hex.ts
* @returns The address encoded according to ERC-55.
* @see https://eips.ethereum.org/EIPS/eip-55
*/
export function getChecksumAddress(address: Hex) {

@MrtenzMrtenzJul 11, 2023

Copy link
Copy Markdown
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.

@mikespositomikespositoJul 11, 2023

Copy link
Copy Markdown
MemberAuthor

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

@mikespositomikesposito changed the title Add isValidHexAddress functionAdd address related utilsJul 11, 2023
@mikesposito
mikesposito merged commit f1024ca into mainJul 11, 2023
@mikesposito
mikesposito deleted the feat/is-valid-hex-address branch July 11, 2023 17:39
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@MrtenzMrtenzMrtenz left review comments
@GudahttGudahttGudahtt approved these changes
+1 more reviewer
@legobeatlegobeatlegobeat left review comments
Reviewers whose approvals may not affect merge requirements

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

@mikesposito@Gudahtt@Mrtenz@mcmire@legobeat