Skip to content
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.

Sign up for GitHub

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

Jump to bottom

EIP-712 support for Trezor T #1835

Merged
merged 5 commits into from Nov 2, 2021
Merged

EIP-712 support for Trezor T #1835

merged 5 commits into from Nov 2, 2021

Conversation

grdddj
Copy link
Contributor

@grdddj grdddj commented Oct 6, 2021

EIP 712 feature based on the original PR #1568

@matejcik matejcik marked this pull request as draft October 6, 2021 13:36
@trezor trezor locked as too heated and limited conversation to collaborators Oct 6, 2021
matejcik
matejcik reviewed Oct 7, 2021
View reviewed changes
Copy link
Contributor

@matejcik matejcik left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

some preliminary comments

core/src/apps/ethereum/layout.py Outdated Show resolved Hide resolved
core/src/apps/ethereum/sign_typed_data.py Outdated Show resolved Hide resolved
core/src/apps/ethereum/sign_typed_data.py Outdated Show resolved Hide resolved
core/src/apps/ethereum/sign_typed_data.py Outdated Show resolved Hide resolved
core/src/apps/ethereum/sign_typed_data.py Outdated Show resolved Hide resolved
common/protob/messages-ethereum.proto Outdated Show resolved Hide resolved
core/src/apps/ethereum/sign_typed_data.py Outdated Show resolved Hide resolved
core/src/apps/ethereum/sign_typed_data.py Outdated Show resolved Hide resolved
core/src/apps/ethereum/sign_typed_data.py Outdated Show resolved Hide resolved
core/src/apps/ethereum/sign_typed_data.py Outdated Show resolved Hide resolved
@prusnak prusnak removed their request for review October 7, 2021 19:57
matejcik
matejcik reviewed Oct 14, 2021
View reviewed changes
Copy link
Contributor

@matejcik matejcik left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

partial review, more to come

core/src/apps/ethereum/sign_typed_data.py Outdated Show resolved Hide resolved
core/src/apps/ethereum/sign_typed_data.py Outdated Show resolved Hide resolved
core/src/apps/ethereum/typed_data.py Outdated Show resolved Hide resolved
core/src/apps/ethereum/typed_data.py Outdated Show resolved Hide resolved
core/src/apps/ethereum/typed_data.py Outdated Show resolved Hide resolved
core/src/apps/ethereum/typed_data.py Outdated Show resolved Hide resolved
core/src/apps/ethereum/typed_data.py Outdated Show resolved Hide resolved
core/src/apps/ethereum/typed_data.py Outdated Show resolved Hide resolved
core/src/apps/ethereum/typed_data.py Outdated Show resolved Hide resolved
tests/device_tests/test_msg_ethereum_sign_typed_data.py Outdated Show resolved Hide resolved
matejcik
matejcik reviewed Oct 14, 2021
View reviewed changes
Copy link
Contributor

@matejcik matejcik left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

using a class to wrap ctx, types, top-level primary_type, metamask_v4_compat, seems reasonable

core/src/apps/ethereum/typed_data.py Outdated Show resolved Hide resolved
core/src/apps/ethereum/typed_data.py Outdated Show resolved Hide resolved
core/src/apps/ethereum/typed_data.py Outdated Show resolved Hide resolved
core/src/apps/ethereum/typed_data.py Outdated Show resolved Hide resolved
core/src/apps/ethereum/typed_data.py Outdated Show resolved Hide resolved
core/src/apps/ethereum/typed_data.py Outdated Show resolved Hide resolved
core/src/apps/ethereum/sign_typed_data.py Outdated Show resolved Hide resolved
core/src/apps/ethereum/sign_typed_data.py Outdated Show resolved Hide resolved
core/src/apps/ethereum/layout.py Outdated Show resolved Hide resolved
core/src/apps/ethereum/typed_data.py Outdated Show resolved Hide resolved
@grdddj
Copy link
Contributor Author

grdddj commented Oct 15, 2021

using a class to wrap ctx, types, top-level primary_type, metamask_v4_compat, seems reasonable

I will try to do it in next commit, together with those async unit-tests of hash_struct and get_and_encode_data

matejcik
matejcik requested changes Oct 19, 2021
View reviewed changes
Copy link
Contributor

@matejcik matejcik left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

style comments

also we can probably flip this into non-draft, correct?

common/protob/messages-ethereum.proto Outdated Show resolved Hide resolved
common/protob/messages-ethereum.proto Outdated Show resolved Hide resolved
core/src/apps/bitcoin/sign_tx/bitcoin.py Outdated Show resolved Hide resolved
core/src/apps/common/confirm.py Outdated Show resolved Hide resolved
core/src/apps/ethereum/sign_typed_data.py Outdated Show resolved Hide resolved
core/src/apps/ethereum/typed_data.py Outdated Show resolved Hide resolved
core/src/apps/ethereum/typed_data.py Outdated Show resolved Hide resolved
core/src/apps/ethereum/typed_data.py Outdated Show resolved Hide resolved
core/src/apps/ethereum/typed_data.py Outdated Show resolved Hide resolved
core/src/apps/ethereum/typed_data.py Outdated Show resolved Hide resolved
matejcik
matejcik requested changes Oct 19, 2021
View reviewed changes
Copy link
Contributor

@matejcik matejcik left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

couple more comments

core/src/apps/ethereum/typed_data.py Outdated Show resolved Hide resolved
core/src/apps/ethereum/typed_data.py Outdated Show resolved Hide resolved
core/src/apps/ethereum/typed_data.py Outdated Show resolved Hide resolved
core/src/apps/ethereum/typed_data.py Outdated Show resolved Hide resolved
@matejcik matejcik added this to To be reviewed in Firmware Pull Requests via automation Oct 19, 2021
@matejcik matejcik moved this from To be reviewed to Finalizing in Firmware Pull Requests Oct 19, 2021
@matejcik matejcik moved this from Finalizing to In progress in Firmware Pull Requests Oct 19, 2021
@grdddj
Copy link
Contributor Author

grdddj commented Oct 19, 2021

also we can probably flip this into non-draft, correct?

Agree, I will flip it

@grdddj grdddj marked this pull request as ready for review October 19, 2021 15:44
@grdddj
Copy link
Contributor Author

grdddj commented Oct 21, 2021

In 2b5a0f1 the code structure was modified.
Changes made/current situation:

  • address.py was renamed to helpers.py, which meant modifying all the files currently importing from .address
  • get_type_name and decode_data moved to helpers.py, so that they can be used both by layout.py and sign_typed_data.py
  • typed_data.py was deleted, all the UI-related functions moved to layout.py and the rest into sign_typed_data.py

Possibilities:

  • we could even remove the need for helpers.py by sign_typed_data.py injecting the raw data to be shown into UI functions (decoding everything to strings), and then layout.py would not need these helpers dependencies at all, so get_type_name and decode_data could stay in sign_typed_data.py

grdddj
grdddj commented Oct 22, 2021
View reviewed changes
core/src/all_modules.py Outdated Show resolved Hide resolved
@matejcik matejcik changed the title Grdddj/eip712 EIP-712 support for Trezor T Oct 26, 2021
@grdddj
Copy link
Contributor Author

grdddj commented Oct 29, 2021

In 8d76e56:

  • putting device tests into right ethereum folder, creating JSON fixtures for it
  • adding changelog entries for core and python
  • regenerating fixtures.json - surprisingly, even some other ethereum-signtx... diff changed, we should check it in CI
  • documenting what is not supported in CLI docs

@matejcik
Copy link
Contributor

UI diff is here: https://satoshilabs.gitlab.io/-/trezor/trezor-firmware/-/jobs/1729625816/artifacts/master_diff/index.html

You changed the message on the "show more" button for Ethereum data, from "Show all" to "Show details". Please change it back -- presumably in the confirm_blob function?

matejcik
matejcik requested changes Oct 29, 2021
View reviewed changes
Copy link
Contributor

@matejcik matejcik left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

final (hopefully) round of comments.

common/protob/messages-ethereum-eip712.proto Outdated Show resolved Hide resolved
core/src/apps/ethereum/layout.py Outdated Show resolved Hide resolved
core/src/apps/ethereum/layout.py Outdated Show resolved Hide resolved
core/src/apps/ethereum/layout.py Outdated Show resolved Hide resolved
core/src/apps/ethereum/sign_typed_data.py Outdated Show resolved Hide resolved
core/src/apps/ethereum/sign_typed_data.py Outdated Show resolved Hide resolved
python/.changelog.d/1835.added Outdated Show resolved Hide resolved
python/src/trezorlib/ethereum.py Outdated Show resolved Hide resolved
@grdddj
Copy link
Contributor Author

grdddj commented Oct 29, 2021

UI diff is here: https://satoshilabs.gitlab.io/-/trezor/trezor-firmware/-/jobs/1729625816/artifacts/master_diff/index.html

You changed the message on the "show more" button for Ethereum data, from "Show all" to "Show details". Please change it back -- presumably in the confirm_blob function?

Thanks for the link and explanation. It was reverted in 76c739c.

Probably the only thing missing now is the UI device test clicking on "Show more", it will be included in the next commit.

@grdddj
Copy link
Contributor Author

grdddj commented Nov 1, 2021

Force-pushed as I forgot to regenerate new UI fixtures, and previous commit was a fixup not on EIP712, but on the legacy change

matejcik
matejcik approved these changes Nov 2, 2021
View reviewed changes
Copy link
Contributor

@matejcik matejcik left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

awesome!

please squash the fixups and rebase on current master

Pull Requests automation moved this from In progress to Finalizing Nov 2, 2021
@grdddj
refactor(core/ethereum): rename address.py to helpers.py
19e075d
@grdddj
fear(core/ui): add should_show_more layout function
454c5b3 
Also adding `hold` argument into confirm_blob function
@grdddj
feat(core): plural function supports "key/keys" string
f7bee94 
Previously function did not work for words ending with "y" and vowel before that
@grdddj
feat(core/ethereum): EIP-712
60e16a2 
Based on original contribution by Max Kupriianov <xlab@hey.com>

Implemented EIP-712 typed data signatures in Ethereum app.

Add eth_abi into pyproject deps

device test for EIP 712

fixed hex decoding for address

fixup! fixed hex decoding for address

code quality, more pythonic code, removing unused imports

running black and isort on changed files

trezorctl file input for EIP 712 data signing

fixup! code quality, more pythonic code, removing unused imports

fixup! fixup! code quality, more pythonic code, removing unused imports

necessary changes after rebase to master

unit tests for sign_typed_data.py

new protobuf messages, working for nonarray types

simplified and verified solution for our simple data

support for simple arrays, without their confirmation

reverting protobuf value messages to bytes, appropriate changes

showing arrays in Trezor, code quality improvements

data validation on Trezor, minor improvements

using custom types for storing type data instead of dicts, addressing feedback from review

moving helper functions to its own file, tests for decode_data

additional overall tests

support for arrays of structs

adding support for metamask_v4_compat variable

using HashWriter object to collect the final hash continously

minor improvements in code quality

validate_field_type function

streaming values from client without saving them, missing UI

prototype of streamed UI using confirm_properties

accounting for bytes in data, more data types in integration tests

rebase on master, using f-strings

minor fixes and improvements from code review

StructHasher class for the whole hashing process

mypy and style changes

asking users whether to show structs and arrays

protobuf descriptions to fix make defs_check

unifying comments, mypy fix

unit tests for StructHasher class

UI fixtures, skipping device tests for T1

addressing majority of code review comments about code quality and structure

changing file structure - layouts, helpers, sign_typed_data

decode_data renaming and docstring, renaming unit test file

using tuples instead of lists in elifs

layout improvements

excluding core/src/apps/common/confirm.py file from the PR

True/False returning layout with Show more button

code review layout improvements

forgotten br_type argument to should_show_more
@grdddj
chore(legacy): fix build after adding EthereumSignTypedData [no chang…
b908672 
…elog]
@matejcik matejcik merged commit 9d64380 into master Nov 2, 2021
@matejcik matejcik deleted the grdddj/eip712 branch November 2, 2021 13:27
Pull Requests automation moved this from Finalizing to Merged Nov 2, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@matejcik matejcik matejcik approved these changes

Assignees
No one assigned
Labels
None yet
Projects
Pull Requests
Merged
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@grdddj @matejcik