From b3fc0f160966d974e0e62de54daa71477e322871 Mon Sep 17 00:00:00 2001 From: nissinanlung <269563815+nissinanlung@users.noreply.github.com> Date: Mon, 19 Oct 2026 18:50:43 +0000 Subject: [PATCH] feat(contracts): add contract upgradeability plan + versioning tests (#1809) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Document the upgradeability scheme (versioning, in-place migration flow, rollback, blue-green cutover) and add real-contract tests for the subscription upgrade hooks. Changes: - Add docs/CONTRACT_UPGRADE_PLAN.md: proxy+storage+implementation architecture, STORAGE_VERSION/ProxyVersion scheme, monotonic upgrade rules, step-by-step in-place upgrade procedure, rollback & emergency procedures, blue-green cutover for breaking changes, version history, and test coverage mapping - Add contracts/subscription/tests/upgrade_versioning.rs: 7 tests against the real SubTrackrSubscription + UpgradeableProxy + SubTrackrStorage trio — get_version, validate_upgrade boundary gates (0/1/2/3), migrate idempotency, unsupported-path rejection, v1→v2 UserPlanIndex rebuild (active only), and proxy-version tracking. Verified passing on stable. Acceptance criteria: - [x] Versioning — STORAGE_VERSION constant + ProxyVersion proxy storage; monotonic upgrades enforced; per-version migrate arms - [x] Migration steps — in-place schedule→timelock→validate→migrate→bump flow plus blue-green cutover for breaking changes; rollback path - [x] Tested — upgrade_flow.rs (proxy lifecycle) + upgrade_versioning.rs (real-contract versioning, compatibility gates, v1→v2 migration) --- .../subscription/tests/upgrade_versioning.rs | 199 ++++++++++++++++++ docs/CONTRACT_UPGRADE_PLAN.md | 178 ++++++++++++++++ 2 files changed, 377 insertions(+) create mode 100644 contracts/subscription/tests/upgrade_versioning.rs create mode 100644 docs/CONTRACT_UPGRADE_PLAN.md diff --git a/contracts/subscription/tests/upgrade_versioning.rs b/contracts/subscription/tests/upgrade_versioning.rs new file mode 100644 index 00000000..e155043c --- /dev/null +++ b/contracts/subscription/tests/upgrade_versioning.rs @@ -0,0 +1,199 @@ +// Upgradeability & storage-versioning tests for the subscription contract +// (issue #1809 — Add contract upgradeability plan). +// +// These exercise the REAL subscription contract's upgrade hooks +// (`get_version`, `validate_upgrade`, `migrate`) together with the real +// UpgradeableProxy + SubTrackrStorage contracts — not mocks — proving that +// the versioning scheme, compatibility gates, and v1→v2 migration actually +// work end-to-end. +// +// cargo test --manifest-path contracts/subscription/Cargo.toml \ +// --test upgrade_versioning + +#![allow(deprecated)] // env.register_contract(..) deprecation + +use soroban_sdk::{ + testutils::{Address as _, Ledger}, + token, Address, Env, IntoVal, String, TryFromVal, Val, +}; +use subtrackr_proxy::{UpgradeableProxy, UpgradeableProxyClient}; +use subtrackr_storage::{SubTrackrStorage, SubTrackrStorageClient}; +use subtrackr_subscription::{SubTrackrSubscription, SubTrackrSubscriptionClient}; +use subtrackr_types::{Interval, StorageKey}; + +const START_TS: u64 = 1_700_000_000; + +struct Setup { + env: Env, + proxy_id: Address, + storage_id: Address, + impl_id: Address, + merchant: Address, + token_id: Address, +} + +impl Setup { + fn proxy(&self) -> UpgradeableProxyClient<'_> { + UpgradeableProxyClient::new(&self.env, &self.proxy_id) + } + fn impl_client(&self) -> SubTrackrSubscriptionClient<'_> { + SubTrackrSubscriptionClient::new(&self.env, &self.impl_id) + } + fn storage(&self) -> SubTrackrStorageClient<'_> { + SubTrackrStorageClient::new(&self.env, &self.storage_id) + } +} + +fn setup() -> Setup { + let env = Env::default(); + env.mock_all_auths_allowing_non_root_auth(); + env.ledger().set_timestamp(START_TS); + + let admin = Address::generate(&env); + let merchant = Address::generate(&env); + let token_admin = Address::generate(&env); + + let storage_id = env.register_contract(None, SubTrackrStorage); + let impl_id = env.register_contract(None, SubTrackrSubscription); + let proxy_id = env.register_contract(None, UpgradeableProxy); + let proxy = UpgradeableProxyClient::new(&env, &proxy_id); + // Zero-second upgrade/rollback delays so timelocks elapse immediately. + proxy.initialize(&admin, &storage_id, &impl_id, &0u64, &0u64); + + let token = env.register_stellar_asset_contract_v2(token_admin); + let token_id = token.address(); + token::StellarAssetClient::new(&env, &token_id).mint(&merchant, &1_000_000_000); + + Setup { env, proxy_id, storage_id, impl_id, merchant, token_id } +} + +/// Read the UserPlanIndex(subscriber, plan_id) entry from storage, if any. +fn user_plan_index(setup: &Setup, subscriber: &Address, plan_id: u64) -> Option { + let key: Val = StorageKey::UserPlanIndex(subscriber.clone(), plan_id).into_val(&setup.env); + let val_opt: Option = setup.storage().persistent_get(&key); + val_opt.map(|v| u64::try_from_val(&setup.env, &v).unwrap()) +} + +/// Remove the UserPlanIndex entry, simulating a pre-v2 (v1) storage state +/// where the index did not exist. +fn remove_user_plan_index(setup: &Setup, subscriber: &Address, plan_id: u64) { + let key: Val = StorageKey::UserPlanIndex(subscriber.clone(), plan_id).into_val(&setup.env); + setup.storage().persistent_remove(&key); +} + +// ─── Tests ────────────────────────────────────────────────────────────────── + +#[test] +fn get_version_reports_current_storage_version() { + let s = setup(); + // STORAGE_VERSION is currently 2 (see contracts/subscription/src/lib.rs). + let version = s.impl_client().get_version(&s.proxy_id, &s.storage_id); + assert_eq!(version, 2, "get_version must return the current STORAGE_VERSION"); +} + +#[test] +fn validate_upgrade_accepts_current_and_prior_versions() { + let s = setup(); + // Upgrading from v1 or from the current v2 must be accepted. + s.impl_client().validate_upgrade(&s.proxy_id, &s.storage_id, &1u32); + s.impl_client().validate_upgrade(&s.proxy_id, &s.storage_id, &2u32); +} + +#[test] +fn validate_upgrade_rejects_zero_and_future_versions() { + let s = setup(); + let client = s.impl_client(); + // from_version == 0 is invalid. + let zero = std::panic::catch_unwind(std::panic::AssertUnwindSafe(|| { + client.validate_upgrade(&s.proxy_id, &s.storage_id, &0u32); + })); + assert!(zero.is_err(), "validate_upgrade must reject from_version 0"); + + // from_version > STORAGE_VERSION is a future/unsupported version. + let future = std::panic::catch_unwind(std::panic::AssertUnwindSafe(|| { + client.validate_upgrade(&s.proxy_id, &s.storage_id, &3u32); + })); + assert!(future.is_err(), "validate_upgrade must reject future versions"); +} + +#[test] +fn migrate_is_a_noop_when_already_at_current_version() { + let s = setup(); + // Migrating from the current version must be a safe no-op. + s.impl_client().migrate(&s.proxy_id, &s.storage_id, &2u32); + // Version is still consistent. + assert_eq!(s.impl_client().get_version(&s.proxy_id, &s.storage_id), 2); +} + +#[test] +fn migrate_rejects_unsupported_future_path() { + let s = setup(); + let client = s.impl_client(); + let res = std::panic::catch_unwind(std::panic::AssertUnwindSafe(|| { + client.migrate(&s.proxy_id, &s.storage_id, &3u32); + })); + assert!(res.is_err(), "migrate must reject from_version > STORAGE_VERSION"); +} + +#[test] +fn migrate_v1_to_v2_rebuilds_user_plan_index_for_active_only() { + let s = setup(); + let proxy = s.proxy(); + + // One plan, two subscribers. + let plan_id = proxy.create_plan( + &s.merchant, + &String::from_str(&s.env, "pro"), + &500, + &s.token_id, + &Interval::Monthly, + ); + + let user_a = Address::generate(&s.env); + let user_b = Address::generate(&s.env); + + let sub_a = proxy.subscribe(&user_a, &plan_id); + let sub_b = proxy.subscribe(&user_b, &plan_id); + + // v2 keeps the index for active subscriptions. + assert_eq!(user_plan_index(&s, &user_a, plan_id), Some(sub_a)); + assert_eq!(user_plan_index(&s, &user_b, plan_id), Some(sub_b)); + + // Cancel user_b — cancellation removes its index entry. + proxy.cancel_subscription(&user_b, &sub_b); + assert_eq!(user_plan_index(&s, &user_b, plan_id), None); + + // Simulate a v1 snapshot: the UserPlanIndex did not exist in v1, so + // drop the remaining active entry before running the migration. + remove_user_plan_index(&s, &user_a, plan_id); + assert_eq!(user_plan_index(&s, &user_a, plan_id), None); + + // Run the v1→v2 migration directly against the real contract. + s.impl_client().migrate(&s.proxy_id, &s.storage_id, &1u32); + + // The active subscription's index is rebuilt... + assert_eq!( + user_plan_index(&s, &user_a, plan_id), + Some(sub_a), + "migrate must rebuild UserPlanIndex for active subscriptions", + ); + // ...and the cancelled subscription is intentionally NOT re-indexed. + assert_eq!( + user_plan_index(&s, &user_b, plan_id), + None, + "migrate must not index cancelled subscriptions", + ); +} + +#[test] +fn proxy_version_tracks_implementation_version_after_initialize() { + let s = setup(); + // During initialize the proxy adopts the implementation's STORAGE_VERSION + // so that subsequent upgrades can gate on a correct baseline. + assert_eq!( + s.proxy().get_version(), + s.impl_client().get_version(&s.proxy_id, &s.storage_id), + "proxy version must track the implementation's STORAGE_VERSION", + ); + assert_eq!(s.proxy().get_version(), 2u32); +} diff --git a/docs/CONTRACT_UPGRADE_PLAN.md b/docs/CONTRACT_UPGRADE_PLAN.md new file mode 100644 index 00000000..e427bac8 --- /dev/null +++ b/docs/CONTRACT_UPGRADE_PLAN.md @@ -0,0 +1,178 @@ +# Contract Upgradeability Plan + +> Issue #1809 — Storage/upgrade risks are unmanaged. This document is the +> authoritative upgradeability plan for the SubTrackr Soroban contracts: the +> versioning scheme, the step-by-step migration procedure, rollback/emergency +> procedures, and the tests that guard them. + +## 1. Architecture + +SubTrackr uses the **proxy + storage + implementation** pattern: + +| Contract | Crate | Role | +|----------|-------|------| +| `UpgradeableProxy` | `contracts/proxy` | Single permanent on-chain address clients call. Holds the active implementation pointer, the storage contract pointer, the proxy version, the upgrade timelock, and the upgrade history. | +| `SubTrackrStorage` | `contracts/storage` | Owns all durable state (instance + persistent + temporary). Only the *authorised implementation* may write; reads are public. | +| `SubTrackrSubscription` | `contracts/subscription` | The implementation. Business logic + the `get_version` / `validate_upgrade` / `migrate` upgrade hooks. | + +Because state lives in `SubTrackrStorage` (not the implementation), the +implementation can be replaced without losing data. The proxy enforces a +timelock and runs compatibility checks + migrations before switching the +pointer. + +## 2. Versioning scheme + +- Every implementation declares a `STORAGE_VERSION: u32` constant + (`contracts/subscription/src/lib.rs`, currently `2`). +- The proxy stores its own `ProxyVersion` (in proxy instance storage), which + tracks the last-known implementation version. During `initialize` the proxy + adopts the implementation's `STORAGE_VERSION` as its baseline. +- A version bump is **required** whenever the storage layout changes: + - New `StorageKey` variants are introduced, **or** + - An existing key's encoding/semantics change, **or** + - A derived index (e.g. `UserPlanIndex`) is added that must be back-filled. +- Versions are **monotonic**: `upgrade_to` rejects a target whose + `get_version` is below the current proxy version + (`assert!(target_version >= current_version, "Target version behind current")`). + +### Storage version history + +| Version | What changed | Migration (`migrate(from_version)`) | +|---------|--------------|--------------------------------------| +| 1 | Initial layout (plans, subscriptions, counts) | — | +| 2 | Added `UserPlanIndex(Address, u64)` derived index | `from_version == 1`: iterate non-cancelled subscriptions and rebuild `UserPlanIndex` | +| 3+ | (reserved) | Add a new `if from_version == N { … }` arm; never edit a past arm | + +### Rules for authoring a new version + +1. Bump `STORAGE_VERSION` by **1** (never skip). +2. Add any new `StorageKey` variants **at the end** of the enum — never + reorder or reuse an existing discriminant. +3. Add a `from_version == N` arm to `migrate` that back-fills derived state. + Never mutate an existing arm (past migrations must stay reproducible). +4. Keep `validate_upgrade` accepting every `from_version` in + `1..=STORAGE_VERSION`; reject `0` and anything `> STORAGE_VERSION`. +5. If a change is **breaking** (renamed/removed key, changed encoding), do + **not** bump in place — use the blue-green cutover in §5 instead. + +## 3. Upgrade procedure (in-place, via the proxy) + +A normal upgrade goes through the proxy's timelocked `schedule → execute` +flow. All steps require the stored admin's auth. + +``` + ┌─────────────┐ + │ authorise │ admin calls authorize_upgrade(new_impl) + └──────┬──────┘ + ▼ + ┌─────────────┐ + │ schedule │ proxy.schedule_upgrade(impl, now + delay) + │ (timelock) │ recorded in ProxyScheduledUpgrade + history + └──────┬──────┘ + │ wait ≥ upgrade_delay_secs + ▼ + ┌─────────────┐ + │ upgrade_to │ admin calls upgrade_to(impl) + └──────┬──────┘ + ▼ + ┌────────────────────────────────────────────────┐ + │ 1. storage.set_implementation(new_impl) │ + │ 2. get_version(new_impl) → target_version │ + │ 3. validate_upgrade(new_impl, current_version) │ ← compatibility gate + │ 4. if target_version > current: │ + │ migrate(new_impl, current_version) │ ← back-fill + │ proxy.set_version(target_version) │ + │ 5. clear scheduled upgrade; append history │ + └────────────────────────────────────────────────┘ +``` + +### Preconditions + +- The new implementation is deployed and exposes `get_version`, + `validate_upgrade`, and `migrate` with the proxy/storage signature. +- `get_version(new_impl) >= proxy.get_version()`. +- No upgrade is already scheduled (cancel it first if needed). + +### Cancellation + +`cancel_scheduled_upgrade()` clears a pending upgrade any time before +`upgrade_to` executes. The history records the cancellation. + +## 4. Rollback & emergency procedures + +- `rollback()` schedules a return to the **immediately-previous** + implementation (tracked on a previous-implementation stack), subject to the + rollback timelock (`rollback_delay_secs`). +- Rollback does **not** re-run `migrate` and does **not** revert storage + writes. It is safe when the previous implementation can read the current + storage version (i.e. the upgrade did not change the layout, or the previous + implementation was forward-compatible). For layout-breaking changes use the + blue-green cutover in §5. +- After a rollback, the previous-implementation stack is swapped (not popped) + so an operator can "roll forward" again if the rollback itself misbehaves. +- Every schedule / execute / cancel / rollback is appended to + `ProxyUpgradeHistory` (queryable via `get_upgrade_history_count` / + `get_upgrade_history_entry`) for auditing. + +## 5. Blue-green cutover (layout-breaking changes) + +For changes that cannot be migrated in place (renamed/removed keys, changed +encoding), use the operational cutover in `contracts/migrations/`: + +1. **Snapshot** — export plans + subscriptions from the source contract + (`contracts/migrations/lib.sh`). +2. **Deploy** — deploy + initialize a replacement contract + storage pair. +3. **Rehydrate** — load the snapshot into the replacement. +4. **Validate** — run read-side checks against the replacement. +5. **Cutover** — switch the application's active `CONTRACT_ID` pointer only + after validation passes. +6. **Rollback** — restore the previous `CONTRACT_ID` if the replacement + misbehaves. + +See `contracts/migrations/README.md` and `001_blue_green_cutover.sh`. + +## 6. Test coverage + +The upgrade path is guarded by two test files: + +### `contracts/proxy/tests/upgrade_flow.rs` + +Proxy-level lifecycle with mock V1/V2 implementations: + +- `upgrade_flow_preserves_state_and_enforces_timelocks` — state survives an + upgrade; the timelock is enforced; the version advances. +- `upgrade_validation_failure_does_not_change_implementation` — a failing + `validate_upgrade` aborts the switch and leaves the pointer/version intact. + +### `contracts/subscription/tests/upgrade_versioning.rs` (new) + +Real-contract versioning hooks against the actual `SubTrackrSubscription` + +`UpgradeableProxy` + `SubTrackrStorage` trio (no mocks): + +- `get_version_reports_current_storage_version` +- `validate_upgrade_accepts_current_and_prior_versions` (v1, v2) +- `validate_upgrade_rejects_zero_and_future_versions` (0, v3) +- `migrate_is_a_noop_when_already_at_current_version` +- `migrate_rejects_unsupported_future_path` +- `migrate_v1_to_v2_rebuilds_user_plan_index_for_active_only` — drops the + `UserPlanIndex` to simulate a v1 snapshot, runs `migrate(1)`, and asserts + the index is rebuilt for active subscriptions **only** (cancelled ones are + skipped). +- `proxy_version_tracks_implementation_version_after_initialize` + +Run locally: + +```bash +cargo test --manifest-path contracts/subscription/Cargo.toml --test upgrade_versioning +cargo test --manifest-path contracts/proxy/Cargo.toml --test upgrade_flow +``` + +## 7. Acceptance criteria + +- [x] **Versioning** — `STORAGE_VERSION` constant + `ProxyVersion` proxy + storage; monotonic upgrades enforced; per-version migration arms. +- [x] **Migration steps** — §3 in-place flow (schedule → timelock → validate + → migrate → version bump) and §5 blue-green cutover for breaking + changes; rollback in §4. +- [x] **Tested** — `upgrade_flow.rs` (proxy lifecycle) + `upgrade_versioning.rs` + (real-contract versioning, compatibility gates, and v1→v2 migration).