Add review suggestions

README.md

@@ -45,7 +45,7 @@ positional arguments:
 options:
   -h, --help  show this help message and exit
   --history   Enable history. By default it's disabled due to security reasons
-  --is-owner  Indicates that address is an owner
+  ----get-safes-from-owner  Indicates that address is an owner (Safe Transaction Service is required for this feature)
 ```
 ### Quick Load Command:
 To load a Safe, use the following command:

safe_cli/main.py

@@ -11,9 +11,6 @@
 from prompt_toolkit.history import FileHistory
 from prompt_toolkit.lexers import PygmentsLexer
 
-from gnosis.eth import EthereumClient
-from gnosis.safe.api import TransactionServiceApi
-
 from safe_cli.argparse_validators import check_ethereum_address
 from safe_cli.operators import (
     SafeOperator,
@@ -23,7 +20,7 @@
 from safe_cli.prompt_parser import PromptParser
 from safe_cli.safe_completer import SafeCompleter
 from safe_cli.safe_lexer import SafeLexer
-from safe_cli.utils import choose_option_question
+from safe_cli.utils import get_safe_from_owner
 
 from .version import version
 
@@ -122,34 +119,6 @@ def loop(self):
                 pass
 
 
-def get_safe_from_owner(
-    owner: ChecksumAddress, node_url: str
-) -> Optional[ChecksumAddress]:
-    """
-    Show a list of Safe to chose between them and return the selected one.
-    :param owner:
-    :param node_url:
-    :return: Safe address of a selected Safe
-    """
-    ethereum_client = EthereumClient(node_url)
-    safe_tx_service = TransactionServiceApi.from_ethereum_client(ethereum_client)
-    safes = safe_tx_service.get_safes_for_owner(owner)
-    if len(safes):
-        # Show safes
-        for option, safe in enumerate(safes):
-            print_formatted_text(HTML(f"{option} - <b>{safe}</b> "))
-        option = choose_option_question(
-            "Select the Safe to initialize the safe-cli", len(safes)
-        )
-        if option is not None:
-            return safes[option]
-
-    print_formatted_text(
-        HTML(f"<ansired>No safe was found for the specified owner {owner}</ansired>")
-    )
-    return None
-
-
 def build_safe_cli() -> Optional[SafeCli]:
     parser = argparse.ArgumentParser()
     parser.add_argument(
@@ -165,14 +134,14 @@ def build_safe_cli() -> Optional[SafeCli]:
         default=False,
     )
     parser.add_argument(
-        "--is-owner",
+        "--get-safes-from-owner",
         action="store_true",
-        help="Indicates that address is an owner",
+        help="Indicates that address is an owner (Safe Transaction Service is required for this feature)",
         default=False,
     )
 
     args = parser.parse_args()
-    if args.is_owner:
+    if args.get_safes_from_owner:
         if (
             safe_address := get_safe_from_owner(args.address, args.node_url)
         ) is not None:

safe_cli/operators/safe_operator.py

@@ -63,7 +63,11 @@
     get_safe_contract_address,
     get_safe_l2_contract_address,
 )
-from safe_cli.utils import choose_option_question, get_erc_20_list, yes_or_no_question
+from safe_cli.utils import (
+    choose_option_from_list_question,
+    get_erc_20_list,
+    yes_or_no_question,
+)
 
 from ..contracts import safe_to_l2_migration
 
@@ -294,12 +298,8 @@ def load_ledger_cli_owners(
             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> "))
-
-            option = choose_option_question(
-                "Select the owner address", len(ledger_accounts)
+            option = choose_option_from_list_question(
+                "Select the owner address", ledger_accounts
             )
             if option is None:
                 return None

safe_cli/utils.py

@@ -1,20 +1,29 @@
 import os
-from typing import Optional
+from typing import List, Optional
 
+from eth_typing import ChecksumAddress
 from prompt_toolkit import HTML, print_formatted_text
 
 from gnosis.eth import EthereumClient
+from gnosis.safe.api import TransactionServiceApi
 
 
-# Return a list of address of ERC20 tokens related with the safe_address
-# block_step is the number of blocks retrieved for each get until get all blocks between from_block until to_block
 def get_erc_20_list(
     ethereum_client: EthereumClient,
     safe_address: str,
     from_block: int,
     to_block: int,
     block_step: int = 500000,
 ) -> list:
+    """
+
+    :param ethereum_client:
+    :param safe_address:
+    :param from_block:
+    :param to_block:
+    :param block_step: is the number of blocks retrieved for each get until get all blocks between from_block until to_block
+    :return: a list of address of ERC20 tokens related with the safe_address
+    """
     addresses = set()
     for i in range(from_block, to_block + 1, block_step):
         events = ethereum_client.erc20.get_total_transfer_history(
@@ -46,11 +55,14 @@ def yes_or_no_question(question: str, default_no: bool = True) -> bool:
         return False if default_no else True
 
 
-def choose_option_question(
-    question: str, number_options: int, default_option: int = 0
+def choose_option_from_list_question(
+    question: str, options: List, default_option: int = 0
 ) -> Optional[int]:
     if "PYTEST_CURRENT_TEST" in os.environ:
         return default_option  # Ignore confirmations when running tests
+    number_options = len(options)
+    for number_option, option in enumerate(options):
+        print_formatted_text(HTML(f"{number_option} - <b>{option}</b> "))
     choices = f" [0-{number_options-1}] default {default_option}: "
     reply = str(get_input(question + choices)).lower().strip() or str(default_option)
     try:
@@ -66,3 +78,28 @@ def choose_option_question(
         return None
 
     return option
+
+
+def get_safe_from_owner(
+    owner: ChecksumAddress, node_url: str
+) -> Optional[ChecksumAddress]:
+    """
+    Show a list of Safe to chose between them and return the selected one.
+    :param owner:
+    :param node_url:
+    :return: Safe address of a selected Safe
+    """
+    ethereum_client = EthereumClient(node_url)
+    safe_tx_service = TransactionServiceApi.from_ethereum_client(ethereum_client)
+    safes = safe_tx_service.get_safes_for_owner(owner)
+    if safes:
+        option = choose_option_from_list_question(
+            "Select the Safe to initialize the safe-cli", safes
+        )
+        if option is not None:
+            return safes[option]
+
+    print_formatted_text(
+        HTML(f"<ansired>No safe was found for the specified owner {owner}</ansired>")
+    )
+    return None

tests/test_entrypoint.py

@@ -27,7 +27,7 @@ def build_test_safe_cli(self, mock_parse_args: MagicMock):
             address=self.random_safe_address,
             node_url=self.ethereum_node_url,
             history=True,
-            is_owner=False,
+            get_safes_from_owner=False,
         )
         return build_safe_cli()
 
@@ -37,7 +37,7 @@ def build_test_safe_cli_for_owner(self, mock_parse_args: MagicMock):
             address=self.random_safe_address,
             node_url=self.ethereum_node_url,
             history=True,
-            is_owner=True,
+            get_safes_from_owner=True,
         )
         return build_safe_cli()
 

tests/test_utils.py

@@ -3,7 +3,9 @@
 from unittest import mock
 from unittest.mock import MagicMock
 
-from safe_cli.utils import choose_option_question, yes_or_no_question
+from eth_account import Account
+
+from safe_cli.utils import choose_option_from_list_question, yes_or_no_question
 
 
 class TestUtils(unittest.TestCase):
@@ -37,17 +39,18 @@ def test_yes_or_no_question(self, input_mock: MagicMock):
     @mock.patch("safe_cli.utils.get_input")
     def test_choose_option_question(self, input_mock: MagicMock):
         pytest_current_test = os.environ.pop("PYTEST_CURRENT_TEST")
-
+        address = Account.create().address
+        options = [address for i in range(0, 5)]
         input_mock.return_value = ""
-        self.assertEqual(choose_option_question("", 1), 0)
+        self.assertEqual(choose_option_from_list_question("", options), 0)
         input_mock.return_value = ""
-        self.assertEqual(choose_option_question("", 5, 4), 4)
+        self.assertEqual(choose_option_from_list_question("", options, 4), 4)
         input_mock.return_value = "m"
-        self.assertIsNone(choose_option_question("", 1))
+        self.assertIsNone(choose_option_from_list_question("", options))
         input_mock.return_value = "10"
-        self.assertIsNone(choose_option_question("", 1))
+        self.assertIsNone(choose_option_from_list_question("", options))
         input_mock.return_value = "1"
-        self.assertEqual(choose_option_question("", 2), 1)
+        self.assertEqual(choose_option_from_list_question("", options), 1)
 
         os.environ["PYTEST_CURRENT_TEST"] = pytest_current_test