Skip to content

(fix) position mode: adopt exchange truth at init, fail loudly on empty-pair switch - #213

Open
fengtality wants to merge 2 commits into
fix/208-per-pair-trading-rulesfrom
fix/210-position-mode-truth
Open

(fix) position mode: adopt exchange truth at init, fail loudly on empty-pair switch#213
fengtality wants to merge 2 commits into
fix/208-per-pair-trading-rulesfrom
fix/210-position-mode-truth

Conversation

@fengtality

@fengtality fengtality commented Aug 7, 2026

Copy link
Copy Markdown
Contributor

Fixes #210. Stacked on the #208 fix (base branch fix/208-per-pair-trading-rules) — retarget after it merges.

Makes position mode honest. Both problems stem from connectors being created with trading_pairs=[] while position-mode implementations apply the switch through the pair list.

1. Init: adopt the exchange's mode instead of forcing HEDGE

The old connector.set_position_mode(HEDGE) at init ran with an empty pair list, and what it did depended on the connector: base-class implementations warned and returned (local trait stayed ONEWAY), while bybit's and bitget's per-pair overrides iterated the empty list, vacuously "succeeded", and flipped the LOCAL trait to HEDGE — in every case without a single exchange call. Local state was guesswork either way.

_adopt_exchange_position_mode reads the exchange's actual mode via _fetch_account_position_mode() and mirrors it locally (guarded by supported_position_modes), so position_mode, executors, and the get endpoint report reality. Unqueryable exchanges (bybit has no fetch override) keep the local default and operators set mode explicitly. Because bitget's fetch needs a registered pair, adoption is retried once on the first pair registration — otherwise it would be dead code at init for bitget.

Behavior note (deliberate): on bybit/bitget accounts whose exchange is genuinely in HEDGE, the old vacuous local-HEDGE happened to match; adoption keeps them matching where queryable (bitget, after first pair) and requires an explicit switch where not (bybit). On ONEWAY accounts this PR fixes the standing local/exchange mismatch that caused reduce-only rejections.

2. Switch endpoint: register first, validate against reality, await the exchange

  • PositionModeRequest gains optional trading_pair (backwards compatible). The pair is registered before validation — bybit's supported_position_modes() depends on the registered pair list and returns both modes vacuously when empty, so validating first fake-accepted HEDGE for inverse pairs. Unknown pairs → 400, never registered.
  • Empty pair list → 400 with instructions instead of the previous false success.
  • The switch awaits _execute_set_position_mode (the old set_position_mode call is fire-and-forget and cannot report rejection, e.g. Binance -4068 with open positions) and verifies the local trait after — 502 when the exchange refused.

Coordination: hummingbot-api-client needs the optional trading_pair field; until then callers should set leverage first (which registers the pair). Condor's MCP tool has been reordered accordingly.

Tests

Refuse-empty / register-and-switch; post-registration validation (bybit-faithful vacuous-modes fake); exchange-rejection surfaces as 502 with the call actually awaited; adoption (adopt/unqueryable/failure/no-hook). Suite: 102 passed, same 7 pre-existing failures as main.

🤖 Generated with Claude Code

@greptile-apps

greptile-apps Bot commented Aug 7, 2026

Copy link
Copy Markdown

Greptile Summary

The PR makes perpetual position-mode handling reflect exchange state during initialization and prevents empty-pair mode switches from reporting false success.

  • Adds an optional trading pair to position-mode requests and registers it before validation.
  • Awaits the exchange mode-switch operation and verifies the resulting local mode.
  • Adopts the exchange-reported mode during connector initialization and retries after the first pair registration.
  • Adds coverage for empty-pair rejection, pair-aware validation, exchange rejection, and mode adoption.

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
models/accounts.py Adds the optional trading pair field to position-mode requests.
routers/trading.py Passes the optional trading pair from the API request into the account service.
services/accounts_service.py Propagates the optional trading pair through the account-service boundary.
services/perpetual_trading_service.py Registers pairs before validation, rejects empty pair sets, awaits mode switching, and checks the resulting mode.
services/unified_connector_service.py Replaces forced initialization mode with exchange-state adoption and retries adoption after first-pair registration.
test/test_sync_pair_derived_state.py Adds focused tests for pair registration, validation ordering, exchange rejection, and mode adoption.

Sequence Diagram

sequenceDiagram
    participant Client
    participant API as Trading API
    participant Service as PerpetualTradingService
    participant Sync as UnifiedConnectorService
    participant Connector
    participant Exchange

    Client->>API: Set position mode(mode, trading_pair?)
    API->>Service: set_position_mode(...)
    opt trading_pair supplied
        Service->>Sync: sync_pair_derived_state(connector, pair)
        Sync->>Connector: register pair and derived limits
        Sync->>Exchange: fetch current position mode
        Exchange-->>Sync: current mode
        Sync->>Connector: adopt mode locally
    end
    Service->>Connector: validate registered pairs and supported mode
    Service->>Exchange: execute position-mode switch
    Exchange-->>Connector: success or rejection
    Service->>Connector: verify resulting local mode
    Service-->>API: success or HTTP error
    API-->>Client: response
Loading

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

@fengtality
fengtality force-pushed the fix/208-per-pair-trading-rules branch from 2db2cee to 698e01e Compare August 7, 2026 06:50
fengtality and others added 2 commits August 6, 2026 23:52
…fail loudly on switch

Two changes making position mode honest:

1. Init: adopt the exchange's actual mode instead of forcing HEDGE. The
   old connector.set_position_mode(HEDGE) at init ran with
   trading_pairs=[], so the py-base implementation warned and returned —
   it never reached any exchange, while the local trait kept its ONEWAY
   default. Accounts run in whatever mode the exchange already had, so
   the exchange is the source of truth: read it via
   _fetch_account_position_mode and mirror it locally so position_mode,
   executors and the position-mode endpoints report reality. Exchanges
   that cannot be queried keep the local default.

2. Switch endpoint: PositionModeRequest gains optional trading_pair
   (backwards compatible), registered via sync_pair_derived_state before
   switching; with no pair provided and none registered the service now
   returns 400 with instructions instead of a false success (the switch
   is applied through the connector's pair list, so an empty list means
   the exchange is never called).

flake8 skipped: violations in accounts_service.py/routers/trading.py are
pre-existing on main.

Fixes #210

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…e exchange call

- Register the pair BEFORE validating supported_position_modes: bybit's
  implementation depends on the registered pair list and returns both
  modes vacuously when it is empty, so pre-registration validation
  fake-accepted HEDGE for an inverse pair and then silently did nothing —
  recreating the exact lie this PR removes. Unknown pairs are rejected 400
  and never registered (validation from the base of the stack).
- Await _execute_set_position_mode instead of the fire-and-forget
  connector.set_position_mode (spawns a background task, cannot report
  rejection, e.g. Binance -4068 with open positions), and verify the local
  trait afterwards — 502 when the exchange did not accept the switch.
- Retry position-mode adoption once on first pair registration: bitget's
  _fetch_account_position_mode returns None with an empty pair list, so
  adoption at connector init was dead code for it.
- Guard adoption with supported_position_modes (future-proofing) and
  correct the narrative in comments/docstrings: with empty pairs the old
  init call left base-class connectors at local ONEWAY but vacuously
  flipped bybit/bitget local state to HEDGE — in every case without any
  exchange call.

flake8 skipped: pre-existing violations in touched files on main.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@fengtality
fengtality force-pushed the fix/210-position-mode-truth branch from 524626f to 903e52d Compare August 7, 2026 06:54
@fengtality

Copy link
Copy Markdown
Contributor Author

Adversarial review — the harshest of the three; the PR's original narrative was factually wrong and the endpoint had three residual lies. All fixed in the latest commit; the PR body has been rewritten accordingly.

  1. False claim (now corrected): "the old code left local mode ONEWAY" was only true for base-class connectors. bybit and bitget override _execute_set_position_mode; with empty pairs their loop is vacuous, all_success stays true, and the old code flipped LOCAL state to HEDGE without any exchange call — so this PR changes live behavior on exactly those two exchanges. The body now states this and why the direction is right (exchange truth over blind local HEDGE).
  2. Validation ran BEFORE registration, recreating the fake-success: bybit's supported_position_modes() is vacuous on an empty pair list, so HEDGE + an inverse pair passed validation and then silently did nothing. Fixed: register → then validate against the real pair set. Test with a bybit-faithful vacuous-modes fake.
  3. The endpoint still lied on real exchange rejections: connector.set_position_mode() is fire-and-forget — success was reported before the exchange responded (e.g. Binance -4068 with open positions). Fixed: the service awaits _execute_set_position_mode and verifies the local trait after — 502 when the exchange refused. Test proves the call is actually awaited.
  4. bitget's adoption was dead code at init (its fetch returns None with an empty pair list, which is always the case at init). Fixed: adoption retries once on the first pair registration. Also added a supported_position_modes guard on adoption (future-proofing).

Compat (unchanged, documented): the 400-on-empty is external-breaking until hummingbot-api-client ships the optional trading_pair field; interim workaround is leverage-first ordering (already applied to condor's MCP tool). Verified clean: no position-mode event listeners exist to skip, CancelledError propagates correctly, request model is backwards compatible.

@rapcmia rapcmia left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • git switch --detach origin/fix/207-throttler-pair-limits
  • git switch --detach origin/fix/208-per-pair-trading-rules
  • git switch --detach origin/fix/210-position-mode-truth
  • Setup with bitget_perpetual
  • Empty-pair position-mode request returned HTTP 400.
  • Valid BTC-USDT pair registration and HEDGE switch succeeded.
  • Position query reported HEDGE, matching the connector and exchange.
  • Unsupported BTC-USDS returned HTTP 400 using ONEWAY without changing the valid pair’s state.
  • An active Bitget order caused the expected HTTP 502 when switching to ONEWAY.
    • The test order was cancelled and confirmed inactive.
  • After restart, Bitget’s exchange mode remained HEDGE. Once BTC-USDT was registered, the local connector adopted HEDGE and matched the exchange.

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.

2 participants