|
| 1 | +# Refactor Roadmap |
| 2 | + |
| 3 | +This document tracks the main simplification and cleanup opportunities in the codebase after the current order-lifecycle and admin-dashboard refactors. |
| 4 | + |
| 5 | +It is intentionally pragmatic: |
| 6 | + |
| 7 | +- focus on reducing duplicate orchestration, |
| 8 | +- keep market-agnostic behavior intact, |
| 9 | +- preserve auditability and permission boundaries, |
| 10 | +- avoid speculative abstractions that do not remove real complexity. |
| 11 | + |
| 12 | +## Current Direction |
| 13 | + |
| 14 | +The codebase has already made two important moves: |
| 15 | + |
| 16 | +- order placement now runs through shared services instead of separate user/admin execution paths |
| 17 | +- order cancellation now also runs through shared services instead of separate route/worker code |
| 18 | + |
| 19 | +The next simplifications should continue in the same direction: |
| 20 | + |
| 21 | +- keep routes thin |
| 22 | +- keep workers focused on scheduling and trigger decisions |
| 23 | +- keep read models and write paths centralized |
| 24 | + |
| 25 | +## Recommended Refactor Sequence |
| 26 | + |
| 27 | +### 1. Shared Portfolio Read Models |
| 28 | + |
| 29 | +Priority: high |
| 30 | +Status: completed |
| 31 | + |
| 32 | +Problem: |
| 33 | + |
| 34 | +- user portfolio and admin single-user portfolio both enrich positions, quotes, and pending orders |
| 35 | +- admin overview also contains a third read-model path that re-derives similar account and position summaries |
| 36 | + |
| 37 | +Current duplication exists across: |
| 38 | + |
| 39 | +- `packages/api/src/routes/account.ts` |
| 40 | +- `packages/api/src/routes/admin.ts` |
| 41 | + |
| 42 | +Why this matters: |
| 43 | + |
| 44 | +- portfolio fields can drift between user and admin views |
| 45 | +- quote handling and perp enrichment rules are being maintained in more than one place |
| 46 | +- small additions such as `maintenanceMargin`, `accumulatedFunding`, or future audit fields require repeated edits |
| 47 | + |
| 48 | +Implemented shape: |
| 49 | + |
| 50 | +- `packages/api/src/services/portfolio-read.ts` |
| 51 | +- `packages/api/src/services/admin-overview.ts` |
| 52 | + |
| 53 | +These builders now centralize: |
| 54 | + |
| 55 | +- quote fetching |
| 56 | +- perp state enrichment |
| 57 | +- funding aggregation |
| 58 | +- open-order shaping |
| 59 | +- account-level totals |
| 60 | + |
| 61 | +### 2. Move Snapshot Writes Out Of `GET /api/admin/overview` |
| 62 | + |
| 63 | +Priority: high |
| 64 | +Status: completed |
| 65 | + |
| 66 | +Problem: |
| 67 | + |
| 68 | +- the admin overview route performs asynchronous `equity_snapshots` writes after building the response |
| 69 | + |
| 70 | +Current location: |
| 71 | + |
| 72 | +- `packages/api/src/routes/admin.ts` |
| 73 | + |
| 74 | +Why this matters: |
| 75 | + |
| 76 | +- a read endpoint is performing background writes |
| 77 | +- failure handling is hidden in route-local `void (...)` fire-and-forget code |
| 78 | +- snapshot cadence policy is tied to a dashboard read path instead of an explicit worker or service |
| 79 | + |
| 80 | +Implemented shape: |
| 81 | + |
| 82 | +- `/api/admin/overview` is read-only |
| 83 | +- snapshot generation runs through `packages/api/src/equity-snapshotter.ts` |
| 84 | +- snapshot cadence is controlled by `EQUITY_SNAPSHOT_INTERVAL_MS` |
| 85 | + |
| 86 | +### 3. Shared Worker Scaffold |
| 87 | + |
| 88 | +Priority: medium |
| 89 | +Status: completed |
| 90 | + |
| 91 | +Problem: |
| 92 | + |
| 93 | +- reconciler, settler, funding collector, and liquidator all repeat the same interval/locking/logging structure |
| 94 | + |
| 95 | +Current locations: |
| 96 | + |
| 97 | +- `packages/api/src/reconciler.ts` |
| 98 | +- `packages/api/src/settler.ts` |
| 99 | +- `packages/api/src/funding-collector.ts` |
| 100 | +- `packages/api/src/liquidator.ts` |
| 101 | + |
| 102 | +Why this matters: |
| 103 | + |
| 104 | +- interval parsing, running guards, startup logging, and stop handlers are duplicated |
| 105 | +- worker ergonomics are inconsistent by file over time |
| 106 | + |
| 107 | +Implemented shape: |
| 108 | + |
| 109 | +- `packages/api/src/periodic-worker.ts` |
| 110 | +- reconciler, settler, funding collector, liquidator, and equity snapshotter all use the shared scaffold |
| 111 | + |
| 112 | +### 4. Shared Timeline Contract Types |
| 113 | + |
| 114 | +Priority: medium |
| 115 | +Status: completed |
| 116 | + |
| 117 | +Problem: |
| 118 | + |
| 119 | +- the web dashboard re-declares the timeline event contract instead of consuming a shared source |
| 120 | + |
| 121 | +Current duplication exists across: |
| 122 | + |
| 123 | +- `packages/api/src/timeline.ts` |
| 124 | +- `packages/api/src/events.ts` |
| 125 | +- `packages/web/src/lib/useAgentTimeline.ts` |
| 126 | +- `packages/web/src/components/ActivityFeed.tsx` |
| 127 | + |
| 128 | +Why this matters: |
| 129 | + |
| 130 | +- adding or renaming event fields requires touching both API and web manually |
| 131 | +- UI assumptions can drift from the actual backend timeline payload |
| 132 | + |
| 133 | +Implemented shape: |
| 134 | + |
| 135 | +- timeline event record types live in `@unimarket/core` |
| 136 | +- API timeline builders and dashboard timeline hooks consume the shared contract |
| 137 | + |
| 138 | +### 5. Break Up `TradePage` |
| 139 | + |
| 140 | +Priority: medium |
| 141 | +Status: completed |
| 142 | + |
| 143 | +Problem: |
| 144 | + |
| 145 | +- the admin trade console currently owns market loading, agent loading, quote refresh, search, portfolio fetch, order form state, and trader creation in one page component |
| 146 | + |
| 147 | +Current location: |
| 148 | + |
| 149 | +- `packages/web/src/pages/TradePage.tsx` |
| 150 | + |
| 151 | +Why this matters: |
| 152 | + |
| 153 | +- the file is large and mixes multiple concerns |
| 154 | +- repeated auth-failure handling is embedded in page-level effects |
| 155 | +- API response types are redeclared inline instead of being shared or centralized |
| 156 | + |
| 157 | +Implemented shape: |
| 158 | + |
| 159 | +- `packages/web/src/pages/TradePage.tsx` now acts as an orchestration page |
| 160 | +- trade-specific UI lives under `packages/web/src/components/trade/` |
| 161 | +- networking and response types no longer live inline inside the page file |
| 162 | + |
| 163 | +### 6. Shared Admin API Client Helpers |
| 164 | + |
| 165 | +Priority: medium |
| 166 | +Status: completed |
| 167 | + |
| 168 | +Problem: |
| 169 | + |
| 170 | +- admin web code still performs repeated `fetch + auth header + auth failure handling + response parsing` logic |
| 171 | + |
| 172 | +Current locations: |
| 173 | + |
| 174 | +- `packages/web/src/pages/TradePage.tsx` |
| 175 | +- other admin-facing web modules |
| 176 | + |
| 177 | +Implemented shape: |
| 178 | + |
| 179 | +- `packages/web/src/lib/admin-api.ts` |
| 180 | +- admin hooks and pages now share: |
| 181 | + - auth header injection |
| 182 | + - `401/403` logout handling |
| 183 | + - JSON error extraction |
| 184 | + - typed response parsing |
| 185 | + |
| 186 | +## Future Reconciler Evolution |
| 187 | + |
| 188 | +Priority: not short-term |
| 189 | + |
| 190 | +The current reconciler model is intentionally simple: |
| 191 | + |
| 192 | +- global scan of `pending` limit orders |
| 193 | +- one quote fetch per `market:symbol` |
| 194 | +- executable-side check using `buy -> ask`, `sell -> bid` |
| 195 | +- once crossed, the whole order fills |
| 196 | + |
| 197 | +This is the right short-term model for paper trading because it is easy to explain and easy to audit. |
| 198 | + |
| 199 | +### What may be added later |
| 200 | + |
| 201 | +If the platform later wants more realistic limit-order simulation, the reconciler may evolve toward: |
| 202 | + |
| 203 | +- depth-aware matching for markets with `orderbook` |
| 204 | +- price-time priority instead of simple traversal order |
| 205 | +- partial fills |
| 206 | +- explicit residual pending quantity |
| 207 | + |
| 208 | +### What should not happen |
| 209 | + |
| 210 | +The platform should not introduce a half-step model such as: |
| 211 | + |
| 212 | +- “sometimes liquidity matters” |
| 213 | +- but without orderbook depth |
| 214 | +- and without partial fills |
| 215 | +- and without price-time priority |
| 216 | + |
| 217 | +That would be more complex than the current model without being meaningfully more correct. |
| 218 | + |
| 219 | +### Recommended future boundary |
| 220 | + |
| 221 | +If this work is taken on later: |
| 222 | + |
| 223 | +- keep the current simple reconciler as the default path |
| 224 | +- enable depth-aware behavior only for markets that expose `orderbook` |
| 225 | +- preserve a clear, documented distinction between: |
| 226 | + - trigger price |
| 227 | + - execution price |
| 228 | + - filled quantity |
| 229 | + - remaining pending quantity |
| 230 | + |
| 231 | +This is intentionally a future path, not an immediate roadmap item. |
| 232 | + |
| 233 | +## What To Avoid |
| 234 | + |
| 235 | +These changes would likely add churn without removing enough complexity: |
| 236 | + |
| 237 | +- merging user and admin HTTP routes into one path |
| 238 | +- pushing permission logic down into core trading functions |
| 239 | +- adding market-specific branches outside adapters and capability checks |
| 240 | +- introducing a generic abstraction before there are at least two real call sites |
| 241 | + |
| 242 | +## Decision Rule |
| 243 | + |
| 244 | +Before taking on any simplification, ask: |
| 245 | + |
| 246 | +1. Does it remove an actual duplicate business rule or orchestration path? |
| 247 | +2. Does it preserve auditability and permission boundaries? |
| 248 | +3. Does it make the next feature cheaper without hiding current behavior? |
| 249 | + |
| 250 | +If the answer is not clearly yes, the refactor probably is not worth doing yet. |
0 commit comments