From 444b5a359e0e9efdafccfab8f49d4d9a8f48f3c1 Mon Sep 17 00:00:00 2001 From: Brian Pearce Date: Mon, 26 Aug 2024 09:42:55 +0200 Subject: [PATCH] feat: ledger reconnect (#6503) Description --- If the HidApi disconnects, the instance will still exist but can no longer be used. We need a new instance, but only a single one can exist at a time. So we're locking it in a mutex to ensure mutable safety. Then if at some point communication errors because the instance is stale we will drop the existing instance by re-assignment of the inner struct field, and create a new working instance. Motivation and Context --- Allow someone with an open console wallet to be able to connect and disconnect their ledger at will. Without the need to restart the wallet. If the connection to the ledger fails, it will re-establish the connection (only one attempt) and then proceed. How Has This Been Tested? --- Manually with the console wallet. Using the ledger demo test file it *mostly* works but there's a weird error return on the reconnection of the ledger before opening the Tari app that could also use some more debugging. What process can a PR reviewer use to test or verify this change? --- - Open a console wallet supported by ledger - Make a transaction successfully - Unplug the ledger - Make a failed transaction because the ledger is missing - Plug the ledger back in - Make a transaction successfully Breaking Changes --- - [x] None - [ ] Requires data directory on base node to be deleted - [ ] Requires hard fork - [ ] Other - Please specify --- .../comms/examples/ledger_demo/main.rs | 2 +- .../minotari_ledger_wallet/comms/src/error.rs | 6 +++ .../comms/src/ledger_wallet.rs | 53 +++++++++++++++---- 3 files changed, 49 insertions(+), 12 deletions(-) diff --git a/applications/minotari_ledger_wallet/comms/examples/ledger_demo/main.rs b/applications/minotari_ledger_wallet/comms/examples/ledger_demo/main.rs index b86add1477..ff18d14dba 100644 --- a/applications/minotari_ledger_wallet/comms/examples/ledger_demo/main.rs +++ b/applications/minotari_ledger_wallet/comms/examples/ledger_demo/main.rs @@ -355,7 +355,7 @@ fn main() { }, Err(e) => { if e != LedgerDeviceError::Processing( - "GetViewKey: Native HID transport error `Ledger device: Io error`".to_string(), + "GetViewKey: Native HID transport error `Ledger device not found`".to_string(), ) { println!("\nError: Unexpected response ({})\n", e); return; diff --git a/applications/minotari_ledger_wallet/comms/src/error.rs b/applications/minotari_ledger_wallet/comms/src/error.rs index 350be4584d..4a042f992f 100644 --- a/applications/minotari_ledger_wallet/comms/src/error.rs +++ b/applications/minotari_ledger_wallet/comms/src/error.rs @@ -30,9 +30,15 @@ pub enum LedgerDeviceError { /// HID API error #[error("HID API error `{0}`")] HidApi(String), + /// HID API error + #[error("HID API error refresh `{0}`")] + HidApiRefresh(String), /// Native HID transport error #[error("Native HID transport error `{0}`")] NativeTransport(String), + /// HID transport exchange error + #[error("Native HID transport exchange error `{0}`")] + NativeTransportExchange(String), /// Ledger application not started #[error("Ledger application not started")] ApplicationNotStarted, diff --git a/applications/minotari_ledger_wallet/comms/src/ledger_wallet.rs b/applications/minotari_ledger_wallet/comms/src/ledger_wallet.rs index a0ea053fe1..20c2e80806 100644 --- a/applications/minotari_ledger_wallet/comms/src/ledger_wallet.rs +++ b/applications/minotari_ledger_wallet/comms/src/ledger_wallet.rs @@ -20,7 +20,7 @@ // WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE // USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. -use std::ops::Deref; +use std::{ops::Deref, sync::Mutex}; use ledger_transport::{APDUAnswer, APDUCommand}; use ledger_transport_hid::{hidapi::HidApi, TransportNativeHID}; @@ -34,17 +34,48 @@ pub const EXPECTED_NAME: &str = "minotari_ledger_wallet"; pub const EXPECTED_VERSION: &str = env!("CARGO_PKG_VERSION"); const WALLET_CLA: u8 = 0x80; -pub fn get_transport() -> Result { - let hid = hidapi()?; - let transport = TransportNativeHID::new(hid).map_err(|e| LedgerDeviceError::NativeTransport(e.to_string()))?; - Ok(transport) +struct HidManager { + inner: Option, +} + +impl HidManager { + fn new() -> Result { + let hidapi = HidApi::new().map_err(|e| LedgerDeviceError::HidApi(e.to_string()))?; + Ok(Self { inner: Some(hidapi) }) + } + + fn refresh_if_needed(&mut self) -> Result<(), LedgerDeviceError> { + // We need to clear out the inner HidApi instance before creating a new one + // This is because only one instance may exist, even when it no longers holds a connection, + // and we want this dropped before replacing + self.inner = None; + + self.inner = Some(HidApi::new().map_err(|e| LedgerDeviceError::HidApiRefresh(e.to_string()))?); + + Ok(()) + } + + fn get_hidapi(&self) -> Option<&HidApi> { + self.inner.as_ref() + } } -fn hidapi() -> Result<&'static HidApi, LedgerDeviceError> { - static HIDAPI: Lazy> = - Lazy::new(|| HidApi::new().map_err(|e| format!("Unable to get HIDAPI: {}", e))); +static HID_MANAGER: Lazy> = + Lazy::new(|| Mutex::new(HidManager::new().expect("Failed to initialize HidManager"))); - HIDAPI.as_ref().map_err(|e| LedgerDeviceError::HidApi(e.to_string())) +pub fn get_transport() -> Result { + let mut manager = HID_MANAGER + .lock() + .map_err(|_| LedgerDeviceError::NativeTransport("Mutex lock error".to_string()))?; + + match TransportNativeHID::new(manager.get_hidapi().unwrap()) { + Ok(transport) => Ok(transport), + Err(_) => { + manager.refresh_if_needed()?; + TransportNativeHID::new(manager.get_hidapi().unwrap()) + .map_err(|e| LedgerDeviceError::NativeTransport(e.to_string())) + }, + } } #[derive(Debug, Clone)] @@ -60,7 +91,7 @@ impl> Command { pub fn execute(&self) -> Result>, LedgerDeviceError> { get_transport()? .exchange(&self.inner) - .map_err(|e| LedgerDeviceError::NativeTransport(e.to_string())) + .map_err(|e| LedgerDeviceError::NativeTransportExchange(e.to_string())) } pub fn execute_with_transport( @@ -69,7 +100,7 @@ impl> Command { ) -> Result>, LedgerDeviceError> { transport .exchange(&self.inner) - .map_err(|e| LedgerDeviceError::NativeTransport(e.to_string())) + .map_err(|e| LedgerDeviceError::NativeTransportExchange(e.to_string())) } pub fn build_command(account: u64, instruction: Instruction, data: Vec) -> Command> {