Merge pull request #3 from radixdlt/finish_early_strategy

README.md

@@ -2,7 +2,7 @@
 
 
 > [!IMPORTANT]  
-> This TEMPRORARY repo is intended for **internal use only** and will later be merged into [Sargon]https://github.com/radixdlt/sargon.
+> This TEMPORARY repo is intended for **internal use only** and will later be merged into [Sargon]https://github.com/radixdlt/sargon.
 > Interfaces will be changing regularly, and we do not recommend other developers integrate the library or align with these standards.
 
 MFA work.

src/lib.rs

@@ -1,6 +1,7 @@
 #![allow(internal_features)]
 #![feature(core_intrinsics)]
 #![feature(iter_repeat_n)]
+#![feature(async_closure)]
 
 mod derivation;
 mod signing;

src/signing/collector/mod.rs

@@ -1,7 +1,11 @@
+mod signatures_collecting_continuation;
 mod signatures_collector;
 mod signatures_collector_dependencies;
 mod signatures_collector_preprocessor;
 mod signatures_collector_state;
+mod signing_finish_early_strategy;
 
+pub use signatures_collecting_continuation::*;
 pub use signatures_collector::*;
 pub use signatures_collector_preprocessor::*;
+pub use signing_finish_early_strategy::*;

src/signing/collector/signatures_collecting_continuation.rs

@@ -0,0 +1,14 @@
+/// Whether to continue collecting signatures or finish early.
+#[derive(Clone, Copy, Debug, PartialEq, Eq)]
+pub enum SignaturesCollectingContinuation {
+    /// It is meaningless to continue collecting signatures, either since either
+    /// all transactions are valid, and the collector is configured to finish early
+    /// in that case, or some transaction is invalid and the collector is configured
+    /// finish early in that case.
+    FinishEarly,
+
+    /// We should continue collecting signatures, either since the collector is
+    /// configured to not finish early, even though we can, or since we cannot
+    /// finish early since not enough factor sources have been signed with.
+    Continue,
+}

src/signing/collector/signatures_collector.rs

@@ -27,7 +27,7 @@ impl SignaturesCollector {
     /// Used by our tests. But Sargon will typically wanna use `SignaturesCollector::new` and passing
     /// it a
     pub(crate) fn with(
-        finish_early_when_all_transactions_are_valid: bool,
+        finish_early_strategy: SigningFinishEarlyStrategy,
         all_factor_sources_in_profile: IndexSet<HDFactorSource>,
         transactions: IndexSet<TXToSign>,
         interactors: Arc<dyn SignatureCollectingInteractors>,
@@ -36,11 +36,8 @@ impl SignaturesCollector {
         let preprocessor = SignaturesCollectorPreprocessor::new(transactions);
         let (petitions, factors) = preprocessor.preprocess(all_factor_sources_in_profile);
 
-        let dependencies = SignaturesCollectorDependencies::new(
-            finish_early_when_all_transactions_are_valid,
-            interactors,
-            factors,
-        );
+        let dependencies =
+            SignaturesCollectorDependencies::new(finish_early_strategy, interactors, factors);
         let state = SignaturesCollectorState::new(petitions);
 
         Self {
@@ -50,7 +47,7 @@ impl SignaturesCollector {
     }
 
     pub fn with_signers_extraction<F>(
-        finish_early_when_all_transactions_are_valid: bool,
+        finish_early_strategy: SigningFinishEarlyStrategy,
         all_factor_sources_in_profile: IndexSet<HDFactorSource>,
         transactions: IndexSet<TransactionIntent>,
         interactors: Arc<dyn SignatureCollectingInteractors>,
@@ -65,7 +62,7 @@ impl SignaturesCollector {
             .collect::<Result<IndexSet<TXToSign>>>()?;
 
         let collector = Self::with(
-            finish_early_when_all_transactions_are_valid,
+            finish_early_strategy,
             all_factor_sources_in_profile,
             transactions,
             interactors,
@@ -75,13 +72,13 @@ impl SignaturesCollector {
     }
 
     pub fn new(
-        finish_early_when_all_transactions_are_valid: bool,
+        finish_early_strategy: SigningFinishEarlyStrategy,
         transactions: IndexSet<TransactionIntent>,
         interactors: Arc<dyn SignatureCollectingInteractors>,
         profile: &Profile,
     ) -> Result<Self> {
         Self::with_signers_extraction(
-            finish_early_when_all_transactions_are_valid,
+            finish_early_strategy,
             profile.factor_sources.clone(),
             transactions,
             interactors,
@@ -102,18 +99,49 @@ impl SignaturesCollector {
     }
 }
 
+use SignaturesCollectingContinuation::*;
+
 // === PRIVATE ===
 impl SignaturesCollector {
-    /// If all transactions already would fail, or if all transactions already are done, then
-    /// no point in continuing.
+    /// Returning `Continue` means that we should continue collecting signatures.
     ///
-    /// `Ok(true)` means "continue", `Ok(false)` means "stop, we are done". `Err(_)` means "stop, we have failed".
-    fn continue_if_necessary(&self) -> Result<bool> {
-        self.state
-            .borrow()
-            .petitions
-            .borrow()
-            .continue_if_necessary()
+    /// Returning `FinishEarly` if it is meaningless to continue collecting signatures,
+    /// either since all transactions are valid and this collector is configured
+    /// to finish early in that case, or if some transaction is invalid and this
+    /// collector is configured to finish early in that case.
+    ///
+    /// N.B. this method does not concern itself with how many or which
+    /// factor sources are left to sign with, that is handled by the main loop,
+    /// i.e. this might return `false` even though there is not factor sources
+    /// left to sign with.
+    fn continuation(&self) -> SignaturesCollectingContinuation {
+        let finish_early_strategy = self.dependencies.finish_early_strategy;
+        let when_all_transactions_are_valid =
+            finish_early_strategy.when_all_transactions_are_valid.0;
+        let when_some_transaction_is_invalid =
+            finish_early_strategy.when_some_transaction_is_invalid.0;
+
+        let petitions_status = self.state.borrow().petitions.borrow().status();
+
+        if petitions_status.are_all_valid() {
+            if when_all_transactions_are_valid == FinishEarly {
+                info!("All valid && should finish early => finish early");
+                return FinishEarly;
+            } else {
+                debug!(
+                    "All valid, BUT the collector is configured to NOT finish early => Continue"
+                );
+            }
+        } else if petitions_status.is_some_invalid() {
+            if when_some_transaction_is_invalid == FinishEarly {
+                info!("Some invalid && should finish early => finish early");
+                return FinishEarly;
+            } else {
+                debug!("Some transactions invalid, BUT the collector is configured to NOT finish early in case of failures => Continue");
+            }
+        }
+
+        Continue
     }
 
     async fn use_factor_sources(&self, factor_sources_of_kind: &FactorSourcesOfKind) -> Result<()> {
@@ -172,7 +200,7 @@ impl SignaturesCollector {
                     // Report the results back to the collector
                     self.process_batch_response(response);
 
-                    if !self.continue_if_necessary()? {
+                    if self.continuation() == FinishEarly {
                         break;
                     }
                 }
@@ -195,13 +223,8 @@ impl SignaturesCollector {
     async fn sign_with_factors(&self) -> Result<()> {
         let factors_of_kind = self.dependencies.factors_of_kind.clone();
         for factor_sources_of_kind in factors_of_kind.into_iter() {
-            if self
-                .dependencies
-                .finish_early_when_all_transactions_are_valid
-                && !self.continue_if_necessary()?
-            {
-                info!("Finished early");
-                break; // finished early, we have fulfilled signing requirements of all transactions
+            if self.continuation() == FinishEarly {
+                break;
             }
             info!(
                 "Use(?) #{:?} factors of kind: {:?}",
@@ -315,7 +338,7 @@ mod tests {
     #[test]
     fn invalid_profile_unknown_account() {
         let res = SignaturesCollector::new(
-            true,
+            SigningFinishEarlyStrategy::default(),
             IndexSet::from_iter([TransactionIntent::new([Account::a0().entity_address()], [])]),
             Arc::new(TestSignatureCollectingInteractors::new(
                 SimulatedUser::prudent_no_fail(),
@@ -328,7 +351,7 @@ mod tests {
     #[test]
     fn invalid_profile_unknown_persona() {
         let res = SignaturesCollector::new(
-            true,
+            SigningFinishEarlyStrategy::default(),
             IndexSet::from_iter([TransactionIntent::new([], [Persona::p0().entity_address()])]),
             Arc::new(TestSignatureCollectingInteractors::new(
                 SimulatedUser::prudent_no_fail(),
@@ -343,7 +366,7 @@ mod tests {
         let factors_sources = HDFactorSource::all();
         let persona = Persona::p0();
         let collector = SignaturesCollector::new(
-            true,
+            SigningFinishEarlyStrategy::default(),
             IndexSet::from_iter([TransactionIntent::new([], [persona.entity_address()])]),
             Arc::new(TestSignatureCollectingInteractors::new(
                 SimulatedUser::prudent_no_fail(),
@@ -355,6 +378,75 @@ mod tests {
         assert!(outcome.successful())
     }
 
+    #[actix_rt::test]
+    async fn continues_even_with_failed_tx_when_configured_to() {
+        let factor_sources = &HDFactorSource::all();
+        let a0 = &Account::a0();
+        let a1 = &Account::a1();
+
+        let t0 = TransactionIntent::address_of([a1], []);
+        let t1 = TransactionIntent::address_of([a0], []);
+
+        let profile = Profile::new(factor_sources.clone(), [a0, a1], []);
+
+        let collector = SignaturesCollector::new(
+            SigningFinishEarlyStrategy::new(
+                WhenAllTransactionsAreValid(FinishEarly),
+                WhenSomeTransactionIsInvalid(Continue),
+            ),
+            IndexSet::<TransactionIntent>::from_iter([t0.clone(), t1.clone()]),
+            Arc::new(TestSignatureCollectingInteractors::new(
+                SimulatedUser::prudent_with_failures(SimulatedFailures::with_simulated_failures([
+                    FactorSourceIDFromHash::fs1(),
+                ])),
+            )),
+            &profile,
+        )
+        .unwrap();
+
+        let outcome = collector.collect_signatures().await;
+        assert!(!outcome.successful());
+        assert_eq!(outcome.failed_transactions().len(), 1);
+        assert_eq!(outcome.successful_transactions().len(), 1);
+    }
+
+    #[actix_rt::test]
+    async fn continues_even_when_all_valid_if_configured_to() {
+        sensible_env_logger::safe_init!();
+        let test = async move |when_all_valid: WhenAllTransactionsAreValid,
+                               expected_sig_count: usize| {
+            let factor_sources = &HDFactorSource::all();
+            let a5 = &Account::a5();
+
+            let t0 = TransactionIntent::address_of([a5], []);
+
+            let profile = Profile::new(factor_sources.clone(), [a5], []);
+
+            let collector = SignaturesCollector::new(
+                SigningFinishEarlyStrategy::new(
+                    when_all_valid,
+                    WhenSomeTransactionIsInvalid::default(),
+                ),
+                IndexSet::<TransactionIntent>::from_iter([t0.clone()]),
+                Arc::new(TestSignatureCollectingInteractors::new(
+                    SimulatedUser::prudent_no_fail(),
+                )),
+                &profile,
+            )
+            .unwrap();
+
+            let outcome = collector.collect_signatures().await;
+            assert!(outcome.successful());
+            assert_eq!(
+                outcome.signatures_of_successful_transactions().len(),
+                expected_sig_count
+            );
+        };
+
+        test(WhenAllTransactionsAreValid(FinishEarly), 1).await;
+        test(WhenAllTransactionsAreValid(Continue), 2).await;
+    }
+
     #[test]
     fn test_profile() {
         let factor_sources = &HDFactorSource::all();
@@ -376,7 +468,7 @@ mod tests {
         let profile = Profile::new(factor_sources.clone(), [a0, a1, a2, a6], [p0, p1, p2, p6]);
 
         let collector = SignaturesCollector::new(
-            true,
+            SigningFinishEarlyStrategy::default(),
             IndexSet::<TransactionIntent>::from_iter([
                 t0.clone(),
                 t1.clone(),

src/signing/collector/signatures_collector_dependencies.rs

@@ -4,7 +4,7 @@ pub(super) struct SignaturesCollectorDependencies {
     /// If `true` we stop collecting signatures as soon as all transactions are
     /// valid. This is typically always set to `true`, but can be useful for
     /// tests to set to `false` to see how the system behaves.
-    pub(super) finish_early_when_all_transactions_are_valid: bool,
+    pub(super) finish_early_strategy: SigningFinishEarlyStrategy,
 
     /// A collection of "interactors" used to sign with factor sources.
     pub(super) interactors: Arc<dyn SignatureCollectingInteractors>,
@@ -23,12 +23,12 @@ pub(super) struct SignaturesCollectorDependencies {
 
 impl SignaturesCollectorDependencies {
     pub fn new(
-        finish_early_when_all_transactions_are_valid: bool,
+        finish_early_strategy: SigningFinishEarlyStrategy,
         interactors: Arc<dyn SignatureCollectingInteractors>,
         factors_of_kind: IndexSet<FactorSourcesOfKind>,
     ) -> Self {
         Self {
-            finish_early_when_all_transactions_are_valid,
+            finish_early_strategy,
             interactors,
             factors_of_kind,
         }

src/signing/collector/signing_finish_early_strategy.rs

@@ -0,0 +1,37 @@
+use crate::prelude::*;
+
+#[derive(Clone, Copy, Debug, PartialEq, Eq)]
+pub struct WhenAllTransactionsAreValid(pub SignaturesCollectingContinuation);
+
+impl Default for WhenAllTransactionsAreValid {
+    fn default() -> Self {
+        Self(SignaturesCollectingContinuation::FinishEarly)
+    }
+}
+
+#[derive(Clone, Copy, Debug, PartialEq, Eq)]
+pub struct WhenSomeTransactionIsInvalid(pub SignaturesCollectingContinuation);
+
+impl Default for WhenSomeTransactionIsInvalid {
+    fn default() -> Self {
+        Self(SignaturesCollectingContinuation::FinishEarly)
+    }
+}
+
+/// Strategy to use for finishing early, i.e. stop collecting more signatures
+#[derive(Clone, Default, Copy, Debug, PartialEq, Eq)]
+pub struct SigningFinishEarlyStrategy {
+    pub when_all_transactions_are_valid: WhenAllTransactionsAreValid,
+    pub when_some_transaction_is_invalid: WhenSomeTransactionIsInvalid,
+}
+impl SigningFinishEarlyStrategy {
+    pub fn new(
+        when_all_transactions_are_valid: WhenAllTransactionsAreValid,
+        when_some_transaction_is_invalid: WhenSomeTransactionIsInvalid,
+    ) -> Self {
+        Self {
+            when_all_transactions_are_valid,
+            when_some_transaction_is_invalid,
+        }
+    }
+}

src/signing/petition_types/petition_entity.rs

@@ -183,17 +183,6 @@ impl PetitionEntity {
         }
     }
 
-    /// `Ok(true)` means "continue", `Ok(false)` means "stop, we are done". `Err(_)` means "stop, we have failed".
-    pub(super) fn continue_if_necessary(&self) -> Result<bool> {
-        match self.status() {
-            PetitionFactorsStatus::InProgress => Ok(true),
-            PetitionFactorsStatus::Finished(PetitionFactorsStatusFinished::Fail) => {
-                Err(CommonError::Failure)
-            }
-            PetitionFactorsStatus::Finished(PetitionFactorsStatusFinished::Success) => Ok(false),
-        }
-    }
-
     pub fn status_if_skipped_factors(
         &self,
         factor_source_ids: IndexSet<FactorSourceIDFromHash>,