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

The documentation of toEthSignedMessageHash makes no sense for me #2545

Open
vporton opened this issue Feb 28, 2021 · 5 comments
Open

The documentation of toEthSignedMessageHash makes no sense for me #2545

vporton opened this issue Feb 28, 2021 · 5 comments

Comments

@vporton
Copy link

vporton commented Feb 28, 2021

https://docs.openzeppelin.com/contracts/3.x/api/cryptography

The documentation of toEthSignedMessageHash makes no sense for me: It argument is bytes32 hash and "This replicates the behavior of the eth_sign JSON-RPC method."

But eth_sign JSON-RPC method accepts an arbitrary string and this string is recommended to be human-readable. So generally it cannot be like bytes32.

The documentation needs to be updated.

@Amxx
Copy link
Collaborator

Amxx commented Feb 28, 2021

In the master, and the latest beta, this was changed to
Returns an Ethereum Signed Message, created from a `hash`. This produces hash corresponding to the one signed with the https://eth.wiki/json-rpc/API#eth_sign[`eth_sign`] JSON-RPC method as part of EIP-191.

see: https://github.com/OpenZeppelin/openzeppelin-contracts/blob/master/contracts/utils/cryptography/ECDSA.sol#L74-L77

@vporton
Copy link
Author

vporton commented Mar 2, 2021

In the master, and the latest beta, this was changed to
Returns an Ethereum Signed Message, created from a `hash`. This produces hash corresponding to the one signed with the https://eth.wiki/json-rpc/API#eth_sign[`eth_sign`] JSON-RPC method as part of EIP-191.

see: https://github.com/OpenZeppelin/openzeppelin-contracts/blob/master/contracts/utils/cryptography/ECDSA.sol#L74-L77

@Amxx I think your change is useful but it does not answer the issue I raised!

@frangio
Copy link
Contributor

frangio commented Mar 10, 2021

I believe the reason we implemented this using bytes32 is that at some point Geth always produced a signature for a message of 32 bytes, which was actually the hash of the data argument to eth_sign. You can see this in this Ethereum Stack Exchange question, and its accepted answer. There is also ethereum/go-ethereum#2397 which indicates that the behavior was the one I just described, but from this issue I can't tell if it has been fixed, or if personal_sign doesn't have the same issue.

This was a long time ago, things may have changed. @vporton Can you confirm that it is now possible to produce signatures with a different length? If so, the issue for that is #890.

@hack3r-0m
Copy link

hack3r-0m commented Aug 19, 2021

ethereum/go-ethereum#2940

the behavior of eth_sign is changed. It now accepts an arbitrary message, prepends a known message, hashes the result using keccak256 it calculates the signature of the hash (breaks backward compatibility!).

@frangio I would like to work on adding support for it in #890

@gdnathan
Copy link

up, as the "eth_sign" hyperlink is now dead, making the documentation even less clear

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

No branches or pull requests

5 participants