From ce4beb7c6b3d33cb41fcd3b7a298b8fe0cc45c10 Mon Sep 17 00:00:00 2001 From: vickman787 Date: Tue, 28 Jul 2026 23:41:27 +0100 Subject: [PATCH] docs: add contributor self-review template Adds a copy-paste self-review checklist covering behaviour, tests, CI, security, edge cases, and docs impact, for contributors to run through before opening a PR. Links it from the PR template, README, and CONTRIBUTING.md, and cross-references it from the existing quality gate. Closes #513 --- .github/PULL_REQUEST_TEMPLATE.md | 7 ++ CONTRIBUTING.md | 2 + README.md | 3 + docs/contribution-quality-gate.md | 5 +- docs/self-review-template.md | 136 ++++++++++++++++++++++++++++++ 5 files changed, 152 insertions(+), 1 deletion(-) create mode 100644 docs/self-review-template.md diff --git a/.github/PULL_REQUEST_TEMPLATE.md b/.github/PULL_REQUEST_TEMPLATE.md index 2127010..cbf4629 100644 --- a/.github/PULL_REQUEST_TEMPLATE.md +++ b/.github/PULL_REQUEST_TEMPLATE.md @@ -1,5 +1,12 @@ # Pull Request + + +- [ ] I completed the [Contributor Self-Review Template](../docs/self-review-template.md) before opening this PR. + ## Issue Reference Closes # diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index bd222d2..780c641 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -99,6 +99,8 @@ Follow the [test naming convention](docs/testing.md) when adding or updating tes ## Pull request expectations +Before opening a pull request, fill in the **[Contributor Self-Review Template](docs/self-review-template.md)** — it covers behaviour, tests, CI, security, edge cases, and docs impact, and is meant to catch requirement gaps yourself before a reviewer does. + Every pull request must fill in the **[PR template](.github/PULL_REQUEST_TEMPLATE.md)** in full. The template requires: - **Issue reference** — a `Closes #N` line linking to the issue being resolved. diff --git a/README.md b/README.md index f5e4599..f911e64 100644 --- a/README.md +++ b/README.md @@ -213,6 +213,7 @@ stellar-pocketpay-contracts/ - [Admin Role](docs/admin-role.md) – Details on the admin address, current capabilities, and future design considerations. - [Admin & Emergency Mechanism Threat Model](docs/admin-pause-threat-model.md) – Security analysis of malicious admin, compromised admin, accidental pause, and blocked-withdrawal scenarios. - [Failure Mode Catalogue](docs/failure-mode-catalogue.md) – Summary of safe-failure behavior, expected errors, affected functions, and related tests for vault operations. +- [Contributor Self-Review Template](docs/self-review-template.md) – Copy-paste checklist covering behaviour, tests, CI, security, edge cases, and docs impact — fill it in before opening a PR. - [Traceability Table Guide](docs/traceability-table.md) – Standard format for mapping PR changes to issue acceptance criteria, with worked examples. - [Payment-Period Conduct Guidance](docs/payment-period-conduct.md) – Expectations for how contributors raise payment-status questions, and how GrantFox's evaluation process relates to this repository's review process. @@ -270,6 +271,8 @@ See **[CONTRIBUTING.md](CONTRIBUTING.md)** for the full guide, including: - How to run the test suite (`cargo test --workspace`) - PR checklist and commit message conventions +Before opening a PR, fill in the **[Contributor Self-Review Template](docs/self-review-template.md)** — a checklist covering behaviour, tests, CI, security, edge cases, and docs impact — so requirement gaps are caught before a reviewer looks at the PR. + Every pull request must use the **[PR template](.github/PULL_REQUEST_TEMPLATE.md)**, which requires: - A reference to the issue being fixed (`Closes #N`) diff --git a/docs/contribution-quality-gate.md b/docs/contribution-quality-gate.md index 12c98e4..2270577 100644 --- a/docs/contribution-quality-gate.md +++ b/docs/contribution-quality-gate.md @@ -56,7 +56,10 @@ To avoid common pitfalls, here are examples of PRs that do **not** pass the qual ## 4. How to Use This Gate -1. **Before Opening a PR**: Review your work against this checklist. +1. **Before Opening a PR**: Review your work against this checklist. The + **[Contributor Self-Review Template](self-review-template.md)** packages + this checklist (plus the security and edge-case checklists) into a single + copy-paste worksheet you can fill in before opening the PR. 2. **In Your PR Description**: Reference this quality gate and confirm that all items are checked. 3. **Reviewers**: Use this checklist as the primary framework for your review. If the gate isn't met, request changes immediately. diff --git a/docs/self-review-template.md b/docs/self-review-template.md new file mode 100644 index 0000000..8bbce7c --- /dev/null +++ b/docs/self-review-template.md @@ -0,0 +1,136 @@ +# Contributor Self-Review Template + +This is a copy-paste template for contributors to fill in and run through +**before** opening a pull request or requesting review. It exists so that a +contributor checks their own work against the issue's acceptance criteria — +behaviour, tests, CI, security, edge cases, and docs — the same way every +time, instead of relying on a reviewer to catch missing requirements first. + +This is not a new review process. It packages checks that already exist +elsewhere in this repo (the [Contribution Quality Gate](contribution-quality-gate.md), +the [Traceability Table Guide](traceability-table.md), the +[Contract Contributor Security Checklist](security-checklist.md), and the +[Invariant Test Checklist](invariant-test-checklist.md)) into one worksheet you +can fill in locally, then paste into your PR description. + +## How to use this template + +1. Copy the [Self-review checklist](#self-review-checklist) section below into + a scratch file, or directly into your PR description. +2. Fill in every section against the issue you're resolving. Use `—` or "not + applicable" for sections that genuinely don't apply (for example, a + documentation-only PR has no contract functions to list) — don't delete a + section just because it's inconvenient. +3. Run `make verify` and paste the result in the CI section. +4. Only open the PR, or move it out of draft, once every checkbox is either + checked or explicitly marked not applicable with a reason. +5. Leave the filled-in checklist in the PR description alongside the + [traceability table](traceability-table.md) — reviewers use both together. + +If an item doesn't pass, fix it before requesting review rather than noting it +as a known gap. See [Examples of Incomplete Work](contribution-quality-gate.md#3-examples-of-incomplete-work) +for the kind of PR this template is meant to catch before it reaches a +reviewer. + +--- + +## Self-review checklist + +### 1. Behaviour + +- [ ] I can restate, in my own words, what the linked issue asks for. +- [ ] Every acceptance criterion in the issue is addressed by this PR (or I've + explained in the PR description which ones are deferred and why). +- [ ] I've filled in the [traceability table](traceability-table.md) mapping + each acceptance criterion to the function(s), test(s), and edge cases + that satisfy it. +- [ ] No `TODO`, `FIXME`, or `HACK` markers remain in the code this PR touches. +- [ ] The change is scoped to one issue — unrelated refactors or fixes are + split into a separate PR. + +### 2. Tests + +- [ ] Every new or changed function has tests for both the happy path and + failure paths (unauthorized caller, invalid input, insufficient + balance, etc.), per the [test naming convention](testing.md). +- [ ] Changes to accounting logic are covered by the relevant + [property tests](../contracts/savings_vault/src/test/property_vault_accounting.rs) + and checked against the [Invariant Test Checklist](invariant-test-checklist.md). +- [ ] Event schema changes have updated + [snapshots](../contracts/savings_vault/test_snapshots/). +- [ ] `cargo test --workspace` passes locally — not just the tests I added. + +### 3. CI + +- [ ] `make verify` passes locally (format, Clippy, workspace tests, release + WASM build). If you ran the steps individually instead, all of + `cargo fmt --check`, `cargo clippy --tests -- -D warnings`, and + `cargo test --workspace` pass. +- [ ] `make build-release` succeeds and the WASM size is reported; I checked + for unexpected artifact size growth. +- [ ] I pushed the branch and confirmed CI is green on GitHub before + requesting review, not just locally. + +### 4. Security + +- [ ] I've gone through the applicable sections of the + [Contract Contributor Security Checklist](security-checklist.md) for + this change (balances, lock state, token transfer atomicity, + authorisation, storage, events, error codes) and noted "not applicable" + for sections this PR doesn't touch, with a reason. +- [ ] Every new or changed state-changing function calls `require_auth()` on + the correct address. +- [ ] New error paths use `ContractError` variants with `panic_with_error!`, + not bare `panic!`. +- [ ] Storage layout changes follow the + [storage change checklist](storage-change-checklist.md); event changes + follow the [event compatibility policy](event-compatibility-policy.md). +- [ ] No secrets, private keys, seed phrases, or populated credential files + are committed, logged, or included in test fixtures. + +### 5. Edge cases + +- [ ] Boundary amounts are tested: `0`, `1`, the maximum representable + amount, and values just above/below any configured limit. +- [ ] For time-locked behaviour, tests cover exactly at, just before, and just + after the relevant maturity or expiry timestamp. +- [ ] Failure paths leave storage byte-for-byte unchanged — no partial + mutation before the failure point (see + [Balance Reconciliation Design Note](balance-reconciliation.md)). +- [ ] Multi-entry state (multiple locks, multiple users) is tested for + independence — acting on one entry doesn't affect another. + +### 6. Docs + +- [ ] Behaviour or architectural changes are reflected in the relevant + `docs/` file(s), not just in code comments. +- [ ] If this PR changes a function signature or add/removes a function, the + [API reference](api-reference.md) and README + [Features](../README.md#features) table are updated to match. +- [ ] New docs are linked from the README + [Documentation](../README.md#documentation) list, per the + [Documentation Style Guide](docs-style-guide.md#linking-related-docs). +- [ ] Wording follows the [Documentation Style Guide](docs-style-guide.md) + (testnet-only framing, no production claims, placeholder values only). + +--- + +## Relationship to other docs + +This template doesn't replace any existing PR requirement — it's the +checklist you personally run through before the PR template and traceability +table get filled in for reviewers. + +- [PR Template](../.github/PULL_REQUEST_TEMPLATE.md) — the contributor fills + this in for the reviewer; this self-review template is what you complete + first, on your own, before that. +- [Contribution Quality Gate](contribution-quality-gate.md) — the fuller + narrative explanation of "payment-ready" work, with examples of what + incomplete work looks like. This template is the condensed, fill-in-the-blank + version of that gate. +- [Traceability Table Guide](traceability-table.md) — the standard format for + mapping acceptance criteria to functions and tests; fill this in as part of + the Behaviour section above. +- [Contract Contributor Security Checklist](security-checklist.md) and + [Invariant Test Checklist](invariant-test-checklist.md) — the detailed + checklists the Security and Edge Cases sections above summarize.