GlcoinCore/specter-desktop
1
0
Fork 0
mirror of https://github.com/cryptoadvance/specter-desktop.git synced 2026-08-13 12:33:29 +02:00
Code Issues Projects Releases Packages Wiki Activity Actions 8

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

Browse source 
* 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 <kim@swanbitcoin.com>
This commit is contained in:
Manolis Mandrapilias 2022-11-22 21:07:36 +01:00 • committed by GitHub
parent dd4b10668d
commit 48bccfa126
No known key found for this signature in database
GPG key ID: 4AEE18F83AFDEB23
13 changed files with 244 additions and 60 deletions
Download patch file Download diff file Expand all files Collapse all files

3
docs/development.md
View file

@ -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:
```

14
src/cryptoadvance/specter/services/service.py
View file

@ -129,6 +129,7 @@ class Service:
)
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 @@ class Service:
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 @@ class Service:
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

3
src/cryptoadvance/specter/wallet.py
View file

@ -628,7 +628,8 @@ class Wallet:
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:

9
src/cryptoadvance/specterext/swan/client.py
View file

@ -238,17 +238,13 @@ class SwanClient:
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 @@ class SwanClient:
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,

3
src/cryptoadvance/specterext/swan/controller.py
View file

@ -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
),
)

31
src/cryptoadvance/specterext/swan/service.py
View file

@ -111,7 +111,7 @@ class SwanService(Service):
@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 @@ class SwanService(Service):
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 @@ class SwanService(Service):
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):

8
src/cryptoadvance/specterext/swan/templates/swan/settings.jinja
View file

@ -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/>

10
tests/conftest.py
View file

@ -35,7 +35,7 @@ from cryptoadvance.specter.util.wallet_importer import WalletImporter
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(
"""

1
tests/conftest_visibility.py
View file

@ -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)
)

5
tests/fix_devices_and_wallets.py
View file

@ -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

41
tests/test_services.py
View file

@ -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",
]

68
tests/test_specterext_swan_client.py
View file

@ -6,7 +6,13 @@ from unittest.mock import MagicMock
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,34 +135,11 @@ def test_SwanClient_update_autowithdrawal_addresses(
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
@ -142,9 +147,16 @@ def test_SwanClient_update_autowithdrawal_addresses(
== "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"

108
tests/test_specterext_swan_service.py Normal file
View file

@ -0,0 +1,108 @@
from mock import patch
from cryptoadvance.specterext.swan.service import SwanService
import json
class SwanServiceNoEncryption(SwanService):
id = "reckless_swan"
encrypt_data = False
@patch(
"cryptoadvance.specterext.swan.client.SwanClient.update_autowithdrawal_addresses"
)
def test_reserve_addresses(mocked_update_autowithdrawal_addresses, app_no_node, wallet):
mocked_update_autowithdrawal_addresses.return_value = "some_id"
specter = app_no_node.specter
storage_manager = specter.service_unencrypted_storage_manager
swan = SwanServiceNoEncryption(True, specter)
# No data stored befre the reserve_addresses call
assert storage_manager.get_current_user_service_data("reckless_swan") == {}
# We need to mock flask's request context since reserve_addresses is calling client() which in turn calls request.url
# Since we already have a test app, the easiest way is to use app.test_request_context(). For details see:
# https://stackoverflow.com/questions/36729846/mock-flask-request-in-python-nosetests
with app_no_node.test_request_context():
swan.reserve_addresses(wallet, label="Swan withdrawals", num_addresses=5)
# Check that the correct addresse list was passed to the client's update_autowithdrawal_addresses-method, should be addresses #1, #3, #5, #7, #9 from the test_wallet (the first address is skipped)
addresses = [
"bcrt1qsqnuk9hulcfta7kj7687favjv66d5e9yy0lr7t",
"bcrt1qee494mauu3fv5aje0t4p6e52hvwq6d5hcqfxqt",
"bcrt1qpnem6p9vr8rmjsf7k49p9sleu0h020g34ggn6k",
"bcrt1q8534jsqkympwaelaqxhvfr6hc3g8y4kjtgr6d6",
"bcrt1qxd6ndd7mt7jqut7797l84675fz4kqhs4fcfgny",
]
assert (
mocked_update_autowithdrawal_addresses.call_args_list[0].kwargs["addresses"]
== addresses
)
# Also check that the wallet reserved the correct addresses
address_obj_list = wallet.get_associated_addresses("reckless_swan")
assert [address["address"] for address in address_obj_list] == addresses
# Check that the reserve_addresses was successful, we should have a swan_wallet_id now and the name of the associated (Specter) wallet
assert storage_manager.get_current_user_service_data("reckless_swan") == {
"swan_wallet_id": "some_id",
"wallet": wallet.alias,
}
mocked_update_autowithdrawal_addresses.return_value = "new_id"
# We are getting a new id since we request to reserve more addresses as we've already reserved
swan.reserve_addresses(wallet, label="Swan withdrawals", num_addresses=7)
# Adding address #11 and #13
additional_addresses = [
"bcrt1q32gd5s7rk9ptkv8e74q4c64ntf48u4sza6c9d9",
"bcrt1q463mg67f3tj5d223vf6387ty30qlx2wep4s5gp",
]
addresses.extend(additional_addresses)
assert (
mocked_update_autowithdrawal_addresses.call_args_list[1].kwargs["addresses"]
== addresses
)
assert storage_manager.get_current_user_service_data("reckless_swan") == {
"swan_wallet_id": "new_id",
"wallet": wallet.alias,
}
mocked_update_autowithdrawal_addresses.return_value = "another_new_id"
# We are not getting a new id since we've already reserved 7 addresses
swan.reserve_addresses(wallet, label="Swan withdrawals", num_addresses=7)
# Check that update_autowithdrawal_addresses was not called anymore (we had two calls so far)
assert len(mocked_update_autowithdrawal_addresses.mock_calls) == 2
assert storage_manager.get_current_user_service_data("reckless_swan") == {
"swan_wallet_id": "new_id",
"wallet": wallet.alias,
}
class SwanServiceWithMockedMethods(SwanService):
id = "mocked_swan"
encrypt_data = False
# Basically patching the reserve_addresses function like this, to avoid making the SwanService to a complete mock
@classmethod
def reserve_addresses(cls, wallet, label: str = None, num_addresses: int = 10):
pass
@patch("cryptoadvance.specterext.swan.client.SwanClient.set_autowithdrawal")
def test_set_autowithdrawal_settings(mocked_set_autowithdrawal, app_no_node, wallet):
autowithdrawal_api_response = """
{
"entity": "automaticWithdrawal",
"item": {
"id": "some_important_withdrawal_id",
"minBtcThreshold": "0.05",
"isActive": false,
"isCanceled": false,
"createdAt": "2022-01-07T02:14:56.070Z",
"walletId": "******************",
"walletAddressId": null
}
}
"""
mocked_set_autowithdrawal.return_value = json.loads(autowithdrawal_api_response)
specter = app_no_node.specter
storage_manager = specter.service_unencrypted_storage_manager
swan = SwanServiceWithMockedMethods(True, specter)
with app_no_node.test_request_context():
swan.set_autowithdrawal_settings(wallet, 0.05)
assert storage_manager.get_current_user_service_data("mocked_swan") == {
"autowithdrawal_id": "some_important_withdrawal_id",
"withdrawal_threshold": 0.05,
}