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.

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

WebWallet Conformance: Metamask & Ledger Nano #51

Closed
kalouo opened this issue May 8, 2020 · 10 comments
Closed

WebWallet Conformance: Metamask & Ledger Nano #51

kalouo opened this issue May 8, 2020 · 10 comments
Labels
blocked Blocked on a dependency

Comments

@kalouo
Copy link

kalouo commented May 8, 2020

No description provided.

@kalouo
Copy link
Author

kalouo commented May 25, 2020

First try:

Screen Shot 2563-05-25 at 15 21 02

@kalouo
Copy link
Author

kalouo commented May 25, 2020

Exits and deposits work but you need to activate "Contract Data" on the Ledger device. Solves the above. Reading around this may not be needed on newer devices.

@kalouo
Copy link
Author

kalouo commented May 25, 2020

Screen Shot 2563-05-25 at 16 41 45

Screen Shot 2563-05-25 at 16 06 29

@kalouo
Copy link
Author

kalouo commented May 25, 2020

However, cannot sign child chain transactions.

Returns Error: Not Supported on this device.

Understand there is no support yet for signing typed data with a hardware wallet. From Metamask documentation, Note that MetaMask supports signing transactions with Trezor and Ledger hardware wallets. These hardware wallets currently only support signing data using the personal_sign method.

Related issues/PRs:

@nicholasmueller
Copy link
Contributor

Adding a note here since I just tried it but the watcher wont accept a signature using personal_sign method

@nicholasmueller nicholasmueller added the blocked Blocked on a dependency label Jun 2, 2020
@pnowosie
Copy link

pnowosie commented Jun 24, 2020

Adding a note here since I just tried it but the watcher wont accept a signature using personal_sign method

OMG Network's transactions are signed with personal_signTypedData method (it's defined in eip-712). There are however some discrepacies how clients implement the method internaly (see also #omgnetwork/elixir-omg/issues/836)

@kalouo
Copy link
Author

kalouo commented Sep 9, 2020

@unnawut
Copy link

unnawut commented Sep 9, 2020

Also inputs from @Pongch

TIL, went through the signature rabbit hole today, may be useful for others; https://ethereum.stackexchange.com/questions/59880/solidity-ecrecovery-and-ethereumjs-ecsign-return-different-address
the reason signing raw private key via ecsign() works for sending transaction on the Network and not eth.sign()/personal_sign() is because web3/wallets may add the message to the prefix of the hash:

"\x19Ethereum Signed Message:\n32"

before the message gets signed.

only if the wallet exposes sign() without the prefix: it would be trivial to add EIP712 support like the one you see here 👇:skin-tone-2: and get child chain transaction to work https://github.com/ethereum/EIPs/blob/master/assets/eip-712/Example.js#L116-L128

@unnawut
Copy link

unnawut commented Sep 11, 2020

Internal Q&As for future reference:

How low level does the integration API get? How are they able to support all the various chains they do?
Are the questions specifically for their web3 support?

My understanding is we have these options:

  1. Integrate with metamask, then metamask talks via web3 API with the Ethereum app on ledger device: The current hurdle is the extra prefix Ledger is appending to the signature for security reasons and metamask is not able to parse it, so we need to come up with a solution that Ledger team is willing to accept. Question 6

  2. Integrate directly with Ethereum app on ledger device via web3 API: Docs are little thin on signing EIP-712 this way, so hopefully we know better from those questions

  3. For different chains, especially with different signing algorithms, a party must build the ledger C app and submit it to ledger for review: Most undesired for us due to effort required for development + submission for review. Plus UX hurdle that it diverges a lot from the usual Ethereum user flow. Would not support signing through metamask. But we might have to consider this route as a last resort.

@nicholasmueller
Copy link
Contributor

nicholasmueller commented Sep 11, 2020

To add to the above:

  • one thought was the feasibility of adjusting our system to accept personal_sign sigs (the only one ledger currently supports). Not feasible because that would affect every component in our system and the signature hashes are already well defined and verified.
  • have to sign without injecting prefixes (ecsign, eth_sign). this is safe due to the structure of the hash and how we check it in the system

solutions:

  • ledger needs to support signTypedData or ethSign (to have full ux compatibility with metamask)
  • for a possible flow outside metamask, ledger needs to provide some kind of api that performs the unprefixed elliptic sign
  • we write our own ledger app that performs the signing (would not fit in the metamask flow) as mentioned above (which they may not even accept if they dont accept ethSign already)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
blocked Blocked on a dependency
Projects
None yet
Development

No branches or pull requests

4 participants