feat: Add EIP-712 support for T1

[alisinabh]

Based on #983

This PR adds support for T1 blind signing to Trezor-Connect. It uses @metamask/eth-sig-util package to calculate the hashes for domain and message then sends them to T1 using EthereumSignTypedHash message. Since T1 will return EthereumTypedDataSignature no other change is needed.

This way we handle T1 and TT differences as an internal logic of connect and will be invisible to the user/developers.

One of the tests is failing. It is titled as struct_list_non_v4. I think we should somehow disable this for T1 (or completely remove it) as in a non V4 this is not supported. The eth-sig-util will throw the error

Arrays are unimplemented in encodeData; use V4 extension

It might be an invalid example IMO.

Also should I move the new dependency @metamask/eth-sig-util to extendedDependencies @matejcik?

Tasks

• Add domain_hash and message_hash to the test fixtures.
• Update the trezor-common repo and sync the submodule.
• Decide about struct_list_non_v4 failing test.
• Rebase on develop after feat: Support Ethereum EIP-712 Sign Typed Data for Trezor T #983 is merged.
• Re-run scripts/protobuf-build.sh.
• Add ethereumSignTypedData to CI tests.
• Edit TrezorConnect.ethereumSignTypedData instructions.
• Mark as Ready for review.

[TheArhaam]

Hey @alisinabh 👋
Great work across the board for getting EIP-712 support on Trezor One, I wasn't aware of the missing support and it was super disappointing but thank god for opensource :)
Just wondering if this would be the last piece of the puzzle or metamask integrations are also pending post this?

[mroz22]

Hey @alisinabh wave Great work across the board for getting EIP-712 support on Trezor One, I wasn't aware of the missing support and it was super disappointing but thank god for opensource :) Just wondering if this would be the last piece of the puzzle or metamask integrations are also pending post this?

We are proposing approach which would require some work to be done on metamask side after this is finished. See my comment #986 (comment)

[alisinabh]

Hi @TheArhaam

Just wondering if this would be the last piece of the puzzle or metamask integrations are also pending post this?

After the support in connect is added, Then it can be adapted in MetaMask/eth-trezor-keyring for the metamask support.

[mroz22]

Hello, thanks for contribution again. Could you please rebase on current develop and resolve conflicts? After that we would proceed with review.

[alisinabh]

@mroz22 Rebase is done. The workflow for both T1 and TT should be OK.

Tests however would need some interventions as we would need to add the extra data in the fixtures (or calculate them somehow in the test environment).

Also the common repo has not been updated.

[davidberiro]

Is there any chance this will be released in the next few days? Unable to perform some important signing with my T1 and don't have the ability (or desire) to migrate to another hardware wallet right now.

[matejcik]

this is wrong -- metamask_v4_compat=False is in fact metamask V5 (doesn't exist yet, i believe)

instead we should always use V4 and always set metamask_v4_compat = True if this is being used.

[alisinabh]

I believe metamask_v4_compat comes from the developers and they might send false here (I don't know why they would but they could).

Ledger already raises an exception if that is set to false. We can also do the same thing. Do you agree?

[matejcik]

Yes, that is a good idea.

[alisinabh]

Fixed in 33d8b90

[davidberiro]

I have a $1500 LOOKS airdrop expiring in 8 days depending on this, would a $200 bounty speed up the process (half joking lol)

[0xWildhare]

I have a $1500 LOOKS airdrop expiring in 8 days depending on this, would a $200 bounty speed up the process (half joking lol)

I will contribute to this bounty. not joking. I'll put 0.1 eth on it if it is complete in time that I can claim the airdrop.

[PabloCastellano]

@alisinabh Could we speed-up this task with a bounty?

I'll happily contribute to the apportionment too. Can we use some kind of platform for the task?

[alisinabh]

Thank you for your offers. I would also like this to be finished (regardless of any bounties).
The firmware for T1 is scheduled to be released in 19 of January and this PR is concerning only T1 users. This already has a task list and probably would some time to finish.

You would also need to wait for the next release of trezor connect and then metamask which would probably take more than 8 days.

BUT if you own a model T device you already have the EIP-712 support on your device and the PR regarding your device has already been merged in #983. What you can do is integrate that into metamask and build it yourself so you can claim your $LOOKS @davidberiro @0xWildhare @PabloCastellano .

However if you lack the technical knowledge or are too lazy to do that yourself 😅 I also can help you build it so you don't loose any of this precious airdrops.

[0xmichalis]

I would also love support for T1 to be in place already, thankfully I had a Ledger laying around that I was able to use to claim the airdrop. It's unfortunate that if you have a T1 today, it's almost unusable in Ethereum. 😅 All that being said, I really appreciate @alisinabh's work on this one, I have already donated 0.1 ETH for his work in the firmware repo and I would urge others to donate regardless of whether you will be able to claim your LOOKS airdrop!

[rsimonton]

For those concerned about the $LOOKS airdrop specifically, they have posted on Discord in their official announcement channel that they are aware of this issue with ERC-712 and that they will make sure that affected users are still able to claim their airdrop. See also: https://twitter.com/zoddlooksrare/status/1481262245456007170?s=21

[mroz22]

Guys, we are now actively working to merge this PR. I am just finishing a branch with updates needed to be able to run tests for model 1 - it should land develop tomorrow. In my local branch tests are already passing, so it shouldn't take much longer.

[Moebius1977]

@alisinabh, thank you for your great work!
Is integrating #983 into Metamask the only way to use EIP-712 with Trezor T in order to get LOOKS airdrop?
Could you please provide a high level instruction on how to do that?

Is the issue (with Trezor T) expected to be fixed soon with the new releases of Trezor Connect and Metamask?

[aloisklink]

@alisinabh, thank you for your great work! Is integrating #983 into Metamask the only way to use EIP-712 with Trezor T in order to get LOOKS airdrop? Could you please provide a high level instruction on how to do that?

Is the issue (with Trezor T) expected to be fixed soon with the new releases of Trezor Connect and Metamask?

Hi @Moebius1977, this is probably the wrong place to discuss this, since this PR is exclusively about Trezor One, not Trezor T.

I've got a draft PR for adding support for Trezor T to Metamask in MetaMask/eth-trezor-keyring#117. I should be able to finalize it once the next version of trezor-connect is released and https://connect.trezor.io/8/ is updated.

It's pretty unlikely for support to officially land in Metamask before 8 days is up though, but if trezor-connect and https://connect.trezor.io/8/ are updated in time, you could manually build a developer version of Metamask and that should work for Trezor T.

Edit: feel free to build the developer version here for Trezor T support. I've confirmed it works (note it's not official, so your mileage may vary) https://github.com/aloisklink/metamask-extension/commits/trezor-model-t-eip-712

[mroz22]

hello, @alisinabh
if you could please rebase on the latest develop. it contains this commit cf4cd18 which allows you to run tests locally against model One. I was able to make your branch green locally (with some tweaking though).
To make tests work, just uncomment the commented code and add @metamask/eth-sig-util as a devDependecy. Then you can do ./tests/run.sh -i ethereumSignTypedData -f 1-master

I will also take care of adding this to CI. You could in the meantime try to resolve flow errors in your branch.

[mroz22]

I played with it a litttle and opened #1015. It should be passing CI now but there is still some mess that needs proper cleanup.

[mroz22]

closing in favor of #1015 which is already merged. thanks for contribution!

[0xWildhare]

I have a $1500 LOOKS airdrop expiring in 8 days depending on this, would a $200 bounty speed up the process (half joking lol)

I will contribute to this bounty. not joking. I'll put 0.1 eth on it if it is complete in time that I can claim the airdrop.

@alisinabh I owe you a bounty of .1 ETH. Please drop an address to claim! (or if you believe this should go to someone else, please tag them so that they can drop the address.

[alisinabh]

Hi @0xWildhare

First of all you are not required to do that. Of course I'm not going to stop you if you like to do that. Also @aloisklink did help in this so if you want to send anything please split it. 🙂

• 50% Me: https://gitcoin.co/tip?username=alisinabh OR 0xb412E9D7c64638bDD554EcaDf2520633147Eaa44
• 50% @aloisklink: https://gitcoin.co/tip?username=aloisklink

Thank you

[0xWildhare]

I said I would do it, so I did it! Thank you for your efforts!