divergencetech / ethier

Golang and Solidity SDK to make Ethereum development ethier

MIT License

217 stars 23 forks source link

Eip-191 conform signed messages #3

Closed cxkoda closed 2 years ago

cxkoda commented 2 years ago

Rationale

Switch to signed messages that conform to the EIP-191 standard.

This PR builds on #1.

Implementation

Solidity
- Use OpenZeppelin's ECDSA.toEthSignedMessageHash to convert hashes in MessageGenerator
Go
- Port OpenZeppelin's ECDSA.toEthSignedMessageHash
- Introduce a new function EthSign that uses conform messages
- Use EthSign instead of Sign for message signing
Should Sign be deprecated?

ARR4N commented 2 years ago

My reading of EIP-191 is that it only requires the 0x19 prefix, and that personal_sign as implemented by ECDSA.toEthSignedMessageHash is consequently a subset of that. I don't think we should be limiting to the most specific functionality.

As for how we change eth.Signer(), we should definitely add PersonalSign() and probably even ERC191Sign(). Let's chat about whether we foresee raw signing being necessary in the future (for now we can always change Sign() to sign() to keep it around but not exposed).

cxkoda commented 2 years ago

Sorry I should have opened an issue for that, since the PR is somewhat outdated now. I agree - let's not touch the raw signs and keep them around for now.

As far as I have understood, it is not only prepending 0x19 but also some version + version specific data like "\x19Ethereum Signed Message:\n" + len(message) in the case of personal signatures. I'll submit a new PR, putting this into Signer.PersonalSign() for now.