Bugfix: Updating the auto-withdrawal in Swan integration could lead to an error (#1981)

moneymanolis · k9ert · web-flow · commit 48bccfa1265a · 2022-11-22T21:07:36.000+01:00
* if enough addresses reserved return an empty list + tests

* client test fixed + test for reserve_addresses for swan service

* ignore_cleanup_errors=True + dev doc updated

* fix select of threshold in settings

* test for set_autowithdrawal_settings

* fix test

Co-authored-by: k9ert &lt;kim@swanbitcoin.com&gt;
diff --git a/docs/development.md b/docs/development.md
@@ -150,6 +150,9 @@ Set up the dependencies:
 pip3 install -r test_requirements.txt
 pip3 install -e .
 ```
+  
+You need a virtual environment based on Python 3.10 for the tests to run successfully, otherwise you get this error:
+`TypeError: __init__() got an unexpected keyword argument 'ignore_cleanup_errors'`
 
 If you have a local bitcoind already installed:
 ```
diff --git a/src/cryptoadvance/specter/services/service.py b/src/cryptoadvance/specter/services/service.py
@@ -129,6 +129,7 @@ def reserve_addresses(
         )
         logger.debug(f"Already have {len(addresses)} addresses reserved for {cls.id}")
 
+        # More addresses ought to be reserved as there are reserved addresses
         if len(addresses) < num_addresses:
             if addresses:
                 # Continuing reserving from where we left off
@@ -149,7 +150,7 @@ def reserve_addresses(
                     continue
 
                 # Mark an Address in a persistent way as being reserved by a Service
-                cls.reserve_address(wallet=wallet, address=address)
+                cls.reserve_address(wallet=wallet, address=address, label=label)
                 logger.debug(f"Reserved {address} for {cls.id}")
 
                 addresses.append(address)
@@ -158,9 +159,18 @@ def reserve_addresses(
                     annotations_storage.set_addr_annotations(
                         addr=address, annotations=annotations, autosave=False
                     )
+        # There are enough addresses already reserved
+        else:
+            logger.debug(
+                f"Returning an empty list from reserve_addresses() since there are enough addresses already reserved."
+            )
+            return []
+
         if annotations:
             annotations_storage.save()
-
+        logger.debug(
+            f"Returning this list of addresses {addresses} from reserve_addresses()."
+        )
         return addresses
 
     @classmethod
diff --git a/src/cryptoadvance/specter/wallet.py b/src/cryptoadvance/specter/wallet.py
@@ -628,7 +628,8 @@ def update(self):
         self.check_addresses()
 
     def check_unused(self):
-        """Check current receive address is unused and get new if needed"""
+        """Check current receive address is unused and get new if needed
+        Used means: the given address having a non-zero amount received in transactions with zero confirmations."""
         addr = self.address
         try:
             while self.rpc.getreceivedbyaddress(addr, 0) != 0:
diff --git a/src/cryptoadvance/specterext/swan/client.py b/src/cryptoadvance/specterext/swan/client.py
@@ -238,17 +238,13 @@ def update_autowithdrawal_addresses(
         specter_wallet_name: str,
         specter_wallet_alias: str,
         addresses: List[str],
-    ) -> dict:
+    ) -> str:
         """
         * If SWAN_WALLET_ID is known, any existing unused addresses are cleared.
         * If there is no known SWAN_WALLET_ID, we `POST` to create an initial Swan wallet and store the resulting SWAN_WALLET_ID.
         * Sends the list of new addresses for SWAN_WALLET_ID.
+        * Returns the swan wallet id if the response provides it or raises an SwanApiException.
         """
-
-        # normalize the strucure compatible with what swan will accept:
-        # like: [{"address": "bcrt1q8k8a73crvjs06jhdj7xee8mace3mhlxj4pdvna"}, {"address": "bcrt ...
-        addresses = [address["address"] for address in addresses]
-
         if swan_wallet_id:
             # We already have a Swan walletId. DELETE the existing unused addresses...
             self.delete_autowithdrawal_addresses(swan_wallet_id)
@@ -261,6 +257,7 @@ def update_autowithdrawal_addresses(
             endpoint = "/apps/v20210824/wallets"
             method = "POST"
 
+        # For the required structure of the paylod see: https://developers.swanbitcoin.com/api/create-a-new-wallet
         resp = self.authenticated_request(
             endpoint=endpoint,
             method=method,
diff --git a/src/cryptoadvance/specterext/swan/controller.py b/src/cryptoadvance/specterext/swan/controller.py
@@ -102,6 +102,9 @@ def settings():
         wallets=wallets,
         cookies=request.cookies,
         num_reserved_addrs=SwanService.MIN_PENDING_AUTOWITHDRAWAL_ADDRS,
+        autowithdrawal_threshold=SwanService.get_current_user_service_data().get(
+            SwanService.AUTOWITHDRAWAL_THRESHOLD
+        ),
     )
 
 
diff --git a/src/cryptoadvance/specterext/swan/service.py b/src/cryptoadvance/specterext/swan/service.py
@@ -111,7 +111,7 @@ def set_associated_wallet(cls, wallet: Wallet):
     @classmethod
     def reserve_addresses(
         cls, wallet: Wallet, label: str = None, num_addresses: int = 10
-    ) -> List[str]:
+    ):
         """
         * Reserves addresses for Swan auto-withdrawals
         * Sets the associated Specter `Wallet` that will receive auto-withdrawals
@@ -123,9 +123,14 @@ def reserve_addresses(
         from . import client as swan_client
 
         # Update Addresses as reserved (aka "associated") with Swan in our Wallet
+        # addresses is list of address strings, like so: ["bcrt1qxak08ykhf7r4js9yncysy5p05xp0fwxhamewc8", ... , "bcrt1qpys58dndrn9sxnk0z7ngm6wsxskpvs9jsjq7q6"]
+        # or it is an empty list if we are requesting to reserve less addresses as we already have
         addresses = super().reserve_addresses(
             wallet=wallet, label=label, num_addresses=num_addresses
         )
+        logger.debug(
+            f"The addresses that go in as arguments to update_autowithdrawal_addresses: {addresses}"
+        )
 
         # Clear out any prior unused reserved addresses if this is a different Wallet
         cur_wallet = cls.get_associated_wallet()
@@ -136,17 +141,19 @@ def reserve_addresses(
         cls.set_associated_wallet(wallet)
 
         # Send the new list to Swan (DELETES any unused ones; creates a new SWAN_WALLET_ID if needed)
-        swan_wallet_id = cls.client().update_autowithdrawal_addresses(
-            cls.get_current_user_service_data().get(cls.SWAN_WALLET_ID),
-            specter_wallet_name=wallet.name,
-            specter_wallet_alias=wallet.alias,
-            addresses=addresses,
-        )
-        logger.debug(f"Updating the Swan wallet id to {swan_wallet_id}")
-        if swan_wallet_id:
-            cls.update_current_user_service_data({cls.SWAN_WALLET_ID: swan_wallet_id})
-
-        return addresses
+        # Only do this if we are requesting to reserve less addresses as we already have
+        if addresses != []:
+            swan_wallet_id = cls.client().update_autowithdrawal_addresses(
+                cls.get_current_user_service_data().get(cls.SWAN_WALLET_ID),
+                specter_wallet_name=wallet.name,
+                specter_wallet_alias=wallet.alias,
+                addresses=addresses,
+            )
+            logger.debug(f"Updating the Swan wallet id to {swan_wallet_id}")
+            if swan_wallet_id:
+                cls.update_current_user_service_data(
+                    {cls.SWAN_WALLET_ID: swan_wallet_id}
+                )
 
     @classmethod
     def set_autowithdrawal_settings(cls, wallet: Wallet, btc_threshold: str):
diff --git a/src/cryptoadvance/specterext/swan/templates/swan/settings.jinja b/src/cryptoadvance/specterext/swan/templates/swan/settings.jinja
@@ -30,10 +30,10 @@
 
             <div>Auto-withdrawal threshold:</div>
             <select name="threshold">
-                <option value="0"     {% if threshold == '0' %}selected{% endif %}>Weekly</option>
-                <option value="0.01"  {% if threshold == '0.01' %}selected{% endif %}>0.010 BTC</option>
-                <option value="0.025" {% if threshold == '0.025' %}selected{% endif %}>0.025 BTC</option>
-                <option value="0.05"  {% if threshold == '0.05' %}selected{% endif %}>0.050 BTC</option>
+                <option value="0"     {% if autowithdrawal_threshold == '0' %}selected{% endif %}>Weekly</option>
+                <option value="0.01"  {% if autowithdrawal_threshold == '0.01' %}selected{% endif %}>0.010 BTC</option>
+                <option value="0.025" {% if autowithdrawal_threshold == '0.025' %}selected{% endif %}>0.025 BTC</option>
+                <option value="0.05"  {% if autowithdrawal_threshold == '0.05' %}selected{% endif %}>0.050 BTC</option>
             </select>
             <br/>
             <br/>
diff --git a/tests/conftest.py b/tests/conftest.py
@@ -35,7 +35,7 @@
 logger = logging.getLogger(__name__)
 
 pytest_plugins = [
-    # "conftest_visibility",
+    "conftest_visibility",
     "fix_ghost_machine",
     "fix_keys_and_seeds",
     "fix_devices_and_wallets",
@@ -268,13 +268,17 @@ def elements_elreg(request):
 @pytest.fixture
 def empty_data_folder():
     # Make sure that this folder never ever gets a reasonable non-testing use-case
-    with tempfile.TemporaryDirectory(prefix="specter_home_tmp_") as data_folder:
+    with tempfile.TemporaryDirectory(
+        prefix="specter_home_tmp_", ignore_cleanup_errors=False
+    ) as data_folder:
         yield data_folder
 
 
 @pytest.fixture
 def devices_filled_data_folder(empty_data_folder):
-    os.makedirs(empty_data_folder + "/devices")
+    devices_folder = empty_data_folder + "/devices"
+    if not os.path.isdir(devices_folder):
+        os.makedirs(devices_folder)
     with open(empty_data_folder + "/devices/trezor.json", "w") as text_file:
         text_file.write(
             """
diff --git a/tests/conftest_visibility.py b/tests/conftest_visibility.py
@@ -28,6 +28,7 @@ def should_intercept(call):
     return not (
         isinstance(call.excinfo.value, RpcError)
         or isinstance(call.excinfo.value, SpecterError)
+        or isinstance(call.excinfo.value, AssertionError)
     )
 
 
diff --git a/tests/fix_devices_and_wallets.py b/tests/fix_devices_and_wallets.py
@@ -194,6 +194,7 @@ def wallet(devices_filled_data_folder, device_manager, node):
         device_manager,
     )
     device = device_manager.get_by_alias("trezor")
-    wm.create_wallet("test_wallet", 1, "wpkh", [device.keys[5]], [device])
-    wallet = wm.wallets["test_wallet"]
+    wallet_name = f"test_wallet_{random.randint(0, 999999)}"
+    wm.create_wallet(wallet_name, 1, "wpkh", [device.keys[5]], [device])
+    wallet = wm.wallets[wallet_name]
     return wallet
diff --git a/tests/test_services.py b/tests/test_services.py
@@ -373,7 +373,7 @@ def test_both_Storages_in_parallel(empty_data_folder, user1, user2):
 
 
 class TestService(Service):
-    id = "mytestservice"
+    id = "test_service"
 
     @classmethod
     def default_address_label(cls):
@@ -389,7 +389,7 @@ def test_Service_reserve_address(empty_data_folder, caplog):
     assert wallet_mock.associate_address_with_service.assert_called_once
 
 
-def test_Service_reserve_addresses(empty_data_folder, caplog):
+def test_reserve_addresses_with_mocks(empty_data_folder, caplog):
     caplog.set_level(logging.DEBUG)
     specter_mock = MagicMock()
     specter_mock.data_folder = empty_data_folder
@@ -415,3 +415,40 @@ def test_Service_reserve_addresses(empty_data_folder, caplog):
     # taking care that no reserved addresses are handed out.
     assert wallet_mock.address_index == 5
     assert addresses == ["a", "b"]
+
+
+def test_reserve_addresses_with_an_actual_wallet(wallet):
+    specter_mock = MagicMock()
+    test_service = TestService(True, specter_mock)
+    # Reserve first address
+    test_service.reserve_address(
+        wallet, "bcrt1qcatuhg0gll3h7py4cmn53rjjn9xlsqfwj3zcej", "reserved_for_john_nash"
+    )
+    first_address_address_obj = wallet.get_address_obj(
+        "bcrt1qcatuhg0gll3h7py4cmn53rjjn9xlsqfwj3zcej"
+    )
+    # Check that labeling works
+    assert first_address_address_obj["label"] == "reserved_for_john_nash"
+    # Simulating that the address has been used (for the definition of "usage" see the check_unused() method in wallet.py)
+    wallet._addresses.set_used(["bcrt1qcatuhg0gll3h7py4cmn53rjjn9xlsqfwj3zcej"])
+    wallet.getnewaddress()
+    assert wallet.address_index == 1
+    assert first_address_address_obj["used"] == True
+    # Check that the correct addresses are reserved, should be #2, #4, #6 - since #0 has been used and there is a gap of one address in between
+    addresses = test_service.reserve_addresses(wallet, "satoshi_dice", 3)
+    assert addresses == [
+        "bcrt1qxak08ykhf7r4js9yncysy5p05xp0fwxhamewc8",
+        "bcrt1q2zv9963acq3g7a62mdjgj60rr3hgmyykaccca7",
+        "bcrt1qpys58dndrn9sxnk0z7ngm6wsxskpvs9jsjq7q6",
+    ]
+    address_obj_list = wallet.get_associated_addresses("test_service")
+    # Reserving 3 addresses results in an empty list since we already have 3 unused addresses (the first one was used) reserved
+    addresses = test_service.reserve_addresses(wallet, "satoshi_dice", 3)
+    assert addresses == []
+    # Check labeling
+    assert [address_obj["label"] for address_obj in address_obj_list] == [
+        "reserved_for_john_nash",
+        "satoshi_dice",
+        "satoshi_dice",
+        "satoshi_dice",
+    ]
diff --git a/tests/test_specterext_swan_client.py b/tests/test_specterext_swan_client.py
@@ -6,7 +6,13 @@
 import pytest
 import mock
 from mock import Mock, patch
-from cryptoadvance.specterext.swan.client import SwanApiException, SwanClient
+from cryptoadvance.specterext.swan.client import (
+    SwanApiException,
+    SwanApiRefreshTokenException,
+    SwanClient,
+)
+
+logger = logging.getLogger(__name__)
 
 
 def construct_access_token_fake_response():
@@ -79,6 +85,28 @@ def test_SwanClient(app):
             )
 
 
+def test_still_valid_access_token(app):
+    sc = SwanClient(
+        "a_hostname", "forever_valid_access_token", 5000000000, "a_refresh_token"
+    )
+    with app.app_context():
+        fake_response = construct_access_token_fake_response()
+        with mock.patch("requests.post", return_value=fake_response):
+            assert sc.is_access_token_valid() == True
+            assert sc._get_access_token() != "muuuhTheAccessToken"
+            assert sc._get_access_token() == "forever_valid_access_token"
+
+
+def test_expired_access_token():
+    sc = SwanClient("umbrel", "aging_access_token", 1000, "")
+    assert sc.is_access_token_valid() == False
+    with pytest.raises(
+        SwanApiRefreshTokenException,
+        match="access_token is expired but we don't have a refresh_token",
+    ):
+        sc._get_access_token()
+
+
 @patch("requests.delete")
 @patch("requests.request")
 @patch("requests.patch")
@@ -107,44 +135,28 @@ def fake_translate(text):
     sc = SwanClient("a_hostname", "a_access_token", curr_timstamp, "a_refresh_token")
 
     with app.app_context():
-        # Issues with Babel with this test
         address_list = [
-            {
-                "address": "bcrt1q4zcc0yppghquz9tzsd9k34m8rpmvav953hx3mk",
-                "index": 12,
-                "change": False,
-                "label": "Reserved for Swan",
-                "used": None,
-                "service_id": "swan",
-            },
-            {
-                "address": "bcrt1q2lmvfypnqcr7w9vcrlem7vdn7w65ac90x2ef6x",
-                "index": 14,
-                "change": False,
-                "label": "Reserved for Swan",
-                "used": None,
-                "service_id": "swan",
-            },
-            {
-                "address": "bcrt1quazsqywlme8ps70vq7xckflztghypp0r4ck9yw",
-                "index": 16,
-                "change": False,
-                "label": "Reserved for Swan",
-                "used": None,
-                "service_id": "swan",
-            },
+            "bcrt1q4zcc0yppghquz9tzsd9k34m8rpmvav953hx3mk",
+            "bcrt1q2lmvfypnqcr7w9vcrlem7vdn7w65ac90x2ef6x",
+            "bcrt1quazsqywlme8ps70vq7xckflztghypp0r4ck9yw",
         ]
-
         assert (
             sc.update_autowithdrawal_addresses(
                 "someWalletId", "walletName", "walletAlias", address_list
             )
             == "someOtherWalletId"
         )
 
+        # Check that the addresse send to the Swan API are in the correct format
         patch_call: Call = mock_req_request.call_args_list[1]
         json_payload = patch_call.kwargs["json"]
+        # "btcAddresses" should look like this:
+        """ {'btcAddresses': [{'address': 'bcrt1q4zcc0yppghquz9tzsd9k34m8rpmvav953hx3mk'}, 
+        {'address': 'bcrt1q2lmvfypnqcr7w9vcrlem7vdn7w65ac90x2ef6x'}, 
+        {'address': 'bcrt1quazsqywlme8ps70vq7xckflztghypp0r4ck9yw'}] """
+        assert isinstance(json_payload["btcAddresses"], list)
         for address in json_payload["btcAddresses"]:
+            assert isinstance(address, dict)
             assert isinstance(
                 address["address"], str
-            ), "Swan is expecting an alomost flat list of addresses"
+            ), "Swan is expecting an almost flat list of addresses"
diff --git a/tests/test_specterext_swan_service.py b/tests/test_specterext_swan_service.py

Original file line number	Diff line number	Diff line change
`@@ -102,6 +102,9 @@ def settings():`
`102`	`102`	`wallets=wallets,`
`103`	`103`	`cookies=request.cookies,`
`104`	`104`	`num_reserved_addrs=SwanService.MIN_PENDING_AUTOWITHDRAWAL_ADDRS,`
	`105`	`+ autowithdrawal_threshold=SwanService.get_current_user_service_data().get(`
	`106`	`+ SwanService.AUTOWITHDRAWAL_THRESHOLD`
	`107`	`+ ),`
`105`	`108`	`)`
`106`	`109`
`107`	`110`
Original file line number	Diff line number	Diff line change
`@@ -28,6 +28,7 @@ def should_intercept(call):`
`28`	`28`	`return not (`
`29`	`29`	`isinstance(call.excinfo.value, RpcError)`
`30`	`30`	`or isinstance(call.excinfo.value, SpecterError)`
	`31`	`+ or isinstance(call.excinfo.value, AssertionError)`
`31`	`32`	`)`
`32`	`33`
`33`	`34`