From 3b7293742a7cebe57dfff976fc845836160feb03 Mon Sep 17 00:00:00 2001 From: Marti Date: Wed, 13 May 2026 17:20:22 +0000 Subject: [PATCH] chore: skills from 1y of PRs --- .../skills/advice-provider-hygiene/SKILL.md | 86 +++++++++++++++++++ .../assert-specific-error-in-tests/SKILL.md | 52 +++++++++++ .../skills/cheap-masm-equivalents/SKILL.md | 58 +++++++++++++ .claude/skills/checked-arithmetic/SKILL.md | 38 ++++++++ .../skills/conversion-method-naming/SKILL.md | 56 ++++++++++++ .../decouple-component-from-storage/SKILL.md | 47 ++++++++++ .../domain-newtypes-over-primitives/SKILL.md | 54 ++++++++++++ .claude/skills/felt-construction/SKILL.md | 43 ++++++++++ .claude/skills/intra-doc-links/SKILL.md | 53 ++++++++++++ .../skills/lowercase-error-messages/SKILL.md | 57 ++++++++++++ .claude/skills/masm-error-constants/SKILL.md | 60 +++++++++++++ .../masm-explicit-stack-inputs/SKILL.md | 50 +++++++++++ .../skills/masm-locals-over-globals/SKILL.md | 49 +++++++++++ .claude/skills/masm-named-literals/SKILL.md | 49 +++++++++++ .../skills/masm-rust-constant-parity/SKILL.md | 45 ++++++++++ .../non-exhaustive-public-types/SKILL.md | 68 +++++++++++++++ .../skills/parametrize-related-tests/SKILL.md | 54 ++++++++++++ .claude/skills/preserve-error-source/SKILL.md | 52 +++++++++++ .../private-fields-with-accessors/SKILL.md | 47 ++++++++++ .../skills/return-error-not-panic/SKILL.md | 77 +++++++++++++++++ .../skills/u32-assert-before-u32-ops/SKILL.md | 46 ++++++++++ .claude/skills/use-bon-builder/SKILL.md | 59 +++++++++++++ .claude/skills/use-test-fixtures/SKILL.md | 54 ++++++++++++ .../skills/validate-in-constructor/SKILL.md | 64 ++++++++++++++ .../workspace-shared-dependencies/SKILL.md | 56 ++++++++++++ 25 files changed, 1374 insertions(+) create mode 100644 .claude/skills/advice-provider-hygiene/SKILL.md create mode 100644 .claude/skills/assert-specific-error-in-tests/SKILL.md create mode 100644 .claude/skills/cheap-masm-equivalents/SKILL.md create mode 100644 .claude/skills/checked-arithmetic/SKILL.md create mode 100644 .claude/skills/conversion-method-naming/SKILL.md create mode 100644 .claude/skills/decouple-component-from-storage/SKILL.md create mode 100644 .claude/skills/domain-newtypes-over-primitives/SKILL.md create mode 100644 .claude/skills/felt-construction/SKILL.md create mode 100644 .claude/skills/intra-doc-links/SKILL.md create mode 100644 .claude/skills/lowercase-error-messages/SKILL.md create mode 100644 .claude/skills/masm-error-constants/SKILL.md create mode 100644 .claude/skills/masm-explicit-stack-inputs/SKILL.md create mode 100644 .claude/skills/masm-locals-over-globals/SKILL.md create mode 100644 .claude/skills/masm-named-literals/SKILL.md create mode 100644 .claude/skills/masm-rust-constant-parity/SKILL.md create mode 100644 .claude/skills/non-exhaustive-public-types/SKILL.md create mode 100644 .claude/skills/parametrize-related-tests/SKILL.md create mode 100644 .claude/skills/preserve-error-source/SKILL.md create mode 100644 .claude/skills/private-fields-with-accessors/SKILL.md create mode 100644 .claude/skills/return-error-not-panic/SKILL.md create mode 100644 .claude/skills/u32-assert-before-u32-ops/SKILL.md create mode 100644 .claude/skills/use-bon-builder/SKILL.md create mode 100644 .claude/skills/use-test-fixtures/SKILL.md create mode 100644 .claude/skills/validate-in-constructor/SKILL.md create mode 100644 .claude/skills/workspace-shared-dependencies/SKILL.md diff --git a/.claude/skills/advice-provider-hygiene/SKILL.md b/.claude/skills/advice-provider-hygiene/SKILL.md new file mode 100644 index 0000000000..463eb65bd1 --- /dev/null +++ b/.claude/skills/advice-provider-hygiene/SKILL.md @@ -0,0 +1,86 @@ +--- +name: advice-provider-hygiene +description: Use when writing kernel, account, or note MASM code that reads from or writes to the advice provider (advice stack / advice map) — key entries by content hash, validate every piece of advice data against a commitment the kernel already trusts, and treat missing required entries as errors rather than silent zeros. +--- + +# Advice Provider Hygiene + +## Rules + +The advice provider is untrusted input supplied by the (potentially adversarial) prover. Any kernel, account, or note procedure that touches it must follow three rules. + +### 1. Validate advice data against a commitment + +Before consuming data loaded from the advice provider: + +1. Read the data from the advice stack / advice map into memory. +2. Compute its hash with the appropriate hasher (typically `Rpo256` over the loaded region). +3. Assert the computed hash equals an expected commitment that the kernel already trusts — on-chain storage, a prior input, or a value already on the stack. + +Do not consume advice data before this check passes. The advice provider's only role is to supply witness data for commitments the kernel has already received. + +### 2. Key advice map entries by content hash + +When inserting into the advice map, the key must be a hash of the value it indexes (or a derived commitment of the same data): + +- Use `Rpo256(value)` (or whichever hash matches the consumer's check) as the key. +- Do not hard-code keys like `0x0000_0000_0000_0001`, `ADVICE_KEY_NOTE_DATA`, or per-procedure magic constants. + +Readers retrieve the entry by recomputing the same hash from data they already trust; rule 1's commitment check binds the lookup result to that trusted hash. + +### 3. Missing advice is an error + +A missing advice-map entry, an empty advice stack, or an absent required value is an error — not a default. Surface it with `assert.err=ERR_...`. Don't substitute zero / empty / a fallback and continue. + +## Why + +The advice provider is filled by the prover, who is potentially adversarial. Each of the three rules closes a different attack: + +- Without commitment validation, a malicious prover injects any data and the kernel runs against it. +- Without content-addressed keys, two procedures (or one procedure plus an adversarial write) can silently collide on the same key in the global per-transaction namespace. +- Without erroring on missing data, the prover skips inputs the kernel would have used to enforce a check; the kernel runs to completion against zeros and produces a "proof" that doesn't actually witness the intended property. + +Content addressing makes collisions cryptographically negligible. The load/hash/assert pattern binds untrusted advice to trusted state. Treating absence as an error preserves the kernel's invariants under adversarial input. + +The pattern is universal in the kernel and account procedures: **load → hash → assert against commitment → use**, with content-addressed keys throughout and `assert.err=...` on absence. + +## Examples + +```masm +# Good: load → hash → check commitment → use +adv_pipe # load advice into memory at the pointer +exec.hash_data # compute hash of the loaded region +push.EXPECTED_COMMITMENT +assert_eqw.err=ERR_COMMITMENT_MISMATCH +# ... safe to use the data here + +# Good: content-addressed advice map insert +push.NOTE_DATA_COMMITMENT +adv.push_mapval # key is the commitment itself + +# Good: missing entry is an error +adv.has_mapkey +assert.err=ERR_MISSING_REQUIRED_ADVICE + +# Bad: consume advice data without validating it +adv_pipe +exec.consume_data # data could be anything the prover wants + +# Bad: hard-coded magic key +push.0x1234_5678_0000_0001 +adv.push_mapval + +# Bad: silent zero on missing key +adv.push_mapval # no-op if key absent; proceed as if zero +``` + +For the Rust analog (returning `Err` on bad/missing external input rather than panicking or defaulting), see `return-error-not-panic`. + +## Evidence + +- PR #1648 (bobbinth): "Verify that data received from the advice provider matches an expected hash to prevent malicious injection." / "Validate the advice data against the on-chain commitment before using it." +- PR #1871 (bobbinth): "Avoid hard-coded advice-map keys; the advice map is a global per-transaction namespace and predictable keys risk collisions." / "The advice data must be checked against the on-chain commitment." +- PR #1896 (PhilippGackstatter): "Use the data commitment as the key rather than a fixed constant." / "Don't silently treat missing data as zero." +- PR #1995 (PhilippGackstatter): "Add a commitment check after the advice pipe." / "Absent advice key must produce a typed error." +- PR #1360 (bobbinth): "Advice map keys should be the hash of the underlying data they index." +- PR #2439 (PhilippGackstatter): "When piping advice data into memory, validate that the supplied hash matches the data rather than blindly inserting from the advice provider." diff --git a/.claude/skills/assert-specific-error-in-tests/SKILL.md b/.claude/skills/assert-specific-error-in-tests/SKILL.md new file mode 100644 index 0000000000..7c9b429bf4 --- /dev/null +++ b/.claude/skills/assert-specific-error-in-tests/SKILL.md @@ -0,0 +1,52 @@ +--- +name: assert-specific-error-in-tests +description: Use when writing a Rust test that exercises a failure path or a MASM test that expects a `panic` / `assert` — assert on the specific expected error variant or `ERR_*` code, not merely that some failure occurred. +--- + +# Negative Tests Must Pin the Expected Error + +## Rule + +A test that exercises an error path must assert on the specific error returned: + +- In Rust: use `assert_matches!(result, Err(MyError::SpecificVariant { .. }))` or destructure the error and assert on its fields. Don't accept "any `Err`" via `assert!(result.is_err())`. +- In MASM: assert that the trapping error code matches the expected `ERR_*` constant, not just that the transaction failed. + +If multiple error conditions could plausibly fire on the same input, assert on the one this test is actually exercising. + +## Why + +A test that only checks `is_err()` passes even when an unrelated bug breaks the function. The test isn't validating the failure mode it claims to — it's validating "this code path doesn't reach the happy path", which a `panic!("not implemented")` would also satisfy. + +Specific-variant assertions catch regressions where one error path starts firing instead of another (e.g. a validation reorder that surfaces `InvalidFormat` before `EmptyInput`), which the looser assertion would silently accept. + +## Examples + +```rust +// Good +use assert_matches::assert_matches; + +let result = AccountId::try_from(&bytes); +assert_matches!(result, Err(AccountError::InvalidLength { expected: 32, got: 5 })); + +// Bad +let result = AccountId::try_from(&bytes); +assert!(result.is_err()); +``` + +```rust +// Good (MASM test) +let err = run_kernel(...).unwrap_err(); +assert_eq!(err.code(), ERR_NOTE_NOT_FOUND); + +// Bad +assert!(run_kernel(...).is_err()); +``` + +## Evidence + +- PR #2740 (PhilippGackstatter): "Negative tests must assert the exact expected error code/variant, not merely that some failure occurred." +- PR #2636 (PhilippGackstatter): "Assert on a specific expected error in negative-path tests instead of asserting only that some error occurred." +- PR #1604 (bobbinth): "Assert on the variant; is_err is too loose." +- PR #1599 (PhilippGackstatter): "Match the specific ERR_ constant." +- PR #1759 (PhilippGackstatter): "Use assert_matches with the specific variant." diff --git a/.claude/skills/cheap-masm-equivalents/SKILL.md b/.claude/skills/cheap-masm-equivalents/SKILL.md new file mode 100644 index 0000000000..bbd122f70f --- /dev/null +++ b/.claude/skills/cheap-masm-equivalents/SKILL.md @@ -0,0 +1,58 @@ +--- +name: cheap-masm-equivalents +description: Use when writing or reviewing MASM hot paths — prefer the cheaper instruction sequence when it is semantically equivalent (`neq.0` over `gt.0` for non-zero checks, `dup` over `loc_load`, `cdrop` over branches selecting between two values). +--- + +# Prefer Cheap MASM Equivalents + +## Rule + +Several MASM idioms have a cheap and an expensive form. Use the cheap one when both produce the same result on the inputs the procedure can see: + +- Non-zero check: `neq.0` (3 cycles) over `gt.0` (16 cycles). +- Selecting between two values on a flag: `cdrop` over an `if.true ... else ... end` branch with the same effect. +- Re-fetch a recently-pushed value: `dup.N` over `loc_load.N` when the value is still on the stack. +- Whole-word equality: `eqw` over element-wise comparisons. +- u32-known operands: `u32gt`/`u32lt` over generic `gt`/`lt`. + +Don't apply the cheap form when the operands violate its precondition (e.g. `u32gt` on a value that might exceed `u32::MAX`). + +## Why + +MASM cycle costs are not uniform. `gt.0` does signed-comparison work that `neq.0` skips entirely; both produce the same boolean for the typical "is it non-zero?" question on non-negative inputs. A hot kernel path that uses `gt.0` everywhere pays the full 16-cycle cost on each call for a check `neq.0` would do in 3. + +These swaps are also free of risk in most cases — the semantics are equivalent under typical preconditions. + +## Examples + +```masm +# Good +push.0 neq # non-zero check, 3 cycles +# or simply +neq.0 + +# Bad +push.0 gt # same answer, 16 cycles +``` + +```masm +# Good: cdrop for ternary selection +# stack: [b, a, cond] +cdrop +# stack: [a if cond else b] + +# Bad: branchy equivalent +if.true + drop # drop b, keep a +else + swap drop # drop a, keep b +end +``` + +## Evidence + +- PR #2636 (PhilippGackstatter): "Prefer neq.0 (3 cycles) over gt.0 (16 cycles) when checking a value is non-zero in MASM." +- PR #2712 (PhilippGackstatter): "Use cdrop here instead of the branch." +- PR #1681 (bobbinth): "Replace this branch with cdrop." +- PR #1968 (bobbinth): "eqw is cheaper than the per-element compare." +- PR #2123 (PhilippGackstatter): "u32gt; we know both operands are u32." diff --git a/.claude/skills/checked-arithmetic/SKILL.md b/.claude/skills/checked-arithmetic/SKILL.md new file mode 100644 index 0000000000..afc014004c --- /dev/null +++ b/.claude/skills/checked-arithmetic/SKILL.md @@ -0,0 +1,38 @@ +--- +name: checked-arithmetic +description: Use when writing Rust arithmetic on amounts, balances, supplies, or other quantities derived from external/user input — use `checked_*` or `overflowing_*` operations and handle the overflow flag explicitly; do not rely on wrapping semantics for untrusted values. +--- + +# Checked Arithmetic on User-Supplied Values + +## Rule + +Any arithmetic where one or more operands originates from external input (transaction body, advice provider, user RPC, deserialized payload) must use checked or overflowing arithmetic and surface the overflow: + +- Prefer `checked_add` / `checked_sub` / `checked_mul` and return an error on `None`. +- Use `overflowing_add` / `widening_mul` when you need the wrapping value *and* the overflow flag; then `assert!(!overflow)` (or branch) before using the result. +- Do not use the default `+`, `-`, `*` operators on untrusted values in release builds — debug-only overflow checks are not enough. + +## Why + +Default arithmetic on `u64`/`u128` wraps in release. Two amounts that fit individually can multiply to overflow, producing a fungible-asset amount that is silently far smaller than the user intended. Checked arithmetic forces the caller to acknowledge the overflow path; wrapping arithmetic hides it. + +## Examples + +```rust +// Good: checked +let total = balance.checked_add(amount).ok_or(Error::Overflow)?; + +// Good: overflowing with explicit flag check +let (product, overflow) = a.widening_mul(b); +if overflow { return Err(Error::Overflow); } + +// Bad: wraps on overflow in release +let total = balance + amount; +``` + +## Evidence + +- PR #2636 (PhilippGackstatter): "Use checked or overflowing arithmetic (overflowing_add, widening_mul) on user-supplied amounts and assert the overflow flag rather than relying on wrapping semantics." +- PR #2712 (PhilippGackstatter): "This addition can overflow on user input — use checked_add and handle the None case." +- PR #1654 (bobbinth): "We should use checked arithmetic here." diff --git a/.claude/skills/conversion-method-naming/SKILL.md b/.claude/skills/conversion-method-naming/SKILL.md new file mode 100644 index 0000000000..61882eed91 --- /dev/null +++ b/.claude/skills/conversion-method-naming/SKILL.md @@ -0,0 +1,56 @@ +--- +name: conversion-method-naming +description: Use when naming a conversion or accessor method in Rust — follow the API guideline prefixes (`as_` for free borrows, `to_` for expensive conversions, `into_` for ownership transfers, `from_` for type-associated constructors) and don't overload `with_` for non-builder methods. +--- + +# Rust Conversion Method Naming + +## Rule + +Pick the prefix that matches the cost and ownership of the conversion: + +- `as_` — free or near-free, returns a borrow (e.g. `as_bytes() -> &[u8]`). +- `to_` — non-trivial cost, returns an owned value, leaves `self` intact (e.g. `to_string() -> String`). +- `into_` — consumes `self`, returns the inner/converted value. +- `from_` — associated function on the target type that constructs it. +- `with_` — *only* for builder-style methods that take and return `Self` with a field set. + +Do not name a non-borrowing method `as_*`. Do not name a non-builder method `with_*`. Do not use `to_*` for a free borrow. + +## Why + +The conventions come from the Rust API guidelines and are deeply baked into the standard library and the wider ecosystem. A reader expects `as_x` to be cheap, `to_x` to allocate, `into_x` to consume — violating that costs every reader a moment of confusion and trains them to distrust the rest of the API. + +`with_*` is especially load-bearing: when readers see `foo.with_bar(b)`, they expect `foo` back with `bar` set. Naming an unrelated method `with_` derails that intuition. + +## Examples + +```rust +// Good +impl Header { + pub fn as_bytes(&self) -> &[u8] { ... } // cheap borrow + pub fn to_vec(&self) -> Vec { ... } // allocates + pub fn into_payload(self) -> Payload { self.payload } // consumes +} + +impl HeaderBuilder { + pub fn with_version(mut self, v: u8) -> Self { self.version = v; self } +} + +// Bad +impl Header { + pub fn as_vec(&self) -> Vec { ... } // allocates, should be to_vec + pub fn to_bytes(&self) -> &[u8] { ... } // borrow, should be as_bytes +} + +// Bad: with_* on a non-builder method +fn with_seed(rng: &mut Rng, seed: u64) { ... } // not returning Self +``` + +## Evidence + +- PR #2053 (PhilippGackstatter): "Follow Rust API naming guidelines for conversions: as_ for free borrows, to_ for expensive conversions, into_ for ownership transfers." +- PR #2712 (PhilippGackstatter): "Reserve with_* for builder methods that return Self; if a method does not return Self, give it a different verb or implement a conversion trait." +- PR #2569 (PhilippGackstatter): "This should be to_, not as_, because it allocates." +- PR #2205 (bobbinth): "Rename as_ to to_ here." +- PR #1426 (PhilippGackstatter): "with_ implies builder semantics; rename." diff --git a/.claude/skills/decouple-component-from-storage/SKILL.md b/.claude/skills/decouple-component-from-storage/SKILL.md new file mode 100644 index 0000000000..f6a689416b --- /dev/null +++ b/.claude/skills/decouple-component-from-storage/SKILL.md @@ -0,0 +1,47 @@ +--- +name: decouple-component-from-storage +description: Use when writing a MASM procedure that lives inside an account component and currently reads from a hard-coded storage slot — accept the required values as stack parameters instead, so the procedure works regardless of where the component is mapped in the account's storage layout. +--- + +# Decouple Component Procedures from Storage Layout + +## Rule + +Procedures that belong to an account component must not bake the component's storage-slot index into their bodies. Instead: + +- Accept the required values as stack parameters (see `masm-explicit-stack-inputs`). +- Let the caller — the account-level glue procedure that knows the actual slot layout — read from storage and push the values. + +If a procedure truly needs to read from "its own" storage, it should take the slot index (or a pointer to it) on the stack, not hard-code it. + +## Why + +A component can be installed into many accounts, each with different storage layouts. Hard-coding a slot index in the component's body ties that component to a single layout — installing it in any other account silently misreads memory. + +Stack-parameter inputs make the component portable: the account-level caller decides where the slot lives and passes the data in. The component then operates on what it was given, with no implicit assumption about where it came from. + +## Examples + +```masm +# Good: component procedure takes its inputs on the stack +proc transfer_asset + # => [ASSET, recipient_id, current_balance] + # operates on what was passed in +end + +# Account-level caller reads from the actual slot, then invokes: +mem_loadw.OUR_VAULT_SLOT +exec.transfer_asset + +# Bad: component procedure reads a hard-coded slot +proc transfer_asset + # => [ASSET, recipient_id] + mem_loadw.4 # only works if THIS component is at slot 4 +end +``` + +## Evidence + +- PR #1712 (PhilippGackstatter): "Decouple component procedures from storage layout assumptions by accepting required values as input parameters." +- PR #2664 (PhilippGackstatter): "This component reads slot 4 directly — pass it as a parameter." +- PR #1599 (bobbinth): "The component shouldn't know its own slot." diff --git a/.claude/skills/domain-newtypes-over-primitives/SKILL.md b/.claude/skills/domain-newtypes-over-primitives/SKILL.md new file mode 100644 index 0000000000..ff99a52c58 --- /dev/null +++ b/.claude/skills/domain-newtypes-over-primitives/SKILL.md @@ -0,0 +1,54 @@ +--- +name: domain-newtypes-over-primitives +description: Use when introducing API parameters, struct fields, or return types that conceptually carry a domain value (asset amount, faucet ID, slot index, fee rate) — wrap them in a newtype that enforces invariants, instead of accepting raw `Word`, `u64`, or `(prefix, suffix)` tuples. +--- + +# Use Domain Newtypes, Not Raw Primitives + +## Rule + +When an API boundary takes or returns a value with a domain meaning, define a newtype that: + +- Validates the value at construction time (see `validate-in-constructor`). +- Exposes the inner representation only through deliberate accessors. +- Is used at every API boundary touching the concept (not raw `u64`/`Word`/tuples). + +Raw `(AccountId, u64)` tuples, bare `Word` parameters, and primitive-typed amounts must be replaced with a named type like `FungibleAsset`, `FaucetId`, `BlockNumber`. + +## Why + +A typed wrapper is the only place invariants get enforced. Once a function accepts `u64` for "amount", every caller is responsible for not passing a value above the max supply, and every reviewer has to check. With `FungibleAsset` (or similar) the type system enforces the rule once and everyone benefits. + +Newtypes also make refactors safer: changing a representation (e.g. moving from `(prefix, suffix)` to a single `[u8; 16]`) touches one type, not every signature. + +## Examples + +```rust +// Good +pub fn mint(asset: FungibleAsset, to: AccountId) -> Result; + +// Bad +pub fn mint(faucet_id: AccountId, amount: u64, to: AccountId) -> Result; +``` + +```rust +// Good: validated wrapper with explicit constructor +pub struct BlockNumber(u32); +impl BlockNumber { + pub fn new(n: u32) -> Result { + if n > MAX_BLOCK_NUMBER { return Err(Error::OutOfRange); } + Ok(Self(n)) + } +} + +// Bad: raw u32 leaks into every signature, every caller checks the bound +pub fn lookup_block(n: u32) -> Option; +``` + +## Evidence + +- PR #2439 (PhilippGackstatter): "Wrap domain-validated values in newtypes that enforce invariants at construction time; avoid raw Word or u64 in APIs." +- PR #2636 (PhilippGackstatter): "Accept strongly-typed domain values (e.g. FungibleAsset) at API boundaries instead of raw (AccountId, u64) tuples or Word arrays." +- PR #2890 (bobbinth): "We should introduce a newtype here rather than passing the raw value." +- PR #1978 (PhilippGackstatter): "Use a typed wrapper instead of a bare Word." +- PR #2246 (bobbinth): "This should be a typed wrapper, not a raw integer." diff --git a/.claude/skills/felt-construction/SKILL.md b/.claude/skills/felt-construction/SKILL.md new file mode 100644 index 0000000000..6e8d65e082 --- /dev/null +++ b/.claude/skills/felt-construction/SKILL.md @@ -0,0 +1,43 @@ +--- +name: felt-construction +description: Use when constructing a `Felt` from a numeric value in Rust — never call `Felt::new(value)` on values that may exceed the field modulus; use `Felt::from(u32)` (infallible) or `Felt::try_from(u64)` (checked) instead. +--- + +# Felt Construction From Untrusted Numeric Inputs + +## Rule + +Do not call `Felt::new(x)` when `x` could exceed the field modulus. `Felt::new` silently truncates oversized values, which produces a valid-looking `Felt` that no longer equals the original input — a classic source of hard-to-attribute bugs. + +Use one of: + +- `Felt::from(x)` where `x` is a `u32` or smaller (infallible). +- `Felt::try_from(x)` for `u64`-and-larger inputs, returning `Result`. +- An explicit `assert!(x < Felt::MODULUS)` before `Felt::new(x)` if you have already proven the bound. + +## Why + +The field modulus is just below `2^64`, so the truncation only kicks in for a narrow band of large values. Most tests pass; production hits one of the bad inputs and the symptom is a value mismatch many layers away from the truncating call. + +`Felt::from(u32)` cannot truncate. `Felt::try_from(u64)` makes the bound check explicit and forces the caller to handle the error. + +## Examples + +```rust +// Good: u32 input, infallible conversion +let f = Felt::from(slot_index as u32); + +// Good: untrusted u64 input, checked conversion +let f = Felt::try_from(user_value).map_err(|_| Error::FeltOverflow)?; + +// Bad: silent truncation on any value >= MODULUS +let f = Felt::new(user_value); +``` + +## Evidence + +- PR #2546 (PhilippGackstatter): "Avoid Felt::new on values that may exceed the field modulus; silent truncation can introduce hard-to-find bugs." +- PR #2439 (PhilippGackstatter): "Use Felt::from on u32 inputs instead of Felt::new." +- PR #2636 (PhilippGackstatter): "Use Felt::try_from for u64 inputs so we surface modulus overflow." +- PR #2712 (PhilippGackstatter): "Felt::new on a user value is unsafe — switch to try_from." +- PR #1925 (PhilippGackstatter): "Felt::new can truncate here; use the checked variant." diff --git a/.claude/skills/intra-doc-links/SKILL.md b/.claude/skills/intra-doc-links/SKILL.md new file mode 100644 index 0000000000..fc3255a693 --- /dev/null +++ b/.claude/skills/intra-doc-links/SKILL.md @@ -0,0 +1,53 @@ +--- +name: intra-doc-links +description: Use when writing or editing a Rust doc comment that references a type, method, or module — write the reference as an intra-doc link with brackets (`[TypeName]`) so renames produce rustdoc warnings instead of silently stale prose. +--- + +# Use Intra-Doc Links in Rust Doc Comments + +## Rule + +When a doc comment mentions a type, method, trait, constant, or module that exists in scope, write it as an intra-doc link: + +```rust +/// Returns the [`AccountId`] associated with this [`Account`]. +/// +/// See also [`AccountStorage::commitment`] for the storage commitment. +``` + +Use `[`Name`]` for items already in scope; use `[`Name`](crate::path::Name)` for items elsewhere; use `[`Name`]: ...` reference-style at the bottom for long paths. + +Do not write type names as plain text or inside single backticks alone (e.g. `` `AccountId` `` without brackets) when the item is reachable from rustdoc. + +## Why + +Intra-doc links are checked by `rustdoc` (with `-D rustdoc::broken-intra-doc-links` or the project's lints). When you rename a type, every intra-doc link to it surfaces as a warning. Plain-text references go silently stale and accumulate over time. + +Intra-doc links also render as clickable in the generated docs, which makes navigation much easier for readers. + +## Examples + +```rust +// Good +/// Returns the [`AccountId`] of this account. +/// +/// # Errors +/// +/// Returns [`AccountError::NotFound`] if the storage slot is empty. +pub fn account_id(&self) -> Result { ... } + +// Bad +/// Returns the `AccountId` of this account. +/// +/// # Errors +/// +/// Returns `AccountError::NotFound` if the storage slot is empty. +pub fn account_id(&self) -> Result { ... } +``` + +## Evidence + +- PR #1832 (PhilippGackstatter): "Write type names in doc comments as intra-doc links so renames surface as warnings." +- PR #1787 (PhilippGackstatter): "Use intra-doc links here." +- PR #1480 (bobbinth): "Link these type references." +- PR #2833 (partylikeits1983): "These type names should be intra-doc links." diff --git a/.claude/skills/lowercase-error-messages/SKILL.md b/.claude/skills/lowercase-error-messages/SKILL.md new file mode 100644 index 0000000000..c333036141 --- /dev/null +++ b/.claude/skills/lowercase-error-messages/SKILL.md @@ -0,0 +1,57 @@ +--- +name: lowercase-error-messages +description: Use when writing or editing a Rust error message, panic message, assertion string, or MASM `ERR_*` constant value — start with a lowercase letter and end without trailing punctuation, unless the first word is a proper noun. +--- + +# Lowercase Error Messages, No Trailing Punctuation + +## Rule + +Error messages (in `Error` enum `#[error("...")]` attributes, `panic!`, `assert!` messages, MASM `ERR_*` constant strings) follow Rust's convention: + +- Start with a lowercase letter (unless beginning with a proper noun or acronym). +- No trailing period, exclamation, or other punctuation. +- Imperative or descriptive, not "ERROR: ..." or "Failed to ...". + +Examples of correct shape: `"invalid account id length"`, `"note not found"`, `"failed to deserialize storage"`. + +## Why + +Error messages get composed into chains (`Display`/`source`) and into anyhow contexts. A trailing period or a capital letter mid-chain ("X failed: Y failed: Z failed.") makes the chain read like a sequence of unrelated sentences instead of a single error. Lowercase-and-no-punctuation is the convention `std::io::Error`, `thiserror`, and most ecosystem crates already follow. + +## Examples + +```rust +// Good +#[derive(Debug, thiserror::Error)] +pub enum AccountError { + #[error("invalid account id length, expected {expected} bytes, got {got}")] + InvalidLength { expected: usize, got: usize }, + #[error("account not found")] + NotFound, +} + +// Bad +#[derive(Debug, thiserror::Error)] +pub enum AccountError { + #[error("Invalid account id length: expected {expected} bytes, got {got}.")] + InvalidLength { ... }, + #[error("Account not found!")] + NotFound, +} +``` + +```masm +# Good +const ERR_NOTE_NOT_FOUND = "note not found" + +# Bad +const ERR_NOTE_NOT_FOUND = "Note not found!" +``` + +## Evidence + +- PR #1832 (PhilippGackstatter): "Use lowercase for error messages and assertion strings unless they begin with a proper noun." +- PR #1599 (bobbinth): "Drop the trailing period." +- PR #1480 (bobbinth): "Lowercase the first word here." +- PR #1507 (PhilippGackstatter): "No exclamation mark." diff --git a/.claude/skills/masm-error-constants/SKILL.md b/.claude/skills/masm-error-constants/SKILL.md new file mode 100644 index 0000000000..858ddc8744 --- /dev/null +++ b/.claude/skills/masm-error-constants/SKILL.md @@ -0,0 +1,60 @@ +--- +name: masm-error-constants +description: Use when adding or editing MASM `assert*` / `panic` instructions — define an `ERR_` constant with a descriptive string message and use it via `assert.err=ERR_NAME`; never use bare assertions without an error code. +--- + +# MASM Error Constants + +## Rule + +Every MASM assertion must carry a descriptive error code: + +```masm +assert.err=ERR_NOTE_NOT_FOUND +assert_eqw.err=ERR_COMMITMENT_MISMATCH +``` + +The error constant must: + +- Use the `ERR_` prefix. +- Live in the file's dedicated errors section (see `masm-constants` skill). +- Have a descriptive string value, not a bare numeric code: `const ERR_NOTE_NOT_FOUND = "note not found"`. +- Be unique per distinct failure condition — do not share one `ERR_` across two unrelated asserts. + +## Why + +Bare `assert` instructions trap with the generic "assertion failed" message, giving the debugger no signal about which check failed. A descriptive `ERR_` constant ties each trap site to a specific failure mode, so a developer reading a transaction's trap output can attribute the failure without disassembling the procedure. + +Distinct constants per failure condition also let tests pin the expected error (see `assert-specific-error-in-tests`). + +## Examples + +```masm +# Good +const ERR_NOTE_NOT_FOUND = "note not found" +const ERR_COMMITMENT_MISMATCH = "stored commitment does not match recomputed value" + +proc verify_note + # ... + assert.err=ERR_NOTE_NOT_FOUND + # ... + assert_eqw.err=ERR_COMMITMENT_MISMATCH +end + +# Bad: bare assertion +proc verify_note + assert +end + +# Bad: shared generic constant for unrelated cases +const ERR_INVALID = "invalid" +assert.err=ERR_INVALID # used in 6 places, each meaning something different +``` + +## Evidence + +- PR #1968 (bobbinth): "Always attach a descriptive error string to MASM assertions to aid debugging and refactoring." +- PR #1871 (bobbinth): "Use the `ERR_` prefix and a dedicated constant for each distinct error condition in MASM." +- PR #2636 (PhilippGackstatter): "This assert needs a descriptive error code." +- PR #1521 (bobbinth): "Move this to an ERR_ constant." +- PR #1609 (PhilippGackstatter): "Bare assert; add an ERR_ code." diff --git a/.claude/skills/masm-explicit-stack-inputs/SKILL.md b/.claude/skills/masm-explicit-stack-inputs/SKILL.md new file mode 100644 index 0000000000..8508b3ebf6 --- /dev/null +++ b/.claude/skills/masm-explicit-stack-inputs/SKILL.md @@ -0,0 +1,50 @@ +--- +name: masm-explicit-stack-inputs +description: Use when defining the interface for a new MASM procedure — pass inputs explicitly on the stack rather than relying on the caller having written values to a known memory address; reserve memory I/O for data that must cross many procedure boundaries. +--- + +# Pass MASM Procedure Inputs Explicitly on the Stack + +## Rule + +A MASM procedure's inputs should arrive on the stack, named in its `Inputs:` doc block. Do not design a procedure that reads its inputs from a fixed memory location that the caller must populate beforehand. + +Use memory I/O only when: + +- The data has a fixed canonical home (account storage, kernel inputs, advice-keyed regions). +- The data is too large to keep on the stack (a full Merkle proof, a large vector). + +For everything else — counts, indices, single words, small structs — pass on the stack. + +## Why + +Hidden memory inputs make the procedure's signature a lie. A reader of `Inputs: [ptr]` cannot tell what's at the other end of that pointer or what invariants the caller had to set up; the contract lives in prose and gets out of sync. + +Stack-passed inputs are typed by the `Inputs:` doc, easy to test in isolation (no global setup required), and impossible to forget — the procedure traps if the stack shape is wrong. + +## Examples + +```masm +# Good +#! Inputs: [note_index, ASSET] +#! Outputs: [] +proc add_asset_to_note + # ... uses values directly from the stack +end + +# Bad: implicit input via memory location the caller had to populate +#! Inputs: [] +#! Outputs: [] +proc add_asset_to_note + mem_load.PENDING_NOTE_PTR # caller had to set this first + mem_loadw.PENDING_ASSET_PTR + # ... +end +``` + +## Evidence + +- PR #2439 (PhilippGackstatter): "Pass parameters explicitly via the stack rather than relying on shared global memory for MASM procedures." +- PR #2664 (PhilippGackstatter): "Make this an explicit stack input rather than reading from a fixed slot." +- PR #1599 (bobbinth): "The caller shouldn't have to populate memory before calling." +- PR #1712 (PhilippGackstatter): "Take this as a stack parameter." diff --git a/.claude/skills/masm-locals-over-globals/SKILL.md b/.claude/skills/masm-locals-over-globals/SKILL.md new file mode 100644 index 0000000000..91b0f10b34 --- /dev/null +++ b/.claude/skills/masm-locals-over-globals/SKILL.md @@ -0,0 +1,49 @@ +--- +name: masm-locals-over-globals +description: Use when writing a MASM procedure that needs temporary scratch storage — prefer procedure-local memory (`loc_store` / `loc_load`) over global memory regions when both achieve the same result. +--- + +# Prefer Procedure Locals for MASM Scratch Storage + +## Rule + +When a MASM procedure needs scratch storage that lives only for the duration of one invocation, use procedure-local memory (`loc_store`, `loc_load`, `loc_storew`, `loc_loadw`) rather than allocating in a shared global memory region. + +Global memory regions are reserved for state that crosses procedure boundaries (kernel inputs, account data, advice-keyed state). Stashing per-call scratch there leaks an implementation detail into a shared namespace and ties the procedure to a fixed address. + +## Why + +Procedure locals are automatically allocated and freed by the VM. Two callers of the same procedure on the same kernel run cannot collide. By contrast, a hard-coded scratch slot in global memory: + +- Risks colliding with another procedure that uses the same slot. +- Forces every caller to know the slot exists (don't clobber it). +- Locks the layout — moving the slot is a cross-cutting change. + +Use globals only for data that must persist across procedure boundaries by design. + +## Examples + +```masm +# Good +proc compute_hash + # allocate two local slots + loc_store.0 + loc_store.1 + # ... + loc_load.0 + loc_load.1 +end + +# Bad: scratch in a shared region +const SCRATCH_PTR = 0x4000 +proc compute_hash + mem_store.SCRATCH_PTR # collides with anyone else using SCRATCH_PTR +end +``` + +## Evidence + +- PR #2871 (PhilippGackstatter): "Prefer procedure locals over scratch memory in MASM procedures when both achieve the same result." +- PR #2636 (PhilippGackstatter): "Keep procedure-scoped temporary values in local memory rather than global memory." +- PR #2381 (bobbinth): "Use loc_store here instead of a global scratch region." +- PR #2292 (PhilippGackstatter): "This scratch is per-call; make it local." diff --git a/.claude/skills/masm-named-literals/SKILL.md b/.claude/skills/masm-named-literals/SKILL.md new file mode 100644 index 0000000000..3b04d2e28b --- /dev/null +++ b/.claude/skills/masm-named-literals/SKILL.md @@ -0,0 +1,49 @@ +--- +name: masm-named-literals +description: Use when writing or editing MASM procedures or Rust modules that contain numeric literals for memory offsets, slot indices, sizes, tag/type/protocol values, or domain constants — promote them to named constants in a single source-of-truth location. +--- + +# Replace Magic Numbers with Named Constants + +## Rule + +Numeric literals embedded inline in MASM and Rust code must be promoted to named constants when they represent: + +- Memory offsets, slot indices, or layout sizes +- Protocol/tag/type/version discriminants +- Domain values reused in more than one place + +Define each constant exactly once. In MASM, declare it in the file's `CONSTANTS` section (see `masm-constants` skill). In Rust, define it as an associated constant on the type it describes (`Type::CAPACITY`, not a free-floating `const CAPACITY`). + +## Why + +A bare `47` or `0x1234` in code is invisible to grep, indistinguishable from coincidental same-valued numbers, and risks drift when one occurrence is updated and another is missed. Naming the literal documents intent and gives refactors a single source of truth. + +## Examples + +```masm +# Good +const ACCOUNT_DATA_PTR = 4 +mem_load.ACCOUNT_DATA_PTR + +# Bad +mem_load.4 # what is at offset 4? +``` + +```rust +// Good +impl AccountStorage { + pub const MAX_NUM_STORAGE_SLOTS: usize = 255; +} + +// Bad +if slots.len() > 255 { ... } // 255 also appears unrelated elsewhere +``` + +## Evidence + +- PR #2390 (PhilippGackstatter): "Replace magic numbers in MASM with named constants." +- PR #2257 (bobbinth): "Define recurring magic numbers (protocol/tag/type values) as named constants in a single central location, not inline literals." +- PR #2871 (PhilippGackstatter): "We should use a named constant here rather than the literal." +- PR #2730 (bobbinth): "It would be good to have a constant for this value." +- PR #2670 (PhilippGackstatter): "Let's name this rather than using the literal." diff --git a/.claude/skills/masm-rust-constant-parity/SKILL.md b/.claude/skills/masm-rust-constant-parity/SKILL.md new file mode 100644 index 0000000000..73446d4b2a --- /dev/null +++ b/.claude/skills/masm-rust-constant-parity/SKILL.md @@ -0,0 +1,45 @@ +--- +name: masm-rust-constant-parity +description: Use when changing a numeric constant in Rust or MASM that has a counterpart on the other side (MAX_NUM_SLOTS, ACCOUNT_ID_SIZE, header field widths, version numbers) — cross-check both sides in the same PR so they stay aligned. +--- + +# Keep Rust and MASM Constants Aligned + +## Rule + +Numeric constants that exist in both Rust and MASM and must agree (memory layouts, capacity limits, version numbers, field widths) need to be updated on both sides in the same PR. Specifically: + +- When changing a Rust constant, search the MASM source tree for its counterpart and update it. +- When changing a MASM constant, search for its Rust counterpart and update it. +- Where possible, generate one side from the other (build script, codegen) so the cross-check is automatic. + +A PR that updates only one side is incomplete — the kernel and the Rust host will disagree at runtime. + +## Why + +The kernel reads memory based on offsets the Rust client wrote. If one side updates `ACCOUNT_HEADER_LEN` and the other doesn't, every transaction misreads its own state. This class of bug is invisible until a real value happens to straddle the changed offset, which is often well after the PR has landed. + +A reviewer-enforced cross-check (or, better, a codegen'd shared definition) catches it before merge. + +## Examples + +```rust +// In Rust +impl AccountHeader { + pub const LEN_FELTS: usize = 8; +} +``` + +```masm +# In MASM — must equal the Rust constant +const ACCOUNT_HEADER_LEN_FELTS = 8 +``` + +If you change either to `9`, change both in the same PR. Add a comment at one declaration pointing to the other (or a build-script assertion) if the relationship isn't obvious. + +## Evidence + +- PR #2795 (mmagician): "Numeric limits must agree across Rust and MASM; cross-check Rust constants against their MASM counterparts when changing either side." +- PR #1532 (bobbinth): "This constant also needs to change in MASM." +- PR #1353 (PhilippGackstatter): "MASM side is now out of sync with the Rust constant." +- PR #1982 (bobbinth): "Cross-check the kernel side." diff --git a/.claude/skills/non-exhaustive-public-types/SKILL.md b/.claude/skills/non-exhaustive-public-types/SKILL.md new file mode 100644 index 0000000000..9b17a3d25e --- /dev/null +++ b/.claude/skills/non-exhaustive-public-types/SKILL.md @@ -0,0 +1,68 @@ +--- +name: non-exhaustive-public-types +description: Use when defining a new public `enum` or `struct` in a library crate that may grow new variants/fields in future releases — mark it `#[non_exhaustive]` so adding variants is not a breaking change. +--- + +# Mark Public Types `#[non_exhaustive]` + +## Rule + +Public enums and public-fielded structs in library crates whose variants/fields are expected to grow should be marked `#[non_exhaustive]`: + +```rust +#[non_exhaustive] +pub enum AssetKind { Fungible, NonFungible } + +#[non_exhaustive] +pub struct Header { + pub version: u8, + pub flags: u32, +} +``` + +This forces external code to use a wildcard match arm (or default field syntax) and lets the library add new variants/fields in a minor release without breaking downstreams. + +Don't mark types `#[non_exhaustive]` when the closed set is part of the contract (e.g. a primitive-like wrapper, a fixed protocol enum that must match a spec). + +## Why + +In a downstream crate, an exhaustive match on a non-`#[non_exhaustive]` enum compiles successfully — but a future minor release that adds a variant breaks every such match. `#[non_exhaustive]` makes the wildcard arm mandatory and shifts variant-additions from breaking to non-breaking. + +The same applies to public-fielded structs: without `#[non_exhaustive]`, every `Header { version, flags }` literal must list every field, and adding a field is a breaking change. + +## Examples + +```rust +// Good +#[non_exhaustive] +pub enum AccountStorageMode { + Public, + Private, +} + +// Internal callers still match exhaustively (in-crate exhaustiveness is allowed) +match mode { + AccountStorageMode::Public => ..., + AccountStorageMode::Private => ..., +} + +// External callers are forced to a wildcard +match mode { + AccountStorageMode::Public => ..., + AccountStorageMode::Private => ..., + _ => ..., // adding a variant in a minor release: compiles +} + +// Bad: closed public enum, additions are breaking +pub enum AccountStorageMode { + Public, + Private, +} +``` + +## Evidence + +- PR #2712 (PhilippGackstatter): "Public enums in library APIs should be marked non_exhaustive when new variants are anticipated." +- PR #1924 (PhilippGackstatter): "Mark this `#[non_exhaustive]`." +- PR #1713 (PhilippGackstatter): "We anticipate more variants — make it non_exhaustive." +- PR #1721 (bobbinth): "Public struct, add non_exhaustive." diff --git a/.claude/skills/parametrize-related-tests/SKILL.md b/.claude/skills/parametrize-related-tests/SKILL.md new file mode 100644 index 0000000000..0c25f80b7d --- /dev/null +++ b/.claude/skills/parametrize-related-tests/SKILL.md @@ -0,0 +1,54 @@ +--- +name: parametrize-related-tests +description: Use when writing two or more tests that differ only by inputs or expected outputs (e.g. one per enum variant, one per "good/bad" input pair) — parameterize them with `rstest` cases instead of copy-pasting per-case test functions. +--- + +# Parameterize Repeated Tests with `rstest` + +## Rule + +When you'd write two or more test functions that share their body and differ only by inputs/expected values, write a single `#[rstest]` function with one `#[case]` per input set: + +```rust +#[rstest] +#[case::happy("abc", true)] +#[case::empty("", false)] +#[case::too_long(LONG_INPUT, false)] +fn validate_name(#[case] input: &str, #[case] expected: bool) { + assert_eq!(validate(input), expected); +} +``` + +This applies especially to "one test per enum variant" patterns and "one test per error condition" patterns. + +## Why + +Copy-pasted test bodies drift: a fix in one variant doesn't propagate to the others. `rstest` cases keep the assertion logic in one place, name each case in test output, and make it obvious from the test file that the coverage is complete (one case per variant). + +Tests parameterized this way are also cheaper to extend: adding a new variant or input is a single `#[case]` line instead of a copy of the whole function. + +## Examples + +```rust +// Good +#[rstest] +#[case::fungible(Asset::Fungible(make_fungible()), AssetKind::Fungible)] +#[case::nft(Asset::Nft(make_nft()), AssetKind::Nft)] +fn asset_kind(#[case] asset: Asset, #[case] expected: AssetKind) { + assert_eq!(asset.kind(), expected); +} + +// Bad: two test functions duplicating the body +#[test] +fn asset_kind_fungible() { assert_eq!(Asset::Fungible(make_fungible()).kind(), AssetKind::Fungible); } +#[test] +fn asset_kind_nft() { assert_eq!(Asset::Nft(make_nft()).kind(), AssetKind::Nft); } +``` + +## Evidence + +- PR #2849 (PhilippGackstatter): "Parametrize tests across all variants of an enum family rather than duplicating per-variant test boilerplate." +- PR #2439 (PhilippGackstatter): "Use rstest to dedupe these test functions." +- PR #2741 (bobbinth): "Could be expressed as rstest cases." +- PR #2390 (PhilippGackstatter): "These tests differ only by input — rstest case form." +- PR #2123 (PhilippGackstatter): "Fold into a single parameterized test." diff --git a/.claude/skills/preserve-error-source/SKILL.md b/.claude/skills/preserve-error-source/SKILL.md new file mode 100644 index 0000000000..ede1392f6c --- /dev/null +++ b/.claude/skills/preserve-error-source/SKILL.md @@ -0,0 +1,52 @@ +--- +name: preserve-error-source +description: Use when defining a new error variant or wrapping a lower-level error — preserve the underlying error via `#[source]` (or `Box`); never stringify it into the message. +--- + +# Preserve Error Source Chains + +## Rule + +When a new error wraps a lower-level error, preserve the source so the chain remains traversable: + +- Use `thiserror`'s `#[source]` attribute (or `#[from]`) to attach the underlying error. +- For dynamic sources, `Box`. +- Do not call `.to_string()` on the source and embed it into the wrapper's message — that breaks `Error::source()` traversal and destroys structured information. + +## Why + +Tools (anyhow, eyre, tracing, logging frameworks) walk `Error::source()` to produce full chains, render context, and group by root cause. Stringifying the source into the message yields one flat string with no machine-readable structure: chains can't be walked, root causes can't be re-attributed, and the inner error's fields are gone. + +A preserved source costs nothing — a `#[source]` annotation is one line — and gives every observer the ability to inspect the chain. + +## Examples + +```rust +// Good +#[derive(Debug, thiserror::Error)] +pub enum AccountError { + #[error("failed to deserialize account storage")] + StorageDeser(#[source] DeserializationError), + + #[error("failed to load account from {path}")] + Load { path: PathBuf, #[source] io: io::Error }, +} + +// Bad: source is stringified, chain is lost +#[derive(Debug, thiserror::Error)] +pub enum AccountError { + #[error("failed to deserialize account storage: {0}")] + StorageDeser(String), +} + +// Bad: source baked into the message via format! +return Err(AccountError::StorageDeser(format!("{e}"))); +``` + +## Evidence + +- PR #2439 (PhilippGackstatter): "Preserve source errors using source attribute rather than stringifying them, so error chains remain useful for debugging." +- PR #2389 (PhilippGackstatter): "Use #[source] here so the underlying io error survives." +- PR #2053 (PhilippGackstatter): "Don't stringify the source; thiserror has source for this." +- PR #1998 (bobbinth): "Box the inner error rather than calling to_string on it." +- PR #2365 (PhilippGackstatter): "Attach the cause as a source." diff --git a/.claude/skills/private-fields-with-accessors/SKILL.md b/.claude/skills/private-fields-with-accessors/SKILL.md new file mode 100644 index 0000000000..cbd017ee13 --- /dev/null +++ b/.claude/skills/private-fields-with-accessors/SKILL.md @@ -0,0 +1,47 @@ +--- +name: private-fields-with-accessors +description: Use when adding a public struct or changing a struct field's visibility in a Rust library crate — keep fields private and expose them through accessor methods so the layout can evolve without breaking callers. +--- + +# Keep Struct Fields Private; Expose Accessors + +## Rule + +Public structs in library crates have private fields. Read access goes through an `pub fn field(&self) -> &T` accessor; mutation goes through dedicated methods (no `pub fn field_mut`). + +Exceptions: + +- "Open" data types whose layout is part of the contract (e.g. `Point { x, y }`) may have public fields, but they should be `#[non_exhaustive]`. +- Internal/`pub(crate)` types may have public fields when keeping them private adds no value. + +## Why + +Public fields freeze the struct's representation. You cannot rename, retype, split, or compute-on-read a public field without breaking every caller. Accessor methods let the type evolve internally — a `pub fn supply(&self) -> u64` can become a computed expression next release without anyone noticing. + +## Examples + +```rust +// Good +pub struct FungibleTokenMetadata { + name: Box, + supply: u64, +} + +impl FungibleTokenMetadata { + pub fn name(&self) -> &str { &self.name } + pub fn supply(&self) -> u64 { self.supply } +} + +// Bad: every consumer locked to these field names and types forever +pub struct FungibleTokenMetadata { + pub name: String, + pub supply: u64, +} +``` + +## Evidence + +- PR #2439 (PhilippGackstatter): "Keep fields of public structs private to preserve the ability to extend them non-breakingly after release." +- PR #2670 (PhilippGackstatter): "Make these fields private and add accessors." +- PR #2712 (PhilippGackstatter): "This field should be private." +- PR #1934 (bobbinth): "We should not expose these fields directly." diff --git a/.claude/skills/return-error-not-panic/SKILL.md b/.claude/skills/return-error-not-panic/SKILL.md new file mode 100644 index 0000000000..865f959693 --- /dev/null +++ b/.claude/skills/return-error-not-panic/SKILL.md @@ -0,0 +1,77 @@ +--- +name: return-error-not-panic +description: Use when writing a public Rust API function, a deserialization path, or any code reachable from external/untrusted input — return a `Result` for recoverable failures (never panic/unwrap/silent-default), and when a genuinely-trusted fast path is needed, expose it as a separate `_unchecked` entry point rather than weakening the default. +--- + +# Return Errors, Don't Panic on External Input + +## Rule + +Functions that touch external input — public API entry points, `Deserializable::read_from`, RPC handlers, advice-provider readers, parsing of user data — must surface failures as `Err`, not panics: + +- No `unwrap()`, `expect()`, or `panic!` on values whose validity depends on external data. +- No `unwrap_or_default()` to silently substitute a fallback for invalid input. +- No `Option` return type that hides the cause of failure when a real error is available. +- Missing required input is itself an error — don't substitute zero/empty/default and continue. + +Convert any internal panic on external-derived values into a typed error variant. + +### Two-tier API for the trusted case + +When a no-check fast path is genuinely needed for callers operating on already-validated in-memory state, expose it as a separate constructor (e.g. `new_unchecked`, `from_parts_unchecked`) with a `# Safety` doc comment spelling out the caller's obligation. The default constructor stays fallible. + +## Why + +A panic on untrusted input is a denial-of-service vector and a debugging black hole. `unwrap_or_default()` is worse: the function silently produces a value that "looks valid" but represents data the caller never sent, and the bug surfaces far from the actual error. + +Silently defaulting on missing input is particularly dangerous in kernel-adjacent code: a malicious prover (or buggy upstream) can skip data the kernel would have used to enforce a check; the kernel runs to completion against zeros and produces a "proof" that doesn't actually witness the property the user wanted. + +Splitting the trusted entry point out by name (`*_unchecked`) keeps the default path strict while still giving performance-sensitive callers an explicit opt-out — and the name forces them to acknowledge what they're skipping. + +## Examples + +```rust +// Good +pub fn parse_account_id(bytes: &[u8]) -> Result { + if bytes.len() != ACCOUNT_ID_LEN { + return Err(AccountError::InvalidLength { expected: ACCOUNT_ID_LEN, got: bytes.len() }); + } + AccountId::try_from(bytes) +} + +// Bad: panics on bad input +pub fn parse_account_id(bytes: &[u8]) -> AccountId { + AccountId::try_from(bytes).unwrap() +} + +// Bad: silently substitutes a default +pub fn parse_account_id(bytes: &[u8]) -> AccountId { + AccountId::try_from(bytes).unwrap_or_default() +} +``` + +Two-tier API when a trusted fast path is justified: + +```rust +impl AccountId { + /// Fallible default — validates the bytes. + pub fn try_from_bytes(b: &[u8]) -> Result { /* ... */ } + + /// # Safety + /// Caller must guarantee `b` came from a previously validated source + /// (e.g. a value already constructed via `try_from_bytes`). + pub fn from_bytes_unchecked(b: &[u8]) -> Self { /* ... */ } +} +``` + +For the MASM analog (validating against a commitment, content-addressed advice keys, erroring on missing advice), see `advice-provider-hygiene`. + +## Evidence + +- PR #2439 (PhilippGackstatter): "Return errors rather than silently swallowing them with Option or defaults in conversion logic." +- PR #2246 (bobbinth): "Treat missing required data as an error rather than silently defaulting or skipping." +- PR #2123 (PhilippGackstatter): "Don't unwrap on deserialized data." +- PR #2006 (PhilippGackstatter): "Deserialization constructors must treat input as untrusted; provide a separate trusted constructor for in-memory use." +- PR #1934 (PhilippGackstatter): "Surface this as a Result variant; users could trigger it." +- PR #1995 (PhilippGackstatter): "Absent advice key must produce a typed error." +- PR #1531 (bobbinth): "Replace this panic with a typed error." diff --git a/.claude/skills/u32-assert-before-u32-ops/SKILL.md b/.claude/skills/u32-assert-before-u32-ops/SKILL.md new file mode 100644 index 0000000000..22ded70bd9 --- /dev/null +++ b/.claude/skills/u32-assert-before-u32-ops/SKILL.md @@ -0,0 +1,46 @@ +--- +name: u32-assert-before-u32-ops +description: Use when writing MASM that uses `u32*` instructions (`u32add`, `u32lt`, `u32div`, etc.) on values that originate from user input or untrusted sources — assert the operands are valid u32s with `u32assert*` before the operation. +--- + +# Validate u32 Operands Before u32 Instructions + +## Rule + +MASM's `u32*` instructions assume their operands are valid `u32` values (i.e. fit in 32 bits). Operating on a non-u32 value silently produces garbage or traps with a generic message. + +Before applying any `u32*` instruction to a value that is not already known to be a valid u32 (e.g. it came from the stack as input, was read from memory, or arose from a non-u32 arithmetic op), assert the bound: + +```masm +u32assert # one value +u32assert2 # two top values +u32assert4 # four top values +``` + +If the operand is already known-valid (just produced by another `u32*` op, or a value loaded from a slot whose layout is u32 by construction), skip the assert. + +## Why + +`u32*` instructions are tuned for performance under the precondition that their inputs fit in 32 bits. The VM does not implicitly check the precondition — it's the procedure's job. Skipping `u32assert*` lets a non-u32 input produce a wrong result or trap with an uninformative message; the explicit assert gives the bug a named failure mode (see `masm-error-constants`). + +## Examples + +```masm +# Good: assert u32 before the u32 op +u32assert.err=ERR_VALUE_NOT_U32 +u32add + +# Good: both operands at once +u32assert2.err=ERR_VALUES_NOT_U32 +u32lt + +# Bad: u32 op on untrusted input +u32add # one operand could be >2^32; silently wraps or traps +``` + +## Evidence + +- PR #1605 (bobbinth): "Before using u32 comparison or arithmetic instructions on user-supplied values, assert they are valid u32s with `u32assert`." +- PR #1638 (PhilippGackstatter): "Add u32assert before this u32lt." +- PR #1871 (bobbinth): "u32assert2 here for both operands." +- PR #1968 (bobbinth): "Need a u32assert before the u32op." diff --git a/.claude/skills/use-bon-builder/SKILL.md b/.claude/skills/use-bon-builder/SKILL.md new file mode 100644 index 0000000000..58fb9f43d6 --- /dev/null +++ b/.claude/skills/use-bon-builder/SKILL.md @@ -0,0 +1,59 @@ +--- +name: use-bon-builder +description: Use when introducing a Rust type whose constructor takes more than ~3 optional or named parameters — derive a builder with `#[bon::builder]` instead of hand-writing a separate Builder module or adding overloaded constructors. +--- + +# Use `bon` for Builders With Many Optional Fields + +## Rule + +When a type's constructor has many optional or named parameters, derive its builder with `#[bon::builder]`: + +```rust +#[bon::builder] +pub fn new( + required: Foo, + #[builder(default)] optional: Option, + #[builder(into)] name: String, +) -> Self { ... } +``` + +Don't hand-write a separate `FooBuilder` module just to expose a fluent API. Don't add a constellation of `new`, `new_with_x`, `new_with_x_and_y` constructors. + +`bon` handles compile-time required-field enforcement, optional defaults, and `impl Into` parameters for free. + +## Why + +Hand-written builders are repetitive boilerplate that drifts over time: a new field gets added to the struct, the builder forgets to set it, and the bug surfaces on the next refactor. `bon` derives the builder from the constructor signature, so the two cannot diverge. + +`bon` also enforces required-field-set at compile time (calling `.build()` without setting a required field is a type error), which most hand-written builders skip. + +## Examples + +```rust +// Good +#[bon::builder] +impl Account { + pub fn new( + id: AccountId, + #[builder(default)] storage: AccountStorage, + #[builder(into)] code: AccountCode, + ) -> Self { ... } +} + +let acc = Account::builder() + .id(my_id) + .code(my_code) + .build(); + +// Bad: bespoke builder module that drifts +pub struct AccountBuilder { id: Option, storage: Option, ... } +impl AccountBuilder { ... 80 lines ... } +``` + +## Evidence + +- PR #2890 (PhilippGackstatter): "Replace hand-written builder modules with `#[bon::builder]` when the builder has many optional fields." +- PR #2636 (PhilippGackstatter): "Use bon here instead of the custom builder." +- PR #2439 (PhilippGackstatter): "This is a perfect case for `#[bon::builder]`." +- PR #1713 (PhilippGackstatter): "bon would replace this boilerplate." diff --git a/.claude/skills/use-test-fixtures/SKILL.md b/.claude/skills/use-test-fixtures/SKILL.md new file mode 100644 index 0000000000..a2bc402126 --- /dev/null +++ b/.claude/skills/use-test-fixtures/SKILL.md @@ -0,0 +1,54 @@ +--- +name: use-test-fixtures +description: Use when a Rust or MASM test needs to construct an account, note, transaction, or other domain object — use the existing fixture builder (`NoteBuilder`, `ScriptBuilder`, `AccountIdBuilder`, `rand_value()`) rather than hand-rolled `dummy(...)` helpers or raw constructors. +--- + +# Use Existing Test Fixtures, Don't Hand-Roll + +## Rule + +When a test needs a domain object, reach for the existing fixture infrastructure: + +- Notes: `NoteBuilder`. +- Scripts: `ScriptBuilder`. +- Account IDs: `AccountIdBuilder` (or the existing `ACCOUNT_ID_*` constants). +- Random felts/words: `rand_value()` (deterministic seed-driven RNG). +- Accounts: `AccountBuilder` with the `testing` feature. + +Don't write a new `AccountId::dummy(...)`, `Note::test_only(...)`, or one-off random helper. If the existing fixtures can't express what you need, extend them — don't fork. + +## Why + +Shared fixtures encode the domain's validation rules and keep tests honest: an `AccountIdBuilder`-built ID has the right tag bits, the right storage-mode bits, and survives a round-trip through serialization. A hand-rolled `dummy()` typically doesn't, which means tests pass against invariants the real code doesn't enforce — or fail randomly on the few invariants that do leak through. + +Reusing fixtures also keeps tests short and lets a single fixture upgrade (e.g. adding a new required field to `Note`) propagate to every test via a single edit. + +## Examples + +```rust +// Good +let note = NoteBuilder::new() + .recipient(test_recipient()) + .with_asset(rand_value()) + .build()?; + +let account_id = AccountIdBuilder::new().build(); + +// Bad +let note = Note { + metadata: NoteMetadata::default(), + inputs: NoteInputs::default(), + assets: NoteAssets::default(), + recipient: NoteRecipient::dummy(), +}; + +let account_id = AccountId::try_from(Word::default()).unwrap(); +``` + +## Evidence + +- PR #2592 (PhilippGackstatter): "In tests, use existing account-id constants or `AccountIdBuilder` rather than ad-hoc `AccountId::dummy(...)` helpers." +- PR #1959 (PhilippGackstatter): "NoteBuilder will do this." +- PR #1916 (PhilippGackstatter): "Use the ScriptBuilder." +- PR #1564 (bobbinth): "There's a fixture for this." +- PR #1622 (PhilippGackstatter): "Use rand_value() instead of inventing a random helper." diff --git a/.claude/skills/validate-in-constructor/SKILL.md b/.claude/skills/validate-in-constructor/SKILL.md new file mode 100644 index 0000000000..6b872a74b4 --- /dev/null +++ b/.claude/skills/validate-in-constructor/SKILL.md @@ -0,0 +1,64 @@ +--- +name: validate-in-constructor +description: Use when writing or reviewing a Rust constructor, `try_new`, or builder `build` — centralize validation so every instance is valid by construction, and reject configurations (empty allowlists, zero thresholds, mutually inconsistent fields) that render the object unusable. +--- + +# Validate Invariants in Constructors + +## Rule + +Every fallible construction path for a struct must run all invariants in a single canonical constructor (or builder `build`). The constructor must: + +1. Validate every invariant the type promises to uphold. +2. Return `Err` on any input that would make the resulting value unusable — empty allowlists, zero thresholds, mutually inconsistent fields, etc. +3. Be the only externally callable way to produce an instance (struct literal construction must not be possible from outside the module). + +Do not split validation across the constructor and downstream methods; do not allow direct field initialization that bypasses checks. + +## Why + +If the type can be constructed in an intermediate or invalid state, every consumer has to defend against it. Centralizing validation means once you hold a `T`, you can trust its invariants without re-checking. + +Rejecting unusable configurations early (e.g. an empty script-roots allowlist that "bricks" the account, a zero threshold for a policy that requires `count >= threshold`) prevents bugs from surfacing far from their cause. + +## Examples + +```rust +// Good: single validating constructor, no public fields +pub struct ProcedurePolicyMode { + immediate_threshold: u32, +} + +impl ProcedurePolicyMode { + pub fn new(immediate_threshold: u32) -> Result { + if immediate_threshold == 0 { + return Err(PolicyError::ZeroThreshold); + } + Ok(Self { immediate_threshold }) + } +} + +// Good: reject empty allowlist that would brick the account +impl ScriptRoots { + pub fn new(roots: BTreeSet) -> Result { + if roots.is_empty() { + return Err(Error::EmptyAllowlist); + } + Ok(Self { roots }) + } +} + +// Bad: in-between invalid state possible +let mut metadata = FungibleTokenMetadata::default(); +metadata.set_name(name); +metadata.set_supply(supply); +metadata.validate()?; // can be forgotten +``` + +## Evidence + +- PR #2795 (mmagician): "Constructors should validate inputs and centralize derived-state computation so callers cannot bypass invariants." +- PR #2883 (bobbinth): "Reject configurations that would render an object permanently unusable at construction time." +- PR #2439 (PhilippGackstatter): "I would in any case suggest validating before building the struct, so that once you have a FungibleTokenMetadata, you know it is valid and do not have an in-between state where the struct could be invalid." +- PR #2670 (PhilippGackstatter): "Ideally, what this function validates should be enforced in `ProcedurePolicyMode`. We should not be able to construct a `ProcedurePolicyMode` with immediate threshold = 0 if this is never a valid state." +- PR #2382 (bobbinth): "Validation should happen in the constructor." diff --git a/.claude/skills/workspace-shared-dependencies/SKILL.md b/.claude/skills/workspace-shared-dependencies/SKILL.md new file mode 100644 index 0000000000..f94410be1c --- /dev/null +++ b/.claude/skills/workspace-shared-dependencies/SKILL.md @@ -0,0 +1,56 @@ +--- +name: workspace-shared-dependencies +description: Use when adding or modifying a dependency entry in a crate's `Cargo.toml` — if the dependency is used by more than one crate in the workspace, declare it once in the workspace root `[workspace.dependencies]` and reference it from each crate with `dep = { workspace = true }`. +--- + +# Workspace-Level Shared Dependencies + +## Rule + +When adding a dependency that another crate in the workspace already uses (or that you anticipate adding to another crate), declare it once in the root `Cargo.toml`'s `[workspace.dependencies]` table. In each crate that uses it, write: + +```toml +[dependencies] +serde = { workspace = true } +``` + +Don't duplicate version strings across crates. Don't keep a dependency crate-local once a second crate adopts it — promote it to the workspace. + +For dependencies used in only one crate, keep them crate-local. Promote when a second crate starts using them. + +## Why + +Workspace-level declarations are the single source of truth for shared versions. Without them, every crate pins its own version, which inevitably drifts — two crates ending up on different minor versions, cargo resolving a duplicate dependency graph, larger compile times, and version-mismatch bugs. + +Promoting on second use (rather than upfront) keeps the workspace table from accumulating one-off entries. + +## Examples + +```toml +# Good (workspace root) +[workspace.dependencies] +serde = "1.0" +thiserror = "1.0" + +# Good (crate) +[dependencies] +serde = { workspace = true, features = ["derive"] } +thiserror = { workspace = true } +``` + +```toml +# Bad: two crates pinning the same dep at potentially different versions +# crates/foo/Cargo.toml +[dependencies] +serde = "1.0" + +# crates/bar/Cargo.toml +[dependencies] +serde = "1.0.130" +``` + +## Evidence + +- PR #1894 (bobbinth): "Define shared dependencies at the workspace level rather than duplicating versions across crates." +- PR #1721 (bobbinth): "Promote crate dependencies to workspace dependencies once they are used in more than one crate." +- PR #1802 (PhilippGackstatter): "This should be `workspace = true`."