Skip to content

Commit

Permalink
feat: ledger reconnect (#6503)
Browse files Browse the repository at this point in the history
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
  • Loading branch information
brianp authored Aug 26, 2024
1 parent e9c420d commit 444b5a3
Show file tree
Hide file tree
Showing 3 changed files with 49 additions and 12 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down
6 changes: 6 additions & 0 deletions applications/minotari_ledger_wallet/comms/src/error.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down
53 changes: 42 additions & 11 deletions applications/minotari_ledger_wallet/comms/src/ledger_wallet.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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};
Expand All @@ -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<TransportNativeHID, LedgerDeviceError> {
let hid = hidapi()?;
let transport = TransportNativeHID::new(hid).map_err(|e| LedgerDeviceError::NativeTransport(e.to_string()))?;
Ok(transport)
struct HidManager {
inner: Option<HidApi>,
}

impl HidManager {
fn new() -> Result<Self, LedgerDeviceError> {
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<Result<HidApi, String>> =
Lazy::new(|| HidApi::new().map_err(|e| format!("Unable to get HIDAPI: {}", e)));
static HID_MANAGER: Lazy<Mutex<HidManager>> =
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<TransportNativeHID, LedgerDeviceError> {
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)]
Expand All @@ -60,7 +91,7 @@ impl<D: Deref<Target = [u8]>> Command<D> {
pub fn execute(&self) -> Result<APDUAnswer<Vec<u8>>, 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(
Expand All @@ -69,7 +100,7 @@ impl<D: Deref<Target = [u8]>> Command<D> {
) -> Result<APDUAnswer<Vec<u8>>, 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<u8>) -> Command<Vec<u8>> {
Expand Down

0 comments on commit 444b5a3

Please sign in to comment.