FactorInstanceProvider ("FIP") with Serializable FactorInstancesCache + consumers of "FIP"

[CyonAlexRDX]

Abstract

Add a cache for FactorInstances on file. We pre-derive FactorInstances for every new FactorSource being added. We always try to use FactorInstances from the cache if possible.

Add foundation for Securifying Accounts/Personas by mapping (Set<EntityAddress>, SecurityStructureOfFactorSources) -> Map<EntityAddress, SecurityStructureOfFactorInstances>
(needs implementation of the proper rules of which FactorSources can be used for which roles according to Matts doc)

Description

Caution

Breaking change in SargonOS, the newWallet now takes a required shouldPrederiveInstances: bool parameter, and you SHOULD pass false for PROD, but you can pass true for DEBUG builds, see
iOS PR: https://github.com/radixdlt/babylon-wallet-ios/pull/1391/files

Note

Coverage is much higher than reported, tarpaulin fails to understand that parameters passed to a function on multiple lines are in fact tested.

This PR does quite a few things, but I will try to describe it as a delta compared to the code being migrated over from Sargon-MFA repo - which post merging this can be deleted.

• Migrate KeysCollector from Sargon-MFA, see original PR with KeysCollector (and SignaturesCollector) which got review by @matiasbzurovski and @GhenadieVP : SignaturesCollector + KeysCollector sargon-mfa#1
• Migrate FactorInstancesProvider + Cache, which also was reviewed @matiasbzurovski and @GhenadieVP : FactorInstancesProvider sargon-mfa#18

Delta compared to Sargon-MFA

The code in Sargon-MFA, as @micbakos-rdx painfully knows, was quite simplified in order for me to solve the crux of the problem, it allowed me to iterate fast and solve the underlying hard problems. One fundamental simplification which Sargon-MFA repo did was to simplify MatrixOfFactor<F> = (PrimaryRoleWithFactors<F>, RecoveryRoleWithFactors<F>, ConfirmationRoleWithFactors<F>) into RoleWithFactors<F> (but called "Matrix"). Another major simplification was Entities and FactorInstances (did not contain real keys).

The implementation in Sargon-MFA deleted the FactorInstances from the cache "eagerly". However, this is not so useful, since we have more "steps" in SargonOS which might fail. For example, when we query the FactorInstancesProvider (which tries to load from cache) for an FactorInstance for creating a new virtual account, we wanna use that FactorInstance to create a new account, then save it into Profile and finally persist the serialized Profile JSON into SecureStorage. If any of these steps would fail, we would already have consumed the FactorInstance, and thus create "gaps" in the Derivation Entities. So an important change I've made here (compared to Sargon-MFA repo impl) is a InstancesInCacheConsumer, which contains a single (async throwing) method consume, which we can call after having successfully persisted the updated Profile. This allows us to ensure we dont create such gaps.

Whats new

Introduce:

• KeysCollector
• FactorInstancesProvider
• FactorInstancesCache
• InstancesInCacheConsumer

And higher level providers:

• VirtualEntityCreatingInstanceProvider
• SecurifyEntityFactorInstancesProvider
• CacheFiller

SargonOS APIs:

Gated behind #[cfg(test)] for now:

make_security_structure_of_factor_instances_for_entities...

Which maps SecurityStructureOfFactorSources -> IndexMap<EntityAddress, SecurityStructureOfFactorInstances> for provided entity addresses.

__OFFLINE_ONLY_securify_accounts

Which mutates profile and changes accounts to be Securified, using make_security_structure_of_factor_instances_for_entities... above, named like that since it does not try to submit any transaction on network - no "real securify".

Prod available

CRUD operations for Persona(s), such as create, update, batch create (useful for tests in hosts).

Further Changes to SargonOS

SargonOS has received a third field:

pub struct SargonOS {
    pub(crate) profile_state_holder: ProfileStateHolder,
    pub(crate) clients: Clients,
+    pub(crate) interactors: Interactors
}

Where Interactors currently is:

pub struct Interactors {
    pub key_derivation: Arc<dyn KeysDerivationInteractors>,
}

Maybe we change this to contain a signing_interactors of type SignInteractors - already in main branch but not used by hosts (only used by tests):

pub struct Interactors {
    pub key_derivation: Arc<dyn KeysDerivationInteractors>,
+. pub signing_interactors: Arc<dyn SignInteractors>
}

But maybe we should merge them into one, a a UseFactorSourcesInteractors which can either be used for "purpose" Sign or KeysDerivation.

Booting SargonOS now has a new ctor (non test, for test we have many new):

    pub async fn boot_with_interactors(
        clients: Clients,
        interactors: Interactors,
    ) -> Arc<Self> {

and the existing boot calls it and creates a default Interactors using NoUIInteractorForKeyDerivation::new(secure_storage):

    pub async fn boot(bios: Arc<Bios>) -> Arc<Self> {
        let clients = Clients::new(bios);
        let secure_storage = Arc::new(clients.secure_storage.clone());
        let derivation_interactors: Arc<dyn KeysDerivationInteractors>;
        #[cfg(test)]
        {
            derivation_interactors = Arc::new(
                TestDerivationInteractors::with_secure_storage(secure_storage),
            );
        }
        #[cfg(not(test))]
        {
            derivation_interactors =
                Arc::new(NoUIInteractorForKeyDerivation::new(secure_storage));
        }

        let interactors = Interactors::new(derivation_interactors);
        Self::boot_with_interactors(clients, interactors).await
    }

I've not updated the sargon-uniffi crate though, so currently SargonOS will be using NoUIInteractorForKeyDerivation::new(secure_storage). But I dont think any of the hosts are using SargonOS method to create accounts or add FactorSources.

Future directions

We need to decide if we wanna merge the KeyDerivationInteractors and SignInteractors and we need production ready implementation in hosts.

With this PR merged and with production ready interactors, all the fundamental pieces for securifying entities are there! Exciting! What remains is changing the __OFFLINE_ONLY_securify_* code here to also building the (batch of) manifest(s) for entity(ies) and ask host to present them to user for review, sign them and (batch) submit them (with a queue) to the network.

And we also need to update the fulfulling_matrix_of_factor_sources_with_instances function to enforce Matts rules for which FactorInstances can be used in each role, according the rules doc.

[codecov]

Codecov Report

Attention: Patch coverage is 95.30271% with 90 lines in your changes missing coverage. Please review.

Project coverage is 93.1%. Comparing base (9967cf2) to head (aebcb04).
Report is 1 commits behind head on main.

Files with missing lines	Patch %	Lines
...ces_provider/provider/factor_instances_provider.rs	92.4%	10 Missing ⚠️
...eSystem/FileSystemDriver+Data+ContentsOf+URL.swift	76.3%	9 Missing ⚠️
...gon/src/keys_collector/collector/keys_collector.rs	88.0%	9 Missing ⚠️
...r/factor_instances_cache/factor_instances_cache.rs	96.6%	4 Missing ⚠️
...pters/virtual_entity_creating_instance_provider.rs	89.7%	4 Missing ⚠️
..._assigner/next_derivation_entity_index_assigner.rs	85.7%	3 Missing ⚠️
...actor_instance_level/matrix_of_factor_instances.rs	94.2%	3 Missing ⚠️
...c/profile/v100/networks/network/profile_network.rs	81.2%	3 Missing ⚠️
crates/sargon/src/core/types/collections/just.rs	50.0%	2 Missing ⚠️
...ializable_cache/factor_instances_cache_snapshot.rs	94.4%	2 Missing ⚠️
... and 25 more

Additional details and impacted files

@@           Coverage Diff           @@
##            main    #254     +/-   ##
=======================================
+ Coverage   84.1%   93.1%   +9.0%     
=======================================
  Files       1241    1068    -173     
  Lines      21630   21429    -201     
  Branches      77      77             
=======================================
+ Hits       18194   19959   +1765     
+ Misses      3422    1456   -1966     
  Partials      14      14             

Flag	Coverage Δ
kotlin	98.0% <ø> (ø)
rust	91.8% <95.6%> (+11.4%)	⬆️
swift	97.6% <76.3%> (-0.3%)	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines	Coverage Δ
crates/sargon/src/core/error/common_error.rs	93.3% <ø> (ø)
...ances_provider/agnostic_paths/derivation_preset.rs	100.0% <100.0%> (ø)
...der/agnostic_paths/quantified_derivation_preset.rs	100.0% <100.0%> (ø)
...provider/factor_instances_cache/keyed_instances.rs	100.0% <100.0%> (ø)
...able_cache/factor_source_id_from_hash_dense_key.rs	100.0% <100.0%> (ø)
...erivation_entity_index_cache_analyzing_assigner.rs	100.0% <100.0%> (ø)
...ivation_entity_index_profile_analyzing_assigner.rs	100.0% <100.0%> (ø)
..._derivation_entity_index_with_ephemeral_offsets.rs	100.0% <100.0%> (ø)
..._index_with_ephemeral_offsets_for_factor_source.rs	100.0% <100.0%> (ø)
...s_provider/provider/instances_in_cache_consumer.rs	100.0% <100.0%> (ø)
... and 182 more

... and 248 files with indirect coverage changes

---- 🚨 Try these New Features:

• Flaky Tests Detection - Detect and resolve failed and flaky tests

[GhenadieVP]

Amazing work!
No comments worth spending time on from my side.

[matiasbzurovski]

couldn't find anything big, which I am not yet sure if it is a good thing or not

anyway, AMAZING job 👏

[matiasbzurovski]

we aren't validating that entities aren't empty but that they all belong to same network

[Sajjon]

Right it is a bit hard to follow, but that function returns None if empty. Here we are unwrapping an Option not a Result.

[matiasbzurovski]

why?

[Sajjon]

Not all users have Stokenet. Not "important" to check if they have stokenet and pre-derive for that network. We can add it in the future if we want ofc.

[micbakos-rdx]

Should we also use RwLock in signatures collector to keep track of the state?

[Sajjon]

Not needed until needed :D

I had to change to RwLock for Rust async reason, Send / Sync and all that.

[micbakos-rdx]

I had the impression this would be hidden for now and we would end up using

self.wrapped.new_wallet(true)

[Sajjon]

I can unbreak the breaking change if we want? Maybe let new_wallet be what it was and add new_wallet_with_pre_derivation(bool) ?

[micbakos-rdx]

I thought that uniffi::new_wallet() will just call self.wrapped.new_wallet(true)

[CyonAlexRDX]

We do not wanna use shouldPreDerive: true for PROD just yet. We wanna try it more first. But we wanna use shouldPreDerive: true for DEBUG build to try it out.

UniFFI does not know about #if DEBUG or not in Swift. So we must surface the ability to control true/false` to consumers. Which is what I've done. So on Android host, just update to what I've done here:
https://github.com/radixdlt/babylon-wallet-ios/pull/1391/files#r1848198515

[micbakos-rdx]

I understand that we use prelude in most of the files. Why in some files we need to explicitly include those mods?

[Sajjon]

this should probably have been crate::prelude actually. But in SOME cases we must use paths, because we have marked some symbol as pub(super) which means that all file that module can access it. So it is not publically re-exported at crate::prelude level.

[micbakos-rdx]

IDE complains about these async F definitions.

[Sajjon]

it does? hmm strange, which IDE, show me error! :)

but if you run cargo check in iterm - all good? Probably your IDE does not recognize the #![feature(async_closure)] in lib.rs: https://github.com/radixdlt/sargon/blob/integrate_keys_collector/crates/sargon/src/lib.rs#L1

[micbakos-rdx]

Thanks, it is rust rover

[micbakos-rdx]

Why do we need this impl?

[Sajjon]

This is essentially a "shortcut"/""typealias"" of traits :) it allows us to constraint some generic U: IsInLocalKeySpace instead of having to write U: HasIndexInLocalKeySpace + IsKeySpaceAware. But since it is only a "shortcut" we do want to consider all types which are HasIndexInLocalKeySpace + IsKeySpaceAware as in fact IsInLocalKeySpace. So:

impl<T: HasIndexInLocalKeySpace + IsKeySpaceAware> IsInLocalKeySpace for T {} gives exactly that! Rust magic! 🦀

[CyonAlexRDX]

This uncovered the bug in Android host - where FactorInstances was shared between Personas and Accounts, which was fixed in radixdlt/babylon-wallet-android#1256