diff --git a/docs/plans/2026-02-22-codebase-deduplication-design.md b/docs/plans/2026-02-22-codebase-deduplication-design.md new file mode 100644 index 000000000..29b73a0a2 --- /dev/null +++ b/docs/plans/2026-02-22-codebase-deduplication-design.md @@ -0,0 +1,127 @@ +# Codebase De-duplication & Cleanup Design + +**Goal:** Eliminate duplicated types, dead code, and code smells across the 37-crate workspace to make the codebase professional and maintainable. + +**Approach:** Bottom-up consolidation in 4 independent phases, each compiling and testing independently. + +**Estimated impact:** ~2,500+ lines removed, 15+ duplicated types consolidated, 274 dead code markers audited. + +--- + +## Phase 1 — Zero-Risk Dedup + +### TLS Config Consolidation (-400 lines) +- **Problem:** 4 identical TLS config implementations across services (trading_service, ml_training_service, backtesting_service, api_gateway) +- **Solution:** Create `common/src/tls.rs` with unified `TlsServiceConfig`. Services use type aliases for backward compat. +- **Files:** + - Create: `common/src/tls.rs` + - Modify: `common/src/lib.rs` (add pub mod) + - Delete content from: `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` + - Replace with re-exports + +### ErrorSeverity Consolidation (-50 lines) +- **Problem:** Identical enum defined in 5 places (database, trading_engine x2, data x2, common) +- **Solution:** Keep canonical in `common/src/error.rs`. Other crates import via `pub use common::error::ErrorSeverity;` +- **Files:** `database/src/error.rs`, `trading_engine/src/types/errors.rs`, `trading_engine/src/types/events.rs`, `data/src/error.rs`, `data/src/validation.rs` + +### ConfigError Merge (-31 lines) +- **Problem:** Duplicate `ConfigError` in `config/src/error.rs` and `services/api_gateway/src/error.rs` +- **Solution:** api_gateway re-exports from config crate +- **Files:** `services/api_gateway/src/error.rs` + +### OrderType in ml/ (-40 lines) +- **Problem:** Identical Buy/Sell/Hold enum in 4 ml/ files +- **Solution:** Create `ml/src/common/action.rs`, re-export from dqn/ppo modules +- **Files:** `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` + +--- + +## Phase 2 — Low-Risk Dedup + +### RetryStrategy Consolidation (-600 lines) +- **Problem:** Duplicate retry logic in `common/src/error.rs` and `trading_engine/src/types/error.rs` + `retry.rs` +- **Solution:** Canonical in `common/src/resilience/retry.rs`. trading_engine imports. Keep float-precision jitter from trading_engine version. +- **Files:** `trading_engine/src/types/error.rs`, `trading_engine/src/types/retry.rs` + +### ErrorCategory Consolidation (-30 lines) +- **Problem:** 3 definitions (common has 12 variants, ml has stub, data has partial) +- **Solution:** Canonical in `common/src/error.rs`. Remove ml/ and data/ definitions, replace with imports. +- **Files:** `ml/src/lib.rs`, `data/src/providers/common.rs` + +### ModelType Unification (-50 lines) +- **Problem:** 6 definitions. ml/ has 11 variants, model_loader/ has 7 incompatible variants. +- **Solution:** Canonical in `ml/src/lib.rs`. model_loader/ and services import from ml. +- **Files:** `model_loader/src/lib.rs`, `ml/src/hyperopt/campaign.rs`, `services/ml_training_service/src/job_spawner.rs` + +### Confusing Re-export Cleanup +- **Problem:** `common/src/lib.rs` has aliased re-exports (`BarEventFromMarketData`, `MarketDataEventFromMarketData`) creating naming confusion +- **Solution:** Remove aliases, use single canonical names +- **Files:** `common/src/lib.rs` (lines 64-69) + +--- + +## Phase 3 — Dead Code & Smell Removal + +### Dead Code Audit (274 instances) +- **Focus:** `adaptive-strategy/` (61 in kelly_position_sizer, 19 in execution, 16 in risk, 8 in models) +- **Action:** For each `#[allow(dead_code)]`: if code IS used → remove allow; if NOT used → delete the code +- **Files:** `adaptive-strategy/src/risk/kelly_position_sizer.rs`, `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` + +### Hardcoded Config Warnings (-30 lines) +- **Problem:** 8 `eprintln!("WARNING: Using hardcoded...")` in `adaptive-strategy/src/config.rs` +- **Action:** Remove warnings. Default impls are legitimate; the warnings add noise. +- **Files:** `adaptive-strategy/src/config.rs` + +### Adam Optimizer Relocation +- **Problem:** Adam optimizer defined in `ml/src/lib.rs` (lines 88-174) instead of separate module +- **Action:** Move to `ml/src/optimizers/adam.rs`, re-export from lib.rs +- **Files:** Create `ml/src/optimizers/adam.rs`, modify `ml/src/lib.rs` + +--- + +## Phase 4 — CircuitBreaker Consolidation (~1,500 lines removed) + +### Current State (5 implementations, 2,781 lines) +1. `common/src/resilience/circuit_breaker.rs` (399 lines) — generic base +2. `ml/src/common/circuit_breaker.rs` (412 lines) — ML throttling +3. `trading_engine/src/types/circuit_breaker.rs` (989 lines) — richest, atomic counters + metrics +4. `risk/src/circuit_breaker.rs` (981 lines) — Redis coordination, portfolio % +5. `broker_gateway_service/src/error_handler.rs` — inline CB +6. `data/src/providers/benzinga/production_streaming.rs` — inline CB + +### Target Architecture +- **Trait:** `CircuitBreaker` in `common/src/resilience/circuit_breaker.rs` — core state machine (Closed→Open→HalfOpen) +- **Generic impl:** `SimpleCircuitBreaker` in common/ (enhanced from current 399-line version) +- **Trading impl:** `TradingCircuitBreaker` in trading_engine/ wrapping common with metrics/atomics +- **Risk impl:** `RiskCircuitBreaker` in risk/ wrapping common with Redis + portfolio % +- **ML:** Keep lightweight version, implement trait +- **broker_gateway, data:** Import SimpleCircuitBreaker from common + +### Files +- Rewrite: `common/src/resilience/circuit_breaker.rs` (trait + SimpleCircuitBreaker) +- Simplify: `trading_engine/src/types/circuit_breaker.rs` (wrap common) +- Simplify: `risk/src/circuit_breaker.rs` (wrap common) +- Update: `ml/src/common/circuit_breaker.rs` (impl trait) +- Update: `broker_gateway_service/src/error_handler.rs` (import from common) +- Update: `data/src/providers/benzinga/production_streaming.rs` (import from common) + +--- + +## Risk Assessment + +| Phase | Risk | Rollback | Est. Time | +|-------|------|----------|-----------| +| 1 | None | git revert | ~1h | +| 2 | Low | git revert | ~3h | +| 3 | Low | git revert | ~2h | +| 4 | Medium | git revert | ~4h | + +Each phase commits independently. Full workspace `cargo check` and `cargo test` between phases. + +--- + +## Out of Scope (Future Work) +- God file splitting (regime/mod.rs 5,020 lines, types.rs 4,909 lines) +- ModelMetadata unification (8 divergent definitions — needs separate design) +- RiskConfig consolidation (8 definitions with domain-specific needs) +- Position type unification (15+ domain-specific definitions — intentional)