Merge pull request #938 from Zalgia-Saga/fix/ip-registry-807-810

Fix/ip registry 807 810

Author: fejilaup-cloud

.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
 

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,19 +2266,63 @@ impl IpRegistry {
             }
         }
 
+        let mut shares: Vec<OwnershipShare> = 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()
             .set(&DataKey::IpRecord(ip_id), &record);
         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, Vec<OwnershipShare>>(&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<OwnershipShare> {
+        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),

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<u64>);
+        // 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<crate::OwnershipShare>;
     }
 
     #[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 ──────────────────────────────────────

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 ────────────────────────────────────────────────────────────
 

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<OwnershipShare>) {
+    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, &not_owner, &record);
     }
+
+    fn shares_summing_to(env: &Env, total: u32) -> soroban_sdk::Vec<OwnershipShare> {
+        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);
+    }
 }