docs: add codebase de-duplication design

4-phase bottom-up consolidation plan:
- Phase 1: TLS, ErrorSeverity, ConfigError, OrderType (zero risk)
- Phase 2: RetryStrategy, ErrorCategory, ModelType, re-exports (low risk)
- Phase 3: Dead code audit, hardcoded config warnings, Adam move (low risk)
- Phase 4: CircuitBreaker trait hierarchy (medium risk)

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
This commit is contained in:
jgrusewski
2026-02-22 19:54:53 +01:00
parent 92b5ba79be
commit f31c852a45

View File

@@ -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)