fixes

crates/sargon/src/factor_instances_provider/agnostic_paths/derivation_preset.rs

@@ -76,7 +76,6 @@ impl DerivationPreset {
         }
     }
 
-
     /// Selects a `DerivationPreset` for MFA based on `CAP26EntityKind`,
     /// i.e. either `DerivationPreset::AccountRola` or `DerivationPreset::IdentityRola`.
     pub fn rola_entity_kind(entity_kind: CAP26EntityKind) -> Self {
@@ -171,13 +170,25 @@ mod tests {
 
     #[test]
     fn test_mfa_entity_kind() {
-        assert_eq!(SUT::mfa_entity_kind(CAP26EntityKind::Account), SUT::AccountMfa);
-        assert_eq!(SUT::mfa_entity_kind(CAP26EntityKind::Identity), SUT::IdentityMfa);
+        assert_eq!(
+            SUT::mfa_entity_kind(CAP26EntityKind::Account),
+            SUT::AccountMfa
+        );
+        assert_eq!(
+            SUT::mfa_entity_kind(CAP26EntityKind::Identity),
+            SUT::IdentityMfa
+        );
     }
 
     #[test]
     fn test_rola_entity_kind() {
-        assert_eq!(SUT::rola_entity_kind(CAP26EntityKind::Account), SUT::AccountRola);
-        assert_eq!(SUT::rola_entity_kind(CAP26EntityKind::Identity), SUT::IdentityRola);
+        assert_eq!(
+            SUT::rola_entity_kind(CAP26EntityKind::Account),
+            SUT::AccountRola
+        );
+        assert_eq!(
+            SUT::rola_entity_kind(CAP26EntityKind::Identity),
+            SUT::IdentityRola
+        );
     }
 }

crates/sargon/src/factor_instances_provider/agnostic_paths/quantified_derivation_preset.rs

@@ -22,8 +22,27 @@ impl QuantifiedDerivationPreset {
         }
     }
 
-    pub fn mfa_for_entities(
+    /// Returns a the QuantifiedDerivationPresets needed to securify the `addresses_of_entities`, including
+    /// a new Authentication Signing factor instance for each entity. Will return
+    /// the `Account` variant of each DerivationPreset for each Account in `addresses_of_entities`
+    /// and the `Identity` variant of each DerivationPreset for each Persona in `addresses_of_entities`.
+    pub fn securifying_unsecurified_entities(
         addresses_of_entities: &IndexSet<AddressOfAccountOrPersona>,
+    ) -> IdentifiedVecOf<Self> {
+        Self::mfa_for_entities(addresses_of_entities, true)
+    }
+
+    /// Returns a the QuantifiedDerivationPresets needed to securify the `addresses_of_entities`,  Will return
+    /// the `Account` variant of each DerivationPreset for each Account in `addresses_of_entities`
+    /// and the `Identity` variant of each DerivationPreset for each Persona in `addresses_of_entities`.
+    ///
+    /// if `include_rola_key_for_each_entity` is `true` a ROLA key for each entity will be included.
+    /// Typically we only set `include_rola_key_for_each_entity` to `true` for securifying
+    /// unsecurified entities. For already securified entities we might not
+    /// need to change the ROLA key.
+    fn mfa_for_entities(
+        addresses_of_entities: &IndexSet<AddressOfAccountOrPersona>,
+        include_rola_key_for_each_entity: bool,
     ) -> IdentifiedVecOf<Self> {
         let account_addresses = addresses_of_entities
             .iter()
@@ -36,44 +55,55 @@ impl QuantifiedDerivationPreset {
 
         match (account_addresses.is_empty(), identity_addresses.is_empty()) {
             (true, true) => IdentifiedVecOf::new(), // weird!
-            (true, false) => IdentifiedVecOf::from_iter([
-                Self::new(
-                    DerivationPreset::IdentityMfa,
-                    identity_addresses.len(),
-                ),
-                Self::new(
-                    DerivationPreset::IdentityRola,
-                    identity_addresses.len(),
-                ),
-            ]),
-            (false, false) => IdentifiedVecOf::from_iter([
-                Self::new(
-                    DerivationPreset::AccountMfa,
-                    account_addresses.len(),
-                ),
-                Self::new(
-                    DerivationPreset::AccountRola,
-                    account_addresses.len(),
-                ),
-                Self::new(
+            (true, false) => {
+                let mut presets = IdentifiedVecOf::just(Self::new(
                     DerivationPreset::IdentityMfa,
                     identity_addresses.len(),
-                ),
-                Self::new(
-                    DerivationPreset::IdentityRola,
-                    identity_addresses.len(),
-                ),
-            ]),
-            (false, true) => IdentifiedVecOf::from_iter([
-                Self::new(
+                ));
+                if include_rola_key_for_each_entity {
+                    presets.append(Self::new(
+                        DerivationPreset::IdentityRola,
+                        identity_addresses.len(),
+                    ));
+                }
+                presets
+            }
+            (false, false) => {
+                let mut presets = IdentifiedVecOf::from_iter([
+                    Self::new(
+                        DerivationPreset::AccountMfa,
+                        account_addresses.len(),
+                    ),
+                    Self::new(
+                        DerivationPreset::IdentityMfa,
+                        identity_addresses.len(),
+                    ),
+                ]);
+                if include_rola_key_for_each_entity {
+                    presets.append(Self::new(
+                        DerivationPreset::AccountRola,
+                        account_addresses.len(),
+                    ));
+                    presets.append(Self::new(
+                        DerivationPreset::IdentityRola,
+                        identity_addresses.len(),
+                    ));
+                }
+                presets
+            }
+            (false, true) => {
+                let mut presets = IdentifiedVecOf::just(Self::new(
                     DerivationPreset::AccountMfa,
                     account_addresses.len(),
-                ),
-                Self::new(
-                    DerivationPreset::AccountRola,
-                    account_addresses.len(),
-                ),
-            ]),
+                ));
+                if include_rola_key_for_each_entity {
+                    presets.append(Self::new(
+                        DerivationPreset::AccountRola,
+                        account_addresses.len(),
+                    ));
+                }
+                presets
+            }
         }
     }
 }

crates/sargon/src/factor_instances_provider/provider/factor_instances_provider_unit_tests.rs

@@ -1801,15 +1801,13 @@ async fn create_single_account() {
         matrix_0,
         bdfs.clone(),
     );
-    println!("👨‍🔬 test calling `make_security_structure_of_factor_instances_for_entities_without_consuming_cache_with_derivation_outcome` for single account Alice");
     let (security_structures_of_fis, instances_in_cache_consumer, derivation_outcome) = os
     .make_security_structure_of_factor_instances_for_entities_without_consuming_cache_with_derivation_outcome(
        IndexSet::just(AddressOfAccountOrPersona::from(alice.address())),
         shield_0.clone(),
     )
     .await
     .unwrap();
-    println!("👨‍🔬 ✅ `make_security_structure_of_factor_instances_for_entities_without_consuming_cache_with_derivation_outcome` for single account Alice ✅");
 
     // Don't forget to consume!
     instances_in_cache_consumer.consume().await.unwrap();

crates/sargon/src/factor_instances_provider/provider/outcome/factor_instances_provider_outcome.rs

@@ -1,7 +1,7 @@
 use crate::prelude::*;
 
-/// Identical to `InternalFactorInstancesProviderOutcome` but `FactorInstancesProviderOutcomeForFactor` instead of `InternalFactorInstancesProviderOutcomeForFactor`, having
-/// renamed field values to make it clear that `to_cache` instances  already have been cached.
+/// A collection of `FactorInstancesProviderOutcomePerFactor` keyed under
+/// DerivationPreset.
 #[derive(Clone, Debug)]
 pub struct FactorInstancesProviderOutcome {
     pub per_derivation_preset:
@@ -26,6 +26,8 @@ impl FactorInstancesProviderOutcome {
     }
 }
 
+/// A collection of `FactorInstancesProviderOutcomeForFactor` keyed by their
+/// FactorSourceID
 #[derive(Clone, Debug)]
 pub struct FactorInstancesProviderOutcomePerFactor {
     pub per_factor: IndexMap<

crates/sargon/src/factor_instances_provider/provider/outcome/internal_factor_instances_provider_outcome.rs

@@ -28,15 +28,6 @@ impl InternalFactorInstancesProviderOutcome {
     ) -> Option<&InternalFactorInstancesProviderOutcomePerFactor> {
         self.per_derivation_preset.get(&preset)
     }
-
-    pub fn get_for_derivation_preset_for_factor(
-        &self,
-        preset: DerivationPreset,
-        factor_source_id: FactorSourceIDFromHash,
-    ) -> Option<&InternalFactorInstancesProviderOutcomeForFactor> {
-        self.get_for_derivation_preset(preset)
-            .and_then(|x| x.per_factor.get(&factor_source_id))
-    }
 }
 
 #[derive(Clone, Debug)]
@@ -59,7 +50,8 @@ impl InternalFactorInstancesProviderOutcome {
         }
     }
 
-    /// "Transposes"
+    /// For each value of each collection, "transposes" it. For more info see
+    /// `InternalFactorInstancesProviderOutcomePerFactor::transpose`
     pub fn transpose(
         pdp_pf_to_cache: InstancesPerDerivationPresetPerFactorSource,
         pdp_pf_to_use_directly: InstancesPerDerivationPresetPerFactorSource,

crates/sargon/src/factor_instances_provider/provider/outcome/internal_factor_instances_provider_outcome_for_factor.rs

@@ -118,16 +118,30 @@ impl HasSampleValues for InternalFactorInstancesProviderOutcomeForFactor {
     }
 
     fn sample_other() -> Self {
-        Self::new(FactorSourceIDFromHash::sample_at(1),    FactorInstances::new(IndexSet::from_iter([
-            HierarchicalDeterministicFactorInstance::sample_mainnet_account_device_factor_fs_1_securified_at_index(2),
-        ])),    FactorInstances::new(IndexSet::from_iter([
-            HierarchicalDeterministicFactorInstance::sample_mainnet_account_device_factor_fs_1_securified_at_index(0),
-        ])),    FactorInstances::new(IndexSet::from_iter([
-            HierarchicalDeterministicFactorInstance::sample_mainnet_account_device_factor_fs_1_securified_at_index(0),
-        ])),    FactorInstances::new(IndexSet::from_iter([
-            HierarchicalDeterministicFactorInstance::sample_mainnet_account_device_factor_fs_1_securified_at_index(1),
-            HierarchicalDeterministicFactorInstance::sample_mainnet_account_device_factor_fs_1_securified_at_index(2),
-        ])))
+        Self::new(
+            FactorSourceIDFromHash::sample_at(1),
+            FactorInstances::new(
+                IndexSet::from_iter([
+                    HierarchicalDeterministicFactorInstance::sample_mainnet_account_device_factor_fs_1_securified_at_index(2),
+                ])
+            ),
+            FactorInstances::new(
+            IndexSet::from_iter([
+                    HierarchicalDeterministicFactorInstance::sample_mainnet_account_device_factor_fs_1_securified_at_index(0),
+                ])
+            ),
+            FactorInstances::new(
+                IndexSet::from_iter([
+                    HierarchicalDeterministicFactorInstance::sample_mainnet_account_device_factor_fs_1_securified_at_index(0),
+                ])
+            ),
+            FactorInstances::new(
+                IndexSet::from_iter([
+                    HierarchicalDeterministicFactorInstance::sample_mainnet_account_device_factor_fs_1_securified_at_index(1),
+                    HierarchicalDeterministicFactorInstance::sample_mainnet_account_device_factor_fs_1_securified_at_index(2),
+                ])
+            )
+        )
     }
 }
 

crates/sargon/src/factor_instances_provider/provider/provider_adopters/securify_entity_factor_instances_provider.rs

@@ -23,7 +23,7 @@ impl SecurifyEntityFactorInstancesProvider {
         interactor: Arc<dyn KeyDerivationInteractor>,
     ) -> Result<(InstancesInCacheConsumer, FactorInstancesProviderOutcome)>
     {
-        Self::for_entity_mfa(
+        Self::securifying_unsecurified(
             cache_client,
             profile,
             security_structure_of_factor_sources,
@@ -52,7 +52,7 @@ impl SecurifyEntityFactorInstancesProvider {
         interactor: Arc<dyn KeyDerivationInteractor>,
     ) -> Result<(InstancesInCacheConsumer, FactorInstancesProviderOutcome)>
     {
-        Self::for_entity_mfa(
+        Self::securifying_unsecurified(
             cache_client,
             profile,
             security_structure_of_factor_sources,
@@ -73,7 +73,11 @@ impl SecurifyEntityFactorInstancesProvider {
     ///
     /// We are always reading from the beginning of each FactorInstance collection in the cache,
     /// and we are always appending to the end.
-    pub async fn for_entity_mfa(
+    pub async fn securifying_unsecurified(
+        // if you need to UPDATE already securified, upgrade this to conditionally consume ROLA
+        // factors, by not using `QuantifiedDerivationPreset::securifying_unsecurified_entities`
+        // below. I.e. create the set of `QuantifiedDerivationPreset` which does not unconditionally
+        // specify ROLA factors.
         cache_client: Arc<FactorInstancesCacheClient>,
         profile: Arc<Profile>,
         security_structure_of_factor_sources: SecurityStructureOfFactorSources,
@@ -132,48 +136,15 @@ impl SecurifyEntityFactorInstancesProvider {
         );
 
         let quantified_derivation_presets =
-            QuantifiedDerivationPreset::mfa_for_entities(
+            QuantifiedDerivationPreset::securifying_unsecurified_entities(
                 &addresses_of_entities,
             );
 
-        println!(
-            "🌮 FIP (Sec) quantified_derivation_presets: {:?}",
-            quantified_derivation_presets
-        );
-
         assert!(quantified_derivation_presets.len() >= 2); // at least one entity kind, and ROLA + TX: at least 2
         let (instances_in_cache_consumer, outcome) = provider
             .provide_for_presets(quantified_derivation_presets, purpose)
             .await?;
 
-        if let Some(rola_accounts_outcome) = outcome
-            .get_for_derivation_preset_for_factor(
-                DerivationPreset::AccountRola,
-                security_structure_of_factor_sources
-                    .authentication_signing_factor
-                    .id_from_hash(),
-            )
-        {
-            println!(
-                "🌮 FIP (Sec) rola_accounts_outcome: {:#?}",
-                rola_accounts_outcome
-            );
-        }
-
-        if let Some(rola_personas_outcome) = outcome
-            .get_for_derivation_preset_for_factor(
-                DerivationPreset::IdentityRola,
-                security_structure_of_factor_sources
-                    .authentication_signing_factor
-                    .id_from_hash(),
-            )
-        {
-            println!(
-                "🌮 FIP (Sec) rola_personas_outcome: {:#?}",
-                rola_personas_outcome
-            );
-        }
-
         Ok((instances_in_cache_consumer, outcome.into()))
     }
 }
@@ -351,18 +322,19 @@ mod tests {
         let profile = Arc::new(os.profile().unwrap());
         let derivation_interactors = os.keys_derivation_interactor();
 
-        let (instances_in_cache_consumer, outcome) = SUT::for_entity_mfa(
-            cache_client.clone(),
-            profile,
-            shield_0.clone(),
-            IndexSet::from_iter([
-                AddressOfAccountOrPersona::from(alice.address()),
-                AddressOfAccountOrPersona::from(batman.address()),
-            ]),
-            derivation_interactors.clone(),
-        )
-        .await
-        .unwrap();
+        let (instances_in_cache_consumer, outcome) =
+            SUT::securifying_unsecurified(
+                cache_client.clone(),
+                profile,
+                shield_0.clone(),
+                IndexSet::from_iter([
+                    AddressOfAccountOrPersona::from(alice.address()),
+                    AddressOfAccountOrPersona::from(batman.address()),
+                ]),
+                derivation_interactors.clone(),
+            )
+            .await
+            .unwrap();
 
         assert_eq!(outcome.per_derivation_preset.len(), 4);