From 3e1c415b8d3ee81e91c5d987e27eb7f13cb52732 Mon Sep 17 00:00:00 2001 From: jgrusewski Date: Sun, 22 Feb 2026 20:01:12 +0100 Subject: [PATCH] docs: add codebase de-duplication implementation plan 18 tasks across 4 phases: - Phase 1: OrderType, ConfigError, TLS types (zero risk) - Phase 2: ErrorCategory, ModelType, re-export cleanup (low risk) - Phase 3: Dead code audit, config warnings, Adam move (low risk) - Phase 4: CircuitBreaker trait + inline CB replacement (medium risk) Scoped out: ErrorSeverity (semantically different variants), RetryStrategy (complex merge), trading_engine/risk CBs (domain-specific) Co-Authored-By: Claude Opus 4.6 --- ...2-codebase-deduplication-implementation.md | 947 ++++++++++++++++++ 1 file changed, 947 insertions(+) create mode 100644 docs/plans/2026-02-22-codebase-deduplication-implementation.md diff --git a/docs/plans/2026-02-22-codebase-deduplication-implementation.md b/docs/plans/2026-02-22-codebase-deduplication-implementation.md new file mode 100644 index 000000000..5e4a182ba --- /dev/null +++ b/docs/plans/2026-02-22-codebase-deduplication-implementation.md @@ -0,0 +1,947 @@ +# Codebase De-duplication Implementation Plan + +> **For Claude:** REQUIRED SUB-SKILL: Use superpowers:executing-plans to implement this plan task-by-task. + +**Goal:** Eliminate duplicated types, dead code, and code smells across the 37-crate Rust workspace. + +**Architecture:** Bottom-up consolidation in 4 phases. Each phase compiles and tests independently. Canonical types live in `common/` or `ml/`; consumers re-export or import. Full `cargo check --workspace` and `cargo test` between phases. + +**Tech Stack:** Rust, Cargo workspace, SQLX_OFFLINE=true (no PostgreSQL needed) + +**Build/test commands:** +- Compile: `SQLX_OFFLINE=true cargo check --workspace` +- Test specific crate: `SQLX_OFFLINE=true cargo test -p --lib` +- Test all: `SQLX_OFFLINE=true cargo test --workspace --lib` + +--- + +## Phase 1: Zero-Risk Dedup + +### Task 1: Consolidate OrderType in ml/ + +Four identical `OrderType` enums (Market/LimitMaker/IoC) exist in ml/. Consolidate into one shared definition. + +**Files:** +- Create: `ml/src/common/action.rs` +- Modify: `ml/src/common/mod.rs` +- Modify: `ml/src/dqn/action_space.rs` +- Modify: `ml/src/ppo/continuous_transaction_costs.rs` +- Modify: `ml/src/ppo/factored_action.rs` +- Modify: `ml/src/ppo/transaction_costs.rs` + +**Step 1: Create the unified OrderType module** + +Create `ml/src/common/action.rs` with the unified type. Include ALL methods from all 4 copies: + +```rust +use crate::error::MLError; +use serde::{Deserialize, Serialize}; + +/// Order type for execution strategy. +/// +/// Three execution modes with calibrated transaction costs (Wave 2.5). +#[derive(Debug, Clone, Copy, PartialEq, Eq, Hash, Serialize, Deserialize)] +pub enum OrderType { + /// Immediate execution, taker fee (15 bps) + Market = 0, + /// Passive order, maker rebate (5 bps) + LimitMaker = 1, + /// Immediate-or-cancel (10 bps) + IoC = 2, +} + +impl OrderType { + /// Transaction cost as decimal (e.g., 0.0015 for 15 bps). + pub fn transaction_cost(&self) -> f64 { + match self { + OrderType::Market => 0.0015, + OrderType::LimitMaker => 0.0005, + OrderType::IoC => 0.0010, + } + } + + /// Transaction cost in basis points (e.g., 15.0 for Market). + pub fn cost_bps(&self) -> f32 { + match self { + OrderType::Market => 15.0, + OrderType::LimitMaker => 5.0, + OrderType::IoC => 10.0, + } + } + + /// Cost as decimal from bps (convenience wrapper). + pub fn cost_decimal(&self) -> f64 { + (self.cost_bps() as f64) / 10000.0 + } + + /// Convert from index (0-2). + pub fn from_index(idx: usize) -> Result { + match idx { + 0 => Ok(OrderType::Market), + 1 => Ok(OrderType::LimitMaker), + 2 => Ok(OrderType::IoC), + _ => Err(MLError::InvalidInput(format!( + "Invalid order type index: {}", + idx + ))), + } + } +} + +impl std::fmt::Display for OrderType { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + match self { + OrderType::Market => write!(f, "Market"), + OrderType::LimitMaker => write!(f, "LimitMaker"), + OrderType::IoC => write!(f, "IoC"), + } + } +} +``` + +**Step 2: Export from ml/src/common/mod.rs** + +Add `pub mod action;` to `ml/src/common/mod.rs` (it currently has `circuit_breaker`, `config`, `metrics`, `performance`). + +**Step 3: Replace in dqn/action_space.rs** + +Remove the `OrderType` enum definition and its `impl` block (lines 44-73). Add at the top: +```rust +pub use crate::common::action::OrderType; +``` + +Keep all other code in the file (ExposureLevel, Urgency, FactoredAction structs use OrderType — they'll pick it up via the re-export). + +**Step 4: Replace in ppo/continuous_transaction_costs.rs** + +Remove the `OrderType` enum definition and its `impl` block (lines 81-108). Add at the top: +```rust +pub use crate::common::action::OrderType; +``` + +**Step 5: Replace in ppo/factored_action.rs** + +Remove the `OrderType` enum definition and its `impl` block (lines 56-85). Add at the top: +```rust +pub use crate::common::action::OrderType; +``` + +**Step 6: Replace in ppo/transaction_costs.rs** + +Remove the `OrderType` enum definition and its `impl` block (lines 17-40). Add at the top: +```rust +pub use crate::common::action::OrderType; +``` + +**Step 7: Verify** + +Run: `SQLX_OFFLINE=true cargo check -p ml` +Run: `SQLX_OFFLINE=true cargo test -p ml --lib` +Expected: PASS with 0 warnings about OrderType + +**Step 8: Commit** + +```bash +git add ml/src/common/action.rs ml/src/common/mod.rs ml/src/dqn/action_space.rs ml/src/ppo/continuous_transaction_costs.rs ml/src/ppo/factored_action.rs ml/src/ppo/transaction_costs.rs +git commit -m "refactor(ml): consolidate OrderType into common/action.rs" +``` + +--- + +### Task 2: Consolidate ConfigError + +api_gateway defines its own `ConfigError` (8 variants) that overlaps with `config/src/error.rs` (5 variants). The api_gateway version has additional variants (Redis, Serialization, Unauthorized) — these are gateway-specific. Rename api_gateway's to `GatewayConfigError` to avoid name collision and confusion. + +**Files:** +- Modify: `services/api_gateway/src/error.rs` +- Check: all files in `services/api_gateway/src/` that reference `ConfigError` + +**Step 1: Audit usages in api_gateway** + +Search for `ConfigError` in all api_gateway source files to understand how it's used. The type and its `ConfigResult` alias need updating everywhere they appear. + +**Step 2: Rename ConfigError to GatewayConfigError** + +In `services/api_gateway/src/error.rs`, rename: +- `ConfigError` → `GatewayConfigError` +- `ConfigResult` → `GatewayConfigResult` + +Keep all 8 variants — they are gateway-specific and belong here. + +**Step 3: Update all references in api_gateway** + +Find and replace `ConfigError` → `GatewayConfigError` and `ConfigResult` → `GatewayConfigResult` throughout `services/api_gateway/src/`. + +**Step 4: Verify** + +Run: `SQLX_OFFLINE=true cargo check -p api_gateway` +Run: `SQLX_OFFLINE=true cargo test -p api_gateway --lib` + +**Step 5: Commit** + +```bash +git add services/api_gateway/ +git commit -m "refactor(api_gateway): rename ConfigError to GatewayConfigError to avoid collision with config crate" +``` + +--- + +### Task 3: Consolidate TLS shared types + +The 4 service TLS configs share identical types: `TlsProtocolVersion` (enum), `UserRole` (enum with 6 RBAC roles), `ClientIdentity` (struct with 4 fields). Extract these shared types to `common/src/tls.rs`. + +**Important context:** The TLS *validation logic* differs between services (async vs sync, delegated vs monolithic), so we only extract the shared type definitions — NOT the full TlsConfig implementations. Each service keeps its own TlsConfig struct. + +**Files:** +- Create: `common/src/tls.rs` +- Modify: `common/src/lib.rs` +- Modify: `services/trading_service/src/tls_config.rs` +- Modify: `services/ml_training_service/src/tls_config.rs` +- Modify: `services/backtesting_service/src/tls_config.rs` +- Modify: `services/api_gateway/src/auth/mtls/tls_config.rs` + +**Step 1: Create common/src/tls.rs** + +```rust +use serde::{Deserialize, Serialize}; + +/// TLS protocol version selection. +#[derive(Debug, Clone, Copy, PartialEq, Eq, Serialize, Deserialize)] +pub enum TlsProtocolVersion { + Tls12, + Tls13, +} + +/// RBAC roles extracted from client certificates. +#[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)] +pub enum UserRole { + Admin, + Trader, + Analyst, + Auditor, + System, + ReadOnly, +} + +/// Client identity extracted from mTLS certificate. +#[derive(Debug, Clone, Serialize, Deserialize)] +pub struct ClientIdentity { + pub common_name: String, + pub organization: String, + pub role: UserRole, + pub certificate_serial: String, +} +``` + +**Step 2: Add `pub mod tls;` to common/src/lib.rs** + +**Step 3: In each service TLS file, remove the local definitions and import from common** + +Replace local `TlsProtocolVersion`, `UserRole`, `ClientIdentity` definitions with: +```rust +pub use common::tls::{ClientIdentity, TlsProtocolVersion, UserRole}; +``` + +Keep all other service-specific logic (TlsConfig struct, validation methods, TlsInterceptor). + +**Step 4: Verify** + +Run: `SQLX_OFFLINE=true cargo check --workspace` +Expected: PASS — services already depend on common + +**Step 5: Commit** + +```bash +git add common/src/tls.rs common/src/lib.rs services/trading_service/src/tls_config.rs services/ml_training_service/src/tls_config.rs services/backtesting_service/src/tls_config.rs services/api_gateway/src/auth/mtls/tls_config.rs +git commit -m "refactor: extract shared TLS types (TlsProtocolVersion, UserRole, ClientIdentity) to common/" +``` + +--- + +### Task 4: Phase 1 verification + +**Step 1: Full workspace check** + +Run: `SQLX_OFFLINE=true cargo check --workspace` +Expected: PASS with 0 errors + +**Step 2: Run affected crate tests** + +Run: `SQLX_OFFLINE=true cargo test -p ml --lib` +Run: `SQLX_OFFLINE=true cargo test -p api_gateway --lib` +Run: `SQLX_OFFLINE=true cargo test -p common --lib` + +**Step 3: Clippy check** + +Run: `SQLX_OFFLINE=true cargo clippy -p ml -p api_gateway -p common -- -D warnings` + +--- + +## Phase 2: Low-Risk Dedup + +### Task 5: Consolidate ErrorCategory + +`ml/src/lib.rs` has a stub `ErrorCategory` with only 1 variant (`System`). The canonical version in `common/src/error.rs` has 24 variants. Replace the ml stub with an import. + +**Files:** +- Modify: `ml/src/lib.rs` (remove stub enum ~line 465-468) +- Check: `ml/src/lib.rs` for all usages of `ErrorCategory` + +**Step 1: Remove the stub enum from ml/src/lib.rs** + +Find and remove: +```rust +#[derive(Debug, Clone, Copy, PartialEq, Eq, serde::Serialize, serde::Deserialize)] +pub enum ErrorCategory { + System, +} +``` + +**Step 2: Add import if needed** + +If `ErrorCategory` is used elsewhere in ml/, add: +```rust +pub use common::error::ErrorCategory; +``` + +If nothing in ml/ actually uses `ErrorCategory` (it may be dead since it was a stub), just delete it without re-exporting. + +**Step 3: Check data/src/providers/common.rs** + +Read the file to see if its `ErrorCategory` is used internally. If it maps cleanly to common's `ErrorCategory` variants, replace it. If it has genuinely different semantics (provider-specific categories like `DataFormat`), keep it but rename to `ProviderErrorCategory` to avoid confusion. + +**Step 4: Verify** + +Run: `SQLX_OFFLINE=true cargo check -p ml -p data` +Run: `SQLX_OFFLINE=true cargo test -p ml --lib` + +**Step 5: Commit** + +```bash +git add ml/src/lib.rs +git commit -m "refactor(ml): remove stub ErrorCategory, use common::error::ErrorCategory" +``` + +--- + +### Task 6: Consolidate ModelType + +Four definitions with incompatible variant counts (15, 7, 4, 2). Strategy: keep `ml/src/lib.rs` as canonical (15 variants), make others import from ml. + +**Important:** `model_loader/Cargo.toml` does NOT depend on ml crate. Adding that dependency might cause issues. Instead: move ModelType to common/ where both ml and model_loader can import it. + +**Files:** +- Create: `common/src/model_types.rs` +- Modify: `common/src/lib.rs` +- Modify: `common/Cargo.toml` (if needed) +- Modify: `ml/src/lib.rs` (~line 2179-2211) +- Modify: `model_loader/src/lib.rs` (~line 22-37) +- Modify: `ml/src/hyperopt/campaign.rs` (~line 13-16) +- Modify: `services/ml_training_service/src/job_spawner.rs` (~line 56-82) + +**Step 1: Create common/src/model_types.rs** + +Move the canonical ModelType enum (15 variants from ml/src/lib.rs) to common. Include the `as_str()` method from model_loader and the `to_db_string()` method from ml_training_service: + +```rust +use serde::{Deserialize, Serialize}; + +/// Canonical model type enum for the ML pipeline. +#[derive(Debug, Clone, Copy, Serialize, Deserialize, PartialEq, Eq, Hash)] +pub enum ModelType { + CompactDQN, + DistilledMicroNet, + DQN, + RainbowDQN, + MAMBA, + TFT, + TGGN, + LNN, + TLOB, + PPO, + Transformer, + /// Alias for MAMBA + Mamba, + /// Alias for LNN + LiquidNet, + /// Alias for TGGN + TGNN, + Ensemble, +} + +impl ModelType { + /// S3/storage prefix string. + pub const fn as_str(&self) -> &'static str { + match self { + ModelType::CompactDQN => "compact_dqn", + ModelType::DistilledMicroNet => "distilled_micro_net", + ModelType::DQN => "dqn", + ModelType::RainbowDQN => "rainbow_dqn", + ModelType::MAMBA | ModelType::Mamba => "mamba", + ModelType::TFT => "tft", + ModelType::TGGN | ModelType::TGNN => "tggn", + ModelType::LNN | ModelType::LiquidNet => "liquid", + ModelType::TLOB => "tlob", + ModelType::PPO => "ppo", + ModelType::Transformer => "transformer", + ModelType::Ensemble => "ensemble", + } + } + + /// Database representation string. + pub fn to_db_string(&self) -> &'static str { + match self { + ModelType::DQN | ModelType::CompactDQN | ModelType::RainbowDQN | ModelType::DistilledMicroNet => "DQN", + ModelType::PPO => "PPO", + ModelType::MAMBA | ModelType::Mamba => "MAMBA-2", + ModelType::TFT => "TFT", + ModelType::TGGN | ModelType::TGNN => "TGGN", + ModelType::LNN | ModelType::LiquidNet => "LNN", + ModelType::TLOB => "TLOB", + ModelType::Transformer => "Transformer", + ModelType::Ensemble => "Ensemble", + } + } +} + +impl std::fmt::Display for ModelType { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + write!(f, "{}", self.as_str()) + } +} +``` + +**Step 2: Add `pub mod model_types;` to common/src/lib.rs** + +Also add to the re-exports: `pub use model_types::ModelType;` + +**Step 3: In ml/src/lib.rs, replace ModelType with re-export** + +Remove the enum definition (~lines 2179-2211). Add: +```rust +pub use common::model_types::ModelType; +``` + +**Step 4: In model_loader/src/lib.rs, replace ModelType with import** + +Add `common` as dependency in `model_loader/Cargo.toml`: +```toml +common = { path = "../common" } +``` + +Remove the local ModelType enum (lines 22-37) and its `as_str()` impl (lines 39-52). Add: +```rust +pub use common::model_types::ModelType; +``` + +**Step 5: In ml/src/hyperopt/campaign.rs, replace 2-variant ModelType** + +Remove the local enum (lines 13-16) and Display impl (lines 18-25). Add: +```rust +use crate::ModelType; +``` + +The file only uses DQN and PPO variants, which exist in the canonical enum. + +**Step 6: In services/ml_training_service/src/job_spawner.rs, replace 4-variant ModelType** + +Remove the local enum (lines 56-82). Add: +```rust +use common::model_types::ModelType; +``` + +Update any match arms that used `MAMBA2` to use `ModelType::MAMBA` instead. + +**Step 7: Verify** + +Run: `SQLX_OFFLINE=true cargo check --workspace` +Run: `SQLX_OFFLINE=true cargo test -p ml --lib` +Run: `SQLX_OFFLINE=true cargo test -p model_loader --lib` +Run: `SQLX_OFFLINE=true cargo test -p ml_training_service --lib` + +**Step 8: Commit** + +```bash +git add common/src/model_types.rs common/src/lib.rs common/Cargo.toml ml/src/lib.rs model_loader/src/lib.rs model_loader/Cargo.toml ml/src/hyperopt/campaign.rs services/ml_training_service/src/job_spawner.rs +git commit -m "refactor: unify ModelType into common/model_types.rs (was 4 separate definitions)" +``` + +--- + +### Task 7: Clean up confusing re-exports in common/lib.rs + +Remove aliased re-exports like `BarEventFromMarketData`, `MarketDataEventFromMarketData` from `common/src/lib.rs`. + +**Files:** +- Modify: `common/src/lib.rs` (lines ~64-69) +- Check: all workspace crates for usage of the aliased names + +**Step 1: Search for usage of aliased names** + +Search the entire workspace for `BarEventFromMarketData` and `MarketDataEventFromMarketData`. If no code outside common/ uses these aliases, delete them. If code does use them, update those references to use the canonical names. + +**Step 2: Remove aliases from common/src/lib.rs** + +Remove lines like: +```rust +pub use market_data::{ + BarEvent as BarEventFromMarketData, + MarketDataEvent as MarketDataEventFromMarketData, + ... +}; +``` + +Keep the canonical re-exports (without aliases) if the types need to be in the public API. + +**Step 3: Verify** + +Run: `SQLX_OFFLINE=true cargo check --workspace` + +**Step 4: Commit** + +```bash +git add common/src/lib.rs +git commit -m "refactor(common): remove confusing aliased re-exports" +``` + +--- + +### Task 8: Phase 2 verification + +**Step 1: Full workspace check** + +Run: `SQLX_OFFLINE=true cargo check --workspace` + +**Step 2: Run all lib tests** + +Run: `SQLX_OFFLINE=true cargo test --workspace --lib 2>&1 | tail -30` + +**Step 3: Clippy** + +Run: `SQLX_OFFLINE=true cargo clippy --workspace -- -D warnings 2>&1 | head -50` + +--- + +## Phase 3: Dead Code & Smell Removal + +### Task 9: Remove dead code in adaptive-strategy/src/risk/kelly_position_sizer.rs + +This file has ~24 `#[allow(dead_code)]` markers on structs that are defined but never used: ConcentrationMonitor, CorrelationMatrix, VolatilityOptimizer, VolatilityEstimate, VolatilityModelType, VolatilityModel, CalibrationRecord, DrawdownTracker, PerformanceTracker, DailyReturn, KellyPerformanceMetrics, AccuracyTracker, and functions calculate_base_kelly, calculate_variance, calculate_win_loss_stats. + +**Files:** +- Modify: `adaptive-strategy/src/risk/kelly_position_sizer.rs` + +**Step 1: Verify these items are truly unused** + +Search the entire workspace for each struct/function name. If they're only referenced in the file where they're defined (and only with `#[allow(dead_code)]`), they're dead. + +**Step 2: Delete all confirmed dead code** + +Remove each dead struct/enum/function and its associated `#[allow(dead_code)]` marker. Be careful to preserve any structs whose fields ARE used even if the struct itself has `#[allow(dead_code)]` on some fields (check the exploration notes — `VolatilityEstimate.current` and `DrawdownTracker.recovery_factor` may be used). + +**Step 3: Verify** + +Run: `SQLX_OFFLINE=true cargo check -p adaptive-strategy` +Run: `SQLX_OFFLINE=true cargo test -p adaptive-strategy --lib` + +**Step 4: Commit** + +```bash +git add adaptive-strategy/src/risk/kelly_position_sizer.rs +git commit -m "refactor(adaptive-strategy): remove dead code from kelly_position_sizer" +``` + +--- + +### Task 10: Remove dead code in adaptive-strategy execution, risk, and models + +Continue dead code cleanup in the remaining adaptive-strategy files. + +**Files:** +- Modify: `adaptive-strategy/src/execution/mod.rs` (~12 dead items: SlippageTracker, routing_rules, venue_performance, ParticipationRateAlgorithm fields, VolumeProfile, VolumeTracker) +- Modify: `adaptive-strategy/src/risk/mod.rs` (~14 dead items: PnLTracker fields, VaRCalculator fields, CorrelationMatrix, convert_regime, calculate_risk_parity_size, etc.) +- Modify: `adaptive-strategy/src/models/traditional.rs` (~4 dead fields: config/ready on MovingAverageModel, MACDModel, BollingerBandsModel, RSIModel) +- Modify: `adaptive-strategy/src/models/deep_learning.rs` (~6 dead fields: config/ready on LSTMModel, TransformerModel, CNNModel) + +**Step 1: For each file, verify which items are truly unused** + +Search the workspace for each flagged item. For struct fields marked dead: if the struct is constructed but the field is never read, the field is dead. If the struct itself is never constructed, the whole struct is dead. + +**Step 2: Delete confirmed dead items** + +For dead struct fields: remove the field and update all constructors. For dead functions: delete them. For dead structs: delete the entire struct and its impl blocks. + +**Important:** Some `#[allow(dead_code)]` fields (like `config` and `ready` on model structs) may be intentionally reserved for future use. If a model struct IS actively used and only some fields are dead, remove just the dead fields rather than the whole struct. + +**Step 3: Verify** + +Run: `SQLX_OFFLINE=true cargo check -p adaptive-strategy` +Run: `SQLX_OFFLINE=true cargo test -p adaptive-strategy --lib` + +**Step 4: Commit** + +```bash +git add adaptive-strategy/src/execution/mod.rs adaptive-strategy/src/risk/mod.rs adaptive-strategy/src/models/traditional.rs adaptive-strategy/src/models/deep_learning.rs +git commit -m "refactor(adaptive-strategy): remove dead code from execution, risk, and model modules" +``` + +--- + +### Task 11: Remove hardcoded config warnings + +`adaptive-strategy/src/config.rs` has 8 `eprintln!("WARNING: Using hardcoded...")` calls in Default impls. These are noise — Default impls are legitimate Rust patterns. + +**Files:** +- Modify: `adaptive-strategy/src/config.rs` + +**Step 1: Remove all eprintln! warnings from Default impls** + +Find and remove all lines matching: +```rust +eprintln!("WARNING: Using hardcoded ... - migrate to database configuration!"); +``` + +These appear in Default impls for: GeneralConfig, AdaptiveStrategyConfig, EnsembleConfig, ModelConfig, RiskConfig, MicrostructureConfig, RegimeConfig, ExecutionConfig. + +Keep all the actual Default field values — just remove the warning prints. + +**Step 2: Verify** + +Run: `SQLX_OFFLINE=true cargo check -p adaptive-strategy` +Run: `SQLX_OFFLINE=true cargo test -p adaptive-strategy --lib` + +**Step 3: Commit** + +```bash +git add adaptive-strategy/src/config.rs +git commit -m "refactor(adaptive-strategy): remove noisy hardcoded config eprintln warnings" +``` + +--- + +### Task 12: Move Adam optimizer out of ml/src/lib.rs + +The Adam optimizer struct and impl are defined inline in `ml/src/lib.rs` (approximately lines 80-180). This belongs in its own module. + +**Files:** +- Create: `ml/src/optimizers/mod.rs` +- Create: `ml/src/optimizers/adam.rs` +- Modify: `ml/src/lib.rs` + +**Step 1: Create ml/src/optimizers/adam.rs** + +Move the `Adam` struct, its `impl Adam` block, and all related imports from `ml/src/lib.rs` into this new file. The struct uses `candle_optimisers::adam::Adam` internally, `candle_core::Var` and `Tensor`. + +**Step 2: Create ml/src/optimizers/mod.rs** + +```rust +pub mod adam; +pub use adam::Adam; +``` + +**Step 3: Update ml/src/lib.rs** + +- Add `pub mod optimizers;` +- Add `pub use optimizers::Adam;` (preserves existing public API) +- Remove the Adam struct and impl block from lib.rs + +**Step 4: Verify** + +Run: `SQLX_OFFLINE=true cargo check -p ml` +Run: `SQLX_OFFLINE=true cargo test -p ml --lib` +Run: `SQLX_OFFLINE=true cargo check --workspace` (other crates may import `ml::Adam`) + +**Step 5: Commit** + +```bash +git add ml/src/optimizers/ ml/src/lib.rs +git commit -m "refactor(ml): move Adam optimizer from lib.rs to optimizers/adam.rs" +``` + +--- + +### Task 13: Phase 3 verification + +**Step 1: Full workspace check** + +Run: `SQLX_OFFLINE=true cargo check --workspace` + +**Step 2: Run affected tests** + +Run: `SQLX_OFFLINE=true cargo test -p ml --lib` +Run: `SQLX_OFFLINE=true cargo test -p adaptive-strategy --lib` + +**Step 3: Clippy** + +Run: `SQLX_OFFLINE=true cargo clippy -p ml -p adaptive-strategy -- -D warnings 2>&1 | head -50` + +--- + +## Phase 4: CircuitBreaker Consolidation + +### Task 14: Define CircuitBreaker trait in common/ + +Create a `CircuitBreaker` trait in `common/src/resilience/circuit_breaker.rs` that captures the core state machine. Keep `SimpleCircuitBreaker` as the default implementation. + +**Important context:** The current `common/src/resilience/circuit_breaker.rs` has: `CircuitBreakerState` enum, `CircuitBreakerConfig` struct (3 fields), `CircuitBreakerMetrics` struct, and `CircuitBreaker` struct with async Mutex. We're adding a trait and renaming the existing struct. + +**Files:** +- Modify: `common/src/resilience/circuit_breaker.rs` + +**Step 1: Read the current file fully** + +Read all 399 lines of `common/src/resilience/circuit_breaker.rs` to understand the existing API. + +**Step 2: Add the trait definition** + +Add above the existing struct: + +```rust +/// Core circuit breaker trait for the state machine pattern. +/// +/// Implementors manage the Closed → Open → HalfOpen state transitions. +#[async_trait::async_trait] +pub trait CircuitBreakerTrait: Send + Sync { + /// Check if a request is allowed through. + async fn can_execute(&self) -> bool; + + /// Record a successful operation. + async fn record_success(&self); + + /// Record a failed operation. + async fn record_failure(&self); + + /// Get current state. + async fn state(&self) -> CircuitBreakerState; + + /// Reset to closed state. + async fn reset(&self); +} +``` + +**Step 3: Implement the trait for existing CircuitBreaker** + +Add `#[async_trait::async_trait]` and `impl CircuitBreakerTrait for CircuitBreaker` that delegates to the existing methods. The existing methods already have the right signatures. + +**Step 4: Ensure async-trait is in common/Cargo.toml** + +Check if `async-trait` is already a dependency. If not, add it. + +**Step 5: Verify** + +Run: `SQLX_OFFLINE=true cargo check -p common` +Run: `SQLX_OFFLINE=true cargo test -p common --lib` + +**Step 6: Commit** + +```bash +git add common/src/resilience/circuit_breaker.rs common/Cargo.toml +git commit -m "feat(common): add CircuitBreakerTrait for shared circuit breaker interface" +``` + +--- + +### Task 15: Implement trait in ml/ CircuitBreaker + +Make `ml/src/common/circuit_breaker.rs` implement the new `CircuitBreakerTrait` from common. + +**Files:** +- Modify: `ml/src/common/circuit_breaker.rs` + +**Step 1: Read the current ml CircuitBreaker** + +Read all 413 lines. It uses `parking_lot::RwLock` + `AtomicU32`/`AtomicU64` and has methods `allow_request()`, `record_success()`, `record_failure()`, `state()`, `reset()`. + +**Step 2: Add trait import and implementation** + +```rust +use common::resilience::circuit_breaker::CircuitBreakerTrait; + +#[async_trait::async_trait] +impl CircuitBreakerTrait for CircuitBreaker { + async fn can_execute(&self) -> bool { + self.allow_request() + } + + async fn record_success(&self) { + CircuitBreaker::record_success(self); + } + + async fn record_failure(&self) { + CircuitBreaker::record_failure(self); + } + + async fn state(&self) -> common::resilience::circuit_breaker::CircuitBreakerState { + // Map ml's CircuitState to common's CircuitBreakerState + match self.state() { + CircuitState::Closed => common::resilience::circuit_breaker::CircuitBreakerState::Closed, + CircuitState::Open => common::resilience::circuit_breaker::CircuitBreakerState::Open, + CircuitState::HalfOpen => common::resilience::circuit_breaker::CircuitBreakerState::HalfOpen, + } + } + + async fn reset(&self) { + CircuitBreaker::reset(self); + } +} +``` + +**Note:** The ml CircuitBreaker's existing methods are synchronous (parking_lot::RwLock). The trait methods are async but the impl can call sync methods from async context without issue. + +**Step 3: Verify** + +Run: `SQLX_OFFLINE=true cargo check -p ml` +Run: `SQLX_OFFLINE=true cargo test -p ml --lib` + +**Step 4: Commit** + +```bash +git add ml/src/common/circuit_breaker.rs +git commit -m "refactor(ml): implement CircuitBreakerTrait for ML circuit breaker" +``` + +--- + +### Task 16: Replace inline CircuitBreaker in broker_gateway_service + +Replace the ad-hoc inline CircuitBreaker in `broker_gateway_service/src/error_handler.rs` with `common::resilience::CircuitBreaker`. + +**Files:** +- Modify: `services/broker_gateway_service/src/error_handler.rs` +- Modify: `services/broker_gateway_service/Cargo.toml` (ensure common dependency) + +**Step 1: Read the inline implementation** + +The inline CircuitBreaker (~line 71) has: `CircuitBreakerState` enum, `CircuitBreaker` struct with `state: Arc>`, `failure_count: Arc>`, `opened_at: Arc>>`. Methods: `new()`, `state()`, `record_success()`, `record_failure()`, `can_execute()` (previously named `allow_request` — check actual name). + +**Step 2: Replace with common's CircuitBreaker** + +Remove the inline CircuitBreakerState enum and CircuitBreaker struct/impl. Import from common: + +```rust +use common::resilience::circuit_breaker::{CircuitBreaker, CircuitBreakerConfig, CircuitBreakerState}; +``` + +Update the `ErrorHandler` struct to use `common::resilience::circuit_breaker::CircuitBreaker`. The config constants at the top of the file (`CIRCUIT_BREAKER_THRESHOLD: usize = 5`, `CIRCUIT_BREAKER_TIMEOUT: Duration = 60s`) should be used to construct a `CircuitBreakerConfig`: + +```rust +let cb_config = CircuitBreakerConfig { + failure_threshold: CIRCUIT_BREAKER_THRESHOLD as u32, + timeout: CIRCUIT_BREAKER_TIMEOUT, + success_threshold: 1, +}; +let circuit_breaker = CircuitBreaker::new(cb_config); +``` + +**Step 3: Update all usages** + +The ErrorHandler uses `circuit_breaker.can_execute()`, `circuit_breaker.record_success()`, `circuit_breaker.record_failure()`, `circuit_breaker.state()`. These should map directly to common's CircuitBreaker methods. Check method name differences (e.g., `allow_request` vs `can_execute`). + +**Step 4: Verify** + +Run: `SQLX_OFFLINE=true cargo check -p broker_gateway_service` +Run: `SQLX_OFFLINE=true cargo test -p broker_gateway_service --lib` + +**Step 5: Commit** + +```bash +git add services/broker_gateway_service/src/error_handler.rs services/broker_gateway_service/Cargo.toml +git commit -m "refactor(broker_gateway): replace inline CircuitBreaker with common::resilience::CircuitBreaker" +``` + +--- + +### Task 17: Replace inline CircuitBreaker in data/benzinga + +Replace the ad-hoc inline CircuitBreaker in `data/src/providers/benzinga/production_streaming.rs` with `common::resilience::CircuitBreaker`. + +**Files:** +- Modify: `data/src/providers/benzinga/production_streaming.rs` + +**Step 1: Read the inline implementation** + +The inline version (~line 203) has `CircuitBreakerState` enum, `CircuitBreaker` struct with configurable `threshold` and `timeout`. Methods: `new(threshold, timeout)`, `is_call_permitted()`, `record_success()`, `record_failure()`. + +**Step 2: Replace with common's CircuitBreaker** + +Remove the inline types. Import from common: +```rust +use common::resilience::circuit_breaker::{CircuitBreaker, CircuitBreakerConfig}; +``` + +Where the inline version was constructed as `CircuitBreaker::new(threshold, timeout)`, use: +```rust +let config = CircuitBreakerConfig { + failure_threshold: threshold, + timeout, + success_threshold: 1, +}; +CircuitBreaker::new(config) +``` + +Replace `is_call_permitted()` calls with `can_execute()`. + +**Step 3: Verify** + +Run: `SQLX_OFFLINE=true cargo check -p data` +Run: `SQLX_OFFLINE=true cargo test -p data --lib` + +**Step 4: Commit** + +```bash +git add data/src/providers/benzinga/production_streaming.rs +git commit -m "refactor(data): replace inline CircuitBreaker with common::resilience::CircuitBreaker" +``` + +--- + +### Task 18: Phase 4 verification and final check + +**Step 1: Full workspace check** + +Run: `SQLX_OFFLINE=true cargo check --workspace` +Expected: 0 errors + +**Step 2: Full lib test suite** + +Run: `SQLX_OFFLINE=true cargo test --workspace --lib 2>&1 | tail -30` + +**Step 3: Clippy on all modified crates** + +Run: `SQLX_OFFLINE=true cargo clippy -p common -p ml -p data -p broker_gateway_service -p adaptive-strategy -p api_gateway -p model_loader -- -D warnings 2>&1 | head -50` + +**Step 4: Verify warning count** + +Run: `SQLX_OFFLINE=true cargo check --workspace 2>&1 | grep -c "warning\["` +Expected: Same or fewer warnings than before + +--- + +## Notes for Implementer + +### ErrorSeverity was scoped out + +The exploration revealed that `ErrorSeverity` is NOT identical across crates — it has different variants in different domains: +- `common/`: Debug, Info, Warn, Error, Critical (logging levels) +- `database/`, `data/`, `trading_engine/errors`: Low, Medium, High, Critical (impact levels) +- `trading_engine/events`: Info, Warning, Error, Critical, Fatal (event levels) + +These are semantically different types that happen to share a name. Consolidating them requires a design decision about whether to create two distinct types or merge into a superset. This is deferred to a future cleanup. + +### RetryStrategy was scoped out + +The exploration revealed that `trading_engine/src/types/retry.rs` (622 lines) is significantly more complex than `common/src/resilience/retry.rs` — it has `RetryPolicy` (18 fields), `RetryContext`, `RetryExecutor`, domain-specific presets (hft_optimized, financial_conservative, etc.), and its own test suite. The trading_engine version also has a separate `RetryStrategy` enum in `types/error.rs`. Merging these requires careful analysis of which consumers depend on each. Deferred to future work. + +### CircuitBreaker in trading_engine and risk + +The `trading_engine/` (989 lines) and `risk/` (981 lines) CircuitBreakers are intentionally complex and domain-specific: +- `trading_engine/`: Success rate thresholds, latency detection, error classification, HFT presets, registry pattern +- `risk/`: Portfolio-percentage-based, per-account state, Redis coordination, PnL tracking + +These are NOT duplicates of common's simple CircuitBreaker — they're specialized implementations. The trait approach lets them implement `CircuitBreakerTrait` in the future while keeping their domain logic. Converting them fully is deferred. + +### What NOT to touch +- `tli/` — being replaced by web-dashboard, don't waste time cleaning it +- `risk/src/circuit_breaker.rs` — portfolio-based, fundamentally different from request-based CB +- `trading_engine/src/types/circuit_breaker.rs` — richest implementation, keep as-is for now +- Test files — dead code in tests is expected and harmless