Skip to content

(fix) refresh per-pair trading rules on dynamic pair registration - #212

Open
fengtality wants to merge 2 commits into
fix/207-throttler-pair-limitsfrom
fix/208-per-pair-trading-rules
Open

(fix) refresh per-pair trading rules on dynamic pair registration#212
fengtality wants to merge 2 commits into
fix/207-throttler-pair-limitsfrom
fix/208-per-pair-trading-rules

Conversation

@fengtality

Copy link
Copy Markdown
Contributor

Fixes #208. Stacked on the #207 fix (base branch fix/207-throttler-pair-limits) — retarget to main after that merges.

Connectors that build trading rules per pair (XRPL queries the ledger for each pair in _trading_pairs) initialize with an empty rules dict, and dynamically registered pairs never get a rule. Executors then die at startup with KeyError in validate_sufficient_balance — but register as RUNNING, order_id: None: silent zombies that report 201 Created and never place an order (reproduced 3/3 on xrpl/USDC-XRP). Direct trades via place_trade 503'd forever ("rules not yet loaded") or 400'd "pair not supported".

Fix

Extends sync_pair_derived_state to refresh trading rules when the registered pair has no rule yet, gated on is_trading_required: data connectors never place orders, and the refresh is a real (possibly on-chain, retry-with-backoff) fetch that would add latency and spurious warnings to every order-book bootstrap. No-op for CEX connectors whose exchange-info fetch already returns rules for all pairs. Also wires the sync into place_trade before its rules checks.

Verified safe: XRPL's _update_trading_rules fetches before clear(), and the clear→repopulate section has no awaits — a failed or concurrent refresh cannot leave a torn/empty rules dict.

Tests

+4: rules built for registered pair; no re-fetch when rule exists; data connector skips rules but still syncs throttler; fetch failure swallowed with registration intact. Suite: 94 passed, same 7 pre-existing failures.

🤖 Generated with Claude Code

@greptile-apps

greptile-apps Bot commented Aug 7, 2026

Copy link
Copy Markdown

Greptile Summary

The PR extends dynamic pair registration to refresh missing per-pair trading rules on order-producing paths while keeping market-data and leverage paths limited to throttler synchronization.

  • Direct trades now synchronize pair-derived state before validating trading rules and return a retryable response when rules remain unavailable.
  • Trading-rule refreshes are restricted to trading connectors and skipped when the requested pair already has a rule.
  • Tests cover successful refreshes, idempotency, data-only paths, refresh failures, and invalid-pair rejection.

Confidence Score: 5/5

The PR appears safe to merge.

No blocking failure remains.

Important Files Changed

Filename Overview
services/unified_connector_service.py Adds optional, trading-only refresh of missing per-pair rules while preserving throttler synchronization across all registration paths.
services/accounts_service.py Synchronizes dynamic pair state before direct-trade rule validation and reports missing refreshed rules as retryable failures.
services/perpetual_trading_service.py Explicitly disables trading-rule refresh for leverage updates while retaining pair-specific throttler synchronization.
test/test_sync_pair_derived_state.py Expands coverage for per-pair rule refreshes, idempotency, data-only behavior, fetch failures, and invalid-pair handling.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A[Dynamic pair request] --> B[sync_pair_derived_state]
    B --> C[Validate and register pair]
    C --> D[Refresh pair-derived throttler limits]
    D --> E{Order-producing path?}
    E -- No --> F[Continue without trading-rule fetch]
    E -- Yes --> G{Trading connector and rule missing?}
    G -- No --> H[Continue]
    G -- Yes --> I[Refresh trading rules]
    I --> J{Requested rule available?}
    J -- Yes --> H
    J -- No --> K[Direct trade returns retryable 503]
Loading

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

fengtality added a commit that referenced this pull request Aug 7, 2026
…uard property eval

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

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

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
fengtality and others added 2 commits August 6, 2026 23:48
Connectors that build trading rules per pair (XRPL queries the ledger for
each pair in _trading_pairs) initialize with an empty rules dict, and
dynamically registered pairs never got a rule: executors then die at
startup with KeyError in validate_sufficient_balance but register as
RUNNING zombies — 201 Created, order_id None, no order ever reaches the
ledger. Reproduced 3/3 on xrpl / USDC-XRP. Direct trades via place_trade
503'd forever on the empty-rules check or 400'd "pair not supported".

Extends sync_pair_derived_state (from #207's fix) to refresh trading
rules when the registered pair has no rule yet, gated on
is_trading_required — data connectors never place orders, and the refresh
is a real (possibly on-chain) fetch that would add latency and warnings
to every order-book bootstrap. Wires the sync into place_trade before its
rules checks.

flake8 skipped: violations in accounts_service.py are pre-existing on
main; the change there is the 6-line place_trade sync.

Fixes #208

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…rade errors

- Add refresh_rules param (default True) to sync_pair_derived_state and
  pass False on every market-data path (_add_trading_pair_to_tracker,
  ensure_data_connector_started) and set_leverage: the is_trading_required
  gate alone was ineffective on the main data path because
  get_best_connector_for_market PREFERS trading connectors, so order-book
  bootstraps paid the possibly-on-chain rules fetch whenever credentials
  existed. Rules now refresh only on paths that lead to order placement
  (add_market registration, place_trade).
- place_trade: surface pair-validation failures as 400 with the validation
  message; replace the misleading 400 "pair not supported / available
  pairs" for a validated pair whose rule is missing with a retryable 503 —
  the pair IS supported, the refresh failed or is pending.
- Tests: order-book path skips the rules fetch but still syncs the
  throttler; a typo'd pair cannot poison the rules refresh for valid pairs
  (XRPL-faithful fake that raises before any rule lands).

flake8 skipped: accounts_service.py violations are pre-existing on main.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@fengtality
fengtality force-pushed the fix/208-per-pair-trading-rules branch from 2db2cee to 698e01e Compare August 7, 2026 06:50
@fengtality

Copy link
Copy Markdown
Contributor Author

Adversarial review — the reviewer empirically reproduced a blocker against the real helper, fixed in the latest commits:

  1. BLOCKER: one bad pair permanently poisoned the connector, and place_trade wired raw user input straight into it. XRPL's rules fetch iterates ALL registered pairs and raises on the first unknown one before returning rules for any — so a single typo'd pair (XRP-USD) meant no pair could ever refresh again, while the 30-min polling loop turned into a ledger-hammering retry storm (0.5s loop × 3-attempt/6.5s backoff per cycle). Fixed by the validation gate now at the base of the stack ((fix) sync throttler pair-templated rate limits on dynamic pair registration #211): unknown pairs are rejected 400 and never registered. New XRPL-faithful test proves a typo'd pair cannot poison refresh for valid pairs.
  2. The is_trading_required gate did not deliver its promise on the main data pathget_best_connector_for_market PREFERS trading connectors, so order-book bootstraps paid the on-chain rules fetch whenever credentials existed. Fixed: new refresh_rules parameter; every market-data path (_add_trading_pair_to_tracker, ensure_data_connector_started) and set_leverage pass False — rules refresh now runs only on paths that lead to order placement (add_market registration, place_trade). Test added.
  3. place_trade errors were still misleading after a failed refresh — a supported pair got 400 "not supported / available pairs". Fixed: validation failures → 400 with the validation message; validated pair with missing rule → retryable 503.

Verified clean by the reviewer: no torn state in XRPL's clear-and-rebuild (no awaits between clear and repopulate), concurrent registrations converge (300 randomized interleavings), symbol-map re-init benign, Gateway safely skipped.

@rapcmia rapcmia left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

  • Setup with condor and added XRPL ok
  • git switch --detach origin/fix/207-throttler-pair-limits
  • git switch --detach origin/fix/208-per-pair-trading-rules
  • Registered RLUSD-XRP first, then confirmed its per-pair trading rule became available using /market-data/trading-pair/add
  • Submitted a minimal XRPL limit order successfully, the order appeared as an active XRPL offer using /trading/orders
  • Rejected unsupported XRP-USDS with HTTP 400.
  • Confirmed the valid RLUSD-XRP rule remained available after rejecting the invalid pair.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants