Support domain-only EthereumSignTypedData and other improvements #1033

aloisklink · 2022-01-27T23:59:16Z

Add documentation on using EthereumSignTypedData on T1 and improved types. These were missing from #1015.

Minor breaking change

EthereumSignTypedData now requires passing Trezor T variables. This seems makes sense from my side, since everybody using this function should pass Trezor T variables anyway, and since this function was only recently released, I doubt this change would affect any users of this library.

Plus, it makes all the code (especially types!) simpler.

Trezor Model 1 parameters are still optional, as blind signing requires using an extra dependency on an external library, and is less secure.

Support for `primaryType` = `EIP712Domain`

I tried adding docs/tests/types for trezor/trezor-firmware#2036 in this PR too, but updating the submodules/trezor-common submodule just causes a bunch of other Protobuf type errors to occur, so I'll do that in another separate PR in the future.

I've added support for trezor/trezor-firmware#2036 in this PR too, since they both edit similar files.

I've tested locally using:

./tests/run.sh -D 'podman' -i 'ethereumSignTypedData' Success now Fix getFirmwareRange() for multiple matching firmware ranges #1042 been merged ~~Fails~~
./tests/run.sh -D 'podman' -i 'ethereumSignTypedData' -f '2-master' Success
./tests/run.sh -D 'podman' -i 'ethereumSignTypedData' -f '1-master' Success

src/js/core/methods/EthereumSignTypedData.js

aloisklink · 2022-01-28T00:03:13Z

src/js/core/methods/EthereumSignTypedData.js

+        if (payload.message_hash) {
+            this.params = {
+                ...this.params,
+                domain_separator_hash: payload.domain_separator_hash,
+                message_hash: payload.message_hash,
+            };
+        }


This is in a separate if-statement to make Flow-type happy.

src/js/core/methods/EthereumSignTypedData.js

mroz22 · 2022-01-31T08:22:24Z

Hi, we are switching to master branch of trezor-common here https://github.com/trezor/connect/pull/1036/files and yes. It needs to sort out some types incompabilities. Patience please.

mroz22 · 2022-02-03T13:29:31Z

Updated trezor-common is now in develop branch.

mroz22 · 2022-02-04T11:45:24Z

Hello @aloisklink
We are generally quite happy for your contributions as they are always really nicely done, so If it suits your needs better this wa I think I am not going to object against that. Just if you could please rebase this on the latest develop so that I can run T1 tests in gitlab CI (we have added better support for this in one of the last commits).

src/js/types/networks/ethereum.js

aloisklink · 2022-02-04T11:57:25Z

We are generally quite happy for your contributions as they are always really nicely done, so If it suits your needs better this wa I think I am not going to object against that. Just if you could please rebase this on the latest develop so that I can run T1 tests in gitlab CI (we have added better support for this in one of the last commits).

No worries, this isn't urgent, this PR is currently mainly just documentation/improved-types.

I'm also very close to being done with adding support for trezor/trezor-firmware#2036 (I can push it into this PR, or into a separate one), which fixes any of the failing tests in ethereumSignTypedData due to updating trezor-common.

I did encounter a bug with getFirmwareRange(): it only uses the first matching entry in src/data/config.json, but I believe I've fixed it. I'm just testing it locally, I'll make a PR in a few hours!

aloisklink · 2022-02-04T14:08:35Z

Just if you could please rebase this on the latest develop so that I can run T1 tests in gitlab CI (we have added better support for this in one of the last commits).

Rebased on the latest develop in fee6389

I did it last night before you commented, so GitHub's UI is showing the commit before your comment.

I also added support for trezor/trezor-firmware#2036 in 0bff3e5, so that the T1/TT tests now pass on the updated fixtures. Let me know if you'd prefer me to put this into a separate PR.

I've tested locally using:

./tests/run.sh -D 'podman' -i 'ethereumSignTypedData' Fails, since getFirmwareRange() isn't throwing a wrong firmware error (see Fix getFirmwareRange() for multiple matching firmware ranges #1042)
./tests/run.sh -D 'podman' -i 'ethereumSignTypedData' -f '2-master' Success
./tests/run.sh -D 'podman' -i 'ethereumSignTypedData' -f '1-master' Success

Fixup commits

2fbfc67 Fixes the unit test for getUnavailableCapabilities() (I didn't realise that changing src/data/config.json would affect this test)
7139d79 Renames flow-type EthereumSignTypedHashOrData to EthereumSignTypedHashAndData and adds it to the TypeScript definitions too.

src/js/plugins/ethereum/typedData.js

mroz22 · 2022-02-08T10:28:05Z

Hi, please rebase and feel free to include everything you need into this PR. I think it looks good so once conflicts are resolved we can merge it.

BREAKING CHANGE: EthereumSignTypedData now requires passing Trezor T variables. Trezor Model 1 parameters are still optional, as blind signing requires using an extra dependency on an external library, and is less secure.

Add support for signing domain-only data in ethereumSignTypedData, e.g. when primaryType = EIP712Domain. On Trezor T, this behaviour previously gave incorrect signatures or threw errors. On Trezor Model 1, not passing the "message_hash" parameter caused an error.

Adds support for domain-only EIP-712 hashes to plugins/ethereum/typedData.js

ethereumSignTypedData now passes on tests, so we can add it to CI.

aloisklink · 2022-02-09T03:53:14Z

Thanks for the feedback! I've rebased on develop and --autosquash-ed the !fixup commits.

Since develop included #1042, the ethereumSignTypedData tests now pass on the new/old firmware versions that I tested, so I also added ethereumSignTypedData into the CI in 5530f40

It's ready to merge from my side 😄

Side-note, I didn't add a CI test for src/js/plugins/ethereum, but I can add one in a separate PR if you think it's worth it (I'd say yes on GitHub Actions because it's free, but maybe not worth the extra CI minutes on GitLab). It would be easy to add, just a cd src/js/plugins/ethereum && yarn install && yarn test:unit.

mroz22 · 2022-02-09T07:13:46Z

Side-note, I didn't add a CI test for src/js/plugins/ethereum, but I can add one in a separate PR if you think it's worth it (I'd say yes on GitHub Actions because it's free, but maybe not worth the extra CI minutes on GitLab). It would be easy to add, just a cd src/js/plugins/ethereum && yarn install && yarn test:unit.

Running plugins in CI would be great. Github is a good fit for this as it runs for external contributions automatically (gitlab does not) and plugins are mostly developed by external contributors I guess. So I think we would appreciate such effort.

aloisklink commented Jan 28, 2022

View reviewed changes

mroz22 reviewed Feb 4, 2022

View reviewed changes

src/js/types/networks/ethereum.js Show resolved Hide resolved

aloisklink mentioned this pull request Feb 4, 2022

Fix getFirmwareRange() for multiple matching firmware ranges #1042

Merged

aloisklink force-pushed the ethereumSignTypedData branch from c7e516b to 0bff3e5 Compare February 4, 2022 13:11

aloisklink commented Feb 7, 2022

View reviewed changes

src/js/plugins/ethereum/typedData.js Show resolved Hide resolved

aloisklink added 3 commits February 9, 2022 02:45

refactor(ethSignTypedData)!: Improve docs & types

b0890bb

BREAKING CHANGE: EthereumSignTypedData now requires passing Trezor T variables. Trezor Model 1 parameters are still optional, as blind signing requires using an extra dependency on an external library, and is less secure.

fix(plugins/ethereum): Support domain-only EIP712

2ad21b0

Adds support for domain-only EIP-712 hashes to plugins/ethereum/typedData.js

aloisklink force-pushed the ethereumSignTypedData branch 2 times, most recently from d780e6e to 4e8873a Compare February 9, 2022 03:18

ci(ethereum): Enable ethereumSignTypedData in CI

5530f40

ethereumSignTypedData now passes on tests, so we can add it to CI.

aloisklink force-pushed the ethereumSignTypedData branch from 4e8873a to 5530f40 Compare February 9, 2022 03:30

aloisklink changed the title ~~refactor(ethSignTypedData)!: Improve docs & types~~ Support domain-only EthereumSignTypedData and other improvements Feb 9, 2022

mroz22 approved these changes Feb 9, 2022

View reviewed changes

mroz22 merged commit d2e4ad3 into trezor:develop Feb 10, 2022

aloisklink deleted the ethereumSignTypedData branch February 10, 2022 12:06

aloisklink mentioned this pull request Feb 13, 2022

Add CI tests for Ethereum plugin #1050

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support domain-only EthereumSignTypedData and other improvements #1033

Support domain-only EthereumSignTypedData and other improvements #1033

aloisklink commented Jan 27, 2022 •

edited

Loading

aloisklink Jan 28, 2022

mroz22 commented Jan 31, 2022

mroz22 commented Feb 3, 2022

mroz22 commented Feb 4, 2022

aloisklink commented Feb 4, 2022

aloisklink commented Feb 4, 2022

mroz22 commented Feb 8, 2022

aloisklink commented Feb 9, 2022

mroz22 commented Feb 9, 2022

Support domain-only EthereumSignTypedData and other improvements #1033

Support domain-only EthereumSignTypedData and other improvements #1033

Conversation

aloisklink commented Jan 27, 2022 • edited Loading

Minor breaking change

Support for primaryType = EIP712Domain

aloisklink Jan 28, 2022

Choose a reason for hiding this comment

mroz22 commented Jan 31, 2022

mroz22 commented Feb 3, 2022

mroz22 commented Feb 4, 2022

aloisklink commented Feb 4, 2022

aloisklink commented Feb 4, 2022

Fixup commits

mroz22 commented Feb 8, 2022

aloisklink commented Feb 9, 2022

mroz22 commented Feb 9, 2022

aloisklink commented Jan 27, 2022 •

edited

Loading

Support for `primaryType` = `EIP712Domain`