accounts/usbwallet: trezor signed message support #19831
Conversation
gballet
force-pushed
the
clef-signdata-with-trezor
branch
from
3e2094c to
9c9734e
Compare
accounts/usbwallet/ledger.go
Outdated
There was a problem hiding this comment.
transaction -> signing request.
accounts/usbwallet/trezor.go
Outdated
There was a problem hiding this comment.
transaction -> signing request.
accounts/usbwallet/wallet.go
Outdated
There was a problem hiding this comment.
transaction -> signing request.
accounts/usbwallet/wallet.go
Outdated
There was a problem hiding this comment.
I'm unsure about passing a hash vs. the data itself. Won't some hardware wallet or Clef itself need the entire data?
There was a problem hiding this comment.
actually the message itself is passed, I'm going to change the variable name.
signer/core/signed_data.go
Outdated
There was a problem hiding this comment.
This is a bit ugly :P. A cleaner solution perhaps would be to look at the account.URL and if it's a keystore, ask for password, for everything else just blindly forward?
There was a problem hiding this comment.
Quoting you (from memory) when you asked me to look into the Trezor stuff: "I would like a quick and ugly solution". Boom, delivered.
accounts/usbwallet/wallet.go
Outdated
There was a problem hiding this comment.
I guess this method should be renamed signData? Also please rename the hash variables everywhere to data.
There was a problem hiding this comment.
Please read the comment and update it too accordingly :P
gballet
force-pushed
the
clef-signdata-with-trezor
branch
from
7bcd24e to
407128b
Compare
accounts/usbwallet/wallet.go
Outdated
| w.deriveChain = chain | ||
| } | ||
|
|
||
| // signHash implements accounts.Wallet, however signing arbitrary data is not |
accounts/usbwallet/ledger.go
Outdated
|
|
||
| // SignData sends a blob of data to the USB device and waits for the user to confirm | ||
| // or deny the signing request. | ||
| func (w *ledgerDriver) SignData(path accounts.DerivationPath, hash []byte) (common.Address, []byte, error) { |
| if addr := response.GetAddressHex(); len(addr) > 0 { // Newer firmwares use hexadecimal fomats | ||
| address = common.HexToAddress(addr) | ||
| } | ||
|
|
|
|
||
| func (w *wallet) SignText(account accounts.Account, text []byte) ([]byte, error) { | ||
| return w.signHash(account, accounts.TextHash(text)) | ||
| return w.signData(account, accounts.TextHash(text)) |
There was a problem hiding this comment.
This should be wrong now. If we changed the signature to sign data, then we can't hash the text here now. What does sign text even do? It's not my method. Ping @holiman ?
There was a problem hiding this comment.
There's an extra catch here that I've spotted yesterday during a debug session: the Trezor actually adds the prefix no matter what, so right now the prefix is added twice. So the whole prefix management has to be changed, I'm still untangling some bits, and it would be interesting to discuss it.
signer/core/signed_data.go
Outdated
| return nil, err | ||
| // Only ask for the passphrase if the account is keystore based | ||
| pw := "" | ||
| if strings.Compare(wallet.URL().Scheme, "keystore") == 0 { |
There was a problem hiding this comment.
wallet.URL().Scheme == "keystore"
|
Putting this into the 1.9.x backlog. We need Trezor firmware support before we can continue (trezor/trezor-firmware#148). |
|
1 |
|
Closing this as it's not supported by Trezor. We can only pick it up if they implement it upstream. Closing until then. |
|
reference: trezor/trezor-firmware#148 |
5 participants
Implements
signDatafor trezor. This has been tested with both the pre-1.8 version of the firmware and the latest. Fixes #19812To work, this PR needs #19827