[ABW-4051] API to perform Spot Check #372

matiasbzurovski · 2025-02-05T15:25:05Z

Changelog

Adds a new fn perform_spot_check(&self, input: SpotCheckInput) -> bool implemented by FactorSourceIDFromHash, FactorSource & FactorSourceID. This determines whether the associated id was created with the same input than provided.
Adds a new function on SargonOS to trigger a Factor Source spot check. This will use a new host interactor that will be responsible for presenting the corresponding UI for user to perform the check. To do so, it will use the function described on previous point.

codecov · 2025-02-05T15:36:52Z

Codecov Report

Attention: Patch coverage is 87.00000% with 13 lines in your changes missing coverage. Please review.

Project coverage is 92.66%. Comparing base (a18623a) to head (6c355b1).
Report is 1 commits behind head on main.

Files with missing lines	Patch %	Lines
...Profile/Factor/FactorSourceID+Wrap+Functions.swift	33.33%	6 Missing ⚠️
...argonOS/ThrowingHostInteractor+Static+Shared.swift	0.00%	3 Missing ⚠️
...factors/factors/src/factor_source_id_spot_check.rs	96.87%	1 Missing ⚠️
...ava/com/radixdlt/sargon/extensions/FactorSource.kt	0.00%	1 Missing ⚠️
...a/com/radixdlt/sargon/extensions/FactorSourceId.kt	50.00%	1 Missing ⚠️
...dixdlt/sargon/extensions/FactorSourceIdFromHash.kt	0.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #372      +/-   ##
==========================================
- Coverage   92.68%   92.66%   -0.03%     
==========================================
  Files        1184     1187       +3     
  Lines       26689    26781      +92     
  Branches       85       85              
==========================================
+ Hits        24737    24816      +79     
- Misses       1941     1954      +13     
  Partials       11       11

Flag	Coverage Δ
kotlin	`98.64% <25.00%> (-0.23%)`	⬇️
rust	`92.29% <98.66%> (+0.01%)`	⬆️
swift	`92.66% <57.14%> (-0.22%)`	⬇️

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.

GhenadieVP · 2025-02-06T09:22:07Z

crates/system/interactors/src/spot_check_interactor.rs

+pub enum SpotCheckResponse {
+    /// The user retrieved the id of a Ledger device.
+    /// Used for the identification of `LedgerHardwareWalletFactorSource`.
+    Ledger { id: Exactly32Bytes },


Arculus can be added with FactorSourceIdFromHash

GhenadieVP · 2025-02-06T09:24:10Z

crates/system/interactors/src/spot_check_interactor.rs

+pub trait SpotCheckInteractor: Send + Sync {
+    async fn spot_check(
+        &self,
+        factor_source_id: FactorSourceIDFromHash,


Any reason to not use FactorSource? I imagine that Hosts will have to load the factor source again using the id so we display the information to the user, no?

matiasbzurovski · 2025-02-07T14:15:10Z

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

+        factor_source: FactorSource,
+    ) -> Result<bool> {
+        self.wrapped
+            .trigger_spot_check(factor_source.into_internal(), false)


When the spot check is triggered by the hosts, it will never allow skip

Sajjon · 2025-02-07T14:48:50Z

crates/factors/factors/src/factor_source_id_spot_check.rs

+}
+
+impl FactorSourceKind {
+    fn is_software(&self) -> bool {


Iwould like a doc here. this is to say that derivation takes place in Radix controlled software? (sargon / hosts)?

because it is not about STORAGE

I will add some docs and rename to something more descriptive, but this is just to validate that the FactorSourceKind is one with which we should use SpotCheckInput::Software to validate its id.

I am actually wondering if these checks (lines 35, 42 & 50) are actually worthy.. because the FactorSourceIDFromHash built when using incorrect combination of SpotCheckInput & FactorSourceKind would actually result in a different FactorSourceIDFromHash than the original one (unless the original one was incorrectly built)

crates/factors/factors/src/factor_source_id_spot_check.rs

GhenadieVP · 2025-02-10T12:34:13Z

crates/factors/factors/src/factor_source_id_spot_check.rs

+                if kind != FactorSourceKind::LedgerHQHardwareWallet {
+                    return false;
+                }


It would be good to have an error of sort to identify this scenario, as it would be a programmer error. Maybe a CommonError would be useful.

GhenadieVP · 2025-02-10T12:39:06Z

crates/factors/factors/src/factor_source_id_spot_check.rs

+
+impl FactorSourceIDSpotCheck for FactorSourceIDFromHash {
+    fn perform_spot_check(&self, input: SpotCheckInput) -> bool {
+        FactorSourceID::from(*self).perform_spot_check(input)


Maybe instead move the whole code starting from line 32 here? since that is specific to FactorSourceIDFromHash.

GhenadieVP · 2025-02-10T12:40:40Z

crates/factors/factors/src/factor_source_id_spot_check.rs

+            | FactorSourceKind::SecurityQuestions => true,
+            FactorSourceKind::LedgerHQHardwareWallet
+            | FactorSourceKind::ArculusCard
+            | FactorSourceKind::TrustedContact => false,


This should be software I think.

FactorSourceIDFromAddress is not supported for spot check so doesn't make a difference

Sure, but still it is not hardware kind.

notice that the function has been renamed a few commits ago into expects_software_spot_check_input, which for TrustedContact isn't true since it doesn't expect any spot check input, but I can change if you prefer

GhenadieVP · 2025-02-10T12:43:58Z

crates/system/interactors/src/spot_check_interactor.rs

+
+/// An enum indicating the result of a spot check.
+///
+/// Note that there isn't a failure case since user never fails a spot check: it either succeeds,


For clarification - abort will be error thrown from the host side, HostInteractionAborted is the error

updating docs

Sajjon · 2025-02-10T12:58:54Z

.../factor_sources/ledger_hardware_wallet_factor_source/ledger_hardware_wallet_factor_source.rs

@@ -26,7 +26,7 @@ pub struct LedgerHardwareWalletFactorSource {
    pub hint: LedgerHardwareWalletHint,
 }

-fn new_ledger_with_mwp(
+unsafe fn new_ledger_with_mwp(


missing /// # Safety see other places

Sajjon · 2025-02-10T13:00:25Z

crates/system/interactors/src/testing/test_spot_check_interactor.rs

+
+#[derive(Clone)]
+pub enum SpotCheckUser {
+    Failure(CommonError),


Skipped is past tense of a verb, Valid and Failure are not verbs. Pick a part of speech kind and use for all variants? :)

GhenadieVP · 2025-02-10T17:05:45Z

crates/factors/factors/src/factor_source_id_spot_check.rs

+            | FactorSourceKind::SecurityQuestions => true,
+            FactorSourceKind::LedgerHQHardwareWallet
+            | FactorSourceKind::ArculusCard
+            | FactorSourceKind::TrustedContact => false,


Sure, but still it is not hardware kind.

matiasbzurovski requested review from micbakos-rdx, CyonAlexRDX and GhenadieVP February 5, 2025 15:25

CyonAlexRDX approved these changes Feb 5, 2025

View reviewed changes

GhenadieVP reviewed Feb 6, 2025

View reviewed changes

matiasbzurovski marked this pull request as ready for review February 6, 2025 14:17

matiasbzurovski force-pushed the mb/ABW-4051-spot-check branch from 8bb3af2 to 68c671b Compare February 6, 2025 14:18

micbakos-rdx approved these changes Feb 7, 2025

View reviewed changes

matiasbzurovski commented Feb 7, 2025

View reviewed changes

Sajjon reviewed Feb 7, 2025

View reviewed changes

CyonAlexRDX approved these changes Feb 10, 2025

View reviewed changes

GhenadieVP reviewed Feb 10, 2025

View reviewed changes

Sajjon reviewed Feb 10, 2025

View reviewed changes

matiasbzurovski added 8 commits February 10, 2025 16:42

new ctor for MnemonicWithPasswphrase without latter

4f7e03e

WIP

12c7edd

WIP

fa73bb8

send FactorSource to interactor

e1a413a

updates to make it work with Android

eeebdc4

update with new behavior

ba4fa19

rename and add docs

0bf14f3

changes after feedback

309fec0

matiasbzurovski force-pushed the mb/ABW-4051-spot-check branch from 1b645ad to 309fec0 Compare February 10, 2025 15:42

GhenadieVP approved these changes Feb 10, 2025

View reviewed changes

matiasbzurovski merged commit 72766db into main Feb 10, 2025
11 of 13 checks passed

matiasbzurovski deleted the mb/ABW-4051-spot-check branch February 10, 2025 17:42

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[ABW-4051] API to perform Spot Check #372

[ABW-4051] API to perform Spot Check #372

matiasbzurovski commented Feb 5, 2025 •

edited

Loading

codecov bot commented Feb 5, 2025 •

edited

Loading

GhenadieVP Feb 6, 2025

GhenadieVP Feb 6, 2025

matiasbzurovski Feb 7, 2025

Sajjon Feb 7, 2025

matiasbzurovski Feb 7, 2025

GhenadieVP Feb 10, 2025

GhenadieVP Feb 10, 2025

GhenadieVP Feb 10, 2025

matiasbzurovski Feb 10, 2025

GhenadieVP Feb 10, 2025

matiasbzurovski Feb 10, 2025

GhenadieVP Feb 10, 2025

matiasbzurovski Feb 10, 2025

Sajjon Feb 10, 2025

Sajjon Feb 10, 2025

GhenadieVP Feb 10, 2025

[ABW-4051] API to perform Spot Check #372

[ABW-4051] API to perform Spot Check #372

Conversation

matiasbzurovski commented Feb 5, 2025 • edited Loading

Changelog

codecov bot commented Feb 5, 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

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

matiasbzurovski commented Feb 5, 2025 •

edited

Loading

codecov bot commented Feb 5, 2025 •

edited

Loading