Add derivation-path option to load ledger owners

README.md

@@ -138,6 +138,9 @@ SUBSYSTEMS=="usb", ATTRS{idVendor}=="2c97", ATTRS{idProduct}=="0000", MODE="0660
 SUBSYSTEMS=="usb", ATTRS{idVendor}=="2c97", ATTRS{idProduct}=="0001", MODE="0660", TAG+="uaccess", TAG+="udev-acl" OWNER="<UNIX username>"
 SUBSYSTEMS=="usb", ATTRS{idVendor}=="2c97", ATTRS{idProduct}=="0004", MODE="0660", TAG+="uaccess", TAG+="udev-acl" OWNER="<UNIX username>"
 ```
+Safe-cli Ledger commands:
+- `load_ledger_cli_owners [--legacy-accounts] [--derivation-path <str>]`: show a list of the first 5 accounts (--legacy-accounts search using ledger legacy derivation) or load an account from provided derivation path.
+
 **NOTE**: before signing anything ensure that the data showing on your ledger is the same as the safe-cli data.
 ## Creating a new Safe
 Use `safe-creator <node_url> <private_key> --owners <checksummed_address_1> <checksummed_address_2> --threshold <uint> --salt-nonce <uint256>`.

safe_cli/operators/hw_accounts/exceptions.py

@@ -3,13 +3,18 @@
 from ledgereth.exceptions import (
     LedgerAppNotOpened,
     LedgerCancel,
+    LedgerError,
     LedgerLocked,
     LedgerNotFound,
 )
 
 from safe_cli.operators.exceptions import HardwareWalletException
 
 
+class InvalidDerivationPath(LedgerError):
+    message = "The provided derivation path is not valid"
+
+
 def raise_as_hw_account_exception(function):
     @functools.wraps(function)
     def wrapper(*args, **kwargs):
@@ -23,6 +28,8 @@ def wrapper(*args, **kwargs):
             raise HardwareWalletException(e.message)
         except LedgerCancel as e:
             raise HardwareWalletException(e.message)
+        except InvalidDerivationPath as e:
+            raise HardwareWalletException(e.message)
         except BaseException as e:
             if "Error while writing" in e.args:
                 raise HardwareWalletException("Ledger error writting, restart safe-cli")

safe_cli/operators/hw_accounts/ledger_manager.py

@@ -5,13 +5,17 @@
 from ledgereth.accounts import LedgerAccount, get_account_by_path
 from ledgereth.comms import init_dongle
 from ledgereth.exceptions import LedgerNotFound
+from ledgereth.utils import is_bip32_path
 from prompt_toolkit import HTML, print_formatted_text
 
 from gnosis.eth.eip712 import eip712_encode
 from gnosis.safe import SafeTx
 from gnosis.safe.signatures import signature_to_bytes
 
-from safe_cli.operators.hw_accounts.exceptions import raise_as_hw_account_exception
+from safe_cli.operators.hw_accounts.exceptions import (
+    InvalidDerivationPath,
+    raise_as_hw_account_exception,
+)
 
 
 class LedgerManager:
@@ -62,15 +66,19 @@ def get_accounts(
         return accounts
 
     @raise_as_hw_account_exception
-    def add_account(self, derivation_path: str):
+    def add_account(self, derivation_path: str) -> ChecksumAddress:
         """
-        Add an account to ledger manager set
+        Add an account to ledger manager set and return the added address
 
         :param derivation_path:
         :return:
         """
+        if not is_bip32_path(derivation_path):
+            raise InvalidDerivationPath()
+
         account = get_account_by_path(derivation_path, self.dongle)
         self.accounts.add(LedgerAccount(account.path, account.address))
+        return account.address
 
     def delete_accounts(self, addresses: List[ChecksumAddress]) -> Set:
         """

safe_cli/operators/safe_operator.py

@@ -277,32 +277,35 @@ def load_cli_owners(self, keys: List[str]):
             except ValueError:
                 print_formatted_text(HTML(f"<ansired>Cannot load key={key}</ansired>"))
 
-    def load_ledger_cli_owners(self, legacy_account: bool = False):
+    def load_ledger_cli_owners(
+        self, derivation_path: str = None, legacy_account: bool = False
+    ):
         if not self.ledger_manager:
             return None
+        if derivation_path is None:
+            ledger_accounts = self.ledger_manager.get_accounts(
+                legacy_account=legacy_account
+            )
+            if len(ledger_accounts) == 0:
+                return None
 
-        ledger_accounts = self.ledger_manager.get_accounts(
-            legacy_account=legacy_account
-        )
-        if len(ledger_accounts) == 0:
-            return None
+            for option, ledger_account in enumerate(ledger_accounts):
+                address, _ = ledger_account
+                print_formatted_text(HTML(f"{option} - <b>{address}</b> "))
 
-        for option, ledger_account in enumerate(ledger_accounts):
-            address, _ = ledger_account
-            print_formatted_text(HTML(f"{option} - <b>{address}</b> "))
+            option = choose_option_question(
+                "Select the owner address", len(ledger_accounts)
+            )
+            if option is None:
+                return None
+            _, derivation_path = ledger_accounts[option]
 
-        option = choose_option_question(
-            "Select the owner address", len(ledger_accounts) - 1
-        )
-        if option is None:
-            return None
-        address, derivation_path = ledger_accounts[option]
-        self.ledger_manager.add_account(derivation_path)
+        address = self.ledger_manager.add_account(derivation_path)
         balance = self.ethereum_client.get_balance(address)
         print_formatted_text(
             HTML(
                 f"Loaded account <b>{address}</b> "
-                f'with balance={Web3.from_wei(balance, "ether")} ether'
+                f'with balance={Web3.from_wei(balance, "ether")} ether.\n'
                 f"Ledger account cannot be defined as sender"
             )
         )

safe_cli/prompt_parser.py

@@ -165,7 +165,9 @@ def load_cli_owners(args):
 
     @safe_exception
     def load_ledger_cli_owners(args):
-        safe_operator.load_ledger_cli_owners(args.legacy_accounts)
+        safe_operator.load_ledger_cli_owners(
+            derivation_path=args.derivation_path, legacy_account=args.legacy_accounts
+        )
 
     @safe_exception
     def unload_cli_owners(args):
@@ -312,6 +314,11 @@ def remove_delegate(args):
     parser_load_cli_owners.set_defaults(func=load_cli_owners)
 
     parser_load_ledger_cli_owners = subparsers.add_parser("load_ledger_cli_owners")
+    parser_load_ledger_cli_owners.add_argument(
+        "--derivation-path",
+        type=str,
+        help="Load address for the provided derivation path",
+    )
     parser_load_ledger_cli_owners.add_argument(
         "--legacy-accounts",
         action="store_true",

safe_cli/utils.py

@@ -51,7 +51,7 @@ def choose_option_question(
 ) -> Optional[int]:
     if "PYTEST_CURRENT_TEST" in os.environ:
         return default_option  # Ignore confirmations when running tests
-    choices = f" [0-{number_options}] default {default_option}:"
+    choices = f" [0-{number_options-1}] default {default_option}: "
     reply = str(get_input(question + choices)).lower().strip() or str(default_option)
     try:
         option = int(reply)

tests/test_ledger_manager.py

@@ -147,7 +147,7 @@ def test_add_account(self, mock_get_account_by_path: MagicMock):
         )
         self.assertEqual(len(ledger_manager.accounts), 0)
 
-        ledger_manager.add_account(derivation_path)
+        self.assertEqual(ledger_manager.add_account(derivation_path), account_address)
 
         self.assertEqual(len(ledger_manager.accounts), 1)
         ledger_account = list(ledger_manager.accounts)[0]

tests/test_safe_operator.py

@@ -18,6 +18,7 @@
     ExistingOwnerException,
     FallbackHandlerNotSupportedException,
     GuardNotSupportedException,
+    HardwareWalletException,
     HashAlreadyApproved,
     InvalidFallbackHandlerException,
     InvalidGuardException,
@@ -87,17 +88,34 @@ def test_load_ledger_cli_owner(self, mock_get_account_by_path: MagicMock):
         safe_operator.ledger_manager.get_accounts = MagicMock(return_value=[])
         safe_operator.load_ledger_cli_owners()
         self.assertEqual(len(safe_operator.ledger_manager.accounts), 0)
-        random_account = Account.create().address
-        other_random_account = Account.create().address
+        random_address = Account.create().address
+        other_random_address = Account.create().address
         safe_operator.ledger_manager.get_accounts.return_value = [
-            (random_account, "44'/60'/0'/0/0"),
-            (other_random_account, "44'/60'/0'/0/1"),
+            (random_address, "44'/60'/0'/0/0"),
+            (other_random_address, "44'/60'/0'/0/1"),
         ]
+
         mock_get_account_by_path.return_value = LedgerAccount(
-            "44'/60'/0'/0/0", random_account
+            "44'/60'/0'/0/0", random_address
         )
         safe_operator.load_ledger_cli_owners()
         self.assertEqual(len(safe_operator.ledger_manager.accounts), 1)
+        self.assertEqual(
+            safe_operator.ledger_manager.accounts.pop().address, random_address
+        )
+
+        # Only accept ethereum derivation paths
+        with self.assertRaises(HardwareWalletException):
+            safe_operator.load_ledger_cli_owners(derivation_path="44'/137'/0'/0/1")
+
+        mock_get_account_by_path.return_value = LedgerAccount(
+            "44'/60'/0'/0/0", owner_address
+        )
+        safe_operator.load_ledger_cli_owners(derivation_path="44'/60'/0'/0/0")
+        self.assertEqual(len(safe_operator.ledger_manager.accounts), 1)
+        self.assertEqual(
+            safe_operator.ledger_manager.accounts.pop().address, owner_address
+        )
 
     def test_approve_hash(self):
         safe_address = self.deploy_test_safe(