rustrade_execution/client/binance/

margin.rs

//! Binance Margin execution client (cross and isolated).
//!
//! [`BinanceMargin`] is a margin counterpart to [`super::spot::BinanceSpot`]. It shares the
//! exchange-agnostic infrastructure in [`super::shared`] (rate-limit tracking, reconnect/backoff,
//! event deduplication, error parsing) and implements the same `ExecutionClient` trait so callers
//! do not branch on spot-vs-margin transport.
//!
//! ## Scope
//! This module provides the client's identity and configuration ([`BinanceMargin`],
//! [`BinanceMarginConfig`], [`MarginSideEffect`]) and a full [`ExecutionClient`] implementation:
//! order submission/cancel and account snapshot / balance / open-order / trade queries over REST,
//! plus a live account event stream ([`ExecutionClient::account_stream`]) over the hand-rolled
//! `userListenToken` user-data WebSocket (the SDK's retired listen-key path is not used). Both
//! **cross** (`isIsolated = "FALSE"`, account-wide collateral) and **isolated** (`isIsolated =
//! "TRUE"`, per-pair sub-accounts) margin are supported, selected by
//! [`BinanceMarginConfig::is_isolated`].
//!
//! ## Cross vs. isolated
//! Cross is account-wide: a single `userListenToken`, one WS-API subscription, and per-asset
//! balances surfaced in the top-level `balances`. Isolated is per-pair: the caller declares the
//! [`BinanceMarginConfig::isolated_symbols`] universe, each symbol gets its own `userListenToken`,
//! and all N tokens are **multiplexed onto one WS-API socket**. Isolated per-pair balances and risk
//! are attached per-instrument (never folded into the asset-keyed model, which cannot represent
//! `(pair, asset)` pools). See [`BinanceMargin`] for the per-method contract.
//!
//! ## Borrow/repay
//! Margin orders carry a `sideEffectType` controlling whether the venue borrows on entry and
//! repays on close. This is configured once per client via [`MarginSideEffect`] (see its docs for
//! the silent-borrow footgun under the default [`MarginSideEffect::AutoBorrowRepay`]).
//!
//! ## No testnet
//! Binance margin/SAPI exists on **no** testnet — the SDK exposes only
//! `MarginTradingRestApi::production`. A `testnet: true` config is therefore inert for margin and
//! always resolves to production endpoints; the constructor logs a warning so this is observable
//! rather than silent. See [`BinanceMarginConfig::testnet`].
//!
//! ## Known limitations
//! - `balanceUpdate` events (deposits/withdrawals) are not forwarded. Callers reconcile balances
//!   after external transfers via [`ExecutionClient::fetch_balances`] or
//!   [`ExecutionClient::account_snapshot`].
//! - `userLiabilityChange` is surfaced at INFO but never folded into balance state: borrow/repay
//!   is a delta, and accumulating it would be the position tracking this library leaves to its
//!   consumers.
//! - A `TRADE` report whose order status (`X`) is neither `PARTIALLY_FILLED` nor `FILLED` emits
//!   the execution but no order snapshot, so a fill arriving after its order's terminal report
//!   cannot resurrect a retired order as a resting one. That order's filled quantity is settled
//!   instead by the terminal report's own `z`, or by
//!   [`ExecutionClient::fetch_open_orders`].

use super::shared::{
    AbortOnDropStream, BINANCE_MAX_TRADES, BinanceOrderType, BinanceTimeInForce,
    CONNECT_TIMEOUT_SECS, ExponentialBackoff, FILL_RECOVERY_TIMEOUT_SECS, HEARTBEAT_TIMEOUT_SECS,
    MyTradesFrom, ORDER_EXECUTIONS_BUDGET, OpenOrderListing, RateLimitTracker, RequestKind,
    SIGNAL_RECOVERY_LOOKBACK_MS, SharedDedupCache, UnrecoveredFills, UserDataFrame, WeightPool,
    classify_order_kind_tif, classify_rest_order_error, classify_rest_query_error,
    convert_execution_report, convert_open_order_listing, convert_open_order_owned_symbol,
    dedup_key_from_event, drop_after, gap_failed, gap_time, is_duplicate, log_unrecognised_frame,
    new_dedup_cache, parse_user_data_frame, recovered_order_totals, response_decode_error,
    rest_call_with_retry,
};
use crate::{
    AccountEventKind, AccountSnapshot, InstrumentAccountSnapshot, InstrumentBalanceUpdate,
    IsolatedInstrumentState, IsolatedMarginRisk, UnindexedAccountEvent, UnindexedAccountSnapshot,
    balance::{AssetBalance, AssetBalanceUpdate, Balance, BalanceUpdate},
    client::ExecutionClient,
    emit_stream_terminated,
    error::{
        ApiError, ConnectivityError, OrderError, StreamTerminationReason, UnindexedClientError,
    },
    order::{
        Order, OrderKey, OrderKind, TimeInForce,
        id::{ClientOrderId, OrderId, StrategyId, VenueOrderId},
        request::{OrderRequestCancel, OrderRequestOpen, UnindexedOrderResponseCancel},
        state::{Cancelled, Filled, Open, OrderState, UnindexedOrderState},
    },
    position::PositionReport,
    trade::{AssetFees, Trade, TradeId},
};
use binance_sdk::{
    common::{
        config::{ConfigurationRestApi, ConfigurationWebsocketApi},
        constants::{MARGIN_TRADING_REST_API_PROD_URL, SPOT_WS_API_PROD_URL},
        models::WebsocketEvent,
        websocket::{
            SendWebsocketMessageResult, Subscription, WebsocketApi as WsApiBase,
            WebsocketMessageSendOptions,
        },
    },
    margin_trading::{
        MarginTradingRestApi,
        rest_api::{
            MarginAccountCancelOrderIsIsolatedEnum, MarginAccountCancelOrderParams,
            MarginAccountNewOrderIsIsolatedEnum, MarginAccountNewOrderNewOrderRespTypeEnum,
            MarginAccountNewOrderParams, MarginAccountNewOrderSideEffectTypeEnum,
            MarginAccountNewOrderSideEnum, MarginAccountNewOrderTimeInForceEnum,
            MarginAccountNewOrderTypeEnum, QueryCrossMarginAccountDetailsParams,
            QueryCrossMarginAccountDetailsResponseUserAssetsInner,
            QueryIsolatedMarginAccountInfoParams,
            QueryIsolatedMarginAccountInfoResponseAssetsInner,
            QueryMarginAccountsOpenOrdersIsIsolatedEnum, QueryMarginAccountsOpenOrdersParams,
            QueryMarginAccountsTradeListIsIsolatedEnum, QueryMarginAccountsTradeListParams,
            QueryMarginAccountsTradeListResponseInner, RestApi,
        },
        websocket_streams::{
            ExecutionReport, MarginLevelStatusChange, OutboundAccountPosition, UserLiabilityChange,
        },
    },
};
use chrono::{DateTime, TimeZone, Utc};
use futures::stream::BoxStream;
use rust_decimal::Decimal;
use rustrade_instrument::{
    Side,
    asset::name::AssetNameExchange,
    exchange::ExchangeId,
    instrument::{kind::InstrumentKindDiscriminant, name::InstrumentNameExchange},
};
use serde::{Deserialize, Serialize};
use smol_str::{SmolStr, format_smolstr};
use std::{
    collections::{BTreeMap, HashMap},
    str::FromStr,
    sync::{
        Arc, Mutex,
        atomic::{AtomicBool, AtomicU64, AtomicUsize, Ordering},
    },
    time::Duration,
};
use tokio::sync::{mpsc, oneshot};
use tracing::{debug, error, info, trace, warn};

// ---------------------------------------------------------------------------
// Margin sideEffectType
// ---------------------------------------------------------------------------

/// Margin `sideEffectType` borrow/repay policy, fixed once per [`BinanceMargin`] client.
///
/// Only the two *intent-agnostic* modes are exposed as client-level variants. The borrow-vs-repay
/// decision derives from open/close direction — position state the library deliberately does not
/// track — so the venue is left to infer it from the account's loan state. The per-order modes
/// (`MARGIN_BUY` / `AUTO_REPAY`) are intentionally **not** modelled here; they belong to a future
/// generic per-order `RequestOpen::margin_effect` field that all margin adapters would map, added
/// only when a venue with genuine per-order borrow intent (e.g. another CEX margin) appears.
/// Mixed borrow appetite on a single venue today means running two clients (one per mode).
///
/// The `Serialize`/`Deserialize` wire form is the config-file value (`"auto_borrow_repay"` /
/// `"no_borrow"`); this is distinct from the Binance API value returned by
/// [`as_binance_str`](Self::as_binance_str) (`"AUTO_BORROW_REPAY"` / `"NO_SIDE_EFFECT"`).
#[derive(Debug, Clone, Copy, PartialEq, Eq, Default, Serialize, Deserialize)]
#[serde(rename_all = "snake_case")]
pub enum MarginSideEffect {
    /// `AUTO_BORROW_REPAY` — the venue borrows on entry and repays on close as needed.
    ///
    /// # Warning
    /// This is the default because it makes shorting work out of the box, but it means a
    /// **mis-sized order silently takes on debt**: an order larger than the free balance borrows
    /// the shortfall without any further opt-in (same footgun class as `RequestOpen::reduce_only`).
    /// Use [`MarginSideEffect::NoBorrow`] for a client that must never silently borrow.
    #[default]
    AutoBorrowRepay,
    /// `NO_SIDE_EFFECT` — never borrow or repay; orders that would require borrowing are rejected
    /// by the venue. The conservative opt-out from the [`AutoBorrowRepay`](Self::AutoBorrowRepay)
    /// silent-borrow behaviour.
    NoBorrow,
}

impl MarginSideEffect {
    /// The Binance `sideEffectType` wire string for this policy.
    pub fn as_binance_str(self) -> &'static str {
        match self {
            MarginSideEffect::AutoBorrowRepay => "AUTO_BORROW_REPAY",
            MarginSideEffect::NoBorrow => "NO_SIDE_EFFECT",
        }
    }

    /// This policy as the margin SDK's `sideEffectType` parameter enum.
    ///
    /// The SDK also exposes `MARGIN_BUY` and `AUTO_REPAY` (the pre-`AUTO_BORROW_REPAY` split
    /// primitives); rustrade deliberately models only the two composite policies, so those
    /// variants are unreachable from here.
    fn as_sdk_enum(self) -> MarginAccountNewOrderSideEffectTypeEnum {
        match self {
            MarginSideEffect::AutoBorrowRepay => {
                MarginAccountNewOrderSideEffectTypeEnum::AutoBorrowRepay
            }
            MarginSideEffect::NoBorrow => MarginAccountNewOrderSideEffectTypeEnum::NoSideEffect,
        }
    }
}

// ---------------------------------------------------------------------------
// Configuration
// ---------------------------------------------------------------------------

/// Configuration for the [`BinanceMargin`] execution client.
///
/// Mirrors [`BinanceSpotConfig`](super::spot::BinanceSpotConfig)'s credential handling (private
/// keys, secret-redacting [`Debug`], `Deserialize`-only) and adds margin-specific knobs.
// Serialize intentionally omitted — would expose secret_key in plaintext
#[derive(Clone, Deserialize)]
pub struct BinanceMarginConfig {
    // not pub — prevents accidental credential exposure via struct access.
    // Use BinanceMarginConfig::new() to construct, or deserialize from config file.
    api_key: String,
    secret_key: String,
    /// Use testnet endpoints instead of production.
    ///
    /// **Inert for margin:** Binance margin/SAPI has no testnet, so this field is always treated
    /// as production. Retained to mirror the spot config shape; [`BinanceMargin::new`] warns if it
    /// is set to `true`.
    pub testnet: bool,
    /// Isolated margin (per-pair sub-accounts) when `true`; cross margin (account-wide) when
    /// `false`.
    ///
    /// Defaults to `false` (cross). When `true`, [`isolated_symbols`](Self::isolated_symbols) must
    /// be non-empty — it declares the pairs to snapshot/stream; [`BinanceMargin::new`] panics
    /// otherwise.
    #[serde(default)]
    pub is_isolated: bool,
    /// The isolated-margin pairs this client snapshots and streams when
    /// [`is_isolated`](Self::is_isolated) is `true`.
    ///
    /// This is the authoritative symbol universe for the isolated per-symbol `userListenToken`
    /// token/subscription fan-out: tokens are per-symbol and must be known at stream start, so the
    /// set is fixed for the stream's lifetime. A pair activated *after* `account_stream` is called
    /// is **not** auto-subscribed — reconfigure and restart the stream to pick it up.
    ///
    /// Ignored for cross margin (`is_isolated = false`). Empty by default; a `true` +
    /// empty-set combination is a misconfiguration that panics in [`BinanceMargin::new`].
    #[serde(default)]
    pub isolated_symbols: Vec<InstrumentNameExchange>,
    /// Borrow/repay policy applied to every order placed by this client.
    ///
    /// Defaults to [`MarginSideEffect::AutoBorrowRepay`] — see its `# Warning` on silent borrowing.
    #[serde(default)]
    pub side_effect: MarginSideEffect,
}

// custom Debug to avoid leaking credentials in logs
impl std::fmt::Debug for BinanceMarginConfig {
    fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
        f.debug_struct("BinanceMarginConfig")
            .field("api_key", &"***")
            .field("secret_key", &"***")
            .field("testnet", &self.testnet)
            .field("is_isolated", &self.is_isolated)
            .field("isolated_symbols", &self.isolated_symbols)
            .field("side_effect", &self.side_effect)
            .finish()
    }
}

impl BinanceMarginConfig {
    /// Construct a [`BinanceMarginConfig`] with explicit control over every field.
    ///
    /// Prefer [`BinanceMarginConfig::cross_margin`] / [`BinanceMarginConfig::isolated`] for the
    /// common cases, or deserialize from a config file to keep credentials out of source. The
    /// positional args expose every field for full control; defaultable knobs (`is_isolated`,
    /// `isolated_symbols`, `side_effect`) also default via `#[serde(default)]` on the file path.
    ///
    /// Note: this does not itself validate `is_isolated` against `isolated_symbols`; that gate
    /// lives in [`BinanceMargin::new`] (it must also cover the `Deserialize`-only path).
    pub fn new(
        api_key: String,
        secret_key: String,
        testnet: bool,
        is_isolated: bool,
        isolated_symbols: Vec<InstrumentNameExchange>,
        side_effect: MarginSideEffect,
    ) -> Self {
        Self {
            api_key,
            secret_key,
            testnet,
            is_isolated,
            isolated_symbols,
            side_effect,
        }
    }

    /// Convenience constructor for the common case: cross margin, production endpoints, and the
    /// default [`MarginSideEffect::AutoBorrowRepay`] borrow/repay policy.
    ///
    /// Equivalent to [`new`](Self::new) with `testnet = false`, `is_isolated = false`, no
    /// `isolated_symbols`, and the default `side_effect`. Use [`new`](Self::new) when any of those
    /// need to differ.
    pub fn cross_margin(api_key: String, secret_key: String) -> Self {
        Self::new(
            api_key,
            secret_key,
            false,
            false,
            Vec::new(),
            MarginSideEffect::default(),
        )
    }

    /// Convenience constructor for isolated margin: production endpoints, the default
    /// [`MarginSideEffect::AutoBorrowRepay`] borrow/repay policy, and the given per-pair symbol
    /// universe (see [`isolated_symbols`](Self::isolated_symbols)).
    ///
    /// `symbols` should be non-empty: an isolated client with no symbols has nothing to
    /// snapshot or stream and [`BinanceMargin::new`] panics on it. Use [`new`](Self::new) to
    /// override `side_effect`.
    pub fn isolated(
        api_key: String,
        secret_key: String,
        symbols: Vec<InstrumentNameExchange>,
    ) -> Self {
        Self::new(
            api_key,
            secret_key,
            false,
            true,
            symbols,
            MarginSideEffect::default(),
        )
    }

    /// Read-only access to the API key (e.g. for logging or header construction).
    pub fn api_key(&self) -> &str {
        &self.api_key
    }
}

// ---------------------------------------------------------------------------
// BinanceMargin client
// ---------------------------------------------------------------------------

/// Binance Cross Margin execution client using the official binance-sdk.
///
/// Places orders and queries account state over the margin REST API
/// (`margin_trading::rest_api`), and streams live account events via a hand-rolled
/// `userListenToken` flow over the WS API, not the SDK's retired listen-key path.
///
/// See [`BinanceMarginConfig::testnet`] for the no-testnet caveat. The behaviour a caller most needs to know
/// is summarised below, with links to the authoritative detail.
///
/// # Borrow/repay (`sideEffectType`)
/// Fixed once per client via [`BinanceMarginConfig::side_effect`] / [`MarginSideEffect`]. The
/// default [`MarginSideEffect::AutoBorrowRepay`] makes shorting work out of the box but lets a
/// **mis-sized order silently borrow** — use [`MarginSideEffect::NoBorrow`] to opt out. Per-order
/// borrow intent is intentionally not modelled (it would require position tracking); see
/// [`MarginSideEffect`] for the rationale and upgrade path.
///
/// # Cross vs. isolated margin
/// The mode is fixed per client via [`BinanceMarginConfig::is_isolated`]:
/// - **Cross** (`is_isolated = false`, `isIsolated = "FALSE"`, account-wide collateral): per-asset
///   balances (incl. debt) are surfaced account-wide in the top-level `balances`.
/// - **Isolated** (`is_isolated = true`): per-pair sub-accounts. Balances + risk are attached
///   **per-instrument** via [`InstrumentAccountSnapshot::isolated`] (the asset-keyed top-level
///   `balances` is left empty, since `(pair, asset)` slots would collide), and per-symbol queries
///   are scoped to the configured [`BinanceMarginConfig::isolated_symbols`]. See
///   [`account_snapshot`](Self::account_snapshot) for the full per-method semantics.
///
/// # Trailing stops unsupported
/// `TrailingStop` / `TrailingStopLimit` return [`OrderError::UnsupportedOrderType`]: the binance-sdk
/// margin new-order binding omits `trailingDelta`. See [`open_order`](Self::open_order).
///
/// # User-data stream (`userListenToken`)
/// [`account_stream`](Self::account_stream) is hand-rolled over the `userListenToken` model — the
/// legacy margin listen-key user-data API was retired by Binance on 2026-02-20 and the SDK binds
/// only the dead endpoint. There is **no keepalive ping** (the retired listen-key `PUT` mechanism):
/// instead the token (~24h validity) is re-acquired and re-subscribed before its `expirationTime`,
/// transparently across reconnects.
///
/// # Margin balances & debt-freshness
/// Balances carry per-asset margin debt: [`Balance::net_asset`](crate::balance::Balance::net_asset)
/// returns `total - borrowed`, with `borrowed`/`interest` exposed via
/// [`MarginDetails`](crate::balance::MarginDetails). Authoritative debt totals come from the REST
/// [`account_snapshot`](Self::account_snapshot) (`BalanceSnapshot`); the WS stream keeps
/// `free`/`locked` live via `BalanceStreamUpdate` but **never** clobbers or re-establishes debt, and
/// `userLiabilityChange` is surfaced as an observable log only, never accumulated into state.
/// Consequently `net_asset` reflects debt only as fresh as the last `account_snapshot` for that
/// asset — call it at startup and refresh on demand (see [`account_stream`](Self::account_stream)'s
/// cold-start note).
///
/// # Rate limits
/// Once a `/sapi` response reports at least 90% of the documented per-IP weight limit (12000 per
/// minute) used, queries wait for the next minute, so the rest is left for orders and cancels,
/// which never wait for it. A query, including a [`fetch_open_orders`](Self::fetch_open_orders)
/// after a reconnect, can therefore take up to about a minute longer. A reconnect's fill recovery
/// and its `userListenToken` request do not wait for it either, since a pause could outlast the
/// recovery's 30 s and delay its fills to a retry. The per-UID `/sapi` limit is not tracked.
/// After Binance answers with a rate-limit error, every REST call waits out a cooldown.
///
/// # One client per engine (`ExchangeId`)
/// All emitted events — cross and isolated alike — are stamped [`ExchangeId::BinanceMargin`], and
/// the engine routes `AssetStates` / `ConnectivityStates` by `ExchangeId`. Running **two**
/// `BinanceMargin` clients in a single engine (e.g. one cross, one isolated) therefore collides
/// their exchange identity. A single engine should run **at most one** `BinanceMargin` client; a
/// consumer needing cross and isolated concurrently runs them in separate engines.
#[derive(Clone)]
pub struct BinanceMargin {
    config: Arc<BinanceMarginConfig>,
    // REST client for orders/queries (order submission/cancel, account snapshots, balances, trades).
    rest: Arc<RestApi>,
    // Production-configured REST config (credentials + base path) reused by the hand-rolled
    // `userListenToken` POST, which goes through the SDK's `common::utils::send_request` (the SDK
    // binds no endpoint for it) rather than the typed `RestApi` surface.
    rest_config: Arc<ConfigurationRestApi>,
    // WS-API configuration (credentials + ws_url) for the user-data stream. Consumed by
    // `account_stream`, which constructs a `common::websocket::WebsocketApi` directly (the SDK's
    // spot ws-api wrapper is unusable for margin — private base, no generic send/receive surface).
    ws_config: ConfigurationWebsocketApi,
    // shared rate-limit tracker across all REST calls
    rate_limiter: Arc<RateLimitTracker>,
}

impl std::fmt::Debug for BinanceMargin {
    fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
        f.debug_struct("BinanceMargin")
            .field("testnet", &self.config.testnet)
            .field("is_isolated", &self.config.is_isolated)
            .field("side_effect", &self.config.side_effect)
            .finish_non_exhaustive()
    }
}

impl BinanceMargin {
    /// Build the production-configured margin REST configuration (credentials + base path).
    ///
    /// The base path is pinned to production (`https://api.binance.com`): margin/SAPI has no
    /// testnet, and the config is reused both for the SDK `RestApi` and the hand-rolled
    /// `userListenToken` POST (which joins `/sapi/v1/userListenToken` onto this base).
    ///
    /// # Panics
    /// Panics if the binance-sdk configuration builder fails (invalid credentials format).
    #[allow(clippy::expect_used)] // Documented panic: invalid credentials detected at startup
    fn build_rest_config(config: &BinanceMarginConfig) -> ConfigurationRestApi {
        ConfigurationRestApi::builder()
            .api_key(config.api_key.clone())
            .api_secret(config.secret_key.clone())
            .base_path(MARGIN_TRADING_REST_API_PROD_URL)
            .build()
            .expect("failed to build Binance margin REST configuration")
    }

    /// Build the WS-API configuration for the `userListenToken` user-data stream.
    ///
    /// Captures the credentials and pins the WS-API endpoint (the same `wss://ws-api.binance.com`
    /// endpoint spot uses). The connection itself is established by
    /// [`account_stream`](crate::client::ExecutionClient::account_stream) later (it
    /// constructs a `common::websocket::WebsocketApi` directly); this only prepares the config.
    ///
    /// # Panics
    /// Panics if the binance-sdk configuration builder fails (invalid credentials format).
    #[allow(clippy::expect_used)] // Documented panic: invalid credentials detected at startup
    fn build_ws_config(config: &BinanceMarginConfig) -> ConfigurationWebsocketApi {
        ConfigurationWebsocketApi::builder()
            .api_key(config.api_key.clone())
            .api_secret(config.secret_key.clone())
            // Margin user-data streams use the same `wss://ws-api.binance.com` endpoint as spot — the
            // WS API is unified; only the REST base (SAPI) differs. The constant name reflects origin,
            // not an accidental reuse of a spot-specific URL.
            .ws_url(SPOT_WS_API_PROD_URL)
            .build()
            .expect("failed to build Binance margin WebSocket configuration")
    }

    /// Resolve the per-call effective isolated symbol set (isolated mode only).
    ///
    /// Isolated margin queries are per-symbol on the venue (there is no account-wide "no symbol =
    /// all" affordance), so every isolated snapshot/query iterates an explicit symbol set:
    /// - empty `instruments` (the "return all" sentinel) → the full configured
    ///   [`isolated_symbols`](BinanceMarginConfig::isolated_symbols);
    /// - non-empty → `instruments ∩ isolated_symbols`; any requested instrument NOT in
    ///   `isolated_symbols` is skipped with a `warn!` (there is no configured isolated
    ///   token/stream/snapshot for it — returning it would be a silent mismatch).
    ///
    /// Shared by `account_snapshot`, `fetch_open_orders`, and `fetch_trades` so their instrument,
    /// open-order, and trade sets cannot drift apart.
    fn effective_isolated_set(
        &self,
        instruments: &[InstrumentNameExchange],
    ) -> Vec<InstrumentNameExchange> {
        if instruments.is_empty() {
            return self.config.isolated_symbols.clone();
        }
        instruments
            .iter()
            .filter(|inst| {
                let in_set = self.config.isolated_symbols.contains(*inst);
                if !in_set {
                    warn!(
                        instrument = %inst.name(),
                        "BinanceMargin isolated: requested instrument not in configured isolated_symbols — skipping"
                    );
                }
                in_set
            })
            .cloned()
            .collect()
    }

    /// Isolated-margin `account_snapshot`: per-pair balances + risk attached per-instrument, with
    /// the asset-keyed top-level `balances` left empty. Open orders and
    /// isolated balances cover the same effective isolated set so they cannot diverge.
    async fn isolated_account_snapshot(
        &self,
        instruments: &[InstrumentNameExchange],
    ) -> Result<UnindexedAccountSnapshot, UnindexedClientError> {
        use futures::{StreamExt as _, TryStreamExt as _};

        let effective = self.effective_isolated_set(instruments);
        if effective.is_empty() {
            // No configured isolated pair matched: a no-symbol isolated call is invalid, so there is
            // nothing to query — return an empty snapshot rather than an account-wide one.
            return Ok(AccountSnapshot::new(
                ExchangeId::BinanceMargin,
                Vec::new(),
                Vec::new(),
            ));
        }

        // Per-pair isolated balances + risk (chunked ≤5 symbols/request), keyed by instrument.
        let mut balances_by_symbol = convert_isolated_margin_assets(
            fetch_isolated_margin_account_info(
                self.rest.clone(),
                self.rate_limiter.clone(),
                effective.clone(),
            )
            .await?,
        );

        // Open orders per instrument over the same effective set, concurrently (bounded to avoid
        // bursting request-weight limits).
        let instrument_snapshots: Vec<_> =
            futures::stream::iter(effective.into_iter().map(|instrument| {
                fetch_margin_open_orders_for_instrument(
                    self.rest.clone(),
                    self.rate_limiter.clone(),
                    instrument,
                    true,
                )
            }))
            .buffer_unordered(8)
            .map(|result| {
                let (inst, listing) = result?;
                let wrapped = listing
                    .orders
                    .into_iter()
                    .map(active_order_snapshot)
                    .collect();
                let isolated = balances_by_symbol.remove(&inst);
                if isolated.is_none() {
                    // The same effective set drives both balances and open orders, so a miss here
                    // means the venue's isolated-account response omitted (or mis-keyed) a requested
                    // pair. Surface it rather than silently emitting `isolated: None`.
                    warn!(
                        instrument = %inst.name(),
                        "BinanceMargin isolated: no isolated balance entry returned for instrument — snapshot will carry isolated: None"
                    );
                }
                Ok::<_, UnindexedClientError>(InstrumentAccountSnapshot::new(
                    inst,
                    wrapped,
                    listing.complete,
                    PositionReport::Unreported,
                    isolated,
                ))
            })
            .try_collect()
            .await?;

        // Isolated balances live on the instrument snapshots; the asset-keyed top-level vec stays
        // empty so the engine's `update_from_account` never writes colliding per-asset slots.
        Ok(AccountSnapshot::new(
            ExchangeId::BinanceMargin,
            Vec::new(),
            instrument_snapshots,
        ))
    }
}

impl ExecutionClient for BinanceMargin {
    const EXCHANGE: ExchangeId = ExchangeId::BinanceMargin;

    // Margin is leverage applied to spot pairs, not a distinct instrument kind: a cross or
    // isolated margin order is still an order on a `Spot` symbol.
    const SUPPORTED_KINDS: &'static [InstrumentKindDiscriminant] =
        &[InstrumentKindDiscriminant::Spot];
    type Config = BinanceMarginConfig;
    type AccountStream = BoxStream<'static, UnindexedAccountEvent>;

    /// Construct a `BinanceMargin` client from its configuration.
    ///
    /// # Panics
    /// Panics if:
    /// - the binance-sdk configuration builder fails (e.g. empty or malformed API key/secret),
    ///   matching [`BinanceSpot`](super::spot::BinanceSpot)'s startup contract; or
    /// - `is_isolated = true` but [`isolated_symbols`](BinanceMarginConfig::isolated_symbols) is
    ///   empty — an isolated client with no configured pairs has nothing to snapshot or stream.
    ///   This gate lives here (not only in [`BinanceMarginConfig::isolated`]) because the config is
    ///   `Deserialize`-only and a deserialized config bypasses the named constructor. The
    ///   `ExecutionClient::new` signature returns `Self`, not `Result`, so an unusable config is a
    ///   fail-fast panic (consistent with the credential `expect`s above), not a recoverable error.
    fn new(config: Self::Config) -> Self {
        if config.testnet {
            warn!(
                "BinanceMarginConfig.testnet = true is ignored: Binance margin has no testnet; \
                 using production endpoints"
            );
        }
        assert!(
            !(config.is_isolated && config.isolated_symbols.is_empty()),
            "BinanceMarginConfig: is_isolated = true requires a non-empty isolated_symbols"
        );
        // Built once and shared: one clone is consumed by the SDK `RestApi`, the original is retained
        // for the hand-rolled `userListenToken` POST via `send_request`. `ConfigurationRestApi: Clone`
        // and is cheap to copy (its `reqwest::Client` is `Arc`-backed), avoiding redundant credential
        // clones from building it twice.
        let rest_config = Self::build_rest_config(&config);
        let rest = Arc::new(MarginTradingRestApi::production(rest_config.clone()));
        let rest_config = Arc::new(rest_config);
        let ws_config = Self::build_ws_config(&config);
        Self {
            config: Arc::new(config),
            rest,
            rest_config,
            ws_config,
            rate_limiter: Arc::new(RateLimitTracker::new(WeightPool::Sapi)),
        }
    }

    /// Submit a margin order over the SAPI `POST /sapi/v1/margin/order` endpoint.
    ///
    /// Mirrors [`BinanceSpot::open_order`](super::spot::BinanceSpot)'s contract: never returns
    /// `None` (every failure is folded into the returned [`Order`]'s state as
    /// [`OrderState::inactive`]), so the engine always sees a definitive outcome.
    ///
    /// Margin specifics:
    /// - `sideEffectType` is the client-level [`MarginSideEffect`] (borrow/repay policy).
    /// - `isIsolated` is config-driven (`"TRUE"` for isolated, `"FALSE"` for cross).
    /// - `autoRepayAtCancel` is set only under [`MarginSideEffect::AutoBorrowRepay`]: a `NoBorrow`
    ///   client takes no loan, so requesting repay-on-cancel would be incoherent.
    /// - Trailing-stop kinds return [`OrderError::UnsupportedOrderType`] (the SDK omits
    ///   `trailingDelta` on the margin binding).
    async fn open_order(
        &self,
        request: OrderRequestOpen<ExchangeId, &InstrumentNameExchange>,
    ) -> Option<Order<ExchangeId, InstrumentNameExchange, UnindexedOrderState>> {
        let instrument = request.key.instrument.clone();
        let side = request.state.side;
        let price = request.state.price;
        let quantity = request.state.quantity;
        let kind = request.state.kind;
        let time_in_force = request.state.time_in_force;
        let cid = request.key.cid.clone();

        let order_key = OrderKey::new(
            ExchangeId::BinanceMargin,
            instrument.clone(),
            request.key.strategy.clone(),
            cid.clone(),
        );

        // Build the returned Order with a given inactive (failure) state, preserving the request
        // fields — keeps the many early-return error paths to one line each.
        let inactive = |state: UnindexedOrderState| {
            Some(Order {
                key: order_key.clone(),
                side,
                price,
                quantity,
                kind,
                time_in_force,
                state,
            })
        };

        let params = match build_new_order_params(
            instrument.name().to_string(),
            side,
            price,
            quantity,
            kind,
            time_in_force,
            cid.0.to_string(),
            self.config.side_effect,
            self.config.is_isolated,
        ) {
            Ok(params) => params,
            Err(BuildOrderError::Unsupported) => {
                return inactive(OrderState::inactive(OrderError::UnsupportedOrderType(
                    format!(
                        "BinanceMargin does not support OrderKind::{kind:?} with {time_in_force:?}"
                    ),
                )));
            }
            Err(BuildOrderError::Build(msg)) => {
                error!(%msg, "BinanceMargin failed to build new order params");
                return inactive(OrderState::inactive(OrderError::Rejected(
                    ApiError::OrderRejected(msg),
                )));
            }
        };

        let response =
            match rest_call_with_retry(&self.rest, &self.rate_limiter, RequestKind::Order, |rest| {
                let params = params.clone();
                Box::pin(async move { rest.margin_account_new_order(params).await })
            })
            .await
            {
                Ok(response) => response,
                Err(e) => {
                    return inactive(OrderState::inactive(classify_rest_order_error(
                        &e,
                        &instrument,
                    )));
                }
            };

        let data = match response.data().await {
            Ok(data) => data,
            Err(e) => {
                // Deserialization failure on a 2xx response — surface as a rejection, not a
                // transport error (the order did reach the venue).
                return inactive(OrderState::inactive(OrderError::Rejected(
                    ApiError::OrderRejected(e.to_string()),
                )));
            }
        };

        let time_exchange = data
            .transact_time
            .and_then(|ms| Utc.timestamp_millis_opt(ms).single())
            .unwrap_or_else(Utc::now);

        let exchange_order_id = match data.order_id {
            Some(id) => OrderId(format_smolstr!("{id}")),
            None => {
                error!("BinanceMargin open_order response missing orderId");
                return inactive(OrderState::inactive(OrderError::Rejected(
                    ApiError::OrderRejected("open_order response missing orderId".into()),
                )));
            }
        };

        let filled_qty = match data.executed_qty.as_deref() {
            Some(q) => Decimal::from_str(q).unwrap_or_else(|_| {
                // Present-but-unparseable executedQty is corrupt data, not an expected
                // absence: silently defaulting to zero could misreport a filled order as
                // Open. Surface it (mirrors margin_avg_price).
                warn!(
                    executed_qty = q,
                    "BinanceMargin: failed to parse executedQty; treating as zero"
                );
                Decimal::ZERO
            }),
            None => {
                // executedQty is expected on margin's REST order/cancel responses; its absence is
                // anomalous (not the expected-empty case), so surface it rather than silently zero.
                warn!("BinanceMargin: executedQty missing in response; treating as zero");
                Decimal::ZERO
            }
        };

        let state = if filled_qty >= quantity {
            // Fully filled on placement — derive avg price from cumulative quote qty (margin's
            // REST response exposes this; spot's WS response does not, hence spot passes None).
            let avg_price = margin_avg_price(data.cummulative_quote_qty.as_deref(), filled_qty);
            OrderState::fully_filled(Filled::new(
                exchange_order_id,
                time_exchange,
                filled_qty,
                avg_price,
            ))
        } else {
            OrderState::active(Open::new(
                VenueOrderId::Assigned(exchange_order_id),
                time_exchange,
                filled_qty,
            ))
        };

        Some(Order {
            key: order_key,
            side,
            price,
            quantity,
            kind,
            time_in_force,
            state,
        })
    }

    /// Cancel a resting margin order via `DELETE /sapi/v1/margin/order`.
    ///
    /// Cancels by exchange `orderId` when present and parseable, otherwise by the originating
    /// client order id. `isIsolated` is config-driven. Mirrors
    /// [`BinanceSpot::cancel_order`](super::spot::BinanceSpot): every failure is folded into the
    /// returned response's `state` as an `Err`.
    ///
    /// The margin cancel response carries no `transactTime`, so the cancellation timestamp is the
    /// local receive time.
    async fn cancel_order(
        &self,
        request: OrderRequestCancel<ExchangeId, &InstrumentNameExchange>,
    ) -> Option<UnindexedOrderResponseCancel> {
        let instrument = request.key.instrument.clone();
        let key = OrderKey {
            exchange: request.key.exchange,
            instrument: instrument.clone(),
            strategy: request.key.strategy.clone(),
            cid: request.key.cid.clone(),
        };

        let params = match build_cancel_order_params(
            instrument.name().to_string(),
            request.state.id.as_ref().and_then(VenueOrderId::assigned),
            &request.key.cid,
            self.config.is_isolated,
        ) {
            Ok(p) => p,
            Err(e) => {
                error!(%e, "BinanceMargin failed to build cancel order params");
                return Some(UnindexedOrderResponseCancel {
                    key,
                    state: Err(OrderError::Rejected(ApiError::OrderRejected(e))),
                });
            }
        };

        let response =
            match rest_call_with_retry(&self.rest, &self.rate_limiter, RequestKind::Order, |rest| {
                let params = params.clone();
                Box::pin(async move { rest.margin_account_cancel_order(params).await })
            })
            .await
            {
                Ok(response) => response,
                Err(e) => {
                    return Some(UnindexedOrderResponseCancel {
                        key,
                        state: Err(classify_rest_order_error(&e, &instrument)),
                    });
                }
            };

        let data = match response.data().await {
            Ok(data) => data,
            Err(e) => {
                // Deserialization failure on a 2xx response — surface as a rejection, not a
                // transport error (the cancel request did reach the venue), mirroring open_order.
                return Some(UnindexedOrderResponseCancel {
                    key,
                    state: Err(OrderError::Rejected(ApiError::OrderRejected(e.to_string()))),
                });
            }
        };

        // Margin cancel response has no transactTime field — use local receive time.
        let time_exchange = Utc::now();

        let exchange_order_id = match data.order_id {
            // NB: the margin cancel response types orderId as a String (the new-order response
            // types it as i64) — use it verbatim.
            Some(id) => OrderId(SmolStr::new(id)),
            None => {
                error!("BinanceMargin cancel response missing orderId");
                return Some(UnindexedOrderResponseCancel {
                    key,
                    state: Err(OrderError::Rejected(ApiError::OrderRejected(
                        "cancel response missing orderId".into(),
                    ))),
                });
            }
        };

        let filled_qty = match data.executed_qty.as_deref() {
            Some(q) => Decimal::from_str(q).unwrap_or_else(|_| {
                // Present-but-unparseable executedQty is corrupt data, not an expected
                // absence: silently defaulting to zero could misreport a filled order as
                // Open. Surface it (mirrors margin_avg_price).
                warn!(
                    executed_qty = q,
                    "BinanceMargin: failed to parse executedQty; treating as zero"
                );
                Decimal::ZERO
            }),
            None => {
                // executedQty is expected on margin's REST order/cancel responses; its absence is
                // anomalous (not the expected-empty case), so surface it rather than silently zero.
                warn!("BinanceMargin: executedQty missing in response; treating as zero");
                Decimal::ZERO
            }
        };

        Some(UnindexedOrderResponseCancel {
            key,
            state: Ok(Cancelled::new(exchange_order_id, time_exchange, filled_qty)),
        })
    }

    /// Fetch a full margin account snapshot: balances plus open orders per instrument.
    ///
    /// **Cross** (`is_isolated = false`): account-wide per-asset balances from
    /// `query_cross_margin_account_details` (carrying `borrowed`/`interest` → [`Balance::new_margin`])
    /// in the top-level `balances`, plus open orders per requested instrument — mirroring
    /// [`BinanceSpot::account_snapshot`](super::spot::BinanceSpot).
    ///
    /// **Isolated** (`is_isolated = true`): per-pair balances + risk from
    /// `query_isolated_margin_account_info` (chunked ≤5 symbols/request), attached **per-instrument**
    /// via [`InstrumentAccountSnapshot::isolated`] rather than folded into the asset-keyed
    /// top-level `balances` (which is left **empty**) — isolated sub-accounts are per-`(pair, asset)`
    /// and would collide in the asset-keyed model. The instrument set is the effective isolated set
    /// (`isolated_symbols`, or `instruments ∩ isolated_symbols` with out-of-set instruments skipped);
    /// each instrument's open orders and isolated balances are fetched over that same set.
    async fn account_snapshot(
        &self,
        assets: &[AssetNameExchange],
        instruments: &[InstrumentNameExchange],
    ) -> Result<UnindexedAccountSnapshot, UnindexedClientError> {
        if self.config.is_isolated {
            return self.isolated_account_snapshot(instruments).await;
        }
        let response =
            rest_call_with_retry(&self.rest, &self.rate_limiter, RequestKind::Query, |rest| {
                Box::pin(async move {
                    let params = QueryCrossMarginAccountDetailsParams::builder().build()?;
                    rest.query_cross_margin_account_details(params).await
                })
            })
            .await
            .map_err(|e| classify_rest_query_error(&e, None))?;

        let account = response.data().await.map_err(response_decode_error)?;

        let balances =
            filter_and_convert_margin_balances(account.user_assets.unwrap_or_default(), assets);

        // Fetch open orders for all instruments concurrently (with retry), limiting concurrency
        // to avoid bursting Binance's request weight limits. account_snapshot wraps Open orders
        // in OrderState::active(); fetch_open_orders returns them without the wrapper — both use
        // fetch_margin_open_orders_for_instrument.
        use futures::{StreamExt as _, TryStreamExt as _};
        let instrument_snapshots: Vec<_> =
            futures::stream::iter(instruments.iter().cloned().map(|instrument| {
                fetch_margin_open_orders_for_instrument(
                    self.rest.clone(),
                    self.rate_limiter.clone(),
                    instrument,
                    // Cross path only: isolated returns early via `isolated_account_snapshot`.
                    false,
                )
            }))
            .buffer_unordered(8)
            .map(|result| {
                let (inst, listing) = result?;
                let wrapped = listing
                    .orders
                    .into_iter()
                    .map(active_order_snapshot)
                    .collect();
                Ok::<_, UnindexedClientError>(InstrumentAccountSnapshot::new(
                    inst,
                    wrapped,
                    listing.complete,
                    PositionReport::Unreported,
                    None,
                ))
            })
            .try_collect()
            .await?;

        Ok(AccountSnapshot::new(
            ExchangeId::BinanceMargin,
            balances,
            instrument_snapshots,
        ))
    }

    /// Fetch current margin balances (incl. `borrowed`/`interest` debt) for the requested assets.
    ///
    /// **Cross**: account-wide per-asset balances; an empty `assets` slice is the "return all"
    /// sentinel.
    ///
    /// **Isolated**: returns an empty `Vec`. Isolated balances are per-`(pair, asset)` and the
    /// asset-keyed return type cannot carry them without collision — they are surfaced per-instrument
    /// via [`account_snapshot`](Self::account_snapshot)'s [`InstrumentAccountSnapshot::isolated`]
    /// instead.
    async fn fetch_balances(
        &self,
        assets: &[AssetNameExchange],
    ) -> Result<Vec<AssetBalance<AssetNameExchange>>, UnindexedClientError> {
        if self.config.is_isolated {
            return Ok(Vec::new());
        }
        let response =
            rest_call_with_retry(&self.rest, &self.rate_limiter, RequestKind::Query, |rest| {
                Box::pin(async move {
                    let params = QueryCrossMarginAccountDetailsParams::builder().build()?;
                    rest.query_cross_margin_account_details(params).await
                })
            })
            .await
            .map_err(|e| classify_rest_query_error(&e, None))?;

        let account = response.data().await.map_err(response_decode_error)?;

        Ok(filter_and_convert_margin_balances(
            account.user_assets.unwrap_or_default(),
            assets,
        ))
    }

    /// Fetch currently open margin orders, optionally filtered by instrument.
    ///
    /// **Cross**: honours the `ExecutionClient::fetch_open_orders` "return all" sentinel — an empty
    /// `instruments` slice is served by a single no-symbol `query_margin_accounts_open_orders` call
    /// (each order's instrument taken from its own `symbol`); a non-empty slice fetches the listed
    /// instruments concurrently, per-symbol.
    ///
    /// **Isolated**: per-symbol on the venue — always iterates the effective isolated set (empty
    /// `instruments` → configured `isolated_symbols`; out-of-set instruments skipped with a warning);
    /// never issues a no-symbol isolated call.
    async fn fetch_open_orders(
        &self,
        instruments: &[InstrumentNameExchange],
    ) -> Result<Vec<Order<ExchangeId, InstrumentNameExchange, Open>>, UnindexedClientError> {
        use futures::{StreamExt as _, TryStreamExt as _};

        // Isolated: per-symbol on the venue — always iterate the effective set (never a no-symbol
        // isolated call, which would error). The empty-`instruments` sentinel resolves to the
        // configured isolated_symbols.
        if self.config.is_isolated {
            let effective = self.effective_isolated_set(instruments);
            let capacity = effective.len();
            return futures::stream::iter(effective.into_iter().map(|instrument| {
                fetch_margin_open_orders_for_instrument(
                    self.rest.clone(),
                    self.rate_limiter.clone(),
                    instrument,
                    true,
                )
            }))
            .buffer_unordered(8)
            .try_fold(
                Vec::with_capacity(capacity),
                |mut acc: Vec<Order<ExchangeId, InstrumentNameExchange, Open>>,
                 (_, listing)| async move {
                    acc.extend(listing.orders);
                    Ok(acc)
                },
            )
            .await;
        }

        // Cross: empty slice = "return all" sentinel — a single no-symbol query is both correct
        // (the contract requires all instruments) and far cheaper than enumerating every symbol.
        if instruments.is_empty() {
            return fetch_margin_all_open_orders(
                self.rest.clone(),
                self.rate_limiter.clone(),
                false,
            )
            .await;
        }
        futures::stream::iter(instruments.iter().cloned().map(|instrument| {
            fetch_margin_open_orders_for_instrument(
                self.rest.clone(),
                self.rate_limiter.clone(),
                instrument,
                false,
            )
        }))
        .buffer_unordered(8)
        .try_fold(
            Vec::with_capacity(instruments.len()),
            |mut acc: Vec<Order<ExchangeId, InstrumentNameExchange, Open>>, (_, listing)| async move {
                acc.extend(listing.orders);
                Ok(acc)
            },
        )
        .await
    }

    /// Fetch margin trades (fills) since `time_since`, optionally filtered by instrument.
    ///
    /// **Documented deviation from the `ExecutionClient::fetch_trades` "return all" contract:**
    /// Binance's margin trade-list endpoint (`myTrades`) requires a symbol — there is no no-symbol
    /// "all trades" query (unlike open orders).
    ///
    /// **Cross**: an empty `instruments` slice has nothing to query and returns an empty `Vec`;
    /// callers wanting all trades must enumerate instruments explicitly.
    ///
    /// **Isolated**: the empty sentinel resolves to the configured `isolated_symbols` (the effective
    /// isolated set; out-of-set instruments skipped with a warning), iterated per-symbol (Design
    /// decision #4).
    ///
    /// **Lookback cost:** Binance serves margin trades by time only in spans under 24 hours, so
    /// each instrument is read in 23-hour windows from `time_since` until its first trade, one
    /// request (weight 10) per window, then by trade id. The cost grows with the lookback and
    /// has no cap: a `time_since` 30 days back with no trades costs about 32 requests per
    /// instrument, and one at the Unix epoch about 21,000. Pass the most recent `time_since` that
    /// covers what you need.
    ///
    /// **Upper bound:** trades are read up to the local clock at the call. A trade stamped later,
    /// including one inside any skew between the local clock and Binance's, is left to the next
    /// read, unless a window before the last one held a trade (or the last one read a full page)
    /// and the walk went on to page by id, which has no time bound.
    async fn fetch_trades(
        &self,
        time_since: DateTime<Utc>,
        instruments: &[InstrumentNameExchange],
    ) -> Result<Vec<Trade<AssetNameExchange, InstrumentNameExchange>>, UnindexedClientError> {
        use futures::StreamExt as _;

        // Resolve the effective instrument set. Isolated resolves the empty sentinel to the
        // configured isolated_symbols; cross keeps the slice verbatim (no no-symbol "all" form).
        let effective: Vec<InstrumentNameExchange> = if self.config.is_isolated {
            self.effective_isolated_set(instruments)
        } else {
            instruments.to_vec()
        };
        if effective.is_empty() {
            // Distinguish the two empty causes: cross with an empty `instruments` slice (the
            // documented no-op sentinel) vs. isolated with no configured `isolated_symbols` matched.
            debug!(
                is_isolated = self.config.is_isolated,
                "BinanceMargin fetch_trades: empty effective instrument set — returning empty result"
            );
            return Ok(Vec::new());
        }
        let start_time_ms = time_since.timestamp_millis();
        let is_isolated = self.config.is_isolated;
        let mut all_trades = Vec::new();

        // Binance requires per-symbol queries for trade history. Limit concurrency to avoid
        // bursting Binance's request weight limits.
        let mut stream = futures::stream::iter(effective.into_iter().map(|inst| {
            let rest = self.rest.clone();
            let rate_limiter = self.rate_limiter.clone();
            async move {
                let pages = paginate_margin_my_trades(
                    &rest,
                    &rate_limiter,
                    &inst,
                    MyTradesFrom::Time(start_time_ms),
                    is_isolated,
                    RequestKind::Query,
                )
                .await?;
                Ok::<_, UnindexedClientError>((inst, pages))
            }
        }))
        .buffer_unordered(8);
        while let Some(result) = stream.next().await {
            let (instrument, trades_data) = result?;
            for t in trades_data {
                if let Some(trade) = convert_margin_trade(&t, &instrument) {
                    all_trades.push(trade);
                }
            }
        }

        Ok(all_trades)
    }

    /// Live stream of account events (fills, order updates, balance changes) over the hand-rolled
    /// `userListenToken` user-data stream.
    ///
    /// Acquires a `userListenToken` (signed `POST /sapi/v1/userListenToken`, cross = no params),
    /// subscribes over the WS API (`userDataStream.subscribe.listenToken`), and keeps the stream
    /// live with auto-reconnect, exponential backoff, heartbeat monitoring, and fill recovery
    /// (mirroring [`BinanceSpot`](super::spot::BinanceSpot)). The token is re-acquired and
    /// re-subscribed before its ~24h expiry — there is **no** listen-key keepalive (that API is
    /// retired).
    ///
    /// As on spot, each instrument's gap after a reconnect is kept until its fills are forwarded:
    /// a gap whose read failed or timed out is retried after 1, 2, 4, 8 and 16 minutes, connected
    /// or not in between, reading only the gap. After five failed retries it is given up, logged
    /// at `error`; [`ExecutionClient::fetch_trades`] can still read its fills.
    ///
    /// # Debt cold-start
    /// This method does **not** seed balances. Margin debt (`borrowed`/`interest`) is correct only
    /// if the caller invokes [`ExecutionClient::account_snapshot`] at startup (the `BalanceSnapshot`
    /// that populates `margin`). WS thereafter keeps `free`/`locked` live via `BalanceStreamUpdate`
    /// but never re-establishes debt; `userLiabilityChange` is logged observably, not applied to
    /// balance state.
    ///
    /// # Startup race window
    /// Like spot, fills arriving between subscribe and the listener being registered may be missed;
    /// callers requiring startup fill completeness must call [`ExecutionClient::fetch_trades`] with
    /// a ~1s lookback after this returns. Callers must also call
    /// [`ExecutionClient::fetch_open_orders`] after each reconnect to reconcile order state — only
    /// TRADE fills are recovered, not order-lifecycle events.
    ///
    /// A recovered fill advances the order too. A live fill carries the order's cumulative filled
    /// quantity in [`Trade::order_filled_quantity`] (`executionReport`'s `z`); REST `myTrades`
    /// carries none, so recovery rebuilds the same figure by reading each recovered order's
    /// executions from its first: one extra request per order (another per further 1,000
    /// executions), up to four orders at a time per instrument. Those lookups have their own
    /// time budget inside the recovery timeout, so they can never cost a fill: a fill whose order
    /// was not looked up in time, or whose lookup failed, goes out with `order_filled_quantity:
    /// None`, logged at `warn`, and advances the position but not the order.
    ///
    /// # Isolated mode (multiplexed `userListenToken`)
    /// Under `is_isolated = true` a **separate** manager drives the stream (the cross path above is
    /// left untouched). It acquires one per-symbol `userListenToken` for each
    /// [`BinanceMarginConfig::isolated_symbols`] entry and multiplexes **all N subscriptions onto a
    /// single WS-API socket**. Every token is acquired, the socket connected, and all subscriptions
    /// confirmed **before this method returns** — if the connect or **any** subscribe fails, it
    /// returns `Err` with nothing spawned (preserving the "can't-start vs started-then-dropped"
    /// distinction). The `instruments` argument does **not** drive the isolated token set: the stream
    /// always covers exactly `isolated_symbols`.
    ///
    /// ## Live per-pair balances — [`InstrumentBalanceUpdate`](crate::AccountEventKind::InstrumentBalanceUpdate)
    /// The isolated stream delivers live fills and order updates (routed by the inner `symbol`) **and**
    /// live per-pair `free`/`locked` balances, emitted as
    /// [`AccountEventKind::InstrumentBalanceUpdate`]
    /// (base + quote per pair). Debt totals (`borrowed`/`interest`) stay REST-`BalanceSnapshot`-fresh
    /// per the debt-freshness contract; the stream keeps only `free`/`locked` live.
    ///
    /// **Consumption contract (important):** the engine deliberately does **not** store
    /// `InstrumentBalanceUpdate` — it falls through `update_from_account`'s `_ => None` wildcard,
    /// mirroring the snapshot's
    /// [`InstrumentAccountSnapshot::isolated`](crate::InstrumentAccountSnapshot::isolated). It is
    /// therefore **never in `EngineState`**, and a `StateReplicaManager` replica replays the same
    /// wildcard so it is **not** in replica state either. A consumer obtains live per-pair balances
    /// **only** by inspecting the raw account event off the stream (`AccountStreamEvent::Item(..)` /
    /// `audit_tick.event`) and matching the variant itself — a consumer reading solely replica
    /// `EngineState` will see nothing and wrongly conclude the feature is broken. A wrapper wanting
    /// engine-queryable per-instrument balances uses the `InstrumentData` extension point instead.
    ///
    /// ## First-prod-run check — `subscriptionId → symbol` routing
    /// `outboundAccountPosition` frames carry no symbol inline; on the multiplexed socket their pair
    /// is recovered via a `subscriptionId → symbol` map. This correlation is the one assumption no
    /// offline source could validate — **verify it on the first production run**. Fills and order
    /// updates are unaffected (they self-identify by `symbol`); if the correlation does not hold,
    /// per-pair balance frames are dropped with a `warn!` (never mis-applied) and balances degrade to
    /// snapshot-polling. The documented fallback is one socket per isolated symbol (symbol implied by
    /// the connection).
    async fn account_stream(
        &self,
        // _assets is intentionally ignored — Binance pushes outboundAccountPosition for all account
        // assets regardless of any filter (mirrors spot). See account_snapshot for filtering.
        _assets: &[AssetNameExchange],
        instruments: &[InstrumentNameExchange],
    ) -> Result<Self::AccountStream, UnindexedClientError> {
        let (tx, rx) = mpsc::unbounded_channel::<UnindexedAccountEvent>();
        let dedup = new_dedup_cache();
        let rest = self.rest.clone();
        let rest_config = self.rest_config.clone();
        let ws_config = self.ws_config.clone();
        let rate_limiter = self.rate_limiter.clone();

        let cm_handle = if self.config.is_isolated {
            // --- Isolated: separate multiplexed manager over the configured isolated_symbols ---
            // The stream covers exactly `isolated_symbols` (the per-symbol token universe — Design
            // decision #1); the trait's `instruments` argument does NOT drive the isolated token set.
            let symbols = self.config.isolated_symbols.clone();
            // All current Binance margin symbols are ≤22 bytes (within SmolStr's 23-byte inline
            // limit); guard the implicit invariant so an over-long symbol is caught early.
            debug_assert!(
                symbols.iter().all(|i| i.name().len() <= 23),
                "instrument name exceeds SmolStr inline capacity: {:?}",
                symbols.iter().find(|i| i.name().len() > 23)
            );

            // (0) Build the invariant `instrument → (base, quote)` map from the isolated account
            // info. Authoritative base/quote split for routing the symbol-less
            // `outboundAccountPosition` frames; built once, reused across
            // reconnects (a pair's base/quote never change). A REST failure here is a hard Err
            // before anything spawns.
            let assets = fetch_isolated_margin_account_info(
                rest.clone(),
                rate_limiter.clone(),
                symbols.clone(),
            )
            .await?;
            let base_quote = Arc::new(build_base_quote_map(&assets));
            for sym in &symbols {
                if !base_quote.contains_key(sym) {
                    warn!(
                        symbol = %sym.name(),
                        "BinanceMargin isolated: account info returned no base/quote for configured \
                         symbol — its live per-pair balance frames will be dropped (fills/orders \
                         unaffected; balances available via account_snapshot)"
                    );
                }
            }

            // (1) Acquire all N per-symbol tokens, (2) connect one socket, (3) subscribe all N —
            // all before returning so connect-or-any-subscribe failure → Err (nothing spawned),
            // preserving cross's "can't-start vs started-then-dropped" distinction.
            let tokens = acquire_all_isolated_tokens(&rest_config, &rate_limiter, &symbols).await?;
            let live =
                isolated_connect_and_subscribe(&ws_config, &tx, &dedup, &base_quote, &tokens)
                    .await
                    .map_err(|e| {
                        UnindexedClientError::Connectivity(ConnectivityError::Socket(e.to_string()))
                    })?;

            tokio::spawn(isolated_connection_manager(
                tx,
                dedup,
                ws_config,
                rest_config,
                rest,
                rate_limiter,
                symbols,
                base_quote,
                Some(live),
            ))
        } else {
            // --- Cross: the live-validated account-wide manager (unchanged by isolated) ---
            let instruments = instruments.to_vec();
            // All current Binance margin symbols are ≤22 bytes (within SmolStr's 23-byte inline
            // limit), making clone() a stack memcpy with no heap allocation. Guard this implicit
            // invariant so future symbols that exceed the limit are caught early.
            debug_assert!(
                instruments.iter().all(|i| i.name().len() <= 23),
                "instrument name exceeds SmolStr inline capacity: {:?}",
                instruments.iter().find(|i| i.name().len() > 23)
            );

            // Validate credentials + connectivity before returning the stream so the caller can
            // distinguish "can't start at all" from "started then disconnected" (auto-reconnect
            // handles the latter). The token POST is a signed REST round-trip; the WS connect proves
            // the socket. Subscription happens inside the manager (after the event listener
            // attaches), so early pushed events are not missed.
            let initial_token =
                acquire_user_listen_token(&rest_config, &rate_limiter, None).await?;
            let initial_ws = connect_margin_ws(&ws_config).await.map_err(|e| {
                UnindexedClientError::Connectivity(ConnectivityError::Socket(e.to_string()))
            })?;

            tokio::spawn(margin_connection_manager(
                tx,
                dedup,
                ws_config,
                rest_config,
                rest,
                rate_limiter,
                instruments,
                Some((initial_ws, initial_token)),
            ))
        };

        let rx_stream = tokio_stream::wrappers::UnboundedReceiverStream::new(rx);
        let guarded_stream = AbortOnDropStream::new(rx_stream, cm_handle);
        Ok(futures::StreamExt::boxed(guarded_stream))
    }
}

// ---------------------------------------------------------------------------
// User-data stream (hand-rolled userListenToken)
// ---------------------------------------------------------------------------

/// Safety margin subtracted from a token's `expirationTime` so renewal happens before it expires.
const TOKEN_RENEW_MARGIN_SECS: i64 = 300; // 5 min
/// Floor for the post-acquire renewal wait, so a near-expired/odd token still yields a sane wait.
const TOKEN_MIN_LIFETIME_SECS: u64 = 60;

/// A `userListenToken` and its expiry (epoch ms), from `POST /sapi/v1/userListenToken`.
struct UserListenToken {
    token: String,
    expiration_time_ms: i64,
}

/// Build the query params for the `userListenToken` POST: empty for **cross**, or
/// `isIsolated=TRUE&symbol=SYM` for **isolated** (a per-symbol token). Factored out so the
/// cross-vs-isolated param shape is unit-testable without a network round-trip.
fn build_listen_token_query(
    symbol: Option<&InstrumentNameExchange>,
) -> BTreeMap<String, serde_json::Value> {
    let mut query = BTreeMap::new();
    if let Some(sym) = symbol {
        query.insert(
            "isIsolated".to_string(),
            serde_json::Value::String("TRUE".to_string()),
        );
        query.insert(
            "symbol".to_string(),
            serde_json::Value::String(sym.name().to_string()),
        );
    }
    query
}

/// Wire shape of the `POST /sapi/v1/userListenToken` response.
#[derive(Deserialize)]
struct UserListenTokenResponse {
    // The live endpoint was observed returning `token`; accept `listenToken` defensively too.
    #[serde(alias = "listenToken")]
    token: String,
    #[serde(rename = "expirationTime")]
    expiration_time: i64,
}

/// Acquire a `userListenToken` via the SDK's generic signed sender.
///
/// The endpoint is unbound in the SDK, so this reuses `common::utils::send_request` (signing,
/// timestamp, `X-MBX-APIKEY`, and the SDK's reqwest client) against the production REST config.
/// `symbol` selects the scope:
/// - `None` → **cross** margin (account-wide), no `isIsolated`/`symbol` params;
/// - `Some(sym)` → **isolated** margin, scoped to that pair (`isIsolated=TRUE&symbol=SYM`). Each
///   isolated symbol gets its own per-symbol token (the isolated stream multiplexes N of them).
///
/// Routed through [`rest_call_with_retry`] so a transient rate-limit during a *planned* token
/// renewal retries in place rather than collapsing the stream into the full reconnect+backoff
/// path. `ConfigurationRestApi` stands in as the retry helper's `R` (it only ever hands back an
/// `Arc` clone to the per-attempt closure). It is [`RequestKind::Essential`]: the stream cannot
/// resume without a token, and every second it waits widens the gap fill recovery must cover.
async fn acquire_user_listen_token(
    rest_config: &Arc<ConfigurationRestApi>,
    rate_limiter: &RateLimitTracker,
    symbol: Option<&InstrumentNameExchange>,
) -> Result<UserListenToken, UnindexedClientError> {
    // Cross sends no params; isolated scopes the token to one symbol. Built once and cloned per
    // retry attempt (the closure runs per attempt; mirrors `fetch_isolated_margin_account_info`).
    let query = build_listen_token_query(symbol);
    let response = rest_call_with_retry(rest_config, rate_limiter, RequestKind::Essential, |cfg| {
        let query = query.clone();
        Box::pin(async move {
            binance_sdk::common::utils::send_request::<UserListenTokenResponse>(
                &cfg,
                "/sapi/v1/userListenToken",
                reqwest::Method::POST,
                // `send_request` takes owned query + body maps; params (if any) ride the query
                // string the signature is computed over, the body stays empty.
                query,
                BTreeMap::new(),
                None,
                true, // is_signed — token is auth-bearing; HMAC/timestamp/api-key by send_request
            )
            .await
        })
    })
    .await
    .map_err(|e| classify_rest_query_error(&e, symbol))?;

    let data = response.data().await.map_err(response_decode_error)?;
    Ok(UserListenToken {
        token: data.token,
        expiration_time_ms: data.expiration_time,
    })
}

/// How long to wait before renewing, from a token's `expirationTime` (epoch ms), with a safety
/// margin and a floor so a near-expired token still yields a sane (non-zero) wait.
fn token_renew_after(expiration_time_ms: i64) -> Duration {
    let now_ms = Utc::now().timestamp_millis();
    // Saturating arithmetic so a malformed/huge `expirationTime` can't overflow i64 (debug panic /
    // release wrap); the `/ 1_000` is a truncating ms→s conversion. try_from then maps any negative
    // (already-expired/odd) result to 0.
    let remaining_secs = expiration_time_ms
        .saturating_sub(now_ms)
        .saturating_div(1_000)
        .saturating_sub(TOKEN_RENEW_MARGIN_SECS);
    let secs = u64::try_from(remaining_secs).unwrap_or(0);
    // The floor guarantees a sane, non-zero wait; warn when it kicks in so a degraded renewal cadence
    // (near-expired or odd token) is observable rather than silent.
    if secs < TOKEN_MIN_LIFETIME_SECS {
        warn!(
            computed_secs = secs,
            floor_secs = TOKEN_MIN_LIFETIME_SECS,
            "BinanceMargin token renewal wait clamped to floor (near-expiry or odd expirationTime)"
        );
    }
    Duration::from_secs(secs.max(TOKEN_MIN_LIFETIME_SECS))
}

/// Connect a directly-constructed common-layer WS-API connection (no subscription yet).
///
/// The SDK's spot ws-api wrapper can't be reused for margin (private base, no generic
/// send/receive surface), so margin drives `common::websocket::WebsocketApi` directly. An empty
/// pool is auto-populated by the SDK. The handshake is bounded by `CONNECT_TIMEOUT_SECS` explicitly
/// rather than relying on the SDK's internal timeout (an unstable, version-pinned detail).
async fn connect_margin_ws(
    ws_config: &ConfigurationWebsocketApi,
) -> anyhow::Result<Arc<WsApiBase>> {
    let ws = WsApiBase::new(ws_config.clone(), vec![]);
    // Mirrors BinanceSpot's reconnect path: a stalled connect must not block the connection
    // manager for an OS-length TCP timeout if the SDK's internal bound ever changes.
    tokio::time::timeout(
        Duration::from_secs(CONNECT_TIMEOUT_SECS),
        ws.clone().connect(),
    )
    .await
    .map_err(|_| {
        anyhow::anyhow!("BinanceMargin WS connect timed out after {CONNECT_TIMEOUT_SECS}s")
    })??;
    Ok(ws)
}

/// Subscribe to the user-data stream over an existing WS-API connection.
///
/// `userDataStream.subscribe.listenToken` is unbound in the SDK, so it is sent via the generic
/// `send_message` with the literal method string. The frame is **unsigned with no API key** — the
/// token is the sole auth (verified live); `WebsocketMessageSendOptions::new()` yields
/// exactly that (no `.signed()`/`.with_api_key()`, no session logon).
async fn subscribe_listen_token(ws: &Arc<WsApiBase>, token: &str) -> anyhow::Result<()> {
    // The SDK's `send_message` takes an owned `BTreeMap<String, Value>`, so the single-entry map and
    // its key/value allocations are unavoidable here. Runs once per (re)connection, not per event.
    let mut payload = BTreeMap::new();
    payload.insert(
        "listenToken".to_string(),
        serde_json::Value::String(token.to_string()),
    );
    ws.send_message::<serde_json::Value>(
        "userDataStream.subscribe.listenToken",
        payload,
        WebsocketMessageSendOptions::new(),
    )
    .await
    .map_err(|e| anyhow::anyhow!("userDataStream.subscribe.listenToken failed: {e}"))?;
    Ok(())
}

/// Subscribe over an existing WS-API connection and return the ack's `subscriptionId` (isolated).
///
/// Identical wire frame to [`subscribe_listen_token`] (unsigned, token-as-auth) but deserializes the
/// ack's `result.subscriptionId` so the isolated manager can build the `subscriptionId → symbol`
/// routing map (the symbol-less `outboundAccountPosition` frames carry only this id — Design
/// decision #5). Returns:
/// - `Ok(Some(id))` — subscribed; the id correlates this symbol's pushed balance frames;
/// - `Ok(None)` — subscribed, but the ack carried no `subscriptionId` (per-instrument balance
///   routing for this pair is then unavailable: its `outboundAccountPosition` frames drop with a
///   `warn!` and the consumer falls back to snapshot-polled balances — fills/orders are unaffected,
///   they self-identify by inner `s`). This is the documented first-prod-run degradation path.
/// - `Err(..)` — the subscribe itself failed (a startup/reconnect error, surfaced by the caller).
async fn subscribe_listen_token_capture(
    ws: &Arc<WsApiBase>,
    token: &str,
) -> anyhow::Result<Option<i64>> {
    /// Ack `result` shape — `subscriptionId` is read defensively as an `Option` (its presence on the
    /// `.listenToken` ack is the one assumption no offline source closes; absent → `Ok(None)`).
    #[derive(Deserialize)]
    struct SubscribeAck {
        #[serde(rename = "subscriptionId", default)]
        subscription_id: Option<i64>,
    }

    let mut payload = BTreeMap::new();
    payload.insert(
        "listenToken".to_string(),
        serde_json::Value::String(token.to_string()),
    );
    let result = ws
        .send_message::<SubscribeAck>(
            "userDataStream.subscribe.listenToken",
            payload,
            WebsocketMessageSendOptions::new(),
        )
        .await
        .map_err(|e| anyhow::anyhow!("userDataStream.subscribe.listenToken failed: {e}"))?;
    let ack = match result {
        SendWebsocketMessageResult::Single(resp) => resp.data()?,
        // The subscribe is a single request; defensively take the first if the SDK ever batches.
        SendWebsocketMessageResult::Multiple(responses) => responses
            .into_iter()
            .next()
            .ok_or_else(|| anyhow::anyhow!("empty subscribe ack"))?
            .data()?,
    };
    Ok(ack.subscription_id)
}

/// Cross-margin balance-arm handler: convert an `outboundAccountPosition` into asset-keyed
/// `BalanceStreamUpdate`s (the `subscriptionId` is unused — cross is account-wide). This is the
/// `handle_position` passed to [`convert_margin_user_data_events_with`] / the cross manager.
fn cross_account_position_handler(
    position: OutboundAccountPosition,
    _subscription_id: Option<i64>,
    buf: &mut Vec<UnindexedAccountEvent>,
) {
    convert_margin_account_position(position, buf);
}

/// Cross-margin convenience wrapper over [`convert_margin_user_data_events_with`] using
/// [`cross_account_position_handler`] (account-wide `BalanceStreamUpdate`s). Test-only: the cross
/// manager calls the listener helper with the handler directly, but the converter unit tests exercise
/// frame discrimination through this two-arg form (they do not route balances per-instrument).
#[cfg(test)]
fn convert_margin_user_data_events(frame: &str, buf: &mut Vec<UnindexedAccountEvent>) -> bool {
    convert_margin_user_data_events_with(frame, buf, &mut cross_account_position_handler)
}

/// Frame discrimination for the WS-API user-data delivery, as observed live.
///
/// Each text frame is either an RPC response (`{ "id", "status", "result", … }` — e.g. the
/// subscribe ack) or a pushed user-data event wrapped as `{ "subscriptionId", "event": { "e", … } }`.
/// Returns `true` if the exchange signalled stream termination (a reconnect trigger). Unrecognised
/// frames are ignored and logged by [`log_unrecognised_frame`] (throttled `warn`); unknown event
/// types are ignored at `trace`. Deserialization of a known event type is defensive: a mismatch is logged and the
/// event dropped (observable), never silently mis-parsed.
///
/// The `outboundAccountPosition` (balance) arm is delegated to `handle_position` — the **only** arm
/// that differs between cross (account-wide `BalanceStreamUpdate`) and isolated (per-instrument
/// `InstrumentBalanceUpdate`, routed via the frame's `subscriptionId`). Everything else
/// (`executionReport` routing by inner `s`, the observable-only margin events, termination
/// signalling) is single-sourced here.
fn convert_margin_user_data_events_with(
    frame: &str,
    buf: &mut Vec<UnindexedAccountEvent>,
    handle_position: &mut impl FnMut(
        OutboundAccountPosition,
        Option<i64>,
        &mut Vec<UnindexedAccountEvent>,
    ),
) -> bool {
    let (subscription_id, event_type, event_raw) = match parse_user_data_frame(frame) {
        // RPC responses (subscribe ack, errors): not a user-data event.
        UserDataFrame::Response => return false,
        UserDataFrame::Event {
            subscription_id,
            event_type,
            event,
        } => (subscription_id, event_type, event),
        UserDataFrame::Unrecognised => {
            static SEEN: AtomicU64 = AtomicU64::new(0);
            log_unrecognised_frame("BinanceMargin", &SEEN, frame);
            return false;
        }
    };
    match event_type {
        "executionReport" => {
            // Single typed pass straight from the raw inner slice — no intermediate DOM, and only
            // the matched branch deserializes its payload.
            match serde_json::from_str::<ExecutionReport>(event_raw) {
                Ok(report) => convert_execution_report(&report, ExchangeId::BinanceMargin, buf),
                Err(e) => {
                    warn!(error = %e, "BinanceMargin: undeserializable executionReport, dropping")
                }
            }
            false
        }
        "outboundAccountPosition" => {
            match serde_json::from_str::<OutboundAccountPosition>(event_raw) {
                // Balance arm is the one cross/isolated divergence — delegate to the handler.
                Ok(position) => handle_position(position, subscription_id, buf),
                Err(e) => {
                    warn!(error = %e, "BinanceMargin: undeserializable outboundAccountPosition, dropping")
                }
            }
            false
        }
        "balanceUpdate" => {
            // Deposit/withdrawal delta; outboundAccountPosition covers trade-driven balance changes.
            // Mirrors spot — not forwarded. Consumers reconcile transfers via account_snapshot.
            // No log here: this is the per-frame receive hot path (fires on every balance change).
            false
        }
        "userLiabilityChange" => {
            // Observable, NOT accumulated into balance state: borrow/repay is a
            // delta, and folding it in would be the position tracking the library refuses. Surfaced
            // via logs; consumers needing exact live debt refresh via account_snapshot. Always
            // deserialized (rare borrow/repay path, not per-frame) so a parse failure — exchange
            // schema drift — is surfaced at WARN rather than silently dropped, even when INFO is
            // filtered out (observable failures over silent ones). The success log itself is INFO-gated.
            match serde_json::from_str::<UserLiabilityChange>(event_raw) {
                Ok(c) => {
                    if tracing::enabled!(tracing::Level::INFO) {
                        info!(
                            asset = c.a.as_deref().unwrap_or("?"),
                            kind = c.t.as_deref().unwrap_or("?"),
                            principal = c.p.as_deref().unwrap_or("?"),
                            interest = c.i.as_deref().unwrap_or("?"),
                            "BinanceMargin userLiabilityChange (observable; not applied to balance state)"
                        );
                    }
                }
                Err(e) => warn!(
                    error = %e,
                    "BinanceMargin: undeserializable userLiabilityChange, dropping"
                ),
            }
            false
        }
        "marginLevelStatusChange" => {
            // Liquidation-risk signal — observable, no policy (the library takes no defensive action).
            // The level guard skips the deserialize allocation when WARN is filtered out, mirroring
            // userLiabilityChange above. A parse failure on a liquidation-risk frame is surfaced
            // rather than dropped silently (observable failures over silent ones).
            if tracing::enabled!(tracing::Level::WARN) {
                match serde_json::from_str::<MarginLevelStatusChange>(event_raw) {
                    Ok(c) => warn!(
                        margin_level = c.l.as_deref().unwrap_or("?"),
                        status = c.s.as_deref().unwrap_or("?"),
                        "BinanceMargin marginLevelStatusChange (liquidation risk; observable, no policy)"
                    ),
                    Err(e) => warn!(
                        error = %e,
                        "BinanceMargin: undeserializable marginLevelStatusChange, dropping"
                    ),
                }
            }
            false
        }
        "eventStreamTerminated" => {
            warn!("BinanceMargin user data stream terminated by exchange, signalling reconnect");
            true
        }
        other => {
            trace!(
                event_type = other,
                "BinanceMargin ignoring unhandled user data event"
            );
            false
        }
    }
}

/// Convert a margin `outboundAccountPosition` to `BalanceStreamUpdate` events (one per asset).
///
/// The WS message is a `free`/`locked` partial (no debt), so it emits `BalanceStreamUpdate` — which
/// structurally cannot clobber the `margin` debt established by a REST `BalanceSnapshot`.
fn convert_margin_account_position(
    position: OutboundAccountPosition,
    buf: &mut Vec<UnindexedAccountEvent>,
) {
    let time_exchange = position
        .u
        .and_then(|ms| Utc.timestamp_millis_opt(ms).single())
        .unwrap_or_else(Utc::now);

    for b in position.b_uppercase.unwrap_or_default() {
        let asset = match b.a {
            Some(a) => AssetNameExchange::new(a),
            None => {
                warn!("BinanceMargin account position entry missing asset name");
                continue;
            }
        };
        let free = match b.f.as_deref().and_then(|s| Decimal::from_str(s).ok()) {
            Some(v) => v,
            None => {
                warn!(%asset, "BinanceMargin account position missing/unparseable 'free' field");
                continue;
            }
        };
        let locked = match b.l.as_deref().and_then(|s| Decimal::from_str(s).ok()) {
            Some(v) => v,
            None => {
                warn!(%asset, "BinanceMargin account position missing/unparseable 'locked' field");
                continue;
            }
        };
        let update =
            AssetBalanceUpdate::new(asset, BalanceUpdate::new(free, locked), time_exchange);
        buf.push(UnindexedAccountEvent::new(
            ExchangeId::BinanceMargin,
            AccountEventKind::BalanceStreamUpdate(
                rustrade_integration::collection::snapshot::Snapshot::new(update),
            ),
        ));
    }
}

/// Route an isolated `outboundAccountPosition` frame to an [`InstrumentBalanceUpdate`] (Tier B).
///
/// The frame carries no symbol inline, so the pair is recovered from `subscription_id` via
/// `sub_map` (populated from the per-symbol subscribe acks). The flat `b[]` asset list is then
/// split into `base`/`quote` by **asset-name equality** against the pair's known `(base, quote)`
/// from `base_quote` (the authoritative startup map — never string-matched against the symbol).
/// Emits one `InstrumentBalanceUpdate` carrying both sides' free/locked. Dropped with a `warn!`
/// (observable, never mis-applied) when: the `subscriptionId` is missing/unmapped; the pair's
/// base/quote is unknown; or either side is absent/unparseable in the frame. The engine does not
/// store this variant (`_ => None` wildcard); the wrapper consumes it off the account-event feed.
fn route_isolated_account_position(
    position: OutboundAccountPosition,
    subscription_id: Option<i64>,
    sub_map: &Mutex<HashMap<i64, InstrumentNameExchange>>,
    base_quote: &HashMap<InstrumentNameExchange, (AssetNameExchange, AssetNameExchange)>,
    buf: &mut Vec<UnindexedAccountEvent>,
) {
    let Some(sub_id) = subscription_id else {
        warn!(
            "BinanceMargin isolated: outboundAccountPosition without subscriptionId — dropping \
             (per-instrument balance routing requires it)"
        );
        return;
    };
    let instrument = {
        // Brief lock: the map is read-only after startup and only written by the subscribe loop on
        // (re)connect. Recover from poisoning rather than killing balance routing.
        let map = sub_map.lock().unwrap_or_else(|p| p.into_inner());
        match map.get(&sub_id) {
            Some(inst) => inst.clone(),
            None => {
                warn!(
                    subscription_id = sub_id,
                    "BinanceMargin isolated: outboundAccountPosition for unmapped subscriptionId — dropping"
                );
                return;
            }
        }
    };
    let Some((base_asset, quote_asset)) = base_quote.get(&instrument) else {
        warn!(
            instrument = %instrument.name(),
            "BinanceMargin isolated: no base/quote known for instrument — dropping balance frame"
        );
        return;
    };

    let time_exchange = position
        .u
        .and_then(|ms| Utc.timestamp_millis_opt(ms).single())
        .unwrap_or_else(Utc::now);

    let mut base_update: Option<AssetBalanceUpdate<AssetNameExchange>> = None;
    let mut quote_update: Option<AssetBalanceUpdate<AssetNameExchange>> = None;
    for b in position.b_uppercase.unwrap_or_default() {
        let Some(asset) = b.a.as_deref() else {
            warn!(instrument = %instrument.name(), "BinanceMargin isolated account position entry missing asset name");
            continue;
        };
        // Only the pair's own base/quote are relevant; ignore any stray asset on the frame.
        let side = if asset == base_asset.name().as_str() {
            &mut base_update
        } else if asset == quote_asset.name().as_str() {
            &mut quote_update
        } else {
            continue;
        };
        let (Some(free), Some(locked)) = (
            b.f.as_deref().and_then(|s| Decimal::from_str(s).ok()),
            b.l.as_deref().and_then(|s| Decimal::from_str(s).ok()),
        ) else {
            warn!(%asset, instrument = %instrument.name(), "BinanceMargin isolated account position missing/unparseable free/locked");
            continue;
        };
        *side = Some(AssetBalanceUpdate::new(
            AssetNameExchange::new(asset),
            BalanceUpdate::new(free, locked),
            time_exchange,
        ));
    }

    // InstrumentBalanceUpdate carries BOTH sides; a frame missing either is dropped rather than
    // fabricating a zero side (which would silently report a wrong free/locked). A trade-driven
    // outboundAccountPosition changes both base and quote together, so both are normally present.
    let (Some(base), Some(quote)) = (base_update, quote_update) else {
        warn!(
            instrument = %instrument.name(),
            "BinanceMargin isolated: outboundAccountPosition missing base or quote side — dropping (no partial InstrumentBalanceUpdate)"
        );
        return;
    };
    buf.push(UnindexedAccountEvent::new(
        ExchangeId::BinanceMargin,
        AccountEventKind::InstrumentBalanceUpdate(InstrumentBalanceUpdate::new(
            instrument, base, quote,
        )),
    ));
}

/// Register the WS-API event-dispatch callback shared by both margin managers.
///
/// Single-sources the demux/dedup/dispatch + heartbeat + terminal-signal logic the cross and
/// isolated managers both need; only the `outboundAccountPosition` (balance) arm differs and is
/// supplied as `handle_position` (cross → account-wide `BalanceStreamUpdate` via
/// [`cross_account_position_handler`]; isolated → per-instrument `InstrumentBalanceUpdate` via
/// [`route_isolated_account_position`]). Returns the [`Subscription`] handle; the caller must
/// `unsubscribe()` it on disconnect. `heartbeat_flag` is shared with the caller's monitor loop
/// (set on every inbound frame/ping); `signal_tx` fires once on a terminal condition
/// (consumer-drop, socket error/close, or exchange `eventStreamTerminated`).
fn register_user_data_listener(
    ws: &Arc<WsApiBase>,
    tx: mpsc::UnboundedSender<UnindexedAccountEvent>,
    dedup: SharedDedupCache,
    heartbeat_flag: Arc<AtomicBool>,
    signal_tx: oneshot::Sender<()>,
    mut handle_position: impl FnMut(
        OutboundAccountPosition,
        Option<i64>,
        &mut Vec<UnindexedAccountEvent>,
    ) + Send
    + 'static,
) -> Subscription {
    let mut signal_tx_opt = Some(signal_tx);
    let mut event_tx = Some(tx);
    // 32 comfortably covers a typical account's outboundAccountPosition (one entry per asset) plus
    // the execution events in a single message; an account with >32 assets reallocates once, after
    // which the buffer is reused across messages via drain(..).
    let mut event_buf = Vec::with_capacity(32);

    // Safety — non-atomic Option::take() in the callback is safe: binance-sdk drives one spawned
    // task per subscription, invoking the FnMut sequentially (verified =70.2.0; re-verify on SDK
    // upgrade). Same contract spot relies on.
    ws.common.events.subscribe(move |event| {
        let Some(ref sender) = event_tx else { return };
        match event {
            WebsocketEvent::Message(json_str) => {
                heartbeat_flag.store(true, Ordering::Release);
                // Frame parsing (including skipping non-JSON frames) happens inside the converter,
                // which parses borrowed slices rather than a full DOM (hot path).
                let terminated = convert_margin_user_data_events_with(
                    &json_str,
                    &mut event_buf,
                    &mut handle_position,
                );
                for ev in event_buf.drain(..) {
                    if let Some(key) = dedup_key_from_event(&ev)
                        && is_duplicate(&dedup, key)
                    {
                        trace!("BinanceMargin dedup: skipping duplicate event");
                        continue;
                    }
                    if sender.send(ev).is_err() {
                        warn!("BinanceMargin account_stream receiver dropped, suppressing sends");
                        event_tx.take();
                        if let Some(s) = signal_tx_opt.take() {
                            let _ = s.send(());
                        }
                        return;
                    }
                }
                if terminated {
                    event_tx.take();
                    if let Some(s) = signal_tx_opt.take() {
                        let _ = s.send(());
                    }
                }
            }
            WebsocketEvent::Ping | WebsocketEvent::Pong => {
                heartbeat_flag.store(true, Ordering::Release);
            }
            WebsocketEvent::Error(e) => {
                warn!(%e, "BinanceMargin WebSocket error, will attempt reconnect");
                event_tx.take();
                if let Some(s) = signal_tx_opt.take() {
                    let _ = s.send(());
                }
            }
            WebsocketEvent::Close(code, reason) => {
                warn!(code, %reason, "BinanceMargin WebSocket closed");
                event_tx.take();
                if let Some(s) = signal_tx_opt.take() {
                    let _ = s.send(());
                }
            }
            _ => {
                trace!("BinanceMargin ignoring unhandled WebsocketEvent variant");
            }
        }
    })
}

/// Recover the fills of every gap in `unrecovered` that is due, via REST `myTrades`, routed
/// through the dedup cache. Mirrors `BinanceSpot::recover_fills`, which documents how gaps are
/// settled and retried.
///
/// Only TRADE fills are recovered — order-lifecycle events (NEW/CANCELED) require a
/// `fetch_open_orders` reconciliation by the caller.
///
/// `myTrades` reports executions only, with no cumulative, so each recovered trade's
/// `order_filled_quantity` is rebuilt from its order's executions by
/// [`recovered_order_totals`]. A trade whose order could not be looked up keeps `None`.
///
/// Margin `myTrades` is read by time in windows under 24 hours (see
/// [`paginate_margin_my_trades`]), so each day of a gap before its first fill costs one sequential
/// request. A gap of weeks can therefore run into [`FILL_RECOVERY_TIMEOUT_SECS`], which bounds the
/// whole recovery; it is then retried, and given up once it has failed every retry.
///
/// Fills forwarded before the deadline stay delivered. Its reads are [`RequestKind::Essential`]:
/// they wait out a rate-limit cooldown but not the pause near the weight limit, which could
/// outlast the budget.
async fn recover_margin_fills(
    rest: &Arc<RestApi>,
    rate_limiter: &Arc<RateLimitTracker>,
    unrecovered: &mut UnrecoveredFills,
    tx: &mpsc::UnboundedSender<UnindexedAccountEvent>,
    dedup: &SharedDedupCache,
    is_isolated: bool,
) {
    use futures::StreamExt as _;

    let due = unrecovered.due(tokio::time::Instant::now());
    let Some(oldest) = due.iter().map(|(_, gap)| gap.start_ms).min() else {
        return;
    };
    info!(
        gaps = due.len(),
        oldest = %gap_time(oldest),
        is_isolated,
        "BinanceMargin recovering fills missed while disconnected"
    );

    // Which gaps of `due` have been settled, recovered or failed, and how many have started: the
    // stream starts them in order, eight at a time, so those from `started` on were never read.
    let mut settled = vec![false; due.len()];
    let started = AtomicUsize::new(0);
    let recovery = async {
        let order_executions_deadline = tokio::time::Instant::now() + ORDER_EXECUTIONS_BUDGET;
        let mut recovered = 0u32;
        let mut duplicates = 0u32;

        let mut stream =
            futures::stream::iter(due.iter().cloned().enumerate().map(|(index, (inst, gap))| {
                started.store(index + 1, Ordering::Relaxed);
                let rest = rest.clone();
                let rl = rate_limiter.clone();
                async move {
                    let span = MyTradesFrom::Span {
                        start: gap.start_ms,
                        end: gap.end_ms,
                    };
                    let raw = match paginate_margin_my_trades(
                        &rest,
                        &rl,
                        &inst,
                        span,
                        is_isolated,
                        RequestKind::Essential,
                    )
                    .await
                    {
                        Ok(pages) => pages,
                        Err(e) => return (index, Err(e)),
                    };
                    // `myTrades` carries no cumulative, so each recovered fill's is rebuilt from
                    // its order's executions; without it the fill advances the position but not
                    // the order.
                    let totals = recovered_order_totals(
                        ExchangeId::BinanceMargin,
                        &inst,
                        &raw,
                        order_executions_deadline,
                        |order_id| {
                            paginate_margin_my_trades(
                                &rest,
                                &rl,
                                &inst,
                                MyTradesFrom::Order(order_id),
                                is_isolated,
                                RequestKind::Essential,
                            )
                        },
                    )
                    .await;
                    let trades = raw
                        .iter()
                        .filter_map(|t| {
                            let mut trade = convert_margin_trade(t, &inst)?;
                            trade.order_filled_quantity =
                                t.id.and_then(|id| totals.get(&id).copied());
                            Some(trade)
                        })
                        .collect::<Vec<_>>();
                    (index, Ok(trades))
                }
            }))
            .buffer_unordered(8);
        while let Some((index, result)) = stream.next().await {
            let (inst, gap) = &due[index];
            settled[index] = true;
            let trades = match result {
                Ok(trades) => trades,
                Err(e) => {
                    gap_failed(
                        unrecovered,
                        ExchangeId::BinanceMargin,
                        inst,
                        gap,
                        &e.to_string(),
                    );
                    continue;
                }
            };
            for trade in trades {
                let event = UnindexedAccountEvent::new(
                    ExchangeId::BinanceMargin,
                    AccountEventKind::Trade(trade),
                );
                if let Some(key) = dedup_key_from_event(&event)
                    && is_duplicate(dedup, key)
                {
                    duplicates += 1;
                    continue;
                }
                if tx.send(event).is_err() {
                    debug!("BinanceMargin fill recovery: consumer dropped during recovery");
                    return;
                }
                recovered += 1;
            }
            unrecovered.recovered(inst, gap);
        }
        info!(
            recovered,
            duplicates, is_isolated, "BinanceMargin fill recovery complete"
        );
    };
    // A timeout drops `recovery` at an await, between gaps: one gap's fills are sent without
    // awaiting, so each is either fully forwarded and settled, or not forwarded at all. A gap
    // whose read started and did not finish has failed; one never started stays due as it was.
    if tokio::time::timeout(Duration::from_secs(FILL_RECOVERY_TIMEOUT_SECS), recovery)
        .await
        .is_err()
    {
        let started = started.load(Ordering::Relaxed);
        for ((inst, gap), _) in due[..started]
            .iter()
            .zip(&settled)
            .filter(|(_, settled)| !**settled)
        {
            gap_failed(
                unrecovered,
                ExchangeId::BinanceMargin,
                inst,
                gap,
                "fill recovery timed out",
            );
        }
    }
}

/// Long-running task driving the `account_stream` WebSocket lifecycle.
///
/// Reconnect loop: acquire token → connect → register listener → subscribe → stream events → on
/// disconnect/heartbeat-timeout/token-expiry → backoff → fill recovery → reconnect. The `tx`
/// channel persists across reconnections so the consumer sees a seamless stream. Terminates when
/// the consumer drops the stream or max reconnect attempts are exhausted.
#[allow(
    clippy::cognitive_complexity,
    reason = "inherent reconnect-loop complexity (token + connect + subscribe + callback + recovery \
              + monitor + cleanup + backoff); mirrors spot's `connection_manager`, not worth splitting further"
)]
async fn margin_connection_manager(
    tx: mpsc::UnboundedSender<UnindexedAccountEvent>,
    dedup: SharedDedupCache,
    ws_config: ConfigurationWebsocketApi,
    rest_config: Arc<ConfigurationRestApi>,
    rest: Arc<RestApi>,
    rate_limiter: Arc<RateLimitTracker>,
    instruments: Vec<InstrumentNameExchange>,
    initial: Option<(Arc<WsApiBase>, UserListenToken)>,
) {
    enum DisconnectReason {
        Signal,
        HeartbeatTimeout,
        TokenRefresh,
        ConsumerDropped,
    }

    let mut backoff = ExponentialBackoff::new();
    let mut disconnect_time: Option<DateTime<Utc>> = None;
    // Gaps not yet recovered: read at reconnect, and retried on a timer while connected, once due.
    let mut unrecovered = UnrecoveredFills::default();
    let (mut current_ws, mut current_token) = match initial {
        Some((ws, token)) => (Some(ws), Some(token)),
        None => (None, None),
    };

    loop {
        // --- Token: reuse the verified/previous token, else acquire a fresh one ---
        let token = match current_token.take() {
            Some(t) => t,
            None => match acquire_user_listen_token(&rest_config, &rate_limiter, None).await {
                Ok(t) => t,
                Err(e) => {
                    error!(%e, "BinanceMargin userListenToken acquisition failed");
                    if !backoff.wait().await {
                        error!("BinanceMargin max reconnect attempts exhausted");
                        emit_stream_terminated(
                            &tx,
                            ExchangeId::BinanceMargin,
                            StreamTerminationReason::ReconnectBudgetExhausted {
                                attempts: backoff.attempts(),
                                last_error: e.to_string(),
                            },
                        );
                        break;
                    }
                    continue;
                }
            },
        };

        // --- Connect (skip if holding a verified initial connection) ---
        let ws = match current_ws.take() {
            Some(ws) => ws,
            None => match connect_margin_ws(&ws_config).await {
                Ok(ws) => ws,
                Err(e) => {
                    error!(%e, "BinanceMargin WS connect failed");
                    if !backoff.wait().await {
                        error!("BinanceMargin max reconnect attempts exhausted");
                        emit_stream_terminated(
                            &tx,
                            ExchangeId::BinanceMargin,
                            StreamTerminationReason::ReconnectBudgetExhausted {
                                attempts: backoff.attempts(),
                                last_error: e.to_string(),
                            },
                        );
                        break;
                    }
                    continue;
                }
            },
        };

        // --- Register the event listener BEFORE subscribing so no pushed event is missed ---
        let (signal_tx, signal_rx) = oneshot::channel::<()>();
        // start true — grant one full heartbeat window before requiring activity.
        let heartbeat_flag = Arc::new(AtomicBool::new(true));
        // Cross is account-wide: balance frames become asset-keyed BalanceStreamUpdates (no
        // per-instrument routing). The shared dispatch/dedup/heartbeat logic lives in the helper.
        let subscription = register_user_data_listener(
            &ws,
            tx.clone(),
            dedup.clone(),
            heartbeat_flag.clone(),
            signal_tx,
            cross_account_position_handler,
        );

        // --- Subscribe (token is the sole auth) ---
        if let Err(e) = subscribe_listen_token(&ws, &token.token).await {
            warn!(%e, "BinanceMargin user-data subscribe failed, cleaning up and retrying");
            subscription.unsubscribe();
            if let Err(de) = ws.disconnect().await {
                warn!(%de, "BinanceMargin failed to disconnect after subscribe failure");
            }
            // token left consumed → next iteration acquires a fresh one (auth/expiry-safe).
            if !backoff.wait().await {
                error!("BinanceMargin max reconnect attempts exhausted");
                emit_stream_terminated(
                    &tx,
                    ExchangeId::BinanceMargin,
                    StreamTerminationReason::ReconnectBudgetExhausted {
                        attempts: backoff.attempts(),
                        last_error: e.to_string(),
                    },
                );
                break;
            }
            continue;
        }
        info!("BinanceMargin account_stream connected and subscribed");
        // Reaching a live, subscribed connection clears any prior failure count so only *consecutive*
        // failures exhaust the reconnect budget — a clean rotation or a transient blip that recovers
        // shouldn't leave the backoff counter inflated for the next genuine failure.
        backoff.reset();

        // Absolute deadline (not a per-iteration relative sleep) so heartbeat ticks that `continue`
        // the monitor loop don't keep restarting the renewal timer.
        let token_deadline =
            tokio::time::Instant::now() + token_renew_after(token.expiration_time_ms);

        // --- Fill recovery after a reconnect (bounded) ---
        // The gap is opened before it is read, so a recovery that fails or times out keeps it.
        if let Some(dt) = disconnect_time.take() {
            unrecovered.open(&instruments, dt, Utc::now());
        }
        // This is the cross manager (account-wide); fill recovery is always cross-scoped.
        // The isolated manager (a separate path) passes `true`.
        recover_margin_fills(&rest, &rate_limiter, &mut unrecovered, &tx, &dedup, false).await;

        // --- Monitor: disconnect signal, heartbeat timeout, token refresh, or consumer drop ---
        let reason = {
            let mut signal_rx = signal_rx;
            // Gaps a recovery did not read are retried as they fall due, alongside the monitor, so
            // a disconnect is still seen at once. It never completes; dropping it when the monitor
            // ends loses nothing, since each gap is settled as soon as its read ends.
            let retry_gaps = async {
                loop {
                    match unrecovered.next_due() {
                        Some(due) => tokio::time::sleep_until(due).await,
                        None => std::future::pending::<()>().await,
                    }
                    recover_margin_fills(
                        &rest,
                        &rate_limiter,
                        &mut unrecovered,
                        &tx,
                        &dedup,
                        false,
                    )
                    .await;
                }
            };
            tokio::pin!(retry_gaps);
            loop {
                tokio::select! {
                    // Biased: a consumer drop is terminal and must win over a simultaneously-ready
                    // disconnect signal, which would otherwise enqueue one pointless reconnect.
                    biased;
                    _ = tx.closed() => {
                        debug!("BinanceMargin account_stream consumer dropped, terminating");
                        break DisconnectReason::ConsumerDropped;
                    }
                    _ = &mut signal_rx => {
                        warn!("BinanceMargin WS disconnected, will attempt reconnect");
                        break DisconnectReason::Signal;
                    }
                    () = tokio::time::sleep_until(token_deadline) => {
                        info!("BinanceMargin userListenToken nearing expiry, renewing");
                        break DisconnectReason::TokenRefresh;
                    }
                    () = tokio::time::sleep(Duration::from_secs(HEARTBEAT_TIMEOUT_SECS)) => {
                        if heartbeat_flag.swap(false, Ordering::AcqRel) {
                            // Backoff is already cleared post-subscribe (see backoff.reset() above);
                            // the monitor loop is only entered on a live connection, so a heartbeat
                            // tick never sees a non-zero count. Just consume the flag and keep waiting.
                            continue;
                        }
                        warn!("BinanceMargin heartbeat timeout ({}s), will attempt reconnect", HEARTBEAT_TIMEOUT_SECS);
                        break DisconnectReason::HeartbeatTimeout;
                    }
                    // Last: the arms above end the monitor and win when ready together.
                    () = &mut retry_gaps => {}
                }
            }
        };
        let should_reconnect = !matches!(reason, DisconnectReason::ConsumerDropped);

        if should_reconnect {
            disconnect_time = Some(match reason {
                DisconnectReason::HeartbeatTimeout => {
                    Utc::now()
                        - chrono::Duration::seconds(HEARTBEAT_TIMEOUT_SECS as i64)
                        - chrono::Duration::milliseconds(SIGNAL_RECOVERY_LOOKBACK_MS)
                }
                DisconnectReason::TokenRefresh => {
                    // A rotation fully disconnects + reconnects (token POST + WS connect + subscribe).
                    // Look back far enough to bound that whole window — the WS handshake alone is
                    // capped at CONNECT_TIMEOUT_SECS — so a fill landing during the gap isn't missed.
                    // The dedup cache absorbs the redundant re-query of fills delivered pre-rotation.
                    Utc::now()
                        - chrono::Duration::seconds(CONNECT_TIMEOUT_SECS as i64)
                        - chrono::Duration::milliseconds(SIGNAL_RECOVERY_LOOKBACK_MS)
                }
                _ => Utc::now() - chrono::Duration::milliseconds(SIGNAL_RECOVERY_LOOKBACK_MS),
            });
        }

        // --- Cleanup ---
        subscription.unsubscribe();
        if let Err(e) = ws.disconnect().await {
            warn!(%e, "BinanceMargin failed to disconnect WebSocket");
        }

        if !should_reconnect || tx.is_closed() {
            // Consumer dropped the receiver — no StreamTerminated emit (channel already closed).
            debug!("BinanceMargin connection manager exiting");
            break;
        }

        // A planned token refresh is not a failure — reconnect immediately (no backoff). Both
        // `current_token` and `current_ws` are already None, forcing a fresh token + connection.
        if !matches!(reason, DisconnectReason::TokenRefresh) && !backoff.wait().await {
            error!("BinanceMargin max reconnect attempts exhausted, stream terminating");
            // reason here is Signal or HeartbeatTimeout (TokenRefresh is guarded above,
            // ConsumerDropped already broke out earlier).
            let last_error = match reason {
                DisconnectReason::HeartbeatTimeout => {
                    format!("heartbeat timeout ({HEARTBEAT_TIMEOUT_SECS}s)")
                }
                _ => "WebSocket disconnected (server close/error)".to_string(),
            };
            emit_stream_terminated(
                &tx,
                ExchangeId::BinanceMargin,
                StreamTerminationReason::ReconnectBudgetExhausted {
                    attempts: backoff.attempts(),
                    last_error,
                },
            );
            break;
        }
    }
}

// ---------------------------------------------------------------------------
// Isolated user-data stream (separate multiplexed manager; cross untouched)
// ---------------------------------------------------------------------------

/// Extract the invariant `instrument → (base_asset, quote_asset)` map from an isolated
/// account-info response.
///
/// The authoritative base/quote split for routing symbol-less `outboundAccountPosition` frames
/// ([`route_isolated_account_position`]) — base/quote never change for a pair,
/// so this is built once at stream start and reused across reconnects. Entries missing a symbol or
/// either asset name are skipped (their balance frames then drop with a `warn!` at routing time).
fn build_base_quote_map(
    assets: &[QueryIsolatedMarginAccountInfoResponseAssetsInner],
) -> HashMap<InstrumentNameExchange, (AssetNameExchange, AssetNameExchange)> {
    let mut map = HashMap::with_capacity(assets.len());
    for entry in assets {
        let (Some(symbol), Some(base), Some(quote)) = (
            entry.symbol.as_deref(),
            entry.base_asset.as_deref().and_then(|b| b.asset.as_deref()),
            entry
                .quote_asset
                .as_deref()
                .and_then(|q| q.asset.as_deref()),
        ) else {
            warn!(
                "BinanceMargin isolated: account-info entry missing symbol/base/quote asset — \
                 skipping base/quote map entry"
            );
            continue;
        };
        map.insert(
            InstrumentNameExchange::new(symbol),
            (AssetNameExchange::new(base), AssetNameExchange::new(quote)),
        );
    }
    map
}

/// Acquire one per-symbol isolated `userListenToken` for every configured symbol, bounded-concurrent.
///
/// Fans out at 8-wide (mirroring `recover_margin_fills`) through the shared rate-limit/backoff
/// machinery — ~1s at the v1 N≤100 ceiling vs ~5–10s sequential. Each token is
/// a weight-1 signed POST; the bound stays well inside SAPI weight limits. Errors if any acquisition
/// fails (the caller treats a partial set as a startup/reconnect failure).
///
/// Takes `&Arc<_>` rather than the file-wide `&RateLimitTracker` convention because each per-symbol
/// `async move` future needs an owned `Arc` clone to share the tracker without borrowing from this
/// frame across the concurrent `buffer_unordered` fan-out. Cloning the `Arc` (a refcount bump) is
/// correct; cloning `RateLimitTracker` itself would be semantically wrong — each clone would get its
/// own `blocked_until`, splitting the shared rate-limit state the fan-out is meant to respect.
async fn acquire_all_isolated_tokens(
    rest_config: &Arc<ConfigurationRestApi>,
    rate_limiter: &Arc<RateLimitTracker>,
    symbols: &[InstrumentNameExchange],
) -> Result<Vec<(InstrumentNameExchange, UserListenToken)>, UnindexedClientError> {
    use futures::{StreamExt as _, TryStreamExt as _};

    futures::stream::iter(symbols.iter().cloned().map(|sym| {
        let rest_config = rest_config.clone();
        let rate_limiter = rate_limiter.clone();
        async move {
            let token = acquire_user_listen_token(&rest_config, &rate_limiter, Some(&sym)).await?;
            Ok::<_, UnindexedClientError>((sym, token))
        }
    }))
    .buffer_unordered(8)
    .try_collect()
    .await
}

/// Earliest `expirationTime` (epoch ms) across the per-symbol tokens — drives the planned-renewal
/// deadline. The N tokens are acquired in a tight startup loop so their 24h expiries cluster;
/// renewing on the earliest refreshes the whole set in one reconnect. Empty → `i64::MAX` (no token
/// to expire; never happens for isolated, which always has ≥1 configured symbol).
fn earliest_token_expiry_ms(tokens: &[(InstrumentNameExchange, UserListenToken)]) -> i64 {
    tokens
        .iter()
        .map(|(_, t)| t.expiration_time_ms)
        .min()
        .unwrap_or(i64::MAX)
}

/// A live, subscribed isolated connection: the single multiplexed socket plus the per-connection
/// lifecycle handles the manager's monitor loop needs.
struct IsolatedLiveConn {
    ws: Arc<WsApiBase>,
    subscription: Subscription,
    signal_rx: oneshot::Receiver<()>,
    heartbeat_flag: Arc<AtomicBool>,
    /// Earliest `expirationTime` across the N tokens — drives the planned-renewal deadline (the
    /// tokens cluster, so renewing on the earliest refreshes the whole set in one reconnect).
    earliest_expiry_ms: i64,
}

/// Connect one WS-API socket and subscribe all N per-symbol `userListenToken`s on it (multiplex).
///
/// Registers the shared dispatch listener (with the isolated per-instrument balance handler) BEFORE
/// the first subscribe so no pushed event between the N acks is missed, then issues all N subscribes,
/// capturing each ack's `subscriptionId` into the shared `subscriptionId → symbol` routing map. If
/// **any** subscribe fails, the partially-subscribed socket is cleaned up and the error returned
/// (startup → `Err` with nothing spawned; reconnect → retry with backoff). Reused by both the
/// initial `account_stream` startup and the manager's reconnect path so connect+subscribe is
/// single-sourced.
async fn isolated_connect_and_subscribe(
    ws_config: &ConfigurationWebsocketApi,
    tx: &mpsc::UnboundedSender<UnindexedAccountEvent>,
    dedup: &SharedDedupCache,
    base_quote: &Arc<HashMap<InstrumentNameExchange, (AssetNameExchange, AssetNameExchange)>>,
    tokens: &[(InstrumentNameExchange, UserListenToken)],
) -> anyhow::Result<IsolatedLiveConn> {
    let ws = connect_margin_ws(ws_config).await?;
    // subscriptionId → symbol routing map: written here as each subscribe ack resolves, read by the
    // dispatch callback on every `outboundAccountPosition`. Shared (Mutex) because the two run on
    // different tasks (manager vs the SDK's per-subscription callback task); contention is negligible
    // (writes only at (re)connect, reads only on balance frames).
    // Sized to the token count up front — exactly one entry is inserted per subscribe ack below.
    let sub_map = Arc::new(Mutex::new(
        HashMap::<i64, InstrumentNameExchange>::with_capacity(tokens.len()),
    ));
    let (signal_tx, signal_rx) = oneshot::channel::<()>();
    // start true — grant one full heartbeat window before requiring activity.
    let heartbeat_flag = Arc::new(AtomicBool::new(true));

    let handle_position = {
        let sub_map = sub_map.clone();
        let base_quote = base_quote.clone();
        move |position, subscription_id, buf: &mut Vec<UnindexedAccountEvent>| {
            route_isolated_account_position(position, subscription_id, &sub_map, &base_quote, buf);
        }
    };
    // Register BEFORE subscribing so a pushed event arriving mid-fan-out is not missed.
    let subscription = register_user_data_listener(
        &ws,
        tx.clone(),
        dedup.clone(),
        heartbeat_flag.clone(),
        signal_tx,
        handle_position,
    );

    let earliest_expiry_ms = earliest_token_expiry_ms(tokens);
    for (sym, token) in tokens {
        match subscribe_listen_token_capture(&ws, &token.token).await {
            Ok(Some(id)) => {
                sub_map
                    .lock()
                    .unwrap_or_else(|p| p.into_inner())
                    .insert(id, sym.clone());
            }
            Ok(None) => warn!(
                symbol = %sym.name(),
                "BinanceMargin isolated: subscribe ack carried no subscriptionId — live per-pair \
                 balances unavailable for this symbol (fills/orders unaffected; balances via snapshot)"
            ),
            Err(e) => {
                // Any subscribe failing voids the whole stream — clean up the partially-subscribed
                // socket rather than leaking it, and surface the error to the caller.
                warn!(%e, symbol = %sym.name(), "BinanceMargin isolated subscribe failed");
                subscription.unsubscribe();
                if let Err(de) = ws.disconnect().await {
                    warn!(%de, "BinanceMargin isolated: disconnect after subscribe failure failed");
                }
                return Err(e);
            }
        }
    }

    Ok(IsolatedLiveConn {
        ws,
        subscription,
        signal_rx,
        heartbeat_flag,
        earliest_expiry_ms,
    })
}

/// Long-running task driving the isolated `account_stream` lifecycle over one multiplexed socket.
///
/// Mirrors [`margin_connection_manager`] (the cross manager, left untouched) but for the N-symbol
/// multiplex: reconnect re-acquires all N tokens + re-subscribes all N from the configured symbol
/// set; renewal is a planned reconnect on the earliest token deadline (`DisconnectReason::TokenRefresh`,
/// reusing cross's `token_renew_after`/`sleep_until` discipline — no make-before-break). One socket
/// ⇒ one heartbeat, one reconnect loop, one backoff. The `base_quote` map is invariant and reused
/// across reconnects (never rebuilt).
#[allow(
    clippy::cognitive_complexity,
    reason = "inherent reconnect-loop complexity (tokens + connect + subscribe + monitor + cleanup \
              + backoff); mirrors the cross manager, not worth splitting further"
)]
async fn isolated_connection_manager(
    tx: mpsc::UnboundedSender<UnindexedAccountEvent>,
    dedup: SharedDedupCache,
    ws_config: ConfigurationWebsocketApi,
    rest_config: Arc<ConfigurationRestApi>,
    rest: Arc<RestApi>,
    rate_limiter: Arc<RateLimitTracker>,
    symbols: Vec<InstrumentNameExchange>,
    base_quote: Arc<HashMap<InstrumentNameExchange, (AssetNameExchange, AssetNameExchange)>>,
    initial: Option<IsolatedLiveConn>,
) {
    enum DisconnectReason {
        Signal,
        HeartbeatTimeout,
        TokenRefresh,
        ConsumerDropped,
    }

    let mut backoff = ExponentialBackoff::new();
    let mut disconnect_time: Option<DateTime<Utc>> = None;
    // Gaps not yet recovered: read at reconnect, and retried on a timer while connected, once due.
    let mut unrecovered = UnrecoveredFills::default();
    let mut current = initial;

    loop {
        // --- (Re)establish the live, subscribed connection ---
        let IsolatedLiveConn {
            ws,
            subscription,
            signal_rx,
            heartbeat_flag,
            earliest_expiry_ms,
        } = match current.take() {
            // Verified initial connection from account_stream — used as-is on the first pass.
            Some(live) => live,
            None => {
                // Reconnect: re-acquire all N tokens (they cluster; a planned refresh re-acquires the
                // whole set) then connect a fresh socket and re-subscribe all N.
                let tokens = match acquire_all_isolated_tokens(
                    &rest_config,
                    &rate_limiter,
                    &symbols,
                )
                .await
                {
                    Ok(t) => t,
                    Err(e) => {
                        error!(%e, "BinanceMargin isolated userListenToken acquisition failed");
                        if !backoff.wait().await {
                            error!("BinanceMargin isolated max reconnect attempts exhausted");
                            emit_stream_terminated(
                                &tx,
                                ExchangeId::BinanceMargin,
                                StreamTerminationReason::ReconnectBudgetExhausted {
                                    attempts: backoff.attempts(),
                                    last_error: e.to_string(),
                                },
                            );
                            break;
                        }
                        continue;
                    }
                };
                match isolated_connect_and_subscribe(&ws_config, &tx, &dedup, &base_quote, &tokens)
                    .await
                {
                    Ok(live) => live,
                    Err(e) => {
                        error!(%e, "BinanceMargin isolated connect/subscribe failed");
                        if !backoff.wait().await {
                            error!("BinanceMargin isolated max reconnect attempts exhausted");
                            emit_stream_terminated(
                                &tx,
                                ExchangeId::BinanceMargin,
                                StreamTerminationReason::ReconnectBudgetExhausted {
                                    attempts: backoff.attempts(),
                                    last_error: e.to_string(),
                                },
                            );
                            break;
                        }
                        continue;
                    }
                }
            }
        };

        info!(
            symbols = symbols.len(),
            "BinanceMargin isolated account_stream connected and subscribed"
        );
        // Reaching a live, subscribed connection clears any prior failure count (see the cross
        // manager) so only *consecutive* failures exhaust the reconnect budget.
        backoff.reset();

        // Absolute deadline (not a per-iteration relative sleep) so heartbeat ticks that `continue`
        // the monitor loop don't keep restarting the renewal timer. Earliest token expiry drives it.
        let token_deadline = tokio::time::Instant::now() + token_renew_after(earliest_expiry_ms);

        // --- Fill recovery after a reconnect (isolated-scoped over the full symbol set) ---
        // The gap is opened before it is read, so a recovery that fails or times out keeps it.
        if let Some(dt) = disconnect_time.take() {
            unrecovered.open(&symbols, dt, Utc::now());
        }
        // is_isolated = true: paginate_margin_my_trades must query isolated trades.
        recover_margin_fills(&rest, &rate_limiter, &mut unrecovered, &tx, &dedup, true).await;

        // --- Monitor: disconnect signal, heartbeat timeout, token refresh, or consumer drop ---
        // One socket regardless of N, so the conditions are identical to cross.
        let reason = {
            let mut signal_rx = signal_rx;
            // Gaps a recovery did not read are retried as they fall due, alongside the monitor, so
            // a disconnect is still seen at once. It never completes; dropping it when the monitor
            // ends loses nothing, since each gap is settled as soon as its read ends.
            let retry_gaps = async {
                loop {
                    match unrecovered.next_due() {
                        Some(due) => tokio::time::sleep_until(due).await,
                        None => std::future::pending::<()>().await,
                    }
                    recover_margin_fills(&rest, &rate_limiter, &mut unrecovered, &tx, &dedup, true)
                        .await;
                }
            };
            tokio::pin!(retry_gaps);
            loop {
                tokio::select! {
                    biased;
                    _ = tx.closed() => {
                        debug!("BinanceMargin isolated consumer dropped, terminating");
                        break DisconnectReason::ConsumerDropped;
                    }
                    _ = &mut signal_rx => {
                        warn!("BinanceMargin isolated WS disconnected, will attempt reconnect");
                        break DisconnectReason::Signal;
                    }
                    () = tokio::time::sleep_until(token_deadline) => {
                        info!("BinanceMargin isolated userListenToken nearing expiry, renewing all");
                        break DisconnectReason::TokenRefresh;
                    }
                    () = tokio::time::sleep(Duration::from_secs(HEARTBEAT_TIMEOUT_SECS)) => {
                        if heartbeat_flag.swap(false, Ordering::AcqRel) {
                            continue;
                        }
                        warn!("BinanceMargin isolated heartbeat timeout ({}s), will attempt reconnect", HEARTBEAT_TIMEOUT_SECS);
                        break DisconnectReason::HeartbeatTimeout;
                    }
                    // Last: the arms above end the monitor and win when ready together.
                    () = &mut retry_gaps => {}
                }
            }
        };
        let should_reconnect = !matches!(reason, DisconnectReason::ConsumerDropped);

        if should_reconnect {
            disconnect_time = Some(match reason {
                DisconnectReason::HeartbeatTimeout => {
                    Utc::now()
                        - chrono::Duration::seconds(HEARTBEAT_TIMEOUT_SECS as i64)
                        - chrono::Duration::milliseconds(SIGNAL_RECOVERY_LOOKBACK_MS)
                }
                DisconnectReason::TokenRefresh => {
                    // A rotation fully disconnects + reconnects (re-acquire N tokens + reconnect +
                    // re-subscribe N). Look back far enough to bound that whole window so a fill
                    // landing during the sub-second gap isn't missed; dedup absorbs the overlap.
                    Utc::now()
                        - chrono::Duration::seconds(CONNECT_TIMEOUT_SECS as i64)
                        - chrono::Duration::milliseconds(SIGNAL_RECOVERY_LOOKBACK_MS)
                }
                _ => Utc::now() - chrono::Duration::milliseconds(SIGNAL_RECOVERY_LOOKBACK_MS),
            });
        }

        // --- Cleanup ---
        subscription.unsubscribe();
        if let Err(e) = ws.disconnect().await {
            warn!(%e, "BinanceMargin isolated failed to disconnect WebSocket");
        }

        if !should_reconnect || tx.is_closed() {
            // Consumer dropped the receiver — no StreamTerminated emit (channel already closed).
            debug!("BinanceMargin isolated connection manager exiting");
            break;
        }

        // A planned token refresh is not a failure — reconnect immediately (no backoff); `current`
        // is already None, forcing fresh tokens + a fresh connection next iteration.
        if !matches!(reason, DisconnectReason::TokenRefresh) && !backoff.wait().await {
            error!("BinanceMargin isolated max reconnect attempts exhausted, stream terminating");
            // reason here is Signal or HeartbeatTimeout (TokenRefresh guarded above,
            // ConsumerDropped already broke out earlier).
            let last_error = match reason {
                DisconnectReason::HeartbeatTimeout => {
                    format!("heartbeat timeout ({HEARTBEAT_TIMEOUT_SECS}s)")
                }
                _ => "WebSocket disconnected (server close/error)".to_string(),
            };
            emit_stream_terminated(
                &tx,
                ExchangeId::BinanceMargin,
                StreamTerminationReason::ReconnectBudgetExhausted {
                    attempts: backoff.attempts(),
                    last_error,
                },
            );
            break;
        }
    }
}

// ---------------------------------------------------------------------------
// Account query helpers (margin-specific)
// ---------------------------------------------------------------------------

/// Fetch open orders for a single instrument via `query_margin_accounts_open_orders`
/// (`isIsolated` config-driven). Mirrors `BinanceSpot::fetch_open_orders_for_instrument`.
async fn fetch_margin_open_orders_for_instrument(
    rest: Arc<RestApi>,
    rate_limiter: Arc<RateLimitTracker>,
    instrument: InstrumentNameExchange,
    is_isolated: bool,
) -> Result<(InstrumentNameExchange, OpenOrderListing), UnindexedClientError> {
    // Convert once before the retry closure to avoid a String allocation on every retry.
    let symbol_str = instrument.name().to_string();
    let isolated = QueryMarginAccountsOpenOrdersIsIsolatedEnum::from_flag(is_isolated);
    let response = rest_call_with_retry(&rest, &rate_limiter, RequestKind::Query, |rest| {
        let sym = symbol_str.clone();
        let isolated = isolated.clone();
        Box::pin(async move {
            let params = QueryMarginAccountsOpenOrdersParams::builder()
                .symbol(sym)
                .is_isolated(isolated)
                .build()?;
            rest.query_margin_accounts_open_orders(params).await
        })
    })
    .await
    .map_err(|e| classify_rest_query_error(&e, Some(&instrument)))?;

    let orders_data = response.data().await.map_err(response_decode_error)?;

    let listing = convert_open_order_listing(&orders_data, ExchangeId::BinanceMargin, &instrument);

    Ok((instrument, listing))
}

/// Fetch *all* open margin orders in a single no-symbol `query_margin_accounts_open_orders`
/// call. Backs the [`fetch_open_orders`](BinanceMargin::fetch_open_orders) "return all" sentinel for
/// **cross** (the no-symbol affordance is cross-only): with no symbol the venue returns orders
/// across every instrument, so each order's instrument is recovered from its own `symbol` field
/// (orders missing it are dropped). `isIsolated` is config-driven, but under isolated the caller
/// iterates the configured symbol set per-symbol rather than using this no-symbol path.
async fn fetch_margin_all_open_orders(
    rest: Arc<RestApi>,
    rate_limiter: Arc<RateLimitTracker>,
    is_isolated: bool,
) -> Result<Vec<Order<ExchangeId, InstrumentNameExchange, Open>>, UnindexedClientError> {
    let isolated = QueryMarginAccountsOpenOrdersIsIsolatedEnum::from_flag(is_isolated);
    let response = rest_call_with_retry(&rest, &rate_limiter, RequestKind::Query, |rest| {
        let isolated = isolated.clone();
        Box::pin(async move {
            let params = QueryMarginAccountsOpenOrdersParams::builder()
                .is_isolated(isolated)
                .build()?;
            rest.query_margin_accounts_open_orders(params).await
        })
    })
    .await
    .map_err(|e| classify_rest_query_error(&e, None))?;

    let orders_data = response.data().await.map_err(response_decode_error)?;

    let orders = orders_data
        .into_iter()
        .filter_map(|o| convert_open_order_owned_symbol(&o, ExchangeId::BinanceMargin))
        .collect();

    Ok(orders)
}

/// Length of each time window [`paginate_margin_my_trades`] walks before it finds a trade.
///
/// Binance requires "less than 24 hours between startTime and endTime" on margin `myTrades`,
/// and returns only 24 hours of trades to a query without `fromId`. Kept well under that
/// limit so no reading of the bound (inclusive ends, clock rounding) can make a window too long.
const MARGIN_MY_TRADES_WINDOW_MS: i64 = 23 * 60 * 60 * 1000;

/// One margin `myTrades` request in a [`paginate_margin_my_trades`] walk. The walk's state is the
/// next request to send.
#[derive(Debug, Clone, Copy, PartialEq, Eq)]
enum MarginTradesQuery {
    /// Trades stamped from `start` to `end`, in epoch milliseconds.
    Window { start: i64, end: i64 },
    /// Trades from this id onwards, whatever their time.
    FromId(i64),
    /// One order's executions, from its first.
    Order(i64),
    /// One order's executions, from this id onwards.
    OrderFromId { order_id: i64, from_id: i64 },
}

impl MarginTradesQuery {
    /// The first request of a walk from `from`, or `None` when a walk by time starts after
    /// `now_ms` and so has nothing to read. `now_ms` is where a walk by time stops stepping by
    /// window: the call's start, or a span's end if earlier.
    fn first(from: MyTradesFrom, now_ms: i64) -> Option<Self> {
        match from {
            MyTradesFrom::Time(start) | MyTradesFrom::Span { start, .. } => {
                (start <= now_ms).then(|| Self::window(start, now_ms))
            }
            MyTradesFrom::Order(order_id) => Some(Self::Order(order_id)),
        }
    }

    /// The window from `start`, ending [`MARGIN_MY_TRADES_WINDOW_MS`] later or at `now_ms`.
    fn window(start: i64, now_ms: i64) -> Self {
        Self::Window {
            start,
            end: start.saturating_add(MARGIN_MY_TRADES_WINDOW_MS).min(now_ms),
        }
    }

    /// The request after this one returned `page_len` trades, the last with id `last_id`, or
    /// `None` when the walk is complete.
    fn next(self, page_len: usize, last_id: Option<i64>, now_ms: i64) -> Option<Self> {
        let full = page_len >= BINANCE_MAX_TRADES;
        let after_last = || last_id?.checked_add(1);
        match self {
            // An empty window steps on until it reaches `now_ms`. The next window starts at this
            // one's end, so a trade stamped exactly on the boundary is read whether Binance treats
            // `endTime` as inclusive or not. That cannot duplicate a trade: windows only follow
            // one another while every window so far was empty.
            Self::Window { end, .. } if page_len == 0 => {
                (end < now_ms).then(|| Self::window(end, now_ms))
            }
            // The final window read short holds everything up to `now_ms`: done.
            Self::Window { end, .. } if end >= now_ms && !full => None,
            // Any other window holding a trade: later trades may lie beyond its end, so read on
            // by id, which Binance serves whatever the time.
            Self::Window { .. } => after_last().map(Self::FromId),
            Self::FromId(_) if full => after_last().map(Self::FromId),
            Self::Order(order_id) | Self::OrderFromId { order_id, .. } if full => {
                after_last().map(|from_id| Self::OrderFromId { order_id, from_id })
            }
            Self::FromId(_) | Self::Order(_) | Self::OrderFromId { .. } => None,
        }
    }
}

/// Paginate the margin trade list for a single instrument from `from` (`isIsolated`
/// config-driven), returning every trade from `from` to the call's start with no gap.
///
/// - [`MyTradesFrom::Order`]: first page by `order_id`, then `from_id = last_id + 1` alongside it,
///   as `BinanceSpot::paginate_my_trades` does.
/// - [`MyTradesFrom::Span`]: as `Time`, stepping by window only up to the span's end, and
///   stopping at the first page by id that reaches past it, without the executions after it.
/// - [`MyTradesFrom::Time`]: unlike spot, margin `myTrades` does not return every trade since a
///   bare `startTime` — without `fromId` it returns only 24 hours of trades, and it rejects a
///   `startTime`..`endTime` span of 24 hours or more. So the walk first steps forward in windows
///   of [`MARGIN_MY_TRADES_WINDOW_MS`], each bounded by `startTime` and `endTime`, from `from` to
///   the call's start by the local clock. At the first window holding a trade it switches to
///   `from_id = last_id + 1` and reads on until a short page, unless that window was the last
///   and read a short page.
///
/// **Cost:** each window before the first trade is one request (weight 10), so the cost grows
/// with the lookback: about one request per day, and about 21,000 from the Unix epoch. An
/// ordinary reconnect gap fits in one window.
///
/// **Isolated correctness:** this also backs reconnect fill-recovery (`recover_margin_fills`), so a
/// missed `isIsolated` flip would silently query *cross* trades on an isolated client — hence the
/// value is threaded from config rather than hardcoded.
async fn paginate_margin_my_trades(
    rest: &Arc<RestApi>,
    rate_limiter: &Arc<RateLimitTracker>,
    instrument: &InstrumentNameExchange,
    from: MyTradesFrom,
    is_isolated: bool,
    kind: RequestKind,
) -> Result<Vec<QueryMarginAccountsTradeListResponseInner>, UnindexedClientError> {
    // Convert once before the retry closure to avoid a String allocation on every retry.
    let symbol_str = instrument.name().to_string();
    let isolated = QueryMarginAccountsTradeListIsIsolatedEnum::from_flag(is_isolated);
    // The const_assert! in shared bounds BINANCE_MAX_TRADES <= i32::MAX, which trivially fits in
    // i64, so this cast is always lossless. clippy::cast_possible_wrap fires only because usize is
    // the same width as i64 on 64-bit targets (a hypothetical >64-bit usize could wrap); the bound
    // rules that out.
    #[allow(clippy::cast_possible_wrap)]
    let limit = BINANCE_MAX_TRADES as i64;
    // A walk by time reads up to here. A trade after it is left to the caller's next read (or, on
    // reconnect, to the stream), unless the walk is already paging by id.
    let now_ms = match from {
        MyTradesFrom::Span { end, .. } => end.min(Utc::now().timestamp_millis()),
        MyTradesFrom::Time(_) | MyTradesFrom::Order(_) => Utc::now().timestamp_millis(),
    };
    let mut all_pages = Vec::new();
    let mut requests = 0u32;
    let mut next = MarginTradesQuery::first(from, now_ms);
    while let Some(query) = next {
        requests += 1;
        let response = rest_call_with_retry(rest, rate_limiter, kind, |rest| {
            let sym = symbol_str.clone();
            let isolated = isolated.clone();
            Box::pin(async move {
                let builder = QueryMarginAccountsTradeListParams::builder(sym)
                    .is_isolated(isolated)
                    .limit(limit);
                let params = match query {
                    MarginTradesQuery::Window { start, end } => {
                        builder.start_time(start).end_time(end)
                    }
                    MarginTradesQuery::FromId(id) => builder.from_id(id),
                    MarginTradesQuery::Order(order_id) => builder.order_id(order_id),
                    MarginTradesQuery::OrderFromId { order_id, from_id } => {
                        builder.order_id(order_id).from_id(from_id)
                    }
                }
                .build()?;
                rest.query_margin_accounts_trade_list(params).await
            })
        })
        .await
        .map_err(|e| classify_rest_query_error(&e, Some(instrument)))?;

        let mut page = response.data().await.map_err(response_decode_error)?;

        let page_len = page.len();
        let last_id = page.last().and_then(|t| t.id);
        // A span ends once a page reaches past it; paging by id would otherwise read on.
        let past_end = match from {
            MyTradesFrom::Span { end, .. } => drop_after(&mut page, end),
            MyTradesFrom::Time(_) | MyTradesFrom::Order(_) => false,
        };
        all_pages.extend(page);
        if past_end {
            break;
        }

        if page_len > 0 && last_id.is_none() {
            warn!(%instrument, "BinanceMargin paginate_my_trades: trade missing ID, cannot page past it");
            break;
        }
        next = query.next(page_len, last_id, now_ms);
    }
    debug!(
        %instrument,
        requests,
        trades = all_pages.len(),
        "BinanceMargin paginate_my_trades: walk complete"
    );
    Ok(all_pages)
}

// ---------------------------------------------------------------------------
// Account conversion helpers (margin-specific)
// ---------------------------------------------------------------------------

/// Convert one cross-margin `userAsset` into a margin [`AssetBalance`], carrying the per-asset
/// `borrowed`/`interest` debt via [`Balance::new_margin`]. `total = free + locked`.
///
/// Distinct from spot's `convert_balance_entry`: spot has no debt fields and calls
/// [`Balance::new`]. Returns `None` (with a warning) if a required field is missing/unparseable.
fn convert_margin_balance_entry(
    b: QueryCrossMarginAccountDetailsResponseUserAssetsInner,
    now: DateTime<Utc>,
) -> Option<AssetBalance<AssetNameExchange>> {
    let asset_name = AssetNameExchange::new(b.asset.as_deref()?);
    let free = match b.free.as_deref().and_then(|s| Decimal::from_str(s).ok()) {
        Some(v) => v,
        None => {
            warn!(%asset_name, "BinanceMargin balance missing/unparseable 'free' field");
            return None;
        }
    };
    let locked = match b.locked.as_deref().and_then(|s| Decimal::from_str(s).ok()) {
        Some(v) => v,
        None => {
            warn!(%asset_name, "BinanceMargin balance missing/unparseable 'locked' field");
            return None;
        }
    };
    // Debt fields default to zero when missing/unparseable: a userAsset row always represents a
    // real margin position, so absent debt means "no debt", not corrupt data — emit a margin
    // Balance (carrying zero debt) rather than dropping the asset. This preserves the
    // Design-decision-#4 invariant that a REST snapshot always populates `margin`.
    let borrowed = b
        .borrowed
        .as_deref()
        .and_then(|s| Decimal::from_str(s).ok())
        .unwrap_or(Decimal::ZERO);
    let interest = b
        .interest
        .as_deref()
        .and_then(|s| Decimal::from_str(s).ok())
        .unwrap_or(Decimal::ZERO);
    Some(AssetBalance::new(
        asset_name,
        Balance::new_margin(free + locked, free, borrowed, interest),
        now,
    ))
}

/// Filter cross-margin `userAssets` to the requested assets and convert to [`AssetBalance`]s.
/// An empty `assets` slice returns all. Mirrors spot's `filter_and_convert_balances`.
fn filter_and_convert_margin_balances(
    user_assets: Vec<QueryCrossMarginAccountDetailsResponseUserAssetsInner>,
    assets: &[AssetNameExchange],
) -> Vec<AssetBalance<AssetNameExchange>> {
    let now = Utc::now();
    // Empty assets slice means "return all" — skip building the set entirely.
    if assets.is_empty() {
        return user_assets
            .into_iter()
            .filter_map(|b| convert_margin_balance_entry(b, now))
            .collect();
    }
    // For small slices (≤16 assets), linear scan avoids allocation and hashing overhead.
    if assets.len() <= 16 {
        return user_assets
            .into_iter()
            .filter_map(|b| {
                let asset_name_str = b.asset.as_deref()?;
                if !assets.iter().any(|a| a.name().as_str() == asset_name_str) {
                    return None;
                }
                convert_margin_balance_entry(b, now)
            })
            .collect();
    }
    use std::collections::HashSet;
    let asset_set: HashSet<&str> = assets.iter().map(|a| a.name().as_str()).collect();
    user_assets
        .into_iter()
        .filter_map(|b| {
            let asset_name_str = b.asset.as_deref()?;
            if !asset_set.contains(asset_name_str) {
                return None;
            }
            convert_margin_balance_entry(b, now)
        })
        .collect()
}

/// Wrap a fetched `Open` order as an `OrderState::active` snapshot for `account_snapshot`.
///
/// `account_snapshot` reports open orders wrapped in `OrderState::active(..)`, whereas
/// `fetch_open_orders` returns the bare `Open` — both share
/// [`fetch_margin_open_orders_for_instrument`], so this is the single wrap used by the cross and
/// isolated snapshot paths.
fn active_order_snapshot(
    o: Order<ExchangeId, InstrumentNameExchange, Open>,
) -> Order<ExchangeId, InstrumentNameExchange, OrderState<AssetNameExchange, InstrumentNameExchange>>
{
    Order {
        key: o.key,
        side: o.side,
        price: o.price,
        quantity: o.quantity,
        kind: o.kind,
        time_in_force: o.time_in_force,
        state: OrderState::active(o.state),
    }
}

/// Convert one side (base or quote) of an isolated `assets[]` entry into a margin [`AssetBalance`].
///
/// Mirrors [`convert_margin_balance_entry`]'s field handling: `free`/`locked` are required (missing
/// → `None`, drop), while `borrowed`/`interest` default to zero when absent — a real isolated
/// sub-account row with absent debt means "no debt", not corrupt data, upholding the
/// Design-decision-#4 invariant that a REST snapshot always populates `margin`. `total = free + locked`.
///
/// Takes the fields rather than the SDK side type because the base and quote sides are distinct
/// nominal types (`...BaseAsset` / `...QuoteAsset`) with identical fields.
fn convert_isolated_asset_balance(
    asset: Option<&str>,
    free: Option<&str>,
    locked: Option<&str>,
    borrowed: Option<&str>,
    interest: Option<&str>,
    now: DateTime<Utc>,
) -> Option<AssetBalance<AssetNameExchange>> {
    let asset_name = AssetNameExchange::new(asset?);
    let free = match free.and_then(|s| Decimal::from_str(s).ok()) {
        Some(v) => v,
        None => {
            warn!(%asset_name, "BinanceMargin isolated balance missing/unparseable 'free' field");
            return None;
        }
    };
    let locked = match locked.and_then(|s| Decimal::from_str(s).ok()) {
        Some(v) => v,
        None => {
            warn!(%asset_name, "BinanceMargin isolated balance missing/unparseable 'locked' field");
            return None;
        }
    };
    let borrowed = borrowed
        .and_then(|s| Decimal::from_str(s).ok())
        .unwrap_or(Decimal::ZERO);
    let interest = interest
        .and_then(|s| Decimal::from_str(s).ok())
        .unwrap_or(Decimal::ZERO);
    Some(AssetBalance::new(
        asset_name,
        Balance::new_margin(free + locked, free, borrowed, interest),
        now,
    ))
}

/// Map isolated `query_isolated_margin_account_info` `assets[]` entries to per-instrument
/// [`IsolatedInstrumentState`], keyed by instrument (pair symbol).
///
/// An entry must carry a `symbol` plus a `baseAsset` and `quoteAsset` with parseable `free`/`locked`;
/// an entry missing any of these is dropped with a `warn!` (observable, never silently mis-mapped).
/// Per-pair risk metrics (`marginLevel`/`marginRatio`/`liquidatePrice`) are best-effort `Option`s — a
/// missing/unparseable risk field becomes `None` and does NOT drop the entry.
fn convert_isolated_margin_assets(
    assets: Vec<QueryIsolatedMarginAccountInfoResponseAssetsInner>,
) -> HashMap<InstrumentNameExchange, IsolatedInstrumentState<AssetNameExchange>> {
    // Single timestamp for the whole batch: every entry came from the same REST response, so this
    // is the response's freshness (mirrors the cross-margin `convert_margin_balance_entry` `now`).
    let now = Utc::now();
    let mut map = HashMap::with_capacity(assets.len());
    for entry in assets {
        let Some(symbol) = entry.symbol.as_deref() else {
            warn!("BinanceMargin isolated asset entry missing 'symbol' — skipping");
            continue;
        };
        let instrument = InstrumentNameExchange::new(symbol);

        let Some(base_raw) = entry.base_asset.as_deref() else {
            warn!(%instrument, "BinanceMargin isolated entry missing baseAsset — skipping");
            continue;
        };
        let Some(quote_raw) = entry.quote_asset.as_deref() else {
            warn!(%instrument, "BinanceMargin isolated entry missing quoteAsset — skipping");
            continue;
        };

        let Some(base) = convert_isolated_asset_balance(
            base_raw.asset.as_deref(),
            base_raw.free.as_deref(),
            base_raw.locked.as_deref(),
            base_raw.borrowed.as_deref(),
            base_raw.interest.as_deref(),
            now,
        ) else {
            warn!(%instrument, "BinanceMargin isolated baseAsset missing required field — skipping");
            continue;
        };
        let Some(quote) = convert_isolated_asset_balance(
            quote_raw.asset.as_deref(),
            quote_raw.free.as_deref(),
            quote_raw.locked.as_deref(),
            quote_raw.borrowed.as_deref(),
            quote_raw.interest.as_deref(),
            now,
        ) else {
            warn!(%instrument, "BinanceMargin isolated quoteAsset missing required field — skipping");
            continue;
        };

        let risk = IsolatedMarginRisk {
            margin_level: entry
                .margin_level
                .as_deref()
                .and_then(|s| Decimal::from_str(s).ok()),
            margin_ratio: entry
                .margin_ratio
                .as_deref()
                .and_then(|s| Decimal::from_str(s).ok()),
            liquidation_price: entry
                .liquidate_price
                .as_deref()
                .and_then(|s| Decimal::from_str(s).ok()),
        };

        map.insert(instrument, IsolatedInstrumentState { base, quote, risk });
    }
    map
}

/// Group symbols into comma-separated request strings, ≤5 symbols each (the venue's per-request
/// cap on `query_isolated_margin_account_info`'s `symbols`). An empty input yields no chunks.
fn chunk_symbols(symbols: &[InstrumentNameExchange]) -> Vec<String> {
    symbols
        .chunks(5)
        .map(|chunk| {
            chunk
                .iter()
                .map(|s| s.name().as_str())
                .collect::<Vec<_>>()
                .join(",")
        })
        .collect()
}

/// Fetch isolated-margin account info for `symbols`, chunked at the venue's max-5-symbols/request
/// cap and flattened to the `assets[]` entries across all chunks.
///
/// `symbols` must be non-empty (a no-symbol isolated call is invalid). Chunks are fetched
/// concurrently (bounded to 8) through the shared rate-limit/backoff machinery.
async fn fetch_isolated_margin_account_info(
    rest: Arc<RestApi>,
    rate_limiter: Arc<RateLimitTracker>,
    symbols: Vec<InstrumentNameExchange>,
) -> Result<Vec<QueryIsolatedMarginAccountInfoResponseAssetsInner>, UnindexedClientError> {
    use futures::{StreamExt as _, TryStreamExt as _};

    let chunks = chunk_symbols(&symbols);

    // One `assets[]` entry per requested symbol, so pre-size the flat accumulator to `symbols.len()`
    // and `try_fold` the per-chunk results straight into it — avoiding the intermediate
    // `Vec<Vec<_>>` + re-copying `flatten().collect()` (mirrors the `try_fold` in `fetch_open_orders`).
    futures::stream::iter(chunks.into_iter().map(|symbols_param| {
        let rest = rest.clone();
        let rate_limiter = rate_limiter.clone();
        async move {
            let response = rest_call_with_retry(&rest, &rate_limiter, RequestKind::Query, |rest| {
                let symbols_param = symbols_param.clone();
                Box::pin(async move {
                    let params = QueryIsolatedMarginAccountInfoParams::builder()
                        .symbols(symbols_param)
                        .build()?;
                    rest.query_isolated_margin_account_info(params).await
                })
            })
            .await
            .map_err(|e| classify_rest_query_error(&e, None))?;

            let info = response.data().await.map_err(response_decode_error)?;
            Ok::<_, UnindexedClientError>(info.assets.unwrap_or_default())
        }
    }))
    .buffer_unordered(8)
    .try_fold(
        Vec::with_capacity(symbols.len()),
        |mut acc, assets| async move {
            acc.extend(assets);
            Ok(acc)
        },
    )
    .await
}

/// Convert a margin trade-list response into a rustrade [`Trade`].
///
/// Field-for-field identical to spot's `convert_my_trade` but typed to the margin SDK response and
/// stamped with [`ExchangeId::BinanceMargin`]. Fee asset is taken verbatim from `commissionAsset`
/// (may be base, quote, or third-party e.g. BNB); `fees_quote` is left `None` for the indexer to
/// resolve.
fn convert_margin_trade(
    t: &QueryMarginAccountsTradeListResponseInner,
    instrument: &InstrumentNameExchange,
) -> Option<Trade<AssetNameExchange, InstrumentNameExchange>> {
    let trade_id_raw = match t.id {
        Some(id) => id,
        None => {
            warn!(%instrument, "BinanceMargin trade missing id");
            return None;
        }
    };
    let trade_id = TradeId(format_smolstr!("{trade_id_raw}"));
    let order_id = match t.order_id {
        Some(id) => OrderId(format_smolstr!("{id}")),
        None => {
            warn!(%instrument, trade_id = %trade_id_raw, "BinanceMargin trade missing orderId");
            return None;
        }
    };
    let side = match t.is_buyer {
        Some(true) => Side::Buy,
        Some(false) => Side::Sell,
        None => {
            warn!(%instrument, trade_id = %trade_id_raw, "BinanceMargin trade missing isBuyer");
            return None;
        }
    };
    let price = match t.price.as_deref().and_then(|s| Decimal::from_str(s).ok()) {
        Some(v) => v,
        None => {
            warn!(%instrument, trade_id = %trade_id_raw, "BinanceMargin trade missing/unparseable price");
            return None;
        }
    };
    let quantity = match t.qty.as_deref().and_then(|s| Decimal::from_str(s).ok()) {
        Some(v) => v,
        None => {
            warn!(%instrument, trade_id = %trade_id_raw, "BinanceMargin trade missing/unparseable qty");
            return None;
        }
    };
    let commission = t
        .commission
        .as_deref()
        .and_then(|s| Decimal::from_str(s).ok())
        .unwrap_or(Decimal::ZERO);
    let time_exchange = match t.time.and_then(|ms| Utc.timestamp_millis_opt(ms).single()) {
        Some(ts) => ts,
        None => {
            warn!(%instrument, trade_id = %trade_id_raw, "BinanceMargin trade missing/unparseable time, using now");
            Utc::now()
        }
    };

    // Use actual commission asset from Binance (e.g., BNB, USDT, BTC). fees_quote is None here;
    // the indexer computes it when the fee is in quote or base asset. "UNKNOWN" fallback (rare:
    // API omits commissionAsset) will fail indexing rather than silently misattribute the fee.
    let fee_asset = t
        .commission_asset
        .as_deref()
        .map(AssetNameExchange::from)
        .unwrap_or_else(|| AssetNameExchange::from("UNKNOWN"));

    Some(Trade::new(
        trade_id,
        order_id,
        instrument.clone(),
        StrategyId::unknown(), // Binance doesn't carry strategy IDs
        time_exchange,
        side,
        price,
        quantity,
        // `myTrades` reports executions only -- no cumulative, no order status.
        None,
        AssetFees::new(fee_asset, commission, None),
    ))
}

// ---------------------------------------------------------------------------
// Order conversion helpers (margin-specific)
// ---------------------------------------------------------------------------

/// Failure modes when constructing [`MarginAccountNewOrderParams`] from a rustrade request.
#[derive(Debug)]
enum BuildOrderError {
    /// The `OrderKind`/`TimeInForce` combination is not supported on Binance margin.
    Unsupported,
    /// The SDK params builder rejected the inputs (carries the builder's error message).
    Build(String),
}

/// Construct a margin endpoint's `isIsolated` parameter enum from the configured margin mode.
///
/// The SDK generates a *separate* two-variant `isIsolated` enum per endpoint, all with identical
/// `True`/`False` variants serialising to `"TRUE"`/`"FALSE"`. This trait keeps the one
/// `bool` → wire-value mapping defined (and unit-tested) in a single place, as the former
/// `isolated_str` helper did, rather than repeating a ternary at each call site.
///
/// `true` → `True` (`"TRUE"`, isolated, per-pair sub-accounts); `false` → `False` (`"FALSE"`,
/// cross, account-wide). Threaded through every margin order/cancel/query call so the mode is
/// config-driven from a single source rather than hardcoded per-call.
trait IsolatedFlag {
    /// The endpoint's `isIsolated` variant for `is_isolated`.
    fn from_flag(is_isolated: bool) -> Self;
}

/// Implement [`IsolatedFlag`] for each endpoint's `isIsolated` enum.
///
/// Every one of these SDK enums is the same two-variant shape, so a macro keeps the impls honest:
/// adding an endpoint is one line here rather than a copy-pasted block that can silently invert.
macro_rules! impl_isolated_flag {
    ($($t:ty),* $(,)?) => {
        $(
            impl IsolatedFlag for $t {
                fn from_flag(is_isolated: bool) -> Self {
                    if is_isolated { Self::True } else { Self::False }
                }
            }
        )*
    };
}

impl_isolated_flag!(
    MarginAccountNewOrderIsIsolatedEnum,
    MarginAccountCancelOrderIsIsolatedEnum,
    QueryMarginAccountsOpenOrdersIsIsolatedEnum,
    QueryMarginAccountsTradeListIsIsolatedEnum,
);

/// Build the margin cancel-order params (pure; no I/O).
///
/// Cancels by exchange `orderId` when present and parseable as `i64`, otherwise falls back to the
/// originating client order id (`origClientOrderId`). `isIsolated` is config-driven. Factored out of
/// [`BinanceMargin::cancel_order`] so the id-vs-cid fallback branch and the `isIsolated` value are
/// unit-testable without a live REST call. Returns the builder's error message (stringified) on the
/// rare build failure (e.g. a malformed symbol).
fn build_cancel_order_params(
    symbol: String,
    id: Option<&OrderId>,
    cid: &ClientOrderId,
    is_isolated: bool,
) -> Result<MarginAccountCancelOrderParams, String> {
    let mut builder = MarginAccountCancelOrderParams::builder(symbol).is_isolated(
        MarginAccountCancelOrderIsIsolatedEnum::from_flag(is_isolated),
    );

    match id {
        Some(order_id) => match order_id.0.parse::<i64>() {
            Ok(id) => builder = builder.order_id(id),
            Err(_) => {
                // exchange order id exists but isn't a valid i64 — fall back to the cid.
                error!(
                    order_id = %order_id.0,
                    "BinanceMargin cancel: exchange orderId not parseable as i64, falling back to clientOrderId"
                );
                builder = builder.orig_client_order_id(cid.0.to_string());
            }
        },
        None => builder = builder.orig_client_order_id(cid.0.to_string()),
    }

    builder.build().map_err(|e| e.to_string())
}

/// Build the margin new-order params from a rustrade order request (pure; no I/O).
///
/// Factored out of [`BinanceMargin::open_order`] so the rustrade→Binance mapping (sideEffectType,
/// `isIsolated`, conditional `stopPrice`, `autoRepayAtCancel` gating, trailing rejection) is
/// unit-testable without a live REST call. `isIsolated` is config-driven via `is_isolated`
/// (`true` = isolated, `false` = cross).
#[allow(clippy::too_many_arguments)] // mirrors the flat request fields; grouping them into a
// struct would just shuffle the same data and obscure the 1:1 mapping to SDK params.
fn build_new_order_params(
    symbol: String,
    side: Side,
    price: Option<Decimal>,
    quantity: Decimal,
    kind: OrderKind,
    time_in_force: TimeInForce,
    new_client_order_id: String,
    side_effect: MarginSideEffect,
    is_isolated: bool,
) -> Result<MarginAccountNewOrderParams, BuildOrderError> {
    let binance_side = match side {
        Side::Buy => MarginAccountNewOrderSideEnum::Buy,
        Side::Sell => MarginAccountNewOrderSideEnum::Sell,
    };

    let (binance_type, binance_tif) =
        convert_order_kind_tif_margin(kind, time_in_force).ok_or(BuildOrderError::Unsupported)?;

    // isIsolated is config-driven: "TRUE" for isolated, "FALSE" for cross.
    let mut builder = MarginAccountNewOrderParams::builder(symbol, binance_side, binance_type)
        .quantity(quantity)
        .is_isolated(MarginAccountNewOrderIsIsolatedEnum::from_flag(is_isolated))
        .side_effect_type(side_effect.as_sdk_enum())
        .new_client_order_id(new_client_order_id)
        .new_order_resp_type(MarginAccountNewOrderNewOrderRespTypeEnum::Full);

    // auto_repay_at_cancel only coheres under AutoBorrowRepay: a NoBorrow client never borrows,
    // so there is no loan to repay when an order is cancelled.
    if side_effect == MarginSideEffect::AutoBorrowRepay {
        builder = builder.auto_repay_at_cancel(true);
    }

    if let Some(tif) = binance_tif {
        builder = builder.time_in_force(tif);
    }

    // Conditional price fields (mirror spot): LIMIT carries price; STOP/TAKE_PROFIT carry a stop
    // (trigger) price; the *_LIMIT variants carry both.
    match kind {
        OrderKind::Limit => {
            builder = builder.price(price);
        }
        OrderKind::Stop { trigger_price } | OrderKind::TakeProfit { trigger_price } => {
            builder = builder.stop_price(trigger_price);
        }
        OrderKind::StopLimit { trigger_price } | OrderKind::TakeProfitLimit { trigger_price } => {
            builder = builder.price(price).stop_price(trigger_price);
        }
        // Market carries no price/stop_price; trailing-stop kinds are rejected earlier by
        // convert_order_kind_tif_margin. A future OrderKind needing price fields that lands here
        // would surface as a BuildOrderError::Build from the SDK builder, not a silent bad order.
        _ => {}
    }

    builder
        .build()
        .map_err(|e| BuildOrderError::Build(e.to_string()))
}

/// Map a rustrade [`OrderKind`] + [`TimeInForce`] to the margin SDK's order `type`/`timeInForce`.
///
/// Reuses the shared [`classify_order_kind_tif`] decision logic, mapping only the venue-neutral
/// result onto margin's SDK output types — a [`MarginAccountNewOrderTypeEnum`] and a
/// [`MarginAccountNewOrderTimeInForceEnum`]. Mirrors spot's own `convert_order_kind_tif` adapter.
///
/// Returns `None` for unsupported combinations. Trailing-stop kinds are rejected here (the margin
/// SDK has no `trailingDelta` binding), unlike spot which maps them to a `STOP_LOSS` with
/// `trailingDelta`.
fn convert_order_kind_tif_margin(
    kind: OrderKind,
    tif: TimeInForce,
) -> Option<(
    MarginAccountNewOrderTypeEnum,
    Option<MarginAccountNewOrderTimeInForceEnum>,
)> {
    if matches!(
        kind,
        OrderKind::TrailingStop { .. } | OrderKind::TrailingStopLimit { .. }
    ) {
        warn!(
            ?kind,
            "BinanceMargin does not support trailing-stop orders (SDK trailingDelta binding gap)"
        );
        return None;
    }

    let (binance_type, binance_tif) = classify_order_kind_tif(kind, tif)?;
    let margin_type = match binance_type {
        BinanceOrderType::Market => MarginAccountNewOrderTypeEnum::Market,
        BinanceOrderType::Limit => MarginAccountNewOrderTypeEnum::Limit,
        BinanceOrderType::LimitMaker => MarginAccountNewOrderTypeEnum::LimitMaker,
        BinanceOrderType::StopLoss => MarginAccountNewOrderTypeEnum::StopLoss,
        BinanceOrderType::StopLossLimit => MarginAccountNewOrderTypeEnum::StopLossLimit,
        BinanceOrderType::TakeProfit => MarginAccountNewOrderTypeEnum::TakeProfit,
        BinanceOrderType::TakeProfitLimit => MarginAccountNewOrderTypeEnum::TakeProfitLimit,
    };
    let margin_tif = binance_tif.map(|t| match t {
        BinanceTimeInForce::Gtc => MarginAccountNewOrderTimeInForceEnum::Gtc,
        BinanceTimeInForce::Ioc => MarginAccountNewOrderTimeInForceEnum::Ioc,
        BinanceTimeInForce::Fok => MarginAccountNewOrderTimeInForceEnum::Fok,
    });
    Some((margin_type, margin_tif))
}

/// Volume-weighted average fill price from a margin order response's cumulative quote quantity.
///
/// `avg_price = cummulative_quote_qty / executed_qty`. Returns `None` when `filled_qty` is zero
/// (no fills, or division would be undefined) or the quote quantity is missing/unparseable.
// `cummulative_quote_qty` keeps Binance's own field-name typo (sic, double-m) to mirror the SDK.
fn margin_avg_price(cummulative_quote_qty: Option<&str>, filled_qty: Decimal) -> Option<Decimal> {
    if filled_qty.is_zero() {
        return None;
    }
    let s = cummulative_quote_qty?;
    match Decimal::from_str(s) {
        Ok(cumulative) => cumulative.checked_div(filled_qty),
        Err(_) => {
            // A filled order with an unparseable cumulative quote qty is corrupt data, not an
            // expected absence — log it rather than silently returning a price-less Filled.
            warn!(
                cummulative_quote_qty = s,
                "BinanceMargin: failed to parse cummulativeQuoteQty; avg price unavailable"
            );
            None
        }
    }
}

#[cfg(test)]
#[allow(clippy::unwrap_used, clippy::expect_used)] // Test code: panics on bad input are acceptable
mod tests {
    use super::*;
    use crate::client::binance::shared::convert_open_order;
    use crate::order::state::ActiveOrderState;
    use binance_sdk::margin_trading::rest_api::QueryMarginAccountsOpenOrdersResponseInner;

    #[test]
    fn margin_side_effect_default_is_auto_borrow_repay() {
        assert_eq!(
            MarginSideEffect::default(),
            MarginSideEffect::AutoBorrowRepay
        );
    }

    #[test]
    fn margin_side_effect_wire_strings() {
        assert_eq!(
            MarginSideEffect::AutoBorrowRepay.as_binance_str(),
            "AUTO_BORROW_REPAY"
        );
        assert_eq!(
            MarginSideEffect::NoBorrow.as_binance_str(),
            "NO_SIDE_EFFECT"
        );
    }

    #[test]
    fn margin_side_effect_serde_round_trip() {
        // serde/config-file form is snake_case, distinct from the Binance wire string (as_binance_str).
        assert_eq!(
            serde_json::to_string(&MarginSideEffect::AutoBorrowRepay).unwrap(),
            r#""auto_borrow_repay""#
        );
        assert_eq!(
            serde_json::to_string(&MarginSideEffect::NoBorrow).unwrap(),
            r#""no_borrow""#
        );
        assert_eq!(
            serde_json::from_str::<MarginSideEffect>(r#""auto_borrow_repay""#).unwrap(),
            MarginSideEffect::AutoBorrowRepay
        );
        assert_eq!(
            serde_json::from_str::<MarginSideEffect>(r#""no_borrow""#).unwrap(),
            MarginSideEffect::NoBorrow
        );
    }

    #[test]
    fn config_debug_redacts_secrets() {
        let config = BinanceMarginConfig::new(
            "my_api_key".to_string(),
            "my_secret_key".to_string(),
            false,
            false,
            Vec::new(),
            MarginSideEffect::default(),
        );
        let debug = format!("{config:?}");
        assert!(!debug.contains("my_api_key"));
        assert!(!debug.contains("my_secret_key"));
        assert!(debug.contains("***"));
    }

    #[test]
    fn cross_margin_uses_common_case_defaults() {
        let config = BinanceMarginConfig::cross_margin("k".to_string(), "s".to_string());
        assert!(!config.testnet);
        assert!(!config.is_isolated);
        assert_eq!(config.side_effect, MarginSideEffect::AutoBorrowRepay);
        assert_eq!(config.api_key(), "k");
    }

    #[test]
    fn config_deserializes_with_defaults() {
        // is_isolated and side_effect default when omitted; testnet has no default and must be present.
        let config: BinanceMarginConfig =
            serde_json::from_str(r#"{"api_key":"k","secret_key":"s","testnet":false}"#)
                .expect("deserialize");
        assert!(!config.is_isolated);
        assert_eq!(config.side_effect, MarginSideEffect::AutoBorrowRepay);
    }

    #[test]
    fn config_deserializes_explicit_side_effect() {
        let config: BinanceMarginConfig = serde_json::from_str(
            r#"{"api_key":"k","secret_key":"s","testnet":false,"is_isolated":true,"side_effect":"no_borrow"}"#,
        )
        .expect("deserialize");
        assert!(config.is_isolated);
        assert_eq!(config.side_effect, MarginSideEffect::NoBorrow);
    }

    #[test]
    fn isolated_ctor_sets_symbols_and_flag() {
        let symbols = vec![
            InstrumentNameExchange::new("BTCUSDT"),
            InstrumentNameExchange::new("ETHUSDT"),
        ];
        let config =
            BinanceMarginConfig::isolated("k".to_string(), "s".to_string(), symbols.clone());
        assert!(config.is_isolated);
        assert_eq!(config.isolated_symbols, symbols);
        assert_eq!(config.side_effect, MarginSideEffect::AutoBorrowRepay);
    }

    #[test]
    fn isolated_config_with_symbols_constructs() {
        // The construction gate only rejects isolated + *empty* symbols; a populated set is fine.
        let config = BinanceMarginConfig::isolated(
            "k".to_string(),
            "s".to_string(),
            vec![InstrumentNameExchange::new("BTCUSDT")],
        );
        let _client = BinanceMargin::new(config); // must not panic
    }

    #[test]
    #[should_panic(expected = "non-empty isolated_symbols")]
    fn isolated_empty_symbols_panics_at_new() {
        // is_isolated = true with no isolated_symbols is an unusable config → fail-fast panic.
        let config = BinanceMarginConfig::new(
            "k".to_string(),
            "s".to_string(),
            false,
            true,
            Vec::new(),
            MarginSideEffect::default(),
        );
        let _ = BinanceMargin::new(config);
    }

    #[test]
    #[should_panic(expected = "non-empty isolated_symbols")]
    fn isolated_empty_symbols_panics_via_deserialize_path() {
        // The gate must also cover the Deserialize-only path (a config file with is_isolated = true
        // and no isolated_symbols bypasses the named constructor).
        let config: BinanceMarginConfig = serde_json::from_str(
            r#"{"api_key":"k","secret_key":"s","testnet":false,"is_isolated":true}"#,
        )
        .expect("deserialize");
        assert!(config.isolated_symbols.is_empty());
        let _ = BinanceMargin::new(config);
    }

    #[test]
    fn isolated_flag_maps_mode() {
        // One assertion per endpoint enum: the SDK generates a separate `isIsolated` type for
        // each, so an inverted arm in any single impl would otherwise go unnoticed.
        assert_eq!(
            MarginAccountNewOrderIsIsolatedEnum::from_flag(true).as_str(),
            "TRUE"
        );
        assert_eq!(
            MarginAccountNewOrderIsIsolatedEnum::from_flag(false).as_str(),
            "FALSE"
        );
        assert_eq!(
            MarginAccountCancelOrderIsIsolatedEnum::from_flag(true).as_str(),
            "TRUE"
        );
        assert_eq!(
            MarginAccountCancelOrderIsIsolatedEnum::from_flag(false).as_str(),
            "FALSE"
        );
        assert_eq!(
            QueryMarginAccountsOpenOrdersIsIsolatedEnum::from_flag(true).as_str(),
            "TRUE"
        );
        assert_eq!(
            QueryMarginAccountsOpenOrdersIsIsolatedEnum::from_flag(false).as_str(),
            "FALSE"
        );
        assert_eq!(
            QueryMarginAccountsTradeListIsIsolatedEnum::from_flag(true).as_str(),
            "TRUE"
        );
        assert_eq!(
            QueryMarginAccountsTradeListIsIsolatedEnum::from_flag(false).as_str(),
            "FALSE"
        );
    }

    // -----------------------------------------------------------------------
    // Cancel param mapping (orderId-vs-cid fallback + config-driven isIsolated)
    // -----------------------------------------------------------------------

    #[test]
    fn cancel_params_cross_with_parseable_order_id() {
        let p = build_cancel_order_params(
            "BTCUSDT".to_string(),
            Some(&OrderId::new("12345")),
            &ClientOrderId::new("cid-1"),
            false,
        )
        .expect("build");
        assert_eq!(p.symbol, "BTCUSDT");
        assert_eq!(p.is_isolated.as_ref().map(|i| i.as_str()), Some("FALSE"));
        assert_eq!(p.order_id, Some(12345));
        assert_eq!(p.orig_client_order_id, None);
    }

    #[test]
    fn cancel_params_isolated_is_config_driven() {
        let p = build_cancel_order_params(
            "BTCUSDT".to_string(),
            Some(&OrderId::new("12345")),
            &ClientOrderId::new("cid-1"),
            true,
        )
        .expect("build");
        assert_eq!(p.is_isolated.as_ref().map(|i| i.as_str()), Some("TRUE"));
        assert_eq!(p.order_id, Some(12345));
    }

    #[test]
    fn cancel_params_non_i64_order_id_falls_back_to_cid() {
        let p = build_cancel_order_params(
            "BTCUSDT".to_string(),
            Some(&OrderId::new("not-an-i64")),
            &ClientOrderId::new("cid-1"),
            false,
        )
        .expect("build");
        assert_eq!(p.order_id, None);
        assert_eq!(p.orig_client_order_id.as_deref(), Some("cid-1"));
    }

    #[test]
    fn cancel_params_absent_order_id_uses_cid() {
        let p = build_cancel_order_params(
            "BTCUSDT".to_string(),
            None,
            &ClientOrderId::new("cid-1"),
            false,
        )
        .expect("build");
        assert_eq!(p.order_id, None);
        assert_eq!(p.orig_client_order_id.as_deref(), Some("cid-1"));
    }

    // -----------------------------------------------------------------------
    // Order param mapping (17.4.x)
    // -----------------------------------------------------------------------

    use crate::order::TrailingOffsetType;

    /// Build params for the common case (cross), overriding only what a test cares about.
    fn params(
        side: Side,
        price: Option<Decimal>,
        kind: OrderKind,
        tif: TimeInForce,
        side_effect: MarginSideEffect,
    ) -> Result<MarginAccountNewOrderParams, BuildOrderError> {
        build_new_order_params(
            "BTCUSDT".to_string(),
            side,
            price,
            Decimal::from(2),
            kind,
            tif,
            "cid-1".to_string(),
            side_effect,
            false, // cross
        )
    }

    fn gtc() -> TimeInForce {
        TimeInForce::GoodUntilCancelled { post_only: false }
    }

    #[test]
    fn new_order_limit_maps_core_fields() {
        let p = params(
            Side::Buy,
            Some(Decimal::from(50_000)),
            OrderKind::Limit,
            gtc(),
            MarginSideEffect::AutoBorrowRepay,
        )
        .expect("build");

        assert_eq!(p.symbol, "BTCUSDT");
        assert_eq!(p.side.as_str(), "BUY");
        assert_eq!(p.r#type.as_str(), "LIMIT");
        assert_eq!(p.quantity, Some(Decimal::from(2)));
        assert_eq!(p.price, Some(Decimal::from(50_000)));
        assert_eq!(p.stop_price, None);
        assert_eq!(p.time_in_force.as_ref().map(|t| t.as_str()), Some("GTC"));
        assert_eq!(p.new_client_order_id.as_deref(), Some("cid-1"));
        // FULL response so immediate fills come back in the order response.
        assert_eq!(
            p.new_order_resp_type.as_ref().map(|r| r.as_str()),
            Some("FULL")
        );
    }

    #[test]
    fn new_order_is_isolated_is_config_driven() {
        // Cross (is_isolated = false) → "FALSE".
        let cross = build_new_order_params(
            "BTCUSDT".to_string(),
            Side::Sell,
            Some(Decimal::from(10)),
            Decimal::from(2),
            OrderKind::Limit,
            gtc(),
            "cid-1".to_string(),
            MarginSideEffect::AutoBorrowRepay,
            false,
        )
        .expect("build");
        assert_eq!(
            cross.is_isolated.as_ref().map(|i| i.as_str()),
            Some("FALSE")
        );
        assert_eq!(cross.side.as_str(), "SELL");

        // Isolated (is_isolated = true) → "TRUE".
        let isolated = build_new_order_params(
            "BTCUSDT".to_string(),
            Side::Sell,
            Some(Decimal::from(10)),
            Decimal::from(2),
            OrderKind::Limit,
            gtc(),
            "cid-1".to_string(),
            MarginSideEffect::AutoBorrowRepay,
            true,
        )
        .expect("build");
        assert_eq!(
            isolated.is_isolated.as_ref().map(|i| i.as_str()),
            Some("TRUE")
        );
    }

    #[test]
    fn side_effect_and_auto_repay_gating() {
        // AutoBorrowRepay → AUTO_BORROW_REPAY + auto_repay_at_cancel(true).
        let auto = params(
            Side::Buy,
            Some(Decimal::from(1)),
            OrderKind::Limit,
            gtc(),
            MarginSideEffect::AutoBorrowRepay,
        )
        .expect("build");
        assert_eq!(
            auto.side_effect_type.as_ref().map(|s| s.as_str()),
            Some("AUTO_BORROW_REPAY")
        );
        assert_eq!(auto.auto_repay_at_cancel, Some(true));

        // NoBorrow → NO_SIDE_EFFECT and NO auto_repay_at_cancel (no loan to repay).
        let no_borrow = params(
            Side::Buy,
            Some(Decimal::from(1)),
            OrderKind::Limit,
            gtc(),
            MarginSideEffect::NoBorrow,
        )
        .expect("build");
        assert_eq!(
            no_borrow.side_effect_type.as_ref().map(|s| s.as_str()),
            Some("NO_SIDE_EFFECT")
        );
        assert_eq!(no_borrow.auto_repay_at_cancel, None);
    }

    #[test]
    fn new_order_market_has_no_price_or_tif() {
        let p = params(
            Side::Buy,
            None,
            OrderKind::Market,
            gtc(),
            MarginSideEffect::AutoBorrowRepay,
        )
        .expect("build");
        assert_eq!(p.r#type.as_str(), "MARKET");
        assert_eq!(p.price, None);
        assert_eq!(p.stop_price, None);
        assert!(p.time_in_force.is_none());
    }

    #[test]
    fn post_only_limit_maps_to_limit_maker() {
        let p = params(
            Side::Buy,
            Some(Decimal::from(10)),
            OrderKind::Limit,
            TimeInForce::GoodUntilCancelled { post_only: true },
            MarginSideEffect::AutoBorrowRepay,
        )
        .expect("build");
        assert_eq!(p.r#type.as_str(), "LIMIT_MAKER");
        // LIMIT_MAKER carries no timeInForce (post-only is the type, not a TIF).
        assert!(p.time_in_force.is_none());
    }

    #[test]
    fn conditional_kinds_set_stop_price() {
        let trigger = Decimal::from(48_000);

        let stop = params(
            Side::Sell,
            None,
            OrderKind::Stop {
                trigger_price: trigger,
            },
            gtc(),
            MarginSideEffect::AutoBorrowRepay,
        )
        .expect("build");
        assert_eq!(stop.r#type.as_str(), "STOP_LOSS");
        assert_eq!(stop.stop_price, Some(trigger));
        assert_eq!(stop.price, None);

        let stop_limit = params(
            Side::Sell,
            Some(Decimal::from(47_900)),
            OrderKind::StopLimit {
                trigger_price: trigger,
            },
            gtc(),
            MarginSideEffect::AutoBorrowRepay,
        )
        .expect("build");
        assert_eq!(stop_limit.r#type.as_str(), "STOP_LOSS_LIMIT");
        assert_eq!(stop_limit.stop_price, Some(trigger));
        assert_eq!(stop_limit.price, Some(Decimal::from(47_900)));
        assert_eq!(
            stop_limit.time_in_force.as_ref().map(|t| t.as_str()),
            Some("GTC")
        );

        let take_profit = params(
            Side::Sell,
            None,
            OrderKind::TakeProfit {
                trigger_price: trigger,
            },
            gtc(),
            MarginSideEffect::AutoBorrowRepay,
        )
        .expect("build");
        assert_eq!(take_profit.r#type.as_str(), "TAKE_PROFIT");
        assert_eq!(take_profit.stop_price, Some(trigger));
    }

    #[test]
    fn trailing_kinds_are_rejected() {
        let trailing_stop = params(
            Side::Sell,
            None,
            OrderKind::TrailingStop {
                offset: Decimal::from(100),
                offset_type: TrailingOffsetType::BasisPoints,
            },
            gtc(),
            MarginSideEffect::AutoBorrowRepay,
        );
        assert!(matches!(trailing_stop, Err(BuildOrderError::Unsupported)));

        let trailing_stop_limit = params(
            Side::Sell,
            Some(Decimal::from(100)),
            OrderKind::TrailingStopLimit {
                offset: Decimal::from(100),
                offset_type: TrailingOffsetType::BasisPoints,
                limit_offset: Decimal::from(10),
            },
            gtc(),
            MarginSideEffect::AutoBorrowRepay,
        );
        assert!(matches!(
            trailing_stop_limit,
            Err(BuildOrderError::Unsupported)
        ));
    }

    #[test]
    fn tif_variants_map_to_margin_enum() {
        for (tif, expected) in [
            (TimeInForce::ImmediateOrCancel, "IOC"),
            (TimeInForce::FillOrKill, "FOK"),
            (gtc(), "GTC"),
        ] {
            let p = params(
                Side::Buy,
                Some(Decimal::from(1)),
                OrderKind::Limit,
                tif,
                MarginSideEffect::AutoBorrowRepay,
            )
            .expect("build");
            assert_eq!(p.time_in_force.as_ref().map(|t| t.as_str()), Some(expected));
        }
    }

    #[test]
    fn avg_price_from_cumulative_quote_qty() {
        // 100 quote / 4 base = 25.
        assert_eq!(
            margin_avg_price(Some("100"), Decimal::from(4)),
            Some(Decimal::from(25))
        );
        // Zero fill → no average (avoids division by zero).
        assert_eq!(margin_avg_price(Some("100"), Decimal::ZERO), None);
        // Missing / unparseable quote qty → None.
        assert_eq!(margin_avg_price(None, Decimal::from(4)), None);
        assert_eq!(
            margin_avg_price(Some("not-a-number"), Decimal::from(4)),
            None
        );
    }

    // -----------------------------------------------------------------------
    // Account query converters (17.5.x)
    // -----------------------------------------------------------------------

    fn user_asset(
        asset: &str,
        free: &str,
        locked: &str,
        borrowed: &str,
        interest: &str,
    ) -> QueryCrossMarginAccountDetailsResponseUserAssetsInner {
        let mut a = QueryCrossMarginAccountDetailsResponseUserAssetsInner::new();
        a.asset = Some(asset.to_string());
        a.free = Some(free.to_string());
        a.locked = Some(locked.to_string());
        a.borrowed = Some(borrowed.to_string());
        a.interest = Some(interest.to_string());
        a
    }

    #[test]
    fn margin_balance_entry_maps_debt() {
        // free=10, locked=2 → total=12; borrowed=3, interest=0.5 → net_asset = total - borrowed = 9.
        let ab =
            convert_margin_balance_entry(user_asset("USDT", "10", "2", "3", "0.5"), Utc::now())
                .expect("convert");
        assert_eq!(ab.asset.name().as_str(), "USDT");
        assert_eq!(ab.balance.total, Decimal::from(12));
        assert_eq!(ab.balance.free, Decimal::from(10));
        assert!(
            ab.balance.margin.is_some(),
            "REST snapshot must populate margin"
        );
        // net_asset deducts borrowed principal (interest is tracked separately, not deducted here).
        assert_eq!(ab.balance.net_asset(), Decimal::from(9));
    }

    #[test]
    fn margin_balance_short_is_negative_net() {
        // A short borrows more base than it holds: total=1, borrowed=5 → net_asset = -4.
        let ab = convert_margin_balance_entry(user_asset("BTC", "1", "0", "5", "0"), Utc::now())
            .expect("convert");
        assert_eq!(ab.balance.net_asset(), Decimal::from(-4));
    }

    #[test]
    fn margin_balance_missing_debt_defaults_to_zero() {
        // A userAsset with no borrowed/interest is a real no-debt position, not corrupt data:
        // still emit a margin Balance (carrying zero debt) so the REST snapshot always sets margin.
        let mut a = QueryCrossMarginAccountDetailsResponseUserAssetsInner::new();
        a.asset = Some("ETH".to_string());
        a.free = Some("4".to_string());
        a.locked = Some("0".to_string());
        let ab = convert_margin_balance_entry(a, Utc::now()).expect("convert");
        assert!(ab.balance.margin.is_some());
        assert_eq!(ab.balance.net_asset(), Decimal::from(4));
    }

    #[test]
    fn margin_balance_missing_free_is_dropped() {
        // Missing free/locked is corrupt (not no-debt) → drop the asset rather than guess.
        let mut a = QueryCrossMarginAccountDetailsResponseUserAssetsInner::new();
        a.asset = Some("USDT".to_string());
        a.locked = Some("0".to_string());
        assert!(convert_margin_balance_entry(a, Utc::now()).is_none());
    }

    #[test]
    fn margin_balance_filtering() {
        let assets = vec![
            user_asset("USDT", "10", "0", "0", "0"),
            user_asset("BTC", "1", "0", "0", "0"),
            user_asset("ETH", "5", "0", "0", "0"),
        ];
        // Empty filter → all assets.
        assert_eq!(
            filter_and_convert_margin_balances(assets.clone(), &[]).len(),
            3
        );
        // Subset filter → only requested assets.
        let filtered = filter_and_convert_margin_balances(
            assets,
            &[AssetNameExchange::new("BTC"), AssetNameExchange::new("ETH")],
        );
        assert_eq!(filtered.len(), 2);
        assert!(filtered.iter().all(|b| b.asset.name().as_str() != "USDT"));
    }

    #[test]
    fn margin_balance_filtering_large_slice_uses_hashset_branch() {
        // >16 requested assets takes the HashSet path (vs the ≤16 linear scan) — exercise it.
        let assets = vec![
            user_asset("BTC", "1", "0", "0", "0"),
            user_asset("ETH", "5", "0", "0", "0"),
            user_asset("DOGE", "9", "0", "0", "0"), // present in account, not requested → dropped
        ];
        // 17 requested assets (A00..A16) forces the HashSet branch; BTC and ETH also requested.
        let mut requested: Vec<AssetNameExchange> = (0..17)
            .map(|i| AssetNameExchange::new(format!("A{i:02}")))
            .collect();
        requested.push(AssetNameExchange::new("BTC"));
        requested.push(AssetNameExchange::new("ETH"));
        assert!(
            requested.len() > 16,
            "must exceed the linear-scan threshold"
        );

        let filtered = filter_and_convert_margin_balances(assets, &requested);
        assert_eq!(filtered.len(), 2);
        assert!(filtered.iter().all(|b| b.asset.name().as_str() != "DOGE"));
    }

    // -----------------------------------------------------------------------
    // Isolated account snapshot converters + query scoping
    // -----------------------------------------------------------------------

    use binance_sdk::margin_trading::rest_api::{
        QueryIsolatedMarginAccountInfoResponseAssetsInnerBaseAsset as IsoBase,
        QueryIsolatedMarginAccountInfoResponseAssetsInnerQuoteAsset as IsoQuote,
    };

    fn d(s: &str) -> Decimal {
        Decimal::from_str(s).unwrap()
    }

    fn iso_base(asset: &str, free: &str, locked: &str, borrowed: &str, interest: &str) -> IsoBase {
        let mut b = IsoBase::new();
        b.asset = Some(asset.to_string());
        b.free = Some(free.to_string());
        b.locked = Some(locked.to_string());
        b.borrowed = Some(borrowed.to_string());
        b.interest = Some(interest.to_string());
        b
    }

    fn iso_quote(
        asset: &str,
        free: &str,
        locked: &str,
        borrowed: &str,
        interest: &str,
    ) -> IsoQuote {
        let mut q = IsoQuote::new();
        q.asset = Some(asset.to_string());
        q.free = Some(free.to_string());
        q.locked = Some(locked.to_string());
        q.borrowed = Some(borrowed.to_string());
        q.interest = Some(interest.to_string());
        q
    }

    fn iso_entry(
        symbol: &str,
        base: IsoBase,
        quote: IsoQuote,
        margin_level: Option<&str>,
        margin_ratio: Option<&str>,
        liquidate_price: Option<&str>,
    ) -> QueryIsolatedMarginAccountInfoResponseAssetsInner {
        let mut e = QueryIsolatedMarginAccountInfoResponseAssetsInner::new();
        e.symbol = Some(symbol.to_string());
        e.base_asset = Some(Box::new(base));
        e.quote_asset = Some(Box::new(quote));
        e.margin_level = margin_level.map(str::to_string);
        e.margin_ratio = margin_ratio.map(str::to_string);
        e.liquidate_price = liquidate_price.map(str::to_string);
        e
    }

    fn isolated_client(symbols: &[&str]) -> BinanceMargin {
        let config = BinanceMarginConfig::isolated(
            "k".to_string(),
            "s".to_string(),
            symbols
                .iter()
                .map(|s| InstrumentNameExchange::new(*s))
                .collect(),
        );
        BinanceMargin::new(config)
    }

    #[test]
    fn isolated_assets_map_base_quote_and_risk() {
        // base: free 0.5 + locked 0.1 = total 0.6, borrowed 0.2 → net 0.4.
        // quote: free 1000 + locked 50 = total 1050, borrowed 300 → net 750.
        let entry = iso_entry(
            "BTCUSDT",
            iso_base("BTC", "0.5", "0.1", "0.2", "0.001"),
            iso_quote("USDT", "1000", "50", "300", "1.5"),
            Some("3.5"),
            Some("0.12"),
            Some("48000"),
        );
        let map = convert_isolated_margin_assets(vec![entry]);
        let state = map
            .get(&InstrumentNameExchange::new("BTCUSDT"))
            .expect("entry present");

        assert_eq!(state.base.asset.name().as_str(), "BTC");
        assert_eq!(state.base.balance.total, d("0.6"));
        assert_eq!(state.base.balance.free, d("0.5"));
        assert_eq!(state.base.balance.net_asset(), d("0.4"));

        assert_eq!(state.quote.asset.name().as_str(), "USDT");
        assert_eq!(state.quote.balance.total, d("1050"));
        assert_eq!(state.quote.balance.net_asset(), d("750"));

        assert_eq!(state.risk.margin_level, Some(d("3.5")));
        assert_eq!(state.risk.margin_ratio, Some(d("0.12")));
        assert_eq!(state.risk.liquidation_price, Some(d("48000")));
    }

    #[test]
    fn isolated_base_short_is_negative_net() {
        // A short on the base side: total 0, borrowed 1.5 → net_asset = -1.5.
        let entry = iso_entry(
            "BTCUSDT",
            iso_base("BTC", "0", "0", "1.5", "0.001"),
            iso_quote("USDT", "100", "0", "0", "0"),
            None,
            None,
            None,
        );
        let map = convert_isolated_margin_assets(vec![entry]);
        let state = map.get(&InstrumentNameExchange::new("BTCUSDT")).unwrap();
        assert_eq!(state.base.balance.net_asset(), d("-1.5"));
    }

    #[test]
    fn isolated_missing_debt_defaults_zero_and_risk_optional() {
        // No borrowed/interest = real no-debt position → margin still populated (zero debt), not
        // dropped. Absent risk fields → None, but the snapshot is still produced.
        let mut base = IsoBase::new();
        base.asset = Some("BTC".to_string());
        base.free = Some("0.5".to_string());
        base.locked = Some("0".to_string());
        let mut quote = IsoQuote::new();
        quote.asset = Some("USDT".to_string());
        quote.free = Some("100".to_string());
        quote.locked = Some("0".to_string());

        let map = convert_isolated_margin_assets(vec![iso_entry(
            "BTCUSDT", base, quote, None, None, None,
        )]);
        let state = map.get(&InstrumentNameExchange::new("BTCUSDT")).unwrap();
        assert!(state.base.balance.margin.is_some());
        assert_eq!(state.base.balance.net_asset(), d("0.5"));
        assert_eq!(state.risk.margin_level, None);
        assert_eq!(state.risk.liquidation_price, None);
    }

    #[test]
    fn isolated_unparseable_risk_is_none_but_entry_kept() {
        let entry = iso_entry(
            "BTCUSDT",
            iso_base("BTC", "1", "0", "0", "0"),
            iso_quote("USDT", "1", "0", "0", "0"),
            Some("not_a_number"),
            None,
            None,
        );
        let map = convert_isolated_margin_assets(vec![entry]);
        let state = map.get(&InstrumentNameExchange::new("BTCUSDT")).unwrap();
        assert_eq!(state.risk.margin_level, None);
    }

    #[test]
    fn isolated_missing_base_free_drops_entry() {
        // Missing free/locked is corrupt (not no-debt) → drop the whole pair entry.
        let mut base = IsoBase::new();
        base.asset = Some("BTC".to_string());
        base.locked = Some("0".to_string()); // no free
        let entry = iso_entry(
            "BTCUSDT",
            base,
            iso_quote("USDT", "100", "0", "0", "0"),
            None,
            None,
            None,
        );
        assert!(convert_isolated_margin_assets(vec![entry]).is_empty());
    }

    #[test]
    fn isolated_missing_symbol_drops_entry() {
        let mut e = QueryIsolatedMarginAccountInfoResponseAssetsInner::new();
        e.base_asset = Some(Box::new(iso_base("BTC", "1", "0", "0", "0")));
        e.quote_asset = Some(Box::new(iso_quote("USDT", "1", "0", "0", "0")));
        // no symbol set
        assert!(convert_isolated_margin_assets(vec![e]).is_empty());
    }

    #[test]
    fn chunk_symbols_batches_by_five() {
        let syms: Vec<InstrumentNameExchange> = (0..12)
            .map(|i| InstrumentNameExchange::new(format!("S{i:02}")))
            .collect();
        let chunks = chunk_symbols(&syms);
        assert_eq!(chunks.len(), 3, "12 symbols → 5 + 5 + 2");
        assert_eq!(chunks[0].split(',').count(), 5);
        assert_eq!(chunks[1].split(',').count(), 5);
        assert_eq!(chunks[2].split(',').count(), 2);

        // ≤5 → a single chunk.
        let three: Vec<_> = (0..3)
            .map(|i| InstrumentNameExchange::new(format!("S{i}")))
            .collect();
        assert_eq!(chunk_symbols(&three).len(), 1);

        // exactly 5 → a single chunk of 5.
        let five: Vec<_> = (0..5)
            .map(|i| InstrumentNameExchange::new(format!("S{i}")))
            .collect();
        let c = chunk_symbols(&five);
        assert_eq!(c.len(), 1);
        assert_eq!(c[0].split(',').count(), 5);

        // empty → no chunks (never a no-symbol call).
        assert!(chunk_symbols(&[]).is_empty());
    }

    #[test]
    fn effective_isolated_set_empty_returns_all_configured() {
        let client = isolated_client(&["BTCUSDT", "ETHUSDT"]);
        assert_eq!(
            client.effective_isolated_set(&[]),
            vec![
                InstrumentNameExchange::new("BTCUSDT"),
                InstrumentNameExchange::new("ETHUSDT"),
            ]
        );
    }

    #[test]
    fn effective_isolated_set_intersects_requested() {
        let client = isolated_client(&["BTCUSDT", "ETHUSDT"]);
        assert_eq!(
            client.effective_isolated_set(&[InstrumentNameExchange::new("ETHUSDT")]),
            vec![InstrumentNameExchange::new("ETHUSDT")]
        );
    }

    #[test]
    fn effective_isolated_set_skips_out_of_set() {
        let client = isolated_client(&["BTCUSDT", "ETHUSDT"]);
        // DOGEUSDT is not configured → skipped (warn), only BTCUSDT survives.
        assert_eq!(
            client.effective_isolated_set(&[
                InstrumentNameExchange::new("BTCUSDT"),
                InstrumentNameExchange::new("DOGEUSDT"),
            ]),
            vec![InstrumentNameExchange::new("BTCUSDT")]
        );
    }

    #[tokio::test]
    async fn fetch_balances_isolated_returns_empty() {
        // Isolated balances are per-pair (on account_snapshot); the asset-keyed fetch returns empty
        // without a network call.
        let client = isolated_client(&["BTCUSDT"]);
        assert!(client.fetch_balances(&[]).await.expect("ok").is_empty());
    }

    fn open_order(order_id: Option<i64>, side: &str) -> QueryMarginAccountsOpenOrdersResponseInner {
        let mut o = QueryMarginAccountsOpenOrdersResponseInner::new();
        o.order_id = order_id;
        o.client_order_id = Some("cid-9".to_string());
        o.side = Some(side.to_string());
        o.price = Some("50000".to_string());
        o.orig_qty = Some("2".to_string());
        o.executed_qty = Some("0.5".to_string());
        o.r#type = Some("LIMIT".to_string());
        o.time_in_force = Some("GTC".to_string());
        o.time = Some(1_700_000_000_000);
        o.status = Some("PARTIALLY_FILLED".to_string());
        o
    }

    #[test]
    fn margin_open_order_converts() {
        let inst = InstrumentNameExchange::new("BTCUSDT");
        let order = convert_open_order(
            &open_order(Some(42), "BUY"),
            ExchangeId::BinanceMargin,
            &inst,
        )
        .expect("convert");
        assert_eq!(order.key.exchange, ExchangeId::BinanceMargin);
        assert_eq!(
            order.state.id.assigned().map(|id| id.0.as_str()),
            Some("42")
        );
        assert_eq!(order.key.cid.0.as_str(), "cid-9");
        assert_eq!(order.side, Side::Buy);
        assert_eq!(order.price, Some(Decimal::from(50_000)));
        assert_eq!(order.quantity, Decimal::from(2));
        assert_eq!(order.state.filled_quantity, Decimal::new(5, 1));
        assert_eq!(order.kind, OrderKind::Limit);
    }

    /// Margin shares the converter, so it shares the guard: a cancelled order that had partly
    /// filled must not come back as `Open`.
    #[test]
    fn margin_open_order_that_is_not_live_is_dropped() {
        let inst = InstrumentNameExchange::new("BTCUSDT");
        let mut o = open_order(Some(42), "BUY");
        o.status = Some("CANCELED".to_string());
        assert!(convert_open_order(&o, ExchangeId::BinanceMargin, &inst).is_none());
    }

    #[test]
    fn margin_open_order_missing_order_id_is_dropped() {
        let inst = InstrumentNameExchange::new("BTCUSDT");
        assert!(
            convert_open_order(&open_order(None, "BUY"), ExchangeId::BinanceMargin, &inst)
                .is_none()
        );
    }

    /// A margin REST snapshot must be stamped with when the order last *changed*, not when it was
    /// created. The engine orders an order's states by `Open::time_exchange` and discards any
    /// snapshot older than the state it already tracks, so a creation stamp -- identical across
    /// every snapshot of one order -- is discarded the moment anything advances that order past
    /// creation. That is exactly the partially-filled order a reconciliation fetch exists to repair.
    #[test]
    fn margin_open_order_prefers_update_time_over_creation_time() {
        let inst = InstrumentNameExchange::new("BTCUSDT");

        let mut updated = open_order(Some(42), "BUY");
        updated.update_time = Some(1_700_000_005_000);
        let order =
            convert_open_order(&updated, ExchangeId::BinanceMargin, &inst).expect("convert");
        assert_eq!(
            order.state.time_exchange,
            Utc.timestamp_millis_opt(1_700_000_005_000)
                .single()
                .unwrap(),
            "update_time must win over time"
        );
    }

    /// Falls back to `time` when the venue omits `update_time`, rather than to `now` -- which would
    /// be worse than creation time, since it is not a venue-reported instant at all.
    #[test]
    fn margin_open_order_falls_back_to_creation_time_when_update_time_absent() {
        let inst = InstrumentNameExchange::new("BTCUSDT");

        let base = open_order(Some(42), "BUY");
        assert!(base.update_time.is_none(), "fixture must omit update_time");
        let order = convert_open_order(&base, ExchangeId::BinanceMargin, &inst).expect("convert");
        assert_eq!(
            order.state.time_exchange,
            Utc.timestamp_millis_opt(1_700_000_000_000)
                .single()
                .unwrap(),
            "absent update_time falls back to time"
        );
    }

    #[test]
    fn margin_open_order_owned_symbol_recovers_instrument() {
        // The no-symbol "return all" path derives the instrument from each order's own `symbol`.
        let mut o = open_order(Some(42), "SELL");
        o.symbol = Some("ETHUSDT".to_string());
        let order =
            convert_open_order_owned_symbol(&o, ExchangeId::BinanceMargin).expect("convert");
        assert_eq!(order.key.instrument.name().as_str(), "ETHUSDT");
        assert_eq!(order.key.exchange, ExchangeId::BinanceMargin);
        assert_eq!(order.side, Side::Sell);
    }

    #[test]
    fn margin_open_order_owned_symbol_missing_symbol_is_dropped() {
        // open_order() leaves `symbol` unset — the return-all path must drop it rather than guess.
        let o = open_order(Some(42), "BUY");
        assert!(o.symbol.is_none());
        assert!(convert_open_order_owned_symbol(&o, ExchangeId::BinanceMargin).is_none());
    }

    fn trade(id: Option<i64>, is_buyer: Option<bool>) -> QueryMarginAccountsTradeListResponseInner {
        let mut t = QueryMarginAccountsTradeListResponseInner::new();
        t.id = id;
        t.order_id = Some(7);
        t.is_buyer = is_buyer;
        t.price = Some("48000".to_string());
        t.qty = Some("0.25".to_string());
        t.commission = Some("0.001".to_string());
        t.commission_asset = Some("BNB".to_string());
        t.time = Some(1_700_000_000_000);
        t
    }

    #[test]
    fn margin_trade_converts() {
        let inst = InstrumentNameExchange::new("BTCUSDT");
        let tr = convert_margin_trade(&trade(Some(11), Some(false)), &inst).expect("convert");
        assert_eq!(tr.id.0.as_str(), "11");
        assert_eq!(tr.order_id.0.as_str(), "7");
        assert_eq!(tr.side, Side::Sell);
        assert_eq!(tr.price, Decimal::from(48_000));
        assert_eq!(tr.quantity, Decimal::new(25, 2));
        // Third-party fee asset (BNB) preserved verbatim; fees_quote left for the indexer.
        assert_eq!(tr.fees.asset.name().as_str(), "BNB");
        assert_eq!(tr.fees.fees, Decimal::new(1, 3));
    }

    #[test]
    fn margin_trade_missing_fields_are_dropped() {
        let inst = InstrumentNameExchange::new("BTCUSDT");
        // Missing id and missing isBuyer both drop the trade rather than guess.
        assert!(convert_margin_trade(&trade(None, Some(true)), &inst).is_none());
        assert!(convert_margin_trade(&trade(Some(11), None), &inst).is_none());
    }

    // -- User-data stream: token renewal -------------------------------------------------------

    #[test]
    fn token_renew_after_applies_margin_and_floor() {
        let now_ms = Utc::now().timestamp_millis();
        // ~24h out: renewal waits roughly (24h − 5min), bounded below 24h.
        let far = token_renew_after(now_ms + 86_400_000).as_secs();
        assert!(
            far > 80_000 && far <= 86_400,
            "expected ~24h-minus-margin, got {far}"
        );
        // Already expired → clamped to the floor, never zero.
        assert_eq!(
            token_renew_after(now_ms - 10_000).as_secs(),
            TOKEN_MIN_LIFETIME_SECS
        );
        // Within the safety margin → also the floor (renew now-ish, not at the deadline).
        assert_eq!(
            token_renew_after(now_ms + 60_000).as_secs(),
            TOKEN_MIN_LIFETIME_SECS
        );
    }

    // -- User-data stream: frame discrimination + event conversion -----------------------------

    /// Wrap an inner user-data `event` object in the WS-API push envelope observed live,
    /// serialized to the on-wire JSON string the converter parses.
    fn push(event: serde_json::Value) -> String {
        serde_json::json!({ "subscriptionId": 1, "event": event }).to_string()
    }

    #[test]
    fn margin_ws_rpc_ack_yields_no_events() {
        // An RPC response (e.g. the subscribe ack) carries a top-level `id` and is not user data.
        let frame = serde_json::json!({ "id": "abc", "status": 200, "result": {} }).to_string();
        let mut buf = Vec::new();
        assert!(!convert_margin_user_data_events(&frame, &mut buf));
        assert!(buf.is_empty());
    }

    /// A bare event, with no envelope, or an envelope whose event has no `e` tag, is not a shape
    /// the subscription delivers: nothing is converted and the stream is not terminated.
    #[test]
    fn margin_ws_unrecognised_frames_yield_no_events() {
        for frame in [
            r#"{"e":"eventStreamTerminated","E":1}"#,
            r#"{"subscriptionId":1,"event":{"E":1}}"#,
        ] {
            let mut buf = Vec::new();
            assert!(!convert_margin_user_data_events(frame, &mut buf), "{frame}");
            assert!(buf.is_empty(), "{frame}");
        }
    }

    /// A partial fill carries two facts, and both must reach the consumer: the execution print
    /// (`l`/`L`) and the order's new cumulative filled quantity (`z`).
    ///
    /// The execution alone never moves the order -- `Orders::update_from_fill` writes only
    /// `filled_quantity` and needs the order already tracked -- so without the paired snapshot a
    /// partially filled margin order reads as having nothing filled until REST reconciliation
    /// refreshes it.
    #[test]
    fn margin_ws_execution_report_trade_maps_to_trade_and_order_snapshot() {
        let frame = push(serde_json::json!({
            "e": "executionReport", "s": "BTCUSDT", "S": "BUY", "o": "LIMIT",
            "x": "TRADE", "X": "PARTIALLY_FILLED", "i": 12_345_i64, "c": "cid-1",
            "t": 99_i64, "l": "0.5", "L": "48000", "z": "0.5",
            "p": "48000", "q": "2", "f": "GTC",
            "n": "0.001", "N": "BNB", "T": 1_700_000_000_000_i64,
        }));
        let mut buf = Vec::new();
        assert!(!convert_margin_user_data_events(&frame, &mut buf));
        assert_eq!(
            buf.len(),
            2,
            "a fill must produce the execution and the order snapshot, got: {buf:?}"
        );
        assert_eq!(buf[0].exchange, ExchangeId::BinanceMargin);
        match &buf[0].kind {
            AccountEventKind::Trade(t) => {
                assert_eq!(t.instrument.name().as_str(), "BTCUSDT");
                assert_eq!(t.side, Side::Buy);
                assert_eq!(t.price, Decimal::from(48_000));
                assert_eq!(t.quantity, Decimal::new(5, 1));
                assert_eq!(t.fees.asset.name().as_str(), "BNB"); // 3rd-party fee asset preserved
                assert_eq!(t.fees.fees, Decimal::new(1, 3));
            }
            other => panic!("expected Trade, got {other:?}"),
        }
        assert_eq!(buf[1].exchange, ExchangeId::BinanceMargin);
        match &buf[1].kind {
            AccountEventKind::OrderSnapshot(snap) => {
                assert_eq!(
                    snap.0.quantity,
                    Decimal::from(2),
                    "the order's total quantity"
                );
                match &snap.0.state {
                    OrderState::Active(ActiveOrderState::Open(open)) => {
                        assert_eq!(open.id.assigned().map(|id| id.0.as_str()), Some("12345"));
                        assert_eq!(
                            open.filled_quantity,
                            Decimal::new(5, 1),
                            "the snapshot must carry the order's cumulative filled quantity (z)"
                        );
                        assert_eq!(
                            open.time_exchange.timestamp_millis(),
                            1_700_000_000_000,
                            "stamped with the execution's transaction time (T), which is what \
                             gives the engine's staleness gate an ordering key"
                        );
                    }
                    other => panic!("expected an Open order, got {other:?}"),
                }
            }
            other => panic!("expected OrderSnapshot, got {other:?}"),
        }
    }

    /// The mirror of the case above: a `TRADE` whose order status says the order is no longer
    /// working must emit the execution *without* a snapshot, or it resurrects an order the
    /// engine has already retired as a resting one.
    #[test]
    fn margin_trade_for_an_order_no_longer_working_emits_no_snapshot() {
        for status in ["CANCELED", "EXPIRED", "REJECTED", "PENDING_CANCEL", "NEW"] {
            let frame = push(serde_json::json!({
                "e": "executionReport", "s": "BTCUSDT", "S": "BUY", "o": "LIMIT",
                "x": "TRADE", "X": status, "i": 12_345_i64, "c": "cid-1",
                "t": 99_i64, "l": "0.5", "L": "48000", "z": "0.5",
                "p": "48000", "q": "2", "f": "GTC", "T": 1_700_000_000_000_i64,
            }));
            let mut buf = Vec::new();
            assert!(!convert_margin_user_data_events(&frame, &mut buf));
            assert_eq!(
                buf.len(),
                1,
                "status {status} must produce the execution alone, got: {buf:?}"
            );
            assert!(
                matches!(buf[0].kind, AccountEventKind::Trade(_)),
                "the execution must still be reported for status {status}"
            );
        }
    }

    #[test]
    fn margin_ws_new_report_maps_to_active_order_snapshot() {
        let frame = push(serde_json::json!({
            "e": "executionReport", "s": "BTCUSDT", "S": "SELL", "o": "LIMIT",
            "x": "NEW", "X": "NEW", "i": 7_i64, "c": "cid-2",
            "p": "48000", "q": "1", "f": "GTC", "z": "0", "T": 1_700_000_000_000_i64,
        }));
        let mut buf = Vec::new();
        assert!(!convert_margin_user_data_events(&frame, &mut buf));
        assert_eq!(buf.len(), 1);
        match &buf[0].kind {
            AccountEventKind::OrderSnapshot(snap) => {
                assert_eq!(snap.0.side, Side::Sell);
                assert_eq!(snap.0.price, Some(Decimal::from(48_000)));
                assert_eq!(snap.0.quantity, Decimal::from(1));
                assert!(
                    matches!(snap.0.state, OrderState::Active(_)),
                    "NEW should be an active (resting) order"
                );
            }
            other => panic!("expected OrderSnapshot, got {other:?}"),
        }
    }

    #[test]
    fn margin_ws_canceled_report_maps_to_order_cancelled() {
        let frame = push(serde_json::json!({
            "e": "executionReport", "s": "BTCUSDT", "S": "BUY", "o": "LIMIT",
            "x": "CANCELED", "X": "CANCELED", "i": 8_i64, "c": "cid-3",
            "z": "0", "T": 1_700_000_000_000_i64,
        }));
        let mut buf = Vec::new();
        assert!(!convert_margin_user_data_events(&frame, &mut buf));
        assert_eq!(buf.len(), 1);
        match &buf[0].kind {
            AccountEventKind::OrderCancelled(resp) => assert!(resp.state.is_ok()),
            other => panic!("expected OrderCancelled, got {other:?}"),
        }
    }

    #[test]
    fn margin_ws_execution_report_missing_symbol_is_dropped() {
        // Symbol absent → defensively dropped (observable warn), never a half-built event.
        let frame = push(serde_json::json!({
            "e": "executionReport", "S": "BUY", "x": "TRADE", "i": 1_i64, "t": 1_i64,
            "l": "1", "L": "100", "T": 1_700_000_000_000_i64,
        }));
        let mut buf = Vec::new();
        assert!(!convert_margin_user_data_events(&frame, &mut buf));
        assert!(buf.is_empty());
    }

    #[test]
    fn margin_ws_outbound_account_position_maps_to_balance_stream_updates() {
        let frame = push(serde_json::json!({
            "e": "outboundAccountPosition", "u": 1_700_000_000_000_i64,
            "B": [
                { "a": "USDT", "f": "100.0", "l": "5.0" },
                { "a": "BTC", "f": "0.5", "l": "0" },
            ],
        }));
        let mut buf = Vec::new();
        assert!(!convert_margin_user_data_events(&frame, &mut buf));
        assert_eq!(buf.len(), 2);
        // WS partial → BalanceStreamUpdate (free/locked), never BalanceSnapshot (no debt clobber).
        for ev in &buf {
            assert!(matches!(ev.kind, AccountEventKind::BalanceStreamUpdate(_)));
        }
    }

    #[test]
    fn margin_ws_stream_terminated_signals_reconnect() {
        let frame = push(serde_json::json!({ "e": "eventStreamTerminated" }));
        let mut buf = Vec::new();
        assert!(
            convert_margin_user_data_events(&frame, &mut buf),
            "eventStreamTerminated must signal reconnect"
        );
        assert!(buf.is_empty());
    }

    #[test]
    fn margin_ws_margin_specific_events_are_observable_only() {
        // userLiabilityChange / marginLevelStatusChange are logged, NOT forwarded as account events
        // and NOT signalled as reconnects: observable, not accumulated.
        let mut buf = Vec::new();
        let liability = push(serde_json::json!({
            "e": "userLiabilityChange", "a": "USDT", "t": "BORROW", "p": "100", "i": "0.01",
        }));
        assert!(!convert_margin_user_data_events(&liability, &mut buf));
        let level = push(serde_json::json!({
            "e": "marginLevelStatusChange", "l": "1.5", "s": "MARGIN_LEVEL_2",
        }));
        assert!(!convert_margin_user_data_events(&level, &mut buf));
        assert!(buf.is_empty());
    }

    // -- User-data stream: dedup -----------------------------------------------------------------

    #[test]
    fn margin_trade_event_dedup_by_key() {
        // A recovered fill and its live WS counterpart share (instrument, trade_id, kind) → the
        // dedup cache must drop the second occurrence (mirrors the reconnect fill-recovery path).
        let frame = push(serde_json::json!({
            "e": "executionReport", "s": "BTCUSDT", "S": "BUY", "o": "LIMIT",
            "x": "TRADE", "X": "FILLED", "i": 1_i64, "c": "cid", "t": 4_242_i64,
            "l": "1", "L": "100", "z": "1", "p": "100", "q": "1", "f": "GTC",
            "n": "0", "N": "USDT", "T": 1_700_000_000_000_i64,
        }));
        let mut buf = Vec::new();
        convert_margin_user_data_events(&frame, &mut buf);
        // Index rather than pop: a fill appends the execution *then* its order snapshot, and
        // popping would take the snapshot and silently test the wrong event's key.
        let event = buf.first().expect("a Trade event");
        assert!(
            matches!(event.kind, AccountEventKind::Trade(_)),
            "the execution is the first of the two events a fill produces"
        );

        let cache = new_dedup_cache();
        // DedupKey isn't Clone; re-derive it from the same event (deterministic) for the 2nd check.
        let first = dedup_key_from_event(event).expect("Trade events have a dedup key");
        assert!(!is_duplicate(&cache, first), "first sighting is fresh");
        let second = dedup_key_from_event(event).expect("Trade events have a dedup key");
        assert!(
            is_duplicate(&cache, second),
            "second sighting is a duplicate"
        );
    }

    /// Drive frames through the same two stages the live WebSocket callback does: convert, then
    /// apply the dedup gate. Returns what a consumer would actually receive.
    ///
    /// Every other user-data test in this file calls the converter directly. That is the wrong
    /// altitude to prove a fill reaches anyone: the converter can be perfectly correct while the
    /// gate downstream of it discards what the converter produced. Margin's gate is structurally
    /// identical to spot's, so it needs the same end-to-end coverage.
    fn drive_ws_pipeline(frames: &[&str]) -> Vec<UnindexedAccountEvent> {
        let cache = new_dedup_cache();
        let mut delivered = Vec::new();
        let mut buf = Vec::new();
        for frame in frames {
            let _ = convert_margin_user_data_events(frame, &mut buf);
            for ev in buf.drain(..) {
                if let Some(key) = dedup_key_from_event(&ev)
                    && is_duplicate(&cache, key)
                {
                    continue;
                }
                delivered.push(ev);
            }
        }
        delivered
    }

    fn ws_new(cumulative_filled: &str) -> String {
        push(serde_json::json!({
            "e": "executionReport", "s": "BTCUSDT", "S": "BUY", "o": "LIMIT",
            "x": "NEW", "X": "NEW", "i": 12_345_i64, "c": "client-1",
            "q": "2", "p": "100", "f": "GTC", "z": cumulative_filled,
            "T": 1_700_000_000_000_i64,
        }))
    }

    fn ws_partial_fill(cumulative_filled: &str) -> String {
        push(serde_json::json!({
            "e": "executionReport", "s": "BTCUSDT", "S": "BUY", "o": "LIMIT",
            "x": "TRADE", "X": "PARTIALLY_FILLED", "i": 12_345_i64, "c": "client-1",
            "q": "2", "p": "100", "f": "GTC", "z": cumulative_filled,
            "l": "1", "L": "100", "t": 555_i64, "n": "0.1", "N": "USDT",
            "T": 1_700_000_001_000_i64,
        }))
    }

    fn open_filled_quantities(events: &[UnindexedAccountEvent]) -> Vec<Decimal> {
        events
            .iter()
            .filter_map(|ev| match &ev.kind {
                AccountEventKind::OrderSnapshot(snap) => match &snap.0.state {
                    OrderState::Active(ActiveOrderState::Open(open)) => Some(open.filled_quantity),
                    _ => None,
                },
                _ => None,
            })
            .collect()
    }

    /// A fill's order snapshot must survive the dedup gate that sits between the converter and
    /// the consumer.
    ///
    /// The acknowledgement and the fill describe one order id, so a key built from the id alone
    /// would make the second look like a replay of the first. `Open::filled_quantity` would then
    /// never leave the `0` the acknowledgement carried -- the defect fixed in the converter,
    /// undone one stage later.
    #[test]
    fn margin_ws_fill_snapshot_survives_the_dedup_gate() {
        let delivered = drive_ws_pipeline(&[&ws_new("0"), &ws_partial_fill("1")]);

        assert_eq!(
            delivered.len(),
            3,
            "expected the ack snapshot, then the fill's execution and its snapshot, got: {delivered:?}"
        );
        assert_eq!(
            open_filled_quantities(&delivered),
            vec![Decimal::ZERO, Decimal::ONE],
            "the fill's snapshot must reach the consumer carrying the advanced filled quantity"
        );
    }

    /// The gate must still collapse a genuinely re-delivered frame, because a replayed
    /// acknowledgement for a retired order is re-inserted as a live resting order by the engine.
    #[test]
    fn margin_ws_replayed_frame_is_still_deduplicated() {
        let delivered = drive_ws_pipeline(&[
            &ws_new("0"),
            &ws_partial_fill("1"),
            &ws_new("0"),
            &ws_partial_fill("1"),
        ]);

        assert_eq!(
            delivered.len(),
            3,
            "re-delivering both frames must add nothing, got: {delivered:?}"
        );
    }

    /// An acknowledgement that already carries a partial fill, followed by the `TRADE` reporting
    /// that same fill, describes one order state twice -- so the gate collapses the second
    /// snapshot. That is correct, not a loss: the dedup key includes `filled_quantity`, the two
    /// snapshots agree on it, and the execution itself carries a distinct trade id and is
    /// delivered either way.
    #[test]
    fn margin_ws_an_ack_and_its_fill_reporting_one_state_deliver_that_state_once() {
        let delivered = drive_ws_pipeline(&[&ws_new("1"), &ws_partial_fill("1")]);

        assert_eq!(
            delivered.len(),
            2,
            "the ack snapshot and the execution, with the fill's duplicate snapshot collapsed, \
             got: {delivered:?}"
        );
        assert!(
            delivered
                .iter()
                .any(|ev| matches!(ev.kind, AccountEventKind::Trade(_))),
            "the execution must reach the consumer regardless"
        );
        assert_eq!(
            open_filled_quantities(&delivered),
            vec![Decimal::ONE],
            "one snapshot, carrying the state both reports agree on"
        );
    }

    // -- Isolated user-data stream: token params, base/quote map, balance routing ---------------

    /// Wrap an inner `event` in the WS-API push envelope with an explicit `subscriptionId` (the
    /// isolated routing key for symbol-less `outboundAccountPosition` frames).
    fn push_with_sub(subscription_id: i64, event: serde_json::Value) -> String {
        serde_json::json!({ "subscriptionId": subscription_id, "event": event }).to_string()
    }

    /// Build the isolated balance handler (closes over the routing maps) for driving
    /// `convert_margin_user_data_events_with` in tests.
    fn isolated_handler(
        sub_map: Arc<Mutex<HashMap<i64, InstrumentNameExchange>>>,
        base_quote: Arc<HashMap<InstrumentNameExchange, (AssetNameExchange, AssetNameExchange)>>,
    ) -> impl FnMut(OutboundAccountPosition, Option<i64>, &mut Vec<UnindexedAccountEvent>) {
        move |position, subscription_id, buf| {
            route_isolated_account_position(position, subscription_id, &sub_map, &base_quote, buf);
        }
    }

    fn btcusdt() -> InstrumentNameExchange {
        InstrumentNameExchange::new("BTCUSDT")
    }

    fn token(expiration_time_ms: i64) -> UserListenToken {
        UserListenToken {
            token: "t".to_string(),
            expiration_time_ms,
        }
    }

    #[test]
    fn listen_token_query_cross_vs_isolated() {
        // Cross → no params (the signed POST sends an empty query, as it did before isolated).
        assert!(build_listen_token_query(None).is_empty());

        // Isolated → isIsolated=TRUE & symbol=<sym> (the per-symbol scoping).
        let q = build_listen_token_query(Some(&btcusdt()));
        assert_eq!(q.get("isIsolated").and_then(|v| v.as_str()), Some("TRUE"));
        assert_eq!(q.get("symbol").and_then(|v| v.as_str()), Some("BTCUSDT"));
        assert_eq!(q.len(), 2);
    }

    #[test]
    fn earliest_token_expiry_picks_min() {
        let tokens = vec![
            (btcusdt(), token(3_000)),
            (InstrumentNameExchange::new("ETHUSDT"), token(1_000)),
            (InstrumentNameExchange::new("BNBUSDT"), token(2_000)),
        ];
        assert_eq!(earliest_token_expiry_ms(&tokens), 1_000);
        // Empty set → sentinel (never happens for isolated, which always has ≥1 symbol).
        assert_eq!(earliest_token_expiry_ms(&[]), i64::MAX);
    }

    #[test]
    fn base_quote_map_extracts_base_and_quote() {
        let entries = vec![
            iso_entry(
                "BTCUSDT",
                iso_base("BTC", "0", "0", "0", "0"),
                iso_quote("USDT", "0", "0", "0", "0"),
                None,
                None,
                None,
            ),
            iso_entry(
                "ETHBTC",
                iso_base("ETH", "0", "0", "0", "0"),
                iso_quote("BTC", "0", "0", "0", "0"),
                None,
                None,
                None,
            ),
        ];
        let map = build_base_quote_map(&entries);
        assert_eq!(
            map.get(&btcusdt())
                .map(|(b, q)| (b.name().as_str(), q.name().as_str())),
            Some(("BTC", "USDT"))
        );
        // ETHBTC: prefix/suffix string-matching would be ambiguous (BTC is the quote here, base
        // elsewhere) — the authoritative map resolves it unambiguously to base=ETH, quote=BTC.
        assert_eq!(
            map.get(&InstrumentNameExchange::new("ETHBTC"))
                .map(|(b, q)| (b.name().as_str(), q.name().as_str())),
            Some(("ETH", "BTC"))
        );
    }

    #[test]
    fn isolated_outbound_position_routes_to_instrument_balance_update() {
        let sub_map = Arc::new(Mutex::new(HashMap::from([(7_i64, btcusdt())])));
        let base_quote = Arc::new(HashMap::from([(
            btcusdt(),
            (
                AssetNameExchange::new("BTC"),
                AssetNameExchange::new("USDT"),
            ),
        )]));
        let mut handler = isolated_handler(sub_map, base_quote);

        // Quote listed before base in `B` → assignment is by asset-name equality, not position.
        let frame = push_with_sub(
            7,
            serde_json::json!({
                "e": "outboundAccountPosition", "u": 1_700_000_000_000_i64,
                "B": [
                    { "a": "USDT", "f": "1000.0", "l": "50.0" },
                    { "a": "BTC", "f": "0.5", "l": "0.1" },
                ],
            }),
        );
        let mut buf = Vec::new();
        assert!(!convert_margin_user_data_events_with(
            &frame,
            &mut buf,
            &mut handler
        ));
        assert_eq!(buf.len(), 1, "one InstrumentBalanceUpdate for the pair");
        match &buf[0].kind {
            AccountEventKind::InstrumentBalanceUpdate(ibu) => {
                assert_eq!(ibu.instrument.name().as_str(), "BTCUSDT");
                assert_eq!(ibu.base.asset.name().as_str(), "BTC");
                assert_eq!(ibu.base.update.free, Decimal::new(5, 1));
                assert_eq!(ibu.base.update.locked, Decimal::new(1, 1));
                assert_eq!(ibu.quote.asset.name().as_str(), "USDT");
                assert_eq!(ibu.quote.update.free, Decimal::from(1000));
                assert_eq!(ibu.quote.update.locked, Decimal::from(50));
            }
            other => panic!("expected InstrumentBalanceUpdate, got {other:?}"),
        }
        // Crucially NOT the asset-keyed BalanceStreamUpdate (which would corrupt AssetStates).
        assert!(!matches!(
            buf[0].kind,
            AccountEventKind::BalanceStreamUpdate(_)
        ));
    }

    #[test]
    fn isolated_outbound_position_unknown_subscription_dropped() {
        // subscriptionId 99 is not in the map → dropped (observable warn), nothing emitted.
        let sub_map = Arc::new(Mutex::new(HashMap::from([(7_i64, btcusdt())])));
        let base_quote = Arc::new(HashMap::from([(
            btcusdt(),
            (
                AssetNameExchange::new("BTC"),
                AssetNameExchange::new("USDT"),
            ),
        )]));
        let mut handler = isolated_handler(sub_map, base_quote);

        let frame = push_with_sub(
            99,
            serde_json::json!({
                "e": "outboundAccountPosition", "u": 1_700_000_000_000_i64,
                "B": [{ "a": "BTC", "f": "1", "l": "0" }, { "a": "USDT", "f": "1", "l": "0" }],
            }),
        );
        let mut buf = Vec::new();
        assert!(!convert_margin_user_data_events_with(
            &frame,
            &mut buf,
            &mut handler
        ));
        assert!(buf.is_empty(), "unmapped subscriptionId frame is dropped");
    }

    #[test]
    fn isolated_outbound_position_missing_side_dropped() {
        // Only the base side present → no partial InstrumentBalanceUpdate (would fabricate a zero
        // quote, silently mis-reporting free/locked). Dropped with a warn instead.
        let sub_map = Arc::new(Mutex::new(HashMap::from([(7_i64, btcusdt())])));
        let base_quote = Arc::new(HashMap::from([(
            btcusdt(),
            (
                AssetNameExchange::new("BTC"),
                AssetNameExchange::new("USDT"),
            ),
        )]));
        let mut handler = isolated_handler(sub_map, base_quote);

        let frame = push_with_sub(
            7,
            serde_json::json!({
                "e": "outboundAccountPosition", "u": 1_700_000_000_000_i64,
                "B": [{ "a": "BTC", "f": "0.5", "l": "0" }],
            }),
        );
        let mut buf = Vec::new();
        assert!(!convert_margin_user_data_events_with(
            &frame,
            &mut buf,
            &mut handler
        ));
        assert!(buf.is_empty(), "missing quote side → frame dropped");
    }

    #[test]
    fn isolated_execution_report_routes_by_inner_symbol_independent_of_map() {
        // Fills self-identify via inner `s` and route regardless of the subscriptionId map (which
        // is empty here) — only balance frames need the map. Confirms fills are unaffected if the
        // subscriptionId→symbol routing ever fails live.
        let sub_map = Arc::new(Mutex::new(HashMap::<i64, InstrumentNameExchange>::new()));
        let base_quote = Arc::new(HashMap::new());
        let mut handler = isolated_handler(sub_map, base_quote);

        let frame = push_with_sub(
            42,
            serde_json::json!({
                "e": "executionReport", "s": "BTCUSDT", "S": "BUY", "o": "LIMIT",
                "x": "TRADE", "X": "FILLED", "i": 1_i64, "c": "cid", "t": 5_i64,
                "l": "1", "L": "100", "z": "1", "p": "100", "q": "1", "f": "GTC",
                "n": "0", "N": "USDT", "T": 1_700_000_000_000_i64,
            }),
        );
        let mut buf = Vec::new();
        assert!(!convert_margin_user_data_events_with(
            &frame,
            &mut buf,
            &mut handler
        ));
        assert_eq!(buf.len(), 2, "the execution and its order snapshot");
        match &buf[0].kind {
            AccountEventKind::Trade(t) => assert_eq!(t.instrument.name().as_str(), "BTCUSDT"),
            other => panic!("expected Trade, got {other:?}"),
        }
        match &buf[1].kind {
            AccountEventKind::OrderSnapshot(snap) => {
                assert_eq!(snap.0.key.instrument.name().as_str(), "BTCUSDT");
            }
            other => panic!("expected OrderSnapshot, got {other:?}"),
        }
    }

    /// A margin `myTrades` venue that enforces Binance's documented 24-hour rules: a
    /// `startTime`..`endTime` span of 24 hours or more is rejected, and a query by time without
    /// `fromId` returns at most 24 hours of trades. A `fromId` query returns trades from that id
    /// onwards whatever their time, and `orderId` narrows any query to one order. Pages hold at
    /// most `limit` trades, in ascending id.
    ///
    /// Two readings here are assumptions, not observed venue behaviour. A bare `startTime` is taken
    /// to mean the 24 hours from `startTime`; the pager under test never sends one, so only the
    /// previous pager's failure depends on it. `endTime` is inclusive unless `end_exclusive`, so a
    /// test can run under either reading.
    struct MarginMyTradesVenue {
        /// `(id, time_ms, order_id)`, in ascending id and time.
        trades: Vec<(i64, i64, i64)>,
        end_exclusive: bool,
    }

    impl wiremock::Respond for MarginMyTradesVenue {
        fn respond(&self, request: &wiremock::Request) -> wiremock::ResponseTemplate {
            const DAY_MS: i64 = 24 * 60 * 60 * 1000;
            let query: std::collections::HashMap<String, String> =
                request.url.query_pairs().into_owned().collect();
            let param = |name: &str| query.get(name).map(|v| v.parse::<i64>().unwrap());
            let limit = usize::try_from(param("limit").unwrap_or(500)).unwrap();
            let order = param("orderId");

            let mut selected: Vec<&(i64, i64, i64)> = self
                .trades
                .iter()
                .filter(|(_, _, order_id)| order.is_none_or(|order| *order_id == order))
                .collect();
            if let Some(from_id) = param("fromId") {
                selected.retain(|(id, _, _)| *id >= from_id);
            } else if order.is_none() {
                let start = param("startTime").expect("a query by time sends startTime");
                let end = match param("endTime") {
                    Some(end) if end - start >= DAY_MS => {
                        return wiremock::ResponseTemplate::new(400).set_body_json(
                            serde_json::json!({"code": -1127, "msg": "More than 24 hours between startTime and endTime."}),
                        );
                    }
                    Some(end) if self.end_exclusive => end - 1,
                    Some(end) => end,
                    None => start + DAY_MS - 1,
                };
                selected.retain(|(_, time, _)| (start..=end).contains(time));
            }

            let page: Vec<serde_json::Value> = selected
                .into_iter()
                .take(limit)
                .map(|(id, time, order_id)| {
                    serde_json::json!({
                        "id": id, "symbol": "BTCUSDT", "time": time, "orderId": order_id,
                        "price": "50000", "qty": "0.01", "commission": "0",
                        "commissionAsset": "USDT", "isBuyer": true, "isMaker": false,
                        "isBestMatch": true, "isIsolated": true,
                    })
                })
                .collect();
            wiremock::ResponseTemplate::new(200).set_body_json(page)
        }
    }

    /// Run `paginate_margin_my_trades` from `from` against `venue`, returning the ids read and the
    /// query parameters of each request sent.
    async fn read_margin_trades(
        venue: MarginMyTradesVenue,
        from: MyTradesFrom,
    ) -> (Vec<i64>, Vec<std::collections::HashMap<String, String>>) {
        let server = wiremock::MockServer::start().await;
        wiremock::Mock::given(wiremock::matchers::method("GET"))
            .and(wiremock::matchers::path("/sapi/v1/margin/myTrades"))
            .respond_with(venue)
            .mount(&server)
            .await;
        let rest = Arc::new(MarginTradingRestApi::from_config(
            ConfigurationRestApi::builder()
                .api_key("key")
                .api_secret("secret")
                .base_path(server.uri())
                .build()
                .unwrap(),
        ));

        let read = paginate_margin_my_trades(
            &rest,
            &Arc::new(RateLimitTracker::new(WeightPool::Sapi)),
            &InstrumentNameExchange::new("BTCUSDT"),
            from,
            true,
            RequestKind::Query,
        )
        .await
        .unwrap();

        let requests = server
            .received_requests()
            .await
            .unwrap()
            .iter()
            .map(|request| request.url.query_pairs().into_owned().collect())
            .collect();
        (read.into_iter().map(|t| t.id.unwrap()).collect(), requests)
    }

    /// A venue whose trades each belong to an order of the same id.
    fn venue(trades: Vec<(i64, i64)>) -> MarginMyTradesVenue {
        MarginMyTradesVenue {
            trades: trades
                .into_iter()
                .map(|(id, time)| (id, time, id))
                .collect(),
            end_exclusive: false,
        }
    }

    const HOUR_MS: i64 = 60 * 60 * 1000;

    /// The only fill lies on day 3 of a five-day lookback. A bare `startTime` query sees only the
    /// first 24 hours, finds nothing and stops; the windows must reach day 3.
    #[tokio::test]
    async fn margin_trades_read_a_lone_fill_beyond_the_first_day() {
        let now = Utc::now().timestamp_millis();
        let (ids, _) = read_margin_trades(
            venue(vec![(7, now - 72 * HOUR_MS)]),
            MyTradesFrom::Time(now - 120 * HOUR_MS),
        )
        .await;

        assert_eq!(ids, vec![7]);
    }

    /// Fills in separate days, with empty days between and a short page in the first window
    /// that holds one: every fill is read once, in order.
    #[tokio::test]
    async fn margin_trades_read_fills_across_several_windows_once_each() {
        let now = Utc::now().timestamp_millis();
        let trades = vec![
            (1, now - 119 * HOUR_MS),
            (2, now - 72 * HOUR_MS),
            (3, now - HOUR_MS),
        ];
        let (ids, _) =
            read_margin_trades(venue(trades), MyTradesFrom::Time(now - 144 * HOUR_MS)).await;

        assert_eq!(ids, vec![1, 2, 3]);
    }

    /// A trade stamped exactly on the end of the first window is read once, whether the venue
    /// treats `endTime` as inclusive or exclusive.
    #[tokio::test]
    async fn margin_trades_read_a_trade_on_a_window_boundary_once() {
        for end_exclusive in [false, true] {
            let now = Utc::now().timestamp_millis();
            let from = now - 48 * HOUR_MS;
            let venue = MarginMyTradesVenue {
                end_exclusive,
                ..venue(vec![(5, from + MARGIN_MY_TRADES_WINDOW_MS)])
            };
            let (ids, _) = read_margin_trades(venue, MyTradesFrom::Time(from)).await;

            assert_eq!(ids, vec![5], "end_exclusive: {end_exclusive}");
        }
    }

    /// A window holding more than one page of fills pages on by `fromId`.
    #[tokio::test]
    async fn margin_trades_page_on_by_id_past_a_full_window() {
        let now = Utc::now().timestamp_millis();
        let count = i64::try_from(BINANCE_MAX_TRADES).unwrap() + 1;
        let trades = (1..=count)
            .map(|id| (id, now - 48 * HOUR_MS + id))
            .collect();
        let (ids, _) =
            read_margin_trades(venue(trades), MyTradesFrom::Time(now - 50 * HOUR_MS)).await;

        assert_eq!(ids, (1..=count).collect::<Vec<_>>());
    }

    /// A final window holding more than one page still pages on by `fromId`: the early stop
    /// after the final window applies only to a short page.
    #[tokio::test]
    async fn margin_trades_page_on_past_a_full_final_window() {
        let now = Utc::now().timestamp_millis();
        let count = i64::try_from(BINANCE_MAX_TRADES).unwrap() + 1;
        let trades = (1..=count).map(|id| (id, now - HOUR_MS + id)).collect();
        let (ids, requests) =
            read_margin_trades(venue(trades), MyTradesFrom::Time(now - 2 * HOUR_MS)).await;

        assert_eq!(ids, (1..=count).collect::<Vec<_>>());
        assert_eq!(requests.len(), 2);
    }

    /// A lookback with no fills costs one request per window, and every window is accepted
    /// (the venue rejects a span of 24 hours or more, which would fail the read).
    #[tokio::test]
    async fn margin_trades_quiet_lookback_sends_one_request_per_window() {
        let now = Utc::now().timestamp_millis();
        let (ids, requests) =
            read_margin_trades(venue(Vec::new()), MyTradesFrom::Time(now - 72 * HOUR_MS)).await;

        assert!(ids.is_empty());
        // Windows start at 0, 23, 46 and 69 hours into the 72-hour lookback.
        assert_eq!(requests.len(), 4);
    }

    /// An ordinary reconnect gap holding a fill is one request: the only window reaches the
    /// call's start and reads short, so there is nothing to page on to.
    #[tokio::test]
    async fn margin_trades_read_a_short_gap_in_one_request() {
        let now = Utc::now().timestamp_millis();
        let (ids, requests) = read_margin_trades(
            venue(vec![(9, now - HOUR_MS)]),
            MyTradesFrom::Time(now - 2 * HOUR_MS),
        )
        .await;

        assert_eq!(ids, vec![9]);
        assert_eq!(requests.len(), 1);
    }

    /// A walk from a time after the call's start has nothing to read and sends nothing.
    #[tokio::test]
    async fn margin_trades_from_the_future_send_no_request() {
        let now = Utc::now().timestamp_millis();
        let (ids, requests) =
            read_margin_trades(venue(Vec::new()), MyTradesFrom::Time(now + HOUR_MS)).await;

        assert!(ids.is_empty());
        assert!(requests.is_empty());
    }

    /// A walk by order pages that order's executions by `fromId`, never by time, and reads no
    /// other order's.
    #[tokio::test]
    async fn margin_trades_by_order_page_by_id_without_time_bounds() {
        let now = Utc::now().timestamp_millis();
        let count = i64::try_from(BINANCE_MAX_TRADES).unwrap() + 1;
        // Order 42's executions, interleaved with another order's.
        let trades = (1..=2 * count)
            .map(|id| {
                (
                    id,
                    now - 48 * HOUR_MS + id,
                    if id % 2 == 0 { 42 } else { 7 },
                )
            })
            .collect();
        let venue = MarginMyTradesVenue {
            trades,
            end_exclusive: false,
        };
        let (ids, requests) = read_margin_trades(venue, MyTradesFrom::Order(42)).await;

        assert_eq!(ids, (1..=count).map(|n| 2 * n).collect::<Vec<_>>());
        assert_eq!(requests.len(), 2);
        assert!(
            requests.iter().all(
                |query| query.get("orderId").map(String::as_str) == Some("42")
                    && !query.contains_key("startTime")
                    && !query.contains_key("endTime")
            )
        );
    }

    /// Run `paginate_margin_my_trades` against a venue that answers every request with
    /// `response`, returning the error it fails with.
    async fn margin_trades_error(response: wiremock::ResponseTemplate) -> UnindexedClientError {
        let server = wiremock::MockServer::start().await;
        wiremock::Mock::given(wiremock::matchers::method("GET"))
            .and(wiremock::matchers::path("/sapi/v1/margin/myTrades"))
            .respond_with(response)
            .mount(&server)
            .await;
        let rest = Arc::new(MarginTradingRestApi::from_config(
            ConfigurationRestApi::builder()
                .api_key("key")
                .api_secret("secret")
                .base_path(server.uri())
                .build()
                .unwrap(),
        ));

        paginate_margin_my_trades(
            &rest,
            &Arc::new(RateLimitTracker::new(WeightPool::Sapi)),
            &InstrumentNameExchange::new("BTCUSDT"),
            MyTradesFrom::Time(Utc::now().timestamp_millis() - HOUR_MS),
            true,
            RequestKind::Query,
        )
        .await
        .unwrap_err()
    }

    /// A venue rejection of a query is reported as such, with its code, and not as a transient
    /// connectivity failure a caller would retry forever.
    #[tokio::test]
    async fn margin_trades_report_a_venue_rejection_as_a_non_transient_request_rejection() {
        let err = margin_trades_error(wiremock::ResponseTemplate::new(400).set_body_json(
            serde_json::json!({"code": -1127, "msg": "More than 24 hours between startTime and endTime."}),
        ))
        .await;

        let UnindexedClientError::Api(ApiError::RequestRejected(msg)) = &err else {
            panic!("expected RequestRejected, got {err:?}");
        };
        assert!(msg.contains("-1127"), "code missing from {msg:?}");
        assert!(!err.is_transient());
    }

    /// A 2xx body that does not fit the SDK's model fails identically on every retry.
    #[tokio::test]
    async fn margin_trades_report_an_undecodable_response_as_internal() {
        let err = margin_trades_error(
            wiremock::ResponseTemplate::new(200)
                .set_body_json(serde_json::json!({"unexpected": 1})),
        )
        .await;

        assert!(
            matches!(err, UnindexedClientError::Internal(_)),
            "got {err:?}"
        );
        assert!(!err.is_transient());
    }

    /// A `/sapi` response reporting `x-sapi-used-ip-weight-1m` at 90% of the documented IP limit
    /// pauses later queries but not orders; under it, or unreadable, nothing pauses.
    #[tokio::test]
    async fn sapi_used_ip_weight_near_the_limit_pauses_queries() {
        for (used, pauses) in [("10799", false), ("10800", true), ("not a number", false)] {
            let server = wiremock::MockServer::start().await;
            wiremock::Mock::given(wiremock::matchers::method("GET"))
                .and(wiremock::matchers::path("/sapi/v1/margin/openOrders"))
                .respond_with(
                    wiremock::ResponseTemplate::new(200)
                        .insert_header("x-sapi-used-ip-weight-1m", used)
                        .set_body_json(serde_json::json!([])),
                )
                .mount(&server)
                .await;
            let rest = Arc::new(MarginTradingRestApi::from_config(
                ConfigurationRestApi::builder()
                    .api_key("key")
                    .api_secret("secret")
                    .base_path(server.uri())
                    .build()
                    .unwrap(),
            ));
            let tracker = Arc::new(RateLimitTracker::new(WeightPool::Sapi));

            fetch_margin_all_open_orders(rest, Arc::clone(&tracker), false)
                .await
                .unwrap();

            let query_waits = tokio::time::timeout(
                Duration::from_millis(50),
                tracker.wait_if_blocked(RequestKind::Query),
            )
            .await
            .is_err();
            assert_eq!(query_waits, pauses, "used IP weight {used}");
            tokio::time::timeout(
                Duration::from_millis(50),
                tracker.wait_if_blocked(RequestKind::Order),
            )
            .await
            .expect("orders never wait on the pause");
        }
    }

    /// Margin fill recovery does not wait for the pause near the weight limit either.
    #[tokio::test]
    async fn margin_fill_recovery_does_not_wait_for_the_weight_pause() {
        let server = wiremock::MockServer::start().await;
        wiremock::Mock::given(wiremock::matchers::method("GET"))
            .and(wiremock::matchers::path("/sapi/v1/margin/myTrades"))
            // One trade, so recovery also looks its order up: both reads run under the pause.
            .respond_with(
                wiremock::ResponseTemplate::new(200).set_body_json(serde_json::json!([{
                    "symbol": "BTCUSDT",
                    "id": 1,
                    "orderId": 7,
                    "price": "100",
                    "qty": "1",
                    "commission": "0",
                    "commissionAsset": "USDT",
                    "time": Utc::now().timestamp_millis(),
                    "isBuyer": true,
                    "isMaker": false,
                }])),
            )
            .expect(2)
            .mount(&server)
            .await;
        let rest = Arc::new(MarginTradingRestApi::from_config(
            ConfigurationRestApi::builder()
                .api_key("key")
                .api_secret("secret")
                .base_path(server.uri())
                .build()
                .unwrap(),
        ));
        let tracker = Arc::new(RateLimitTracker::new(WeightPool::Sapi));
        tracker.throttle(Duration::from_secs(60));
        let (tx, _rx) = tokio::sync::mpsc::unbounded_channel();
        let mut unrecovered = UnrecoveredFills::default();
        unrecovered.open(
            &[InstrumentNameExchange::new("BTCUSDT")],
            Utc::now() - chrono::Duration::minutes(1),
            Utc::now(),
        );

        // Far under the 60 s pause set above, and ample for two local round trips.
        tokio::time::timeout(
            Duration::from_millis(800),
            recover_margin_fills(
                &rest,
                &tracker,
                &mut unrecovered,
                &tx,
                &new_dedup_cache(),
                false,
            ),
        )
        .await
        .expect("recovery does not wait for the pause");
    }

    /// A walk over a span reads only the trades inside it, stopping at the first page by id that
    /// reaches past its end.
    #[tokio::test]
    async fn margin_trades_read_over_a_span_stop_at_its_end() {
        let now = Utc::now().timestamp_millis();
        let trades = vec![
            (1, now - 50 * HOUR_MS),
            (2, now - 49 * HOUR_MS),
            (3, now - 10 * HOUR_MS),
        ];
        let (ids, _) = read_margin_trades(
            venue(trades),
            MyTradesFrom::Span {
                start: now - 60 * HOUR_MS,
                end: now - 20 * HOUR_MS,
            },
        )
        .await;

        assert_eq!(ids, vec![1, 2]);
    }

    /// A margin recovery that fails keeps its gap, closed at its own start; the retry reads only
    /// that span, forwards only the fill inside it, and clears it.
    #[tokio::test]
    async fn a_failed_margin_recovery_keeps_its_closed_gap_for_the_retry() {
        let disconnect = Utc::now() - chrono::Duration::minutes(10);
        let reconnect = Utc::now() - chrono::Duration::minutes(5);
        let server = wiremock::MockServer::start().await;
        wiremock::Mock::given(wiremock::matchers::method("GET"))
            .and(wiremock::matchers::path("/sapi/v1/margin/myTrades"))
            .respond_with(
                wiremock::ResponseTemplate::new(400)
                    .set_body_json(serde_json::json!({"code": -1100, "msg": "rejected"})),
            )
            .up_to_n_times(1)
            .with_priority(1)
            .mount(&server)
            .await;
        wiremock::Mock::given(wiremock::matchers::method("GET"))
            .and(wiremock::matchers::path("/sapi/v1/margin/myTrades"))
            .respond_with(venue(vec![
                (
                    1,
                    (disconnect + chrono::Duration::minutes(1)).timestamp_millis(),
                ),
                (
                    2,
                    (Utc::now() - chrono::Duration::minutes(1)).timestamp_millis(),
                ),
            ]))
            .mount(&server)
            .await;
        let rest = Arc::new(MarginTradingRestApi::from_config(
            ConfigurationRestApi::builder()
                .api_key("key")
                .api_secret("secret")
                .base_path(server.uri())
                .build()
                .unwrap(),
        ));
        let tracker = Arc::new(RateLimitTracker::new(WeightPool::Sapi));
        let (tx, mut rx) = tokio::sync::mpsc::unbounded_channel();
        let dedup = new_dedup_cache();
        let mut unrecovered = UnrecoveredFills::default();
        unrecovered.open(
            &[InstrumentNameExchange::new("BTCUSDT")],
            disconnect,
            reconnect,
        );

        recover_margin_fills(&rest, &tracker, &mut unrecovered, &tx, &dedup, false).await;
        assert!(!unrecovered.is_empty(), "the failed gap is kept");
        assert!(rx.try_recv().is_err(), "nothing forwarded");

        unrecovered.make_due();
        recover_margin_fills(&rest, &tracker, &mut unrecovered, &tx, &dedup, false).await;
        assert!(unrecovered.is_empty(), "the retry recovered it");
        let forwarded: Vec<_> = std::iter::from_fn(|| rx.try_recv().ok()).collect();
        assert_eq!(
            forwarded.len(),
            1,
            "only the fill inside the gap: {forwarded:?}"
        );
    }

    /// When margin recovery times out, a gap whose read started has failed and waits for its retry, but
    /// one never started (eight are read at a time) is not charged and stays due.
    #[tokio::test]
    async fn a_timed_out_margin_recovery_charges_only_the_gaps_it_started() {
        let server = wiremock::MockServer::start().await;
        wiremock::Mock::given(wiremock::matchers::method("GET"))
            .and(wiremock::matchers::path("/sapi/v1/margin/myTrades"))
            .respond_with(
                wiremock::ResponseTemplate::new(200)
                    .set_body_json(serde_json::json!([]))
                    .set_delay(Duration::from_secs(3_600)),
            )
            .mount(&server)
            .await;
        let rest = Arc::new(MarginTradingRestApi::from_config(
            ConfigurationRestApi::builder()
                .api_key("key")
                .api_secret("secret")
                .base_path(server.uri())
                .timeout(3_600_000_u64)
                .retries(0_u32)
                .build()
                .unwrap(),
        ));
        let instruments: Vec<_> = (0..9)
            .map(|i| InstrumentNameExchange::new(format!("SYM{i}USDT")))
            .collect();
        let mut unrecovered = UnrecoveredFills::default();
        unrecovered.open(
            &instruments,
            Utc::now() - chrono::Duration::minutes(10),
            Utc::now(),
        );
        let (tx, _rx) = tokio::sync::mpsc::unbounded_channel();
        // Paused: once every read is waiting, the clock jumps to the recovery timeout.
        tokio::time::pause();

        recover_margin_fills(
            &rest,
            &Arc::new(RateLimitTracker::new(WeightPool::Sapi)),
            &mut unrecovered,
            &tx,
            &new_dedup_cache(),
            false,
        )
        .await;

        let now = tokio::time::Instant::now();
        assert_eq!(
            unrecovered.due(now).len(),
            1,
            "the gap never started stays due"
        );
        assert_eq!(
            unrecovered
                .due(now + Duration::from_secs(crate::client::binance::shared::GAP_RETRY_BASE_SECS))
                .len(),
            9,
            "the eight started wait for their first retry"
        );
    }
}