OhShii Labs review, round 2 — 6/6 · 4 findings (#28.1–#28.4): a test that cannot fail, two latent items, and the method note

[rvnt9999]

Round 2, part 6 of 6. Two check-integrity items, two small latent ones, and then what we walked
and what we deliberately did not.

---

1. tests/test_market_walks_locked.sh can never fail

Where: tests/test_market_walks_locked.sh:17, verdict region :48-55; tests/run_all.sh:138

pass() { echo -e "${GREEN}✓${NC} $1"; }
fail() { echo -e "${RED}✗${NC} $1 — $2"; }     # line 17 — echoes and nothing else
...
if [ "$OK" = "1" ]; then
  pass "market buy soaked >= 9 ETH (across immediate + pending). Was: $TOTAL"
else
  fail "Engine stopped after only $TOTAL ETH — locked-maker walk-through broken" "$RES"
fi                                              # last statement in the file

fail() increments no counter. The file declares no pass=0; fail=0, never calls
finish_test, never reads _TEST_ERRORS, and does not set -e. Its last executed command is an
echo in either branch, so the exit code is 0 whether the assertion held or not, and
run_all.sh:138 (if bash "$t"; then rc=0) records PASS.

Proven, not inferred. Replicating the file's exact verdict shape with a forced-failing input:

✗ Engine stopped after only 0.0 ETH - locked-maker walk-through broken - RES
>>> EXIT CODE: 0        # run_all.sh:138 records this as PASS

Control, running the same failing assertion through the finish_test logic instead: exit code
1. So the harness can express failure; this file does not use it.

What it was guarding. The test asserts the matching engine walks through a locked/protected
maker and soaks ≥ 9 ETH across immediate fills plus pending matches. If the engine regressed to
stopping at the first protected maker, this test prints ✗ and reports PASS.

Why it matters beyond one file. This is the second dead check on matching-engine
walk-through behaviour. tests/MatchingEngine.test.mo does not compile (M0151, missing
beneficialOwner) — reported in our #10 and independently by Menese in #2, who also found the
root cause we lacked (scripts/lint-ratchet.sh:58 globs src/backend only, so tests/ is never
type-checked). The two independent checks on how the engine traverses protected makers are both
non-functional, and they fail in different ways: one does not build, one cannot fail.

Fix. One line, using the idiom 36 of the other 62 shell tests already use: add
pass=0; fail=0, increment in fail(), and end the file with exit $fail.

---

2. run_all.sh computes the failing-assertion count and then discards it

Where: tests/run_all.sh:147-148, :155-160

passed=$(LC_ALL=C grep -ac '✓' "$log" || true)
failed=$(LC_ALL=C grep -ac '✗' "$log" || true)
...
if [ "$rc" -eq 0 ]; then
  echo -e "  ${GREEN}PASS${NC} (${dur}s · ${passed} assertions)"    # `failed` never read

On the PASS branch, failed is never consulted. A test that prints ✗ lines but exits 0 —
i.e. item 1 — is reported as PASS (Xs · 0 assertions), with the evidence of its own failure
sitting in a shell variable one line above.

This is not the cause of item 1; it is the reason item 1 is invisible. Two one-line assertions
catch it and the next one: failed -eq 0, and passed -gt 0 to flag a test that asserted
nothing at all.

In fairness to the harness, and because we checked before filing: tests/_lib.sh:259-273
(finish_test) is correct — it reads _TEST_ERRORS and exits 1. The 36 tests using the
self-contained pass/fail counters end with exit $fail, which is correct. run_all.sh:50
discovers tests dynamically (ls test_*.sh), so there is no drift list. run_all.sh:103 gates
correctly on mops test's exit code. We formed and killed two hypotheses here before arriving at
the one real defect — see the method note below.

Context, not a finding: there is no .github/ directory and no CI configuration anywhere in
the tree, and package.json declares no test script. Every gate in this repository runs only
when a human remembers to run it. That is the precondition that lets a dead test and a red suite
persist, and it is why we think items 1 and 2 are worth the two lines each.

---

3. Two actor-level constants are missing transient, so no upgrade can retune them

Where: main.mo:128 (MARGIN_CASH_SETTLE_USD, currently unused), :3634 (EPISODE_CAP); the rule is documented at :87-90

Under persistent actor with enhanced orthogonal persistence, every let/var in the actor body
is implicitly stable — which is why the tree contains zero stable keywords and why
transient is used deliberately throughout. The rule is documented at main.mo:87-90 and called
load-bearing in those exact words:

transient is load-bearing: a plain let in a persistent actor is implicitly STABLE, so an
edited literal would be silently overwritten by the old stored value on --mode upgrade and
the flip would never land. Transient re-evaluates the literal on install AND upgrade.

We counted the whole actor body rather than eyeballing it, because the naive version of this
finding would be wrong. src/backend/main.mo has 199 transient let declarations and 85
plain let ones — but 83 of those 85 are state containers (Map.empty, List.empty,
Accounts.emptyState(), OrderBook.emptyStore(), MarginEngine.emptyState() …), which should
be implicitly stable, because they are the data. That is correct by design and not a finding.

Exactly two of the plain lets are scalar tuning constants:

main.mo:128    let MARGIN_CASH_SETTLE_USD : Nat = 5_000_000_000;   // $50 at 10^8
main.mo:3634   let EPISODE_CAP : Nat = 200;

Both are frozen at their first-installed value, so a --mode upgrade that edits either literal
has no effect — the exact failure the comment at :87-90 warns about. EPISODE_CAP is a live
tuning knob; MARGIN_CASH_SETTLE_USD is currently unused, which is presumably why neither was
noticed.

This corrects a clearance in our own audit notes, which asserted the constant set was uniformly
transient. It is INFO/LOW — but it is the kind of thing discovered during an incident, when the
retune does not take.

---

4. subPendingQty clamps where non-negativity is a real invariant, while its twin fails closed

Where: main.mo:1152-1154; contrast subReserved at :1062-1065, and SafeMath.mo:13-16

SafeMath.mo's own header states the rule:

Use it ONLY where clamping a negative result to 0 is the INTENDED semantics […] Do NOT use it
to silence a subtraction whose non-negativity is a real INVARIANT — there, the trap is a
feature (it surfaces a violated invariant instead of masking it with a 0).

subReserved follows that rule: on an underflow it fails closed and logs the desync.
subPendingQty, its structural twin, calls SafeMath.subOrZero and silently produces 0. If the
pending-quantity lock ever desyncs, one path shouts and the other hides it.

We read all 26 subOrZero call sites before filing this. Twenty-five are legitimate clamps
whose zero case is the intended semantics and is documented at the site — headroom and pagination
arithmetic, running balances, and so on. This is the one that is not. Reporting it with the other
25 named is the point: it is one of two siblings that got different treatment, not a pattern.

INFO/INFO today — we could not construct a reachable desync. It is filed because the next
change to the pending-match accounting is the one that would need the alarm.

---

Method note, and what we did not cover

What this round was. A commit-point audit: on the IC a canister is single-threaded per
message, not per call, so every await commits the state written so far and lets other
messages run before the continuation resumes. We walked every await in the first-party tree and
asked, at each, what was read before the yield and acted on after it. Plus a check-integrity pass
over the repository's own tests.

Inventory (comment- and string-stripped; a raw grep -c await returns 95 because this
codebase comments heavily about awaits, and 29 hits are prose):

await occurrences, first-party src/	66 — 64 real, 2 await* (inlined, not commit points)
by file	backend/main.mo 53 · arb/main.mo 10 · bridge/main.mo 2 · fuel-mock 1
by context	42 private helpers · 19 public shared updates · 3 composite queries · 2 system
finally blocks in the whole first-party tree	1 (main.mo:7440)
entrypoints	112 public shared · 132 public query · 2 composite query · 7 system func

The most useful thing we found is a negative, and we want it on the record. The trading core —
placeLimitOrder, placeMarketOrder, swap, deposit, withdrawal, liquidation,
MatchingEngine, OrderBook, AMM, LiquidityManager, VaultMath, MarginEngine,
BorrowEngine, the insurance tranche — contains zero awaits. Every one of those runs to
completion inside a single message, and single-message execution on the IC is atomic. The classic
"read a balance, yield, act on the stale snapshot" class is structurally absent from every path
that moves value between users. None of our 26 findings this round is in the order book, the
matching engine, the liquidator, the vault or the insurance tranche.

That is a design property, and it is worth defending explicitly in review, because the first
await added to any of those functions converts a class of impossible bugs into possible ones
and nothing in the build will say so.

Things we checked and found sound, listed because a finding list without its rejects cannot
be reviewed: the Bridge's claiming guard is a genuine check-and-set (bridge:233 → :239, no
yield between); claim's continuation increments relatively rather than setting absolutely
(bridge:248); creditAndRegister credits from the DEX's own high-water, never the
caller-supplied amount (main.mo:5254), with an inverse-divergence guard at :5271;
playDepositReserve (main.mo:5670-5707) contains zero awaits, so its read-cap-debit is one
message; tickShipEvents' finally (:7440-7442) is correct on every axis and its flag
acquisition is a genuine check-and-set (:7245-7246); the arb's _inFlight is transient, so an
upgrade clears it, and a trap in runTick is catchable because runTick is a real async in its
own call context; tickOnce and burnTreasuryIcpToCycles both check authorization before their
first await; the Bridge's inspect is not stricter than its method bodies. On the arithmetic
side we read every Nat subtraction in the 17 library files and traced each to its guard —
including PriceFeed.parseLeadingFloat, which subtracts on an operand taken from an
HTTPS-outcall body from an external venue and is correctly fenced by Char.isDigit.

What we did not cover, named rather than implied: the OQL engine internals
(oql/Auth.mo, Executor.mo, SecondaryIndex.mo); most of src/frontend/src/*.js; the
candid/ directory; mops.lock dependency pinning; and the gate scripts under scripts/ and
ops/ — which after #21 we consider uncleared, not clean. We touched no live deployment; the
only thing we executed was this repository's own unit suite on a local checkout.

Two of our own instruments were wrong before they were right, which is the same failure mode
we were auditing for and is why we re-ran everything: a raw await grep over-counted by 44%, and
our first function-slicer silently dropped every multi-line signature — which is exactly the
classification that decides whether the three composite-query awaits are commit points at all.
We also formed, and then killed, a much larger version of item 1 above: we believed 41 of 63
shell tests could not fail, and the tell was our own counter reporting zero assertions in
files we were claiming had unprotected ones. 36 of them use a second, sound harness. The finding
collapsed from 41 files to one.

---

That closes round 2. Cross-references to the other teams' work are in each part; where our
earlier clearances have been corrected — #21 on lint-ratchet.sh, #17 on the oracle-trim remedy,
#15 on settleNettedPair, #18 on the archive delete loop, #3 on verifyChain, #14 on
cancel/replace — we have accepted the correction rather than re-argued it, and said so in 1/6.

We remain happy to open PRs; several of these are one to a few lines and reuse patterns already
in the tree (the finally idiom at :7440, the _captureEpoch bump, Error.isCleanReject,
stats() as a controllership-free health probe, exit $fail).

— Ravenith, OhShii Labs