Web3 Provider eth_sign trezor

[dekz]

#5021 is similar but doesn't seem to make sense. Why would eth_sign not call trezor ethereumSignMessage

Describe the bug
JSON RPC method eth_sign is not handled by the trezor feature of metamask. Metamask pops up with a Sign button but nothing is hooked up.

To Reproduce
Steps to reproduce the behaviour:

web3.currentProvider.sendAsync({method: 'eth_sign', id: 1, params: [ web3.eth.coinbase, '0xdeadbeef']}, console.log);

Expected behavior

Send the request to the trezor device

Screenshots
If applicable, add screenshots to help explain your problem.

Browser details (please complete the following information):

• OS: [e.g. iOS]
• Browser [e.g. chrome, safari]
• MetaMask Version [e.g. 4.9.0]
• Old UI or New / Beta UI?

Additional context
Add any other context about the problem here.

[brunobar79]

Trezor method ethereumSignMessage is an implementation of personal_sign (not eth_sign)
Also, eth_sign has been replaced by personal_sign for security reasons.

Read the developer note at the beginning of this article

I'll keep this issue open since you should be seeing an error message (see this) but it looks like that's not happening. I'll take a look early next week.

[dekz]

I'm not quite sure I'm understanding what you're referencing. eth_sign is the JSON RPC defined implementation here.

From my understanding the trezor implementation of ethereumSignMessage is in line with the definition of eth_sign JSON RPC. Historically Metamask has implemented eth_sign incorrectly and does not prefix the message as expected.

Why is ethereumSignMessage an implementation of personal_sign (a web3 function, not in the JSON RPC definition) versus an implementation of eth_sign. You have an issue which discusses replacing personal_sign with eth_sign as per spec: #2215. It appears that this issue is almost close to being merged after EIP712 support, so trezor support should land under eth_sign no?

[brunobar79]

Right now with Metamask and Trezor you can only use personal_sign. For example: web3.currentProvider.sendAsync({method: 'personal_sign', id: 1, params: [ web3.eth.coinbase, '0xdeadbeef']}, console.log);

RE: From my understanding the trezor implementation of ethereumSignMessage is in line with the definition of eth_sign JSON RPC . I don't think that's correct. Like I said before, Trezor implementation of ethereumSignMessage is inline with personal_sign.

Regarding EIP712, Trezor needs to add support for eth_signTypedData on their end first in order for us to enable it on the Integration. See trezor/trezor-core#122

[dekz]

I don't think that's correct

Can you expand on this?

Right now it is not possible to digest some data and sign the message with MM Trezor. Personal_sign uses utf8 encoding in which not all byte sequences can be represented, so you cannot simply convert the hex into ut8 and back without loss of data.

Please refer here to where trezor now supports hex signing (which is inline with eth_sign) trezor/connect#216.

From our perspective it looks like MM should support the following:

Trezor + eth_sign - Spec compliant implementation

TrezorConnect.ethereumSignMessage({
    path: "m/44'/60'/0'",
    message: "0xdeadbeef",
    hex: true
}); 

Trezor + personal_sign

TrezorConnect.ethereumSignMessage({
    path: "m/44'/60'/0'",
    message: "dead beef",
    hex: false
}); 

[brunobar79]

@dekz Good to know that Trezor made that change. We can get support for this right after we get the TrezorConnect v5 PR merged (MetaMask/eth-trezor-keyring#11)

[sidsverma]

Hey @brunobar79, have you managed to add the hex support?
If not, then how should I do it specifically for Trezor? Should I call the TrezorConnect.ethereumSignMessage with the hex: true directly?
I am currently using web3@0.20.3 and invoking the personal_sign via:
window.web3.currentProvider.sendAsync( { method: "personal_sign", params: ["0x187F21A4b04DBa4552e900BDfA55D8F7eb59e554", walletAddress], from: walletAddress }, async (err, result) => {})

[sidsverma]

Also, does the same issue exist in Ledger too?

[sidsverma]

On trezor, this is a very straight forward fix. If the message is hex, then just add a parameter:
hex: true
I have tested it out with trezor-connect's native code and it works. Do you think this will be resolved anytime soon? Should I create a pr for it?

[brunobar79]

@sidsverma Feel free to submit a PR enabling signMessage in eth-trezor-keyring which should be identical to signPersonalMessage with hex: true and without the ascii part and I’ll take a look. Adding a repro example would be great too.

Regarding ledger, I don’t think it’s supported, but you can confirm by taking a look here https://github.com/LedgerHQ/ledgerjs

[sidsverma]

Hi @brunobar79, I have created a PR for the same: MetaMask/eth-trezor-keyring#15

I do not understand how I can test this though. I have tested the individual parts of the code that I have added in my project using TrezorConnect, but I have no way to test it via Metamask. Do you have any suggestions for me here? Also, by when do you think this will be live on Metamask so that I can give a timeline to the users in my dapp(Nuo)?

[sidsverma]

Also, is there any way to determine if the account is Trezor or Ledger once the user has signed one transaction via Metamask from his Trezor in one dapp?