Skip to content

Add Ledger signer provider #1946

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Open
wants to merge 1 commit into
base: feature/ledger_signer
Choose a base branch
from
Open

Add Ledger signer provider #1946

wants to merge 1 commit into from

Conversation

OBorce
Copy link
Contributor

@OBorce OBorce commented Jul 31, 2025

No description provided.

@OBorce OBorce force-pushed the feature/ledger_signer branch from abe8a88 to 89285d1 Compare August 22, 2025 10:58
@OBorce OBorce force-pushed the feature/ledger_signer_provider branch 2 times, most recently from 10d2120 to a8231af Compare August 25, 2025 07:11
@OBorce OBorce marked this pull request as ready for review August 25, 2025 13:30
@OBorce OBorce force-pushed the feature/ledger_signer branch from 4c64407 to 10bd693 Compare September 18, 2025 22:31
@OBorce OBorce force-pushed the feature/ledger_signer_provider branch 2 times, most recently from 5b2cae2 to 2de6ff8 Compare September 19, 2025 07:36
@OBorce OBorce force-pushed the feature/ledger_signer branch from ab50373 to b304ef1 Compare September 19, 2025 08:48
@OBorce OBorce force-pushed the feature/ledger_signer_provider branch 2 times, most recently from d4a8f4d to 32a8870 Compare September 19, 2025 09:57
@OBorce OBorce force-pushed the feature/ledger_signer branch from 61292c5 to eeb6484 Compare September 23, 2025 22:28
@OBorce OBorce marked this pull request as draft September 25, 2025 08:50
@OBorce OBorce force-pushed the feature/ledger_signer branch 3 times, most recently from 5d3edf0 to d6fd484 Compare September 30, 2025 23:37
@OBorce OBorce closed this Oct 3, 2025
@OBorce OBorce force-pushed the feature/ledger_signer_provider branch from 32a8870 to 39d3e58 Compare October 3, 2025 07:06
@OBorce OBorce reopened this Oct 3, 2025
@OBorce OBorce marked this pull request as ready for review October 6, 2025 07:59
ImplOfAnImpl
ImplOfAnImpl reviewed Oct 9, 2025
View reviewed changes
Copy link
Contributor

@ImplOfAnImpl ImplOfAnImpl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd like to look at this code one more time later. But perhaps it's better to merge it to the parent PR first, to review all the changes together.

node-gui/src/main_window/main_widget/tabs/wallet/status_bar.rs
Comment on lines +40 to +42
TEXT_SIZE + 2. * VERTICAL_PADDING
// Same as Trezor
+ 4.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Plz don't duplicate the value.

node-gui/src/main_window/main_widget/tabs/wallet/status_bar.rs
}
#[cfg(feature = "ledger")]
WalletExtraInfo::LedgerWallet { firmware_version } => {
use iced::widget::{rich_text, span};
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Move this out of the function? (for the trezor case too).

node-gui/src/main_window/main_widget/tabs/wallet/status_bar.rs
Comment on lines +79 to +83
row![rich_text([
span("Firmware version: ").font(bold_font),
span(firmware_version.clone())
])
.size(TEXT_SIZE),]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMO we should be able to obtain the device name for the ledger too (which will be the model name, not the label set in Ledger Live, but it's still better than nothing).

As I've mentioned in the ledger app PR, it seems like the APDU 0xE0/0x01 is "get device info" (need to check if it works when an app is opened though). One of the returned values is targetId, from which the model name can be deduced.

Ledger Live sometimes calls this endpoint - https://manager.api.live.ledger.com/api/devices - to get the device model name (by comparing obtained the device's targetId with target_id inside device_versions of the returned json.)
(I'm not suggesting to use their endpoint, but we might just take its current values and hardcode them).

And it also has a hardcoded array of device infos and this function obtains them by targetId - https://github.com/LedgerHQ/ledger-live/blob/2dddf3a308ec6bd9fa436c7bc5bc02bcb33593e6/libs/ledgerjs/packages/devices/src/index.ts#L176-L182

wallet/types/src/hw_data.rs

#[cfg(feature = "ledger")]
#[derive(Debug, Clone, PartialEq, Eq, Encode, Decode)]
pub struct LedgerDataFullInfo {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LedgerFullInfo

P.S. plz also add doc comments to LedgerData and LedgerFullInfo, similar to those for TrezorData and TrezorFullInfo. Or, if you think they're redundant, then remove them for Trezor too, for consistency.

wallet/src/signer/ledger_signer/ledger_messages.rs
Comment on lines +225 to +234
let firmware_version = match ver.as_slice() {
[major, minor, patch] => common::primitives::semver::SemVer {
major: *major,
minor: *minor,
patch: *patch as u16,
},
_ => return Err(SignerError::LedgerError(LedgerError::InvalidResponse)),
};

Ok(LedgerDataFullInfo { firmware_version })
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's a mintlayer app version, not firmware version

wallet/src/signer/ledger_signer/mod.rs
#[derive(Clone)]
pub struct LedgerSignerProvider {
client: Arc<Mutex<LedgerHandle>>,
data: LedgerDataFullInfo,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since LedgerDataFullInfo should be renamed to LedgerFullInfo, this is "info", not "data".

wallet/src/wallet/mod.rs

if let Some(info) = signer_provider.get_hardware_wallet_info() {
db_tx.set_hardware_wallet_data(info.into())?;
if let Some(data) = signer_provider.get_hardware_wallet_info() {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well it's actually "info", not "data".

wallet/wallet-cli-commands/src/command_handler/mod.rs
)
}
WalletExtraInfo::LedgerWallet { firmware_version } => format!(
"This is a ledger wallet' firmware version {}",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Broken text.

Also, currently this is not really a firmware version.

wallet/wallet-controller/src/sync/mod.rs
Comment on lines +18 to 19
use async_trait::async_trait;
use common::{
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Plz put 3rd-party imports in a separate group

wallet/wallet-controller/src/types/mod.rs
},
#[cfg(feature = "ledger")]
LedgerWallet {
firmware_version: String,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Again, this is not the firmware version, it's the version of the ledger app.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@ImplOfAnImpl ImplOfAnImpl ImplOfAnImpl left review comments

@erubboli erubboli Awaiting requested review from erubboli erubboli is a code owner

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@OBorce @ImplOfAnImpl