From ea1ee95bc214a3707322d372892f389bfd21cce3 Mon Sep 17 00:00:00 2001 From: moisses89 <7888669+moisses89@users.noreply.github.com> Date: Mon, 6 Nov 2023 18:01:58 +0100 Subject: [PATCH 1/2] Add derivation-path option to load ledger owners --- README.md | 3 ++ safe_cli/operators/hw_accounts/exceptions.py | 7 ++++ .../operators/hw_accounts/ledger_manager.py | 14 ++++++-- safe_cli/operators/safe_operator.py | 35 ++++++++++--------- safe_cli/prompt_parser.py | 7 +++- tests/test_ledger_manager.py | 2 +- tests/test_safe_operator.py | 28 ++++++++++++--- 7 files changed, 70 insertions(+), 26 deletions(-) diff --git a/README.md b/README.md index 3ad2b57b..582b8d0a 100644 --- a/README.md +++ b/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="" SUBSYSTEMS=="usb", ATTRS{idVendor}=="2c97", ATTRS{idProduct}=="0004", MODE="0660", TAG+="uaccess", TAG+="udev-acl" OWNER="" ``` +Safe-cli Ledger commands: +- `load_ledger_cli_owners [--legacy-accounts] [--derivation-path ]`: 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 --owners --threshold --salt-nonce `. diff --git a/safe_cli/operators/hw_accounts/exceptions.py b/safe_cli/operators/hw_accounts/exceptions.py index f74947f0..b12dcb70 100644 --- a/safe_cli/operators/hw_accounts/exceptions.py +++ b/safe_cli/operators/hw_accounts/exceptions.py @@ -3,6 +3,7 @@ from ledgereth.exceptions import ( LedgerAppNotOpened, LedgerCancel, + LedgerError, LedgerLocked, LedgerNotFound, ) @@ -10,6 +11,10 @@ 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") diff --git a/safe_cli/operators/hw_accounts/ledger_manager.py b/safe_cli/operators/hw_accounts/ledger_manager.py index 7eb4e446..14aba719 100644 --- a/safe_cli/operators/hw_accounts/ledger_manager.py +++ b/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: """ diff --git a/safe_cli/operators/safe_operator.py b/safe_cli/operators/safe_operator.py index 81d50d58..816c357f 100644 --- a/safe_cli/operators/safe_operator.py +++ b/safe_cli/operators/safe_operator.py @@ -277,27 +277,30 @@ def load_cli_owners(self, keys: List[str]): except ValueError: print_formatted_text(HTML(f"Cannot load key={key}")) - 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} - {address} ")) - for option, ledger_account in enumerate(ledger_accounts): - address, _ = ledger_account - print_formatted_text(HTML(f"{option} - {address} ")) + option = choose_option_question( + "Select the owner address", len(ledger_accounts) - 1 + ) + 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( diff --git a/safe_cli/prompt_parser.py b/safe_cli/prompt_parser.py index 202d150f..08bc26b2 100644 --- a/safe_cli/prompt_parser.py +++ b/safe_cli/prompt_parser.py @@ -165,7 +165,7 @@ 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(args.derivation_path, args.legacy_accounts) @safe_exception def unload_cli_owners(args): @@ -312,6 +312,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", diff --git a/tests/test_ledger_manager.py b/tests/test_ledger_manager.py index 8b9a103d..487255c9 100644 --- a/tests/test_ledger_manager.py +++ b/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] diff --git a/tests/test_safe_operator.py b/tests/test_safe_operator.py index a6ef3791..8c9cef0c 100644 --- a/tests/test_safe_operator.py +++ b/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( From a604c5a9556c62dd554165d66a7fdb4b59f3cd14 Mon Sep 17 00:00:00 2001 From: moisses89 <7888669+moisses89@users.noreply.github.com> Date: Tue, 7 Nov 2023 11:00:42 +0100 Subject: [PATCH 2/2] Fix minor issues --- safe_cli/operators/safe_operator.py | 4 ++-- safe_cli/prompt_parser.py | 4 +++- safe_cli/utils.py | 2 +- 3 files changed, 6 insertions(+), 4 deletions(-) diff --git a/safe_cli/operators/safe_operator.py b/safe_cli/operators/safe_operator.py index 816c357f..8c9695ed 100644 --- a/safe_cli/operators/safe_operator.py +++ b/safe_cli/operators/safe_operator.py @@ -294,7 +294,7 @@ def load_ledger_cli_owners( print_formatted_text(HTML(f"{option} - {address} ")) option = choose_option_question( - "Select the owner address", len(ledger_accounts) - 1 + "Select the owner address", len(ledger_accounts) ) if option is None: return None @@ -305,7 +305,7 @@ def load_ledger_cli_owners( print_formatted_text( HTML( f"Loaded account {address} " - 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" ) ) diff --git a/safe_cli/prompt_parser.py b/safe_cli/prompt_parser.py index 08bc26b2..f85d43e7 100644 --- a/safe_cli/prompt_parser.py +++ b/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.derivation_path, 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): diff --git a/safe_cli/utils.py b/safe_cli/utils.py index 132861fa..d24471e6 100644 --- a/safe_cli/utils.py +++ b/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)