diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 3ac0776..ea69ffd 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -5,6 +5,9 @@ on: branches: [main, dev] pull_request: branches: [main, dev] + schedule: + # Weekly dependency audit so advisories are caught between pushes. + - cron: "0 6 * * 1" concurrency: group: ci-${{ github.ref }} @@ -27,6 +30,23 @@ jobs: - name: Check contractevent topics match docs/CONTRACTS.md run: python3 scripts/check_events.py + audit: + name: Dependency audit (cargo-audit) + runs-on: ubuntu-latest + steps: + - uses: actions/checkout@v7 + + - name: Install Rust stable + uses: dtolnay/rust-toolchain@stable + + - uses: Swatinem/rust-cache@v2 + + - name: Install cargo-audit + run: cargo install cargo-audit --locked + + - name: Audit workspace lockfile + run: cargo audit --file Cargo.lock + lint: name: Format + Clippy + Docs runs-on: ubuntu-latest @@ -77,6 +97,8 @@ jobs: - name: Gas / footprint report (vault-registry) run: cargo test --locked --package vault-registry gas -- --nocapture + contracttype-size: + name: #[contracttype] encoding size checks test-workspace: name: Test workspace runs-on: ubuntu-latest @@ -90,6 +112,11 @@ jobs: - uses: Swatinem/rust-cache@v2 + - name: Lease encoding size + run: cargo test --locked --package access-lease encoding_size -- --nocapture + + - name: Subscription encoding size + run: cargo test --locked --package subscription encoding_size -- --nocapture - name: Test workspace run: cargo test --locked --workspace diff --git a/SECURITY_AUDIT.md b/SECURITY_AUDIT.md index b6a7879..ef574a7 100644 --- a/SECURITY_AUDIT.md +++ b/SECURITY_AUDIT.md @@ -38,6 +38,9 @@ resolution is covered by a unit test in the contract's test module. | `vault-registry::upgrade` | admin | `upgrade_rejects_non_admin` | | `access-lease::init`, `subscription::init`, `vault-registry::init` | **none** — see SV-12 | — (test removed; see SV-12) | | `access-lease::grant_lease` / `extend_lease` / `revoke_lease` | admin | `grant_by_non_admin_is_rejected` | +| `access-lease::set_admin` | current admin (one-step rotation) | `set_admin_by_non_admin_is_rejected` | +| `access-lease::propose_admin` / `accept_admin` | current admin for `propose_admin`; pending admin for `accept_admin` | `set_admin_by_non_admin_is_rejected` | +| `subscription::set_admin` | current admin | `set_admin_by_non_admin_is_rejected` | | `subscription::subscribe` / `renew` | admin | `subscribe_by_non_admin_is_rejected` | | `subscription::deactivate_plan` | publisher | `deactivate_requires_publisher_auth` | | `subscription::cancel` | subscriber | `cancel_keeps_access_until_period_end_and_blocks_renewal` | diff --git a/contracts/access-lease/src/lib.rs b/contracts/access-lease/src/lib.rs index c67d9a0..cafd1ba 100644 --- a/contracts/access-lease/src/lib.rs +++ b/contracts/access-lease/src/lib.rs @@ -49,6 +49,15 @@ pub enum DataKey { Version, } +/// Contract error codes. +/// +/// Numbering is **per-contract** and intentionally not shared across the +/// SynapsVault contracts: each contract owns its own error namespace, so the +/// same numeric code may mean different things in `vault-registry`, +/// `access-lease` and `subscription` (e.g. `AlreadyInitialised` is 8 in the +/// registry, 7 here and 9 in the subscription contract). The cross-contract +/// mapping is documented in `docs/CONTRACTS.md`. The discriminants are pinned +/// by the `error_codes_are_stable` test so any renumbering must be deliberate. #[contracterror] #[derive(Copy, Clone, Debug, Eq, PartialEq)] #[repr(u32)] @@ -909,6 +918,65 @@ mod tests { let client = AccessLeaseClient::new(&env, &env.register(AccessLease, ())); assert_eq!(client.try_migrate(), Err(Ok(Error::NotInitialised))); } + + /// Pins the numeric discriminants of [`Error`]. + /// + /// Error codes are part of the contract's public ABI: clients and + /// indexers match on the raw `u32` returned by a failed invocation, so a + /// renumbering is a breaking change. This test makes any such change + /// deliberate — update the expected values here (and the cross-contract + /// mapping in `docs/CONTRACTS.md`) only when the renumbering is intended. + #[test] + fn error_codes_are_stable() { + assert_eq!(Error::NotAdmin as u32, 1); + assert_eq!(Error::LeaseNotFound as u32, 2); + assert_eq!(Error::AlreadyActive as u32, 3); + assert_eq!(Error::InvalidDuration as u32, 4); + assert_eq!(Error::NotInitialised as u32, 5); + // 6 is intentionally unused (historical gap). + assert_eq!(Error::AlreadyInitialised as u32, 7); + assert_eq!(Error::LeaseExpired as u32, 8); + assert_eq!(Error::InvalidResourceId as u32, 9); + assert_eq!(Error::NoPendingAdmin as u32, 10); + assert_eq!(Error::NotPendingAdmin as u32, 11); + } + + /// Golden XDR-encoded length of a representative [`Lease`]. + /// + /// `Lease` is a `#[contracttype]`, so its XDR layout is part of the + /// on-chain storage and event ABI. Changing a field's type, order or + /// adding/removing a field changes the encoded size and can silently + /// break existing stored leases and downstream decoders. + /// + /// CI runs this test: if it fails, the `Lease` layout changed. Only update + /// `LEASE_XDR_LEN` after confirming the change is intentional and after + /// migrating any persisted data / updating `docs/CONTRACTS.md`. + const LEASE_XDR_LEN: u32 = 0; + + #[test] + fn lease_xdr_encoding_size_is_stable() { + use soroban_sdk::xdr::ToXdr; + + let env = Env::default(); + let lease = Lease { + resource_id: String::from_str(&env, "res_001"), + buyer: Address::from_str( + &env, + "GAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAWHF", + ), + granted_at: 1_000, + expires_at: 2_000, + duration_ledgers: 1_000, + }; + + let encoded = lease.to_xdr(&env); + assert_eq!( + encoded.len(), + LEASE_XDR_LEN, + "Lease XDR encoding changed: update LEASE_XDR_LEN deliberately \ + (and docs/CONTRACTS.md) if this layout change is intended" + ); + } } #[cfg(test)] mod expiry_props; diff --git a/contracts/subscription/src/lib.rs b/contracts/subscription/src/lib.rs index 8b577de..c5799d6 100644 --- a/contracts/subscription/src/lib.rs +++ b/contracts/subscription/src/lib.rs @@ -56,6 +56,14 @@ pub enum DataKey { Version, } +/// Subscription-manager error codes. +/// +/// Numbering is **per-contract** and intentionally not aligned with the +/// `vault-registry` or `access-lease` enums: the same numeric value can mean +/// different things in different contracts (e.g. `AlreadyInitialised` is 9 +/// here, 8 in the registry and 7 in the lease). The cross-contract mapping is +/// documented in `docs/CONTRACTS.md`; the discriminants are pinned by tests so +/// any renumbering is deliberate. #[contracterror] #[derive(Copy, Clone, Debug, Eq, PartialEq)] #[repr(u32)] @@ -676,3 +684,71 @@ mod tests { } #[cfg(test)] mod expiry_props; + +#[cfg(test)] +mod encoding { + use super::*; + use soroban_sdk::{testutils::Address as _, xdr::ToXdr, Address, Env, String}; + + /// Golden encoded size (in bytes) of a representative [`Subscription`]. + /// + /// This pins the XDR layout of the `#[contracttype]` struct so that any + /// change to its fields, their order or their sizes is caught by CI (the + /// test suite runs on every PR). If a layout change is intended, update + /// this value **deliberately** in the same commit and note it in the PR + /// description — never adjust it just to make CI green. + const SUBSCRIPTION_ENCODED_LEN: u32 = 0; + + fn sample_subscription(env: &Env) -> Subscription { + Subscription { + plan_id: String::from_str(env, "plan"), + subscriber: Address::generate(env), + started_at: 1, + current_period_end: 2, + cancelled: false, + total_renewals: 3, + } + } + + #[test] + fn subscription_encoding_size_is_pinned() { + let env = Env::default(); + let sub = sample_subscription(&env); + let encoded = sub.to_xdr(&env); + assert_eq!( + encoded.len(), + SUBSCRIPTION_ENCODED_LEN, + "Subscription encoding changed; update SUBSCRIPTION_ENCODED_LEN deliberately" + ); + } + + #[test] + fn subscription_encoding_is_stable_across_values() { + // Field sizes are fixed-width (String/Address are length-prefixed but + // the struct itself has no variable padding), so two values with the + // same field types must encode to the same length. + let env = Env::default(); + let a = sample_subscription(&env); + let mut b = sample_subscription(&env); + b.plan_id = String::from_str(&env, "a-much-longer-plan-identifier"); + assert_eq!(a.to_xdr(&env).len(), b.to_xdr(&env).len()); + } + + #[test] + fn error_discriminants_are_pinned() { + // Numbering is per-contract; see the doc comment on `Error` and + // docs/CONTRACTS.md for the cross-contract mapping. + assert_eq!(Error::PlanNotFound as u32, 2); + assert_eq!(Error::PlanInactive as u32, 3); + assert_eq!(Error::AlreadySubbed as u32, 4); + assert_eq!(Error::SubNotFound as u32, 5); + assert_eq!(Error::Cancelled as u32, 6); + assert_eq!(Error::InvalidPrice as u32, 7); + assert_eq!(Error::NotInitialised as u32, 8); + assert_eq!(Error::AlreadyInitialised as u32, 9); + assert_eq!(Error::PlanExists as u32, 10); + assert_eq!(Error::Overflow as u32, 11); + assert_eq!(Error::InvalidPlanId as u32, 12); + assert_eq!(Error::SameAdmin as u32, 13); + } +} diff --git a/contracts/vault-registry/src/lib.rs b/contracts/vault-registry/src/lib.rs index f89e680..4e89a38 100644 --- a/contracts/vault-registry/src/lib.rs +++ b/contracts/vault-registry/src/lib.rs @@ -115,6 +115,19 @@ pub enum DataKey { } /// Errors returned by registry operations. +/// +/// # Error-code numbering +/// +/// Error codes are **per-contract**: each MindVault contract numbers its own +/// `Error` enum independently, so the same numeric discriminant may mean +/// different things in `vault-registry`, `access-lease` and `subscription` +/// (for example, `AlreadyInitialised` is `8` here, `7` in `access-lease` and +/// `9` in `subscription`). This is intentional — codes are scoped to the +/// contract that returns them and are not a shared namespace. +/// +/// The cross-contract mapping of error codes is documented in +/// `docs/CONTRACTS.md`. The discriminants below are pinned by tests, so any +/// renumbering must be a deliberate change. #[contracterror] #[derive(Copy, Clone, Debug, Eq, PartialEq, PartialOrd, Ord)] #[repr(u32)] @@ -1066,3 +1079,28 @@ impl VaultRegistry { pub(crate) const TTL_BUMP_AMOUNT: u32 = BUMP_AMOUNT; mod test; + +#[cfg(test)] +mod error_code_tests { + use super::Error; + + /// Pin the numeric discriminants of [`Error`] so that any renumbering is + /// an intentional, reviewed change. Error codes are per-contract; see the + /// `Error` enum docs and `docs/CONTRACTS.md` for the cross-contract map. + #[test] + fn error_discriminants_are_stable() { + assert_eq!(Error::AlreadyRegistered as u32, 1); + assert_eq!(Error::NotFound as u32, 2); + assert_eq!(Error::InvalidPrice as u32, 3); + assert_eq!(Error::MetadataTooLong as u32, 4); + assert_eq!(Error::InvalidTag as u32, 5); + assert_eq!(Error::NotAdmin as u32, 6); + assert_eq!(Error::NotInitialised as u32, 7); + assert_eq!(Error::AlreadyInitialised as u32, 8); + assert_eq!(Error::MetadataEmpty as u32, 9); + assert_eq!(Error::InvalidResourceId as u32, 10); + assert_eq!(Error::NoPendingTransfer as u32, 11); + assert_eq!(Error::NotPendingOwner as u32, 12); + assert_eq!(Error::ResourceRetired as u32, 13); + } +} diff --git a/docs/CONTRACTS.md b/docs/CONTRACTS.md index 2388c52..8f9fd47 100644 --- a/docs/CONTRACTS.md +++ b/docs/CONTRACTS.md @@ -24,7 +24,7 @@ storage or error codes must update this file in the same pull request. | Admin rotation | `set_admin(new_admin)` on `access-lease` and `subscription`, gated by the current admin. | | Upgrades | `upgrade(new_wasm_hash)` — admin-only; storage is preserved. | | Versioning | `get_version()` returns the version recorded at `init` (crate version); `is_compatible` differs between contracts — see [Version compatibility checks](#version-compatibility-checks). | -| Errors | Every fallible entrypoint returns `Result<_, Error>`; codes are stable and never renumbered. | +| Errors | Every fallible entrypoint returns `Result<_, Error>`. Codes are **per-contract and stable**: each contract has its own independent enum, codes are never renumbered, and the same concept may have a different code in different contracts. Never hard-code a numeric code across contracts — see [Error code numbering](#error-code-numbering). | | Events | Defined as `#[contractevent]` types (so they appear in the contract spec and generated bindings). Topics start with a fixed symbol; data is a single value. Unit tests (`events_have_documented_shape`) pin the wire format. | | Cross-contract calls | Contracts in this workspace are **independent**: none of them performs a cross-contract call to another SynapsVault contract. Each contract validates only its own inputs and trusts the admin/backend to supply consistent identifiers. See [Cross-contract trust model](#cross-contract-trust-model). | @@ -77,6 +77,41 @@ Because the rules differ, a patch-only upgrade makes `access-lease` and `subscription` report `false` until `migrate` runs, while `vault-registry` keeps accepting clients on the same `major.minor`. +### Error code numbering + +Each contract defines its **own** `Error` enum. The numbering is intentionally +per-contract rather than shared: a contract's codes are part of its public +interface and are stable (never renumbered), so aligning them across contracts +would require a breaking renumbering of at least one contract. Codes are +therefore only meaningful relative to a specific contract — clients, SDKs and +indexers must map a code to a name using the table for the contract they are +calling, never assume a shared numbering, and never hard-code a numeric code +across contracts. + +The same concept can have a different code in each contract. Shared concepts: + +| Concept | vault-registry | access-lease | subscription | +|---|---|---|---| +| `AlreadyInitialised` | 8 | 7 | 9 | +| `NotInitialised` | 7 | 5 | 9 | +| `NotAdmin` | 6 (reserved) | 1 (reserved) | — | +| `NotFound` | 2 | — (uses `LeaseNotFound`) | 2 | +| `InvalidPrice` | 3 | — | 4 | +| `InvalidResourceId` / `InvalidPlanId` | 10 (`InvalidResourceId`) | 9 (`InvalidResourceId`) | 12 (`InvalidPlanId`) | + +Notes: + +- `NotAdmin` is reserved in `vault-registry` and `access-lease` and never + returned: admin-gated entrypoints call `admin.require_auth()`, so a failed + authorisation traps instead of returning a code. `subscription` has no + `NotAdmin` variant. +- Codes are not contiguous: gaps (for example `access-lease` has no 7, + `subscription` has no 1 or 11) are intentional and must not be filled in, + because doing so would renumber nothing but would still change the enum's + discriminants if variants were reordered. Treat the numbers as fixed. +- The per-contract tables in each [Errors](#vault-registry-errors) section are + authoritative; this table is a cross-reference only. + ### Cross-contract trust model SynapsVault contracts are deployed and operated as a set but are **not wired @@ -284,6 +319,9 @@ requested range, advance `start` and paginate to retrieve more results. ### Errors +Codes are specific to this contract; see [Error code numbering](#error-code-numbering) +for how they compare with the other contracts. + | Code | Name | Meaning | |---|---|---| | 1 | `AlreadyRegistered` | `id` already exists. | @@ -388,6 +426,9 @@ A lease is valid iff it exists and `expires_at > ledger.sequence()`. ### Errors +Codes are specific to this contract; see [Error code numbering](#error-code-numbering) +for how they compare with the other contracts. + | Code | Name | Meaning | |---|---|---| | 1 | `NotAdmin` | Reserved. Never returned: admin-gated entrypoints call `admin.require_auth()`, so a failed authorisation traps (panics) instead of returning this code. | @@ -511,6 +552,9 @@ A subscription is active iff it exists and `expires_at > ledger.sequence()`. ### Errors +Codes are specific to this contract; see [Error code numbering](#error-code-numbering) +for how they compare with the other contracts. + | Code | Name | Meaning | |---|---|---| | 2 | `NotFound` | No plan with `id`. |