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 checksum validation for Ethereum address #630

Merged
merged 5 commits into from Sep 27, 2020
Merged

Add checksum validation for Ethereum address #630

merged 5 commits into from Sep 27, 2020

Conversation

elias19r
Copy link
Contributor

@elias19r elias19r commented Jul 3, 2020

Fixes Or Enhances #631

Make sure that you've checked the boxes below before you submit PR:

  • Tests exist or have been written that cover this particular change.

Change Details:

  • Add checksum validation for Ethereum address

@go-playground/admins

baked_in.go Outdated Show resolved Hide resolved
h.Write([]byte(strings.ToLower(address)))
hash := hex.EncodeToString(h.Sum(nil))

for i := 0; i < len(address); i++ {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Here len(address) definitely has value 40, but I preferred to write len(address) instead of 40. Thus we keep the information that "Ethereum addresses have 40 hex digits" declared only in one place, in the regexes.go (ethAddress{Upper,Lower,}RegexString).

Let me know if otherwise

@deankarn
Copy link
Contributor

deankarn commented Jul 7, 2020

@elias19r made one suggested change, other than that looks great and can merge next time I merge a group of PR's

elias19r and others added 2 commits July 7, 2020 10:45
Explicitly indicate we are ignoring errors from hash.Hash's io.Writer implementation

Co-authored-by: Dean Karn <Dean.Karn@gmail.com>
@elias19r
Copy link
Contributor Author

elias19r commented Jul 7, 2020

@deankarn Thank you, let me know if you would like me to squash commits or you'll do it upon merging

@deankarn
Copy link
Contributor

deankarn commented Jul 7, 2020

@elias19r I'll do it during the merge no worries

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.

None yet

3 participants