Fix ledger nano downcasting and add test (#986)

* Fix ledger nano downcasting and add test * changelogs * fix test * forgot channels exist * Improve event handlers and remove remainder assertion
iotaledger · Aug 2, 2023 · d8cbb77 · d8cbb77
1 parent b0c0c38
commit d8cbb77
Show file tree

Hide file tree

Showing 12 changed files with 231 additions and 124 deletions.
diff --git a/.changes/ledger-nano-events.md b/.changes/ledger-nano-events.md
@@ -0,0 +1,5 @@
+---
+"wallet-nodejs-binding": patch
+---
+
+Ledger Nano events properly created when preparing transactions;
diff --git a/bindings/nodejs/CHANGELOG.md b/bindings/nodejs/CHANGELOG.md
@@ -24,6 +24,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 ### Fixed
 
 - Missing production profile when no prebuild binary is available;
+- Ledger Nano events properly created when preparing transactions;
 
 ## 1.0.3 - 2023-07-31
 

diff --git a/bindings/python/CHANGELOG.md b/bindings/python/CHANGELOG.md
@@ -19,6 +19,12 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 
 ### Security -->
 
+## 1.0.1 - 2023-MM-DD
+
+### Fixed
+
+- Ledger Nano events properly created when preparing transactions;
+
 ## 1.0.0 - 2023-07-24
 
 ### Added

diff --git a/sdk/CHANGELOG.md b/sdk/CHANGELOG.md
@@ -24,6 +24,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 ### Fixed
 
 - `Clients` returning the default protocol parameters when multiple `Client` instances are used;
+- Ledger Nano events properly created when preparing transactions using a `SecretManager`;
 
 ## 1.0.2 - 2023-07-28
 

diff --git a/sdk/src/wallet/account/operations/address_generation.rs b/sdk/src/wallet/account/operations/address_generation.rs
@@ -69,28 +69,56 @@ where
         // needs to have it visible on the computer first, so we need to generate it without the
         // prompt first
         #[cfg(feature = "ledger_nano")]
-        let addresses = if options.ledger_nano_prompt
-            && self
-                .wallet
-                .secret_manager
-                .read()
-                .await
+        let addresses = {
+            use crate::wallet::account::SecretManager;
+            let secret_manager = self.wallet.secret_manager.read().await;
+            if secret_manager
                 .downcast::<LedgerSecretManager>()
+                .or_else(|| {
+                    secret_manager.downcast::<SecretManager>().and_then(|s| {
+                        if let SecretManager::LedgerNano(n) = s {
+                            Some(n)
+                        } else {
+                            None
+                        }
+                    })
+                })
                 .is_some()
-        {
-            #[cfg(feature = "events")]
-            let changed_options = {
-                // Change options so ledger will not show the prompt the first time
-                let mut changed_options = options;
-                changed_options.ledger_nano_prompt = false;
-                changed_options
-            };
-            let mut addresses = Vec::new();
-
-            for address_index in address_range {
+            {
                 #[cfg(feature = "events")]
-                {
-                    // Generate without prompt to be able to display it
+                let changed_options = {
+                    // Change options so ledger will not show the prompt the first time
+                    let mut changed_options = options;
+                    changed_options.ledger_nano_prompt = false;
+                    changed_options
+                };
+                let mut addresses = Vec::new();
+
+                for address_index in address_range {
+                    #[cfg(feature = "events")]
+                    {
+                        // Generate without prompt to be able to display it
+                        let address = self
+                            .wallet
+                            .secret_manager
+                            .read()
+                            .await
+                            .generate_ed25519_addresses(
+                                account_details.coin_type,
+                                account_details.index,
+                                address_index..address_index + 1,
+                                Some(changed_options),
+                            )
+                            .await?;
+                        self.emit(
+                            account_details.index,
+                            WalletEvent::LedgerAddressGeneration(AddressData {
+                                address: address[0].to_bech32(bech32_hrp),
+                            }),
+                        )
+                        .await;
+                    }
+                    // Generate with prompt so the user can verify
                     let address = self
                         .wallet
                         .secret_manager
@@ -100,45 +128,25 @@ where
                             account_details.coin_type,
                             account_details.index,
                             address_index..address_index + 1,
-                            Some(changed_options),
+                            Some(options),
                         )
                         .await?;
-                    self.emit(
-                        account_details.index,
-                        WalletEvent::LedgerAddressGeneration(AddressData {
-                            address: address[0].to_bech32(bech32_hrp),
-                        }),
-                    )
-                    .await;
+                    addresses.push(address[0]);
                 }
-                // Generate with prompt so the user can verify
-                let address = self
-                    .wallet
+                addresses
+            } else {
+                self.wallet
                     .secret_manager
                     .read()
                     .await
                     .generate_ed25519_addresses(
                         account_details.coin_type,
                         account_details.index,
-                        address_index..address_index + 1,
+                        address_range,
                         Some(options),
                     )
-                    .await?;
-                addresses.push(address[0]);
+                    .await?
             }
-            addresses
-        } else {
-            self.wallet
-                .secret_manager
-                .read()
-                .await
-                .generate_ed25519_addresses(
-                    account_details.coin_type,
-                    account_details.index,
-                    address_range,
-                    Some(options),
-                )
-                .await?
         };
 
         #[cfg(not(feature = "ledger_nano"))]

diff --git a/sdk/src/wallet/account/operations/output_consolidation.rs b/sdk/src/wallet/account/operations/output_consolidation.rs
@@ -153,17 +153,26 @@ where
             Some(t) => t,
             None => {
                 #[cfg(feature = "ledger_nano")]
-                if self
-                    .wallet
-                    .secret_manager
-                    .read()
-                    .await
-                    .downcast::<LedgerSecretManager>()
-                    .is_some()
                 {
-                    DEFAULT_LEDGER_OUTPUT_CONSOLIDATION_THRESHOLD
-                } else {
-                    DEFAULT_OUTPUT_CONSOLIDATION_THRESHOLD
+                    use crate::wallet::account::SecretManager;
+                    let secret_manager = self.wallet.secret_manager.read().await;
+                    if secret_manager
+                        .downcast::<LedgerSecretManager>()
+                        .or_else(|| {
+                            secret_manager.downcast::<SecretManager>().and_then(|s| {
+                                if let SecretManager::LedgerNano(n) = s {
+                                    Some(n)
+                                } else {
+                                    None
+                                }
+                            })
+                        })
+                        .is_some()
+                    {
+                        DEFAULT_LEDGER_OUTPUT_CONSOLIDATION_THRESHOLD
+                    } else {
+                        DEFAULT_OUTPUT_CONSOLIDATION_THRESHOLD
+                    }
                 }
                 #[cfg(not(feature = "ledger_nano"))]
                 DEFAULT_OUTPUT_CONSOLIDATION_THRESHOLD
@@ -184,31 +193,37 @@ where
         }
 
         #[cfg(feature = "ledger_nano")]
-        let max_inputs = if let Some(ledger) = self
-            .wallet
-            .secret_manager
-            .read()
-            .await
-            .downcast::<LedgerSecretManager>()
-        {
-            let ledger_nano_status = ledger.get_ledger_nano_status().await;
-            // With blind signing we are only limited by the protocol
-            if ledger_nano_status.blind_signing_enabled() {
-                INPUT_COUNT_MAX
+        let max_inputs = {
+            use crate::wallet::account::SecretManager;
+            let secret_manager = self.wallet.secret_manager.read().await;
+            if let Some(ledger) = secret_manager.downcast::<LedgerSecretManager>().or_else(|| {
+                secret_manager.downcast::<SecretManager>().and_then(|s| {
+                    if let SecretManager::LedgerNano(n) = s {
+                        Some(n)
+                    } else {
+                        None
+                    }
+                })
+            }) {
+                let ledger_nano_status = ledger.get_ledger_nano_status().await;
+                // With blind signing we are only limited by the protocol
+                if ledger_nano_status.blind_signing_enabled() {
+                    INPUT_COUNT_MAX
+                } else {
+                    ledger_nano_status
+                        .buffer_size()
+                        .map(|buffer_size| {
+                            // Calculate how many inputs we can have with this ledger, buffer size is different for
+                            // different ledger types
+                            let available_buffer_size_for_inputs =
+                                buffer_size - ESSENCE_SIZE_WITHOUT_IN_AND_OUTPUTS - MIN_OUTPUT_SIZE_IN_ESSENCE;
+                            (available_buffer_size_for_inputs / INPUT_SIZE) as u16
+                        })
+                        .unwrap_or(INPUT_COUNT_MAX)
+                }
             } else {
-                ledger_nano_status
-                    .buffer_size()
-                    .map(|buffer_size| {
-                        // Calculate how many inputs we can have with this ledger, buffer size is different for
-                        // different ledger types
-                        let available_buffer_size_for_inputs =
-                            buffer_size - ESSENCE_SIZE_WITHOUT_IN_AND_OUTPUTS - MIN_OUTPUT_SIZE_IN_ESSENCE;
-                        (available_buffer_size_for_inputs / INPUT_SIZE) as u16
-                    })
-                    .unwrap_or(INPUT_COUNT_MAX)
+                INPUT_COUNT_MAX
             }
-        } else {
-            INPUT_COUNT_MAX
         };
         #[cfg(not(feature = "ledger_nano"))]
         let max_inputs = INPUT_COUNT_MAX;

diff --git a/sdk/src/wallet/account/operations/transaction/sign_transaction.rs b/sdk/src/wallet/account/operations/transaction/sign_transaction.rs
@@ -39,31 +39,37 @@ where
         .await;
 
         #[cfg(all(feature = "events", feature = "ledger_nano"))]
-        if let Some(ledger) = self
-            .wallet
-            .secret_manager
-            .read()
-            .await
-            .downcast::<LedgerSecretManager>()
         {
-            let ledger_nano_status = ledger.get_ledger_nano_status().await;
-            if let Some(buffer_size) = ledger_nano_status.buffer_size() {
-                if needs_blind_signing(prepared_transaction_data, buffer_size) {
-                    self.emit(
-                        self.details().await.index,
-                        WalletEvent::TransactionProgress(TransactionProgressEvent::PreparedTransactionEssenceHash(
-                            prefix_hex::encode(prepared_transaction_data.essence.hash()),
-                        )),
-                    )
-                    .await;
-                } else {
-                    self.emit(
-                        self.details().await.index,
-                        WalletEvent::TransactionProgress(TransactionProgressEvent::PreparedTransaction(Box::new(
-                            PreparedTransactionDataDto::from(prepared_transaction_data),
-                        ))),
-                    )
-                    .await;
+            use crate::wallet::account::SecretManager;
+            let secret_manager = self.wallet.secret_manager.read().await;
+            if let Some(ledger) = secret_manager.downcast::<LedgerSecretManager>().or_else(|| {
+                secret_manager.downcast::<SecretManager>().and_then(|s| {
+                    if let SecretManager::LedgerNano(n) = s {
+                        Some(n)
+                    } else {
+                        None
+                    }
+                })
+            }) {
+                let ledger_nano_status = ledger.get_ledger_nano_status().await;
+                if let Some(buffer_size) = ledger_nano_status.buffer_size() {
+                    if needs_blind_signing(prepared_transaction_data, buffer_size) {
+                        self.emit(
+                            self.details().await.index,
+                            WalletEvent::TransactionProgress(TransactionProgressEvent::PreparedTransactionEssenceHash(
+                                prefix_hex::encode(prepared_transaction_data.essence.hash()),
+                            )),
+                        )
+                        .await;
+                    } else {
+                        self.emit(
+                            self.details().await.index,
+                            WalletEvent::TransactionProgress(TransactionProgressEvent::PreparedTransaction(Box::new(
+                                PreparedTransactionDataDto::from(prepared_transaction_data),
+                            ))),
+                        )
+                        .await;
+                    }
                 }
             }
         }

diff --git a/sdk/src/wallet/core/mod.rs b/sdk/src/wallet/core/mod.rs
@@ -178,7 +178,7 @@ impl<S: SecretManage> WalletInner<S> {
     pub async fn listen<F, I: IntoIterator<Item = WalletEventType> + Send>(&self, events: I, handler: F)
     where
         I::IntoIter: Send,
-        F: Fn(&Event) + 'static + Clone + Send + Sync,
+        F: Fn(&Event) + 'static + Send + Sync,
     {
         let mut emitter = self.event_emitter.write().await;
         emitter.on(events, handler);

diff --git a/sdk/src/wallet/events/mod.rs b/sdk/src/wallet/events/mod.rs
@@ -3,14 +3,15 @@
 
 pub mod types;
 
+use alloc::sync::Arc;
 use std::{
     collections::HashMap,
     fmt::{Debug, Formatter, Result},
 };
 
 pub use self::types::{Event, WalletEvent, WalletEventType};
 
-type Handler<T> = Box<dyn Fn(&T) + Send + Sync + 'static>;
+type Handler<T> = Arc<dyn Fn(&T) + Send + Sync + 'static>;
 
 pub struct EventEmitter {
     handlers: HashMap<WalletEventType, Vec<Handler<Event>>>,
@@ -28,9 +29,10 @@ impl EventEmitter {
     /// multiple listeners for a single event.
     pub fn on<F>(&mut self, events: impl IntoIterator<Item = WalletEventType>, handler: F)
     where
-        F: Fn(&Event) + 'static + Clone + Send + Sync,
+        F: Fn(&Event) + 'static + Send + Sync,
     {
         let mut events = events.into_iter().peekable();
+        let handler = Arc::new(handler);
         // if no event is provided the handler is registered for all event types
         if events.peek().is_none() {
             // we could use a crate like strum or a macro to iterate over all values, but not sure if it's worth it
@@ -43,14 +45,11 @@ impl EventEmitter {
                 #[cfg(feature = "ledger_nano")]
                 WalletEventType::LedgerAddressGeneration,
             ] {
-                self.handlers
-                    .entry(event_type)
-                    .or_default()
-                    .push(Box::new(handler.clone()));
+                self.handlers.entry(event_type).or_default().push(handler.clone());
             }
         }
         for event in events {
-            self.handlers.entry(event).or_default().push(Box::new(handler.clone()));
+            self.handlers.entry(event).or_default().push(handler.clone());
         }
     }