Implement support for EIP-712

[rmeissner]

Support for EIP-712 should be added to the trezor firmware and the python client.

Reference: trezor/trezor-core#122

[rmeissner]

@prusnak I started working on it and I might need some help. The passed data for signing is json and this needs to be parsed. For this I wanted to use ujson, but it is not available. Could you give me a pointer how to parse json with micropython (didn't find anything good on google)

[prusnak]

Simple: Don't parse JSON with MicroPython. Parsing should be done on the client and the result of parsing should be passed in a protobuf message.

[prusnak]

Quick look at EIP721 shows you need to convert the following schema into a protobuf message and fill it accordingly: https://github.com/ethereum/EIPs/blob/master/EIPS/eip-712.md#parameters

There is even a tool for that: https://www.npmjs.com/package/jsonschema-protobuf

[rmeissner]

I thought I could go the lazy way, but makes sense. Will look into that. Thanks for the quick answer

[rmeissner]

@prusnak I am running into the problem that EIP-712 requires nested structures (basically json objects). While it is not hard to represent this in protobuf, but the python code generated by pb2py contains cyclic imports and cyclic class dependencies. Before spending a very long time on getting around that I was wondering if there is a known solution for this.

[matejcik]

@rmeissner care to show an example of a structure that is causing problems?

[rmeissner]

Initially I wanted to use https://github.com/protocolbuffers/protobuf/blob/master/src/google/protobuf/struct.proto and adjust it for trezor (use proto2 and change double to string, since double is not supported). But I then simplified it to get started to

   message TypedData {
        required uint32 type = 1;
        optional string value = 2;
        repeated TypedData data = 3;
        required string name = 4;
    }

But for that I also had to make minor adjustments to pb2py (prevent self-imports).

I will also look into how I can have a different representation of the EIP-712 data.

[matejcik]

Let's look at this from a different angle. What sort of data are actually going to be signed? What sort of data can be displayed on Trezor screen? Couldn't we limit the impl to one or a bunch of predefined structures?

[rmeissner]

So the data signed in the end is a hash, but the whole point is that it is possible to display everything that is signed readable to the user. It can be compressed to 2-4 lines that then in the end are hashed for the signature. But I thought it would make sense that the hardware wallet can verify the integrity of the data to sign.

I will play with a couple structures that are simplified.

[rmeissner]

Another question related to that, if I want to "scroll" through the text, is there some helper for that or do I have to implement it myself? Also are there some docs related to the layouting (taking about the new Trezor for now)

[matejcik]

re scrolling: look at this: https://github.com/trezor/trezor-firmware/pull/87/files#diff-a910630f08a24cf339ee06d510f2c32eR303

no docs at the moment, but we have a rework of the layout API in progress, so maybe then.

re signed hash: yes, i understand that we would like to display the structured data. Where I'm going is, can we constrain what sort of messages are allowed, and still support >90% of usecases?
At first look it would make sense to disallow nesting and complex types.
Vaguely someting like:
say:

message SignEIP712 {
	optional string struct_name;
	optional uint32 num_members;
}

message EIP712Request {
	optional uint32 member_index;
}

enum EIP712Type {
	BOOLEAN = 1;
	UINT = 2;
	...
}

message EIP712Ack {
	optional string member_name;
	optional EIP712Type type;
	optional bytes value;
}

(this way you also avoid memory problems with large messages)

[rmeissner]

I see what you mean (similar to https://github.com/trezor/trezor-firmware/blob/master/core/src/apps/ethereum/sign_tx.py#L82). Wasn't aware that this is possible.

I have a very straight forward implementation for now (all data at once), but I see your point about the memory optimizations.

Sending data parameter should not be that hard to implement and should probably also be extendable for nested objects.

For our use case we don't need nested objects, so I will try to get something.

I will push the "naive" implementation later to get some feedback (since implementing for embeded systems is different than for mobile or services :) )

[rmeissner]

@matejcik I created a PR to get some feedback. I used a similar scheme to what you proposed, but currently I am running into the issue that the trezor emulator gets stuck after I display a confirm dialog (the next call blocks everything), am I missing something obvious?

[matejcik]

let's continue discussion in the PR

[ZdenekSL]

Closed due to inactivity.

[raymonddurk]

This is a prerequisite to any ZKrollups in the future. This will limit certain uses cases and dapps if it isn't picked up again @rmeissner

[rmeissner]

yes I will try to pick this up at some point again, but this is not the most trivial implementation to do on the side.

[SCBuergel]

yes I will try to pick this up at some point again, but this is not the most trivial implementation to do on the side.

This is a very desperately needed feature for a more safe and scalable Ethereum. Any chance we can get this going ASAP?

[angyts]

hi, dear devs, please help us look into this. I have had multiple issues with a couple of dApps and it really frustrates me to no end. So much so that I'm considering abandoning the idea of using a trezor.

1. Coordinape is a Yearn finance thing to help us allocate tokens to people who have contributed to a project. I just couldn't sign properly. The signature will appear on metamask, but will not appear on the trezor.

2. Opyn is an options trading platform, I cannot get any limit orders done, because it requires a certain signature type, it will appear on metamask and again , will not appear on the trezor.

3. Community canvas drawing is a fun thing for everyone who is part of a community to participate in drawing a shared canvas. And metamask + trezor just doesn't support it.

Thank you so much for looking into this. Please let us know what we can do about this.

Should I write up a gitcoin grant to get some funding?

[prusnak]

@angyts the feature is being implemented here - #1568

[0xmichalis]

It seems like Trezor T support is merged: #1835

Are there any plans to add support in Trezor One?

[prusnak]

Are there any plans to add support in Trezor One?

Not at the moment, but there is a slight chance we will revisit that later.

In the meantime I am suggesting everyone getting a Trezor Model T if you need to use EIP-712 now.

[prusnak]

EIP-712 for Trezor One is being developed here and will be hopefully released in January: #1970