diff --git a/models/accounts.py b/models/accounts.py index 4f7b8c2a..33eeb20e 100644 --- a/models/accounts.py +++ b/models/accounts.py @@ -1,5 +1,6 @@ +from typing import Any, Dict, Optional + from pydantic import BaseModel, Field -from typing import Dict, Any class LeverageRequest(BaseModel): @@ -11,8 +12,13 @@ class LeverageRequest(BaseModel): class PositionModeRequest(BaseModel): """Request model for setting position mode on perpetual connectors""" position_mode: str = Field(description="Position mode (HEDGE or ONEWAY)") + trading_pair: Optional[str] = Field( + default=None, + description="Pair to register on the connector before switching. Position-mode " + "implementations apply the switch through the connector's trading " + "pairs, so at least one registered pair is required.") class CredentialRequest(BaseModel): """Request model for adding connector credentials""" - credentials: Dict[str, Any] = Field(description="Connector credentials dictionary") \ No newline at end of file + credentials: Dict[str, Any] = Field(description="Connector credentials dictionary") diff --git a/routers/trading.py b/routers/trading.py index 4b6df289..2d882da4 100644 --- a/routers/trading.py +++ b/routers/trading.py @@ -424,7 +424,8 @@ async def set_position_mode( try: # Convert string to PositionMode enum mode = PositionMode[request.position_mode.upper()] - result = await accounts_service.set_position_mode(account_name, connector_name, mode) + result = await accounts_service.set_position_mode( + account_name, connector_name, mode, trading_pair=request.trading_pair) return result except KeyError: raise HTTPException( diff --git a/services/accounts_service.py b/services/accounts_service.py index e1ce570f..59b12a55 100644 --- a/services/accounts_service.py +++ b/services/accounts_service.py @@ -902,7 +902,13 @@ 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") - + + # Register the pair and sync pair-derived state (throttler limits, per-pair + # trading rules) — without this, per-pair-rules connectors (e.g. XRPL) 503 + # forever on the empty-rules check below and 400 on the pair check after it, + # because rules are only built for pairs known at connector init (#207/#208). + await self._connector_service.sync_pair_derived_state(connector, trading_pair) + # Check if trading rules are loaded if not connector.trading_rules: raise HTTPException( @@ -1061,12 +1067,14 @@ async def set_leverage(self, account_name: str, connector_name: str, return await self.perpetual_trading_service.set_leverage(account_name, connector_name, trading_pair, leverage) async def set_position_mode(self, account_name: str, connector_name: str, - position_mode: PositionMode) -> Dict[str, str]: + position_mode: PositionMode, + trading_pair: Optional[str] = None) -> Dict[str, str]: """ Set position mode for a perpetual connector. Delegates to PerpetualTradingService. """ - return await self.perpetual_trading_service.set_position_mode(account_name, connector_name, position_mode) + return await self.perpetual_trading_service.set_position_mode( + account_name, connector_name, position_mode, trading_pair=trading_pair) async def get_position_mode(self, account_name: str, connector_name: str) -> Dict[str, str]: """ diff --git a/services/perpetual_trading_service.py b/services/perpetual_trading_service.py index b0b24485..a903e9bd 100644 --- a/services/perpetual_trading_service.py +++ b/services/perpetual_trading_service.py @@ -1,6 +1,6 @@ import asyncio import logging -from typing import Any, Awaitable, Callable, Dict, List +from typing import Any, Awaitable, Callable, Dict, List, Optional from fastapi import HTTPException from hummingbot.core.data_type.common import PositionMode @@ -66,6 +66,12 @@ async def set_leverage(self, account_name: str, connector_name: str, if not hasattr(connector, '_execute_set_leverage'): raise HTTPException(status_code=400, detail=f"Connector '{connector_name}' does not support leverage setting") + # Set-leverage endpoints can be pair-scoped (e.g. bybit's + # 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) + try: await connector._execute_set_leverage(trading_pair, leverage) message = f"Leverage for {trading_pair} set to {leverage} on {connector_name}" @@ -77,7 +83,8 @@ async def set_leverage(self, account_name: str, connector_name: str, raise HTTPException(status_code=500, detail=f"Failed to set leverage: {str(e)}") async def set_position_mode(self, account_name: str, connector_name: str, - position_mode: PositionMode) -> Dict[str, str]: + position_mode: PositionMode, + trading_pair: Optional[str] = None) -> Dict[str, str]: """ Set position mode for a perpetual connector. @@ -85,6 +92,9 @@ async def set_position_mode(self, account_name: str, connector_name: str, account_name: Name of the account connector_name: Name of the connector (must be perpetual) position_mode: PositionMode.HEDGE or PositionMode.ONEWAY + trading_pair: Pair to register before switching. Position-mode + implementations apply the switch through the connector's trading + pairs, so at least one registered pair is required. Returns: Dictionary with success status and message @@ -103,6 +113,22 @@ async def set_position_mode(self, account_name: str, connector_name: str, detail=f"Position mode '{position_mode.value}' not supported. Supported modes: {supported_values}" ) + # Position-mode implementations apply the switch through the connector's + # trading pairs (the py-base default and e.g. bybit both log a warning and + # return when the list is empty), and API connectors are created with + # trading_pairs=[] — so without a registered pair the exchange is never + # called while this endpoint would report success. Register the provided + # pair, then refuse to proceed with an empty list rather than lie. + if trading_pair: + from services.unified_connector_service import UnifiedConnectorService + await UnifiedConnectorService.sync_pair_derived_state(connector, trading_pair) + if not getattr(connector, "trading_pairs", None): + raise HTTPException( + status_code=400, + detail=f"No trading pairs registered on {connector_name}; pass trading_pair " + f"so the position mode switch can be applied on the exchange" + ) + try: # Try to call the method - it might be sync or async result = connector.set_position_mode(position_mode) diff --git a/services/trading_service.py b/services/trading_service.py index 11041d61..f788fc77 100644 --- a/services/trading_service.py +++ b/services/trading_service.py @@ -128,6 +128,17 @@ async def add_market( """ await self.ensure_connector(connector_name) + connector = self.connectors.get(connector_name) + if not connector: + raise ValueError(f"Connector {connector_name} not available. Check credentials.") + + # Sync pair-derived state (throttler limits, per-pair trading rules) BEFORE + # the early return below: a no-op when already synced, and it retries a + # previously failed sync even when the market is already tracked with a + # healthy order book — otherwise a transient failure (e.g. node outage + # during the rules fetch) would stick until restart (#207/#208). + await self._register_trading_pair_with_connector(connector, trading_pair) + if connector_name not in self._markets: self._markets[connector_name] = set() @@ -151,11 +162,6 @@ async def add_market( self._markets[connector_name].add(trading_pair) - # Get connector from our account's connectors - connector = self.connectors.get(connector_name) - if not connector: - raise ValueError(f"Connector {connector_name} not available. Check credentials.") - # Initialize order book via MarketDataService (uses best available connector) logger.info(f"Initializing order book for {connector_name}/{trading_pair}") success = await self._market_data_service.initialize_order_book( @@ -170,9 +176,6 @@ async def add_market( logger.info(f"Order book initialized successfully for {connector_name}/{trading_pair}") - # Register trading pair with connector - self._register_trading_pair_with_connector(connector, trading_pair) - # Update balances to include tokens from new trading pair if hasattr(connector, '_update_balances'): try: @@ -217,7 +220,7 @@ async def remove_market( logger.info(f"Removed market {connector_name}/{trading_pair}") - def _register_trading_pair_with_connector( + async def _register_trading_pair_with_connector( self, connector: ConnectorBase, trading_pair: str @@ -225,12 +228,17 @@ def _register_trading_pair_with_connector( """ Register a trading pair with the connector's internal structures. + Delegates to UnifiedConnectorService.sync_pair_derived_state so that state + built from the pair list at connector init (throttler pair-templated rate + limits, per-pair trading rules) is refreshed too — see issues #207 / #208. + Args: connector: The connector instance (ExchangePyBase) trading_pair: Trading pair to register """ - if trading_pair not in connector._trading_pairs: - connector._trading_pairs.append(trading_pair) + already_registered = trading_pair in (getattr(connector, "_trading_pairs", None) or []) + await self._connector_service.sync_pair_derived_state(connector, trading_pair) + if not already_registered: logger.debug(f"Registered {trading_pair} with connector {type(connector).__name__}") # ======================================== diff --git a/services/unified_connector_service.py b/services/unified_connector_service.py index 48f0cedf..3ecb9a33 100644 --- a/services/unified_connector_service.py +++ b/services/unified_connector_service.py @@ -244,9 +244,8 @@ async def ensure_data_connector_started( connector = self.get_data_connector(connector_name) try: - # Add trading pair before starting network - if trading_pair not in connector._trading_pairs: - connector._trading_pairs.append(trading_pair) + # Add trading pair and sync pair-derived state before starting network + await self.sync_pair_derived_state(connector, trading_pair) # Start network await connector.start_network() @@ -412,6 +411,75 @@ def _is_tracker_running(self, tracker) -> bool: return True return False + @staticmethod + async def sync_pair_derived_state(connector: ConnectorBase, trading_pair: str) -> 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 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. + """ + # 1. The pair list itself — everything below derives from it. + pairs = getattr(connector, "_trading_pairs", None) + if pairs is None: + connector._trading_pairs = [trading_pair] + elif trading_pair not in pairs: + pairs.append(trading_pair) + + # 2. Throttler rate limits (#207). add_rate_limits() skips known limit_ids. + # Synced on data connectors too — their REST fetches go through the same + # throttler and can hit pair-templated limit_ids. + throttler = getattr(connector, "_throttler", None) + if ( + throttler is not None + and hasattr(throttler, "add_rate_limits") + and hasattr(connector, "rate_limits_rules") + ): + try: + throttler.add_rate_limits(connector.rate_limits_rules) + except Exception as e: + logger.warning( + f"Could not sync throttler rate limits for {trading_pair} on " + f"{type(connector).__name__}: {e}" + ) + + # 3. Trading rules (#208). Trading connectors only — data connectors never + # place orders, and for per-pair-rules connectors a refresh is a real + # (possibly on-chain) fetch that would add latency and warnings to every + # order-book bootstrap. Only refresh when the pair has no rule yet. + if 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, @@ -428,6 +496,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) + # 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: logger.debug(f"Connector {type(connector).__name__} doesn't have order book tracker") diff --git a/test/test_sync_pair_derived_state.py b/test/test_sync_pair_derived_state.py new file mode 100644 index 00000000..326b1c27 --- /dev/null +++ b/test/test_sync_pair_derived_state.py @@ -0,0 +1,175 @@ +"""Tests for UnifiedConnectorService.sync_pair_derived_state (issues #207 / #208). + +Connectors are created with trading_pairs=[] and pairs registered dynamically, but +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 + +from services.unified_connector_service import UnifiedConnectorService + + +class FakePerPairRulesConnector: + """Mimics a connector (bybit-style throttler + XRPL-style per-pair rules).""" + + 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 + + @property + def is_trading_required(self): + return self._trading_required + + @staticmethod + def _build_limits(trading_pairs): + limits = [RateLimit(limit_id="global", limit=10, time_interval=1)] + for pair in trading_pairs: + limits.append(RateLimit(limit_id=f"order/create-{pair}", limit=5, time_interval=1)) + return limits + + @property + def rate_limits_rules(self): + return self._build_limits(self._trading_pairs or []) + + @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) + + +def test_pair_appended_and_rules_built(): + connector = FakePerPairRulesConnector(trading_pairs=[]) + _run(UnifiedConnectorService.sync_pair_derived_state(connector, "USDC-XRP")) + + assert connector._trading_pairs == ["USDC-XRP"] + assert "USDC-XRP" in connector.trading_rules # 208: rules refreshed + assert connector.rules_fetch_count == 1 + + +def test_throttler_learns_pair_scoped_limit_in_place(): + connector = FakePerPairRulesConnector(trading_pairs=[]) + # WebAssistantsFactory captures the throttler instance at init — the fix must + # mutate that same object, not replace it. + original_throttler = connector._throttler + + _run(UnifiedConnectorService.sync_pair_derived_state(connector, "BTC-USDT")) + + assert connector._throttler is original_throttler + limit_ids = {limit.limit_id for limit in original_throttler._rate_limits} + assert "order/create-BTC-USDT" in limit_ids # 207: limit added dynamically + + +def test_idempotent_no_refetch_when_rule_exists(): + connector = FakePerPairRulesConnector(trading_pairs=[]) + _run(UnifiedConnectorService.sync_pair_derived_state(connector, "USDC-XRP")) + _run(UnifiedConnectorService.sync_pair_derived_state(connector, "USDC-XRP")) + + assert connector._trading_pairs == ["USDC-XRP"] + assert connector.rules_fetch_count == 1 # rule present -> no second on-chain fetch + limit_ids = [limit.limit_id for limit in connector._throttler._rate_limits] + assert limit_ids.count("order/create-USDC-XRP") == 1 + + +def test_none_trading_pairs_initialized(): + connector = FakePerPairRulesConnector(trading_pairs=None) + _run(UnifiedConnectorService.sync_pair_derived_state(connector, "RLUSD-XRP")) + assert connector._trading_pairs == ["RLUSD-XRP"] + + +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 = FakePerPairRulesConnector(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_minimal_connector_does_not_raise(): + class Minimal: + pass + + minimal = Minimal() + _run(UnifiedConnectorService.sync_pair_derived_state(minimal, "BTC-USDT")) + assert minimal._trading_pairs == ["BTC-USDT"] + + +def test_set_position_mode_refuses_empty_pairs_and_registers_provided_pair(): + """Position-mode implementations iterate connector.trading_pairs and silently + no-op when the list is empty — the service must refuse loudly instead of + reporting success, and must register a provided pair before switching.""" + from fastapi import HTTPException + from hummingbot.core.data_type.common import PositionMode + + from services.perpetual_trading_service import PerpetualTradingService + + class FakePerpConnector(FakePerPairRulesConnector): + def __init__(self): + super().__init__(trading_pairs=[]) + self.mode_set = None + + @property + def trading_pairs(self): + return self._trading_pairs or [] + + def supported_position_modes(self): + return [PositionMode.HEDGE, PositionMode.ONEWAY] + + def set_position_mode(self, mode): + self.mode_set = mode + + connector = FakePerpConnector() + + async def provider(account_name, connector_name): + return connector + + service = PerpetualTradingService(provider) + + # No pair provided and none registered -> loud 400, exchange never touched + try: + _run(service.set_position_mode("master", "bybit_perpetual", PositionMode.HEDGE)) + raise AssertionError("expected HTTPException for empty trading_pairs") + except HTTPException as e: + assert e.status_code == 400 + assert connector.mode_set is None + + # Pair provided -> registered (with rules/throttler synced), then switched + result = _run(service.set_position_mode( + "master", "bybit_perpetual", PositionMode.HEDGE, trading_pair="BTC-USDT")) + assert connector._trading_pairs == ["BTC-USDT"] + assert connector.mode_set == PositionMode.HEDGE + assert result["status"] == "success" + + +def test_rules_fetch_failure_is_swallowed(): + connector = FakePerPairRulesConnector(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