-
-
Notifications
You must be signed in to change notification settings - Fork 671
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Adding support of EIP-712 into Ethereum app #1568
Conversation
PTAL @prusnak, is there something that holds from reviewing this feature implementation? |
PR looks rather fine. Thank you for that! We are in the process of working on stuff that landed in our pipeline earlier than this PR. Once our engineers have more spare cycles they'll review the PR and provided more detailed feedback. |
Heads up, without this we cannot sign for Uniswap v3 - this is a big deal and would prevent all trezor users from making use of their liquidity. |
I'm new to github and trying to understand how I can check progress of tickets, in particular this one and when it will be complete. This is a pretty big deal. Thanks for all the work devs! |
Any feedback for integrating this into the next firmware update? |
Hi everyone, thanks a lot for the PR. We won't be able to include this in the upcoming June release. Thank you for your patience! |
Tomas, that is quite unfortunate. Many of us on Injective Protocol Discord and Telegram have been discussing this issue. Many of us couldn't participate in the INJ Equinox Testnet and earn APY because we had our INJ tokens in the Trezor wallet. We have been patiently waiting for the firmware update for this. INJ will be launching mainnet very soon, anytime now within 2Q (before end of June 2021), and we were all hoping to use our Trezor wallets to start staking upon the INJ mainnet launch. Is there anything you can manage to get this included asap? Otherwise, all of us would have to get a Ledger and migrate all of our INJ from Trezor to Ledger since Ledger supports EIP-712. Please advise? |
This PR concerns not only Injective Protocol users but Uniswap v3 users as well. The number of people that are restricted in using both Injective Protocol apps and Uniswaps V3 update is enormous. Please reconsider having the update in the next firmware release. Kind regards. |
Please keep the discussion on-topic. Off-topic comments will be removed. Even if the EIP-712 support was merged into the Firmware, there is still Connect part of things missing, so it's not like the support will be immediately usable after this PR is merged. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi Max! Thank you for the pull request, I went through the changes and got some preliminary feedback. I think the overall design looks good, however there's couple of things we need before we can consider merging:
- There has to be good test coverage - encoders/decoders like this one often have tricky corner cases.
- CI pipeline needs to pass: https://gitlab.com/satoshilabs/trezor/trezor-firmware/-/pipelines/310943878 - the legacy jobs probably failed due to "gen prebuild". Let me know when you want to rerun it.
We'll also need the Product team to review the UI.
raw_type = parsed_type["raw_type"] | ||
|
||
print("abi_decode_single", parsed_type, "data:", data, "offset:", offset) | ||
print("type_name", type_name, "raw_type", raw_type) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No print()
s please.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch, leftovers from debug :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Deleted in a05275a
*/ | ||
message EthereumSignTypedData { | ||
repeated uint32 address_n = 1; // BIP-32 path to derive the key from master node | ||
optional bool use_v4 = 2 [default=true]; // use EIP-712 (v4) for typed data signing |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What exactly does false
mean, that v3
is used? From OP and metamask docs it appears that uint or enum field would make sense here if there's a chance of future versions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Metamask has its own implementation history, all v1-v2-v3 were not compliant with the latest standard and should not be considered.
Their implementation v4
is widely adopted now, but still, has a bug described there:
MetaMask/eth-sig-util#107 (comment)
So at one day, it might be a v5
.
I agree that having version enumeration is better than trying to maintain only a single or two "true" versions. I'm not a big fan of maintaining that as part of HW, but we have no choice there.
@@ -45,6 +45,7 @@ autoflake = "*" | |||
flake8-requirements = ">=1.3.2" | |||
|
|||
# common | |||
eth_abi = "^2.1.1" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this dependency belongs to python/setup.py
in extras_require
, this pyproject.toml is just for the development/build environment.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Got it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Addressed in a05275a
@cli.command() | ||
@click.option("-n", "--address", required=True, help=PATH_HELP) | ||
@click.option("--use-v4", type=bool, default=True, required=False, help=TYPED_DATA_USE_V4) | ||
@click.argument("typed_data_json") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The jsons are quite large, would be nice to be able to read them from file. With @click.argument("file", type=click.File("r"))
you can still use stdin as -
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Got it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Implemented in a0ce878
@@ -2,332 +2,668 @@ | |||
# fmt: off | |||
|
|||
from .Address import Address | |||
from .Address import Address |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Something bad happened to this file:(
|
||
m = type_name_re.match(primary_type) | ||
if m: | ||
primary_type = m.string[m.start():m.end()] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
primary_type = m.string[m.start():m.end()] | |
primary_type = m.group(0) |
^should also work. Also what's the point of the match, strip []
from the end? Can primary type be an empty string?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If it's more canonical then OK. I'm from Go background, for us, the explicit code is better.
The goal is to strip also [][5]
and whatever it might have on the end. We're interested in the Type name only.
Can primary type be an empty string?
The type name must be a valid identifier as a name in Solidity. So the answer is no.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, it's how it's usually written. If it can't be empty then let's change the regex to ^\w+
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changed in a05275a
for key in allowed_typed_data_properties: | ||
val = data.get(key) | ||
if val is None: | ||
continue |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking at the spec it seems that the 4 data properties are in fact required?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch. I was following this code https://github.com/MetaMask/eth-sig-util/blob/41a62c8255ce061130f85876be178e65e6ae99a2/src/index.ts#L270-L289
But didn't notice they actually check the result using TypeScript's type matching Required<TypedMessage<T>>
.
I think we need to require every key here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Addressed in a05275a
sanitized_data[key] = val | ||
|
||
if "types" in sanitized_data: | ||
sanitized_data["types"] = { "EIP712Domain": [], **sanitized_data["types"] } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not sanitized_data["types"]["EIP712Domain"] = []
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't get the problem here. This line was a carbon copy of this one:
https://github.com/MetaMask/eth-sig-util/blob/41a62c8255ce061130f85876be178e65e6ae99a2/src/index.ts#L286
I tried to stay close to that implementation. Isn't this a correct translation?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not familiar with TypeScript, the translation is probably alright, just very unusual to write it like this in python.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
that might be idiomatic typescript (presumably? i'm not fluent) but it's decidedly not idiomatic Python. let's go with @mmilata's suggestion instead
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems this line has a meaning in the way it is written - we want to make sure the EIP712Domain
key is always there and defaulting to an empty list. When it is already present in sanitized_data["types"]
, it will then overwrite this empty list with current value (therefore the dict unpacking with **
).
What is the best way of handling this situation? We could do an explicit check for this:
if "EIP712Domain" not in sanitized_data["types"]:
sanitized_data["types"]["EIP712Domain"] = []
When using only sanitized_data["types"]["EIP712Domain"] = []
, we received FirmwareError (99)
- most probably is complaining about the EIP712Domain
being empty (it probably shouldn't though)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changed in a05275a
|
||
return result, result_indexed | ||
|
||
allowed_typed_data_properties = ["types", "primaryType", "domain", "message"] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Constant sequences are usually defined as tuple and named in SCREAMING_SNAKE_CASE.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changed in a05275a
|
||
def is_array(type_name: str) -> bool: | ||
if type_name: | ||
return type_name[len(type_name) - 1] == ']' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
return type_name[len(type_name) - 1] == ']' | |
return type_name[-1] == ']' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changed in a05275a
Hey, thanks for getting some time to review this. I really appreciate this is moving :) Yeah, I agree that some tests are required, especially for the encoders and decoders I implemented. I was waiting until the logic itself is reviewed. So my next steps would be:
|
Yep, sounds good! As part of testing it would also be interesting to find out what's the largest structure the device can sign before running out of memory, or at least make sure that it doesn't happen with structures that have "normal" size. |
Hello everyone. Just to inform you all about the state of things, we will need our Product team to look into this as we are not sure showing such advanced structures on Trezor is a good idea. We are really trying to push usability of Trezor the right direction and we are worried users will just blindly confirm whatever structure is presented before them which is something we do not want to encourage. We will get back to you soon on this. |
Most of EIP712 use cases are rather short. Just domain confirmation and then a small message of 1-2 fields. The alternative approaches would be to blindly approve hash (Ledger's approach) or just don't support EIP 712 and keep user's funds locked. The approach proposed here allows skipping message verification for known domains. Or skip everything, just approving the hash - in this way it falls back to the same flow as Ledger already has. For people who decided to actually study the data they signing, I was very glad that Trezor can bootstrap a nice UI to walk through it. My example with our message is an extreme case that aims to validate the approach on big payloads. It's easy to implement bad UI when use cases just contain a few strings. |
Any approximate timeline on how long this will take? As earlier posters have pointed out it sounds like this is needed for Uniswap V.3 as well as for Injective staking. If you foresee this taking several months longer please let us know so we can look into other options like a Ledger. Thank you. |
If we get the Trezor One or Trezor T now, can we sign eip 712 signTypedData_v4 transactions as of now? After reading all the comments, it's unclear and with all the activities happening on Ethereum, this is holding trezor users back. |
This is stoping people to use their trezor for opensea on matic, can we speed this up please for the users and community |
Sorry guys for been recursive as I replied on #1272 , but this issue is quite critical. Yes guys, we need this to be implemented! Signing in OpenSea doesn't even works with a Trezor One (with latest firmware v1.10.3) connected to MetaMask. You guys can do it! |
We are also blocked in our development by this issue. Thanks for solving in advance. |
@matejcik has these changes gone live yet as a firmware update? I'm here losing a ton of money over here. I want my NFTs out of trezor already. This is insanely unacceptable. |
trezor has completely abandoned their original flagship product, the trezor one, and my nfts stuck in there will be forever. They’re also actively false advertising by saying the trezor one supports ETH yet it doesn’t support one of the most critical EIPs in existence. Even worse is there are many people with NFTs trapped in trezor ones and we are all collectively ignored and told to extract our seeds to software wallets or the model T. Ledger supported ETH fully on the Nano S a device with less memory than a flip phone from 1996, trezor should be embarrassed |
@roshhhyy @remixie since they don't support new transactions, you can rollback to a year old metamask extension to support older transactions, you will pay more since you cannot control basefee and priorityfee, but hopefully you will be able to do legacy transactions. Regarding signing newer transactions, you are unfortunately stuck until metamask implements it, since trezor was very very late to implementing EIPs. But yea, Trezor is unfortunately a Bitcoin first company, but majority of its users are Ethereum. |
@mohamedmansour I have tried the tactic of using metamask 9.8.4. I get the same errors on opensea. I have tried multiple computers, reconnecting my trezor, reinstalling metamask on a clean profile for both browsers, everything but I still get stopped by trezor's lack of support rn |
EIP-712 for Trezor One is being developed here and will be hopefully released in January: #1970 |
Wow! Thanks! That's what I'm talking about! Being unstoppable and decided guys! I'm going to remove my complaint tweet now regarding the other ticket! I hope everything goes well with that and that you comply with it! |
thank you this means a lot |
Done, I deleted, but sad, no idea you blocked me on twiter. I run out of hard feelings. So i will be alone with my Trezor and other people I recommended months ago! |
So what is the timeframe for Trezor Model T? |
After much much investigation, I found out that I can export my trezor private wallet key by following this youtube tutorial: https://www.youtube.com/watch?v=aBy3EUqQLrg
Note: you have to hover over the private key for the QR code to appear By doing these steps, I was able to take out my NFT's from my trezor and put them in my ledger right away. |
i tred to enter it withmeta mask wallet, but cant, then most of my etheirium is gone less than1% was lest in my account. what do i do |
why cant i sign in to dcl and pls join my account i cant open more im getting lost |
Intro
Hello 👋! I'm Max from Injective Protocol. We're building a fully decentralized trading platform for derivatives. It's an L2 chain based on Cosmos SDK, and we rely heavily on EIP-712 typed data standard for interoperation with Ethereum and MetaMask. Basically, this allows us to sign arbitrary Cosmos transactions using Eth wallets like MetaMask or Ledger.
I know there are a lot of people trading/minting NFTs nowadays, many of which are minted through L2 solutions, almost all the time depending on EIP-712 as well. There are plenty of issues about EIP-712 support in hardware wallets like this one MetaMask/metamask-extension#10240
We have users with Trezor wallets too, so I decided to step in and help with the EIP-712 feature implementation for the firmware. My Pythonic code is based upon
signTypedData_v4
reference implementation from MetaMask (see the TypeScript eth-sig-util code also signTypedData_v4 docs) since we need to maintain compatibility with current Web3 industry standard. It's also a fact that their*_v4
implementation has a bug that makes it not compliant to the official EIP-712 standard - see MetaMask/eth-sig-util#106, so one day it could be*_v5
.Meanwhile, we have to use Metamask's
v4
everywhere and allow users to fall back to Geth's (which I'm not sure is used anywhere). There is a special flag--use-v4
in the reference client implementation that can be set toFalse
.In this PR I'm submitting a working implementation of
v4
typed data signatures, also a nice UI that in my opinion has the best balance between verbosity of the layout and the ability to introspect data on the device. This is not a "WIP" request, in my opinion, it's ready for being field-tested.Click for the video👇
Data exchange flow
The exchange between client and device is defined by proto schema and consists of 3 messages:
EthereumSignTypedData
- initial request for starting a signing flowEthereumTypedDataRequest
- request from device to send additional dataEthereumTypedDataAck
- the fulfilled request with additional data types and valuesIn the final
EthereumTypedDataRequest
device tellssignature
of the data andaddress
of the signer.The general idea is that the device gets all the types from EIP712 struct first, and uses them as a stencil to get the rest of the data values. In that way, deeply nested structures are still a possibility, but the device cannot be "led by the nose" by the client. The device always knows the full layout before reading any values.
The flow on device can be summarized like this:
Where
collect_domain_types
andcollect_types
are collecting the data schema, andcollect_values
is collecting the data values, communicating with the client. The rest is a 100% computational workload.Ethereum ABI
Another important detail for working with EIP-712 typed data, and Ethereum in general, is the data encoding format called Eth ABI. EIP712 tells that there is a subset of types supported in the data definitions, but the encoding of the types really matches the standard ABI. Basically, ABI encoding allows us to represent any primitive data (no structs) as bytes, even an array of primitive types, like
[]uint256
, so in the data flow described above the client transfers data on the device already ABI-encoded.For the reference implementation in Python, I've picked eth-abi as it is the most sophisticated implementation out there and works nicely with native Python types already. I'm also using its
eth_abi.packed
variant since EIP-712 doesn't expect all those offset headers in the encoded values.For the device-side ABI encoding and decoding, which is important (since we need to show all those arrays of data on the UI), I've decided to port JS version of ABI codec from ethereumjs-abi - it's relatively small, easy to understand, also we encode only primitive types for EIP712 sake. Also, it's used by the aforementioned
eth-sig-util
lib from MetaMask. Some helper functions for detecting type properties were useful for the client implementation as well.UI/UX Flow
And the most important part of this implementation is the interface for easy data introspection. Based on standard Trezor Layout practics, it gradually briefs users about the data being signed and allows them to descend into the deepest depths of the schema, if the user wants that.
A typical signing flow looks like this on the device:
1. Domain brief 👀
We present some brief about
EIP712Domain
, such as Name and Version data from it, so the user can know the vendor of the message immediately. User may choose to inspect the domain closely, a good thing it's a plain simple struct.2. Domain inspection 👀
Structs are shown as paginated lists of fields. In the title, there's a struct name, also a pagination mark. The pagination mark shows the current field number and total field number in the struct. It's very simple for a static domain struct.
3. Message brief 👀
Once the user dealt with domain verification, we show him the message brief. It's impossible to print all fields from the message, so we decided to compose a maximum of 3 elements and note that there are more to inspect. Many times the user would skip checking the message thoroughly just because he trusts the domain already, and sees some of the fields of the message. The title contains the
primary_type
of the message.4. Message inspection 👀
And this is where the rabbit hole begins 🐰. Since the combination of types, structs and arrays is completely arbitrary and depends only on the application, it's up to app developer to design messages in a clean way. Our Cosmos transactions converted into EIP-712 are not the best UX example, but at least it's a good durability test for the inspection.
👆This is a reference example of a typed data transaction shown in MetaMask
The inspection flow on Trezor consists of the following steps:
fee 3/6
(typeFee
) andmsgs 5/6
(type[]Msgs
)Some tricky examples:
(1a)(1b)(2a)(2b)(3a)(3b)
3/6
field namedfee
in primary struct (Tx
). Struct has typeFee
and has 2 fields.1/2
field in theFee
struct namedamount
. It's array[]Coin
with only 1 element.value
field ofMsg
struct as the first element inmsgs []Msg
array field in the primary struct. Thevalue
struct has typeMsgValue
and has 4 fields.4/4
) ofMsgValue
, which is calledvalue.bridge_fee
(gracefully trimmed), it's aTypeBridgeFee
type and it has 2 fields inside.2/4
) field ofMsgValue
in the paginated struct overview. It's a simple string field namedeth_dest
.6/6
) field of the primary struct (Tx
) paginated for final overview. At this point, the user has the option to approve the primary struct, and finalize the signing. Trezor emits a signature.Short path:
Obviously, not everybody needs a full introspection of the typed data mazes. There's always a short path for typed data signing without the need to look into data at all. It takes only
2 clicks
and1 hold
to sign a typed data hash.Notes
Support for EIP-712 is not implemented for Trezor T1 model (Legacy) yet, but it's far more trivial to implement. The approach would be the same as for the Ledger app, where the device only accepts 2 hashes (domain separator, a message hash) and simply signs the EIP-712 digest. Maybe it will be implemented in the upcoming days.
This PR: