(fix) sync pair-derived connector state (throttler limits, trading rules) on dynamic pair registration

[fengtality]

Fixes #207
Fixes #208

Problem

Trading connectors are created with trading_pairs=[] and pairs are registered dynamically — but several pieces of connector state are built from that list during __init__ and never refreshed afterwards. Two independently reported failures share this root cause:

• AsyncThrottler missing pair-specific rate limits for dynamically registered trading pairs (AttributeError: 'NoneType' object has no attribute 'weight') #207 — AsyncThrottler pair-templated rate limits (bybit_perpetual et al.): a pair added later has no rate limit, so pair-scoped endpoints resolve limit_id to None and crash with AttributeError: 'NoneType' object has no attribute 'weight'.
• Trading rules missing for dynamically registered pairs on per-pair-rules connectors (KeyError, silent zombie executors on XRPL) #208 — per-pair trading rules (xrpl et al.): XRPL builds trading rules by querying the ledger for each pair in _trading_pairs, so a dynamically added pair has no rule. Executors then die at startup with KeyError in validate_sufficient_balance — but register as RUNNING, order_id: None: silent zombies that report 201 Created and never place an order. Reproduced live 3/3 on xrpl / USDC-XRP.

Point-patching either symptom leaves the other (and the next pair-derived component) broken, so this PR fixes the registration path itself.

Fix

New UnifiedConnectorService.sync_pair_derived_state(connector, trading_pair) — idempotent, called on every dynamic registration:

1. Pair list — append to connector._trading_pairs (handles the None case).
2. Throttler (AsyncThrottler missing pair-specific rate limits for dynamically registered trading pairs (AttributeError: 'NoneType' object has no attribute 'weight') #207) — throttler.add_rate_limits(connector.rate_limits_rules), mutating the instance WebAssistantsFactory captured at init (reassigning _throttler provably doesn't work). add_rate_limits skips known limit_ids, so repeat calls are free.
3. Trading rules (Trading rules missing for dynamically registered pairs on per-pair-rules connectors (KeyError, silent zombie executors on XRPL) #208) — if the pair has no rule yet, await connector._update_trading_rules(). Guarded so it's a no-op for connectors whose rules already cover all pairs (one exchange-info call), and only triggers a real (possibly on-chain) fetch when the rule is genuinely missing.

Wired into every dynamic registration site:

• TradingService.add_market → _register_trading_pair_with_connector (the executor-creation path)
• UnifiedConnectorService.ensure_data_connector_started
• UnifiedConnectorService._add_trading_pair_to_tracker — covers the data-connector bootstrap path AsyncThrottler missing pair-specific rate limits for dynamically registered trading pairs (AttributeError: 'NoneType' object has no attribute 'weight') #207 flagged as bypassing registration helpers

All sync failures degrade to warnings; registration never hard-fails on a sync step.

Tests

test/test_sync_pair_derived_state.py (6 tests, using the real AsyncThrottler):

• pair appended + trading rules built for a per-pair-rules connector
• throttler learns the pair-scoped limit in place (same object identity — the WebAssistantsFactory constraint)
• idempotency: repeat registration does not re-fetch rules or duplicate limits
• _trading_pairs=None initialization (XRPL allows None)
• minimal connector without throttler/rules doesn't raise
• rules-fetch failure (node unreachable) is swallowed; registration still completes

Full suite: 91 passed, 7 failed — the 7 failures are identical on clean main (5 × test_gateway_lp_executor, 2 × test_controller_config_class_loading), i.e. pre-existing and unrelated.

Verified the deployed image's hummingbot build has AsyncThrottlerBase.add_rate_limits (also confirmed end-to-end by the #207 reporter on two pairs).

🤖 Generated with Claude Code

[greptile-apps]

Greptile Summary

The PR synchronizes connector state derived from dynamically registered trading pairs and extends pair registration to leverage, position-mode, order, and market-data paths.

• Adds an idempotent synchronization helper for connector pair lists, throttler limits, and trading rules.
• Routes dynamic pair-registration paths through the synchronization helper.
• Adds an optional trading pair to position-mode requests and rejects switches when no pair is registered.
• Adds focused tests for in-place throttler updates, trading-rule refreshes, idempotency, missing pair lists, and refresh failures.

Confidence Score: 5/5

The PR appears safe to merge.

No blocking failure remains.

Important Files Changed

Filename	Overview
services/unified_connector_service.py	Adds centralized, idempotent synchronization of dynamically registered pairs, throttler limits, and per-pair trading rules.
services/trading_service.py	Moves connector pair-state synchronization before market-registration early returns and awaits the new asynchronous helper.
services/perpetual_trading_service.py	Synchronizes pairs before leverage changes and requires a registered pair before changing position mode.
services/accounts_service.py	Synchronizes pair-derived connector state before validating trading rules and forwards position-mode pair input.
routers/trading.py	Passes the optional trading pair from position-mode requests into the service layer.
models/accounts.py	Extends position-mode requests with an optional trading-pair field.
test/test_sync_pair_derived_state.py	Covers pair registration, throttler mutation, rule refreshes, idempotency, missing pair lists, and degraded refresh failures.

Sequence Diagram

sequenceDiagram
    participant API as API service
    participant Sync as sync_pair_derived_state
    participant Connector as Connector
    participant Throttler as AsyncThrottler
    API->>Sync: Register trading pair
    Sync->>Connector: Append pair if missing
    Sync->>Throttler: Add pair-derived rate limits
    alt Trading connector lacks pair rule
        Sync->>Connector: update_trading_rules()
    end
    Sync-->>API: Continue registration or operation

Loading

_{Reviews (4): Last reviewed commit: "(fix) set_position_mode: register pair a..." | Re-trigger Greptile}

[fengtality]

Skeptical self-review of the first commit found four defects, fixed in 62fcb9e:

1. Fixes #207 was overclaimed — set_leverage (the issue's headline symptom, v5/position/set-leverage-{PAIR}) calls _execute_set_leverage directly and never routed through any registration site, so the exact reported crash survived the first commit. Now synced in PerpetualTradingService.set_leverage.
2. Rules refresh ran on data connectors — they're created with trading_required=False and never place orders, so the refresh injected wasted (possibly on-chain, retry-with-backoff) fetches and spurious warnings into every order-book bootstrap. Now gated on is_trading_required; throttler sync still runs everywhere since data-connector REST fetches share pair-templated limit_ids.
3. place_trade had the same bug twice — on a per-pair-rules connector it 503'd forever ("rules not yet loaded", rules are empty at init) and, once any rule existed, 400'd with a misleading "pair not supported". Now synced before the checks.
4. rate_limits_rules absence was handled via caught AttributeError → warning noise per registration on connectors without the property; now an explicit hasattr guard.

Also verified during review: XRPL's _update_trading_rules fetches before clear(), and the clear→repopulate section is synchronous (no awaits), so a failed or concurrent refresh can't leave a torn/empty rules dict.

Note on diff size: accounts_service.py carries mechanical flake8 cleanup (whitespace/indent/line-length) — pre-existing violations the pre-commit hook surfaced once the file was touched. The only behavior change there is the place_trade sync.

New test: data connector skips the rules fetch but still gets the throttler limit. Suite: 92 passed, same 7 pre-existing failures as clean main.

[fengtality]

Second skeptical pass (3b7f63c) — two more findings:

1. add_market early-return defeated the sync's retry — if a pair was already tracked with a healthy order book but a previous rules sync had failed transiently (the helper deliberately swallows sync failures as warnings), every subsequent add_market returned before the sync could run again, leaving the connector broken until restart. Registration now runs before the early return; it's a no-op when state is already consistent.

2. set_position_mode silently no-ops — documented, NOT fixed here. _execute_set_position_mode (e.g. bybit) loops over connector.trading_pairs; with connectors created as trading_pairs=[] the loop never executes and the endpoint reports success while the exchange was never called. This needs either a pair-aware position-mode API or eager pair registration and is out of scope for this PR — flagging so it isn't assumed covered by the AsyncThrottler missing pair-specific rate limits for dynamically registered trading pairs (AttributeError: 'NoneType' object has no attribute 'weight') #207 fix.

Also reverted the mechanical flake8 cleanup in accounts_service.py — the diff there is now just the functional place_trade sync (the file's pre-existing lint violations remain as on main).

Diff is now: unified_connector_service (+helper, 3 sites), trading_service (registration hoisted), perpetual_trading_service (set_leverage), accounts_service (place_trade, 7 lines), tests. Suite: 92 passed, same 7 pre-existing failures as main.

[fengtality]

1351798 closes the set_position_mode gap flagged in the previous comment — it is now fixed rather than just documented:

• PositionModeRequest gains an optional trading_pair (backwards compatible). When provided, the pair is registered via sync_pair_derived_state before the switch — which also picks up pair-templated throttler limits (e.g. bybit's switch-mode endpoint).
• Empty pair list now fails loudly. The py-base default applies the switch through trading_pairs[0] and bybit loops over all pairs; both log a warning and return when the list is empty, so the endpoint previously reported success while the exchange was never called. With no pair provided and none registered, the service now returns 400 with instructions instead of a false success.

New test covers both behaviors (400 on empty + exchange untouched; register-then-switch with a provided pair). Suite: 93 passed, same 7 pre-existing failures.

With this, all three silent-failure paths from the dynamic-pair root cause are closed: executors (KeyError zombies), set_leverage (throttler crash), and set_position_mode (silent no-op).

[fengtality]

Third skeptical pass, on the set_position_mode commit specifically — verdict: right design, but two costs were under-weighted and are now handled outside this PR:

1. The 400 is a breaking change with a coordination cost. hummingbot-api-client's set_position_mode has a fixed signature (no trading_pair), and the MCP tool set_account_position_mode_and_leverage called mode-before-leverage — so on a fresh connector that flow now hits the 400. Mitigations: the MCP tool has been reordered to set leverage first (leverage registers the pair via sync_pair_derived_state, after which the mode call succeeds), and hummingbot-api-client should gain the optional trading_pair field in its next release. Callers that already have markets added are unaffected.

2. The most common instance of the silent no-op is NOT in this PR — deliberately. _create_and_initialize_trading_connector sets a HEDGE default at init, where trading_pairs is always [] — meaning that call has never reached any exchange. Making it suddenly effective would flip long-running accounts from their current exchange mode to HEDGE on the next restart, which is a live-behavior decision, not a bugfix. Split out as Init-time HEDGE default has never reached the exchange (set_position_mode no-ops with trading_pairs=[]) #210 with a suggested direction (record intent at init, apply on first pair registration — idempotent because the base checks exchange state first).

Why the 4-layer param threading is the minimal correct shape, for the record: the switch is applied through the connector's pair list, so with an empty list the only honest options are "refuse" or "accept a pair to register". Refuse-only makes the endpoint unusable standalone; inferring a pair would be magic. Optional-pair + loud 400 is the smallest surface that is both usable and truthful.

[fengtality]

Split by issue per review: #211 fixes #207 (throttler rate limits), #212 fixes #208 (per-pair trading rules, stacked on #211), #213 fixes #210 (position-mode truth: exchange-mode adoption at init + loud-failure switch endpoint, stacked on #212). All commits from this PR are preserved across the three, including every self-review fix discussed here. Branch kept for reference.