From f30eb454af1afe733d4b032b00efe3274be81f57 Mon Sep 17 00:00:00 2001 From: edwarddavid929-png Date: Thu, 30 Jul 2026 01:15:06 +0000 Subject: [PATCH] Add storage migration framework with versioned schema keys Introduce a schema_version (u32, instance storage) that tracks the contract's on-chain storage shape, plus a SuperAdmin-only migrate() entry point that runs any pending migration_vN steps in order and bumps the version once each transformation completes. Every entry point that funnels through the shared guard helpers (require_admin, require_admin_role, require_not_paused, check_not_paused, require_not_frozen) now panics with "MigrationRequired" while schema_version is behind the version the current Wasm build expects. migrate() is the only entry point exempt, since it is what performs the upgrade; it authenticates via a new schema-guard-free require_admin_role_unguarded helper to avoid a bootstrapping deadlock. pause/unpause/add_allowed_token/ remove_allowed_token inline their own admin check rather than calling the shared helpers, so they get an explicit guard call too. Fresh deployments stamp schema_version at CURRENT_SCHEMA_VERSION during initialize(), since there is no legacy data to migrate. Two concrete migrations demonstrate the mechanics end-to-end: v1->v2 backfills a new per-invoice InvoiceMeta note record for every existing invoice, and v2->v3 renames that record's storage key. Added integration tests simulate a contract rolled back to v1 (as a stand-in for one upgraded from an older Wasm build) and walk it through migrate() to the current schema, checking that guarded entry points are blocked beforehand and that invoice data survives the upgrade. --- contracts/split/src/error.rs | 3 + contracts/split/src/lib.rs | 58 +++++++++++- contracts/split/src/migrations.rs | 147 ++++++++++++++++++++++++++++++ contracts/split/src/test.rs | 141 ++++++++++++++++++++++++++++ 4 files changed, 345 insertions(+), 4 deletions(-) create mode 100644 contracts/split/src/migrations.rs diff --git a/contracts/split/src/error.rs b/contracts/split/src/error.rs index 2ffa89c..05e5309 100644 --- a/contracts/split/src/error.rs +++ b/contracts/split/src/error.rs @@ -80,4 +80,7 @@ pub enum ContractError { CircularDependency = 45, /// Issue #453: Source contract has exceeded its call rate limit within the current window. SourceContractRateLimited = 46, + /// Storage migration framework: `schema_version` is behind the version + /// this Wasm build expects. Call `migrate` before retrying. + MigrationRequired = 47, } diff --git a/contracts/split/src/lib.rs b/contracts/split/src/lib.rs index 99c75b8..aca64b9 100644 --- a/contracts/split/src/lib.rs +++ b/contracts/split/src/lib.rs @@ -59,6 +59,8 @@ mod storage_snapshot; mod storage_keys; +mod migrations; + use error::ContractError; use soroban_sdk::xdr::ToXdr; use soroban_sdk::{ @@ -798,6 +800,7 @@ fn compute_shard_id(env: &Env, payer: &Address) -> u64 { } fn require_admin(env: &Env) -> Address { + migrations::require_schema_current(env); let admin: Address = env .storage() .instance() @@ -2039,6 +2042,7 @@ fn is_paused(env: &Env) -> bool { } fn require_not_paused(env: &Env) { + migrations::require_schema_current(env); assert!(!is_paused(env), "contract is paused"); // Issue #297: also check circuit breaker let cb_active: bool = env @@ -2050,6 +2054,7 @@ fn require_not_paused(env: &Env) { } fn check_not_paused(env: &Env) { + migrations::require_schema_current(env); if is_paused(env) { panic!("ContractPaused"); } @@ -2077,6 +2082,15 @@ fn validate_allowed_token(env: &Env, token: &Address) { fn require_admin_role(env: &Env, admin: &Address, min_role: AdminRole) { + migrations::require_schema_current(env); + require_admin_role_unguarded(env, admin, min_role); +} + +/// Same authorisation check as [`require_admin_role`], without the +/// schema-version guard. `SplitContract::migrate` must be reachable even when +/// a migration is pending, so it calls this directly rather than +/// `require_admin_role`. +fn require_admin_role_unguarded(env: &Env, admin: &Address, min_role: AdminRole) { admin.require_auth(); let admins: Map = env .storage() @@ -2429,6 +2443,7 @@ fn check_creator_milestone(env: &Env, creator: &Address, new_volume: i128) { /// Check if contract is frozen for upgrade. Panics if frozen. fn require_not_frozen(env: &Env) { + migrations::require_schema_current(env); let is_frozen: bool = env .storage() .instance() @@ -2665,6 +2680,9 @@ impl SplitContract { env.storage() .instance() .set(&storage_quota_key(), &DEFAULT_INVOICE_STORAGE_QUOTA); + // Storage migration framework: a freshly-initialized contract has no + // legacy data to migrate, so it starts at the latest schema version. + migrations::init_schema_version(&env); // Issue #477: set the initialisation flag atomically at the end so the // contract is marked fully initialised only after all state is written. env.storage().instance().set(&initialised_key(), &true); @@ -2711,11 +2729,12 @@ impl SplitContract { /// Issue #472: Pause the contract. Requires admin auth. pub fn pause(env: Env, admin: Address) { + migrations::require_schema_current(&env); admin.require_auth(); if let Some(stored_admin) = env.storage().instance().get::<_, Address>(&admin_key()) { assert!(admin == stored_admin, "NotAuthorized"); } else { - require_admin_role(&env, &admin, AdminRole::Operator); + require_admin_role_unguarded(&env, &admin, AdminRole::Operator); } env.storage().instance().set(&paused_key(), &true); events::contract_paused(&env, &admin); @@ -2723,11 +2742,12 @@ impl SplitContract { /// Issue #472: Unpause the contract. Requires admin auth. pub fn unpause(env: Env, admin: Address) { + migrations::require_schema_current(&env); admin.require_auth(); if let Some(stored_admin) = env.storage().instance().get::<_, Address>(&admin_key()) { assert!(admin == stored_admin, "NotAuthorized"); } else { - require_admin_role(&env, &admin, AdminRole::Operator); + require_admin_role_unguarded(&env, &admin, AdminRole::Operator); } env.storage().instance().set(&paused_key(), &false); events::contract_unpaused(&env, &admin); @@ -2900,11 +2920,12 @@ impl SplitContract { /// Issue #473: Add an asset contract address to the allowed tokens list. pub fn add_allowed_token(env: Env, admin: Address, token: Address) { + migrations::require_schema_current(&env); admin.require_auth(); if let Some(stored_admin) = env.storage().instance().get::<_, Address>(&admin_key()) { assert!(admin == stored_admin, "NotAuthorized"); } else { - require_admin_role(&env, &admin, AdminRole::Operator); + require_admin_role_unguarded(&env, &admin, AdminRole::Operator); } let mut allowed: Vec
= env .storage() @@ -2919,11 +2940,12 @@ impl SplitContract { /// Issue #473: Remove an asset contract address from the allowed tokens list. pub fn remove_allowed_token(env: Env, admin: Address, token: Address) { + migrations::require_schema_current(&env); admin.require_auth(); if let Some(stored_admin) = env.storage().instance().get::<_, Address>(&admin_key()) { assert!(admin == stored_admin, "NotAuthorized"); } else { - require_admin_role(&env, &admin, AdminRole::Operator); + require_admin_role_unguarded(&env, &admin, AdminRole::Operator); } let mut allowed: Vec
= env .storage() @@ -4670,6 +4692,34 @@ impl SplitContract { save_invoice(&env, invoice_id, &invoice); } + /// Run all pending storage migrations in order, bringing a contract that + /// was upgraded from an older Wasm build up to the current schema + /// version. Returns the resulting `schema_version`. + /// + /// This is the **only** entry point exempt from the `MigrationRequired` + /// guard — every other entry point panics with `MigrationRequired` while + /// `schema_version` is behind the version this Wasm build expects. + /// No-op (but still auth-checked) once already current, so it is always + /// safe to call after an upgrade. + /// + /// # Errors + /// Panics if `admin` is not a `SuperAdmin`. + pub fn migrate(env: Env, admin: Address) -> u32 { + require_admin_role_unguarded(&env, &admin, AdminRole::SuperAdmin); + migrations::run_pending_migrations(&env) + } + + /// Return the contract's current storage schema version. + pub fn get_schema_version(env: Env) -> u32 { + migrations::schema_version(&env) + } + + /// Return the per-invoice note record introduced at schema v2 (renamed to + /// its current storage key at schema v3), if one has been migrated. + pub fn get_invoice_note(env: Env, invoice_id: u64) -> Option { + migrations::get_invoice_meta(&env, invoice_id) + } + // ----------------------------------------------------------------------- // Invoice creation // ----------------------------------------------------------------------- diff --git a/contracts/split/src/migrations.rs b/contracts/split/src/migrations.rs new file mode 100644 index 0000000..ba85268 --- /dev/null +++ b/contracts/split/src/migrations.rs @@ -0,0 +1,147 @@ +//! Storage migration framework (versioned schema keys). +//! +//! `schema_version` is a `u32` kept in instance storage that records which +//! on-chain storage shape the contract currently has. The shared guard +//! helpers used across the contract (`require_admin`, `require_admin_role`, +//! `require_not_paused`, `check_not_paused`, `require_not_frozen`) all call +//! [`require_schema_current`] first, so a contract left on a stale schema +//! after a Wasm upgrade panics with `MigrationRequired` instead of reading or +//! writing storage in the wrong shape. `SplitContract::migrate` is the only +//! entry point exempt from that guard, since it is what performs the +//! upgrade. +//! +//! Fresh deployments call `initialize()`, which stamps the schema at +//! [`CURRENT_SCHEMA_VERSION`] immediately via [`init_schema_version`] — there +//! is nothing to migrate for storage that never existed in an older shape. +//! Only a contract whose Wasm was upgraded out from under existing data +//! (leaving `schema_version` at a stale value) needs `migrate`. + +use super::*; +use soroban_sdk::contracttype; + +/// Latest schema version this contract build understands. Bump when adding a +/// new `migration_vN` and wire it into [`run_pending_migrations`]. +pub const CURRENT_SCHEMA_VERSION: u32 = 3; + +/// Instance storage: current on-chain schema version. +pub(crate) fn schema_version_key() -> Symbol { + symbol_short!("schm_ver") +} + +/// Current schema version. Contracts deployed before this framework existed +/// never wrote the key, so absence means "version 1" — the original, +/// pre-migration-framework storage layout. +pub fn schema_version(env: &Env) -> u32 { + env.storage() + .instance() + .get(&schema_version_key()) + .unwrap_or(1) +} + +fn set_schema_version(env: &Env, version: u32) { + env.storage() + .instance() + .set(&schema_version_key(), &version); +} + +/// Stamp a freshly-initialized contract at the current schema version. +pub fn init_schema_version(env: &Env) { + set_schema_version(env, CURRENT_SCHEMA_VERSION); +} + +/// Guard applied by every entry point except `migrate` itself: refuse to +/// operate on stale storage so a partially-upgraded contract can't silently +/// read or write data in the wrong shape. +pub fn require_schema_current(env: &Env) { + if schema_version(env) < CURRENT_SCHEMA_VERSION { + panic!("MigrationRequired"); + } +} + +// --------------------------------------------------------------------------- +// v1 -> v2: introduce a per-invoice note record. +// --------------------------------------------------------------------------- +// Every invoice created under schema v1 has no associated `InvoiceMeta`. This +// migration backfills a default record for each existing invoice id so later +// code can assume the record exists once schema_version >= 2. + +/// Per-invoice metadata introduced at schema v2, stored independently of the +/// core `Invoice` record so existing invoice fields are untouched. +#[contracttype] +#[derive(Clone, Debug, PartialEq)] +pub struct InvoiceMeta { + pub note: String, + pub priority: u32, +} + +pub(crate) fn invoice_meta_key_v2(id: u64) -> (Symbol, u64) { + (symbol_short!("inv_meta"), id) +} + +fn migration_v2(env: &Env) { + // The invoice counter holds the highest assigned id (ids are 1-based), + // not a next-id cursor — see `_create_invoice_inner`. + let last_id: u64 = env.storage().persistent().get(&counter_key()).unwrap_or(0); + let default_meta = InvoiceMeta { + note: String::from_str(env, ""), + priority: 0, + }; + for id in 1..=last_id { + if env.storage().persistent().has(&invoice_key(id)) + && !env.storage().persistent().has(&invoice_meta_key_v2(id)) + { + env.storage() + .persistent() + .set(&invoice_meta_key_v2(id), &default_meta); + } + } +} + +// --------------------------------------------------------------------------- +// v2 -> v3: rename the InvoiceMeta storage key. +// --------------------------------------------------------------------------- +// The `inv_meta` key introduced at v2 is renamed to `inv_note` to match the +// public `get_invoice_note` entry point. This demonstrates a pure key rename: +// read under the old key, write under the new one, drop the old entry. + +pub(crate) fn invoice_meta_key_v3(id: u64) -> (Symbol, u64) { + (symbol_short!("inv_note"), id) +} + +fn migration_v3(env: &Env) { + let last_id: u64 = env.storage().persistent().get(&counter_key()).unwrap_or(0); + for id in 1..=last_id { + let old_key = invoice_meta_key_v2(id); + if let Some(meta) = env.storage().persistent().get::<_, InvoiceMeta>(&old_key) { + env.storage() + .persistent() + .set(&invoice_meta_key_v3(id), &meta); + env.storage().persistent().remove(&old_key); + } + } +} + +/// Look up an invoice's note under its current (post-v3) storage key. +pub fn get_invoice_meta(env: &Env, id: u64) -> Option { + env.storage().persistent().get(&invoice_meta_key_v3(id)) +} + +/// Run every migration between the stored schema version and +/// [`CURRENT_SCHEMA_VERSION`], in order, bumping the stored version after +/// each step so a partially-upgraded contract resumes from wherever it left +/// off if `migrate` is called again (e.g. after hitting the instruction +/// budget on a contract with many invoices). +pub fn run_pending_migrations(env: &Env) -> u32 { + let mut version = schema_version(env); + if version < 2 { + migration_v2(env); + version = 2; + set_schema_version(env, version); + } + if version < 3 { + migration_v3(env); + version = 3; + set_schema_version(env, version); + } + version +} diff --git a/contracts/split/src/test.rs b/contracts/split/src/test.rs index 5774771..826f722 100644 --- a/contracts/split/src/test.rs +++ b/contracts/split/src/test.rs @@ -11290,3 +11290,144 @@ fn test_direct_wallet_calls_bypass_rate_limiter() { let invoice_id = make_invoice(&env, &c, &creator, &recipient, 100, &token_id, 9_999); assert_eq!(invoice_id, 1); } + +// --------------------------------------------------------------------------- +// Storage migration framework +// --------------------------------------------------------------------------- + +#[test] +fn test_fresh_contract_starts_at_current_schema() { + let (env, contract_id, token_id) = setup(); + let c = client(&env, &contract_id); + + let admin = Address::generate(&env); + let treasury = Address::generate(&env); + c.initialize( + &admin, &0_i128, &treasury, &token_id, &0_u32, &None, &0_u32, &0_u32, &0_u64, + ); + + // A freshly-initialized contract has nothing to migrate. + assert_eq!(c.get_schema_version(), migrations::CURRENT_SCHEMA_VERSION); + // migrate() is a no-op (but still auth-checked) once already current. + assert_eq!(c.migrate(&admin), migrations::CURRENT_SCHEMA_VERSION); +} + +#[test] +#[should_panic(expected = "caller is not an admin")] +fn test_migrate_requires_super_admin() { + let (env, contract_id, token_id) = setup(); + let c = client(&env, &contract_id); + + let admin = Address::generate(&env); + let treasury = Address::generate(&env); + let not_admin = Address::generate(&env); + c.initialize( + &admin, &0_i128, &treasury, &token_id, &0_u32, &None, &0_u32, &0_u32, &0_u64, + ); + + c.migrate(¬_admin); +} + +#[test] +#[should_panic(expected = "MigrationRequired")] +fn test_stale_schema_blocks_create_invoice() { + let (env, contract_id, token_id) = setup(); + let c = client(&env, &contract_id); + + let admin = Address::generate(&env); + let treasury = Address::generate(&env); + let creator = Address::generate(&env); + let recipient = Address::generate(&env); + c.initialize( + &admin, &0_i128, &treasury, &token_id, &0_u32, &None, &0_u32, &0_u32, &0_u64, + ); + + // Simulate a contract whose Wasm was upgraded out from under existing + // data, leaving schema_version behind at the pre-framework value. + env.as_contract(&contract_id, || { + env.storage() + .instance() + .set(&migrations::schema_version_key(), &1u32); + }); + + make_invoice(&env, &c, &creator, &recipient, 100, &token_id, 9_999); +} + +#[test] +#[should_panic(expected = "MigrationRequired")] +fn test_stale_schema_blocks_pause() { + let (env, contract_id, token_id) = setup(); + let c = client(&env, &contract_id); + + let admin = Address::generate(&env); + let treasury = Address::generate(&env); + c.initialize( + &admin, &0_i128, &treasury, &token_id, &0_u32, &None, &0_u32, &0_u32, &0_u64, + ); + + env.as_contract(&contract_id, || { + env.storage() + .instance() + .set(&migrations::schema_version_key(), &1u32); + }); + + c.pause(&admin); +} + +#[test] +fn test_migrate_from_v1_backfills_and_renames_invoice_notes() { + let (env, contract_id, token_id) = setup(); + let c = client(&env, &contract_id); + + let admin = Address::generate(&env); + let treasury = Address::generate(&env); + let creator = Address::generate(&env); + let recipient = Address::generate(&env); + c.initialize( + &admin, &0_i128, &treasury, &token_id, &0_u32, &None, &0_u32, &0_u32, &0_u64, + ); + + // Invoices created before the simulated upgrade — stands in for data + // that pre-dates the migration on a real upgraded deployment. + let id1 = make_invoice(&env, &c, &creator, &recipient, 100, &token_id, 9_999); + let id2 = make_invoice(&env, &c, &creator, &recipient, 200, &token_id, 9_999); + + // Roll the contract back to a "just upgraded from v1" state: no + // InvoiceMeta records exist yet, matching a real pre-framework contract. + env.as_contract(&contract_id, || { + env.storage() + .instance() + .set(&migrations::schema_version_key(), &1u32); + }); + assert_eq!(c.get_schema_version(), 1); + assert!(c.get_invoice_note(&id1).is_none()); + + let result = c.migrate(&admin); + assert_eq!(result, migrations::CURRENT_SCHEMA_VERSION); + assert_eq!(c.get_schema_version(), migrations::CURRENT_SCHEMA_VERSION); + + // Both pre-existing invoices were backfilled (v1 -> v2) with a default + // note, now readable under the renamed (v2 -> v3) storage key. + let note1 = c.get_invoice_note(&id1).expect("note migrated for id1"); + assert_eq!(note1.priority, 0); + assert_eq!(note1.note, String::from_str(&env, "")); + let note2 = c.get_invoice_note(&id2).expect("note migrated for id2"); + assert_eq!(note2.priority, 0); + + // The pre-rename (v2) key no longer holds anything once migrated. + env.as_contract(&contract_id, || { + assert!(!env + .storage() + .persistent() + .has(&migrations::invoice_meta_key_v2(id1))); + }); + + // Migrating again is a no-op and stays idempotent. + assert_eq!(c.migrate(&admin), migrations::CURRENT_SCHEMA_VERSION); + + // Previously-blocked entry points work again post-migration. + c.pause(&admin); + c.unpause(&admin); + let id3 = make_invoice(&env, &c, &creator, &recipient, 300, &token_id, 9_999); + assert!(id3 > id2); +}