changes after feedback

apple/Tests/TestCases/Profile/Factor/FactorSourceIDFromHashTests.swift

@@ -26,6 +26,5 @@ final class FactorSourceIDFromHashTests: SpecificFactorSourceIDTest<FactorSource
 	func test_spot_check() {
 		let input = SpotCheckInput.software(mnemonicWithPassphrase: .sample)
 		XCTAssertTrue(SUT.sample.spotCheck(input: input))
-		XCTAssertFalse(SUT.sampleOther.spotCheck(input: input))
 	}
 }

apple/Tests/TestCases/Profile/Factor/FactorSourceIDTests.swift

@@ -14,6 +14,5 @@ final class FactorSourceIDTests: FactorSourceIDTest<FactorSourceID> {
 	func test_spot_check() {
 		let input = SpotCheckInput.software(mnemonicWithPassphrase: .sample)
 		XCTAssertTrue(SUT.sample.spotCheck(input: input))
-		XCTAssertFalse(SUT.sampleOther.spotCheck(input: input))
 	}
 }

apple/Tests/TestCases/Profile/Factor/FactorSourceTests.swift

@@ -33,6 +33,5 @@ final class FactorSourceTests: FactorSourceTest<FactorSource> {
 	func test_spot_check() {
 		let input = SpotCheckInput.software(mnemonicWithPassphrase: .sample)
 		XCTAssertTrue(SUT.sample.spotCheck(input: input))
-		XCTAssertFalse(SUT.sampleOther.spotCheck(input: input))
 	}
 }

crates/factors/factors/src/factor_source_id_spot_check.rs

@@ -9,7 +9,7 @@ pub enum SpotCheckInput {
     Ledger { id: Exactly32Bytes },
 
     /// The user retrieved the `FactorSourceIdFromHash` that identified an Arculus card.
-    /// /// Used for the identification of `ArculusCardFactorSource`.
+    /// Used for the identification of `ArculusCardFactorSource`.
     ArculusCard { id: FactorSourceIDFromHash },
 
     /// The user retrieved a `MnemonicWithPassphrase`.
@@ -21,35 +21,42 @@ pub enum SpotCheckInput {
 
 pub trait FactorSourceIDSpotCheck {
     /// Performs a spot check and returns whether the `FactorSourceID` was created with the same input that has been provided.
+    /// Returns `Err` when the input is not valid for the `FactorSourceKind`.
     fn perform_spot_check(&self, input: SpotCheckInput) -> bool;
 }
 
-impl FactorSourceIDSpotCheck for FactorSourceID {
+impl FactorSourceIDSpotCheck for FactorSourceIDFromHash {
     fn perform_spot_check(&self, input: SpotCheckInput) -> bool {
-        let Some(id_from_hash) = self.as_hash().cloned() else {
-            panic!("Address FactorSourceID not supported for spot check")
-        };
-        let kind = id_from_hash.kind;
-        match input {
+        let id_from_hash = *self;
+        let kind = self.kind;
+        match input.clone() {
             SpotCheckInput::Ledger { id } => {
-                if kind != FactorSourceKind::LedgerHQHardwareWallet {
-                    return false;
-                }
+                assert_eq!(
+                    kind,
+                    FactorSourceKind::LedgerHQHardwareWallet,
+                    "Unexpected Ledger input for kind: {:?}",
+                    kind
+                );
                 let built_id = FactorSourceIDFromHash::new(kind, id);
                 built_id == id_from_hash
             }
             SpotCheckInput::ArculusCard { id } => {
-                if kind != FactorSourceKind::ArculusCard {
-                    return false;
-                }
+                assert_eq!(
+                    kind,
+                    FactorSourceKind::ArculusCard,
+                    "Unexpected ArculusCard input for kind: {:?}",
+                    kind
+                );
                 id == id_from_hash
             }
             SpotCheckInput::Software {
                 mnemonic_with_passphrase,
             } => {
-                if !kind.expects_software_spot_check_input() {
-                    return false;
-                }
+                assert!(
+                    kind.expects_software_spot_check_input(),
+                    "Unexpected Software input for kind: {:?}",
+                    kind
+                );
                 let built_id =
                     FactorSourceIDFromHash::from_mnemonic_with_passphrase(
                         kind,
@@ -61,15 +68,20 @@ impl FactorSourceIDSpotCheck for FactorSourceID {
     }
 }
 
-impl FactorSourceIDSpotCheck for FactorSource {
+impl FactorSourceIDSpotCheck for FactorSourceID {
     fn perform_spot_check(&self, input: SpotCheckInput) -> bool {
-        self.id().perform_spot_check(input)
+        match self {
+            FactorSourceID::Hash { value } => value.perform_spot_check(input),
+            FactorSourceID::Address { .. } => {
+                panic!("Address FactorSourceID does not support spot check")
+            }
+        }
     }
 }
 
-impl FactorSourceIDSpotCheck for FactorSourceIDFromHash {
+impl FactorSourceIDSpotCheck for FactorSource {
     fn perform_spot_check(&self, input: SpotCheckInput) -> bool {
-        FactorSourceID::from(*self).perform_spot_check(input)
+        self.id().perform_spot_check(input)
     }
 }
 
@@ -118,13 +130,13 @@ mod tests {
     }
 
     #[test]
+    #[should_panic(expected = "Unexpected Ledger input for kind: Device")]
     fn spot_check__device__wrong_input() {
         let sut = SUT::sample_device();
         let input = SpotCheckInput::Ledger {
             id: Exactly32Bytes::sample(),
         };
-        let result = sut.perform_spot_check(input);
-        assert!(!result);
+        let _ = sut.perform_spot_check(input);
     }
 
     #[test]
@@ -154,13 +166,15 @@ mod tests {
     }
 
     #[test]
+    #[should_panic(
+        expected = "Unexpected ArculusCard input for kind: LedgerHQHardwareWallet"
+    )]
     fn spot_check__ledger__wrong_input() {
         let sut = SUT::sample_ledger();
         let input = SpotCheckInput::ArculusCard {
             id: FactorSourceIDFromHash::sample(),
         };
-        let result = sut.perform_spot_check(input);
-        assert!(!result);
+        let _ = sut.perform_spot_check(input);
     }
 
     #[test]
@@ -182,15 +196,14 @@ mod tests {
     }
 
     #[test]
+    #[should_panic(
+        expected = "Unexpected Software input for kind: ArculusCard"
+    )]
     fn spot_check__arculus__wrong_input() {
         let input = SpotCheckInput::Software {
             mnemonic_with_passphrase: MnemonicWithPassphrase::sample(),
         };
-        let result = SUT::sample_arculus().perform_spot_check(input.clone());
-        assert!(!result);
-
-        let result = SUT::sample_ledger().perform_spot_check(input.clone());
-        assert!(!result);
+        let _ = SUT::sample_arculus().perform_spot_check(input.clone());
     }
 
     #[test]

crates/factors/factors/src/factor_sources/ledger_hardware_wallet_factor_source/ledger_hardware_wallet_factor_source.rs

@@ -26,6 +26,10 @@ pub struct LedgerHardwareWalletFactorSource {
     pub hint: LedgerHardwareWalletHint,
 }
 
+/// # Safety
+/// Rust memory safe, but marked "unsafe" since this ctor is only used for tests (and shouldn't be
+/// used by production code). A real Ledger device won't be initialized with a `MnemonicWithPassphrase`,
+/// but with `Exactly32Bytes` detailing the device's ID.
 unsafe fn new_ledger_with_mwp(
     mwp: MnemonicWithPassphrase,
     hint: LedgerHardwareWalletHint,

crates/system/interactors/src/spot_check_interactor.rs

@@ -15,8 +15,10 @@ pub trait SpotCheckInteractor: Send + Sync {
 
 /// 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,
-/// skips it or aborts the interaction.
+/// Note that there isn't a failure case since user never fails a spot check. It either:
+/// - succeeds (`Valid` returned by host),
+/// - skips (`Skipped` returned by host),
+/// - aborts (`CommonError::HostInteractionAborted` thrown by host)
 #[derive(Clone, Debug, PartialEq, Eq, std::hash::Hash)]
 pub enum SpotCheckResponse {
     /// The factor source was successfully validated.

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

@@ -12,17 +12,17 @@ impl SpotCheckInteractor for TestSpotCheckInteractor {
         _allow_skip: bool,
     ) -> Result<SpotCheckResponse> {
         match self.user.clone() {
-            SpotCheckUser::Failure(common_error) => Err(common_error),
-            SpotCheckUser::Valid => Ok(SpotCheckResponse::Valid),
+            SpotCheckUser::Failed(common_error) => Err(common_error),
+            SpotCheckUser::Succeeded => Ok(SpotCheckResponse::Valid),
             SpotCheckUser::Skipped => Ok(SpotCheckResponse::Skipped),
         }
     }
 }
 
 #[derive(Clone)]
 pub enum SpotCheckUser {
-    Failure(CommonError),
-    Valid,
+    Failed(CommonError),
+    Succeeded,
     Skipped,
 }
 
@@ -31,12 +31,12 @@ impl TestSpotCheckInteractor {
         Self { user }
     }
 
-    pub fn new_failing(common_error: CommonError) -> Self {
-        Self::new(SpotCheckUser::Failure(common_error))
+    pub fn new_failed(common_error: CommonError) -> Self {
+        Self::new(SpotCheckUser::Failed(common_error))
     }
 
-    pub fn new_valid() -> Self {
-        Self::new(SpotCheckUser::Valid)
+    pub fn new_succeeded() -> Self {
+        Self::new(SpotCheckUser::Succeeded)
     }
 
     pub fn new_skipped() -> Self {

crates/system/os/os/src/sargon_os_factors.rs

@@ -1082,7 +1082,7 @@ mod tests {
         let interactors =
             Interactors::new_from_clients_and_spot_check_interactor(
                 &clients,
-                Arc::new(TestSpotCheckInteractor::new_valid()),
+                Arc::new(TestSpotCheckInteractor::new_succeeded()),
             );
         let os = timeout(
             SARGON_OS_TEST_MAX_ASYNC_DURATION,
@@ -1133,7 +1133,7 @@ mod tests {
         let interactors =
             Interactors::new_from_clients_and_spot_check_interactor(
                 &clients,
-                Arc::new(TestSpotCheckInteractor::new_failing(error.clone())),
+                Arc::new(TestSpotCheckInteractor::new_failed(error.clone())),
             );
         let os = timeout(
             SARGON_OS_TEST_MAX_ASYNC_DURATION,

crates/system/os/os/src/testing_interactors.rs

@@ -59,7 +59,7 @@ impl InteractorsCtors for Interactors {
         Self::new(
             Arc::new(use_factor_sources_interactors),
             Arc::new(TestAuthorizationInteractor::stubborn_authorizing()),
-            Arc::new(TestSpotCheckInteractor::new_valid()),
+            Arc::new(TestSpotCheckInteractor::new_succeeded()),
         )
     }
 
@@ -84,7 +84,7 @@ impl InteractorsCtors for Interactors {
         Self::new(
             Arc::new(use_factor_sources_interactors),
             authorization_interactor,
-            Arc::new(TestSpotCheckInteractor::new_valid()),
+            Arc::new(TestSpotCheckInteractor::new_succeeded()),
         )
     }
 

crates/system/os/signing/src/sargon_os_signing.rs

@@ -464,7 +464,7 @@ mod test {
         let interactors = Interactors::new(
             use_factor_sources_interactors,
             Arc::new(TestAuthorizationInteractor::stubborn_authorizing()),
-            Arc::new(TestSpotCheckInteractor::new_valid()),
+            Arc::new(TestSpotCheckInteractor::new_succeeded()),
         );
         SUT::boot_with_clients_and_interactor(clients, interactors).await
     }

crates/uniffi/uniffi_SPLIT_ME/src/types/spot_check.rs

@@ -3,8 +3,10 @@ use sargon::SpotCheckResponse as InternalSpotCheckResponse;
 
 /// 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,
-/// skips it or aborts the interaction.
+/// Note that there isn't a failure case since user never fails a spot check. It either:
+/// - succeeds (`Valid` returned by host),
+/// - skips (`Skipped` returned by host),
+/// - aborts (`CommonError::HostInteractionAborted` thrown by host)
 #[derive(Clone, PartialEq, Eq, InternalConversion, uniffi::Enum)]
 pub enum SpotCheckResponse {
     /// The factor source was successfully validated.