Changes to web3.eth.sign (and web3.personal.sign)

[pipermerriam]

What was wrong?

The current implementation of web3.eth.sign is confusing and very difficult to use, as well as being very easy to use wrongly

1. It confusingly expect to receive the hexidecimal encoded pre-computed hash of the message to be signed.
2. It doesn't conform to the current web3js implementation.
3. It is fundamentally broken against the modern goethereum backend implementation.

How can it be fixed?

I propose the following options for fixing this. Given the broken-ness of this API, a breaking change should be fine but we should strive to provide some level of deprecation warnings if possible.

1. Change API to accept the full message.

The web3.*.sign methods should be changed to expect the full unhashed message.

2. Change to use bytes as the default encoding.

The web3.*.sign methods should expect the message to be encoded as bytes by default. This should be validated.

The method should take an optional second parameter which allows for alternately encoded messages. We should support the bytes, utf8 and hex encodings.

web3.eth.sign(account_to_sign_with, b"the raw bytes to be signed")
web3.eth.sign(account_to_sign_with, "0x123456abcd", encoding="hex")
web3.eth.sign(account_to_sign_with, "💩💩💩💩💩", encoding="utf8")

[carver]

Another option to consider is following the web3.js v1 beta model with several signature options:

Scenarios:

1. No client? eth.accounts.sign
2. Client with unlocked account? eth.sign
3. Client with locked account? eth.personal.sign

[carver]

Since eth_sign is the only standardized RPC call, I'll focus on that first.

The docs seem to disagree that it expects the hash:

The sign method calculates an Ethereum specific signature with: sign(keccak256("\x19Ethereum Signed Message:\n" + len(message) + message))).

Parameters

DATA, 20 Bytes - address
DATA, N Bytes - message to sign

Some people have taken to hashing their message before signing it (resulting in being hashed twice, with the prefix the second time).

---

It's pretty surprising to me that there's no ecrecover in web3.js. I'll add one, at least so I can run a test.

[carver]

Hah, yeah, ecrecover wasn't happening today. We'll get it for free when we upgrade pyethereum.

Opened #301 for option # 1 above.

I'm still on the fence about the other super-non-standard methods.

[carver]

IMO, there's too much disagreement about the personal_* API to put much effort into it right now. Note that parity doesn't implement any of it.

web3.eth.sign() is in an okay place after #301, and web3.eth.accounts deserves its own issue, which I just created here: #323

If I'm missing something, please reopen.

[pipermerriam]

👍