diff --git a/routstr/upstream/auto_topup.py b/routstr/upstream/auto_topup.py index 6dd5ac7d..b4325e24 100644 --- a/routstr/upstream/auto_topup.py +++ b/routstr/upstream/auto_topup.py @@ -200,15 +200,48 @@ def validate_ppq_auto_topup_settings(settings: dict | None) -> str | None: return None +_legacy_threshold_hinted: set[int] = set() + + +def _routstr_threshold_sats(row: UpstreamProviderRow, settings: dict) -> float: + """Balance, in sats, below which a top-up fires. + + ``topup_threshold_sats`` is used as written. A legacy ``topup_threshold`` + keeps the thousandfold it has always been compared with — reinterpreting it + as sats would drop an operator's trigger point by a factor of 1000 on + upgrade and leave the peer to run dry. The hint names the value to migrate + to, once per provider rather than once per scheduler tick. + """ + explicit = settings.get("topup_threshold_sats") + if explicit is not None: + return float(typing.cast(int | float, explicit)) + + threshold_sats = float(typing.cast(int | float, settings["topup_threshold"])) * 1000 + if row.id is not None and row.id not in _legacy_threshold_hinted: + _legacy_threshold_hinted.add(row.id) + logger.warning( + "Routstr auto top-up uses the legacy unitless threshold; set " + "topup_threshold_sats to state the unit", + extra={"provider_id": row.id, "threshold_sats": threshold_sats}, + ) + return threshold_sats + + def validate_routstr_auto_topup_settings(settings: dict | None) -> str | None: """Return why enabled Routstr auto top-up settings are invalid, if anything.""" if not settings or not settings.get("auto_topup"): return None - threshold = settings.get("topup_threshold") + threshold_sats = settings.get("topup_threshold_sats") + legacy_threshold = settings.get("topup_threshold") amount = settings.get("topup_amount_limit") mint_url = settings.get("topup_mint_url") - if _invalid_topup_number(threshold): + if threshold_sats is not None: + if _invalid_topup_number(threshold_sats): + return "Routstr auto top-up threshold must be a positive number of sats" + elif legacy_threshold is None: + return "Routstr auto top-up requires a threshold" + elif _invalid_topup_number(legacy_threshold): return "Routstr auto top-up threshold must be a positive number" if _invalid_topup_number(amount, integer=True): return "Routstr auto top-up amount must be a positive whole number" @@ -270,7 +303,7 @@ async def _check_and_topup(row: UpstreamProviderRow) -> None: ) return - threshold = float(settings["topup_threshold"]) + threshold_sats = _routstr_threshold_sats(row, settings) amount = int(settings["topup_amount_limit"]) mint_url = str(settings["topup_mint_url"]) @@ -293,7 +326,7 @@ async def _check_and_topup(row: UpstreamProviderRow) -> None: ) return - if balance >= threshold * 1000: + if balance >= threshold_sats: return spent_24h_sats = await _routstr_spent_last_24h_sats() @@ -323,7 +356,7 @@ async def _check_and_topup(row: UpstreamProviderRow) -> None: extra={ "provider_id": row.id, "balance": balance, - "threshold": threshold, + "threshold_sats": threshold_sats, "topup_amount": amount, "mint_url": mint_url, }, diff --git a/tests/integration/test_routstr_auto_topup_threshold_units.py b/tests/integration/test_routstr_auto_topup_threshold_units.py new file mode 100644 index 00000000..a680b31d --- /dev/null +++ b/tests/integration/test_routstr_auto_topup_threshold_units.py @@ -0,0 +1,164 @@ +"""The Routstr top-up threshold has to state its unit. + +``topup_amount_limit`` was already plain sats — it goes straight to +``send_token(amount, "sat", ...)`` and is added to the balance for logging — but +its sibling ``topup_threshold`` was compared as ``balance >= threshold * 1000``. +Two keys from one settings blob, two different units, and nothing naming either. +An operator asking for "top up below 1000 sats" got one below 1,000,000. + +``topup_threshold_sats`` says what it means. Legacy values keep their effective +trigger point: reinterpreting them as sats would silently drop the trigger a +thousandfold and let a provider run dry. +""" + +import json +from typing import Any +from unittest.mock import patch + +import pytest + +from routstr.core.db import UpstreamProviderRow, create_session +from routstr.upstream import auto_topup as auto_topup_module +from routstr.upstream.auto_topup import ( + _check_and_topup, + validate_routstr_auto_topup_settings, +) + +from .test_routstr_auto_topup_claim import _patch_wallet, _peer, _sent_tokens + +# No module-level asyncio mark: the settings cases are sync, and the suite +# already runs asyncio in auto mode. + +TOPUP_SATS = 50 + + +@pytest.fixture(autouse=True) +def _forget_legacy_hints() -> Any: + # The hint fires once per provider for the life of the process. Reach it + # through the module: a sibling test reloads auto_topup, which rebinds the + # set, so a name imported here would go on clearing the old one. + auto_topup_module._legacy_threshold_hinted.clear() + yield + auto_topup_module._legacy_threshold_hinted.clear() + + +async def _seed(**topup_settings: Any) -> UpstreamProviderRow: + row = UpstreamProviderRow( + id=1, + slug="peer-1", + provider_type="routstr", + base_url="https://peer.test", + api_key="secret", + enabled=True, + provider_settings=json.dumps( + { + "auto_topup": True, + "topup_amount_limit": TOPUP_SATS, + "topup_mint_url": "https://mint.test", + **topup_settings, + } + ), + ) + async with create_session() as session: + session.add(row) + await session.commit() + await session.refresh(row) + return row + + +async def _topped_up(row: UpstreamProviderRow, balance: float) -> bool: + peer = _peer(balance) + with _patch_wallet(auto_topup_module, peer, "cashu-token-1"): + await _check_and_topup(row) + return bool(await _sent_tokens()) + + +async def test_explicit_sats_threshold_is_compared_against_a_sats_balance( + patched_db_engine: Any, +) -> None: + row = await _seed(topup_threshold_sats=1000) + assert await _topped_up(row, 1500.0) is False + + +async def test_explicit_sats_threshold_tops_up_below_the_stated_amount( + patched_db_engine: Any, +) -> None: + row = await _seed(topup_threshold_sats=1000) + assert await _topped_up(row, 500.0) is True + + +@pytest.mark.parametrize( + ("balance", "expected"), + [(1500.0, False), (500.0, True)], +) +async def test_legacy_threshold_keeps_its_effective_trigger_point( + patched_db_engine: Any, balance: float, expected: bool +) -> None: + # 1 x 1000 == the 1000 sats the old comparison actually used. Reading it as + # 1 sat instead would leave the peer to run dry. + row = await _seed(topup_threshold=1) + assert await _topped_up(row, balance) is expected + + +async def test_explicit_sats_threshold_overrides_the_legacy_key( + patched_db_engine: Any, +) -> None: + row = await _seed(topup_threshold=1, topup_threshold_sats=100) + assert await _topped_up(row, 500.0) is False + + +async def test_legacy_threshold_reports_the_sats_value_to_migrate_to( + patched_db_engine: Any, +) -> None: + row = await _seed(topup_threshold=1) + # The app logger does not propagate to root, so caplog would see nothing. + with patch.object(auto_topup_module, "logger") as log: + await _topped_up(row, 5000.0) + hints = [ + c for c in log.warning.call_args_list if "topup_threshold_sats" in c.args[0] + ] + assert len(hints) == 1 + assert hints[0].kwargs["extra"]["threshold_sats"] == 1000.0 + + # The scheduler runs every minute; the hint must not run with it. + await _topped_up(row, 5000.0) + assert ( + sum("topup_threshold_sats" in c.args[0] for c in log.warning.call_args_list) + == 1 + ) + + +def test_settings_accept_the_sats_threshold_on_its_own() -> None: + assert ( + validate_routstr_auto_topup_settings( + { + "auto_topup": True, + "topup_threshold_sats": 1000, + "topup_amount_limit": TOPUP_SATS, + "topup_mint_url": "https://mint.test", + } + ) + is None + ) + + +@pytest.mark.parametrize("value", [0, -1, True, float("inf"), "1000", None]) +def test_settings_reject_an_unusable_sats_threshold(value: object) -> None: + assert validate_routstr_auto_topup_settings( + { + "auto_topup": True, + "topup_threshold_sats": value, + "topup_amount_limit": TOPUP_SATS, + "topup_mint_url": "https://mint.test", + } + ) + + +def test_settings_require_one_of_the_threshold_keys() -> None: + assert validate_routstr_auto_topup_settings( + { + "auto_topup": True, + "topup_amount_limit": TOPUP_SATS, + "topup_mint_url": "https://mint.test", + } + ) diff --git a/ui/components/providers/RoutstrNodeSettings.tsx b/ui/components/providers/RoutstrNodeSettings.tsx index 18ace0dc..19499f10 100644 --- a/ui/components/providers/RoutstrNodeSettings.tsx +++ b/ui/components/providers/RoutstrNodeSettings.tsx @@ -14,12 +14,32 @@ import { Switch } from '@/components/ui/switch'; interface ProviderSettings { topup_mint_url?: string; auto_topup?: boolean; + topup_threshold_sats?: number; + /** Legacy, read-only here: the backend compares it as `x 1000` sats. */ topup_threshold?: number; topup_amount_limit?: number; refund_on_expiry?: boolean; [key: string]: unknown; } +/** + * Sats a legacy provider actually tops up below. + * + * This field has always been labelled Sats, but the backend compared + * `topup_threshold` as `balance >= threshold * 1000`, so the number shown here + * was never the number in force. Show the figure the backend uses, so editing + * starts from the truth instead of silently moving the trigger a thousandfold + * on the next save. + */ +function thresholdSats(settings: ProviderSettings): number | undefined { + if (settings.topup_threshold_sats !== undefined) { + return settings.topup_threshold_sats; + } + return settings.topup_threshold !== undefined + ? settings.topup_threshold * 1000 + : undefined; +} + interface RoutstrNodeSettingsProps { settings: ProviderSettings; onSettingsChange: (settings: ProviderSettings) => void; @@ -92,19 +112,23 @@ export function RoutstrNodeSettings({
- update({ topup_threshold: parseInt(e.target.value) }) + update({ + topup_threshold_sats: parseInt(e.target.value), + // Drop the legacy key so the saved blob carries one unit. + topup_threshold: undefined, + }) } />