search

MetaMask / metamask-extension

:globe_with_meridians: :electric_plug: The MetaMask browser extension enables browsing Ethereum blockchain enabled websites
https://metamask.io
Other
11.87k stars 4.85k forks source link

Support EIP-712 signing for TREZOR accounts #11498

Closed angyts closed 2 years ago

angyts commented 3 years ago

Dear Devs, thank you for reading this.

This is a long standing problem that breaks support of hardware wallets for many web 3 applications.

I know that ledger already currently supports with a not ideal solution but working nonetheless.

Is anyone looking into the trezor side of things?

The devs on the trezor side are currently looking into the following to fully support EIP-712. https://github.com/trezor/trezor-firmware/pull/1568

It would be lovely if Metamask could look into the connect part of the specifications. I'm sorry I cannot help as this is really not in my field of expertise.

brad-decker commented 3 years ago

Could you say more about why the Ledger solution is not ideal?

angyts commented 3 years ago

Please correct me if I'm wrong.

I think the ledger team gets the user sign the hash of the message, whereas the trezor team is trying to port the entire message over.

I personally would prefer to see the entire message i'm signing.

darkwing commented 3 years ago

There is presently a PR open for this (https://github.com/trezor/trezor-firmware/pull/1568) which Trezor must merge before we can complete this task.

angyts commented 3 years ago

You are absolutely right about that.

Thank you for looking into in. So that when the trezor side has merged, metamask will be able to support.

Gudahtt commented 2 years ago

There is presently a PR open for this (trezor/trezor-firmware#1568) which Trezor must merge before we can complete this task.

It looks like that PR was superseded by https://github.com/trezor/trezor-firmware/pull/1835, which has been merged! :tada:

I don't know whether it has been released yet or not. Once it has been released, this issue will no longer be blocked.

yura-pakhuchiy commented 2 years ago

Firmware 2.4.3 for Trezor T with EIP-712 is available now. https://github.com/trezor/trezor-suite/pull/4602/files @darkwing @danjm

dmihal commented 2 years ago

The new Trezor T firmware has been released, supporting EIP-712

Metamask is now un-blocked on this issue, hopefully the feature will be supported soon.

gewure commented 2 years ago

Any Update on this?

blueblack2 commented 2 years ago

The new Trezor T firmware has been released, supporting EIP-712

Metamask is now un-blocked on this issue, hopefully the feature will be supported soon.

Does this Firmware EIP 712 update for the Model T work with signing transactions with Metamask yet?

astanciu commented 2 years ago

Any updates here? As soon as you try to sign anything, the following error shows up in console: MetaMask Message Signature: Error: Not supported on this device

This is using Trezor T, 2.4.3, MM@10.8.0

aloisklink commented 2 years ago

~Just to give people an update, I've added support to trezor-connect (Trezor JavaScript library) for the Trezor Model T in https://github.com/trezor/connect/pull/983.~

~We're currently waiting for the next release of trezor-connect before I work on adding it to the MetaMask side. I'm not an employee of @MetaMask or @TREZOR and can't promise anything, but depending on how much free time I have, I should be able to add it pretty quickly once it releases, since I'm already familiar with the code.~

~trezor-connect@8.2.6 has been released, and it adds EIP-721 support for both Trezor T and T1! Next steps:~

Support has been added to the develop branch by https://github.com/MetaMask/metamask-extension/pull/13693!

It's in the v10.11.0 RC pull request, so it will hopefully be included in v10.11.0! https://github.com/MetaMask/metamask-extension/pull/13706

RudyB commented 2 years ago

Trezor added support for EIP-712 for the Model One as of January 19, 2022 however it remains unsupported in Metamask

astanciu commented 2 years ago

For anyone following, this is the piece we're waiting on. It's done and can be manually installed, just not shipped https://github.com/MetaMask/eth-trezor-keyring/pull/117#issuecomment-1040733651

msubash26 commented 2 years ago

Hi Alex, any timeline on by when this will be shipped? many users are stuck with Trezor model one after the recent Wyvern exchange contract upgrade by Opensea, Please address this as early as possible. Thanks for the support!

astanciu commented 2 years ago

Hi Alex, any timeline on by when this will be shipped? many users are stuck with Trezor model one after the recent Wyvern exchange contract upgrade by Opensea, Please address this as early as possible. Thanks for the support!

No clue, I'm not involved with the project, was just sharing the link ;)

metaver5o commented 2 years ago

still not working!

digitalgermination commented 2 years ago

I can attest that this does not appear to have resolved the issue. Still can't use Trezor on OpenSea or LooksRare

FrederikBolding commented 2 years ago

The feature was merged but has not been released yet. Will possibly be included in the next release!

digitalgermination commented 2 years ago

The feature was merged but has not been released yet. Will possibly be included in the next release!

Oh, thank you for the heads up!

KOOLMOLL commented 2 years ago

В правильности выбора вопрос?и без инструментов влияния алгоритм тот же.

blueblack2 commented 2 years ago

When will this be released?

msubash26 commented 2 years ago

Why the latest release ([Version 10.10.1] has the fix for Lattice hardware wallet support to sign EIP-712 and not for Trezor though its fixed as per the comments above?

astanciu commented 2 years ago

Yeah seriously, why wasn't this included?

spoonknife22 commented 2 years ago

Lattice but not Trezor? You guys are stupid.

FrederikBolding commented 2 years ago

@msubash26 @astanciu @spoonknife22 It has been included in the release candidate for: 10.11.0. Which should be the next release, but it needs to go through the QA process first!

For 10.10.1 we fixed an issue with the existing Lattice EIP.712 integration, which was already released and tested, thus it could go out faster.

msubash26 commented 2 years ago

@FrederikBolding , Thanks for the update, could you confirm the release date for 10.11.0?

ching2302 commented 2 years ago

@msubash26 @astanciu @spoonknife22 It has been included in the release candidate for: 10.11.0. Which should be the next release, but it needs to go through the QA process first!

For 10.10.1 we fixed an issue with the existing Lattice EIP.712 integration, which was already released and tested, thus it could go out faster.

Hi Sir, any idea regarding 10.11.0 release date?

blueblack2 commented 2 years ago

@msubash26 @astanciu @spoonknife22 It has been included in the release candidate for: 10.11.0. Which should be the next release, but it needs to go through the QA process first!

For 10.10.1 we fixed an issue with the existing Lattice EIP.712 integration, which was already released and tested, thus it could go out faster.

Is this working yet? According to this https://github.com/MetaMask/metamask-extension/releases 10.11 hasn't included support for Trezor EIP 712 signing but 10.10 v2 says it has, is it working?

FrederikBolding commented 2 years ago

@blueblack2 It was released in 10.10.2 instead and should be live as of last week! https://github.com/MetaMask/metamask-extension/releases/tag/v10.10.2