f31c852a45
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>
128 lines
6.6 KiB
Markdown
128 lines
6.6 KiB
Markdown
# 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)
|