feat: trezor support #663
feat: trezor support #663
Conversation
There was a problem hiding this comment.
This is great.
can't speak to the maturity of the trezor client lib, but have some nits:
there are a quite a few unwraps, which may be fine but to be sure I'd rather see a basic helper function like split(&tx) -> Result<(nonce,gasprice,...)> that gets all values in a safe way.
Any plans for publishing the trezor on crates?
ethers-signers/Cargo.toml
Outdated
| @@ -28,6 +28,7 @@ yubihsm = { version = "0.39.0", features = ["secp256k1", "http", "usb"], optiona | |||
| futures-util = "0.3.18" | |||
| futures-executor = "0.3.18" | |||
| semver = "1.0.4" | |||
| trezor-client = { package="trezor", git = "https://github.com/joshiedo/rust-trezor-api", rev = "f0c9775", optional = true } | |||
There was a problem hiding this comment.
this would prevent us from publishing a new version of ethers,
what's the state of that crate, any chance you can publish this beforehand?
There was a problem hiding this comment.
done: trezor-client
cleaned it up (also on the trezor client lib side) |
Cargo.lock
Outdated
| [[package]] | ||
| name = "bitcoin" | ||
| version = "0.18.2" | ||
| source = "registry+https://github.com/rust-lang/crates.io-index" | ||
| checksum = "cd5e7935f613ba170459072926f01dc5ddb8aa22382dc4badf44bbb55e2d243d" | ||
| dependencies = [ | ||
| "bitcoin-bech32", | ||
| "bitcoin_hashes", | ||
| "byteorder", | ||
| "hex 0.3.2", | ||
| "rand 0.3.23", | ||
| "secp256k1", | ||
| ] |
There was a problem hiding this comment.
It seems like we're pulling in a few dependencies which should not be required? Maybe we can feature gate the Bitcion/Ethereum APIs in the trezor library?
Motivation
Ref: #1
Adding trezor signing support, with the end goal of adding it to foundry/cast.
Solution
Forked a year old rust client (https://github.com/romanz/rust-trezor-api -> https://github.com/joshieDo/rust-trezor-api) and added the ethereum interface it lacked, along with updating the libusb (+rusb) to be compatible with ethers-rs. I'm not much experienced in Rust, so an additional pair of eyes in my fork is most welcome.
On the
ethers-signersimplementation, I tried to follow the existing Ledger structure as much as possible.WIPFixing: Passphrase is being requested on every requestMissing EIP712
I need to understand EIP712 better from both sides, At first glance, it seems tricky to pass the intended data from
ethers-rsto thetrezor client. I was hoping that could be done in a future issue/PR. What do you think?PR Checklist