refactor+style: Implement changes asked in PR reviews

* Simpify some code docs.
* Enhance `delete_buffered_single_signature` tests
* Make some naming more clear

Author: Alenar

mithril-aggregator/src/database/query/buffered_single_signature/delete_buffered_single_signature.rs

@@ -56,6 +56,7 @@ mod tests {
     use mithril_persistence::sqlite::ConnectionExtensions;
 
     use crate::database::query::GetBufferedSingleSignatureQuery;
+    use crate::database::record::strip_buffered_sigs_date;
     use crate::database::test_helper::{insert_buffered_single_signatures, main_db_connection};
 
     use super::*;
@@ -82,10 +83,17 @@ mod tests {
             .unwrap();
         assert_eq!(2, cursor.count());
 
-        let cursor = connection
-            .fetch(GetBufferedSingleSignatureQuery::all())
+        let remaining_records: Vec<BufferedSingleSignatureRecord> = connection
+            .fetch_collect(GetBufferedSingleSignatureQuery::all())
             .unwrap();
-        assert_eq!(3, cursor.count());
+        assert_eq!(
+            strip_buffered_sigs_date(&BufferedSingleSignatureRecord::fakes(&[
+                ("party_2", CardanoTransactions),
+                ("party_1", CardanoTransactions),
+                ("party_2", MithrilStakeDistribution),
+            ])),
+            strip_buffered_sigs_date(&remaining_records)
+        );
 
         let cursor = connection
             .fetch(
@@ -97,9 +105,15 @@ mod tests {
             .unwrap();
         assert_eq!(2, cursor.count());
 
-        let cursor = connection
-            .fetch(GetBufferedSingleSignatureQuery::all())
+        let remaining_records: Vec<BufferedSingleSignatureRecord> = connection
+            .fetch_collect(GetBufferedSingleSignatureQuery::all())
             .unwrap();
-        assert_eq!(1, cursor.count());
+        assert_eq!(
+            strip_buffered_sigs_date(&BufferedSingleSignatureRecord::fakes(&[(
+                "party_2",
+                MithrilStakeDistribution
+            ),])),
+            strip_buffered_sigs_date(&remaining_records)
+        );
     }
 }

mithril-aggregator/src/database/record/buffered_single_signature_record.rs

@@ -160,6 +160,17 @@ impl SqLiteEntity for BufferedSingleSignatureRecord {
     }
 }
 
+/// Test only - strip the date from the given records to make them comparable.
+#[cfg(test)]
+pub(crate) fn strip_buffered_sigs_date(
+    records: &[BufferedSingleSignatureRecord],
+) -> Vec<BufferedSingleSignatureRecord> {
+    records
+        .iter()
+        .map(BufferedSingleSignatureRecord::with_stripped_date)
+        .collect::<Vec<_>>()
+}
+
 #[cfg(test)]
 mod tests {
     use mithril_common::entities::SignedEntityTypeDiscriminants::{

mithril-aggregator/src/database/repository/buffered_single_signature_repository.rs

@@ -107,18 +107,11 @@ mod tests {
     };
     use mithril_common::test_utils::fake_keys;
 
-    use crate::database::record::BufferedSingleSignatureRecord;
+    use crate::database::record::{strip_buffered_sigs_date, BufferedSingleSignatureRecord};
     use crate::database::test_helper::{insert_buffered_single_signatures, main_db_connection};
 
     use super::*;
 
-    fn strip_date(records: &[BufferedSingleSignatureRecord]) -> Vec<BufferedSingleSignatureRecord> {
-        records
-            .iter()
-            .map(BufferedSingleSignatureRecord::with_stripped_date)
-            .collect::<Vec<_>>()
-    }
-
     #[test]
     fn retrieve_all() {
         let connection = main_db_connection().unwrap();
@@ -136,12 +129,12 @@ mod tests {
 
         let buffered_signatures_ctx = store.get_all().unwrap();
         assert_eq!(
-            strip_date(&BufferedSingleSignatureRecord::fakes(&[
+            strip_buffered_sigs_date(&BufferedSingleSignatureRecord::fakes(&[
                 ("party3", MithrilStakeDistribution),
                 ("party2", CardanoTransactions),
                 ("party1", CardanoTransactions),
             ])),
-            strip_date(&buffered_signatures_ctx)
+            strip_buffered_sigs_date(&buffered_signatures_ctx)
         );
     }
 
@@ -164,22 +157,22 @@ mod tests {
             .get_by_discriminant::<BufferedSingleSignatureRecord>(CardanoTransactions)
             .unwrap();
         assert_eq!(
-            strip_date(&BufferedSingleSignatureRecord::fakes(&[
+            strip_buffered_sigs_date(&BufferedSingleSignatureRecord::fakes(&[
                 ("party2", CardanoTransactions),
                 ("party1", CardanoTransactions),
             ])),
-            strip_date(&buffered_signatures_ctx)
+            strip_buffered_sigs_date(&buffered_signatures_ctx)
         );
 
         let buffered_signatures_msd = store
             .get_by_discriminant::<BufferedSingleSignatureRecord>(MithrilStakeDistribution)
             .unwrap();
         assert_eq!(
-            strip_date(&BufferedSingleSignatureRecord::fakes(&[(
+            strip_buffered_sigs_date(&BufferedSingleSignatureRecord::fakes(&[(
                 "party3",
                 MithrilStakeDistribution
             ),])),
-            strip_date(&buffered_signatures_msd)
+            strip_buffered_sigs_date(&buffered_signatures_msd)
         );
     }
 
@@ -295,11 +288,11 @@ mod tests {
 
         let remaining_msd_sigs = store.get_all().unwrap();
         assert_eq!(
-            strip_date(&BufferedSingleSignatureRecord::fakes(&[
+            strip_buffered_sigs_date(&BufferedSingleSignatureRecord::fakes(&[
                 ("party4", CardanoTransactions),
                 ("party2", MithrilStakeDistribution),
             ])),
-            strip_date(&remaining_msd_sigs)
+            strip_buffered_sigs_date(&remaining_msd_sigs)
         );
     }
 }

mithril-aggregator/src/http_server/routes/signatures_routes.rs

@@ -36,7 +36,7 @@ mod handlers {
     use crate::{
         http_server::routes::reply,
         message_adapters::FromRegisterSingleSignatureAdapter,
-        services::{CertifierService, CertifierServiceError, RegistrationStatus},
+        services::{CertifierService, CertifierServiceError, SignatureRegistrationStatus},
         unwrap_to_internal_server_error, SingleSignatureAuthenticator,
     };
 
@@ -99,8 +99,8 @@ mod handlers {
                     Ok(reply::server_error(err))
                 }
             },
-            Ok(RegistrationStatus::Registered) => Ok(reply::empty(StatusCode::CREATED)),
-            Ok(RegistrationStatus::Buffered) => Ok(reply::empty(StatusCode::ACCEPTED)),
+            Ok(SignatureRegistrationStatus::Registered) => Ok(reply::empty(StatusCode::CREATED)),
+            Ok(SignatureRegistrationStatus::Buffered) => Ok(reply::empty(StatusCode::ACCEPTED)),
         }
     }
 }
@@ -119,7 +119,7 @@ mod tests {
     use crate::{
         http_server::SERVER_BASE_PATH,
         initialize_dependencies,
-        services::{CertifierServiceError, MockCertifierService, RegistrationStatus},
+        services::{CertifierServiceError, MockCertifierService, SignatureRegistrationStatus},
         SingleSignatureAuthenticator,
     };
 
@@ -145,7 +145,7 @@ mod tests {
             .expect_register_single_signature()
             .withf(|_, signature| signature.is_authenticated())
             .once()
-            .return_once(move |_, _| Ok(RegistrationStatus::Registered));
+            .return_once(move |_, _| Ok(SignatureRegistrationStatus::Registered));
         let mut dependency_manager = initialize_dependencies().await;
         dependency_manager.certifier_service = Arc::new(mock_certifier_service);
         dependency_manager.single_signer_authenticator =
@@ -210,7 +210,7 @@ mod tests {
         let mut mock_certifier_service = MockCertifierService::new();
         mock_certifier_service
             .expect_register_single_signature()
-            .return_once(move |_, _| Ok(RegistrationStatus::Registered));
+            .return_once(move |_, _| Ok(SignatureRegistrationStatus::Registered));
         let mut dependency_manager = initialize_dependencies().await;
         dependency_manager.certifier_service = Arc::new(mock_certifier_service);
 
@@ -243,7 +243,7 @@ mod tests {
         let mut mock_certifier_service = MockCertifierService::new();
         mock_certifier_service
             .expect_register_single_signature()
-            .return_once(move |_, _| Ok(RegistrationStatus::Buffered));
+            .return_once(move |_, _| Ok(SignatureRegistrationStatus::Buffered));
         let mut dependency_manager = initialize_dependencies().await;
         dependency_manager.certifier_service = Arc::new(mock_certifier_service);
 
@@ -276,7 +276,7 @@ mod tests {
         let mut mock_certifier_service = MockCertifierService::new();
         mock_certifier_service
             .expect_register_single_signature()
-            .return_once(move |_, _| Ok(RegistrationStatus::Registered));
+            .return_once(move |_, _| Ok(SignatureRegistrationStatus::Registered));
         let mut dependency_manager = initialize_dependencies().await;
         dependency_manager.certifier_service = Arc::new(mock_certifier_service);
 

mithril-aggregator/src/multi_signer.rs

@@ -27,7 +27,7 @@ pub trait MultiSigner: Sync + Send {
     ) -> StdResult<()>;
 
     /// Verify a single signature using the stake distribution of the next epoch
-    async fn verify_single_signature_for_next_epoch(
+    async fn verify_single_signature_for_next_stake_distribution(
         &self,
         message: &str,
         signatures: &entities::SingleSignatures,
@@ -87,7 +87,7 @@ impl MultiSigner for MultiSignerImpl {
         self.run_verify_single_signature(message, single_signature, protocol_multi_signer)
     }
 
-    async fn verify_single_signature_for_next_epoch(
+    async fn verify_single_signature_for_next_stake_distribution(
         &self,
         message: &str,
         single_signature: &entities::SingleSignatures,
@@ -190,7 +190,7 @@ mod tests {
                 .await
                 .unwrap();
 
-            multi_signer.verify_single_signature_for_next_epoch(&message.to_message(), &signature).await.expect_err(
+            multi_signer.verify_single_signature_for_next_stake_distribution(&message.to_message(), &signature).await.expect_err(
                 "single signature issued in the current epoch should not be valid for the next epoch",
             );
         }
@@ -199,7 +199,7 @@ mod tests {
             let next_epoch_signature = next_fixture.signers_fixture()[0].sign(&message).unwrap();
 
             multi_signer
-                .verify_single_signature_for_next_epoch(
+                .verify_single_signature_for_next_stake_distribution(
                     &message.to_message(),
                     &next_epoch_signature,
                 )

mithril-aggregator/src/services/certifier/buffered_certifier.rs

@@ -10,10 +10,11 @@ use mithril_common::StdResult;
 
 use crate::entities::OpenMessage;
 use crate::services::{
-    BufferedSingleSignatureStore, CertifierService, CertifierServiceError, RegistrationStatus,
+    BufferedSingleSignatureStore, CertifierService, CertifierServiceError,
+    SignatureRegistrationStatus,
 };
 
-/// A decorator of [CertifierService] that buffers that can buffer registration of single signatures
+/// A decorator of [CertifierService] that can buffer registration of single signatures
 /// when the open message is not yet created.
 ///
 /// When an open message is created, buffered single signatures for the open message type are
@@ -90,7 +91,7 @@ impl CertifierService for BufferedCertifierService {
         &self,
         signed_entity_type: &SignedEntityType,
         signature: &SingleSignatures,
-    ) -> StdResult<RegistrationStatus> {
+    ) -> StdResult<SignatureRegistrationStatus> {
         match self
             .certifier_service
             .register_single_signature(signed_entity_type, signature)
@@ -110,7 +111,7 @@ impl CertifierService for BufferedCertifierService {
                         .buffer_signature(signed_entity_type.into(), signature)
                         .await?;
 
-                    Ok(RegistrationStatus::Buffered)
+                    Ok(SignatureRegistrationStatus::Buffered)
                 }
                 _ => Err(error),
             },
@@ -122,14 +123,8 @@ impl CertifierService for BufferedCertifierService {
         signed_entity_type: &SignedEntityType,
         protocol_message: &ProtocolMessage,
     ) -> StdResult<OpenMessage> {
-        // IMPORTANT: this method should not fail if the open message creation succeeds.
-        // else:
-        // 1 - state machine won't create a pending certificate for the signed entity type
-        // 2 - Without a pending certificate, the signers won't send their signatures
-        // 3 - state machine will retry the transition to signing and, since an open message was
-        // opened for the signed entity type, it will try the next on the list.
-        // 4 - since the state machine never was in signing it will never try to aggregate
-        // signatures for the signed entity type
+        // IMPORTANT: this method should not fail if the open message creation succeeds
+        // Otherwise, the state machine won't aggregate signatures for this open message.
 
         let creation_result = self
             .certifier_service
@@ -234,7 +229,10 @@ mod tests {
     async fn run_register_signature_scenario(
         decorated_certifier_mock_config: impl FnOnce(&mut MockCertifierService),
         signature_to_register: &SingleSignatures,
-    ) -> (StdResult<RegistrationStatus>, Vec<SingleSignatures>) {
+    ) -> (
+        StdResult<SignatureRegistrationStatus>,
+        Vec<SingleSignatures>,
+    ) {
         let store = Arc::new(BufferedSingleSignatureRepository::new(Arc::new(
             main_db_connection().unwrap(),
         )));
@@ -267,14 +265,14 @@ mod tests {
                 |mock_certifier| {
                     mock_certifier
                         .expect_register_single_signature()
-                        .returning(|_, _| Ok(RegistrationStatus::Registered));
+                        .returning(|_, _| Ok(SignatureRegistrationStatus::Registered));
                 },
                 &SingleSignatures::fake("party_1", "a message"),
             )
             .await;
 
         let status = registration_result.expect("Registration should have succeed");
-        assert_eq!(status, RegistrationStatus::Registered);
+        assert_eq!(status, SignatureRegistrationStatus::Registered);
         assert_eq!(
             buffered_signatures_after_registration,
             Vec::<SingleSignatures>::new()
@@ -306,7 +304,7 @@ mod tests {
                 .await;
 
             let status = registration_result.expect("Registration should have succeed");
-            assert_eq!(status, RegistrationStatus::Buffered);
+            assert_eq!(status, SignatureRegistrationStatus::Buffered);
             assert_eq!(
                 buffered_signatures_after_registration,
                 vec![SingleSignatures::fake("party_1", "a message")]
@@ -379,14 +377,14 @@ mod tests {
                         eq(SingleSignatures::fake("party_1", "message 1")),
                     )
                     .once()
-                    .returning(|_, _| Ok(RegistrationStatus::Registered));
+                    .returning(|_, _| Ok(SignatureRegistrationStatus::Registered));
                 mock.expect_register_single_signature()
                     .with(
                         eq(SignedEntityType::MithrilStakeDistribution(Epoch(5))),
                         eq(SingleSignatures::fake("party_2", "message 2")),
                     )
                     .once()
-                    .returning(|_, _| Ok(RegistrationStatus::Registered));
+                    .returning(|_, _| Ok(SignatureRegistrationStatus::Registered));
             }),
             store.clone(),
             TestLogger::stdout(),
@@ -441,7 +439,7 @@ mod tests {
 
                     mock.expect_register_single_signature()
                         .with(always(), eq(fake_data::single_signatures(vec![1])))
-                        .returning(|_, _| Ok(RegistrationStatus::Registered))
+                        .returning(|_, _| Ok(SignatureRegistrationStatus::Registered))
                         .once();
                     mock.expect_register_single_signature()
                         .with(always(), eq(fake_data::single_signatures(vec![2])))
@@ -455,7 +453,7 @@ mod tests {
                         .once();
                     mock.expect_register_single_signature()
                         .with(always(), eq(fake_data::single_signatures(vec![3])))
-                        .returning(|_, _| Ok(RegistrationStatus::Registered))
+                        .returning(|_, _| Ok(SignatureRegistrationStatus::Registered))
                         .once();
                 },
                 |mock| {
@@ -482,7 +480,7 @@ mod tests {
                     mock.expect_create_open_message()
                         .returning(|_, _| Ok(OpenMessage::dummy()));
                     mock.expect_register_single_signature()
-                        .returning(|_, _| Ok(RegistrationStatus::Registered));
+                        .returning(|_, _| Ok(SignatureRegistrationStatus::Registered));
                 },
                 |mock| {
                     mock.expect_get_buffered_signatures()
@@ -493,7 +491,7 @@ mod tests {
         }
 
         #[tokio::test]
-        async fn do_not_return_an_error_if_getting_registering_signature_fail() {
+        async fn do_not_return_an_error_if_registering_signature_fail() {
             run_scenario(
                 |mock| {
                     mock.expect_create_open_message()
@@ -516,7 +514,7 @@ mod tests {
                     mock.expect_create_open_message()
                         .returning(|_, _| Ok(OpenMessage::dummy()));
                     mock.expect_register_single_signature()
-                        .returning(|_, _| Ok(RegistrationStatus::Registered));
+                        .returning(|_, _| Ok(SignatureRegistrationStatus::Registered));
                 },
                 |mock| {
                     mock.expect_get_buffered_signatures()