Skip to content

(fix) sync throttler pair-templated rate limits on dynamic pair registration - #211

Open
fengtality wants to merge 2 commits into
mainfrom
fix/207-throttler-pair-limits
Open

(fix) sync throttler pair-templated rate limits on dynamic pair registration#211
fengtality wants to merge 2 commits into
mainfrom
fix/207-throttler-pair-limits

Conversation

@fengtality

Copy link
Copy Markdown
Contributor

Fixes #207

Trading connectors are created with trading_pairs=[], so AsyncThrottler builds its rate limits from an empty pair list at init. Pairs registered dynamically never reach the throttler, and pair-templated limit_ids (bybit's v5/order/create-{PAIR}, v5/position/set-leverage-{PAIR}) resolve to None: AttributeError: 'NoneType' object has no attribute 'weight'.

Fix

UnifiedConnectorService.sync_pair_derived_state(connector, trading_pair) — idempotent, appends the pair and mutates the existing throttler in place via add_rate_limits() (the instance WebAssistantsFactory captured at init; reassigning _throttler provably has no effect — see #207). add_rate_limits skips known limit_ids, so repeat calls are free.

Wired into every dynamic registration site:

  • TradingService.add_market — registration hoisted before the early return, so a transiently failed sync retries instead of sticking until restart
  • ensure_data_connector_started and _add_trading_pair_to_tracker (covers the data-connector bootstrap path the issue flags)
  • set_leverage — pair-scoped endpoint that bypassed all registration paths (the issue's headline symptom)

Tests

5 tests using the real AsyncThrottler, including an object-identity check that the captured instance learns the new limit, and idempotency (no duplicate limits). Suite: 90 passed; the 7 failures are pre-existing on main (gateway-LP / controller-config).

Stacked series: this PR → #208 fix (trading rules) → #210 fix (position mode).

🤖 Generated with Claude Code

…tration

Connectors are created with trading_pairs=[], so AsyncThrottler builds its
rate limits from an empty pair list at init. Pairs registered dynamically
never reach the throttler, and pair-scoped limit_ids (e.g. bybit's
v5/order/create-{PAIR}, v5/position/set-leverage-{PAIR}) resolve to None:
AttributeError 'NoneType' object has no attribute 'weight'.

Adds UnifiedConnectorService.sync_pair_derived_state(), which appends the
pair and mutates the existing throttler in place via add_rate_limits()
(the instance WebAssistantsFactory captured at init — reassigning
_throttler provably has no effect). Wired into every dynamic registration
site: TradingService.add_market (hoisted before its early return so a
transiently failed sync retries), ensure_data_connector_started,
_add_trading_pair_to_tracker (covers the data-connector bootstrap path),
and set_leverage (pair-scoped endpoint that bypassed registration).

Fixes #207

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@greptile-apps

greptile-apps Bot commented Aug 7, 2026

Copy link
Copy Markdown

Greptile Summary

This PR synchronizes state derived from dynamically registered trading pairs, particularly pair-templated throttler limits, while preserving the throttler instance captured during connector initialization.

  • Adds an idempotent helper that validates and registers pairs before extending the existing throttler’s rate limits.
  • Invokes synchronization from market registration, data-connector startup, order-book tracking, and leverage-setting paths.
  • Adds tests covering in-place throttler mutation, idempotency, empty pair initialization, validation, and minimal connectors.

Confidence Score: 5/5

The PR appears safe to merge because no blocking failure remains within the eligible follow-up-review scope.

No blocking failure remains.

Important Files Changed

Filename Overview
services/unified_connector_service.py Adds centralized pair validation and registration with in-place synchronization of pair-derived throttler limits, then uses it in data and order-book connector paths.
services/trading_service.py Moves dynamic pair synchronization ahead of the existing-market early return and makes the registration helper asynchronous.
services/perpetual_trading_service.py Synchronizes pair-derived connector state before executing pair-scoped leverage requests.
test/test_sync_pair_derived_state.py Covers pair registration, throttler identity, idempotency, missing pair lists, unknown-pair rejection, and repeated synchronization.

Sequence Diagram

sequenceDiagram
    participant Caller
    participant Service
    participant Connector
    participant Throttler
    participant Exchange
    Caller->>Service: Register pair or set leverage
    Service->>Connector: Resolve trading-pair symbol
    Connector-->>Service: Pair recognized
    Service->>Connector: Append pair if absent
    Service->>Connector: Rebuild rate-limit rules
    Service->>Throttler: add_rate_limits(rules)
    Note over Throttler: Existing instance is mutated in place
    Service->>Exchange: Initialize market or execute request
    Exchange-->>Caller: Result
Loading

Reviews (2): Last reviewed commit: "(fix) adversarial-review fixes: validate..." | Re-trigger Greptile

…uard property eval

- Validate the pair against the connector's symbol map BEFORE appending to
  _trading_pairs, raising ValueError for unknown pairs. There is no
  rollback path, and a poisoned entry breaks every per-pair consumer:
  status polling raises KeyError on the symbol map inside gathers without
  return_exceptions (killing balance/position/order updates until
  restart), and per-pair trading-rules rebuilds fail for ALL pairs.
  Previously a typo'd pair via add_market or set_leverage was registered
  permanently. Already-registered pairs skip re-validation (the symbol map
  may be transiently unavailable).
- Evaluate rate_limits_rules inside the try: it is a property that can
  itself raise, and hasattr() evaluated it outside the guard (any
  non-AttributeError escaping aborted registration; an internal
  AttributeError silently skipped the sync with zero logging).
- Scope comments to what this PR implements (the rules-fetch retry
  rationale belongs to the stacked #212).

Findings from adversarial review; tests added for rejection-without-
registration and skip-revalidation-when-registered.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@fengtality

Copy link
Copy Markdown
Contributor Author

Adversarial review (independent reviewer + author verification) — verdict: core throttler fix confirmed sound (in-place mutation genuinely required; add_rate_limits idempotent, single limits_pct application, full-list call correct; installed wheel ships the method). Two real defects found and fixed in the latest commit:

  1. No rollback: a failed registration permanently poisoned connector._trading_pairs — a typo'd pair via add_market/set_leverage was appended before any validation, then every status poll raised KeyError on the symbol map inside a safe_gather without return_exceptions, killing balance/position/order updates until restart. Fixed: pairs are validated against the connector's symbol map before registration; unknown pairs raise ValueError and nothing is registered. Tests: rejection-without-registration, and skip-revalidation for already-registered pairs (symbol map may be transiently down).
  2. hasattr(connector, "rate_limits_rules") evaluated the property outside the try (double evaluation; non-AttributeError getter exceptions escaped the helper; internal AttributeError silently skipped the sync). Fixed: single evaluation inside the guard.

Also corrected: the hoist-rationale comment referenced a "rules fetch" that only exists in the stacked #212.

Verified clean: Gateway/Cython _trading_pairs append, circular imports, concurrency (no await between check-and-append), _update_balances/_markets ordering. Known accepted behavior: registered pairs are enrolled in per-pair status polling (documented in the helper docstring).

@rapcmia

rapcmia commented Aug 7, 2026

Copy link
Copy Markdown
Contributor

Test update:

  • Setup with Condor latest and add bybit (spot/perp) and backpack (spot/perp)
  • Test HAPI with docker build make setup; make deploy

Bybit Perpetual leverage for a dynamically registered pair ✅

#### set leverage for a valid Bybit pair
curl -s -u XXX:XXX -H 'Content-Type: application/json' -X POST http://localhost:8000/trading/master_account/bybit_perpetual/leverage -d '
{
  "trading_pair": "BTC-USDT",
  "leverage": 1
}' | jq
{
  "status": "success",
  "message": "Leverage for BTC-USDT set to 1 on bybit_perpetual"
}
logs: `POST /trading/master_account/bybit_perpetual/leverage` returned 200 OK; no related `NoneType`, `weight`, or retry errors found.


Add a second valid pair through the market/order-book flow ✅

  • The XRP-USDT order book initialized successfully with five bids and five asks.
  • Trading rules and a current price were available for the pair.
    #### add XRP-USDT through the market-data flow
    curl -s -u XXX:XXX -H 'Content-Type: application/json' -X POST http://localhost:8000/market-data/trading-pair/add -d '
    {
      "connector_name": "bybit_perpetual",
      "trading_pair": "XRP-USDT",
      "account_name": "master_account",
      "timeout": 30
    }' | jq
    {
      "success": true,
      "connector_name": "bybit_perpetual",
      "trading_pair": "XRP-USDT",
      "message": "Order book initialized for XRP-USDT"
    }
    
    #### verify the XRP-USDT order book
    curl -s -u XXX:XXX -H 'Content-Type: application/json' -X POST http://localhost:8000/market-data/order-book -d '
    {
      "connector_name": "bybit_perpetual",
      "trading_pair": "XRP-USDT",
      "depth": 5
    }' | jq
    {
      "trading_pair": "XRP-USDT",
      "bids": "5 levels",
      "asks": "5 levels"
    }
    
  • Repeat XRP-USDT registration returned success, no duplicate-limit error occurred.
  • The follow-up order-book request returned five bids and five asks; no second initialization failure occurred.
  • API logs showed both routes returning HTTP 200. Unrelated recurring gateway-ping/Backpack warnings did not affect these routes.
    #### repeat XRP-USDT registration
    curl -s -u XXX:XXX -H 'Content-Type: application/json' -X POST http://localhost:8000/market-data/trading-pair/add -d '
    {
      "connector_name": "bybit_perpetual",
      "trading_pair": "XRP-USDT",
      "account_name": "master_account",
      "timeout": 30
    }' | jq
    {
      "success": true,
      "connector_name": "bybit_perpetual",
      "trading_pair": "XRP-USDT",
      "message": "Order book initialized for XRP-USDT"
    }
    
    #### verify the second XRP-USDT order book request
    curl -s -u XXX:XXX -H 'Content-Type: application/json' -X POST http://localhost:8000/market-data/order-book -d '
    {
      "connector_name": "bybit_perpetual",
      "trading_pair": "XRP-USDT",
      "depth": 5
    }' | jq
    {
      "trading_pair": "XRP-USDT",
      "bids": "5 levels",
      "asks": "5 levels"
    }
    logs: both repeat-test routes returned HTTP 200; no duplicate-limit or second order-book initialization failure found.
    

Hummingbot currently has synchronous overrides for Backpack spot and Backpack perpetual. ❌

  • For those connectors, the method returns a string directly. Awaiting that string raises an exception, which the PR converts into ValueError. This can mbreak normal dynamic pair registration and leverage setup for Backpack.
  • Backpack spot pair registration failed with HTTP 500 during order-book initialization.
  • Backpack perpetual leverage setup failed because sync_pair_derived_state() awaited a synchronous resolver and converted the resulting TypeError into a false unknown-pair ValueError.
  • This blocks merge until valid Backpack registration and leverage setup succeed.
    #### Backpack spot: register a valid pair
    curl -u XXX:XXX -X POST http://localhost:8000/market-data/trading-pair/add -H 'Content-Type: application/json' -d '
    {
      "connector_name": "backpack",
      "trading_pair": "BTC-USDC",
      "account_name": "master_account",
      "timeout": 30
    }' | jq
    {
      "detail": "Failed to initialize order book for BTC-USDC"
    }
    observed: HTTP 500. Container logs identify `TypeError: object str can't be used in 'await' expression` at `services/unified_connector_service.py:449` for `BackpackExchange`.
    
    #### Backpack perpetual: set leverage for a valid pair
    curl -u XXX:XXX -X POST http://localhost:8000/trading/master_account/backpack_perpetual/leverage -H 'Content-Type: application/json' -d '
    {
      "trading_pair": "BTC-USDC",
      "leverage": 1
    }' | jq
    {
      "detail": "Unexpected error setting leverage: Cannot register 'BTC-USDC' on BackpackPerpetualDerivative: the connector does not recognize this trading pair (object str can't be used in 'await' expression)"
    }
    observed: HTTP 500. This is the same synchronous-resolver regression in the leverage path.
    
  • Comparing this with main branch using same curl tests above ✅
    • Backpack spot registered valid pair BTC-USDC successfully and initialized its order book.
    • Backpack perpetual leverage setup for BTC-USDC returned success with leverage 1.
    • The targeted API logs showed HTTP 200 for both requests and no resolver TypeError, NoneType/weight, or retry errors. Unrelated ticker warnings were not part of this test path.
      #### main-branch Backpack spot registration
      curl -u XXX:XXX -X POST http://localhost:8000/market-data/trading-pair/add -H 'Content-Type: application/json' -d '
      {
        "connector_name": "backpack",
        "trading_pair": "BTC-USDC",
        "account_name": "master_account",
        "timeout": 30
      }' | jq
      {
        "success": true,
        "connector_name": "backpack",
        "trading_pair": "BTC-USDC",
        "message": "Order book initialized for BTC-USDC"
      }
      observed: HTTP 200.
      
      #### main-branch Backpack perpetual leverage
      curl -u XXX:XXX -X POST http://localhost:8000/trading/master_account/backpack_perpetual/leverage -H 'Content-Type: application/json' -d '
      {
        "trading_pair": "BTC-USDC",
        "leverage": 1
      }' | jq
      {
        "status": "success",
        "message": "Leverage for BTC-USDC set to 1 on backpack_perpetual"
      }
      observed: HTTP 200.
      

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

AsyncThrottler missing pair-specific rate limits for dynamically registered trading pairs (AttributeError: 'NoneType' object has no attribute 'weight')

2 participants