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

#890: Add ECDSA.toEthSignedMessageHash(bytes) for abritrary length message hashing #2865

Merged
merged 8 commits into from Oct 11, 2021

Conversation

td-bn
Copy link
Contributor

@td-bn td-bn commented Sep 16, 2021

Fixes #890
Issue link

The PR adds a function toEthSignedMessageHash(bytes) to the ECDSA contract, that works for any length of a bytes array.

PR Checklist

  • Tests
  • Documentation
  • Changelog entry

@Amxx
Copy link
Collaborator

Amxx commented Oct 6, 2021

Hello @td-bn , and thank you for this PR!

Do you think it would be possible to add a test with a message that is not 32 bytes long?

@td-bn
Copy link
Contributor Author

td-bn commented Oct 6, 2021

@Amxx, I've added said test. Please take a look.

@frangio
Copy link
Contributor

frangio commented Oct 6, 2021

Please add a test in the section that says using web3.eth.sign, so we can confirm that this function is doing the same that eth_sign is doing under the hood.

Also, the function name of the new function should be toEthSignedMessageHash, not toEthSignedMessage. The "hash" part of the name refers to the output, not to the argument. We want to overload the current function so that it also supports bytes arguments.

@Amxx Amxx mentioned this pull request Oct 6, 2021
1 task
@frangio
Copy link
Contributor

frangio commented Oct 6, 2021

Thanks @td-bn! I made two small changes in the tests. Note that I replaced the longer string with a short one, because it's easier to tell that it doesn't happen to have length 32.

@Amxx Amxx changed the title #890: Add ECDSA#toEthSignedMessage for bytes type #890: Add ECDSA.toEthSignedMessageHash(bytes) Oct 7, 2021
@Amxx Amxx changed the title #890: Add ECDSA.toEthSignedMessageHash(bytes) #890: Add ECDSA.toEthSignedMessageHash(bytes) for abritrary length message hashing Oct 7, 2021
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.

Add ECDSA#toEthSignedMessageHash for bytes type
3 participants