trezor / trezor-firmware

:lock: Trezor Firmware Monorepo
https://trezor.io
Other
1.34k stars 653 forks source link

Add support for EIP-712 #1272

Closed danfinlay closed 2 years ago

danfinlay commented 4 years ago

Re-opening from old repo here.

EIP 712 is an Ethereum signature format that has two goals:

The hope is that this signature format can allow more human-consensual layer-2 interactions on the Ethereum blockchain.

MetaMask has supported this signature format since it was first defined three years ago. There are a few different variations of it (each with a different RPC suffix: _v1, _v3, _v4, I would recommend focusing on _v4 if only implementing one).

You can read a bit more about it on the MetaMask docs, which do need to be expanded on this topic. We also have some working examples on this demo repo.

I'm happy to answer any questions I can to aid in this development. Many applications would like to adopt this signature scheme, but then users of those applications are currently forced to not use hardware wallets, which is a real shame.

prusnak commented 4 years ago

Reposting from the old issue:

I suggest you start with Model T first: https://github.com/trezor/trezor-firmware/blob/master/core/src/apps/ethereum/sign_message.py

Then proceed by creating an unit test similar to https://github.com/trezor/trezor-firmware/blob/master/tests/device_tests/test_msg_ethereum_signmessage.py

matejcik commented 4 years ago

there have been issues about this already: https://github.com/trezor/trezor-firmware/issues/131 even a WIP PR: https://github.com/trezor/trezor-firmware/pull/148

I'll note that a big problem with EIP712 is that it is very generic and thus difficult to translate to protobuf (and then properly display). This was also the problem with the previous attempt.

I would suggest not implementing the full format, instead supporting some reasonable subset.

mikec commented 3 years ago

I'm working on an app that's currently incompatible with Trezor because of this issue. The DAI contract uses EIP-712 for permit https://github.com/makerdao/dss/blob/master/src/dai.sol#L120, and many other smart contracts deployed on Ethereum rely on EIP-712.

dmihal commented 3 years ago

@matejcik I wonder if a "reasonable subset" would be just supporting permit() signatures, since that's probably the most common use of EIP-712.

It should be noted that Dai uses a different signature format than most other apps, which follow the ERC-2612 standard.

matejcik commented 3 years ago

I wonder if a "reasonable subset" would be just supporting permit() signatures, since that's probably the most common use of EIP-712.

I'm not watching the space so i'd have to take your word for it. But if you feel that useful, please open a separate issue first (and note wheter you're willing to contribute a PR) so that we can take it into consideration.

mikec commented 3 years ago

@dmihal @matejcik we're using EIP-712 for a use case that is not permit() / ERC-2612. I would think that adding permit() as a subset would actually be more difficult than just adding EIP-712 signing support more generically. I'm willing to contribute a PR, just not sure when I'll be able to get to it.

st-lemniscap commented 3 years ago

Would be good to get a status update from Trezor on support for signTypedData. Is the team even considering this?

0xklapouchy commented 3 years ago

What is the status of this issue? More and more users are complaining that they can't use Trezor with our Dapp that uses EIP-712

prusnak commented 3 years ago

What is the status of this issue?

There is a pull request for the issue here (https://github.com/trezor/trezor-firmware/pull/1568) which is being reviewed.

jamesmorgan commented 2 years ago

It would be really good if this was added, 712 signatures are being used by many and this would enable wider spread use of trezor within the nft space especially

TheArhaam commented 2 years ago

So strangely I was able to unlock currency on OpenSea earlier using my Trezor One, I had to unlist my NFT and when I tried to put it up for sale again it wouldn't work.

Any idea why or how it worked the first time? 🤔

prusnak commented 2 years ago

Fixed via https://github.com/trezor/trezor-firmware/pull/1835

Will be released to Trezor Model T in December update

bonustrack commented 2 years ago

@prusnak Can you confirm if the model Trezor One will also support EIP-712?

matejcik commented 2 years ago

Trezor One support is currently not planned.

bonustrack commented 2 years ago

@matejcik Isn't there some plan to backport the T firmware to Trezor One?

matejcik commented 2 years ago

Even if this happens, it's unclear if EIP-712 support would be enabled on T1 due to the UI complexity.

prusnak commented 2 years ago

Even if this happens, it's unclear if EIP-712 support would be enabled on T1 due to the UI complexity.

RIght. The best way how to interact with EIP-712 contracts is to get Trezor Model T.

bonustrack commented 2 years ago

Isn't Trezor One the model that is the most used? Feel like EIP712 support will still be a big blocking point for apps if you dont tackle it for model One.

roshhhyy commented 2 years ago

Even if this happens, it's unclear if EIP-712 support would be enabled on T1 due to the UI complexity.

RIght. The best way how to interact with EIP-712 contracts is to get Trezor Model T.

this isn't a viable solution as many users have polygon nfts trapped on trezor ones and its costing us a lot of money. See sources here:

https://www.reddit.com/r/TREZOR/comments/pwwnb3/polygon_nft_trapped_on_trezor/

https://www.reddit.com/r/TREZOR/comments/o0vxbn/lost_suck_nft_on_trezor/

these are just two examples of the issue, restoring the seed on a different trezor or software wallet is also not viable as it eliminates the entire purpose of having a hardware wallet where the seed is never typed or entered into any computer ever

please support eip712 for the trezor one somehow, it will be a huge service to your most longtime loyal users as the trezor one was the wallet of choice for everyone getting in before 2017

prusnak commented 2 years ago

these are just two examples of the issue, restoring the seed on a different trezor or software wallet is also not viable as it eliminates the entire purpose of having a hardware wallet where the seed is never typed or entered into any computer ever

You can restore the seed on Trezor Model T by typing it directly on the device screen.

PabloCastellano commented 2 years ago

Even if this happens, it's unclear if EIP-712 support would be enabled on T1 due to the UI complexity.

@matejcik This answer is simply disappointing. You guys are just pushing us, owners of Trezor One, to buy the new model which costs almost 4x. In addition, the website doesn't warn about this lack of support for this model, which is totally the opposite that the buyer expects.

I have no idea about what is the UI complexity you are mentioning. Is it related to reviewing the data you are signing and having such a small screen? If that's the case, I'm sure we can find a balance between having this feature and security.

I'm sure that users still prefer having EIP-712 with a bad UI than otherwise.

My two cents

roshhhyy commented 2 years ago

Even if this happens, it's unclear if EIP-712 support would be enabled on T1 due to the UI complexity.

I think everyone here wouldn't mind having the UI not display the signature information and have to sign the txn relatively blindly in exchange for being able to use the Trezor One for NFTs. I've had a Trezor One since 2017 and a loyal supporter of the product, Ledger figured out EIP712 with the S and X at the same time despite the S having limited memory and UI constraints. The Trezor One should simply support EIP712, UI issues aside.

bonustrack commented 2 years ago

I think everyone here wouldn't mind having the UI not display the signature information and have to sign the txn relatively blindly in exchange for being able to use the Trezor One for NFTs.

+1

@prusnak Could you explain what it would take to get support on model One? Is the UI the main blocker?

prusnak commented 2 years ago

Is the UI the main blocker?

No. Trezor One runs completely different software stack than Trezor Model T.

bonustrack commented 2 years ago

@prusnak Is it technically possible to add support on that software? Anything we can do to help making this happen?

roshhhyy commented 2 years ago

these responses are so dismissive - okay yes sure, it runs a different software stack fine. Can said software stack support EIP-712? I feel like saying the software stack makes it IMPOSSIBLE to support EIP712 is a cop out, tons of ETH dapps use EIP712 and other EVM based chains will require signatures on apps to not have the One support EIP712 is basically not supporting ETH itself.

Trezor support likes to say "The Trezor One doesn't support NFTs" which isn't even true - by not supporting EIP712 the One is not supporting a massive sector of ETH dapps and might as well remove ETH from supported assets. Ledger Nano S figured this out with barely enough memory on the device to have 2-3 asset apps, Trezor One somehow 2 years late is still just dismissing the issue and abandoning ETH + all EVM chains. You're leaving your earliest supporters and users in the dust.

zaneclaes commented 2 years ago

@prusnak Can we get a beta copy of the Trezor Model T firmware update to our users? Our community is being forced to turn away from Trezor (practically everyone is switching to Ledger). I'd love to give them a better option.

prusnak commented 2 years ago

@zaneclaes Firmware for Trezor Model T with EIP-712 will be officially released in few days - 8th of December.

alisinabh commented 2 years ago

@prusnak To be clear and honest to us (T1 owners), Are you suggesting that we will not receive this update?

prusnak commented 2 years ago

Are you suggesting that we will not receive this update?

I am suggesting you get a Trezor Model T if you need to use EIP-712 now. Everything else is speculation.

bonustrack commented 2 years ago

I am suggesting you get a Trezor Model T if you need to use EIP-712 now. Everything else is speculation.

Can you mention in your web store that model One does not support EIP-712?

prusnak commented 2 years ago

Thank you for your valuable feedback. We'll discuss this internally. In the meantime I am suggesting everyone getting a Trezor Model T if you need to use EIP-712 now.

prusnak commented 2 years ago

EIP-712 for Trezor One is being developed here and will be hopefully released in January: https://github.com/trezor/trezor-firmware/pull/1970