Adding new device factor source #380

sergiupuhalschi-rdx · 2025-02-12T13:44:44Z

Defined a uniffi::Object called DeviceMnemonicBuilder exposing functionality required for the new UI flow for adding a device factor source. Similarly,PasswordMnemonicBuilder and OffDeviceMnemonicBuilder will follow;
Added sargonOS.is_factor_source_already_in_use checking if a factor source is already is use;
Added sargonOS.add_new_factor_source, which performs the required validations, pre-derives and fills the cache with instances, save factor source into profile, and emits corresponding events.

codecov · 2025-02-12T14:13:16Z

Codecov Report

Attention: Patch coverage is 89.93289% with 15 lines in your changes missing coverage. Please review.

Project coverage is 92.56%. Comparing base (4576ccc) to head (7e9c279).
Report is 1 commits behind head on main.

Files with missing lines	Patch %	Lines
...on_os/add_factor_source/device_mnemonic_builder.rs	0.00%	6 Missing ⚠️
crates/factors/factors/src/factor_source.rs	84.00%	4 Missing ⚠️
.../uniffi_SPLIT_ME/src/core/utils/builder_arc_map.rs	0.00%	3 Missing ⚠️
...add_factor_source/sargon_os_factor_source_adder.rs	94.59%	2 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #380      +/-   ##
==========================================
- Coverage   92.56%   92.56%   -0.01%     
==========================================
  Files        1227     1230       +3     
  Lines       28348    28495     +147     
  Branches       85       85              
==========================================
+ Hits        26239    26375     +136     
- Misses       2098     2109      +11     
  Partials       11       11

Flag	Coverage Δ
kotlin	`98.57% <ø> (ø)`
rust	`92.20% <89.93%> (+<0.01%)`	⬆️
swift	`92.67% <ø> (ø)`

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

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

crates/system/os/factors/src/add_factor/device_factor_adding_manager.rs

CyonAlexRDX · 2025-02-13T07:17:17Z

crates/system/os/factors/src/add_factor/device_factor_adding_manager.rs

+        let mnemonic_with_passphrase =
+            MnemonicWithPassphrase::new(mnemonic.clone());
+        let factor_source = DeviceFactorSource::babylon(
+            false,


Can you decl_bool_type for this bool? Is it if it is Main?

This cascades into many changes and I'm not sure it's worth it, WDYT?

CyonAlexRDX · 2025-02-13T07:19:28Z

crates/system/os/factors/src/add_factor/device_factor_adding_manager.rs

+        let mut csprng = OsRng;
+        let mut confirmation_indices: HashSet<u8> = HashSet::new();
+
+        while (confirmation_indices.len() as u8)


Instead of this you can use:

generate_bytes::<3>().into_iter().map(|b| b as usize).collect::<IndexSet<_>>()

And done.

I think this will uncover the case when the same value is generated, with the while we're sure we'll end up with 3 distinct values.

right, so lets do generate_bytes::<100>().into_iter().map(|b| b as usize).take(3).collect::<IndexSet<_>>() that will statistically never fail.

and when #380 (comment)

it becomes:

generate_bytes::<100>() .into_iter() .map(|b| b as usize) .take(Self::NUMBER_OF_WORDS_OF_MNEMONIC_USER_NEED_TO_CONFIRM_EXCL_CHECKSUM) .collect::<IndexSet<_>>()

I'm not sure this is a good idea anymore, because I got the test failing a few times, then I increased the value from 100 to 1000 and it seemed fine for a while, but just now it failed again. @CyonAlexRDX Do you agree to revert to the previous code to avoid the tests occasionally failing? Or should we increase the value again? Not sure what's better.

Improved this by taking only the first N unique values from the randomly generated ones, so now statistically it shouldn't ever fail.

crates/system/os/factors/src/add_factor/device_factor_adding_manager.rs

crates/uniffi/uniffi_SPLIT_ME/src/system/sargon_os/mod.rs

crates/uniffi/uniffi_SPLIT_ME/src/system/sargon_os/add_factor/device_factor_adding_manager.rs

crates/system/os/factors/src/add_factor_source/device_factor_source_adding_manager.rs

jvm/sargon-android/src/test/java/com/radixdlt/sargon/os/factors/AddNewFactorManagerTest.kt

CyonAlexRDX · 2025-02-14T11:37:42Z

crates/system/os/factors/src/add_factor_source/sargon_os_factor_source_adder.rs

+        .await?;
+
+        if let Err(e) = self
+            .pre_derive_and_fill_cache_with_instances_for_factor_source(


hmm would be nice with .inspect_err on the Result from pre_derive_and_fill_cache_with_instances_for_factor_source to avoid the if let Err but I think maybe tricky due to async...?

It would have been nice, but I didn't manage to make it work. Besides await is only allowed inside async functions and blocks error there is also the fact that the operation we perform in case of error returns a Result which we want to propagate.

crates/factors/factors/src/factor_source.rs

Sajjon · 2025-02-19T08:17:21Z

crates/uniffi/uniffi_SPLIT_ME/src/system/sargon_os/add_factor_source/device_mnemonic_builder.rs

+#[uniffi::export]
+impl DeviceMnemonicBuilder {
+    /// Generates a new mnemonic
+    pub fn create_new_mnemonic_with_passphrase(self: Arc<Self>) -> Arc<Self> {


I think this should be named:

generate_new not create_new I think.

What about create_mnemonic_with_passphrase_from_words? Should it be generate_mnemonic_with_passphrase_from_words for consistency?

Generate if generating random entropy. Create If using mnemonic words

So your last commit looks good

Sajjon · 2025-02-19T14:19:30Z

crates/uniffi/uniffi_SPLIT_ME/src/system/sargon_os/add_factor_source/device_mnemonic_builder.rs

+
+/// The result of the `build` function from `DeviceMnemonicBuilder`.
+#[derive(Debug, PartialEq, uniffi::Enum)]
+pub enum DeviceMnemonicBuildResult {


I would call this DeviceMnemonicBuildOutcome - we try to always let **Result be a Result type.

CyonAlexRDX · 2025-02-20T09:58:33Z

crates/uniffi/uniffi_SPLIT_ME/src/system/sargon_os/add_factor_source/device_mnemonic_builder.rs

+#[uniffi::export]
+impl DeviceMnemonicBuilder {
+    /// Generates a new mnemonic
+    pub fn generate_new_mnemonic_with_passphrase(self: Arc<Self>) -> Arc<Self> {


maybe we should drop the _with_passphrase here and in create_mnemonic_with_passphrase_from_words since we do not in fact support BIP39Passphrases to be used.

CyonAlexRDX · 2025-02-20T10:00:30Z

crates/uniffi/uniffi_SPLIT_ME/src/system/sargon_os/add_factor_source/device_mnemonic_builder.rs

+
+        let indices = sut.clone().get_indices_in_mnemonic_of_words_to_confirm();
+
+        let r0 = sut.clone().build(


r stands for? ah result? lets call it let outcome. And Rust supports shadowing! So no need for outcome1 you can just let outcome = // second and again and again. since it looks like you dont need r0 and r1?

CyonAlexRDX

LGTM, left some last minor remarks. Good job!

account-for-display@1.2.2 addresses@1.2.2 assert-json@1.2.2 build-info@1.2.2 bytes@1.2.2 cap26-models@1.2.2 clients@1.2.2 core-collections@1.2.2 core-misc@1.2.2 core-utils@1.2.2 drivers@1.2.2 ecc@1.2.2 encryption@1.2.2 entity-by-address@1.2.2 entity-foundation@1.2.2 error@1.2.2 factor-instances-provider@1.2.2 factors@1.2.2 factors-supporting-types@1.2.2 gateway-client-and-api@1.2.2 gateway-models@1.2.2 has-sample-values@1.2.2 hash@1.2.2 hierarchical-deterministic@1.2.2 home-cards@1.2.2 host-info@1.2.2 http-client@1.2.2 identified-vec-of@1.2.2 interactors@1.2.2 key-derivation-traits@1.2.2 keys-collector@1.2.2 manifests@1.2.2 metadata@1.2.2 network@1.2.2 next-derivation-index-ephemeral@1.2.2 numeric@1.2.2 prelude@1.2.2 profile@1.2.2 profile-account@1.2.2 profile-account-or-persona@1.2.2 profile-app-preferences@1.2.2 profile-base-entity@1.2.2 profile-gateway@1.2.2 profile-logic@1.2.2 profile-persona@1.2.2 profile-persona-data@1.2.2 profile-security-structures@1.2.2 profile-state-holder@1.2.2 profile-supporting-types@1.2.2 radix-connect@1.2.2 radix-connect-models@1.2.2 sargon@1.2.2 sargon-os@1.2.2 sargon-os-accounts@1.2.2 sargon-os-derive-public-keys@1.2.2 sargon-os-factors@1.2.2 sargon-os-security-center@1.2.2 sargon-os-signing@1.2.2 sargon-os-transaction@1.2.2 sargon-uniffi@1.2.2 sargon-uniffi-conversion-macros@1.2.2 security-center@1.2.2 short-string@1.2.2 signatures-collector@1.2.2 signing-traits@1.2.2 sub-systems@1.2.2 time-utils@1.2.2 transaction-foundation@1.2.2 transaction-models@1.2.2 Generated by cargo-workspaces

GhenadieVP · 2025-02-19T06:58:04Z

crates/factors/factors/src/factor_source.rs

@@ -155,6 +156,63 @@ impl FactorSource {
    }
 }

+impl FactorSource {
+    pub fn with_details(


This function is mostly constrained by mnemonic_with_passphrase, so I would add an its name the mention of MWP, to know this applies only to the kinds of factor sources which have their id derived from MWP.

GhenadieVP · 2025-02-20T10:35:02Z

crates/system/os/factors/src/add_factor_source/device_mnemonic_builder.rs

+/// Exposes functions to be called by hosts to be able to use the resulting `MnemonicWithPassphrase`.
+#[derive(Debug)]
+pub struct DeviceMnemonicBuilder {
+    mnemonic_with_passphrase: RwLock<Option<MnemonicWithPassphrase>>,


Do we actually need the RwLock here? Is it expected to have many concurrent readers/writers

I don't think so, at least not according to the current requirements, but I think this is a good thing to have in place for the values accessed/modified by methods called from hosts directly. Do you think we should remove the lock?

Well it is no big impact having it here, it just adds additional complexity for no apparent need. It creates a sort of wrong expectation, that the state really needs to be protected. I would remove it if possible, unless it is really needed.

GhenadieVP · 2025-02-20T10:47:48Z

crates/system/os/factors/src/add_factor_source/sargon_os_factor_source_adder.rs

+    ///
+    /// And also emits `Event::ProfileSaved` after having successfully written the JSON
+    /// of the active profile to secure storage.
+    async fn add_new_factor_source(


The name should probably me more specific to mnemonic_with_passphrase

GhenadieVP · 2025-02-20T10:49:08Z

crates/system/os/factors/src/add_factor_source/sargon_os_factor_source_adder.rs

+        if factor_source_kind == FactorSourceKind::Device {
+            self.secure_storage
+                .save_mnemonic_with_passphrase(
+                    &mnemonic_with_passphrase,
+                    &factor_source.id_from_hash(),
+                )
+                .await?;
+        }


If this fails, the profile saved factor source should be reverted

Good point. Or we could update the profile after saving the mnemonic to storage.

GhenadieVP · 2025-02-20T10:52:25Z

crates/system/os/factors/src/add_factor_source/sargon_os_factor_source_adder.rs

+            self.secure_storage
+                .delete_mnemonic(&factor_source.id_from_hash())
+                .await?;


What if the kind is not device?

GhenadieVP · 2025-02-20T10:57:55Z

crates/system/os/factors/src/add_factor_source/sargon_os_factor_source_adder.rs

+        let result = os
+            .with_timeout(|x| {
+                x.add_new_factor_source(
+                    FactorSourceKind::Device,


Do add a test with another kind than Device.

GhenadieVP · 2025-02-20T11:03:10Z

crates/system/os/factors/src/add_factor_source/device_mnemonic_builder.rs

+/// Exposes functions to be called by hosts to be able to use the resulting `MnemonicWithPassphrase`.
+#[derive(Debug)]
+pub struct DeviceMnemonicBuilder {
+    mnemonic_with_passphrase: RwLock<Option<MnemonicWithPassphrase>>,


Well it is no big impact having it here, it just adds additional complexity for no apparent need. It creates a sort of wrong expectation, that the state really needs to be protected. I would remove it if possible, unless it is really needed.

GhenadieVP · 2025-02-20T13:24:01Z

crates/system/os/factors/src/add_factor_source/sargon_os_factor_source_adder.rs

+        self.update_profile_with(|p| {
+            p.factor_sources.append(factor_source.clone());
+            Ok(())
+        })
+        .await?;


Same thing applies here - if profile update failed, the previously added device factor source should be removed from the secure storage.
Consider using a block to group together this and self.pre_derive_and_fill_cache_with_instances_for_factor_source, something like:

let setup_result = { self.update_profile_with(|p| { p.factor_sources.append(factor_source.clone()); Ok(()) }) .await?; self .pre_derive_and_fill_cache_with_instances_for_factor_source( factor_source.clone(), ) .await }; if let Err(e) = setup_result { // revert logic on error }

sergiupuhalschi-rdx added 6 commits February 7, 2025 12:22

Initial setup new factor adding builder

acce421

WIP

a79d8cd

Uniffi export

fb37f0b

WIP

8661ecb

WIP

3e5616d

WIP

1f0097b

sergiupuhalschi-rdx self-assigned this Feb 12, 2025

Merge branch 'main' into sp/add-new-factor-builder

73b1483

sergiupuhalschi-rdx changed the title ~~Adding new factor source~~ Adding new device factor source Feb 12, 2025

sergiupuhalschi-rdx changed the title ~~Adding new device factor source~~ WIP - Adding new device factor source Feb 12, 2025

CyonAlexRDX reviewed Feb 13, 2025

View reviewed changes

PR comments

32a32f1

GhenadieVP reviewed Feb 13, 2025

View reviewed changes

crates/system/os/factors/src/add_factor/device_factor_adding_manager.rs Outdated Show resolved Hide resolved

CyonAlexRDX reviewed Feb 13, 2025

View reviewed changes

crates/uniffi/uniffi_SPLIT_ME/src/system/sargon_os/mod.rs Outdated Show resolved Hide resolved

CyonAlexRDX reviewed Feb 13, 2025

View reviewed changes

crates/uniffi/uniffi_SPLIT_ME/src/system/sargon_os/add_factor/device_factor_adding_manager.rs Outdated Show resolved Hide resolved

CyonAlexRDX reviewed Feb 13, 2025

View reviewed changes

crates/uniffi/uniffi_SPLIT_ME/src/system/sargon_os/add_factor/device_factor_adding_manager.rs Outdated Show resolved Hide resolved

CyonAlexRDX reviewed Feb 13, 2025

View reviewed changes

crates/uniffi/uniffi_SPLIT_ME/src/system/sargon_os/add_factor/device_factor_adding_manager.rs Outdated Show resolved Hide resolved

sergiupuhalschi-rdx added 3 commits February 13, 2025 17:45

Address comments

dab1d1c

Merge branch 'main' into sp/add-new-factor-builder

7d76099

WIP

284abe8

CyonAlexRDX reviewed Feb 14, 2025

View reviewed changes

jvm/sargon-android/src/test/java/com/radixdlt/sargon/os/factors/AddNewFactorManagerTest.kt Outdated Show resolved Hide resolved

CyonAlexRDX reviewed Feb 14, 2025

View reviewed changes

sergiupuhalschi-rdx added 3 commits February 14, 2025 18:52

WIP

93bb0ec

WIP

c59b561

Merge main

4a9113b

Sajjon reviewed Feb 17, 2025

View reviewed changes

crates/factors/factors/src/factor_source.rs Outdated Show resolved Hide resolved

WIP

aa0e36a

WIP

6e92c10

sergiupuhalschi-rdx changed the title ~~WIP - Adding new device factor source~~ Adding new device factor source Feb 17, 2025

sergiupuhalschi-rdx added 5 commits February 17, 2025 17:36

Add tests

ea6d363

Add tests

7efc3b8

Add test

5d12b26

Fix fn cfg

3edd3e1

Fix typo

a3232f6

sergiupuhalschi-rdx marked this pull request as ready for review February 18, 2025 08:44

Expose function

14fab67

Sajjon reviewed Feb 19, 2025

View reviewed changes

sergiupuhalschi-rdx added 3 commits February 19, 2025 10:35

PR review

32cc180

Expose error

c1272b4

Improve random indices generating

e6af3bd

Sajjon reviewed Feb 19, 2025

View reviewed changes

PR review

cdf3635

CyonAlexRDX reviewed Feb 20, 2025

View reviewed changes

CyonAlexRDX approved these changes Feb 20, 2025

View reviewed changes

sergiupuhalschi-rdx added 2 commits February 20, 2025 12:19

PR review

55cc10e

GhenadieVP reviewed Feb 20, 2025

View reviewed changes

GhenadieVP requested changes Feb 20, 2025

View reviewed changes

Review comments

9ad682b

GhenadieVP reviewed Feb 20, 2025

View reviewed changes

sergiupuhalschi-rdx added 3 commits February 20, 2025 17:20

Review comments

5ca9092

Improve API

9df9ba4

Improve API

7e9c279

GhenadieVP approved these changes Feb 21, 2025

View reviewed changes

sergiupuhalschi-rdx merged commit 3e4786f into main Feb 21, 2025
12 of 13 checks passed

sergiupuhalschi-rdx deleted the sp/add-new-factor-builder branch February 21, 2025 09:31

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adding new device factor source #380

Adding new device factor source #380

sergiupuhalschi-rdx commented Feb 12, 2025 •

edited

Loading

codecov bot commented Feb 12, 2025 •

edited

Loading

CyonAlexRDX Feb 13, 2025

sergiupuhalschi-rdx Feb 17, 2025

CyonAlexRDX Feb 13, 2025

sergiupuhalschi-rdx Feb 13, 2025

CyonAlexRDX Feb 13, 2025

CyonAlexRDX Feb 13, 2025

sergiupuhalschi-rdx Feb 17, 2025 •

edited

Loading

sergiupuhalschi-rdx Feb 20, 2025

CyonAlexRDX Feb 14, 2025

sergiupuhalschi-rdx Feb 17, 2025

Sajjon Feb 19, 2025

sergiupuhalschi-rdx Feb 19, 2025

CyonAlexRDX Feb 19, 2025

CyonAlexRDX Feb 19, 2025

Sajjon Feb 19, 2025

CyonAlexRDX Feb 20, 2025

CyonAlexRDX Feb 20, 2025

CyonAlexRDX left a comment

GhenadieVP Feb 19, 2025

GhenadieVP Feb 20, 2025

sergiupuhalschi-rdx Feb 20, 2025

GhenadieVP Feb 20, 2025 •

edited

Loading

GhenadieVP Feb 20, 2025

GhenadieVP Feb 20, 2025

sergiupuhalschi-rdx Feb 20, 2025

GhenadieVP Feb 20, 2025

GhenadieVP Feb 20, 2025

GhenadieVP Feb 20, 2025 •

edited

Loading

GhenadieVP Feb 20, 2025 •

edited

Loading


		let indices = sut.clone().get_indices_in_mnemonic_of_words_to_confirm();

		let r0 = sut.clone().build(

Adding new device factor source #380

Adding new device factor source #380

Conversation

sergiupuhalschi-rdx commented Feb 12, 2025 • edited Loading

codecov bot commented Feb 12, 2025 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sergiupuhalschi-rdx Feb 17, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CyonAlexRDX left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

GhenadieVP Feb 20, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

GhenadieVP Feb 20, 2025 • edited Loading

Choose a reason for hiding this comment

GhenadieVP Feb 20, 2025 • edited Loading

Choose a reason for hiding this comment

sergiupuhalschi-rdx commented Feb 12, 2025 •

edited

Loading

codecov bot commented Feb 12, 2025 •

edited

Loading

sergiupuhalschi-rdx Feb 17, 2025 •

edited

Loading

GhenadieVP Feb 20, 2025 •

edited

Loading

GhenadieVP Feb 20, 2025 •

edited

Loading

GhenadieVP Feb 20, 2025 •

edited

Loading