search

OpenZeppelin / openzeppelin-contracts

OpenZeppelin Contracts is a library for secure smart contract development.
https://openzeppelin.com/contracts
MIT License
24.86k stars 11.77k forks source link

The documentation of toEthSignedMessageHash makes no sense for me #2545

Open vporton opened 3 years ago

vporton commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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 https://github.com/ethereum/go-ethereum/issues/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 commented 3 years ago

https://github.com/ethereum/go-ethereum/pull/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 commented 10 months ago

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