Skip to content

Review request: critique the 20 PRs merged in #94–#114, plus #115–#117 and the polarity guard #112

Description

@HodlDee

What this is

Twenty pull requests landed between #94 and #114 — three factual corrections, three bugs, two consistency passes, four new guides, one new section, and a rewritten glossary. main builds clean with every guard passing and the offline artifact digest unchanged.

A second batch has landed since — three new guides (#115, #116, #117) and a build guard that cannot be pushed yet. It is in the round-two comment below, and the guard is the piece I most want looked at before it goes up.

This issue is a request to attack the work, not to admire it. Below is where I think the weak points are, ordered by how much a wrong call would cost.

Full change log with reasoning per PR: review/CHANGES.md is kept locally alongside the working notes; the merged PR descriptions carry the same reasoning.

Judgement calls worth a second opinion

These are decisions, not facts. Each could reasonably have gone the other way.

1. Softening "a chip you cannot audit" — #99.
supply-chain-and-vendor-risk presented secure element versus open silicon as a clean binary. Trezor's Safe 7 ships TROPIC01, which Trezor describe as "the world's only independently auditable secure element". I rewrote the passage to state the historical position, name the exception, and say one device is not a trend. Is one product enough to move that framing, or should it have stayed a binary until the pattern is established?

2. The effort field's ten hand-set values — #104.
"About 45 minutes" meant reading time on some guides and workbench time on others. I could not derive which: the prerequisites block misses genuine tasks, reading rate is useless because the estimates are deliberately unhurried, and the two signals disagree on exactly half the library. So ten guides have effort set by hand — own-node-connection, passphrase-setup, seed-backup-metal, air-gapped-psbt-workflow, coldcard-advanced-features, exchange-account-security, satscard-setup, three-dice-seed, bip85-child-seeds, inheritance-plan. Those are editorial calls. multisig-key-geography and duress-and-coercion I left as "read" and could argue either way.

3. Glossary exclusions — #110 and #93.
238 terms written in-house, replacing 514 fetched from a third party. I dropped trading metrics, meme vocabulary, altcoins and ideology entries on the grounds that a glossary attached to custody guides has no business defining "lambo" or the 200-week moving average. That is the most opinionated thing in the whole batch. #93 lists 15 borderline terms still undecided.

4. Qualifying Wasabi's CoinJoin cell — #100.
The client still speaks WabiSabi; zkSNACKs stopped running a coordinator in June 2024. I changed a plain tick to a partial with a note. Arguably it should still be a tick, since the capability exists.

Factual claims to re-derive

I verified each of these; independent confirmation is worth more than my word.

New writing — is it right, and does it sound like the site?

Four guides and one section, roughly 7,000 words in the house voice. Accuracy matters more than style, but both are fair game.

  • seed-to-key — BIP39 and BIP32 mechanics. The hardened-derivation section is the part to check: the claim that an xpub plus one normal child private key reconstructs the parent is load-bearing and stated as fact.
  • verify-a-download — GPG verification. Check the commands still match what each project publishes.
  • inside-the-device — secure elements. Check the attempt-counter framing is the right emphasis.
  • how-custody-fails — Mt. Gox, Quadriga, Celsius, FTX. Figures are deliberately approximate; check the Quadriga characterisation is fair, since the OSC findings and the popular account differ.
  • Dusting section in chain-analysis-heuristics — check it belongs where it was placed.

Structural changes to shared code

Not judgement calls, but a reviewer should know these landed — and whoever works next will meet them.

callout() takes a third argument. It is now callout(title, body, level = "h3"). Used 115 times; only two pass anything (#103), and the default preserves existing behaviour. The stylesheet already matched .sc-callout h2 and h3 in one rule, so an h2 callout renders identically.

New source file: build/glossary.mjs (2,374 lines, 238 terms). Consumed by build/content.mjs, which renders the cards into glossary.html at build time. That is a change in kind: the #term- anchors now exist in the HTML and resolve with scripting disabled, which they never did before.

docs/assets/js/site-refresh.js lost 167 lines and gained 31. It no longer fetches the lexicon; it reads the rendered cards back out of the DOM to build the search index. Search and the letter filter became enhancements over a page that already works without them. Note this file is hand-maintained, not generated (CONTRIBUTING §4) — so it is one of the few places in docs/ where the diff is authored rather than rebuilt.

docs/assets/css/site-refresh.css changed in two places, also hand-maintained:

build/tools/assert-no-fetch.mjs allowlist shrank. btclexicon.com is gone, so the guard itself now proves no page requests it. Worth confirming you are happy the guard is the right place for that assurance.

LICENSING.md gained a row for build/glossary.mjs — MIT for the module code, CC BY 4.0 for the definitions, mirroring how build/guides.mjs is recorded. Given the project's provenance discipline, this is the line most worth a second pair of eyes.

ASSET_VERSION is now derived, not typed (#113). It was a hand-maintained string stamped onto every versioned stylesheet and script link — the only thing telling a browser a changed asset is worth re-fetching. Forgetting to bump it shipped a fix to new visitors and to nobody who had been to the site before, silently, because the build passes and the page looks right to whoever made the change. It is now a truncated SHA-256 over the four files it stamps.

Three things about it are worth a reviewer's attention:

  • The asset list is written out rather than derived from the ASSET_QUERY regex beside it. Adding a file to one and forgetting the other should be a visible mismatch rather than a silent one — but it does mean two lists to keep in step, which is a trade you may disagree with.
  • It is content-addressed, not monotonic. Reverting an asset restores its previous version string rather than minting a new one. Correct for a cache key; surprising if you expected it to only increase.
  • Determinism was the constraint, since the reproducibility guard rebuilds and compares. Verified: three consecutive builds produce an identical version, appending a byte to either site-refresh.css or site-refresh.js changes it, and reverting restores the original.

This one is not hypothetical. The mechanism failed twice while the rest of this batch was being written — #94's mobile fix and #110's rewritten site-refresh.js both appeared broken in a browser until the constant was bumped by hand — and it caused the only merge friction, since three PRs bumped it and each rewrites the query string on eleven pages.

Heading levels on the root pages (#114). A final sweep found the guide finder on guides.html going from h2 "Guide finder" straight to h4 category labels, with a nested h5 — the same defect fixed on two guides in #103, missed because that audit only walked docs/guides/. Shifted to h3/h4.

The catch is worth noting: the stylesheet targeted .sc-chip-group h4, .sc-chip-group h5 by tag, so the shift would have silently unstyled three headings. The selector moved with them and all four render identically afterwards — verified in the browser rather than assumed.

Same PR gave contact.html and merch.html a meta description, the only two root pages without one. content.mjs defines one for contact but the renderer only uses that field for guide pages, so it was never emitted.

Deliberately left: those two pages still go h1 → h4, because the shared footer's section headings are h4 and a parked page has no content heading between. Fixing it means touching every page on the site to correct the outline on two temporary ones. Disagree with that if you think the footer headings are wrong in themselves — it is the one call in this PR I am least sure of.

Commit authorship. Every commit in this batch is authored and committed as 130502840+HodlDee@users.noreply.github.com, matching the repository's existing history. An earlier push was correctly rejected by the account's email-privacy setting; the commits were rewritten before anything reached GitHub.

Read #107 from main, not from its diff

PR #107's diff does not show what landed, and a reviewer working from it would review the wrong content.

Its branch collided with main two ways. #108 had inserted its guide at the same point in the guides array, and #99 had rewritten the exact sentence in supply-chain-and-vendor-risk that #107 adds a link to — four interleaved conflicts in build/guides.mjs.

Rather than hand-resolve those, I re-applied the guide onto current main and force-pushed. The substantive difference: the inline link now sits on #99's improved wording rather than reverting it to the older "by construction, a chip you cannot audit" sentence, which is what a mechanical resolution would have produced.

So for inside-the-device, read build/guides.mjs on main rather than the PR diff. Everything else merged without source conflicts and its diff is accurate.

Things I got wrong along the way

Recorded because they show where my judgement was weakest.

  • Claimed the four-step path never addressed the empty-restore failure. It does — how-wallets-find-coins covers it well. I had planned a companion guide that would have duplicated it.
  • Claimed there was no sourcing convention. There is: <h2>Sources</h2> in the entropy guides, sc-source-note on device guides.
  • Called sparrow-first-wallet thin twice on word count alone. It is tight, not thin — the metric was measuring the wrong thing.
  • Filed Passport Prime's missing microSD as an error. Foundation's own spec confirms the site was right; my first search had conflated two pages.
  • Under-scoped the glossary twice, at 51 then 152 terms, by bucketing the remainder and treating the leftovers as noise without reading them.

Still open

  • Glossary: record what was excluded, and decide on 15 borderline terms #93 — 15 borderline glossary terms. Zero-knowledge proof is the one I would add regardless; the glossary page's own hero copy promises it.
  • exchanges.html says "Links checked August 6, 2026" — one sentence conflating a link check with a product-detail check, which decay at different rates.
  • contact.html shows "Coming soon" and is linked 271 times from 69 pages, including the homepage's twelve-service "Need Help?" section. Deliberate, and the largest gap between what the site promises and what it delivers.
  • 41 "Image to come" placeholders across 38 guides. (Corrected from 37/34 — recounted against figureSlot() in build/guides.mjs.)

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions