MESH-1722 fix dedup in compliance requirements (#1142)

* Add test for `change_compliance_requirement` bug.

* Fix bug with `change_compliance_requirement`

* Add missing `ensure_issuers_in_req_limited` check.

`change_compliance_requirement` was missing a check that the new
compliance requirement had a limited number of issuers.

* Test for dedup bug in `replace_asset_compliance`

* Fix dedup in `replace_asset_compliance`

* Add comment about sorting.

* Use binary search.

* Improve benchmarks for `replace_asset_compliance`

Author: Neopallium

pallets/compliance-manager/src/benchmarking.rs

@@ -291,9 +291,9 @@ benchmarks! {
     }
 
     replace_asset_compliance {
-        let c in 0..(MAX_COMPLIANCE_REQUIREMENTS-1);
+        let c in 0..MAX_COMPLIANCE_REQUIREMENTS;
 
-        // Add the compliance requirement.
+        // Always add at least one compliance requirement.
         let d = ComplianceRequirementBuilder::<T>::new(
             MAX_TRUSTED_ISSUER_PER_CONDITION,
             MAX_SENDER_CONDITIONS_PER_COMPLIANCE,
@@ -304,8 +304,8 @@ benchmarks! {
         let sender_conditions = make_conditions(MAX_SENDER_CONDITIONS_PER_COMPLIANCE, &issuers);
         let receiver_conditions = make_conditions(MAX_RECEIVER_CONDITIONS_PER_COMPLIANCE, &issuers);
 
-        // Add new requirements to the asset.
-        (0..c).for_each( |_i| {
+        // Add more requirements to the asset, if `c > 1`.
+        (1..c).for_each( |_i| {
             let _ = Module::<T>::add_compliance_requirement(
                 d.owner.origin.clone().into(),
                 d.ticker.clone(),

pallets/compliance-manager/src/lib.rs

@@ -201,10 +201,9 @@ decl_module! {
             // Bundle as a requirement.
             let id = Self::get_latest_requirement_id(ticker) + 1u32;
             let mut new_req = ComplianceRequirement { sender_conditions, receiver_conditions, id };
-            new_req.dedup();
 
-            // Ensure issuers are limited in length.
-            Self::ensure_issuers_in_req_limited(&new_req)?;
+            // Dedup `ClaimType`s and ensure issuers are limited in length.
+            Self::dedup_and_ensure_requirement_limited(&mut new_req)?;
 
             // Add to existing requirements, and place a limit on the total complexity.
             let mut asset_compliance = AssetCompliances::get(ticker);
@@ -245,6 +244,9 @@ decl_module! {
 
         /// Replaces an asset's compliance by ticker with a new compliance.
         ///
+        /// Compliance requirements will be sorted (ascending by id) before
+        /// replacing the current requirements.
+        ///
         /// # Arguments
         /// * `ticker` - the asset ticker,
         /// * `asset_compliance - the new asset compliance.
@@ -265,14 +267,14 @@ decl_module! {
             // Ensure there are no duplicate requirement ids.
             let mut asset_compliance = asset_compliance;
             let start_len = asset_compliance.len();
+            asset_compliance.sort_by_key(|r| r.id);
             asset_compliance.dedup_by_key(|r| r.id);
             ensure!(start_len == asset_compliance.len(), Error::<T>::DuplicateComplianceRequirements);
 
-            // Dedup `ClaimType`s in `TrustedFor::Specific`.
-            asset_compliance.iter_mut().for_each(|r| r.dedup());
+            // Dedup `ClaimType`s and ensure issuers are limited in length.
+            asset_compliance.iter_mut().try_for_each(Self::dedup_and_ensure_requirement_limited)?;
 
-            // Ensure issuers are limited in length + limit the complexity.
-            asset_compliance.iter().try_for_each(Self::ensure_issuers_in_req_limited)?;
+            // Ensure the complexity is limited.
             Self::verify_compliance_complexity(&asset_compliance, ticker, 0)?;
 
             // Commit changes to storage + emit event.
@@ -400,19 +402,24 @@ decl_module! {
             // Ensure `Scope::Custom(..)`s are limited.
             Self::ensure_custom_scopes_limited(new_req.conditions())?;
 
-            ensure!(Self::get_latest_requirement_id(ticker) >= new_req.id, Error::<T>::InvalidComplianceRequirementId);
-
             let mut asset_compliance = AssetCompliances::get(ticker);
             let reqs = &mut asset_compliance.requirements;
-            if let Some(req) = reqs.iter_mut().find(|req| req.id == new_req.id) {
-                let mut new_req = new_req;
-                new_req.dedup();
-
-                *req = new_req.clone();
-                Self::verify_compliance_complexity(&reqs, ticker, 0)?;
-                AssetCompliances::insert(&ticker, asset_compliance);
-                Self::deposit_event(Event::ComplianceRequirementChanged(did, ticker, new_req));
-            }
+
+            // If the compliance requirement is not found, throw an error.
+            let pos = reqs.binary_search_by_key(&new_req.id, |req| req.id)
+                .map_err(|_| Error::<T>::InvalidComplianceRequirementId)?;
+
+            // Dedup `ClaimType`s and ensure issuers are limited in length.
+            let mut new_req = new_req;
+            Self::dedup_and_ensure_requirement_limited(&mut new_req)?;
+
+            // Update asset compliance and verify complexity is limited.
+            reqs[pos] = new_req.clone();
+            Self::verify_compliance_complexity(&reqs, ticker, 0)?;
+
+            // Store updated asset compliance.
+            AssetCompliances::insert(&ticker, asset_compliance);
+            Self::deposit_event(Event::ComplianceRequirementChanged(did, ticker, new_req));
         }
     }
 }
@@ -617,6 +624,14 @@ impl<T: Config> Module<T> {
             .try_for_each(Identity::<T>::ensure_custom_scopes_limited)
     }
 
+    fn dedup_and_ensure_requirement_limited(req: &mut ComplianceRequirement) -> DispatchResult {
+        // Dedup `ClaimType`s in `TrustedFor::Specific`.
+        req.dedup();
+
+        // Ensure issuers are limited in length.
+        Self::ensure_issuers_in_req_limited(req)
+    }
+
     fn ensure_issuers_in_req_limited(req: &ComplianceRequirement) -> DispatchResult {
         req.conditions().try_for_each(|cond| {
             ensure_length_ok::<T>(cond.issuers.len())?;

pallets/runtime/tests/src/compliance_manager_test.rs

@@ -73,6 +73,14 @@ macro_rules! assert_add_claim {
     };
 }
 
+fn get_latest_requirement_id(ticker: Ticker) -> u32 {
+    ComplianceManager::asset_compliance(ticker)
+        .requirements
+        .last()
+        .map(|r| r.id)
+        .unwrap_or(0)
+}
+
 #[test]
 fn should_add_and_verify_compliance_requirement() {
     ExtBuilder::default()
@@ -244,19 +252,35 @@ fn should_add_and_verify_compliance_requirement_we() {
         receiver_condition2
     );
 
+    let comp_req = ComplianceRequirement {
+        sender_conditions: vec![sender_condition.clone()],
+        receiver_conditions: vec![receiver_condition1.clone(), receiver_condition2.clone()],
+        id: 1,
+    };
+
+    // Add two more compliance requirements.
     for _ in 0..2 {
         assert_ok!(ComplianceManager::add_compliance_requirement(
             owner.origin(),
             ticker,
-            vec![sender_condition.clone()],
-            vec![receiver_condition1.clone(), receiver_condition2.clone()],
+            comp_req.sender_conditions.clone(),
+            comp_req.receiver_conditions.clone(),
         ));
     }
+    assert_eq!(get_latest_requirement_id(ticker), 3);
     assert_ok!(ComplianceManager::remove_compliance_requirement(
         owner.origin(),
         ticker,
         1
     )); // OK; latest == 3
+    assert_eq!(get_latest_requirement_id(ticker), 3);
+
+    // Try changing the removed compliance requirement.
+    assert_noop!(
+        ComplianceManager::change_compliance_requirement(owner.origin(), ticker, comp_req),
+        CMError::<TestStorage>::InvalidComplianceRequirementId
+    ); // BAD OK; latest == 3, but 1 was just removed.
+
     assert_noop!(
         ComplianceManager::remove_compliance_requirement(owner.origin(), ticker, 1),
         CMError::<TestStorage>::InvalidComplianceRequirementId
@@ -265,6 +289,7 @@ fn should_add_and_verify_compliance_requirement_we() {
         ComplianceManager::remove_compliance_requirement(owner.origin(), ticker, 1),
         CMError::<TestStorage>::InvalidComplianceRequirementId
     );
+    assert_eq!(get_latest_requirement_id(ticker), 3);
 }
 
 #[test]
@@ -309,6 +334,53 @@ fn should_replace_asset_compliance_we() {
     assert_eq!(asset_compliance.requirements, new_asset_compliance);
 }
 
+#[test]
+fn test_dedup_replace_asset_compliance() {
+    ExtBuilder::default()
+        .build()
+        .execute_with(test_dedup_replace_asset_compliance_we);
+}
+
+fn test_dedup_replace_asset_compliance_we() {
+    let owner = User::new(AccountKeyring::Alice);
+
+    // Create & mint token
+    let (ticker, _) = create_token(owner);
+
+    Balances::make_free_balance_be(&owner.acc(), 1_000_000);
+
+    allow_all_transfers(ticker, owner);
+
+    let asset_compliance = ComplianceManager::asset_compliance(ticker);
+    assert_eq!(asset_compliance.requirements.len(), 1);
+
+    let make_req = |id: u32| ComplianceRequirement {
+        sender_conditions: vec![],
+        receiver_conditions: vec![],
+        id,
+    };
+
+    // Replace should throw an error if there are duplicate requirement ids.
+    assert_noop!(
+        ComplianceManager::replace_asset_compliance(
+            owner.origin(),
+            ticker,
+            vec![make_req(1), make_req(2), make_req(2),],
+        ),
+        CMError::<TestStorage>::DuplicateComplianceRequirements
+    );
+
+    // Test with mixed duplicate ids.  To test sorting.
+    assert_noop!(
+        ComplianceManager::replace_asset_compliance(
+            owner.origin(),
+            ticker,
+            vec![make_req(2), make_req(1), make_req(2),],
+        ),
+        CMError::<TestStorage>::DuplicateComplianceRequirements
+    );
+}
+
 #[test]
 fn should_reset_asset_compliance() {
     ExtBuilder::default()