docs(contributing): codify thin-client architecture as a hard rule - #128
Merged
Merged
Conversation
Document the rule that surfaced from the May 2026 `07-send-success` incident: the App is a thin client. The private key + UI rendering live in the App; everything else (balance, num_sends, transaction history, server capabilities, account proofs, commitment lookups) lives on `zk-coins/node` and the App must fetch the authoritative value before every operation that depends on it. The local-store `numPubkeys` counter that produced the `prove_account_update_with_in_and_out_coins_and_sources failed` class of bugs is called out explicitly so the next maintainer doesn't repeat the assumption that App-side derivation is "free". Update the State Management subsection to point at the new rule and trim the list of authoritative store fields to those that genuinely belong in the App (xpriv, address, UI flags) instead of the previously-listed `transactions` (server-owned).
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Make the thin-client architecture rule explicit in `CONTRIBUTING.md` so the next contributor can't accidentally re-introduce the class of bugs that produced the May 2026 `07-send-success` E2E failure.
What this rule says
The App's only responsibilities are:
Everything else (balance, `num_sends`, transaction history, capabilities, account proofs, commitment lookups) lives on `zk-coins/node`. The App MUST fetch the authoritative value from the node before every operation that depends on it.
Why this is a hard rule, not a guideline
May 2026 incident: the App used a local `numPubkeys` counter from the Zustand store. The counter resets to 0 on every fresh page load and Playwright retry. After Alice's first successful send (server-side `num_sends → 1`), every retry signed the next send with `pk(0)` instead of `pk(1)`, violating the in-circuit AccountUpdate continuity constraint at `program-plonky2/src/circuit/main.rs:615-623`. Three correct server-side fixes (PR #129 `num_sends` counter, PR #132 server-owned `commitment_public_key`, PR #135 canonical SMT) all shipped before anyone noticed the App was still signing with the wrong index. The fix (PR #127, `api.balance` hydration before every signed request) is correct, but the rule prevents the next instance.
What changed in CONTRIBUTING.md
New section "Architecture Principle — Thin Client" placed between Project Structure and Git Workflow, with explicit "MUST / MUST NOT" bullets:
State Management subsection updated: the previous list of store fields included `transactions` (which is server-owned). The list now only enumerates xpriv + address + UI flags; ground-truth fields are explicitly called out as on-demand fetches.
Test plan