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.

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.
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.

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.

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.

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.

// 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.

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:

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.

Structure

Layers, with dependencies pointing one way only: main.mo → mixins/ → lib/ → types.mo.

• 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.

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 — recipes per invariant, and the migration each one needs
• 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