reviewing-motoko

# Reviewing Motoko This skill judges whether Motoko code is **well designed**. It does not teach the language — load `writing-motoko` for syntax, `mo:core` APIs, and mechanical rules, and `migrating-motoko-actors` before changing anything that is stable state. Four questions decide a review, in this order — the first three because each can invalidate the next, the fourth last because it is the least consequential: 1. **Is every invariant the code relies on encoded in a type?** Where the type system *can* rule out a bad state, leaving that to a runtime check or a comment is a defect, not a style preference. 2. **Is state declared with the right persistence?** Under enhanced migration, stable and transient are separate worlds. Blurring them produces state that silently stops updating. 3. **Does the code live in the right files?** `types.mo` / `lib/` / `mixins/` / `main.mo` is a contract about what may depend on what, not a folder convention. 4. **Does it read like Motoko?** Contextual dot notation, inferred implicits, no annotation the call already supplies, no classes. `mops check --fix` handles most of this; only what it cannot see is the reviewer's job. ## Procedure **Run the mechanical pass first** — do not spend review attention on what the compiler already reports. ```bash mops check --fix ``` Its style auto-fixes (M0236 dot notation, M0237 redundant implicits, M0223 redundant instantiation) only fire where `[moc] args` enables `-W` for them; `writing-motoko`'s `references/project-setup.md` has the setup. Where they are off, the **Conventions** findings below are all yours to make by hand. Then read in this order — outside in, because a misplaced file makes every type inside it suspect: 1. `mops.toml` — is there a `[canisters.*.migrations]` chain? If not, finding **B1** below outranks everything else. 2. **Structure** — the file layout, then each file's imports. 3. `types.mo` — every type declaration. This is where most findings are. 4. **State** — the actor fields in `main.mo`, and what each `include` passes. See [references/state-and-persistence.md](references/state-and-persistence.md). 5. **The API boundary** — every `public shared` / `public query` function: its parameter types, its return type, its authorization. 6. **Bodies** — last, and only for the smells under **Maintainability** below. ## Severity Report findings with one of three levels. Do not inflate: a reviewer who calls everything a blocker gets ignored. | Level | Meaning | | ------------ | --------------------------------------------------------------------------------------- | | **Blocker** | Can corrupt or silently lose state, or lets a caller reach an unauthorized action. | | **Warning** | An invariant the type system could enforce is left to runtime, a comment, or a convention. | | **Note** | Readability, duplication, or naming. Fix while you are in the file; do not gate on it. | State the concrete failure, not the rule. "`status : Text` accepts `"complete"`, and the three call sites spell it `"completed"`" beats "prefer variants over strings". ## Type encoding The rule: **if a value has a fixed set of shapes, the type must have that set of shapes.** Full recipes, including how to carry each change through a migration, are in [references/type-encoding.md](references/type-encoding.md). ### T1. `Text` for a closed set of values — **Warning**, **Blocker** if it gates authorization The most common defect in generated Motoko. Every producer and consumer agrees on spelling by convention and the compiler checks nothing — no exhaustiveness, no error on a typo, no error when a new state is added and half the call sites are not updated. ```motoko public type Task = { id : Nat; var status : Text }; // WRONG public type TaskStatus = { #open; #claimed : Principal; #done : { at : Time.Time } }; ``` Variants are shared, so this reaches the frontend as a discriminated union too. **Flag `Text` (or `Nat`, or `Bool`) whenever the code, a comment, or a doc lists the permitted values** — `== "..."` comparisons, a `switch` on a `Text` with a `case _`, role names held as `Text`. Keep `Text` for genuinely open sets, and for strings an external system owns. ### T2. Correlated fields — **Warning** Two fields that are only valid together belong in one variant. A `Bool` plus an optional payload has four representable combinations and two meaningful ones. ```motoko // WRONG — { hasDeadline = true; deadline = null } compiles and means nothing { hasDeadline : Bool; deadline : ?Time.Time } // RIGHT { deadline : { #none; #due : Time.Time } } // or simply `deadline : ?Time.Time` ``` If a `?T` already says it, drop the `Bool` — do not introduce a variant that only restates `?T`. ### T3. Wrong cardinality — **Note**, **Warning** when callers loop defensively Use the type that matches the real count. A `[T]` that is always empty-or-one makes every caller write a loop or an `if size == 1`; a `?T` tells them there is nothing to iterate. ### T4. Errors as `Text` — **Warning** `Result<Ok, Text>` forces every caller to string-match to tell one failure from another. Make `Err` a variant with the data each case needs. See the `Result` section of `writing-motoko` for the full rule, including when to return `?T` and when to trap instead. ### T5. `var` fields leaking into the API — **Blocker** (it does not compile) or **Warning** (it compiles but over-shares) A record with a `var` field is not shared, so it cannot be returned from a public function. Where a project already has the internal/view split (`Task` internal, `TaskView` shared), check that: - no public signature mentions the internal type; - the view is **narrower** than the internal record, not a mechanical field-for-field copy. A view that re-exports an internal `Principal` list, an audit trail, or another user's data is an information leak that compiles. ### T6. `case _` swallowing variants — **Warning** A catch-all arm is what turns "adding a variant tag is a compile error everywhere it matters" into "adding a variant tag silently takes the default branch". Keep `case _` only where the arms are genuinely uniform and the set is expected to grow (e.g. mapping many tags to one label). Anywhere a new tag would need new behaviour, enumerate the tags. ### T7. Transparent aliases sold as safety — **Note** `type UserId = Principal` is a documentation alias: any `Principal` still type-checks where a `UserId` is expected. That is fine, and usually the right trade. Just do not treat it as an enforced distinction — and if two different id types are genuinely being confused at call sites, a one-field record (`{ userId : Principal }`) or a variant tag (`#user(Principal)`) is what actually enforces it, at the cost of changing the Candid shape. ## State and persistence Under enhanced migration (EM) the actor's stable fields are declared **type-only** and every initial value comes from the migration chain. That separation is the point: it is what makes stable state impossible to initialize from something transient. Details in [references/state-and-persistence.md](references/state-and-persistence.md). ### B1. No enhanced migration — **Blocker** No `[canisters.<name>.migrations]` chain in `mops.toml` means pre-EM persistence, where stable fields carry inline initializers — so a stable field can be initialized from a transient one: ```motoko transient let defaultQuota = 100; let quota = defaultQuota; // stable; the initializer runs ONCE, on first install ``` Change `defaultQuota` to `250`, deploy, and `quota` is still `100` — forever, no warning. Point at `migrating-motoko-actors` for the conversion; everything below assumes EM. ### S1. Inline initializer on a stable field — **Blocker** Under EM this is M0250 / M0014, so it will not compile — but it appears constantly in code copied from pre-EM examples and in half-finished refactors. The fix is never to add `transient`: move the initial value into the pending migration's `NewActor`. ### S2. `transient` misused — **Warning** both ways Ask of every actor and mixin field: *if this is lost on upgrade, is the app still correct?* - **Missing `transient`**: caches, derived indexes, rate-limit counters, and anything rebuildable are being persisted. They then have to be migrated forever, and a stale cache survives the upgrade that was supposed to clear it. - **Wrongly `transient`**: durable domain data silently resets to its initializer on every upgrade. Look for `transient` on anything a user would expect to still be there after a deploy. ### S3. `var` actor field shared with a mixin — **Blocker** `var` parameters pass by value, so the mixin mutates a copy and the actor's field never changes. Wrap it in a record and pass the record. Also flag a fresh record literal at the `include` site (`include Api({ var n = state.n })`) — each mixin then gets its own copy. See `writing-motoko`. ### S4. Stable state inside a `mixin` block — **Blocker** Every bare `let`/`var` at the top of a `mixin` is implicitly stable and traps at runtime with `IC0503`. State comes in as a parameter; constants go in a module; only `transient` is allowed inline. ### S5. Migration hygiene — **Blocker** - A migration file importing anything other than `mo:core/...` — the chain replays forever, so a project import makes it wrong the moment that type changes. (`caffeineai-lints` catches this.) - More than one pending migration in a build, or an edit to a migration that predates this build. - A stable field in `main.mo` that no migration in the chain supplies (M0254 / M0267). An identity migration body is a **Warning** — the change was stable-compatible, so delete the file. [Full table](references/state-and-persistence.md#migration-hygiene). ## Structure Layers, with dependencies pointing one way only: `main.mo` → `mixins/` → `lib/` → `types.mo`. | File | Holds | Must not hold | | --------------- | ----------------------------------------------------- | -------------------------------------- | | `types.mo` | Type declarations; small pure helpers on those types | State, endpoints, business rules | | `lib/*.mo` | Domain logic as stateless modules, state as parameters | Public endpoints, actor fields | | `mixins/*.mo` | Public endpoints: authorize, delegate, map to a view | Domain logic, stable state | | `main.mo` | State declarations and `include`s | **Any public method**, any logic | | `migrations/` | The frozen chain, one module per file | Project imports | - **A1 — Blocker/Warning:** a public method in `main.mo`. Move it to a mixin. - **A2 — Warning:** a monolithic file. One `.mo` holding types, state, logic, and endpoints together. - **A3 — Warning:** domain logic in a mixin body. If an endpoint's body is more than authorize → delegate → map, the middle belongs in `lib/`. - **A4 — Warning:** wrong dependency direction — `lib/` importing a mixin, or `types.mo` importing either. - **A5 — Note:** a mixin that is not a feature. Mixins split by *feature* (`mixins/Bookings.mo`), not by verb (`mixins/Getters.mo`). - **A6 — Warning:** over-broad state injection. A mixin receiving state it never touches widens what a change can break; pass only the slices it uses. ## Maintainability Most of this is caught in the last, bodies pass. The exception is **M1**, the authorization blocker — it is found during the API-boundary pass (step 5), not with the body smells below. - **M1 — Blocker:** a `public shared` function that does not check `caller`. Every state change and every read of another user's data needs authorization on the backend. - **M2 — Warning:** `Nat` subtraction that is not provably non-negative at the operation. It traps. - **M3 — Warning:** a trap where the caller could have recovered. Traps roll back the message and reach the client as an opaque reject; caller-fixable failures belong in a `Result` error variant. - **M4 — Note:** duplicated internal→view mapping. One `toView` per type, in `lib/` or `types.mo`. - **M5 — Note:** magic values — bare literals for limits, quotas, or durations. Name them in a module. - **M6 — Note:** a comment asserting an invariant the type could carry. Every "must be non-empty" / "only set when …" comment is a type-encoding finding in disguise. Where Motoko cannot express the invariant, keep the comment but validate the value once at the boundary rather than re-checking it at every use. ## Conventions Least consequential of the four axes, and `mops check --fix` fixes most of it where the `-W` flags are on — so raise these only after the passes above, and only where the tooling cannot. `writing-motoko` has the rules; this is what to look for. - **C1 — Note:** a module-function call where the function takes `self` — `List.add(list, x)`, `Principal.toText(p)`. M0236, auto-fixed. - **C2 — Note:** an implicit passed explicitly (`map.add(Text.compare, k, v)`), or a type instantiation inference already resolved. M0237 / M0223, auto-fixed. - **C3 — Warning:** type annotations on an inline `func` passed as a **call argument** — `xs.filter(func(x : Nat) : Bool { x > 1 })`. No diagnostic catches this, so it is the reviewer's job: the call already fixes both types, and the annotation duplicates them so they can drift. Write `xs.filter(func x = x > 1)`. Instantiate the *call* (`map<In, Out>`) when M0098 demands it — never the lambda. One exception: `: async ()` on an async callback is load-bearing. - **C4 — Blocker:** a `class`, or any object holding functions, used as actor state. Functions are not stable, so it fails with M0131 (`declared stable but has non-stable type`). Outside state a class still is not the idiom here — a module taking `self` plus dot notation gets the same call sites and stays stable. ## Refactoring Design fixes in Motoko are not free: **changing a type that is reachable from a stable actor field requires a migration**, and the chain is one-way. Before proposing a type change, establish whether that type is stable state. 1. `mops check --fix` first, so the mechanical noise is gone. 2. **Batch the stable-state type changes** — one pending migration per build, so they land together. 3. **Write the migration** (`migrating-motoko-actors`). For `Text` → variant, decide explicitly what an unrecognized string becomes rather than inventing an `#unknown` tag. 4. **Move code before changing it** — relocating, then editing, keeps each diff reviewable. 5. `mops check --fix` until clean, then `mops build` once. ## Reporting Group findings by severity, blockers first. Per finding: `file:line`, the level, the concrete failure it permits, and the change. Suppress findings the compiler already reports. ```text Blocker src/backend/mixins/Tasks.mo:34 (M1) `claimTask` never reads `caller` — any principal can claim any task. Warning src/backend/types.mo:12 (T1) `Task.status : Text` is a closed set, so the comparison at Tasks.mo:51 spells it "complete" and is never true. Stable state — needs a migration. ``` If nothing is wrong, say so plainly. Do not manufacture findings to fill a report. ## Additional References - [references/type-encoding.md](references/type-encoding.md) — recipes per invariant, and the migration each one needs - [references/state-and-persistence.md](references/state-and-persistence.md) — why EM matters, the stable/transient question, migration hygiene - `writing-motoko` — language mechanics, `mo:core` APIs, `Result`, dot notation - **mops tooling**: Load `mops-cli` for `mops.toml` configuration and `mops check`/`mops build` details - `migrating-motoko-actors` — writing the migration a type change requires - `troubleshooting-motoko-migrations` — compatibility diagnostics that do not match the source