Trezor support for Solana CLI

[stevenbooke]

Problem

Trezor wallet is not supported.
#4911

Summary of Changes

Add support for the cli to use the wallet.

[CriesofCarrots]

Thanks for moving this over! CI is complaining about some trailing whitespace, and it looks like you might need to run cargo fmt as well.

[stevenbooke]

Thanks for moving this over! CI is complaining about some trailing whitespace, and it looks like you might need to run cargo fmt as well.

My pleasure! Ok, I will work on that

[stevenbooke]

@CriesofCarrots Any other feedback?

[CriesofCarrots]

Can you please run ./scripts/cargo-for-all-lock-files.sh tree and commit the result to get further in CI?

[stevenbooke]

Can you please run ./scripts/cargo-for-all-lock-files.sh tree and commit the result to get further in CI?

Yes

[stevenbooke]

@CriesofCarrots CI is giving the below error:

error: Cargo.toml for solana is not formatted
--
  | 🚨 Error: The command exited with status 1

I have run cargo +nightly-2024-01-05 fmt and no formatting changes are made. Where should I go from here?

[CriesofCarrots]

error: Cargo.toml for solana is not formatted

I have run cargo +nightly-2024-01-05 fmt and no formatting changes are made. Where should I go from here?

It's actually cargo sort that's throwing that error. (cargo fmt is remarkably un-opinionated about toml files.) Sort is complaining about the lack of a space before the closing bracket in Cargo.toml L432

However, if you want to wait to push a fix, I'm working on reviewing the implementation details right now and will have some feedback.

[CriesofCarrots]

Thanks for taking a crack at this! Here's my first pass. I'm particularly interested in supporting multiple trezor wallets at once, and making wallet behavior consistent with what's implemented for ledger wallets.

[CriesofCarrots]

Suggested change

	[Ledger hardware wallet](https://www.ledger.com/) and [Trezor hardware wallet](https://trezor.io/), offers a great blend of
	[Ledger hardware wallet](https://www.ledger.com/) or [Trezor hardware wallet](https://trezor.io/), offers a great blend of

[stevenbooke]

I will make this change

[CriesofCarrots]

Add default-features = "false"? Looks like bitcoin and ethereum are on by default. Are they being used for anything?

[stevenbooke]

It is my understanding that by specifying features = ["solana"] we are explicitly telling Cargo to only include the "solana" feature for the trezor-client dependency when building, so we wouldn't need default-features = "false".

[CriesofCarrots]

In the ledger version of this method, this str is used to disambiguate device selection for different keypairs in a transaction. How does trezor-client handle multiple trezor wallets being connected? (I only have one to test with)

[stevenbooke]

In the ledger version of this method, this str is used to disambiguate device selection for different keypairs in a transaction. How does trezor-client handle multiple trezor wallets being connected? (I only have one to test with)

https://github.com/trezor/trezor-firmware/blob/main/rust/trezor-client/src/lib.rs#L3

[CriesofCarrots]

This is unfortunate and feels brittle. Perhaps we could add a conversion method to DerivationPath, like DerivationPath::iter_u32() or similar, and avoid using the bip32 crate altogether?

[stevenbooke]

I will make this change

[CriesofCarrots]

Suggested change

	let mut solana_sign_tx = SolanaSignTx::new();
	let address_n = convert_path(
	&bip32::DerivationPath::from_str(derivation_path_str)
	.expect("Hardended Derivation Path"),
	);
	solana_sign_tx.address_n = address_n;
	solana_sign_tx.serialized_tx = Some(data.to_vec());
	let address_n = convert_path(
	&bip32::DerivationPath::from_str(derivation_path_str)
	.expect("Hardended Derivation Path"),
	);
	let solana_sign_tx = SolanaSignTx {
	address_n,
	serialized_tx: Some(data.to_vec()),
	.. SolanaSignTx::default()
	};

Can avoid the mut, and I think it makes it a little more obvious how this struct is being initialized.

Ideally also remove the expect, although if we go the direction I suggested above, conversion should be infallible.

[CriesofCarrots]

While I'm not sure how often a device would fail to return features, let's handle this gracefully with an Error instead of panicking.
Same comment for the next 2 methods.

[CriesofCarrots]

Suggested change

	match &self.trezor_client {
	Some(trezor_client) => {
	let features = trezor_client
	.borrow_mut()
	.features()
	.expect("no features")
	.clone();
	Ok(FirmwareVersion::new(
	features.major_version().into(),
	features.minor_version().into(),
	features.patch_version().into(),
	))
	}
	_ => Err(RemoteWalletError::NoDeviceFound),
	}
	let trezor_client = self
	.trezor_client
	.as_ref()
	.ok_or(RemoteWalletError::NoDeviceFound)?
	.borrow();
	let features = trezor_client.features().expect("no features");
	Ok(FirmwareVersion::new(
	features.major_version().into(),
	features.minor_version().into(),
	features.patch_version().into(),
	))

I like something like this to avoid cloning the whole feature object.
Please double-check me: is there a reason we should retain borrow_mut() here?

Same suggestion for the next 2 methods.

[CriesofCarrots]

How about converting to a Pubkey instead? Then we can remove this additional use of tje bitcoin crate.

[CriesofCarrots]

I think we probably want to support multiple trezor devices, don't we? I imagine users may want to use more than one for a transaction: as an account authority vs fee-payer, for instance. Certainly we see that use-case with Ledgers commonly.

It looks like trezor-client should make this relatively straightforward. Then I think it would probably make sense to roll trezor device recognition into update_devices()

[stevenbooke]

Ok, this can be done. I'm going to link some references for the sake of auditing.

https://github.com/trezor/trezor-firmware/blob/main/rust/trezor-client/src/lib.rs#L100

https://github.com/trezor/trezor-firmware/blob/main/rust/trezor-client/src/transport/webusb.rs#L81

https://github.com/trezor/trezor-firmware/blob/main/rust/trezor-client/src/transport/udp.rs#L99

[CriesofCarrots]

This may be helpful if/when we support multiple trezor wallets.

[t-nelson]

i smell a refactor

[t-nelson]

can you use the existing bip32 and base58 dependencies instead of introducing a new one?

[stevenbooke]

Yes. I did not see the already existing ed25519-dalek-bip32 dependency, thanks for pointing that out.

[t-nelson]

why?

[t-nelson]

can we not populate this based on the contents of the hid device list? better to not need implementation details directly exposed to the api

[stevenbooke]

I don't think we can. Trezor uses Protobuf v2 for host-device communication and as a consequence the trezor-client crate has a WebUsbTransport transport implementation that utilizes the rusb crate for USB communication with Trezor devices, and a UdpTransport implementation that uses standard Rust networking libraries for communication over UDP. It seems to me if we wanted to use the hidapi crate it would involve integrating the hidapi crate into the trezor-client transport module which could require significant changes to the codebase depending on how tightly coupled the existing transport implementation is with the rest of the crate.

[t-nelson]

ideally we don't introduce implementation specific interfaces like this. get_ledger() needs deprecated

[McSim85]

Adding reference PR: solana-labs#35433

I can't wait for Trezor's support 🫶

[joncinque]

This PR contains changes to the solana sdk, which will be moved to a new repo within the next week, when v2.2 is branched from master.

Please merge or close this PR as soon as possible, or re-create the sdk changes when the new repository is ready at https://github.com/anza-xyz/solana-sdk

[github-actions]

This pull request is stale because it has been open 60 days with no activity. Remove stale label or comment or this will be closed in 7 days.

[steviez]

It looks like we did eventually get this support added with #8378 so I'm going to close this issue