diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index c340af4..582ae49 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -33,6 +33,17 @@ jobs: restore-keys: | ${{ runner.os }}-cargo- + - name: Check for stale merge-conflict FIXMEs + # benchmarks.rs and invariant_tests.rs are intentionally still disabled + # (tracked separately) and carry accurate FIXMEs; everything else must + # be clean, so a re-enabled module never ships with a stale FIXME again. + run: | + if grep -rn "FIXME.*merge conflict" --include='*.rs' . \ + | grep -v 'benchmarks\.rs\|invariant_tests\.rs'; then + echo "Found FIXME comments referencing unresolved merge conflicts. Resolve or remove them." >&2 + exit 1 + fi + - name: Build run: cargo build --workspace --verbose diff --git a/contracts/ip_registry/src/lib.rs b/contracts/ip_registry/src/lib.rs index 38a7462..6ec60fe 100644 --- a/contracts/ip_registry/src/lib.rs +++ b/contracts/ip_registry/src/lib.rs @@ -16,8 +16,6 @@ use types::*; mod zk_commitment; -// FIXME: test.rs has compilation errors from merge conflict - re-enable after fix -// FIXME: test.rs has pre-existing compilation errors from a merge conflict - fix before enabling #[cfg(test)] mod test; @@ -1866,8 +1864,24 @@ impl IpRegistry { ) -> bool { let record = require_ip_exists(&env, ip_id); - // Reject if expired - // Expiry check removed - field not in types + // Emit EXPIRY_TOPIC exactly once per expiry transition so off-chain + // indexers can cheaply detect an IP crossing into its grace period. + if record.expiry_timestamp != 0 && env.ledger().timestamp() >= record.expiry_timestamp { + let already_notified: bool = env + .storage() + .persistent() + .get(&DataKey::ExpiryNotified(ip_id)) + .unwrap_or(false); + if !already_notified { + env.events().publish( + (EXPIRY_TOPIC, ip_id), + (record.owner.clone(), record.expiry_timestamp), + ); + env.storage() + .persistent() + .set(&DataKey::ExpiryNotified(ip_id), &true); + } + } // Concatenate secret || blinding_factor into Bytes, then SHA256 let mut preimage = soroban_sdk::Bytes::new(&env); @@ -2236,9 +2250,12 @@ impl IpRegistry { .get(&DataKey::SuggestedPrice(ip_id)) } - /// Add a co-owner to an IP. Owner-only. + /// Add a co-owner to an IP with an explicit ownership percentage. Owner-only. /// Co-owners can verify commitments but cannot transfer or revoke the IP. - pub fn add_co_owner(env: Env, ip_id: u64, co_owner: Address) { + /// + /// `percentage` is deducted from the current owner's remaining share, so the + /// full cap table (owner + co-owners) always sums to exactly 100. + pub fn add_co_owner(env: Env, ip_id: u64, co_owner: Address, percentage: u32) { let mut record = require_ip_exists(&env, ip_id); record.owner.require_auth(); @@ -2249,6 +2266,41 @@ impl IpRegistry { } } + let mut shares: Vec = env + .storage() + .persistent() + .get(&DataKey::OwnershipShares(ip_id)) + .unwrap_or_else(|| { + let mut v = Vec::new(&env); + v.push_back(OwnershipShare { + address: record.owner.clone(), + percentage: 100, + }); + v + }); + + let owner_idx = shares + .iter() + .position(|s| s.address == record.owner) + .unwrap_or_else(|| { + env.panic_with_error(Error::from_contract_error( + ContractError::InvalidShareTotal as u32, + )) + }) as u32; + let mut owner_share = shares.get(owner_idx).unwrap(); + if percentage == 0 || percentage > owner_share.percentage { + env.panic_with_error(Error::from_contract_error( + ContractError::InvalidOwnershipPercentage as u32, + )); + } + owner_share.percentage -= percentage; + shares.set(owner_idx, owner_share); + shares.push_back(OwnershipShare { + address: co_owner.clone(), + percentage, + }); + require_valid_share_total(&env, &shares); + record.co_owners.push_back(co_owner.clone()); env.storage() .persistent() @@ -2256,12 +2308,21 @@ impl IpRegistry { env.storage() .persistent() .extend_ttl(&DataKey::IpRecord(ip_id), LEDGER_BUMP, LEDGER_BUMP); + env.storage() + .persistent() + .set(&DataKey::OwnershipShares(ip_id), &shares); + env.storage().persistent().extend_ttl( + &DataKey::OwnershipShares(ip_id), + LEDGER_BUMP, + LEDGER_BUMP, + ); env.events() .publish((symbol_short!("co_add"), record.owner), (ip_id, co_owner)); } /// Remove a co-owner from an IP. Owner-only. + /// The removed co-owner's percentage is returned to the primary owner's share. pub fn remove_co_owner(env: Env, ip_id: u64, co_owner: Address) { let mut record = require_ip_exists(&env, ip_id); record.owner.require_auth(); @@ -2278,11 +2339,47 @@ impl IpRegistry { LEDGER_BUMP, ); + if let Some(mut shares) = env + .storage() + .persistent() + .get::>(&DataKey::OwnershipShares(ip_id)) + { + if let Some(share_idx) = shares.iter().position(|s| s.address == co_owner) { + let removed = shares.get(share_idx as u32).unwrap(); + shares.remove(share_idx as u32); + if let Some(owner_idx) = shares.iter().position(|s| s.address == record.owner) + { + let mut owner_share = shares.get(owner_idx as u32).unwrap(); + owner_share.percentage += removed.percentage; + shares.set(owner_idx as u32, owner_share); + } + require_valid_share_total(&env, &shares); + env.storage() + .persistent() + .set(&DataKey::OwnershipShares(ip_id), &shares); + env.storage().persistent().extend_ttl( + &DataKey::OwnershipShares(ip_id), + LEDGER_BUMP, + LEDGER_BUMP, + ); + } + } + env.events() .publish((symbol_short!("co_rem"), record.owner), (ip_id, co_owner)); } } + /// Get the current ownership cap table (owner + co-owners) for an IP. + /// Returns an empty vector if no co-owners have ever been added. + pub fn get_ownership_shares(env: Env, ip_id: u64) -> Vec { + require_ip_exists(&env, ip_id); + env.storage() + .persistent() + .get(&DataKey::OwnershipShares(ip_id)) + .unwrap_or_else(|| Vec::new(&env)) + } + /// Create a new version of an existing IP commitment. /// /// This function allows an IP owner to create a new version of their IP @@ -5515,6 +5612,9 @@ impl IpRegistry { env.storage() .persistent() .extend_ttl(&DataKey::IpRecord(ip_id), LEDGER_BUMP, LEDGER_BUMP); + env.storage() + .persistent() + .remove(&DataKey::ExpiryNotified(ip_id)); } /// Renew an IP commitment's expiry. Owner-only. @@ -5537,6 +5637,9 @@ impl IpRegistry { env.storage() .persistent() .extend_ttl(&DataKey::IpRecord(ip_id), LEDGER_BUMP, LEDGER_BUMP); + env.storage() + .persistent() + .remove(&DataKey::ExpiryNotified(ip_id)); env.events().publish( (symbol_short!("ip_renew"), record.owner), diff --git a/contracts/ip_registry/src/test.rs b/contracts/ip_registry/src/test.rs index 5a967c6..5aa1f93 100644 --- a/contracts/ip_registry/src/test.rs +++ b/contracts/ip_registry/src/test.rs @@ -197,6 +197,10 @@ mod tests { fn set_ip_expiry(env: Env, ip_id: u64, expiry_timestamp: u64, grace_period_seconds: u64); fn renew_ip_commitment(env: Env, ip_id: u64, new_expiry: u64) -> bool; fn cleanup_expired_ips(env: Env, ip_ids: Vec); + // Issue #809 + fn add_co_owner(env: Env, ip_id: u64, co_owner: Address, percentage: u32); + fn remove_co_owner(env: Env, ip_id: u64, co_owner: Address); + fn get_ownership_shares(env: Env, ip_id: u64) -> Vec; } #[test] @@ -2760,6 +2764,92 @@ mod expiry_tests { let events = env.events().all(); assert!(events.events().len() >= 1, "ip_clean event must be emitted"); } + + #[test] + fn test_verify_commitment_emits_expiry_event_exactly_once() { + let (env, client, _owner, ip_id) = setup(); + let now = env.ledger().timestamp(); + client.set_ip_expiry(&ip_id, &(now + 100), &500); + + // Advance past expiry but still within the grace period. + env.ledger().with_mut(|l| l.timestamp = now + 150); + + let secret = BytesN::from_array(&env, &[1u8; 32]); + let blinding = BytesN::from_array(&env, &[2u8; 32]); + + // No events emitted yet (set_ip_expiry does not publish). + assert_eq!(env.events().all().events().len(), 0); + + // Calling verify_commitment repeatedly must only fire the expiry + // event once for this expiry transition. + client.verify_commitment(&ip_id, &secret, &blinding); + assert_eq!(env.events().all().events().len(), 1); + + client.verify_commitment(&ip_id, &secret, &blinding); + client.verify_commitment(&ip_id, &secret, &blinding); + assert_eq!( + env.events().all().events().len(), + 1, + "EXPIRY_TOPIC must fire exactly once per expiry transition" + ); + } +} + +// ── Issue #809: OwnershipShare Percentage Sum Invariant ────────────────────── + +#[cfg(test)] +mod ownership_share_tests { + use super::tests::IpRegistryClient; + use soroban_sdk::{testutils::Address as _, Address, BytesN, Env}; + + fn setup() -> (Env, IpRegistryClient<'static>, Address, u64) { + let env = Env::default(); + env.mock_all_auths(); + let contract_id = env.register(crate::IpRegistry, ()); + let client = IpRegistryClient::new(&env, &contract_id); + let owner = Address::generate(&env); + let hash = BytesN::from_array(&env, &[0x55u8; 32]); + let ip_id = client.commit_ip(&owner, &hash, &0u32); + (env, client, owner, ip_id) + } + + #[test] + fn test_add_co_owner_splits_shares_to_100() { + let (env, client, owner, ip_id) = setup(); + let co_owner = Address::generate(&env); + client.add_co_owner(&ip_id, &co_owner, &40); + + let shares = client.get_ownership_shares(&ip_id); + assert_eq!(shares.len(), 2); + let total: u32 = shares.iter().map(|s| s.percentage).sum(); + assert_eq!(total, 100); + assert_eq!(shares.get(0).unwrap().address, owner); + assert_eq!(shares.get(0).unwrap().percentage, 60); + assert_eq!(shares.get(1).unwrap().address, co_owner); + assert_eq!(shares.get(1).unwrap().percentage, 40); + } + + #[test] + #[should_panic] + fn test_add_co_owner_rejects_percentage_exceeding_owner_share() { + let (env, client, _owner, ip_id) = setup(); + let co_owner = Address::generate(&env); + // Owner only holds 100; requesting 101 must panic (sum would exceed 100). + client.add_co_owner(&ip_id, &co_owner, &101); + } + + #[test] + fn test_remove_co_owner_returns_share_to_owner() { + let (env, client, owner, ip_id) = setup(); + let co_owner = Address::generate(&env); + client.add_co_owner(&ip_id, &co_owner, &30); + client.remove_co_owner(&ip_id, &co_owner); + + let shares = client.get_ownership_shares(&ip_id); + assert_eq!(shares.len(), 1); + assert_eq!(shares.get(0).unwrap().address, owner); + assert_eq!(shares.get(0).unwrap().percentage, 100); + } } // ── #464: get_blinded_owner_batch tests ────────────────────────────────────── diff --git a/contracts/ip_registry/src/types.rs b/contracts/ip_registry/src/types.rs index c0546bb..ff8cac2 100644 --- a/contracts/ip_registry/src/types.rs +++ b/contracts/ip_registry/src/types.rs @@ -12,6 +12,7 @@ pub const LEDGER_BUMP: u32 = 6_307_200; pub const REVOKE_TOPIC: Symbol = soroban_sdk::symbol_short!("revoke"); pub const TRANSFER_TOPIC: Symbol = soroban_sdk::symbol_short!("ip_xfer"); pub const BATCH_VERIFY_TOPIC: Symbol = soroban_sdk::symbol_short!("batch_vfy"); +pub const EXPIRY_TOPIC: Symbol = soroban_sdk::symbol_short!("ip_expiry"); // ── Access Control ──────────────────────────────────────────────────────────── diff --git a/contracts/ip_registry/src/validation.rs b/contracts/ip_registry/src/validation.rs index 3a24901..8e25281 100644 --- a/contracts/ip_registry/src/validation.rs +++ b/contracts/ip_registry/src/validation.rs @@ -3,8 +3,8 @@ //! This module provides reusable validation functions to reduce code duplication //! and ensure consistent error handling across the contract. -use crate::{ContractError, DataKey, IpRecord}; -use soroban_sdk::{symbol_short, Address, BytesN, Env, Error}; +use crate::{ContractError, DataKey, IpRecord, OwnershipShare}; +use soroban_sdk::{symbol_short, Address, BytesN, Env, Error, Vec}; /// Retrieves an IP record by ID, panicking if not found. /// @@ -182,6 +182,25 @@ pub fn calculate_commitment_strength(secret_length: u32, pow_difficulty: u32) -> } } +/// Validates that a set of `OwnershipShare` percentages sums to exactly 100. +/// +/// # Arguments +/// +/// * `env` - The Soroban environment +/// * `shares` - The full cap table (owner + co-owners) for an IP +/// +/// # Panics +/// +/// Panics with `InvalidShareTotal` if the percentages do not sum to exactly 100. +pub fn require_valid_share_total(env: &Env, shares: &Vec) { + let total: u32 = shares.iter().map(|s| s.percentage).sum(); + if total != 100 { + env.panic_with_error(Error::from_contract_error( + ContractError::InvalidShareTotal as u32, + )); + } +} + #[cfg(test)] mod tests { use super::*; @@ -306,4 +325,37 @@ mod tests { }; require_owner(&env, ¬_owner, &record); } + + fn shares_summing_to(env: &Env, total: u32) -> soroban_sdk::Vec { + let mut shares = soroban_sdk::Vec::new(env); + shares.push_back(OwnershipShare { + address: Address::generate(env), + percentage: total, + }); + shares + } + + #[test] + #[should_panic] + fn test_require_valid_share_total_panics_for_99() { + let env = Env::default(); + let shares = shares_summing_to(&env, 99); + require_valid_share_total(&env, &shares); + } + + #[test] + fn test_require_valid_share_total_succeeds_for_100() { + let env = Env::default(); + let shares = shares_summing_to(&env, 100); + // Should not panic + require_valid_share_total(&env, &shares); + } + + #[test] + #[should_panic] + fn test_require_valid_share_total_panics_for_101() { + let env = Env::default(); + let shares = shares_summing_to(&env, 101); + require_valid_share_total(&env, &shares); + } }