From f81fd2a6edfb14f82e5d4262660bc4a1caf4f15f Mon Sep 17 00:00:00 2001 From: Damola09 Date: Tue, 29 Sep 2026 02:56:46 +0100 Subject: [PATCH] docs: document group_treasury storage/multisig, local object store, and contribution workflow Closes #469 Closes #564 Closes #471 Closes #540 --- CONTRIBUTING.md | 156 ++++++++++++++++ README.md | 3 + .../docs/concepts-local-object-store.md | 172 ++++++++++++++++++ .../docs/concepts-treasury-multisig-model.md | 145 +++++++++++++++ .../docs/contracts-group-treasury-storage.md | 168 +++++++++++++++++ 5 files changed, 644 insertions(+) create mode 100644 CONTRIBUTING.md create mode 100644 apps/backend/docs/concepts-local-object-store.md create mode 100644 contracts/docs/concepts-treasury-multisig-model.md create mode 100644 contracts/docs/contracts-group-treasury-storage.md diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md new file mode 100644 index 0000000..f17b0d1 --- /dev/null +++ b/CONTRIBUTING.md @@ -0,0 +1,156 @@ +# Contributing to clicked + +## Pull requests must target `dev` + +**Open every pull request against `dev`, not `main`.** + +If you open a PR against `main` and you are not the repository maintainer (or a collaborator +with admin/maintain permission), the `guard-main-branch.yml` workflow automatically: + +1. Posts a comment on the PR explaining that PRs must target `dev`. +2. Closes the PR (`state: closed`). + +**This is not a rejection of your work.** It is an automated branch-policy check, not a review +outcome — nobody looked at your code and decided against it. To continue: open a new PR with +the same branch against `dev` (or retarget the base branch on the existing PR and reopen it, +per the bot's comment — GitHub's base-branch editing UI is available on an open or reopened +PR; once the workflow has already closed it, opening a fresh PR against `dev` is the reliable +path). + +`dev` is the integration branch contributors merge into; `main` is reserved for the +maintainer. This is why the `close-linked-issues.yml` workflow (below) exists at all — GitHub's +built-in "Closes #N" auto-close only fires on merges to the *default* branch, and this repo's +default branch is `main`, not `dev`. + +## Forking and branching + +```bash +# 1. Fork the repo on GitHub, then clone your fork +git clone https://github.com//clicked.git +cd clicked + +# 2. Add the upstream repo so you can pull in new dev commits later +git remote add upstream https://github.com/codebestia/clicked.git + +# 3. Make sure you're branching from an up-to-date dev +git fetch upstream +git checkout -b your-feature-branch upstream/dev + +# 4. ...make your changes, keeping the branch focused on one concern... + +# 5. Push to your fork +git push origin your-feature-branch + +# 6. Open a PR: base = codebestia/clicked:dev, compare = /clicked:your-feature-branch +``` + +Keep unrelated changes out of the branch — a docs fix and a feature change belong in separate +PRs so each can be reviewed and merged independently. + +## Commit conventions + +This repository follows [Conventional Commits](https://www.conventionalcommits.org/), with an +optional scope naming the area touched. Examples consistent with the existing commit history: + +``` +feat: add protection workflow +feat(backend): lock in Signal-protocol server invariants with guards and tests +feat(web): session reset and re-handshake on peer identity key change +fix(messages): apply shared payload validation to file messages +fix(conversations): serialize list preview to prevent plaintext leaks +docs: add testing strategy, migration workflow, and security policy +docs(backend): add background GC jobs reference +docs(web): add frontend accessibility guide +docs(contracts): add contract testing guide +``` + +- **Format:** `(): `. +- **Common types:** `feat`, `fix`, `docs` (also seen in history: `style`, `refactor`, `chore`, + `test`). +- **Scopes in use:** `backend`, `web`, `contracts`, `ai_agent`, `messages`, `conversations`, + `security`, among others — pick the one matching the app or area your change touches, or omit + the scope for changes that span multiple areas. +- Write the description in the imperative, present tense (`add`, not `added`/`adds`), matching + the pattern above. + +## Linking issues + +If your PR closes an issue, reference it with a closing keyword directly in the PR title or +body: `Closes #123`, `Fixes #123`, or `Resolves #123` (case-insensitive; `-s`/`-es`/`-d`/`-ed` +suffixes are all recognized). + +When your PR merges into `dev`, the `close-linked-issues.yml` workflow scans the PR title and +body for that pattern and closes each matched issue, with a comment +(`Closed by #, merged into dev.`) — this replaces GitHub's native auto-close, which only +triggers on merges to the default branch (`main`). + +**Limitation to know about:** the workflow's matching regex requires the closing keyword +immediately before each issue number. `Closes #123, #124` only closes `#123` — `#124` has no +keyword directly in front of it and will be left open. If your PR closes multiple issues, repeat +the keyword for each: `Closes #123, Closes #124`. + +## PR template + +Every PR uses the template at +[`.github/pull_request_template.md`](.github/pull_request_template.md), which asks for: + +- A **description** of the change. +- The **type of change** (bug fix / new feature / documentation update / other). +- A **checklist**: contributing guidelines read, changes tested locally, code follows the + project's coding standards. + +Fill in the description and check off the boxes that genuinely apply — an unchecked box is a +signal to the reviewer, not just a formality. + +## CI requirements + +CI is split by area, using GitHub Actions `paths` filters — a workflow only runs when a PR +touches the paths it's scoped to. Two checks run on every PR regardless of what changed. + +### Repository-wide (always run) + +- **PR Check** (`pr.yml`) — installs dependencies and runs `pnpm run lint` at the repo root. +- **Security CI** (`security-ci.yml`) — runs on every PR (and every push to `main`): + - `regression`: the ciphertext-only guard and secret-field scan + (`apps/backend/src/__tests__/security.regression.test.ts`). + - `dependency-audit`: `pnpm audit` scoped to crypto-relevant backend dependencies (`ioredis`, + `jsonwebtoken`, `web-push`, `@stellar/stellar-sdk`, `drizzle-orm`, `socket.io`). + +### Backend (`apps/backend/**`) + +`backend-ci.yml` spins up Postgres, Redis, and MinIO containers, runs migrations, then: +`pnpm format:check`, `pnpm lint`, `pnpm test`. (The suite's actual tests don't dial into those +containers — see [`apps/backend/docs/testing.md`](apps/backend/docs/testing.md) — the +containers exist so CI can validate migrations against a real Postgres.) + +### Web (`apps/web/**`) + +`frontend-ci.yml`: `pnpm --filter web lint`, `pnpm --filter web test`, +`pnpm --filter web build`. + +### AI agent (`apps/ai_agent/**`) + +`ai-agent-ci.yml`, three jobs: `ruff check` + `ruff format --check` (lint), `mypy main.py` +(typecheck), and `pytest --cov=main` (test + coverage, uploaded to Codecov). + +### Contracts (`contracts/**`) + +`contracts-ci.yml`, matrixed over the `token_transfer`, `group_treasury`, and `proposals` +packages: `cargo test -p `, then `cargo build -p --target +wasm32-unknown-unknown --release` with a 100 KB per-WASM size gate reported as a PR comment. +A separate `clippy` job runs `cargo clippy --workspace --target wasm32-unknown-unknown -- -D +warnings` (with `dead_code` and `too-many-arguments` allowed), and an `audit` job runs +`cargo audit`. This workflow also runs on a weekly schedule independent of any PR. + +### Running checks locally + +Match the workflow's own commands where practical: `pnpm lint` / `pnpm format:check` / +`pnpm test` for the app you touched, or `cargo test -p ` / +`cargo clippy --workspace --target wasm32-unknown-unknown` for contracts. Running the same +commands the workflow runs, in the affected app's directory, is the most reliable way to know +CI will pass before you push. + +--- + +See also the top-level [documentation index](docs/README.md) for architecture and per-app +references. diff --git a/README.md b/README.md index c793e85..c0db05e 100644 --- a/README.md +++ b/README.md @@ -172,6 +172,9 @@ Frequently opened: We welcome contributions from developers, designers, and researchers. +**➡️ See [CONTRIBUTING.md](CONTRIBUTING.md)** for the branch policy (PRs target `dev`, not +`main`), commit conventions, issue linking, and CI requirements before opening a PR. + --- ## 📌 How to Contribute diff --git a/apps/backend/docs/concepts-local-object-store.md b/apps/backend/docs/concepts-local-object-store.md new file mode 100644 index 0000000..70c3cf3 --- /dev/null +++ b/apps/backend/docs/concepts-local-object-store.md @@ -0,0 +1,172 @@ +# Local Object Store (development/test) + +Source: [`src/lib/localObjectStore.ts`](../src/lib/localObjectStore.ts), +[`src/lib/objectStore.ts`](../src/lib/objectStore.ts), +[`src/routes/localStorage.ts`](../src/routes/localStorage.ts), +[`src/lib/storage.ts`](../src/lib/storage.ts), +[`src/routes/uploads.ts`](../src/routes/uploads.ts). + +See also: [File Uploads API](api-files-uploads.md) for the upload/confirm route contract this +store backs, and the [backend testing guide](testing.md) / +[cross-app testing strategy](../../../docs/testing.md) for the no-Docker-in-tests policy this +store exists to satisfy. + +## `ObjectStoreLike` — the shared interface + +Both the production `ObjectStore` (S3-compatible, via `@aws-sdk/client-s3`) and the +development-only `LocalDiskObjectStore` implement the same `ObjectStoreLike` interface, defined +in `objectStore.ts`: + +```ts +export interface ObjectStoreLike { + putObject(key: string, body: ..., contentType?: string): Promise; + getObject(key: string): Promise; + deleteObject(key: string): Promise; + headObject(key: string): Promise<{ exists: boolean; size?: number }>; + getPresignedPutUrl(key: string, contentType: string | undefined, ttlSeconds: number): Promise; + getPresignedGetUrl(key: string, ttlSeconds: number): Promise; +} +``` + +**Call sites only ever depend on `ObjectStoreLike`** (`getObjectStore()`'s return type, +imported by `routes/uploads.ts` and `lib/storage.ts`); nothing in the application imports +`ObjectStore` or `LocalDiskObjectStore` directly except the factory code itself. The backend +selected underneath is swapped without call-site changes — verified below. + +| Method | Purpose | Local (`LocalDiskObjectStore`) behavior | Production (`ObjectStore`) behavior | +|---|---|---|---| +| `putObject(key, body, contentType?)` | Store bytes at `key`. | Writes the body to a file under the local storage root, plus a sidecar `.meta.json` recording `contentType`. Creates parent directories as needed (`mkdir(..., { recursive: true })`). | `PutObjectCommand` against the S3-compatible bucket. | +| `getObject(key)` | Retrieve an object. | Reads the file and its sidecar meta file; returns `{ Body: Buffer, ContentType? }`. Throws (propagates the `fs` error) if the file doesn't exist. | `GetObjectCommand`; returns the raw SDK response. The two implementations' return shapes are intentionally left loosely typed (`unknown`) because nothing in the codebase consumes `getObject()` polymorphically today — callers that use it know which implementation they're calling. | +| `deleteObject(key)` | Remove an object. | `rm` on both the object file and its sidecar meta file, with `{ force: true }` (no error if already absent). | `DeleteObjectCommand`. | +| `headObject(key)` | Existence + size check, **used by upload-confirm verification** (`routes/uploads.ts`, `#356`). | `stat()`s the resolved path; returns `{ exists: true, size }` on success, `{ exists: false }` on any error (e.g. `ENOENT`). | `HeadObjectCommand`; maps `NotFound` / `NoSuchKey` names or a `404` status code to `{ exists: false }`, otherwise returns `{ exists: true, size: ContentLength }` (omitting `size` if the SDK didn't return `ContentLength`). Any other error is rethrown rather than swallowed. | +| `getPresignedPutUrl(key, contentType, ttlSeconds)` | Issue a time-limited upload URL. | Builds a URL back to this backend's own `/local-storage/` route (see below) with an HMAC signature and an `expires` timestamp `ttlSeconds` seconds out. `contentType` is accepted for interface parity but not used to constrain the signature. | Real S3 presigned URL via `getSignedUrl` from `@aws-sdk/s3-request-presigner`, expiring after `ttlSeconds`. | +| `getPresignedGetUrl(key, ttlSeconds)` | Issue a time-limited download URL. | Same local-URL scheme as the PUT case, signed for `GET`. | Real S3 presigned GET URL. | + +`headObject` matters specifically because `routes/uploads.ts`'s upload-confirm handler calls it +to prove an object actually landed in the store — at the expected size — before marking a file +row `ready`; both implementations must answer this the same way for that check to behave +identically in dev and production. + +## Local filesystem behavior + +- **Root directory:** `process.env.LOCAL_STORAGE_DIR`, falling back to `/.local-storage` + (`DEFAULT_ROOT_DIR` in `localObjectStore.ts`). This directory is expected to be gitignored; + this document does not assume any specific absolute path beyond that default. +- **Key → path mapping:** `resolvePath(key)` joins the root with `key` via `node:path`'s + `join`/`normalize`, then verifies the resolved path is still inside the root + (`resolved === root || resolved.startsWith(root + sep)`); otherwise it throws + `"Invalid storage key: "`. This is the path-traversal guard — a key like + `../../etc/passwd` cannot escape the root directory. +- **Directory creation:** `putObject` creates any missing parent directories with + `mkdir(dirname(path), { recursive: true })` before writing, so nested keys (e.g. + `uploads//`, the shape `generateStorageKey` in `lib/storage.ts` + produces) work without a separate provisioning step. +- **Metadata persistence:** yes, but only `contentType`, and only in a sidecar file + `.meta.json` next to the object (`metaPath(key)`), written by `putObject` and + read back by `getObject`. There is no metadata store beyond this — no size, no timestamps, + no arbitrary user metadata. +- **Missing objects:** `getObject` and `putObject`'s underlying `fs` calls throw/reject on a + missing file; `headObject` and the exported test helper `exists()` instead catch the error + and return `false`/`{ exists: false }`. Callers that need a non-throwing existence check use + `headObject`. +- **Concurrent writes:** no special handling — `writeFile` is used as-is, with no locking, + temp-file-then-rename, or write queuing. Two concurrent `putObject` calls to the same key race + at the filesystem level exactly as plain `fs.writeFile` calls would. + +## Signed local URLs + +Format (both PUT and GET use the same shape, differing only in HTTP method and which key each +verifies against): + +``` +http://localhost:/local-storage/?expires=&sig= +``` + +- **Base URL:** `process.env.STORAGE_ENDPOINT` if set (trailing slashes stripped), otherwise + `http://localhost:${process.env.PORT ?? 3001}/local-storage`. +- **Object identifier:** the storage `key`, appended as the path segment(s) after + `/local-storage/` (e.g. `uploads//`). +- **`expires`:** a Unix timestamp in seconds — `Math.floor(Date.now() / 1000) + ttlSeconds` at + signing time. +- **`sig`:** `HMAC-SHA256(processSecret, "::")`, hex-encoded. + `processSecret` is `randomBytes(32)` generated once per process at module load — it is not + persisted or shared across processes, which is fine because the same process that signs a URL + is the one that verifies it later in the same dev/test run. + +Example (placeholders, not a real signature): + +``` +http://localhost:3001/local-storage/uploads/abc123/def456?expires=1780000000&sig=4f3c...e9a1 +``` + +### Validation on the serving route + +`routes/localStorage.ts` mounts `PUT` and `GET` handlers on `/*splat` under `/local-storage` +(only outside production — see below). Both call `checkSignature(req, res, method)`, which: + +1. **Object/key:** extracts the key from the wildcard route params (`req.params['splat']`, + joined with `/` if Express gives it as an array). +2. **Signature:** reads `sig` from the query string and calls + `verifySignedRequest(method, key, expires, sig)` in `localObjectStore.ts`, which + recomputes the expected HMAC for `(method, key, expires)` and compares it to the supplied + signature using `timingSafeEqual` (constant-time, after confirming both buffers are the same + length — a length mismatch is treated as a non-match rather than passed to + `timingSafeEqual`, which throws on unequal lengths). +3. **Expiry:** `verifySignedRequest` also checks `Number.isFinite(expires)` and rejects if + `Date.now() / 1000 > expires`. +4. **No other validation** is performed by this route beyond signature + expiry — there is no + separate auth/session check, which is intentional: the route is mounted outside + `requireAuth` (see `app.ts`) because a valid signed URL is meant to be the credential, the + same way a real S3 presigned URL is. + +On failure, the route responds `403 { error: 'Invalid or expired signed URL' }` without +distinguishing "bad signature" from "expired" from "missing key/sig" in the response body. + +The `PUT` handler reads the raw body (`express.raw({ type: () => true, limit: '150mb' })`) and +the `content-type` header, then calls `getLocalObjectStore().putObject(key, body, contentType)`. +The `GET` handler calls `getObject(key)` and streams back `Body` with `Content-Type` set from +the stored metadata if present, returning `404 { error: 'Object not found' }` on any error +(e.g. the file doesn't exist). + +## Production/development selection + +Two independent switches exist in the codebase, both keyed on `process.env.NODE_ENV`: + +- **`objectStore.ts`'s `getObjectStore()`** (used by `routes/uploads.ts` for `headObject`, and + elsewhere for delete/head operations): a lazily-constructed singleton that picks + `createObjectStore(loadEnv())` (real S3 client) when `NODE_ENV === 'production'`, otherwise + `getLocalObjectStore()`. `resetObjectStoreForTests()` clears the singleton so tests can + change env and re-resolve it. +- **`lib/storage.ts`'s `generatePresignedPut` / `generatePresignedGet`**: each has its own + `isProduction()` check (`process.env.NODE_ENV === 'production'`) and calls + `getLocalObjectStore()` directly outside production, or `getObjectStore()` in production. This + is a second, independent branch rather than routing through `getObjectStore()`'s own switch — + both land on the same backend selection, but it's worth knowing these are two separate + `NODE_ENV` checks in the source rather than one shared decision point. +- **Route mounting:** `app.ts` only mounts `localStorageRouter` at `/local-storage` when + `process.env['NODE_ENV'] !== 'production'` — in production the route doesn't exist at all, so + a presigned local URL could never be dereferenced even if one were somehow issued. + +**Call-site agnosticism, verified:** `routes/uploads.ts` calls `getObjectStore().headObject(...)` +without any environment branching of its own — it only ever sees `ObjectStoreLike`. The +environment switch lives entirely in the two factory functions above, not in application code +that consumes the store. + +## Test architecture: why this exists + +The [cross-app testing strategy](../../../docs/testing.md) states the project-wide rule +directly: **no test in the repository may require Redis, Postgres, or an S3/MinIO server to be +running.** `LocalDiskObjectStore` is what makes that rule achievable for object-store-dependent +code: `getObjectStore()` resolves to it automatically whenever `NODE_ENV !== 'production'` +(the default for `vitest`), so upload/presign/download/head flows exercise a real, working +implementation — actual bytes on disk, actual signature verification — without a Docker +dependency, on a fresh clone, with Docker stopped. + +This does **not** mean every backend test is Docker-free for the same reason: the same +cross-app testing document also states that Postgres access goes through a hand-built `db` +mock rather than a real database in tests. `LocalDiskObjectStore` specifically replaces the +need for a MinIO/S3 dependency; it makes no claim about, and has no bearing on, how other +external dependencies (Postgres, Redis) are avoided in the test suite. Separately, the backend +CI workflow (`backend-ci.yml`) does start real Postgres, Redis, and MinIO containers — but per +the testing doc, that's to validate migrations against a real Postgres, not because the test +suite itself dials into any of those services. diff --git a/contracts/docs/concepts-treasury-multisig-model.md b/contracts/docs/concepts-treasury-multisig-model.md new file mode 100644 index 0000000..0fa6713 --- /dev/null +++ b/contracts/docs/concepts-treasury-multisig-model.md @@ -0,0 +1,145 @@ +# `group_treasury` Multisig / Authorization Model + +Source: [`contracts/contracts/group_treasury/src/lib.rs`](../contracts/group_treasury/src/lib.rs), +[`contracts/contracts/group_treasury/src/test.rs`](../contracts/group_treasury/src/test.rs). + +For the exact storage keys, types, and the proposal state diagram, see +[`contracts-group-treasury-storage.md`](contracts-group-treasury-storage.md). For function +signatures, see [`api-proposals.md`](api-proposals.md) (the related `proposals` contract) — +`group_treasury` itself has no dedicated API reference page in `contracts/docs/` today; this +document and the storage doc above are the canonical references for its own functions, cited +by exact name from `lib.rs`. + +## Two independent authorization surfaces + +`group_treasury` has two ways funds can move, and they do not share logic: + +1. **Admin-gated `withdraw(to, token, amount)`** — a single admin address, set once at + `initialize`, can move funds directly. No proposal or vote is involved. +2. **Member-voted proposals** (`propose_withdraw` / `approve_withdraw` / `reject_withdraw`) — + the subject of this document. As of the current `lib.rs`, reaching `Passed` does **not** + move funds: there is no execute entrypoint that reads a `Passed` proposal and calls + `withdraw` on its behalf. The external `proposals` contract calls this contract's + `is_member` / `balance` / `withdraw` after running its *own*, separate proposal/vote/finalize + cycle (see `contracts/contracts/proposals/src/treasury_interface.rs` and + [`api-proposals.md`](api-proposals.md)) — it does not call `propose_withdraw` or + `approve_withdraw` on `group_treasury`. Treat the two proposal systems as parallel, not + layered. + +Everything below describes surface 2, the `group_treasury`-internal voting system. + +## Membership + +- **Adding a member:** `add_member(member)`. Admin-only (`require_admin`, i.e. + `admin.require_auth()`). Panics with `"member already exists"` if the address is already in + `DataKey::Members` — duplicates are prevented by a linear scan before insertion. +- **Removing a member:** `remove_member(member)`. Admin-only. Panics with + `"member not found"` if the address isn't currently a member. Implemented by rebuilding the + members vector without the target address (not swap-remove), so relative order of remaining + members is preserved. +- **Self-removal:** `remove_member` takes an arbitrary `member` argument but the call itself is + gated by `require_admin`, not by the caller being `member`. An admin who is also a member + could call `remove_member(self)`, and any member removal (including of the admin's own + address as a member entry, if it were ever added as one) is authorized the same way — there + is no separate "members can remove themselves" path. +- **Effect on existing proposals:** membership changes do **not** touch any + `DataKey::Proposal(id)` record. A proposal created while an address was a member keeps + whatever `approvals`/`rejections` it already accumulated even if that address is later + removed — the counters are not recomputed. However, a *newly removed* address can no longer + vote (`require_votable` calls `is_member` at vote time, checking current membership, not + membership at proposal-creation time), and a *newly added* member can vote on any + still-`Active` proposal that predates their membership, since eligibility is also checked + against current membership. + +## Threshold model + +`Threshold` (a `u32`) is the number of approval votes a proposal needs to move from `Active` to +`Passed`. It is **not** expressed as "N of M current members" in the code — it is a fixed +number set once, independent of how membership changes afterward. + +- **Storage:** `DataKey::Threshold`, instance storage (see the storage doc for tier details). +- **Initial value:** set by the `threshold` parameter to `initialize`, which panics with + `"threshold must be at least 1"` if `threshold == 0`. There is no other default. +- **Who can set it:** only `initialize`, which itself panics with `"already initialized"` if + called a second time. **There is no `set_threshold` function in `lib.rs`** — the threshold + cannot be changed after initialization by any current entrypoint. +- **Valid range:** any `u32 >= 1`. The contract does not validate `threshold` against the + member count (which is `0` at `initialize` time in any case, since members are added + afterward via `add_member`) — a treasury could be initialized with a threshold higher than + it will ever have members, making it permanently impossible to pass a proposal. See + Limitations below. +- **Changing membership after the threshold is set:** since threshold is fixed and membership + is not, the *effective* difficulty of reaching `Threshold` approvals rises and falls as + members are added or removed. Example: `Threshold = 2`, 3 members. If one member is removed + (2 members left), a proposal still needs 2 approvals — now unanimous among the remaining + members instead of 2-of-3. + +## Withdrawal authorization: why proposal → approvals → threshold, not one signature + +A single member's approval is deliberately insufficient — `approve_withdraw` only flips +`status` to `Passed` once `proposal.approvals >= threshold`, and there is no path that lets one +vote or one admin action mark a member-voted proposal as approved early. This requires +independent agreement from multiple members before a proposal is even eligible to move funds +(when a future execution step consumes `Passed`), rather than trusting any single member's +judgment. + +- **Who may create proposals:** any current member, via `propose_withdraw` + (`proposer.require_auth()` + `is_member` check). Also requires `amount > 0` and that the + treasury's recorded balance for `token` is `>= amount` **at creation time**. +- **Who may approve/reject:** any current member, via `approve_withdraw` / `reject_withdraw`. + Both require `require_auth()` from the voter and `is_member` on the voter. +- **Can the proposer approve their own proposal?** Yes, implicitly and automatically — + `propose_withdraw` sets `approvals: 1` and records `DataKey::Vote(id, proposer) = true` at + creation. The proposer does not call `approve_withdraw` separately for this; doing so + afterward would hit the "already voted" panic below. +- **Duplicate approval/rejection:** `require_votable` checks + `env.storage().instance().has(&DataKey::Vote(proposal_id, voter))` and panics with + `"already voted"` if the address has already voted in either direction. A member cannot vote + twice, and cannot switch an approval to a rejection (or vice versa) after voting once. +- **Required number of approvals:** exactly `Threshold` (the stored value, not derived from + member count at vote time). +- **Approvals tied to current or proposal-time membership?** Current membership, checked at + each vote via `is_member`. Whether a given address's *earlier* vote still "counts" after that + address is removed is not re-validated — the `approvals` counter is never decremented on + membership change, so a vote cast while a member remains counted after removal. +- **When does execution become possible?** As of the current `lib.rs`, it doesn't — reaching + `Passed` is a terminal state as far as `group_treasury`'s own functions are concerned. + `src/test.rs` (`multisig_flow::multisig_execute_on_approved`, `#[ignore]`d) documents this as + planned but unimplemented, with no issue identified in-repo at the time of writing. +- **Who may execute, and must they be an approver?** Not applicable today — there is no + execute function. +- **What happens after execution?** Not applicable today. + +## Security properties and limitations + +**What the model does provide today:** +- No single member (other than the admin, via the separate `withdraw` path) can move funds + through the proposal system — reaching `Passed` requires `Threshold` distinct members' + authenticated approvals. +- Each member's vote is authenticated (`require_auth`) and counted at most once per proposal. +- A proposal can be blocked before reaching threshold: once `rejections` reaches the + *blocking minority* — `member_count.saturating_sub(threshold) + 1`, i.e. the smallest number + of rejections that makes it mathematically impossible for the remaining members to still + supply `Threshold` approvals — `reject_withdraw` flips the proposal to `Rejected`. Example + from `src/test.rs`: `Threshold = 2` of 3 members → blocking minority = `3 - 2 + 1 = 2`. +- Voting past `expires_at` is blocked: `require_votable` panics with `"proposal expired"` once + `env.ledger().timestamp() >= proposal.expires_at`, even though (see below) the stored + `status` does not itself change to `Expired`. + +**Known limitations, verified against the current source (not speculative):** + +| Scenario | Actual behavior | +|---|---| +| Threshold never reached, proposal not rejected either | The proposal stays `Active` indefinitely once past `expires_at`; nothing in `lib.rs` transitions it out. `get_pending_proposals` will keep returning it (it filters on `status == Active`, not on `expires_at`) even though no further approval can be recorded past expiry. | +| Fewer members than `Threshold` | `initialize` does not check members against threshold (members don't exist yet at init time), and nothing later re-validates it. A treasury can end up with `Threshold` permanently unreachable if members are removed below that count, with no function to lower `Threshold` to compensate. | +| Member removed during an active vote | Verified in `remove_member`/`require_votable`: removal only updates `DataKey::Members`. It does not touch any `DataKey::Proposal` or `DataKey::Vote` entry. A removed member's prior vote stays counted in `approvals`/`rejections`; they simply can no longer cast new votes (`is_member` check fails on their next attempt). | +| Threshold changed during an active proposal | Not reachable — there is no function to change `Threshold` after `initialize`, so this scenario cannot occur in the current contract. | +| Stale / expired proposals | `require_votable`'s expiry check blocks new votes, but no function transitions `status` to `Expired` or otherwise finalizes a stale proposal. It remains `Active` in storage and in `list_proposals`/`get_pending_proposals` output. | +| Rejected / cancelled proposals | `Rejected` is terminal — no function reads or acts on a `Rejected` proposal afterward. There is no separate "cancel" function; rejection via `reject_withdraw` is the only way to end a proposal without reaching threshold. | +| Already-executed proposals | Not reachable — no function ever sets `status` to `Executed` in the current contract, so this state cannot occur yet. | +| Expired proposals (status-level) | As above, `Expired` is defined in `ProposalStatus` but no function sets it; expiry is enforced only as a vote-time guard, not a stored transition. | + +Do not read the presence of `Executed`/`Expired` in the `ProposalStatus` enum, or the +`multisig_execute_on_approved` / `multisig_finalize_expired` test names, as evidence that +execution or expiry-finalization exists — both are explicitly `#[ignore]`d in `src/test.rs` +with a stated reason of being blocked on not-yet-implemented functionality. diff --git a/contracts/docs/contracts-group-treasury-storage.md b/contracts/docs/contracts-group-treasury-storage.md new file mode 100644 index 0000000..9942a37 --- /dev/null +++ b/contracts/docs/contracts-group-treasury-storage.md @@ -0,0 +1,168 @@ +# `group_treasury` Storage Layout + +Source: [`contracts/contracts/group_treasury/src/storage.rs`](../contracts/group_treasury/src/storage.rs), +[`contracts/contracts/group_treasury/src/lib.rs`](../contracts/group_treasury/src/lib.rs), +[`contracts/contracts/group_treasury/src/token_interface.rs`](../contracts/group_treasury/src/token_interface.rs). + +For the authorization semantics built on top of this storage (membership, threshold, voting), +see [`concepts-treasury-multisig-model.md`](concepts-treasury-multisig-model.md). For the +related `proposals` contract that calls into this one, see +[`api-proposals.md`](api-proposals.md). + +## Storage tier: everything is instance storage + +Every read/write in `group_treasury` goes through `env.storage().instance()`. The contract +never calls `.persistent()`, `.temporary()`, `.extend_ttl()`, or `bump()` anywhere in +`lib.rs` or `storage.rs`. (`persistent()` calls do appear in the test suite, but only inside +`test.rs`'s in-file mock token contract — an unrelated test fixture, not part of +`group_treasury` itself.) + +**Practical effect:** all state — admin, members, balances, proposals, votes — shares the +single instance-storage TTL that Soroban maintains for the contract's instance entry as a +whole. There is no per-key TTL management in this contract: nothing here explicitly bumps or +extends the TTL of any individual entry, and there is no TTL constant defined in the source. +Whether the instance entry itself is kept alive (via the account/operator maintaining rent, a +platform-level restore, or another contract's invocation) is a Soroban platform concern +external to this contract's code — `group_treasury` does not implement any TTL or restoration +logic of its own. If the instance entry expires, every key documented below expires with it; +this contract has no expired-entry recreation logic beyond normal `initialize()` (which itself +panics if `DataKey::Admin` is already set, so it cannot be used to "revive" prior state). + +## `DataKey` — storage key inventory + +```rust +pub enum DataKey { + Admin, + Balances, + Members, + Threshold, + ProposalCount, + Proposal(u32), + Vote(u32, Address), +} +``` + +| Key | Value type | Tier | Meaning | Owner/domain | Lifecycle | +|---|---|---|---|---|---| +| `DataKey::Admin` | `Address` | instance | The address with admin rights (`add_member`, `remove_member`, `withdraw`). | Access control | Set once in `initialize`; never updated after. Read by `require_admin` on every admin-gated call. | +| `DataKey::Balances` | `Map` | instance | Per-token balances held by the treasury, keyed by token contract address. | Funds accounting | Initialized empty in `initialize`. Updated by `deposit` (increment) and `withdraw` (decrement). Read by `balance`, `propose_withdraw` (funds check). | +| `DataKey::Members` | `Vec
` | instance | The current membership list. | Membership | Initialized empty in `initialize`. Mutated by `add_member` (append, rejects duplicates) and `remove_member` (rebuilds the vector without the removed address). Read by `is_member`, `get_members`, and every voting/proposal function that checks membership. | +| `DataKey::Threshold` | `u32` | instance | Number of approvals required for a proposal to pass. | Multisig config | Set once in `initialize` (must be `>= 1`, enforced by panic). **No function updates it after initialization** — there is no `set_threshold` in `lib.rs`. Read by `get_threshold`, `approve_withdraw`, `reject_withdraw` (blocking-minority calculation). | +| `DataKey::ProposalCount` | `u32` | instance | Total number of proposals ever created; doubles as the next proposal id. | Proposal accounting | Initialized to `0` in `initialize`. Incremented by `propose_withdraw` before assigning the new proposal's id (so ids are `0`-based and monotonically increasing). Read by `list_proposals` and `get_pending_proposals` to bound their scan range. | +| `DataKey::Proposal(u32)` | `WithdrawProposal` | instance | The full record for withdraw proposal `id`. | Proposal state | Created by `propose_withdraw`. Updated in place by `approve_withdraw` and `reject_withdraw` (approval/rejection counters and `status`). Read by `get_proposal`, `list_proposals`, `get_pending_proposals`, and the `require_votable` helper used by both voting functions. | +| `DataKey::Vote(u32, Address)` | `bool` | instance | Whether `Address` voted to approve (`true`) or reject (`false`) proposal `id`. | Vote record | Written once by `approve_withdraw` or `reject_withdraw` the first time an address votes on a given proposal. `require_votable` checks `has(&DataKey::Vote(id, voter))` to reject a second vote from the same address — there is no function that clears or changes an existing vote record. | + +`token_interface.rs` defines no storage of its own — it declares the minimal SEP-41 +`TokenInterface` trait (`transfer`, `balance`) and the generated `TokenClient` used by +`deposit` and `withdraw` to call the external token contract. It is a cross-contract call +helper, not a storage module. + +## Events (not storage, but part of the same source files) + +`storage.rs` also defines the event payload types published via `env.events().publish(...)`: +`DepositEvent`, `WithdrawEvent`, `MemberAddedEvent`, `MemberRemovedEvent`, +`WithdrawVoteCastEvent`, `ProposalApprovedEvent`, `ProposalRejectedEvent`, +`ProposalCreatedEvent`. These are emitted, not stored — see +[`contracts-events.md`](contracts-events.md) for the general event catalog. + +## `WithdrawProposal` + +```rust +pub struct WithdrawProposal { + pub id: u32, + pub proposer: Address, + pub to: Address, + pub token: Address, + pub amount: i128, + pub approvals: u32, + pub rejections: u32, + pub status: ProposalStatus, + pub expires_at: u64, +} +``` + +| Field | Type | Meaning | Set by | How it changes | Participates in auth/state checks? | +|---|---|---|---|---|---| +| `id` | `u32` | Proposal identifier, equal to the pre-increment value of `ProposalCount`. | `propose_withdraw` | Immutable after creation. | Used as the `DataKey::Proposal`/`DataKey::Vote` key. | +| `proposer` | `Address` | Member who created the proposal. | `propose_withdraw`, from the authenticated `proposer` argument. | Immutable. | Auto-recorded as the first approval vote (see below); not otherwise checked against later calls. | +| `to` | `Address` | Withdrawal recipient. | `propose_withdraw`. | Immutable. | No — informational only within this contract (no execute path reads it yet; see Security section). | +| `token` | `Address` | Token contract address to withdraw from. | `propose_withdraw`. | Immutable. | Used against `DataKey::Balances` when the proposal is created (funds check), not re-checked later. | +| `amount` | `i128` | Amount requested. | `propose_withdraw`, must be `> 0`. | Immutable. | Checked against the treasury's current balance for `token` at proposal-creation time only. | +| `approvals` | `u32` | Running count of approval votes, starting at `1`. | `propose_withdraw` (initializes to `1`), incremented by `approve_withdraw`. | Monotonically increasing. | Compared against `Threshold` after each `approve_withdraw` call to decide the `Passed` transition. | +| `rejections` | `u32` | Running count of rejection votes, starting at `0`. | `propose_withdraw` (initializes to `0`), incremented by `reject_withdraw`. | Monotonically increasing. | Compared against the blocking-minority value after each `reject_withdraw` call to decide the `Rejected` transition. | +| `status` | `ProposalStatus` | Current lifecycle state. | `propose_withdraw` sets `Active`. | Flipped to `Passed` or `Rejected` by the voting functions once their respective thresholds are met; never flipped by any other function. | Gates voting (`require_votable` requires `Active`) and read by `get_pending_proposals` (filters on `Active`). | +| `expires_at` | `u64` | Ledger timestamp after which the proposal can no longer be voted on. | `propose_withdraw`, computed as `env.ledger().timestamp() + (ttl_ledgers as u64 * 5)` — an approximate 5-seconds-per-ledger conversion from the caller-supplied `ttl_ledgers`. | Immutable after creation. | Checked in `require_votable`: a vote panics with `"proposal expired"` once `env.ledger().timestamp() >= expires_at`, even if `status` is still `Active`. | + +**Proposer auto-approval:** `propose_withdraw` initializes `approvals: 1` and writes +`DataKey::Vote(id, proposer)` to `true` at creation time — the proposer's own approval counts +immediately, before any other member votes. + +### `ProposalStatus` + +```rust +pub enum ProposalStatus { + Active, + Passed, + Rejected, + Executed, + Expired, +} +``` + +| Status | Set by | Meaning | +|---|---|---| +| `Active` | `propose_withdraw` | Open for voting. | +| `Passed` | `approve_withdraw`, once `approvals >= Threshold` | Approval threshold reached. | +| `Rejected` | `reject_withdraw`, once `rejections >= blocking_minority` | Enough rejections that `Threshold` approvals can no longer be reached (see the multisig model doc for the exact formula). | +| `Executed` | **No function in `lib.rs` sets this today.** | Defined in the enum for a future execution step; not reachable through any current `group_treasury` entrypoint. | +| `Expired` | **No function in `lib.rs` sets this today.** | Defined in the enum; expiry is currently enforced only as a *guard* inside `require_votable` (a vote after `expires_at` panics with `"proposal expired"`), not as a stored status transition. A proposal whose deadline has passed still reads back as `Active` from `get_proposal`/`list_proposals` until/unless a future finalizer writes `Expired`. | + +`src/test.rs` documents this gap directly: its `multisig_flow` test module contains passing +tests for the `Active → Passed` and `Active → Rejected` transitions and for the expiry +*guard*, plus explicitly `#[ignore]`d stub tests — `multisig_execute_on_approved`, +`multisig_execute_on_pending_panics`, and `multisig_finalize_expired` — each annotated +`"blocked: ... not yet implemented (no issue identified)"`. Do not treat `Executed` or a +stored `Expired` transition as implemented behavior. + +### Proposal state diagram (as implemented) + +```mermaid +stateDiagram-v2 + [*] --> Active: propose_withdraw\n(proposer auto-approves) + Active --> Passed: approve_withdraw\napprovals >= Threshold + Active --> Rejected: reject_withdraw\nrejections >= blocking_minority + Active --> Active: approve_withdraw / reject_withdraw\n(threshold not yet reached) + note right of Active + Voting on an Active proposal panics + ("proposal expired") once + ledger timestamp >= expires_at, + but status stays Active in storage. + end note + Passed --> [*]: no execute entrypoint exists + Rejected --> [*]: terminal, no further transitions +``` + +No transition reaches `Executed` or a stored `Expired` state in the current implementation. +Funds only move via the separate, admin-gated `withdraw` function (see below) — approving a +`group_treasury`-internal proposal to `Passed` does not itself transfer funds. + +| Transition | Triggering function | Required authorization | Approval requirement | Resulting status | Funds move? | +|---|---|---|---|---|---| +| (none) → `Active` | `propose_withdraw` | `proposer.require_auth()`; proposer must be a current member | n/a | `Active` | No | +| `Active` → `Active` | `approve_withdraw` / `reject_withdraw` | Voter `require_auth()`; must be a current member; must not have voted before; proposal must not be expired | Count below the relevant threshold | `Active` (unchanged) | No | +| `Active` → `Passed` | `approve_withdraw` | Same as above | `approvals >= Threshold` | `Passed` | No — `group_treasury` has no function that reads a `Passed` proposal and executes it | +| `Active` → `Rejected` | `reject_withdraw` | Same as above | `rejections >= member_count - Threshold + 1` (saturating) | `Rejected` | No | + +## Relationship to `withdraw` and the `proposals` contract + +`group_treasury` also exposes a separate, simpler `withdraw(to, token, amount)` function that +is **admin-gated only** (`require_admin`) and moves funds immediately — it does not consult +`DataKey::Proposal` or `DataKey::Vote` at all. This is the function the external `proposals` +contract calls (via `TokenInterface`'s sibling trait in `proposals/src/treasury_interface.rs`, +using `is_member`, `balance`, and `withdraw`) once *its own* independent proposal/vote/finalize +cycle passes. `group_treasury`'s own `propose_withdraw` / `approve_withdraw` / `reject_withdraw` +voting system is a separate, currently execution-less code path within the same contract — the +two are not wired together. See +[`concepts-treasury-multisig-model.md`](concepts-treasury-multisig-model.md) for the full +authorization picture and [`api-proposals.md`](api-proposals.md) for the `proposals` contract's +cross-contract call flow.