diff --git a/.github/pull_request_template.md b/.github/pull_request_template.md index 6e31932..92c6bb5 100644 --- a/.github/pull_request_template.md +++ b/.github/pull_request_template.md @@ -20,3 +20,18 @@ If this PR changes any of these human-readable contract snapshots, reviewers mus - [ ] No snapshot files changed in those directories. - [ ] Snapshot files changed, and the PR description explains the intended storage/event diff. - [ ] Snapshot files changed, and a reviewer has explicitly confirmed the JSON diff is expected. + +## Fixture diff review + +If this PR regenerated any XDR or JSON event-schema fixture files under +`contracts/tipjar/tests/fixtures/` (i.e. `UPDATE_FIXTURES=1` was used), the +companion `.json` files must be reviewed before merging. The JSON diff shows +exactly which fields, types, or field ordering changed in the on-chain event +schema — a change that is invisible in the binary `.xdr` diff. + +**Before ticking any box below, open the "Files changed" tab and read every +changed `.json` file in `contracts/tipjar/tests/fixtures/`.** + +- [ ] No fixture files changed in this PR. +- [ ] Fixture files changed, and the JSON companion diff was reviewed. The changes are intentional and described in the summary above. +- [ ] Fixture files changed, and a reviewer has explicitly confirmed the JSON diff matches the intended event-schema change. diff --git a/.github/workflows/fixture-review.yml b/.github/workflows/fixture-review.yml new file mode 100644 index 0000000..1094b1c --- /dev/null +++ b/.github/workflows/fixture-review.yml @@ -0,0 +1,72 @@ +name: Fixture Review Gate + +# This workflow triggers on any PR that touches an XDR or JSON event-fixture +# file under contracts/tipjar/tests/fixtures/. Its sole job is to ensure that +# the PR body contains an explicit reviewer acknowledgment before the PR can +# be merged. +# +# Why this exists: +# Binary XDR golden files are not human-readable in a standard `git diff`. +# The companion .json files decode the same payload into a reviewable form, +# but only if someone actually looks at them. This gate makes that look +# mandatory — the CI check goes red until the PR body contains a checked +# checkbox from the fixture review section of the PR template. +# +# To pass this check: +# 1. Review the diff of every changed .json companion file. +# 2. Tick the appropriate checkbox in the "Fixture diff review" section of +# the PR description (see .github/pull_request_template.md). + +on: + pull_request: + branches: [main, develop] + paths: + - 'contracts/tipjar/tests/fixtures/*.xdr' + - 'contracts/tipjar/tests/fixtures/*.json' + - '.github/workflows/fixture-review.yml' + +jobs: + require-fixture-review-acknowledgment: + name: Require fixture-diff review acknowledgment + runs-on: ubuntu-latest + steps: + - name: Check PR body for fixture review checkbox + env: + PR_BODY: ${{ github.event.pull_request.body }} + run: | + set -eu + + # Accept either of these checked-box patterns: + # [x] Fixture files changed, and the JSON companion diff was reviewed. + # [X] No fixture files changed in this PR. + # The second option is included so a PR that only touches + # fixture-review.yml itself (workflow change, no fixture diff) can + # still pass by ticking the "no fixtures changed" box. + if printf '%s\n' "$PR_BODY" | grep -Eiq '\[[xX]\].*(fixture|no fixture)'; then + echo "Fixture review acknowledgment found. ✓" + exit 0 + fi + + cat <<'MSG' + ────────────────────────────────────────────────────────────────────── + FIXTURE REVIEW GATE FAILED + ────────────────────────────────────────────────────────────────────── + This PR modifies one or more XDR or JSON fixture files under + contracts/tipjar/tests/fixtures/ + + Those files are golden event-schema fixtures. The companion .json + files exist precisely so that reviewers can see "the token field + moved from position 2 to position 3" in a readable diff, rather than + staring at an opaque binary blob. + + Before this PR can be merged: + 1. Open the "Fixture diff review" section of the PR description. + 2. Review every changed .json file in the PR diff. + 3. Tick the checkbox that describes what changed and confirm you + reviewed it. + + See CONTRIBUTING.md § "Event fixture golden files" for the full + process, including how to regenerate fixtures with UPDATE_FIXTURES=1. + ────────────────────────────────────────────────────────────────────── + MSG + exit 1 diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 268faba..23c9260 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -94,6 +94,82 @@ single PR. (real SAC test token, `mock_all_auths()`, `try_()` for expected errors, `env.events().all().filter_by_contract(...)` for event assertions). +## Event fixture golden files + +`contracts/tipjar/tests/fixtures/` holds binary XDR golden files that pin the +exact on-chain encoding of each event's topics and data fields: + +``` +tip_topics.xdr — VecM for the Tip event's topics +tip_data.xdr — ScVal for the Tip event's data +withdraw_topics.xdr — VecM for the Withdraw event's topics +withdraw_data.xdr — ScVal for the Withdraw event's data +``` + +Alongside every `.xdr` file there is a **human-readable `.json` companion** +(e.g. `tip_data.json`). Both files are generated and verified by the +`fixtures::event_schema_golden_fixtures` test in `src/test.rs`. + +### What the JSON files contain + +The `.json` files decode the same XDR payload into a readable representation +where each `ScVal` variant is wrapped in a one-key JSON object showing the +type: + +```json +{ + "Vec": [ + { "Address": { "Contract": "692c36..." } }, + { "Address": { "Contract": "000000...06" } }, + { "I128": "250" } + ] +} +``` + +This format is designed for git diffs: if a field moves from position 2 to +position 3, or its type changes from `I128` to `I64`, the JSON diff shows it +immediately — unlike the binary `.xdr` diff which is opaque. + +### Regenerating fixtures after an intentional event-schema change + +If you intentionally change the topics or data of the `tip` or `withdraw` +event (field order, type, presence, or any other schema detail), you must +regenerate both the `.xdr` and `.json` companion files: + +```bash +# Linux / macOS +UPDATE_FIXTURES=1 cargo test -p tipjar fixtures::event_schema_golden_fixtures + +# Windows PowerShell +$env:UPDATE_FIXTURES="1"; cargo test -p tipjar fixtures::event_schema_golden_fixtures +``` + +This overwrites both the `.xdr` binary and the `.json` companion with values +produced by the current code. + +**After regenerating:** + +1. Run `cargo test -p tipjar` (without `UPDATE_FIXTURES`) and confirm all + tests pass — the freshly written fixtures must round-trip correctly. +2. Open the "Files changed" view in your PR and read the diff of every changed + `.json` file carefully. The JSON diff is the ground truth for what changed + in the event schema. +3. Tick the appropriate checkbox in the **Fixture diff review** section of the + PR description (see `.github/pull_request_template.md`). + +### CI gate: fixture-review workflow + +`.github/workflows/fixture-review.yml` triggers on any PR that touches +`contracts/tipjar/tests/fixtures/*.xdr` or `*.json`. It fails with a +descriptive error message until the PR body contains a checked checkbox from +the fixture review section. + +This gate exists because event-schema changes are part of the contract's +on-chain interface. Off-chain indexers and the frontend SDK both depend on +`tip` and `withdraw` events having specific fields in a specific order. A +silently-absorbed schema regression would break them in production without any +compile-time signal. + ## Commit Convention Use [Conventional Commits](https://www.conventionalcommits.org/): diff --git a/contracts/tipjar/Cargo.toml b/contracts/tipjar/Cargo.toml index 477b416..de94918 100644 --- a/contracts/tipjar/Cargo.toml +++ b/contracts/tipjar/Cargo.toml @@ -20,6 +20,10 @@ soroban-sdk = "26.1.0" [dev-dependencies] soroban-sdk = { version = "26.1.0", features = ["testutils"] } proptest = "1.4" +# Used in the `fixtures` test module to produce human-readable JSON companion +# files alongside binary XDR golden fixtures — making event-schema diffs +# reviewable in PRs without decoding opaque binary blobs. +serde_json = "1.0.149" # `pause_tests` and `partial_pause_tests` live under the repo-root `tests/` # directory (shared with `tests/common`) rather than `contracts/tipjar/tests/` diff --git a/contracts/tipjar/src/test.rs b/contracts/tipjar/src/test.rs index 1b1ebe3..385a401 100644 --- a/contracts/tipjar/src/test.rs +++ b/contracts/tipjar/src/test.rs @@ -719,7 +719,11 @@ fn cancel_admin_transfer_with_nothing_pending_errors() { mod fixtures { extern crate std; use super::*; - use soroban_sdk::{testutils::Events, xdr::WriteXdr, Address, Env}; + use soroban_sdk::{ + testutils::Events, + xdr::{ScVal, WriteXdr}, + Address, Env, + }; use std::{fs, path::PathBuf}; #[test] @@ -775,26 +779,215 @@ mod fixtures { ); } + /// Decode raw XDR bytes to a JSON string that is human-readable in a PR + /// diff. The XDR payload is always a single `ScVal` (for event `data`) or + /// a `ScVec` (for event `topics`). We attempt both: first a `ScVec` + /// (topics form), then a bare `ScVal` (data form). The resulting JSON is + /// pretty-printed and deterministic. + fn xdr_to_json(xdr_bytes: &[u8]) -> String { + use soroban_sdk::xdr::{Limits, ReadXdr, ScVal, ScVec}; + + // Try decoding as ScVec (topics form) first, then as a bare ScVal + // (data form). This matches how the indexer decodes the same bytes. + let json_value = if let Ok(vec_sc) = ScVec::from_xdr(xdr_bytes, Limits::none()) { + let items: Vec = vec_sc.iter().map(scval_to_json).collect(); + serde_json::Value::Array(items) + } else { + let sc_val = ScVal::from_xdr(xdr_bytes, Limits::none()) + .unwrap_or_else(|e| { + panic!( + "Failed to decode XDR as ScVal or ScVec: {:?}", + e + ) + }); + scval_to_json(&sc_val) + }; + + // Two-space indent for readable diffs; trailing newline for clean + // `diff` output. + let mut out = serde_json::to_string_pretty(&json_value) + .expect("serde_json serialization is infallible for Value"); + out.push('\n'); + out + } + + /// Recursively convert an XDR `ScVal` to a `serde_json::Value`. + /// + /// The representation is chosen to be maximally readable in a git diff: + /// - Every variant is wrapped in a one-key object so the type is + /// immediately visible without looking at the surrounding context. + /// - Large integers that exceed JS's safe-integer range are emitted as + /// JSON strings to avoid precision loss in tools that parse the JSON. + /// - Raw byte sequences (e.g. `Bytes`, `BytesN`) are hex-encoded. + /// - `Address` is emitted as a `{"AccountId": ""}` object (the raw + /// XDR public-key bytes; the strkey form is not available in test-utils + /// without extra dependencies). + fn scval_to_json(val: &ScVal) -> serde_json::Value { + use soroban_sdk::xdr::{AccountId, Hash, PublicKey, ScAddress, Uint256}; + use serde_json::{json, Value}; + + match val { + ScVal::Bool(b) => json!({"Bool": b}), + ScVal::Void => json!({"Void": null}), + ScVal::Error(e) => json!({"Error": std::format!("{:?}", e)}), + ScVal::U32(n) => json!({"U32": n}), + ScVal::I32(n) => json!({"I32": n}), + // U64/I64: values outside JS's safe integer range are strings. + ScVal::U64(n) => { + if *n <= 9_007_199_254_740_991u64 { + json!({"U64": n}) + } else { + json!({"U64": n.to_string()}) + } + } + ScVal::I64(n) => { + if *n >= -9_007_199_254_740_991i64 && *n <= 9_007_199_254_740_991i64 { + json!({"I64": n}) + } else { + json!({"I64": n.to_string()}) + } + } + ScVal::Timepoint(tp) => json!({"Timepoint": tp.0.to_string()}), + ScVal::Duration(d) => json!({"Duration": d.0.to_string()}), + // U128/I128: always emit as strings to avoid precision loss. + ScVal::U128(parts) => { + let hi = parts.hi as u128; + let lo = parts.lo as u128; + let v = (hi << 64) | lo; + json!({"U128": v.to_string()}) + } + ScVal::I128(parts) => { + // hi is i64, lo is u64 — reconstruct the i128 value. + let hi = parts.hi as i128; + let lo = parts.lo as u128 as i128; + let v = (hi << 64) | lo; + json!({"I128": v.to_string()}) + } + ScVal::U256(parts) => { + json!({"U256": std::format!( + "{:016x}:{:016x}:{:016x}:{:016x}", + parts.hi_hi, parts.hi_lo, parts.lo_hi, parts.lo_lo + )}) + } + ScVal::I256(parts) => { + json!({"I256": std::format!( + "{:016x}:{:016x}:{:016x}:{:016x}", + parts.hi_hi, parts.hi_lo, parts.lo_hi, parts.lo_lo + )}) + } + ScVal::Bytes(b) => { + // ScBytes is BytesM — deref to &[u8] for iteration. + let hex: String = b.iter().map(|byte| std::format!("{:02x}", byte)).collect(); + json!({"Bytes": hex}) + } + ScVal::String(s) => { + // ScString is StringM — use as_slice() for bytes. + let text = std::str::from_utf8(s.as_slice()) + .map(|t| Value::String(t.to_string())) + .unwrap_or_else(|_| { + let hex: String = + s.as_slice().iter().map(|b| std::format!("{:02x}", b)).collect(); + Value::String(std::format!("0x{}", hex)) + }); + json!({"String": text}) + } + ScVal::Symbol(sym) => { + // ScSymbol has a to_string() impl that gives the symbol text. + json!({"Symbol": sym.to_string()}) + } + ScVal::Vec(Some(vec)) => { + // ScVec implements Deref. + let items: Vec = vec.iter().map(scval_to_json).collect(); + json!({"Vec": items}) + } + ScVal::Vec(None) => json!({"Vec": null}), + ScVal::Map(Some(map)) => { + // ScMap is a sorted VecM. Represent as ordered + // array of {key, val} pairs — preserves sort order and avoids + // the JSON-object restriction of string-only keys. + let pairs: Vec = map.iter() + .map(|entry| json!({ + "key": scval_to_json(&entry.key), + "val": scval_to_json(&entry.val) + })) + .collect(); + json!({"Map": pairs}) + } + ScVal::Map(None) => json!({"Map": null}), + ScVal::Address(addr) => { + match addr { + ScAddress::Account(AccountId( + PublicKey::PublicKeyTypeEd25519(Uint256(bytes)), + )) => { + let hex: String = + bytes.iter().map(|b| std::format!("{:02x}", b)).collect(); + json!({"Address": {"Account": hex}}) + } + ScAddress::Contract(Hash(bytes)) => { + let hex: String = + bytes.iter().map(|b| std::format!("{:02x}", b)).collect(); + json!({"Address": {"Contract": hex}}) + } + } + } + ScVal::LedgerKeyContractInstance => json!({"LedgerKeyContractInstance": null}), + ScVal::LedgerKeyNonce(n) => json!({"LedgerKeyNonce": n.nonce}), + ScVal::ContractInstance(_inst) => { + json!({"ContractInstance": ""}) + } + } + } + + /// Assert that `actual_xdr` matches the committed `.xdr` golden file and + /// that the decoded `.json` companion matches as well. + /// + /// When `UPDATE_FIXTURES=1` is set both files are written (or overwritten) + /// from the current run. The `.json` file exists solely so that PR diffs + /// show a human-readable representation of what changed — reviewers should + /// look at the JSON diff, not the binary XDR diff. fn assert_fixture(name: &str, actual_xdr: &[u8]) { let manifest_dir = std::env::var("CARGO_MANIFEST_DIR").unwrap(); - let fixture_path = PathBuf::from(manifest_dir) + let fixture_dir = PathBuf::from(manifest_dir) .join("tests") - .join("fixtures") - .join(std::format!("{}.xdr", name)); + .join("fixtures"); + let xdr_path = fixture_dir.join(std::format!("{}.xdr", name)); + let json_path = fixture_dir.join(std::format!("{}.json", name)); + + let actual_json = xdr_to_json(actual_xdr); if std::env::var("UPDATE_FIXTURES").is_ok() { - fs::create_dir_all(fixture_path.parent().unwrap()).unwrap(); - fs::write(&fixture_path, actual_xdr).unwrap(); + fs::create_dir_all(&fixture_dir).unwrap(); + fs::write(&xdr_path, actual_xdr).unwrap(); + fs::write(&json_path, actual_json.as_bytes()).unwrap(); } else { - let expected_xdr = fs::read(&fixture_path).unwrap_or_else(|_| { + // --- XDR assertion --- + let expected_xdr = fs::read(&xdr_path).unwrap_or_else(|_| { panic!( - "Fixture missing: {:?}. Run with UPDATE_FIXTURES=1", - fixture_path + "XDR fixture missing: {:?}. Run with UPDATE_FIXTURES=1", + xdr_path ) }); assert_eq!( expected_xdr, actual_xdr, - "Fixture {} mismatch! Run with UPDATE_FIXTURES=1 to update.", + "XDR fixture '{}' mismatch — run with UPDATE_FIXTURES=1 to regenerate, \ + then review the companion .json diff carefully before committing.", + name + ); + + // --- JSON companion assertion --- + // This is the human-readable counterpart: its diff is what + // reviewers must inspect when UPDATE_FIXTURES=1 was used in a PR. + let expected_json = fs::read_to_string(&json_path).unwrap_or_else(|_| { + panic!( + "JSON companion fixture missing: {:?}. Run with UPDATE_FIXTURES=1", + json_path + ) + }); + assert_eq!( + expected_json, actual_json, + "JSON companion fixture '{}' mismatch — if you ran UPDATE_FIXTURES=1, \ + the .json file was also regenerated. Review it carefully: the diff \ + shows exactly which event fields, types, or ordering changed.", name ); } diff --git a/contracts/tipjar/tests/fixtures/tip_data.json b/contracts/tipjar/tests/fixtures/tip_data.json new file mode 100644 index 0000000..5e690fa --- /dev/null +++ b/contracts/tipjar/tests/fixtures/tip_data.json @@ -0,0 +1,17 @@ +{ + "Vec": [ + { + "Address": { + "Contract": "692c360a04a982db02db346a106cbf008ad9e058c384bdaaf77bc0c48799b3a4" + } + }, + { + "Address": { + "Contract": "0000000000000000000000000000000000000000000000000000000000000006" + } + }, + { + "I128": "250" + } + ] +} diff --git a/contracts/tipjar/tests/fixtures/tip_topics.json b/contracts/tipjar/tests/fixtures/tip_topics.json new file mode 100644 index 0000000..c7d4537 --- /dev/null +++ b/contracts/tipjar/tests/fixtures/tip_topics.json @@ -0,0 +1,10 @@ +[ + { + "Symbol": "tip" + }, + { + "Address": { + "Contract": "0000000000000000000000000000000000000000000000000000000000000005" + } + } +] diff --git a/contracts/tipjar/tests/fixtures/withdraw_data.json b/contracts/tipjar/tests/fixtures/withdraw_data.json new file mode 100644 index 0000000..b60ca23 --- /dev/null +++ b/contracts/tipjar/tests/fixtures/withdraw_data.json @@ -0,0 +1,17 @@ +{ + "Vec": [ + { + "Address": { + "Contract": "692c360a04a982db02db346a106cbf008ad9e058c384bdaaf77bc0c48799b3a4" + } + }, + { + "I128": "250" + }, + { + "Address": { + "Contract": "0000000000000000000000000000000000000000000000000000000000000005" + } + } + ] +} diff --git a/contracts/tipjar/tests/fixtures/withdraw_topics.json b/contracts/tipjar/tests/fixtures/withdraw_topics.json new file mode 100644 index 0000000..f99340a --- /dev/null +++ b/contracts/tipjar/tests/fixtures/withdraw_topics.json @@ -0,0 +1,10 @@ +[ + { + "Symbol": "withdraw" + }, + { + "Address": { + "Contract": "0000000000000000000000000000000000000000000000000000000000000005" + } + } +]