Implement ethereumSignMessage

[LogvinovLeon]

#66

[prusnak]

Can you please also implement EthereumVerifyMessage? It would be nice to have implemented both.

[LogvinovLeon]

Sure

[prusnak]

Thank you very much Leonid! My colleagues @szymonlesisz and @Runn1ng will review the request.

[LogvinovLeon]

Just to give you some context - we need this change, cause after a lot of requests from Trezor users we decided to support your amazing device during the https://0xproject.com token launch, which is starting on the 9th of August and lasts 3 days.

We're happy to help with anything if needed.

[prusnak]

Err, I am afraid this will not work for such short notice. Not because of us, we can update connect even today, but because of wrong implementation of SignMessage in Ethereum. See here for more details: ethereum/go-ethereum#14794

I don't see into detail how your token launch will work, but I'd suggest you look deeper into that issue and how it interacts with how you use Sign/Verify message methods.

[LogvinovLeon]

I agree that the current standard could be better.
At this moment we have Metamask, Parity, Geth and Ledger Nano S implementing it and we support those.
Etherscan and MyEtherWallet do it differently.

[LogvinovLeon]

I think - for now we can implement this: https://github.com/ethereum/wiki/wiki/JSON-RPC#eth_sign to be compatible with all major ethereum clients/wallets.
And later - if EIP gets accepted, we can change it

[prusnak]

No, you don't understand. Signing code is in the TREZOR firmware and it is using the new method already, because the original method is vulnerable.

[LogvinovLeon]

Ok. We can special-case trezor, cause we check signatures server-side.

[prusnak]

Great!

[fabioberger]

Are we still waiting on @szymonlesisz and @Runn1ng to review the PR?

[prusnak]

Yes, @szymonlesisz will review it

[szymonlesisz]

sorry guys, i was doing some changes in connect and i didn't want to mess up branches.
i'm on in few minutes

[saleemrashid]

@szymonlesisz @Runn1ng Why is buttonCallback necessary here (and in the Bitcoin equivalents of these functions)? It should already be registered in initDevice.

[szymonlesisz]

@saleemrashid it looks like you're right.
But this is not the only problem why it doesn't work.
We need to make some changes in dependency library (trezor.js) also we need to release new config_signed.bin in order to communicate with browser extension.
Stay tuned, we are on it

[saleemrashid]

@szymonlesisz I haven't tested it (and I most likely won't because I don't use Ethereum), that was just a quick review of the code

[szymonlesisz]

@LogvinovLeon @fabioberger
We've managed to sign message but verification throw some errors.
We'll build it tomorrow first thing in the morning, sorry for delay

[fabioberger]

Ok @szymonlesisz, the sooner the better. Thanks.

[fabioberger]

@szymonlesisz good morning! Any progress on this?

[fabioberger]

@szymonlesisz can you please post the raw signature that you were able to generate but not verify? Perhaps we can help you debug the verification issue.

[szymonlesisz]

@fabioberger sure, this is signature i get:

{
"address": "d01e3b00f98f8e2c535a638fb580f84f654198d0",
"message": "an ascii message",
"signature": "7d16a0c9c733394f9b37e56d2781e078d4ee0dc987609f309aed6d269ba854e41668d9add66fc9bf92170822cbac11fb6acda67abc490eb27d694005840c8d951b"
}

[szymonlesisz]

@LogvinovLeon @fabioberger
Quick update: It's working. Now we need to do few cosmetic things like documentation, update dependency lib (trezor.js) and we are ready to release

[fabioberger]

Thats fantastic @szymonlesisz! Thanks for all the hard work! 💃 Please let me know when I can successfully call these methods from Trezor connect.

[szymonlesisz]

@fabioberger yes you can test it locally with #69 before we put it in production, it's builded with new version of trezor.js (6.1.0)

[karelbilek]

Closing, implemented in #69