# Project Mistakes Recurring mistake patterns found in this project. Refer to this before implementing code or writing design documents. ## How to Add When a mistake is found, ask Claude: "Add this mistake to project-mistakes.md". Format: `- ❌ [mistake pattern] → [correct approach]` > Seeded from the sibling projects `af-trade-stream` and `sol-alpha-finder-tracker`. > Items proven in both are marked **[both]**. --- ## Rich Model — where logic lives - ❌ **[both]** Build an entity from a partial object literal: `repository.create({ account_id, token_address, status: 'WATCHING' })` → define a `static of...()` / `from()` factory on the model, then `repository.create(instance)`. A missing field in an object literal compiles fine; a factory makes the omission explicit. Use partials only for update queries where it is actually needed. - ❌ **[both]** Mutate entity fields from a service: `entity.status = X; entity.updated_at = new Date();` → put a named behavior method on the model (`open()`, `updateBaseline(v)`, `markFailed()`) and call that. Scattered assignment is how derived-field updates get missed. - ❌ Write conditional domain logic in a service: `if (entity.state === State.FAILED) { ... }` → add the predicate to the model (`isInitializing()`, `resetFromFailed()`). - ❌ Write date/parse/transform logic in a service → move it onto the model (private static helper if it is internal). - ⚠️ **typegoose models must not use `private`** — the `Document` type does not carry private members, so consumers fail to compile. Use a `_` prefix with `public` for internal members. > **Judgment question: does any argument hold a value the receiver could reach on its own?** - ❌ The caller unpacks a value from a domain object and hands it back: `f(order, position, position.trading_config, now)` → **a signal that the decision lives in the wrong place**. Move the logic to where the data is and delete the argument. - ❌ The caller re-evaluates a condition the callee already evaluates internally → **check first** whether the callee already decides. If it duplicates, the branch is waste; if the two disagree, it is a bug. - ❌ A constant is passed as an argument but there is only one caller → move it to a consts file and reference it directly. - ❌ Injecting `now` from above → justified **only when a reproduction path (simulation / backtest) actually exists**. If every caller is the live path, call `new Date()` inside. ## Layer responsibilities - ❌ Business logic in a repository (state changes, date math) → repositories do pure data access only: `save()`, `findByCode()`. - ❌ Service implements business logic directly → a service **composes**: call model method → `repository.save()`. - ❌ Collect everything and save once at the end → save in chunks and update progress, so the job is resumable. ## Defensive condition checks — highest-priority prohibition - ❌ **[both]** Re-check a condition the caller already guarantees (query filter, type system, lock held): `if (_isNil(x)) return;`, `if (arr.length === 0) return;`, or re-testing `status === PENDING` on rows a `findPending...()` query returned → **delete it and just access the value.** Let a genuine violation crash naturally. - Reason: the check's **existence plants the false assumption that the value can be null** in every future reader's head. Making readers trace "can this be nil here?" on every line is the main destroyer of readability. - Condition checks belong **only at input/output boundaries** — data outside system control: external API responses, user input, contracts that return a meaningful null. Trust internal invariants. - **Never swallow with a silent `return` or `throw`.** Quietly returning on an "impossible" state hides the bug when it does happen. - Test: "is there a realistic scenario where this condition is false?" — if not, no guard. - ⚠️ **Exception — invariants that break over time**: "cannot be nil inside the lock" does **not** hold when the candidate query ran *outside* the lock. Another path can change state in the gap between query and acquire; that check is a **boundary check**, not defensive noise. - Test: was the query that produced this value run **inside the same lock**? If not, it is not an invariant. - When adding a new cron or worker, verify it does not invalidate an existing "cannot be nil" premise elsewhere. ## try/catch and cross-cutting wrappers - ❌ **[both]** Wrap internal service calls in try/catch → the error is swallowed, callers never learn of the failure, and the original stack trace is lost. Just call it and let the top-level exception handler deal with it. try/catch belongs at external boundaries only (SDKs, HTTP, `JSON.parse`). > **Judgment question: can an existing try/catch already do this job?** - ❌ Wrap every call in a thunk: `wrap(config, () => realCall())` → call sites get re-indented and the real call hides one layer down. **Count the boundaries first**; if one already exists, use its catch. - ❌ Attach a cross-cutting concern at every step → narrow the criterion to **side-effect boundaries (external I/O)** and the count usually drops to 2–3. Local operations in between (pure computation, serialization, signing) are not wrapped. - ❌ Keep a thunk signature just to also accept sync functions → if every target is a `Promise`, `.catch(handler)` is enough. ## Naming convention: DB schema vs DTO/interface - ❌ `snake_case` in DTOs, request/response types, or domain interfaces → DB schema naming leaks through the whole codebase. - ✅ Only the Model (DB schema) uses `snake_case`. Request DTO, Response DTO, input/output interfaces, and domain interfaces all use `camelCase`. Do the snake → camel mapping inside the DTO's `from(entity)`. ## Enums - ❌ String union types: `type Status = 'WATCHING' | 'OPEN' | 'CLOSED'` → typos, no autocomplete, string literals sprinkled through comparisons. - ✅ Define a TypeScript `enum` and use enum members for assignment and comparison. Pass the enum object to typegoose too: `@prop({ enum: RecordStatus })`, not a string array. ## Constants - ❌ Hardcode a magic value inside a function: `const chunkSize = 100;` → move it to a consts file with a descriptive name (`HISTORICAL_SYNC_CHUNK_SIZE` in `*.consts.ts`). ## DateTime math - ❌ Inline millisecond arithmetic: `(a.getTime() - b.getTime()) / 1000`, `Date.now() + sec * 1000` → the intent is buried in a magic constant and the unit is easy to confuse. - ✅ Use the helpers in `src/common/utils/datetime.ts` (`getTimeDifferenceInSec`, `getTimeDifferenceInMinutes`, `getTimeDifferenceInHours`, `getDateAfter`, ...). If the unit you need has no helper, add it to `datetime.ts` first. ## Async - ❌ Call a promise without `await` and drop it (floating promise) → always `await`, or, for intentional fire-and-forget, handle the error with `.catch()` and state the intent in a comment. - ⚠️ **Exception — promise memoization (single-flight)**: when concurrent callers must share one in-flight fetch, cache **the promise, not the value**. Caching the value lets everyone else slip past the `if (!cached)` check while the first caller yields at `await` (thundering herd). Here the assignment having no `await` is correct — the promise is returned and every caller awaits it, so it is not floating. Clear the cache in `.catch()` so a rejected promise is not retained. ## Cache - ❌ Add a cache without an invalidation path → design the **fill path, the refresh path, and the clear path together**. (Real incident in `af-trade-stream`: a cache froze on the first snapshot taken in live and never refreshed.) - ❌ Bound cache size by truncating a time series (`slice(-N)`) → if accumulated state depends on the head of the series (first-seen timestamp, first buyer, per-address totals), truncation destroys it. Evict by **entry**, not by series position. - ❌ Roll back accumulated floating-point state by subtraction → `a + b - b ≠ a`, so drift accumulates. Re-fold from a checkpoint using addition only. ## Failure cases — branches and labels > **Judgment question: if I delete this branch, does behavior change?** - ❌ Create a branch because the case table has a row for it → the reason to branch is not *that the case exists* but **whether its handling differs from the fallback**. Same outcome as the fallback means it is a **label**, not logic. - ❌ Treat every case as equally weighted → judge **frequency** and **severity** separately. - frequency 0 + same outcome as fallback → **delete** - low frequency + catastrophic outcome (double execution, permanent stuck state) → **keep** - ❌ Keep a case the domain owner said cannot happen, just to fill the table → refuting them requires **evidence**; inertia is not evidence. - ❌ Keep a branch to preserve a label → the reason is usually already in the error message. If a label is genuinely needed, confirm the aggregation key is actually consumed. ## Terminal states — can you get out? > **Judgment question: if I move the entity to this state, who still queries its dangling unfinished work?** - ❌ Move an entity to a terminal state without checking for unfinished child work → check the **query condition of whatever reaps it** (cron, worker). If the terminal state falls out of that query, the work is **abandoned forever**. - ❌ Infer "unfinished" from an indirect signal → derived values like held quantity cannot count pending operations. **Count the unresolved items directly.** - ❌ Multiple exit paths each testing different conditions → share one predicate. > **Judgment question: is there a path out of this state?** - ❌ Choose a state because "when unsure, be conservative" → decide by **whether an exit path exists**. A state you cannot leave is not conservative, it is the **worst** option. - ❌ Apply a risk argument to every branch without checking → "being optimistic risks double execution" applies **only to branches that retry**. Branches that do not retry do not carry that risk. - ❌ A fallback that cannot carry the key (identifier) later processing needs → a keyless fallback creates an **unresolvable** state. If the key cannot be carried, do not enter that state. - ❌ Terminate on retry *count* → count is coupled to the schedule interval, so the same number means different things over time. Use a **deadline** or a direct measurement of the real cost. ## Accumulating fields > **Judgment question: does more than one code path write this field?** - ❌ Assign (`=`) to a field written from more than one source → funnel writes through a single method whose name carries the accumulation semantics. - ❌ Add a new write site without checking the operator at existing ones → if an existing site assigns, previously accumulated values **disappear silently**. - ❌ Ship a feature without reviewing the **field combinations it newly makes possible** → a path like "failed, retried, then succeeded" produces combinations that could not exist before. ## Manual reassembly of optional fields > Trigger: check this **only when adding or changing a field on a config/condition type that is passed through several hops.** Not a standing review item. - ❌ Copy a type into another by **listing fields by hand** → an omitted optional (`?`) field compiles clean, becomes `undefined` at runtime, and **silently falls back to the default**. Nothing throws, so a wrong result looks normal. - ✅ Prefer `...spread` so new fields propagate automatically. - ✅ If dummy defaults or snake↔camel conversion make listing unavoidable, concentrate the reassembly in **one place** so "sites to review when adding a field" are traceable from a single spot. - Test: "how many hand-written hops does this field pass through between source and destination?" — grep the whole path. ## Wiring verification > **Judgment question: is there a test that reaches the new unit through a public entry point?** - ❌ Only write unit tests that **call the new unit directly** → they cannot catch "it was built but nothing calls it". Write at least one test that injects input at a **public entry point and asserts the resulting effect**. - ❌ Loosen an assertion to make a test pass when stubs are missing → the looseness covers the hole. Do not make it green; find out **why it cannot reach**. ## Document synchronization > **Judgment question: did I grep the docs for the case/state IDs this commit changed?** - ❌ Delete or redefine a case/state and fix only the corresponding table in the canonical doc → **grep the ID and fix every prose reference too**. - ❌ Mark a case list "resolved" while leaving the state description table untouched → the earlier table is read first, which makes it worse than no update. - ❌ Land a bug-fix commit with no doc change → for every code commit, confirm whether the canonical doc needs to move with it. ## Working agreement — ask before diverging fixes - ❌ Fix a bug, design, or implementation **before the user asked for a fix** → especially when the fix could go several ways (anchor, baseline, semantics of a behavior), picking one direction unilaterally diverges from intent. - ❌ Read questions and inspection requests — "can you add logging?", "check whether there's a bug" — as a signal to start implementing → those are design/confirmation-stage signals. - ✅ On finding a problem, present **why it is a problem · when it occurs · how it could be fixed (options A/B with trade-offs)** and let the user choose. Fix after approval. - ✅ Do not change a pure function's signature or return type to accommodate observability/logging → handle it with values the call site already has. - Test: "does this fix have exactly one right answer, or does it depend on user intent?" — if it can diverge, **always ask first**. The only exception is an explicit "just fix it".