diff --git a/services/accounts_service.py b/services/accounts_service.py index e1ce570f..0117a40c 100644 --- a/services/accounts_service.py +++ b/services/accounts_service.py @@ -902,24 +902,26 @@ async def place_trade(self, account_name: str, connector_name: str, trading_pair # Validate price for limit orders if order_type in [OrderType.LIMIT, OrderType.LIMIT_MAKER] and price is None: raise HTTPException(status_code=400, detail="Price is required for LIMIT and LIMIT_MAKER orders") - - # Check if trading rules are loaded - if not connector.trading_rules: - raise HTTPException( - status_code=503, - detail=f"Trading rules not yet loaded for {connector_name}. Please try again in a moment." - ) - - # Validate trading pair and get trading rule + + # Register the pair and sync pair-derived state (throttler limits, per-pair + # trading rules) — without this, per-pair-rules connectors (e.g. XRPL) had + # no rule for dynamically requested pairs and every direct trade failed, + # because rules are only built for pairs known at connector init (#207/#208). + # An unknown pair is rejected here with 400 before anything is registered. + try: + await self._connector_service.sync_pair_derived_state(connector, trading_pair) + except ValueError as e: + raise HTTPException(status_code=400, detail=str(e)) + + # The pair was validated above, so a missing rule here means the refresh + # failed or is pending (e.g. node outage) — retryable, not "unsupported". if trading_pair not in connector.trading_rules: - available_pairs = list(connector.trading_rules.keys())[:10] # Show first 10 - more_text = f" (and {len(connector.trading_rules) - 10} more)" if len(connector.trading_rules) > 10 else "" raise HTTPException( - status_code=400, - detail=f"Trading pair '{trading_pair}' not supported on {connector_name}. " - f"Available pairs: {available_pairs}{more_text}" + status_code=503, + detail=f"Trading rules for '{trading_pair}' on {connector_name} are not " + f"available yet (refresh failed or pending). Please retry shortly." ) - + trading_rule = connector.trading_rules[trading_pair] # Validate order type is supported diff --git a/services/perpetual_trading_service.py b/services/perpetual_trading_service.py index da757f8e..20a28b18 100644 --- a/services/perpetual_trading_service.py +++ b/services/perpetual_trading_service.py @@ -70,7 +70,7 @@ async def set_leverage(self, account_name: str, connector_name: str, # v5/position/set-leverage-{PAIR}); register the pair so the throttler # learns its rate limit before the request — see issue #207. from services.unified_connector_service import UnifiedConnectorService - await UnifiedConnectorService.sync_pair_derived_state(connector, trading_pair) + await UnifiedConnectorService.sync_pair_derived_state(connector, trading_pair, refresh_rules=False) try: await connector._execute_set_leverage(trading_pair, leverage) diff --git a/services/unified_connector_service.py b/services/unified_connector_service.py index 3b12d2c2..321cfcae 100644 --- a/services/unified_connector_service.py +++ b/services/unified_connector_service.py @@ -245,7 +245,8 @@ async def ensure_data_connector_started( try: # Add trading pair and sync pair-derived state before starting network - await self.sync_pair_derived_state(connector, trading_pair) + # (data path: throttler only, no trading-rules fetch) + await self.sync_pair_derived_state(connector, trading_pair, refresh_rules=False) # Start network await connector.start_network() @@ -412,18 +413,28 @@ def _is_tracker_running(self, tracker) -> bool: return False @staticmethod - async def sync_pair_derived_state(connector: ConnectorBase, trading_pair: str) -> None: + async def sync_pair_derived_state( + connector: ConnectorBase, + trading_pair: str, + refresh_rules: bool = True, + ) -> None: """Sync connector state that was derived from the trading-pair list at init. Connectors are created with ``trading_pairs=[]`` and pairs are registered - dynamically, but state built FROM that list during ``__init__`` is never - refreshed afterwards. ``AsyncThrottler`` pair-templated rate limits (issue - #207): connectors like bybit_perpetual build ``rate_limits_rules`` from - ``trading_pairs``, so a pair added later has no rate limit and pair-scoped - requests crash with ``AttributeError: 'NoneType' object has no attribute - 'weight'``. The throttler must be mutated in place — - ``WebAssistantsFactory`` captured the instance at connector init, so - reassigning ``connector._throttler`` has no effect. + dynamically, but several pieces of state are built FROM that list during + ``__init__`` and never refreshed afterwards: + + - ``AsyncThrottler`` pair-templated rate limits (issue #207): connectors like + bybit_perpetual build ``rate_limits_rules`` from ``trading_pairs``, so a + pair added later has no rate limit and pair-scoped requests crash with + ``AttributeError: 'NoneType' object has no attribute 'weight'``. The + throttler must be mutated in place — ``WebAssistantsFactory`` captured the + instance at connector init, so reassigning ``connector._throttler`` has no + effect. + - Trading rules on connectors that build them per pair (issue #208): XRPL + fetches rules on-ledger for each pair in ``_trading_pairs``, so a pair + added later has no trading rule and any executor for it dies at startup + with ``KeyError`` before placing an order. Idempotent — safe to call on every dynamic pair registration, including pairs that arrive via the data-connector bootstrap path. @@ -481,6 +492,31 @@ async def sync_pair_derived_state(connector: ConnectorBase, trading_pair: str) - f"{type(connector).__name__}: {e}" ) + # 3. Trading rules (#208). Only on paths that lead to order placement + # (refresh_rules=True: add_market registration, place_trade) and only on + # trading connectors. For per-pair-rules connectors the refresh is a real + # (possibly on-chain) fetch — order-book/data bootstrap paths pass + # refresh_rules=False so market-data requests never pay for it, even when + # get_best_connector_for_market hands them a trading connector. Only + # refresh when the pair has no rule yet. + if not refresh_rules or not getattr(connector, "is_trading_required", False): + return + try: + rules = getattr(connector, "trading_rules", None) + if rules is not None and trading_pair not in rules and hasattr(connector, "_update_trading_rules"): + await connector._update_trading_rules() + if trading_pair not in connector.trading_rules: + logger.warning( + f"Trading rules still missing for {trading_pair} on " + f"{type(connector).__name__} after refresh — orders for this " + f"pair will fail" + ) + except Exception as e: + logger.warning( + f"Could not refresh trading rules for {trading_pair} on " + f"{type(connector).__name__}: {e}" + ) + async def _add_trading_pair_to_tracker( self, connector: ExchangePyBase, @@ -497,9 +533,10 @@ async def _add_trading_pair_to_tracker( 2. Otherwise, register the pair and start the tracker """ try: - # Sync pair-derived state (throttler limits, trading rules) regardless of - # which path below registers the order book — see sync_pair_derived_state. - await self.sync_pair_derived_state(connector, trading_pair) + # Sync pair-derived state regardless of which path below registers the + # order book — throttler only: this is a market-data path, so it never + # pays for a trading-rules fetch even on a trading connector. + await self.sync_pair_derived_state(connector, trading_pair, refresh_rules=False) # Safety check - gateway/AMM connectors don't have order book trackers if not hasattr(connector, 'order_book_tracker') or connector.order_book_tracker is None: diff --git a/test/test_sync_pair_derived_state.py b/test/test_sync_pair_derived_state.py index 16c5f24a..517c5907 100644 --- a/test/test_sync_pair_derived_state.py +++ b/test/test_sync_pair_derived_state.py @@ -1,10 +1,11 @@ -"""Tests for UnifiedConnectorService.sync_pair_derived_state (issue #207). +"""Tests for UnifiedConnectorService.sync_pair_derived_state (issues #207 / #208). Connectors are created with trading_pairs=[] and pairs registered dynamically, but -throttler pair-templated rate limits are built from that list at init. The helper -must re-sync them, idempotently, on every registration. +throttler rate limits and per-pair trading rules are built from that list at init. +The helper must re-sync all of it, idempotently, on every registration. """ import asyncio +from decimal import Decimal from hummingbot.core.api_throttler.async_throttler import AsyncThrottler from hummingbot.core.api_throttler.data_types import RateLimit @@ -13,11 +14,15 @@ class FakePairLimitsConnector: - """Mimics a connector with pair-templated rate limits (bybit-style).""" + """Mimics a connector with pair-templated rate limits (bybit-style) and + per-pair trading rules (XRPL-style).""" - def __init__(self, trading_pairs=None): + def __init__(self, trading_pairs=None, trading_required=True): self._trading_pairs = trading_pairs self._throttler = AsyncThrottler(rate_limits=self._build_limits(trading_pairs or [])) + self._trading_rules = {} + self._trading_required = trading_required + self.rules_fetch_count = 0 @staticmethod def _build_limits(trading_pairs): @@ -30,6 +35,20 @@ def _build_limits(trading_pairs): def rate_limits_rules(self): return self._build_limits(self._trading_pairs or []) + @property + def is_trading_required(self): + return self._trading_required + + @property + def trading_rules(self): + return self._trading_rules + + async def _update_trading_rules(self): + """XRPL-style: builds rules only for pairs currently in _trading_pairs.""" + self.rules_fetch_count += 1 + for pair in self._trading_pairs or []: + self._trading_rules[pair] = {"min_order_size": Decimal("1")} + def _run(coro): return asyncio.run(coro) @@ -117,3 +136,96 @@ async def exchange_symbol_associated_to_pair(self, trading_pair): connector = BrokenResolverConnector(trading_pairs=["BTC-USDT"]) _run(UnifiedConnectorService.sync_pair_derived_state(connector, "BTC-USDT")) assert connector._trading_pairs == ["BTC-USDT"] + + +def test_rules_built_for_registered_pair(): + connector = FakePairLimitsConnector(trading_pairs=[]) + _run(UnifiedConnectorService.sync_pair_derived_state(connector, "USDC-XRP")) + + assert "USDC-XRP" in connector.trading_rules + assert connector.rules_fetch_count == 1 + + +def test_no_refetch_when_rule_exists(): + connector = FakePairLimitsConnector(trading_pairs=[]) + _run(UnifiedConnectorService.sync_pair_derived_state(connector, "USDC-XRP")) + _run(UnifiedConnectorService.sync_pair_derived_state(connector, "USDC-XRP")) + + assert connector.rules_fetch_count == 1 # rule present -> no second on-chain fetch + + +def test_data_connector_skips_rules_but_syncs_throttler(): + """Data connectors (trading_required=False) never place orders — no rules + fetch (possibly on-chain, would add latency to order-book bootstrap), but + their REST fetches share the throttler, so limits must still sync.""" + connector = FakePairLimitsConnector(trading_pairs=[], trading_required=False) + _run(UnifiedConnectorService.sync_pair_derived_state(connector, "BTC-USDT")) + + assert connector._trading_pairs == ["BTC-USDT"] + assert connector.rules_fetch_count == 0 + limit_ids = {limit.limit_id for limit in connector._throttler._rate_limits} + assert "order/create-BTC-USDT" in limit_ids + + +def test_rules_fetch_failure_is_swallowed(): + connector = FakePairLimitsConnector(trading_pairs=[]) + + async def broken_update(): + connector.rules_fetch_count += 1 + raise ConnectionError("node unreachable") + + connector._update_trading_rules = broken_update + _run(UnifiedConnectorService.sync_pair_derived_state(connector, "USDC-XRP")) + + assert connector._trading_pairs == ["USDC-XRP"] # registration still happened + assert connector.rules_fetch_count == 1 + + +def test_order_book_path_skips_rules_fetch(): + """Market-data paths pass refresh_rules=False: even on a trading connector + (get_best_connector_for_market prefers them), an order-book bootstrap must + never pay for a possibly-on-chain trading-rules fetch.""" + connector = FakePairLimitsConnector(trading_pairs=[]) + _run(UnifiedConnectorService.sync_pair_derived_state( + connector, "USDC-XRP", refresh_rules=False)) + + assert connector._trading_pairs == ["USDC-XRP"] + assert connector.rules_fetch_count == 0 + limit_ids = {limit.limit_id for limit in connector._throttler._rate_limits} + assert "order/create-USDC-XRP" in limit_ids # throttler still synced + + +def test_unknown_pair_cannot_poison_rules_refresh(): + """XRPL's rules fetch iterates ALL registered pairs and raises on the first + unknown one — so a single bad pair entering _trading_pairs would break rules + refresh for every valid pair, permanently. Validation must prevent entry.""" + + class XrplLikeConnector(FakePairLimitsConnector): + KNOWN = {"SOLO-XRP", "USDC-XRP"} + + async def exchange_symbol_associated_to_pair(self, trading_pair): + if trading_pair not in self.KNOWN: + raise KeyError(trading_pair) + return trading_pair + + async def _update_trading_rules(self): + self.rules_fetch_count += 1 + for pair in self._trading_pairs or []: + if pair not in self.KNOWN: # faithful: raises before ANY rule lands + raise ValueError(f"Market {pair} not found in markets list") + for pair in self._trading_pairs or []: + self._trading_rules[pair] = {"min_order_size": Decimal("1")} + + connector = XrplLikeConnector(trading_pairs=[]) + + # Typo'd pair (the reviewer's reproduction): rejected, never registered + try: + _run(UnifiedConnectorService.sync_pair_derived_state(connector, "XRP-USD")) + raise AssertionError("expected ValueError") + except ValueError: + pass + assert connector._trading_pairs == [] + + # Valid pairs still get rules — refresh is not poisoned + _run(UnifiedConnectorService.sync_pair_derived_state(connector, "SOLO-XRP")) + assert "SOLO-XRP" in connector.trading_rules