Feature/ledger signer #1944

OBorce · 2025-07-30T13:35:25Z

Ledger signer
Ledger tests

Tests assume the ledger app is already running in the emulator same as Trezor tests

ImplOfAnImpl

I haven't dug into the code much yet, will continue next week.

Tests assume the ledger app repo is cloned next to this one with name ledger-mintlayer

To be honest, I'm not a huge fan of this approach. And also of the fact that the emulator is always started automatically. E.g. in the Trezor case it was sometimes useful to see the emulator logs to understand what went wrong.
Was there any particular reason to do it this way instead of expecting the emuator to be running?

wallet/src/signer/ledger_signer/speculus/drivers/mod.rs

wallet/src/signer/ledger_signer/speculus/handle.rs

Cargo.toml

ImplOfAnImpl · 2025-08-28T16:46:53Z

Cargo.toml


 [profile.release]
-panic = "abort" # prevent panic catching (mostly for the tokio runtime)
+panic = "abort"        # prevent panic catching (mostly for the tokio runtime)


Weird indentation and it's inconsistent with the similar line above

I guess the formatter is looking at the next line if it had a comment it would require bigger indentation.

I guess the formatter is looking at the next line if it had a comment it would require bigger indentation.

Well, we don't have any "official" formatter for Cargo.toml files. So if any particualar formatter ends up uglifying the code, then I'd say the corresponding changes have to be reverted.

wallet/src/signer/ledger_signer/speculus/mod.rs

ImplOfAnImpl · 2025-10-06T08:28:13Z

wallet/src/signer/ledger_signer/speculos/handle.rs

+use reqwest::Client;
+use serde::{Deserialize, Serialize};
+use strum::Display;
+use tracing::debug;


Normally we use macros from our log crate (unless some tracing-specific features are needed), and we use them in a qualified form - log::debug!
Let's be consistent.

ImplOfAnImpl · 2025-10-06T08:34:25Z

wallet/src/signer/ledger_signer/speculos/handle.rs

+    /// Check button string encoding
+    #[test]
+    fn button_encoding() {
+        let tests = &[(Button::Left, "left"), (Button::Right, "right"), (Button::Both, "both")];


It'd be better if this test would fail if a new element was added to the enum. So I'd suggest deriving one of the strum traits (e.g. EnumIter or just EnumCount) and then either iterate over all enum variants and check each individually, or at least assert that the number of elements in tests is the same as the number of enum variants.

Same for the action_encoding test below.

ImplOfAnImpl · 2025-10-06T08:36:15Z

wallet/src/signer/ledger_signer/speculos/handle.rs

+/// Button action object for serialization and use with the HTTP API
+#[derive(Clone, Copy, PartialEq, Debug, Serialize, Deserialize)]
+struct ButtonAction {
+    pub action: Action,


Why is it pub when the whole struct is not?

ImplOfAnImpl · 2025-10-06T08:43:03Z

wallet/src/signer/ledger_signer/speculos/drivers/mod.rs

+//! Podman driver for speculos execution, runs a speculos instance within
+//! a Podman container.


There is nothing in this file that can be called a driver.

Also, a separate folder (drivers) seems to be redundant

ImplOfAnImpl · 2025-10-06T09:19:17Z

wallet/src/signer/ledger_signer/ledger_messages.rs

+#[derive(Clone, Copy)]
+pub enum LedgerAddrType {
+    PublicKey,
+    PublicKeyHash,
+}
+
+impl From<LedgerAddrType> for u8 {
+    fn from(addr_type: LedgerAddrType) -> u8 {
+        match addr_type {
+            LedgerAddrType::PublicKey => 0,
+            LedgerAddrType::PublicKeyHash => 1,
+        }
+    }
+}


This could be written as

#[derive(Clone, Copy, num_enum::IntoPrimitive)] #[repr(u8)] pub enum LedgerAddrType { PublicKey = 0, PublicKeyHash = 1, }

We don't use num_enum directly yet, but we do have it as an indirect dependency, so it's not entirely new.

ImplOfAnImpl · 2025-10-07T15:50:16Z

Cargo.toml

+[workspace.dependencies.ledger-lib]
+git = "https://github.com/ledger-community/rust-ledger.git"
+rev = "510bb3ca30639af4bdb12a918b6bbbdb75fa5f52"
+
+[workspace.dependencies.ledger-proto]
+git = "https://github.com/ledger-community/rust-ledger.git"
+rev = "510bb3ca30639af4bdb12a918b6bbbdb75fa5f52"
+


Is ledger-proto needed here? We don't seem to use it explicitly.

Plz add a note mentioning the other place below where the same revision is specified (i.e. the [patch.crates-io] one), so that when this one is updated, the other one is not forgotten about.

ImplOfAnImpl · 2025-10-07T15:53:29Z

Cargo.toml

 # is fontconfig-parser <- fontdb <- cosmic-text <- various "iced" crates.
 # TODO: investigate this further.
 fontconfig-parser = { git = "https://github.com/Riey/fontconfig-parser", rev = "f7d13a779e6ee282ce75acbc00a1270c0350e0c2" }
+# The patch is needed because there is now release of the library. We use the same hash for all Ledger libs


now -> no?

The whole comment sounds a bit off. Let's write something like:
"
This patch is needed because there is no release of the library and because ledger-lib depends on ledger-proto, so this is the only way to make the former find the latter.
Note that the revision specified here must be the same as the one used in the workspace.dependencies section.
"

ImplOfAnImpl · 2025-10-08T08:31:37Z

wallet/src/signer/ledger_signer/ledger_messages.rs

+
+use ledger_lib::Exchange;
+
+const MAX_MSG_SIZE: usize = (u8::MAX - 5) as usize; // 4 bytes for the header + 1 for len


MSG sounds vague, I guess it's "MAX_COMMAND_DATA_SIZE"

ImplOfAnImpl · 2025-10-08T08:49:10Z

wallet/src/signer/ledger_signer/ledger_messages.rs

+    let mut msg_buf = vec![];
+    msg_buf.extend([CLA, Ins::PUB_KEY, 0, P2::DONE]);
+    let encoded_path = address_n.encode();
+    let size: u8 = encoded_path.len().try_into().map_err(|_| LedgerError::PathToLong)?;
+    msg_buf.push(size + 1);
+    msg_buf.push(chain_type);
+    msg_buf.extend(encoded_path);


Here and in other places: plz introduce some types for the commands' data. E.g. here it could be

#[derive(Encode)] struct GetExtendedPublicKey { derivation_path: DerivationPath, chain_type: u8, }

Same for the response. And on the app side the same structs should be decoded/encoded as well.
(Also, since these structs are part of the protocol, they should be in one place. IMO that place should be in the ledger app repo.)

Plz also encapsulate the message creation in a function that accepts the ins, p1, p2 and a generic Data: Encode and returns the msg_buf.

ImplOfAnImpl · 2025-10-08T12:25:48Z

wallet/src/signer/ledger_signer/ledger_messages.rs

+pub async fn get_app_name<L: Exchange>(ledger: &mut L) -> Result<Vec<u8>, ledger_lib::Error> {
+    let msg_buf = [CLA, Ins::APP_NAME, 0, P2::DONE];
+    ledger.exchange(&msg_buf, Duration::from_millis(100)).await
+}
+
+#[allow(dead_code)]
+pub async fn check_current_app<L: Exchange>(ledger: &mut L) -> SignerResult<()> {
+    let resp = get_app_name(ledger)
+        .await
+        .map_err(|err| LedgerError::DeviceError(err.to_string()))?;


I don't think it's a good way to check for our app, because the INS values are app-specific.

There is a standard way to obtain the app name via CLA=0xB0 and INS=1. E.g. here it's handled by the SDK on the device - https://github.com/LedgerHQ/ledger-device-rust-sdk/blob/4262899a325b9b2fe10f2524d8e4b2f9fec38b83/ledger_device_sdk/src/io_legacy.rs#L330-L331
(The INS is processed inside the handle_bolos_apdu function).

Also, ledger-proto contains something called AppInfoReq which mentions CLA 0xB0 and INS 1, so I guess you don't have to construct the APDU by hand and parse the request.

ImplOfAnImpl · 2025-10-09T15:05:28Z

wallet/src/signer/ledger_signer/mod.rs

+        // Data is empty there is nothing to compare
+        return Ok(());


This should be an error, similar to Trezor's HardwareWalletDifferentMnemonicOrPassphrase

ImplOfAnImpl · 2025-10-09T15:06:37Z

wallet/src/signer/ledger_signer/mod.rs

+        return Ok(());
+    }
+
+    Err(LedgerError::HardwareWalletDifferentFile.into())


Btw, let's maybe rename HardwareWalletDifferentFile both for Ledger and Trezor to something else? Because the error means that a software wallet was found when a hw one was expected, but the error name is more generic than that.
Possible better names: HardwareWalletFileExpected, WalletFileIsSoftwareWallet (or something similar)

ImplOfAnImpl · 2025-10-09T15:38:40Z

wallet/src/signer/ledger_signer/tests.rs

+use wallet_storage::WalletStorageReadLocked;
+use wallet_types::hw_data::LedgerData;
+
+use async_trait::async_trait;


Plz put 3rd-party imports in a separate group below the std ones.
(i.e. this line, ledger_lib, rstest, tokio)

OBorce changed the base branch from master to refactor/wallet-async-signing July 30, 2025 13:35

OBorce force-pushed the feature/ledger_signer branch 2 times, most recently from ecf51c0 to abe8a88 Compare July 31, 2025 08:24

OBorce force-pushed the refactor/wallet-async-signing branch from 59bd742 to 9099e90 Compare August 22, 2025 10:57

OBorce force-pushed the feature/ledger_signer branch from abe8a88 to 89285d1 Compare August 22, 2025 10:58

OBorce marked this pull request as ready for review August 25, 2025 12:43

OBorce requested review from ImplOfAnImpl and erubboli as code owners August 25, 2025 12:43

OBorce force-pushed the feature/ledger_signer branch from 5f20a4f to 4c64407 Compare August 25, 2025 12:56

ImplOfAnImpl reviewed Aug 28, 2025

View reviewed changes

OBorce force-pushed the refactor/wallet-async-signing branch 3 times, most recently from f21e82b to 75ff652 Compare September 18, 2025 22:31

OBorce force-pushed the feature/ledger_signer branch 4 times, most recently from 61292c5 to eeb6484 Compare September 23, 2025 22:28

OBorce marked this pull request as draft September 25, 2025 08:51

OBorce force-pushed the refactor/wallet-async-signing branch from 75ff652 to 96016cf Compare September 25, 2025 08:57

OBorce added 3 commits September 29, 2025 22:18

Add ledger signer

8508014

fix ledger tests

23e2503

ledger cleanup

6ab3a14

OBorce force-pushed the feature/ledger_signer branch 2 times, most recently from 0a69bc5 to 5d3edf0 Compare September 30, 2025 23:21

Remove podman usage from Ledger tests, fix comments

d6fd484

OBorce force-pushed the feature/ledger_signer branch from 5d3edf0 to d6fd484 Compare September 30, 2025 23:37

OBorce marked this pull request as ready for review October 1, 2025 07:09

OBorce force-pushed the feature/ledger_signer branch from 39d3e58 to d6fd484 Compare October 3, 2025 07:07

ImplOfAnImpl requested changes Oct 8, 2025

View reviewed changes

ImplOfAnImpl reviewed Oct 9, 2025

View reviewed changes

		//! Podman driver for speculos execution, runs a speculos instance within
		//! a Podman container.


		use ledger_lib::Exchange;

		const MAX_MSG_SIZE: usize = (u8::MAX - 5) as usize; // 4 bytes for the header + 1 for len

Feature/ledger signer #1944

Are you sure you want to change the base?

Feature/ledger signer #1944

Uh oh!

Conversation

OBorce commented Jul 30, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ImplOfAnImpl left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

OBorce commented Jul 30, 2025 •

edited

Loading