Refactors cli to avoid multi-type returns (normal return + None). Adds exceptions for errors.

cli/exceptions.py

@@ -0,0 +1,22 @@
+class InvalidParameter(ValueError):
+    """Raised when a command line parameter is invalid (either missing or wrong)"""
+
+    def __init__(self, msg, **kwargs):
+        self.msg = msg
+        self.kwargs = kwargs
+
+
+class InvalidKey(Exception):
+    """Raised when there is an error loading the keys"""
+
+    def __init__(self, msg, **kwargs):
+        self.reason = msg
+        self.kwargs = kwargs
+
+
+class TowerResponseError(Exception):
+    """Raised when the tower responds with an error"""
+
+    def __init__(self, msg, **kwargs):
+        self.reason = msg
+        self.kwargs = kwargs

cli/teos_cli.py

@@ -6,12 +6,13 @@ import requests
 from sys import argv
 from uuid import uuid4
 from coincurve import PublicKey
+from requests import Timeout, ConnectionError
 from getopt import getopt, GetoptError
-from requests import ConnectTimeout, ConnectionError
 from requests.exceptions import MissingSchema, InvalidSchema, InvalidURL
 
-from cli.help import show_usage, help_add_appointment, help_get_appointment, help_register, help_get_all_appointments
 from cli import DEFAULT_CONF, DATA_DIR, CONF_FILE_NAME, LOG_PREFIX
+from cli.exceptions import InvalidKey, InvalidParameter, TowerResponseError
+from cli.help import show_usage, help_add_appointment, help_get_appointment, help_register, help_get_all_appointments
 
 import common.cryptographer
 from common.blob import Blob
@@ -36,26 +37,29 @@ def register(compressed_pk, teos_url):
         teos_url (:obj:`str`): the teos base url.
 
     Returns:
-        :obj:`dict` or :obj:`None`: a dictionary containing the tower response if the registration succeeded. ``None``
+        :obj:`dict`: a dictionary containing the tower response if the registration succeeded.
-        otherwise.
+
+    Raises:
+        :obj:`InvalidParameter <cli.exceptions.InvalidParameter>`: if `compressed_pk` is invalid.
+        :obj:`ConnectionError`: if the client cannot connect to the tower.
+        :obj:`TowerResponseError <cli.exceptions.TowerResponseError>`: if the tower responded with an error, or the
+        response was invalid.
     """
 
     if not is_compressed_pk(compressed_pk):
-        logger.error("The cli public key is not valid")
+        raise InvalidParameter("The cli public key is not valid")
-        return None
 
     # Send request to the server.
     register_endpoint = "{}/register".format(teos_url)
     data = {"public_key": compressed_pk}
 
     logger.info("Registering in the Eye of Satoshi")
-    server_response = post_request(data, register_endpoint)
+    response = process_post_response(post_request(data, register_endpoint))
-    if server_response:
+
-        response_json = process_post_response(server_response)
+    return response
-        return response_json
 
 
-def add_appointment(appointment_data, cli_sk, teos_pk, teos_url, appointments_folder_path):
+def add_appointment(appointment_data, cli_sk, teos_pk, teos_url):
     """
     Manages the add_appointment command.
 
@@ -67,76 +71,67 @@ def add_appointment(appointment_data, cli_sk, teos_pk, teos_url, appointments_fo
         - Send the appointment to the tower
         - Wait for the response
         - Check the tower's response and signature
-        - Store the receipt (appointment + signature) on disk
 
     Args:
         appointment_data (:obj:`dict`): a dictionary containing the appointment data.
         cli_sk (:obj:`PrivateKey`): the client's private key.
         teos_pk (:obj:`PublicKey`): the tower's public key.
         teos_url (:obj:`str`): the teos base url.
-        appointments_folder_path (:obj:`str`): the path to the appointments folder.
-
 
     Returns:
-        :obj:`bool`: True if the appointment is accepted by the tower and the receipt is properly stored. False if any
+        :obj:`tuple`: A tuple (`:obj:Appointment <common.appointment.Appointment>`, :obj:`str`) containing the
-        error occurs during the process.
+        appointment and the tower's signature.
+
+    Raises:
+        :obj:`InvalidParameter <cli.exceptions.InvalidParameter>`: if `appointment_data` or any of its fields is
+        invalid.
+        :obj:`ValueError`: if the appointment cannot be signed.
+        :obj:`ConnectionError`: if the client cannot connect to the tower.
+        :obj:`TowerResponseError <cli.exceptions.TowerResponseError>`: if the tower responded with an error, or the
+        response was invalid.
     """
 
-    if appointment_data is None:
+    if not appointment_data:
-        logger.error("The provided appointment JSON is empty")
+        raise InvalidParameter("The provided appointment JSON is empty")
-        return False
-
-    if not is_256b_hex_str(appointment_data.get("tx_id")):
-        logger.error("The provided txid is not valid")
-        return False
 
     tx_id = appointment_data.get("tx_id")
     tx = appointment_data.get("tx")
 
-    if None not in [tx_id, tx]:
+    if not is_256b_hex_str(tx_id):
+        raise InvalidParameter("The provided locator is wrong or missing")
+
+    if not tx:
+        raise InvalidParameter("The provided data is missing the transaction")
+
     appointment_data["locator"] = compute_locator(tx_id)
     appointment_data["encrypted_blob"] = Cryptographer.encrypt(Blob(tx), tx_id)
-
-    else:
-        logger.error("Appointment data is missing some fields")
-        return False
-
     appointment = Appointment.from_dict(appointment_data)
     signature = Cryptographer.sign(appointment.serialize(), cli_sk)
 
-    if not (appointment and signature):
+    # FIXME: the cryptographer should return exception we can capture
-        return False
+    if not signature:
+        raise ValueError("The provided appointment cannot be signed")
 
     data = {"appointment": appointment.to_dict(), "signature": signature}
 
     # Send appointment to the server.
-    add_appointment_endpoint = "{}/add_appointment".format(teos_url)
     logger.info("Sending appointment to the Eye of Satoshi")
-    server_response = post_request(data, add_appointment_endpoint)
+    add_appointment_endpoint = "{}/add_appointment".format(teos_url)
-    if server_response is None:
+    response = process_post_response(post_request(data, add_appointment_endpoint))
-        return False
 
-    response_json = process_post_response(server_response)
+    signature = response.get("signature")
-
-    if response_json is None:
-        return False
-
-    signature = response_json.get("signature")
     # Check that the server signed the appointment as it should.
-    if signature is None:
+    if not signature:
-        logger.error("The response does not contain the signature of the appointment")
+        raise TowerResponseError("The response does not contain the signature of the appointment")
-        return False
 
     rpk = Cryptographer.recover_pk(appointment.serialize(), signature)
     if not Cryptographer.verify_rpk(teos_pk, rpk):
-        logger.error("The returned appointment's signature is invalid")
+        raise TowerResponseError("The returned appointment's signature is invalid")
-        return False
 
     logger.info("Appointment accepted and signed by the Eye of Satoshi")
-    logger.info("Remaining slots: {}".format(response_json.get("available_slots")))
+    logger.info("Remaining slots: {}".format(response.get("available_slots")))
 
-    # All good, store appointment and signature
+    return appointment, signature
-    return save_appointment_receipt(appointment.to_dict(), signature, appointments_folder_path)
 
 
 def get_appointment(locator, cli_sk, teos_pk, teos_url):
@@ -150,17 +145,20 @@ def get_appointment(locator, cli_sk, teos_pk, teos_url):
         teos_url (:obj:`str`): the teos base url.
 
     Returns:
-        :obj:`dict` or :obj:`None`: a dictionary containing the appointment data if the locator is valid and the tower
+        :obj:`dict`: a dictionary containing the appointment data.
-        responds. ``None`` otherwise.
+
+    Raises:
+        :obj:`InvalidParameter <cli.exceptions.InvalidParameter>`: if `appointment_data` or any of its fields is
+        invalid.
+        :obj:`ConnectionError`: if the client cannot connect to the tower.
+        :obj:`TowerResponseError <cli.exceptions.TowerResponseError>`: if the tower responded with an error, or the
+        response was invalid.
     """
 
     # FIXME: All responses from the tower should be signed. Not using teos_pk atm.
 
-    valid_locator = is_locator(locator)
+    if not is_locator(locator):
-
+        raise InvalidParameter("The provided locator is not valid", locator=locator)
-    if not valid_locator:
-        logger.error("The provided locator is not valid", locator=locator)
-        return None
 
     message = "get appointment {}".format(locator)
     signature = Cryptographer.sign(message.encode(), cli_sk)
@@ -169,10 +167,9 @@ def get_appointment(locator, cli_sk, teos_pk, teos_url):
     # Send request to the server.
     get_appointment_endpoint = "{}/get_appointment".format(teos_url)
     logger.info("Sending appointment to the Eye of Satoshi")
-    server_response = post_request(data, get_appointment_endpoint)
+    response = process_post_response(post_request(data, get_appointment_endpoint))
-    response_json = process_post_response(server_response)
 
-    return response_json
+    return response
 
 
 def get_all_appointments(teos_url):
@@ -218,45 +215,41 @@ def load_keys(teos_pk_path, cli_sk_path, cli_pk_path):
         cli_pk_path (:obj:`str`): path to the client public key file.
 
     Returns:
-        :obj:`tuple` or ``None``: a three-item tuple containing a ``PrivateKey``, a ``PublicKey`` and a ``str``
+        :obj:`tuple`: a three-item tuple containing a ``PrivateKey``, a ``PublicKey`` and a ``str``
-        representing the tower pk, user sk and user compressed pk respectively if all keys can be loaded.
+        representing the tower pk, user sk and user compressed pk respectively.
-        ``None`` otherwise.
+
+    Raises:
+        :obj:`InvalidKey <cli.exceptions.InvalidKey>`: if any of the keys is invalid or cannot be loaded.
     """
 
-    if teos_pk_path is None:
+    if not teos_pk_path:
-        logger.error("TEOS's public key file not found. Please check your settings")
+        raise InvalidKey("TEOS's public key file not found. Please check your settings")
-        return None
 
-    if cli_sk_path is None:
+    if not cli_sk_path:
-        logger.error("Client's private key file not found. Please check your settings")
+        raise InvalidKey("Client's private key file not found. Please check your settings")
-        return None
 
-    if cli_pk_path is None:
+    if not cli_pk_path:
-        logger.error("Client's public key file not found. Please check your settings")
+        raise InvalidKey("Client's public key file not found. Please check your settings")
-        return None
 
     try:
         teos_pk_der = Cryptographer.load_key_file(teos_pk_path)
         teos_pk = PublicKey(teos_pk_der)
 
     except ValueError:
-        logger.error("TEOS public key is invalid or cannot be parsed")
+        raise InvalidKey("TEOS public key is invalid or cannot be parsed")
-        return None
 
     cli_sk_der = Cryptographer.load_key_file(cli_sk_path)
     cli_sk = Cryptographer.load_private_key_der(cli_sk_der)
 
     if cli_sk is None:
-        logger.error("Client private key is invalid or cannot be parsed")
+        raise InvalidKey("Client private key is invalid or cannot be parsed")
-        return None
 
     try:
         cli_pk_der = Cryptographer.load_key_file(cli_pk_path)
         compressed_cli_pk = Cryptographer.get_compressed_pk(PublicKey(cli_pk_der))
 
     except ValueError:
-        logger.error("Client public key is invalid or cannot be parsed")
+        raise InvalidKey("Client public key is invalid or cannot be parsed")
-        return None
 
     return teos_pk, cli_sk, compressed_cli_pk
 
@@ -270,26 +263,25 @@ def post_request(data, endpoint):
         endpoint (:obj:`str`): the endpoint to send the post request.
 
     Returns:
-        :obj:`dict` or ``None``: a json-encoded dictionary with the server response if the data can be posted.
+        :obj:`dict`: a json-encoded dictionary with the server response if the data can be posted.
-        ``None`` otherwise.
+
+    Raises:
+        :obj:`ConnectionError`: if the client cannot connect to the tower.
     """
 
     try:
         return requests.post(url=endpoint, json=data, timeout=5)
 
-    except ConnectTimeout:
+    except Timeout:
-        logger.error("Can't connect to the Eye of Satoshi's API. Connection timeout")
+        message = "Can't connect to the Eye of Satoshi's API. Connection timeout"
 
     except ConnectionError:
-        logger.error("Can't connect to the Eye of Satoshi's API. Server cannot be reached")
+        message = "Can't connect to the Eye of Satoshi's API. Server cannot be reached"
 
     except (InvalidSchema, MissingSchema, InvalidURL):
-        logger.error("Invalid URL. No schema, or invalid schema, found ({})".format(endpoint))
+        message = "Invalid URL. No schema, or invalid schema, found ({})".format(endpoint)
 
-    except requests.exceptions.Timeout:
+    raise ConnectionError(message)
-        logger.error("The request timed out")
-
-    return None
 
 
 def process_post_response(response):
@@ -300,27 +292,26 @@ def process_post_response(response):
         response (:obj:`requests.models.Response`): a ``Response`` object obtained from the request.
 
     Returns:
-        :obj:`dict` or :obj:`None`: a dictionary containing the tower's response data if the response type is
+        :obj:`dict`: a dictionary containing the tower's response data if the response type is
-        ``HTTP_OK`` and the response can be properly parsed. ``None`` otherwise.
+        ``HTTP_OK``.
-    """
 
-    if not response:
+    Raises:
-        return None
+        :obj:`TowerResponseError <cli.exceptions.TowerResponseError>`: if the tower responded with an error, or the
+        response was invalid.
+    """
 
     try:
         response_json = response.json()
 
     except (json.JSONDecodeError, AttributeError):
-        logger.error(
+        raise TowerResponseError(
             "The server returned a non-JSON response", status_code=response.status_code, reason=response.reason
         )
-        return None
 
     if response.status_code != constants.HTTP_OK:
-        logger.error(
+        raise TowerResponseError(
             "The server returned an error", status_code=response.status_code, reason=response.reason, data=response_json
         )
-        return None
 
     return response_json
 
@@ -334,15 +325,18 @@ def parse_add_appointment_args(args):
         option and the path to a file containing a json encoded appointment.
 
     Returns:
-        :obj:`dict` or :obj:`None`: A dictionary containing the appointment data if it can be loaded. ``None``
+        :obj:`dict`: A dictionary containing the appointment data.
-        otherwise.
+
+    Raises:
+        :obj:`InvalidParameter <cli.exceptions.InvalidParameter>`: if the appointment data is not JSON encoded.
+        :obj:`FileNotFoundError`: if -f is passed and the appointment file is not found.
+        :obj:`IOError`: if -f was passed and the file cannot be read.
     """
 
     use_help = "Use 'help add_appointment' for help of how to use the command"
 
     if not args:
-        logger.error("No appointment data provided. " + use_help)
+        raise InvalidParameter("No appointment data provided. " + use_help)
-        return None
 
     arg_opt = args.pop(0)
 
@@ -353,22 +347,20 @@ def parse_add_appointment_args(args):
         if arg_opt in ["-f", "--file"]:
             fin = args.pop(0)
             if not os.path.isfile(fin):
-                logger.error("Can't find file", filename=fin)
+                raise FileNotFoundError("Cannot find {}".format(fin))
-                return None
 
             try:
                 with open(fin) as f:
                     appointment_data = json.load(f)
 
             except IOError as e:
-                logger.error("I/O error", errno=e.errno, error=e.strerror)
+                raise IOError("Cannot read appointment file. {}".format(str(e)))
-                return None
+
         else:
             appointment_data = json.loads(arg_opt)
 
     except json.JSONDecodeError:
-        logger.error("Non-JSON encoded data provided as appointment. " + use_help)
+        raise InvalidParameter("Non-JSON encoded data provided as appointment. " + use_help)
-        return None
 
     return appointment_data
 
@@ -382,9 +374,6 @@ def save_appointment_receipt(appointment, signature, appointments_folder_path):
         signature (:obj:`str`): the signature of the appointment performed by the tower.
         appointments_folder_path (:obj:`str`): the path to the appointments folder.
 
-    Returns:
-        :obj:`bool`: True if the appointment if properly saved. False otherwise.
-
     Raises:
         IOError: if an error occurs whilst writing the file on disk.
     """
@@ -403,14 +392,12 @@ def save_appointment_receipt(appointment, signature, appointments_folder_path):
         with open(filename, "w") as f:
             json.dump(data, f)
             logger.info("Appointment saved at {}".format(filename))
-            return True
 
     except IOError as e:
-        logger.error("There was an error while saving the appointment", error=e)
+        raise IOError("There was an error while saving the appointment. {}".format(e))
-        return False
 
 
-def main(args, command_line_conf):
+def main(command, args, command_line_conf):
     # Loads config and sets up the data folder and log file
     config_loader = ConfigLoader(DATA_DIR, CONF_FILE_NAME, DEFAULT_CONF, command_line_conf)
     config = config_loader.build_config()
@@ -424,26 +411,19 @@ def main(args, command_line_conf):
     if not teos_url.startswith("http"):
         teos_url = "http://" + teos_url
 
-    keys = load_keys(config.get("TEOS_PUBLIC_KEY"), config.get("CLI_PRIVATE_KEY"), config.get("CLI_PUBLIC_KEY"))
-    if keys is not None:
-        teos_pk, cli_sk, compressed_cli_pk = keys
-
     try:
-            if args:
+        teos_pk, cli_sk, compressed_cli_pk = load_keys(
-                command = args.pop(0)
+            config.get("TEOS_PUBLIC_KEY"), config.get("CLI_PRIVATE_KEY"), config.get("CLI_PUBLIC_KEY")
+        )
 
-                if command in commands:
         if command == "register":
             register_data = register(compressed_cli_pk, teos_url)
-                        if register_data:
+            logger.info("Registration succeeded. Available slots: {}".format(register_data.get("available_slots")))
-                            print(register_data)
 
         if command == "add_appointment":
-                        # Get appointment data from user.
             appointment_data = parse_add_appointment_args(args)
-                        add_appointment(
+            appointment, signature = add_appointment(appointment_data, cli_sk, teos_pk, teos_url)
-                            appointment_data, cli_sk, teos_pk, teos_url, config.get("APPOINTMENTS_FOLDER_NAME")
+            save_appointment_receipt(appointment.to_dict(), signature, config.get("APPOINTMENTS_FOLDER_NAME"))
-                        )
 
         elif command == "get_appointment":
             if not args:
@@ -477,23 +457,21 @@ def main(args, command_line_conf):
                 elif command == "get_appointment":
                     sys.exit(help_get_appointment())
 
-                            else:
-                                logger.error("Unknown command. Use help to check the list of available commands")
-
                 elif command == "get_all_appointments":
                     sys.exit(help_get_all_appointments())
 
-                        else:
-                            sys.exit(show_usage())
-
                 else:
                     logger.error("Unknown command. Use help to check the list of available commands")
 
             else:
-                logger.error("No command provided. Use help to check the list of available commands")
+                sys.exit(show_usage())
 
-        except json.JSONDecodeError:
+    except (FileNotFoundError, IOError, ConnectionError, ValueError) as e:
-            logger.error("Non-JSON encoded appointment passed as parameter")
+        logger.error(str(e))
+    except (InvalidKey, InvalidParameter, TowerResponseError) as e:
+        logger.error(e.reason, **e.params)
+    except Exception as e:
+        logger.error("Unknown error occurred", error=str(e))
 
 
 if __name__ == "__main__":
@@ -518,7 +496,13 @@ if __name__ == "__main__":
             if opt in ["-h", "--help"]:
                 sys.exit(show_usage())
 
-        main(args, command_line_conf)
+        command = args.pop(0)
+        if command in commands:
+            main(command, args, command_line_conf)
+        elif not command:
+            logger.error("No command provided. Use help to check the list of available commands")
+        else:
+            logger.error("Unknown command. Use help to check the list of available commands")
 
     except GetoptError as e:
         logger.error("{}".format(e))

test/cli/unit/test_teos_cli.py

@@ -1,19 +1,22 @@
 import os
 import json
 import shutil
+import pytest
 import responses
 from binascii import hexlify
 from coincurve import PrivateKey
 from requests.exceptions import ConnectionError, Timeout
 
+from common.blob import Blob
 import common.cryptographer
 from common.logger import Logger
 from common.tools import compute_locator
 from common.appointment import Appointment
 from common.cryptographer import Cryptographer
 
-from common.blob import Blob
 import cli.teos_cli as teos_cli
+from cli.exceptions import InvalidParameter, InvalidKey, TowerResponseError
+
 from test.cli.unit.conftest import get_random_value_hex, get_config
 
 common.cryptographer.logger = Logger(actor="Cryptographer", log_name_prefix=teos_cli.LOG_PREFIX)
@@ -85,9 +88,7 @@ def test_add_appointment():
         "available_slots": 100,
     }
     responses.add(responses.POST, add_appointment_endpoint, json=response, status=200)
-    result = teos_cli.add_appointment(
+    result = teos_cli.add_appointment(dummy_appointment_data, dummy_cli_sk, dummy_teos_pk, teos_url)
-        dummy_appointment_data, dummy_cli_sk, dummy_teos_pk, teos_url, config.get("APPOINTMENTS_FOLDER_NAME")
-    )
 
     assert len(responses.calls) == 1
     assert responses.calls[0].request.url == add_appointment_endpoint
@@ -106,13 +107,9 @@ def test_add_appointment_with_invalid_signature(monkeypatch):
     }
 
     responses.add(responses.POST, add_appointment_endpoint, json=response, status=200)
-    result = teos_cli.add_appointment(
-        dummy_appointment_data, dummy_cli_sk, dummy_teos_pk, teos_url, config.get("APPOINTMENTS_FOLDER_NAME")
-    )
 
-    assert result is False
+    with pytest.raises(TowerResponseError):
-
+        teos_cli.add_appointment(dummy_appointment_data, dummy_cli_sk, dummy_teos_pk, teos_url)
-    shutil.rmtree(config.get("APPOINTMENTS_FOLDER_NAME"))
 
 
 @responses.activate
@@ -139,7 +136,8 @@ def test_get_appointment_err():
     # Test that get_appointment handles a connection error appropriately.
     responses.add(responses.POST, get_appointment_endpoint, body=ConnectionError())
 
-    assert not teos_cli.get_appointment(locator, dummy_cli_sk, dummy_teos_pk, teos_url)
+    with pytest.raises(ConnectionError):
+        teos_cli.get_appointment(locator, dummy_cli_sk, dummy_teos_pk, teos_url)
 
 
 def test_load_keys():
@@ -151,7 +149,7 @@ def test_load_keys():
         f.write(dummy_cli_sk.to_der())
     with open(public_key_file_path, "wb") as f:
         f.write(dummy_cli_compressed_pk)
-    with open(empty_file_path, "wb") as f:
+    with open(empty_file_path, "wb"):
         pass
 
     # Now we can test the function passing the using this files (we'll use the same pk for both)
@@ -159,25 +157,32 @@ def test_load_keys():
     assert isinstance(r, tuple)
     assert len(r) == 3
 
-    # If any param does not match we should get None as result
+    # If any param does not match the expected, we should get an InvalidKey exception
-    assert teos_cli.load_keys(None, private_key_file_path, public_key_file_path) is None
+    with pytest.raises(InvalidKey):
-    assert teos_cli.load_keys(public_key_file_path, None, public_key_file_path) is None
+        teos_cli.load_keys(None, private_key_file_path, public_key_file_path)
-    assert teos_cli.load_keys(public_key_file_path, private_key_file_path, None) is None
+    with pytest.raises(InvalidKey):
+        teos_cli.load_keys(public_key_file_path, None, public_key_file_path)
+    with pytest.raises(InvalidKey):
+        teos_cli.load_keys(public_key_file_path, private_key_file_path, None)
 
     # The same should happen if we pass a public key where a private should be, for instance
-    assert teos_cli.load_keys(private_key_file_path, public_key_file_path, private_key_file_path) is None
+    with pytest.raises(InvalidKey):
+        teos_cli.load_keys(private_key_file_path, public_key_file_path, private_key_file_path)
 
     # Same if any of the files is empty
-    assert teos_cli.load_keys(empty_file_path, private_key_file_path, public_key_file_path) is None
+    with pytest.raises(InvalidKey):
-    assert teos_cli.load_keys(public_key_file_path, empty_file_path, public_key_file_path) is None
+        teos_cli.load_keys(empty_file_path, private_key_file_path, public_key_file_path)
-    assert teos_cli.load_keys(public_key_file_path, private_key_file_path, empty_file_path) is None
+    with pytest.raises(InvalidKey):
+        teos_cli.load_keys(public_key_file_path, empty_file_path, public_key_file_path)
+    with pytest.raises(InvalidKey):
+        teos_cli.load_keys(public_key_file_path, private_key_file_path, empty_file_path)
 
+    # Remove the tmp files
     os.remove(private_key_file_path)
     os.remove(public_key_file_path)
     os.remove(empty_file_path)
 
 
-# WIP: HERE
 @responses.activate
 def test_post_request():
     response = {
@@ -208,24 +213,23 @@ def test_process_post_response():
 
     # If we modify the response code for a rejection (lets say 404) we should get None
     responses.replace(responses.POST, add_appointment_endpoint, json=response, status=404)
+    with pytest.raises(TowerResponseError):
         r = teos_cli.post_request(json.dumps(dummy_appointment_data), add_appointment_endpoint)
-    assert teos_cli.process_post_response(r) is None
+        teos_cli.process_post_response(r)
 
-    # The same should happen if the response is not in json
+    # The same should happen if the response is not in json independently of the return type
     responses.replace(responses.POST, add_appointment_endpoint, status=404)
+    with pytest.raises(TowerResponseError):
         r = teos_cli.post_request(json.dumps(dummy_appointment_data), add_appointment_endpoint)
-    assert teos_cli.process_post_response(r) is None
+        teos_cli.process_post_response(r)
+
+    responses.replace(responses.POST, add_appointment_endpoint, status=200)
+    with pytest.raises(TowerResponseError):
+        r = teos_cli.post_request(json.dumps(dummy_appointment_data), add_appointment_endpoint)
+        teos_cli.process_post_response(r)
 
 
 def test_parse_add_appointment_args():
-    # If no args are passed, function should fail.
-    appt_data = teos_cli.parse_add_appointment_args(None)
-    assert not appt_data
-
-    # If file doesn't exist, function should fail.
-    appt_data = teos_cli.parse_add_appointment_args(["-f", "nonexistent_file"])
-    assert not appt_data
-
     # If file exists and has data in it, function should work.
     with open("appt_test_file", "w") as f:
         json.dump(dummy_appointment_data, f)
@@ -233,12 +237,22 @@ def test_parse_add_appointment_args():
     appt_data = teos_cli.parse_add_appointment_args(["-f", "appt_test_file"])
     assert appt_data
 
-    os.remove("appt_test_file")
-
     # If appointment json is passed in, function should work.
     appt_data = teos_cli.parse_add_appointment_args([json.dumps(dummy_appointment_data)])
     assert appt_data
 
+    os.remove("appt_test_file")
+
+
+def test_parse_add_appointment_args_wrong():
+    # If no args are passed, function should fail.
+    with pytest.raises(InvalidParameter):
+        teos_cli.parse_add_appointment_args(None)
+
+    # If file doesn't exist, function should fail.
+    with pytest.raises(FileNotFoundError):
+        teos_cli.parse_add_appointment_args(["-f", "nonexistent_file"])
+
 
 def test_save_appointment_receipt(monkeypatch):
     appointments_folder = "test_appointments_receipts"

test/teos/e2e/conftest.py

@@ -41,13 +41,16 @@ def setup_node(bitcoin_cli):
     bitcoin_cli.generatetoaddress(106, new_addr)
 
 
-@pytest.fixture()
+def create_txs(bitcoin_cli, n=1):
-def create_txs(bitcoin_cli):
     utxos = bitcoin_cli.listunspent()
 
-    if len(utxos) == 0:
+    if len(utxos) < n:
-        raise ValueError("There're no UTXOs.")
+        raise ValueError("There're no enough UTXOs.")
 
+    signed_commitment_txs = []
+    signed_penalty_txs = []
+
+    for _ in range(n):
         utxo = utxos.pop(0)
         while utxo.get("amount") < Decimal(2 / pow(10, 5)):
             utxo = utxos.pop(0)
@@ -57,32 +60,13 @@ def create_txs(bitcoin_cli):
 
         signed_penalty_tx = create_penalty_tx(bitcoin_cli, decoded_commitment_tx)
 
-    return signed_commitment_tx, signed_penalty_tx
-
-
-@pytest.fixture()
-def create_five_txs(bitcoin_cli):
-    utxos = bitcoin_cli.listunspent()
-
-    signed_commitment_txs = []
-    signed_penalty_txs = []
-
-    for i in range(5):
-        if len(utxos) == 0:
-            raise ValueError("There're no UTXOs.")
-
-        utxo = utxos.pop(0)
-        while utxo.get("amount") < Decimal(2 / pow(10, 5)):
-            utxo = utxos.pop(0)
-
-        signed_commitment_tx = create_commitment_tx(bitcoin_cli, utxo)
-
         signed_commitment_txs.append(signed_commitment_tx)
-        decoded_commitment_tx = bitcoin_cli.decoderawtransaction(signed_commitment_tx)
+        signed_penalty_txs.append(signed_penalty_tx)
-
-        signed_penalty_txs.append(create_penalty_tx(bitcoin_cli, decoded_commitment_tx))
 
+    if len(signed_penalty_txs) > 1:
         return signed_commitment_txs, signed_penalty_txs
+    else:
+        return signed_commitment_txs[0], signed_penalty_txs[0]
 
 
 def run_teosd():

test/teos/e2e/test_basic_e2e.py

@@ -1,9 +1,11 @@
 import json
+import pytest
 from time import sleep
 from riemann.tx import Tx
 from binascii import hexlify
 from coincurve import PrivateKey
 
+from cli.exceptions import TowerResponseError
 from cli import teos_cli, DATA_DIR, DEFAULT_CONF, CONF_FILE_NAME
 
 import common.cryptographer
@@ -20,6 +22,7 @@ from test.teos.e2e.conftest import (
     create_penalty_tx,
     run_teosd,
     get_config,
+    create_txs,
 )
 
 cli_config = get_config(DATA_DIR, CONF_FILE_NAME, DEFAULT_CONF)
@@ -50,9 +53,7 @@ def get_appointment_info(locator, sk=cli_sk):
 
 
 def add_appointment(appointment_data, sk=cli_sk):
-    return teos_cli.add_appointment(
+    return teos_cli.add_appointment(appointment_data, sk, teos_pk, teos_base_endpoint)
-        appointment_data, sk, teos_pk, teos_base_endpoint, cli_config.get("APPOINTMENTS_FOLDER_NAME")
-    )
 
 
 def get_all_appointments():
@@ -60,65 +61,72 @@ def get_all_appointments():
     return json.loads(r)
 
 
-def test_commands_non_registered(bitcoin_cli, create_txs):
+def test_commands_non_registered(bitcoin_cli):
     # All commands should fail if the user is not registered
 
     # Add appointment
-    commitment_tx, penalty_tx = create_txs
+    commitment_tx, penalty_tx = create_txs(bitcoin_cli)
     commitment_tx_id = bitcoin_cli.decoderawtransaction(commitment_tx).get("txid")
     appointment_data = build_appointment_data(bitcoin_cli, commitment_tx_id, penalty_tx)
 
-    assert add_appointment(appointment_data) is False
+    with pytest.raises(TowerResponseError):
+        assert add_appointment(appointment_data)
 
     # Get appointment
-    assert get_appointment_info(appointment_data.get("locator")) is None
+    with pytest.raises(TowerResponseError):
+        assert get_appointment_info(appointment_data.get("locator"))
 
 
-def test_commands_registered(bitcoin_cli, create_txs):
+def test_commands_registered(bitcoin_cli):
     # Test registering and trying again
     teos_cli.register(compressed_cli_pk, teos_base_endpoint)
 
     # Add appointment
-    commitment_tx, penalty_tx = create_txs
+    commitment_tx, penalty_tx = create_txs(bitcoin_cli)
     commitment_tx_id = bitcoin_cli.decoderawtransaction(commitment_tx).get("txid")
     appointment_data = build_appointment_data(bitcoin_cli, commitment_tx_id, penalty_tx)
 
-    assert add_appointment(appointment_data) is True
+    appointment, available_slots = add_appointment(appointment_data)
+    assert isinstance(appointment, Appointment) and isinstance(available_slots, str)
 
     # Get appointment
     r = get_appointment_info(appointment_data.get("locator"))
-    assert r.get("locator") == appointment_data.get("locator")
+    assert r.get("locator") == appointment.locator
-    assert r.get("appointment").get("locator") == appointment_data.get("locator")
+    assert r.get("appointment") == appointment.to_dict()
-    assert r.get("appointment").get("encrypted_blob") == appointment_data.get("encrypted_blob")
-    assert r.get("appointment").get("start_time") == appointment_data.get("start_time")
-    assert r.get("appointment").get("end_time") == appointment_data.get("end_time")
 
 
-def test_appointment_life_cycle(bitcoin_cli, create_txs):
+def test_appointment_life_cycle(bitcoin_cli):
     # First of all we need to register
-    # FIXME: requires register command in the cli
+    teos_cli.register(compressed_cli_pk, teos_base_endpoint)
-    commitment_tx, penalty_tx = create_txs
+
+    # After that we can build an appointment and send it to the tower
+    commitment_tx, penalty_tx = create_txs(bitcoin_cli)
     commitment_tx_id = bitcoin_cli.decoderawtransaction(commitment_tx).get("txid")
     appointment_data = build_appointment_data(bitcoin_cli, commitment_tx_id, penalty_tx)
     locator = compute_locator(commitment_tx_id)
+    appointment, available_slots = add_appointment(appointment_data)
 
-    assert add_appointment(appointment_data) is True
+    # Get the information from the tower to check that it matches
-
     appointment_info = get_appointment_info(locator)
     assert appointment_info is not None
     assert appointment_info.get("status") == "being_watched"
+    assert appointment_info.get("locator") == locator
+    assert appointment_info.get("appointment") == appointment.to_dict()
 
+    # Check also the get_all_appointment endpoint
     all_appointments = get_all_appointments()
     watching = all_appointments.get("watcher_appointments")
     responding = all_appointments.get("responder_trackers")
     assert len(watching) == 1 and len(responding) == 0
 
+    # Trigger a breach and check again
     new_addr = bitcoin_cli.getnewaddress()
     broadcast_transaction_and_mine_block(bitcoin_cli, commitment_tx, new_addr)
 
     appointment_info = get_appointment_info(locator)
     assert appointment_info is not None
     assert appointment_info.get("status") == "dispute_responded"
+    assert appointment_info.get("locator") == locator
 
     all_appointments = get_all_appointments()
     watching = all_appointments.get("watcher_appointments")
@@ -137,29 +145,29 @@ def test_appointment_life_cycle(bitcoin_cli, create_txs):
         assert False
 
     # Now let's mine some blocks so the appointment reaches its end.
-    # Since we are running all the nodes remotely data may take more time than normal, and some confirmations may be
+    for _ in range(END_TIME_DELTA):
-    # missed, so we generate more than enough confirmations and add some delays.
-    for _ in range(int(1.5 * END_TIME_DELTA)):
-        sleep(1)
         bitcoin_cli.generatetoaddress(1, new_addr)
 
-    assert get_appointment_info(locator) is None
+    # The appointment is no longer in the tower
+    with pytest.raises(TowerResponseError):
+        get_appointment_info(locator)
 
 
-def test_multiple_appointments_life_cycle(bitcoin_cli, create_five_txs):
+def test_multiple_appointments_life_cycle(bitcoin_cli):
-    # Tests that get_all_appointments returns all the appointments the tower is storing at various stages in the appointment lifecycle.
+    # Tests that get_all_appointments returns all the appointments the tower is storing at various stages in the
+    # appointment lifecycle.
     appointments = []
 
-    commitment_txs, penalty_txs = create_five_txs
+    commitment_txs, penalty_txs = create_txs(bitcoin_cli, n=5)
 
     # Create five appointments.
-    for i in range(5):
+    for commitment_tx, penalty_tx in zip(commitment_txs, penalty_txs):
         appointment = {}
 
-        appointment["commitment_tx"] = commitment_txs[i]
+        appointment["commitment_tx"] = commitment_tx
-        appointment["penalty_tx"] = penalty_txs[i]
+        appointment["penalty_tx"] = penalty_tx
-        commitment_tx_id = bitcoin_cli.decoderawtransaction(commitment_txs[i]).get("txid")
+        commitment_tx_id = bitcoin_cli.decoderawtransaction(commitment_tx).get("txid")
-        appointment_data = build_appointment_data(bitcoin_cli, commitment_tx_id, penalty_txs[i])
+        appointment_data = build_appointment_data(bitcoin_cli, commitment_tx_id, penalty_tx)
         appointment["appointment_data"] = appointment_data
         locator = compute_locator(commitment_tx_id)
         appointment["locator"] = locator
@@ -171,10 +179,12 @@ def test_multiple_appointments_life_cycle(bitcoin_cli, create_five_txs):
         add_appointment(appt.get("appointment_data"))
 
     # Two of these appointments are breached, and the watchtower responds to them.
+    breached_appointments = []
     for i in range(2):
         new_addr = bitcoin_cli.getnewaddress()
         broadcast_transaction_and_mine_block(bitcoin_cli, appointments[i]["commitment_tx"], new_addr)
-        bitcoin_cli.generatetoaddress(3, new_addr)
+        bitcoin_cli.generatetoaddress(1, new_addr)
+        breached_appointments.append(appointments[i]["locator"])
         sleep(1)
 
     # Test that they all show up in get_all_appointments at the correct stages.
@@ -182,6 +192,8 @@ def test_multiple_appointments_life_cycle(bitcoin_cli, create_five_txs):
     watching = all_appointments.get("watcher_appointments")
     responding = all_appointments.get("responder_trackers")
     assert len(watching) == 3 and len(responding) == 2
+    responder_locators = [appointment["locator"] for uuid, appointment in responding.items()]
+    assert set(responder_locators) == set(breached_appointments)
 
     # Now let's mine some blocks so these appointments reach the end of their lifecycle.
     # Since we are running all the nodes remotely data may take more time than normal, and some confirmations may be
@@ -192,9 +204,9 @@ def test_multiple_appointments_life_cycle(bitcoin_cli, create_five_txs):
         bitcoin_cli.generatetoaddress(1, new_addr)
 
 
-def test_appointment_malformed_penalty(bitcoin_cli, create_txs):
+def test_appointment_malformed_penalty(bitcoin_cli):
     # Lets start by creating two valid transaction
-    commitment_tx, penalty_tx = create_txs
+    commitment_tx, penalty_tx = create_txs(bitcoin_cli)
 
     # Now we can modify the penalty so it is invalid when broadcast
     mod_penalty_tx = Tx.from_hex(penalty_tx)
@@ -205,21 +217,27 @@ def test_appointment_malformed_penalty(bitcoin_cli, create_txs):
     appointment_data = build_appointment_data(bitcoin_cli, commitment_tx_id, mod_penalty_tx.hex())
     locator = compute_locator(commitment_tx_id)
 
-    assert add_appointment(appointment_data) is True
+    appointment, _ = add_appointment(appointment_data)
+
+    # Get the information from the tower to check that it matches
+    appointment_info = get_appointment_info(locator)
+    assert appointment_info.get("status") == "being_watched"
+    assert appointment_info.get("locator") == locator
+    assert appointment_info.get("appointment") == appointment.to_dict()
 
     # Broadcast the commitment transaction and mine a block
     new_addr = bitcoin_cli.getnewaddress()
     broadcast_transaction_and_mine_block(bitcoin_cli, commitment_tx, new_addr)
 
     # The appointment should have been removed since the penalty_tx was malformed.
-    sleep(1)
+    with pytest.raises(TowerResponseError):
-    assert get_appointment_info(locator) is None
+        get_appointment_info(locator)
 
 
-def test_appointment_wrong_key(bitcoin_cli, create_txs):
+def test_appointment_wrong_decryption_key(bitcoin_cli):
     # This tests an appointment encrypted with a key that has not been derived from the same source as the locator.
     # Therefore the tower won't be able to decrypt the blob once the appointment is triggered.
-    commitment_tx, penalty_tx = create_txs
+    commitment_tx, penalty_tx = create_txs(bitcoin_cli)
 
     # The appointment data is built using a random 32-byte value.
     appointment_data = build_appointment_data(bitcoin_cli, get_random_value_hex(32), penalty_tx)
@@ -239,7 +257,6 @@ def test_appointment_wrong_key(bitcoin_cli, create_txs):
 
     # Check that the server has accepted the appointment
     signature = response_json.get("signature")
-    assert signature is not None
     rpk = Cryptographer.recover_pk(appointment.serialize(), signature)
     assert Cryptographer.verify_rpk(teos_pk, rpk) is True
     assert response_json.get("locator") == appointment.locator
@@ -249,45 +266,43 @@ def test_appointment_wrong_key(bitcoin_cli, create_txs):
     broadcast_transaction_and_mine_block(bitcoin_cli, commitment_tx, new_addr)
 
     # The appointment should have been removed since the decryption failed.
-    sleep(1)
+    with pytest.raises(TowerResponseError):
-    assert get_appointment_info(appointment.locator) is None
+        get_appointment_info(appointment.locator)
 
 
-def test_two_identical_appointments(bitcoin_cli, create_txs):
+def test_two_identical_appointments(bitcoin_cli):
     # Tests sending two identical appointments to the tower.
     # This tests sending an appointment with two valid transaction with the same locator.
-    # If they come from the same user, the last one will be kept
+    # If they come from the same user, the last one will be kept.
-    commitment_tx, penalty_tx = create_txs
+    commitment_tx, penalty_tx = create_txs(bitcoin_cli)
     commitment_tx_id = bitcoin_cli.decoderawtransaction(commitment_tx).get("txid")
 
     appointment_data = build_appointment_data(bitcoin_cli, commitment_tx_id, penalty_tx)
     locator = compute_locator(commitment_tx_id)
 
     # Send the appointment twice
-    assert add_appointment(appointment_data) is True
+    add_appointment(appointment_data)
-    assert add_appointment(appointment_data) is True
+    add_appointment(appointment_data)
 
     # Broadcast the commitment transaction and mine a block
     new_addr = bitcoin_cli.getnewaddress()
     broadcast_transaction_and_mine_block(bitcoin_cli, commitment_tx, new_addr)
 
     # The last appointment should have made it to the Responder
-    sleep(1)
     appointment_info = get_appointment_info(locator)
 
-    assert appointment_info is not None
     assert appointment_info.get("status") == "dispute_responded"
     assert appointment_info.get("appointment").get("penalty_rawtx") == penalty_tx
 
 
 # FIXME: This test won't work since we're still passing appointment replicas to the Responder.
 #        Uncomment when #88 is addressed
-# def test_two_identical_appointments_different_users(bitcoin_cli, create_txs):
+# def test_two_identical_appointments_different_users(bitcoin_cli):
 #     # Tests sending two identical appointments from different users to the tower.
 #     # This tests sending an appointment with two valid transaction with the same locator.
 #     # If they come from different users, both will be kept, but one will be dropped fro double-spending when passing to
 #     # the responder
-#     commitment_tx, penalty_tx = create_txs
+#     commitment_tx, penalty_tx = create_txs(bitcoin_cli)
 #     commitment_tx_id = bitcoin_cli.decoderawtransaction(commitment_tx).get("txid")
 #
 #     appointment_data = build_appointment_data(bitcoin_cli, commitment_tx_id, penalty_tx)
@@ -328,9 +343,9 @@ def test_two_identical_appointments(bitcoin_cli, create_txs):
 #     assert appointment_info.get("appointment").get("penalty_rawtx") == penalty_tx
 
 
-def test_two_appointment_same_locator_different_penalty_different_users(bitcoin_cli, create_txs):
+def test_two_appointment_same_locator_different_penalty_different_users(bitcoin_cli):
-    # This tests sending an appointment with two valid transaction with the same locator.
+    # This tests sending an appointment with two valid transaction with the same locator fro different users
-    commitment_tx, penalty_tx1 = create_txs
+    commitment_tx, penalty_tx1 = create_txs(bitcoin_cli)
     commitment_tx_id = bitcoin_cli.decoderawtransaction(commitment_tx).get("txid")
 
     # We need to create a second penalty spending from the same commitment
@@ -342,48 +357,45 @@ def test_two_appointment_same_locator_different_penalty_different_users(bitcoin_
     appointment2_data = build_appointment_data(bitcoin_cli, commitment_tx_id, penalty_tx2)
     locator = compute_locator(commitment_tx_id)
 
-    # tmp keys from a different user
+    # tmp keys for a different user
     tmp_sk = PrivateKey()
     tmp_compressed_pk = hexlify(tmp_sk.public_key.format(compressed=True)).decode("utf-8")
     teos_cli.register(tmp_compressed_pk, teos_base_endpoint)
 
-    assert add_appointment(appointment1_data) is True
+    appointment, _ = add_appointment(appointment1_data)
-    assert add_appointment(appointment2_data, sk=tmp_sk) is True
+    appointment_2, _ = add_appointment(appointment2_data, sk=tmp_sk)
 
     # Broadcast the commitment transaction and mine a block
     new_addr = bitcoin_cli.getnewaddress()
     broadcast_transaction_and_mine_block(bitcoin_cli, commitment_tx, new_addr)
 
     # One of the transactions must have made it to the Responder while the other must have been dropped for
-    # double-spending
+    # double-spending. That means that one of the responses from the tower should fail
-    sleep(1)
+    appointment_info = None
+    with pytest.raises(TowerResponseError):
         appointment_info = get_appointment_info(locator)
         appointment2_info = get_appointment_info(locator, sk=tmp_sk)
 
-    # One of the two request must be None, while the other must be valid
-    assert (appointment_info is None and appointment2_info is not None) or (
-        appointment2_info is None and appointment_info is not None
-    )
-
     if appointment_info is None:
         appointment_info = appointment2_info
         appointment1_data = appointment2_data
 
     assert appointment_info.get("status") == "dispute_responded"
     assert appointment_info.get("locator") == appointment1_data.get("locator")
+    assert appointment_info.get("appointment").get("penalty_tx") == appointment1_data.get("penalty_tx")
 
 
-def test_appointment_shutdown_teos_trigger_back_online(create_txs, bitcoin_cli):
+def test_appointment_shutdown_teos_trigger_back_online(bitcoin_cli):
     global teosd_process
 
     teos_pid = teosd_process.pid
 
-    commitment_tx, penalty_tx = create_txs
+    commitment_tx, penalty_tx = create_txs(bitcoin_cli)
     commitment_tx_id = bitcoin_cli.decoderawtransaction(commitment_tx).get("txid")
     appointment_data = build_appointment_data(bitcoin_cli, commitment_tx_id, penalty_tx)
     locator = compute_locator(commitment_tx_id)
 
-    assert add_appointment(appointment_data) is True
+    appointment, _ = add_appointment(appointment_data)
 
     # Restart teos
     teosd_process.terminate()
@@ -392,40 +404,36 @@ def test_appointment_shutdown_teos_trigger_back_online(create_txs, bitcoin_cli):
     assert teos_pid != teosd_process.pid
 
     # Check that the appointment is still in the Watcher
-    sleep(1)
     appointment_info = get_appointment_info(locator)
 
-    assert appointment_info is not None
     assert appointment_info.get("status") == "being_watched"
+    assert appointment_info.get("appointment") == appointment.to_dict()
 
     # Trigger appointment after restart
     new_addr = bitcoin_cli.getnewaddress()
     broadcast_transaction_and_mine_block(bitcoin_cli, commitment_tx, new_addr)
 
     # The appointment should have been moved to the Responder
-    sleep(1)
     appointment_info = get_appointment_info(locator)
-
-    assert appointment_info is not None
     assert appointment_info.get("status") == "dispute_responded"
 
 
-def test_appointment_shutdown_teos_trigger_while_offline(create_txs, bitcoin_cli):
+def test_appointment_shutdown_teos_trigger_while_offline(bitcoin_cli):
     global teosd_process
 
     teos_pid = teosd_process.pid
 
-    commitment_tx, penalty_tx = create_txs
+    commitment_tx, penalty_tx = create_txs(bitcoin_cli)
     commitment_tx_id = bitcoin_cli.decoderawtransaction(commitment_tx).get("txid")
     appointment_data = build_appointment_data(bitcoin_cli, commitment_tx_id, penalty_tx)
     locator = compute_locator(commitment_tx_id)
 
-    assert add_appointment(appointment_data) is True
+    appointment, _ = add_appointment(appointment_data)
 
     # Check that the appointment is still in the Watcher
     appointment_info = get_appointment_info(locator)
-    assert appointment_info is not None
     assert appointment_info.get("status") == "being_watched"
+    assert appointment_info.get("appointment") == appointment.to_dict()
 
     # Shutdown and trigger
     teosd_process.terminate()
@@ -437,10 +445,7 @@ def test_appointment_shutdown_teos_trigger_while_offline(create_txs, bitcoin_cli
     assert teos_pid != teosd_process.pid
 
     # The appointment should have been moved to the Responder
-    sleep(1)
     appointment_info = get_appointment_info(locator)
-
-    assert appointment_info is not None
     assert appointment_info.get("status") == "dispute_responded"
 
     teosd_process.terminate()