Fix wrong default derivation path for Trezor

safe-global · Jan 5, 2024 · 0d2a84c · 0d2a84c
1 parent 687a58b
commit 0d2a84c
Show file tree

Hide file tree

Showing 3 changed files with 21 additions and 12 deletions.
diff --git a/safe_cli/operators/hw_wallets/hw_wallet_manager.py b/safe_cli/operators/hw_wallets/hw_wallet_manager.py
@@ -53,24 +53,20 @@ def get_hw_wallet(self, hw_wallet_type: HwWalletType) -> Optional[HwWallet]:
     def get_accounts(
         self,
         hw_wallet_type: HwWalletType,
-        legacy_account: Optional[bool] = False,
+        template_derivation_path: str,
         number_accounts: Optional[int] = 5,
     ) -> List[Tuple[ChecksumAddress, str]]:
         """
 
         :param hw_wallet: Trezor or Ledger
-        :param legacy_account:
+        :param template_derivation_path: formatted string to indicate which path iterate
         :param number_accounts:  number of accounts requested to ledger
         :return: a list of tuples with address and derivation path
         """
         accounts = []
         hw_wallet = self.get_hw_wallet(hw_wallet_type)
         for i in range(number_accounts):
-            if legacy_account:
-                path_string = f"44'/60'/0'/{i}"
-            else:
-                path_string = f"44'/60'/{i}'/0/0"
-
+            path_string = template_derivation_path.format(i=i)
             accounts.append((hw_wallet(path_string).address, path_string))
         return accounts
 

diff --git a/safe_cli/operators/safe_operator.py b/safe_cli/operators/safe_operator.py
@@ -275,13 +275,16 @@ def load_cli_owners(self, keys: List[str]):
                 print_formatted_text(HTML(f"<ansired>Cannot load key={key}</ansired>"))
 
     def load_hw_wallet(
-        self, hw_wallet_type: HwWalletType, derivation_path: str, legacy_account: bool
+        self,
+        hw_wallet_type: HwWalletType,
+        derivation_path: str,
+        template_derivation_path: str,
     ):
         if not self.hw_wallet_manager.is_supported_hw_wallet(hw_wallet_type):
             return None
         if derivation_path is None:
             ledger_accounts = self.hw_wallet_manager.get_accounts(
-                hw_wallet_type, legacy_account=legacy_account
+                hw_wallet_type, template_derivation_path
             )
             if len(ledger_accounts) == 0:
                 return None
@@ -316,12 +319,22 @@ def load_hw_wallet(
     def load_ledger_cli_owners(
         self, derivation_path: str = None, legacy_account: bool = False
     ):
-        self.load_hw_wallet(HwWalletType.LEDGER, derivation_path, legacy_account)
+        if legacy_account:
+            self.load_hw_wallet(HwWalletType.LEDGER, derivation_path, "44'/60'/0'/{i}")
+        else:
+            self.load_hw_wallet(
+                HwWalletType.LEDGER, derivation_path, "44'/60'/{i}'/0/0"
+            )
 
     def load_trezor_cli_owners(
         self, derivation_path: str = None, legacy_account: bool = False
     ):
-        self.load_hw_wallet(HwWalletType.TREZOR, derivation_path, legacy_account)
+        if legacy_account:
+            self.load_hw_wallet(HwWalletType.TREZOR, derivation_path, "44'/60'/0'/{i}")
+        else:
+            self.load_hw_wallet(
+                HwWalletType.TREZOR, derivation_path, "44'/60'/0'/0/{i}"
+            )
 
     def unload_cli_owners(self, owners: List[str]):
         accounts_to_remove: Set[Account] = set()

diff --git a/tests/test_hw_wallet_manager.py b/tests/test_hw_wallet_manager.py
@@ -48,7 +48,7 @@ def test_get_accounts(
         mock_get_address.side_effect = addresses
         # Choosing LEDGER because function is mocked for LEDGER
         hw_wallets = hw_wallet_manager.get_accounts(
-            HwWalletType.LEDGER, number_accounts=2
+            HwWalletType.LEDGER, "44'/60'/{i}'/0/0", number_accounts=2
         )
         self.assertEqual(len(hw_wallets), 2)
         for hw_wallet, expected_address, expected_derivation_path in zip(