fix: eip712 signing with ledger hw

[sebastinez]

Motivation

This PR fixes the way a EIP712 derived struct is sent to the Ledger HW to obtain the signature.

Solution

Based on the following specification: https://github.com/LedgerHQ/app-ethereum/blob/master/doc/ethapp.asc#sign-eth-eip-712

• Adds a new INS variant = SIGN_ETH_EIP_712
• Replaces self.sign_message with self.signed_typed_struct
• signed_typed_struct creates a payload consisting of the domain separator and the typed data hash.
• This allows the Ledger to display both in its display to be signed by the user.
• It also checks if the Ledger ETH app is at least 1.6.0 since this is a requirement, according to Support for EIP-712 message signing LedgerHQ/app-ethereum#105 (comment).
  • If you prefer a different way to handle this issue please let me know!

PR Checklist

• Added Tests
• Added Documentation
• Updated the changelog

I tested the generated signature on a deployed contract on Rinkeby and it works great.
Please let me know if you have any feedback @gakonst @prestwich @Ryanmtate.

[sebastinez]

This PR resolves the issue #516

[Ryanmtate]

Nice work @sebastinez! Looks great!

[Ryanmtate]

Can we add a test case to the tests module?

[sebastinez]

Can we add a test case to the tests module?

Yeah should be no problem, the thing I'm missing is since I'm not able to mock a Ledger (at least I think so) it's kinda hard to assert something.
We could eventually check that an error gets thrown when the Eth app version does not match semver.

Did you have some specific test in mind?

[gakonst]

I think just writing a simple test for any 712 struct that you ignore by default with the Ledger signer is sufficient. Just want something that I could run locally and check that it works!

[sebastinez]

I think just writing a simple test for any 712 struct that you ignore by default with the Ledger signer is sufficient. Just want something that I could run locally and check that it works!

Okay understood, np 👍

[Ryanmtate]

I suggest we move the version check to the sign_typed_struct method, as it should be checked there as well if the method was used instead of sign_typed_data.

[sebastinez]

In b5587ba I moved the version check and the parsing of domain_separator and struct_hash to sign_typed_struct and let sign_typed_data be a simple entrypoint, like the other functions do in mod.rs

If we are able to merge let me know and I squash some commits and sign the remaining ones.

[gakonst]

OK this looks great. Thank you for the work on it. I made the test slightly tighter and manually verified that it works!