Taking into account the remarks of the PR

sfauvel · sfauvel · commit 4c4463aa90cc · 2024-09-09T18:12:14.000+02:00
diff --git a/mithril-common/src/messages/epoch_settings.rs b/mithril-common/src/messages/epoch_settings.rs
@@ -132,7 +132,7 @@ mod tests {
         }
     }
 
-    // Test the retro compatibility with previous structure.
+    // Test the backward compatibility with previous structure.
     #[test]
     fn test_actual_json_deserialized_into_previous_message() {
         let json = ACTUAL_JSON;
diff --git a/mithril-common/src/messages/message_parts/signer.rs b/mithril-common/src/messages/message_parts/signer.rs
@@ -70,35 +70,46 @@ impl SignerWithStakeMessagePart {
 
     /// Convert a set of signer message parts into a set of signers with stake
     pub fn try_into_signers(messages: Vec<Self>) -> StdResult<Vec<SignerWithStake>> {
-        let mut signers: Vec<SignerWithStake> = Vec::new();
+        messages
+            .into_iter()
+            .map(SignerWithStakeMessagePart::try_into)
+            .collect()
+    }
+}
 
-        for message in messages {
-            let verification_key_signature: Option<ProtocolSignerVerificationKeySignature> = message.verification_key_signature
-                .map(|f| f.try_into())
-                .transpose()
-                .with_context(|| format!("Error while parsing verification key signature message, party_id = '{}'", message.party_id))?;
-            let operational_certificate: Option<ProtocolOpCert> = message
-                .operational_certificate
-                .map(|f| f.try_into())
-                .transpose()
-                .with_context(|| {
-                    format!(
-                        "Error while parsing operational certificate message, party_id = '{}'.",
-                        message.party_id
-                    )
-                })?;
-            let value = SignerWithStake {
-                party_id: message.party_id,
-                verification_key: message.verification_key.try_into()?,
-                verification_key_signature,
-                kes_period: message.kes_period,
-                operational_certificate,
-                stake: message.stake,
-            };
-            signers.push(value);
-        }
+impl TryInto<SignerWithStake> for SignerWithStakeMessagePart {
+    type Error = StdError;
 
-        Ok(signers)
+    fn try_into(self) -> Result<SignerWithStake, Self::Error> {
+        let verification_key_signature: Option<ProtocolSignerVerificationKeySignature> = self
+            .verification_key_signature
+            .map(|f| f.try_into())
+            .transpose()
+            .with_context(|| {
+                format!(
+                    "Error while parsing verification key signature message, party_id = '{}'",
+                    self.party_id
+                )
+            })?;
+        let operational_certificate: Option<ProtocolOpCert> = self
+            .operational_certificate
+            .map(|f| f.try_into())
+            .transpose()
+            .with_context(|| {
+                format!(
+                    "Error while parsing operational certificate message, party_id = '{}'.",
+                    self.party_id
+                )
+            })?;
+        let value = SignerWithStake {
+            party_id: self.party_id,
+            verification_key: self.verification_key.try_into()?,
+            verification_key_signature,
+            kes_period: self.kes_period,
+            operational_certificate,
+            stake: self.stake,
+        };
+        Ok(value)
     }
 }
 
diff --git a/mithril-signer/src/message_adapters/from_epoch_settings.rs b/mithril-signer/src/message_adapters/from_epoch_settings.rs
@@ -1,21 +1,26 @@
+use anyhow::Context;
 use mithril_common::{
     entities::EpochSettings,
-    messages::{EpochSettingsMessage, FromMessageAdapter, SignerMessagePart},
+    messages::{EpochSettingsMessage, SignerMessagePart, TryFromMessageAdapter},
+    StdResult,
 };
 
 /// Adapter to convert [EpochSettingsMessage] to [EpochSettings].
 pub struct FromEpochSettingsAdapter;
 
-impl FromMessageAdapter<EpochSettingsMessage, EpochSettings> for FromEpochSettingsAdapter {
+impl TryFromMessageAdapter<EpochSettingsMessage, EpochSettings> for FromEpochSettingsAdapter {
     /// Method to convert.
-    fn adapt(message: EpochSettingsMessage) -> EpochSettings {
-        EpochSettings {
+    fn try_adapt(message: EpochSettingsMessage) -> StdResult<EpochSettings> {
+        let epoch_settings = EpochSettings {
             epoch: message.epoch,
             protocol_parameters: message.protocol_parameters,
             next_protocol_parameters: message.next_protocol_parameters,
-            current_signers: SignerMessagePart::try_into_signers(message.current_signers).unwrap(),
-            next_signers: SignerMessagePart::try_into_signers(message.next_signers).unwrap(),
-        }
+            current_signers: SignerMessagePart::try_into_signers(message.current_signers)
+                .with_context(|| "'FromMessageAdapter' can not convert the current signers")?,
+            next_signers: SignerMessagePart::try_into_signers(message.next_signers)
+                .with_context(|| "'FromMessageAdapter' can not convert the next signers")?,
+        };
+        Ok(epoch_settings)
     }
 }
 
@@ -27,7 +32,7 @@ mod tests {
     fn test_simple_message() {
         let message = EpochSettingsMessage::dummy();
         let epoch = message.epoch;
-        let epoch_settings = FromEpochSettingsAdapter::adapt(message);
+        let epoch_settings = FromEpochSettingsAdapter::try_adapt(message).unwrap();
 
         assert_eq!(epoch, epoch_settings.epoch);
     }
diff --git a/mithril-signer/src/message_adapters/from_pending_certificate_message.rs b/mithril-signer/src/message_adapters/from_pending_certificate_message.rs
@@ -19,9 +19,9 @@ impl TryFromMessageAdapter<CertificatePendingMessage, CertificatePending>
             signed_entity_type: message.signed_entity_type,
             protocol_parameters: message.protocol_parameters,
             next_protocol_parameters: message.next_protocol_parameters,
-            // This field is deprecated and should not be used in Signer.
+            // This field is deprecated and should not be used by the Mithril signer.
             signers: vec![],
-            // This field is deprecated and should not be used in Signer.
+            // This field is deprecated and should not be used by the Mithril signer.
             next_signers: vec![],
         };
 
diff --git a/mithril-signer/src/runtime/runner.rs b/mithril-signer/src/runtime/runner.rs
@@ -117,6 +117,16 @@ impl SignerRunner {
             .next_signers_with_stake()
             .await
     }
+
+    /// Get the current signers.
+    async fn get_current_signers(&self) -> StdResult<Vec<Signer>> {
+        self.services
+            .epoch_service
+            .read()
+            .await
+            .current_signers()
+            .cloned()
+    }
 }
 
 #[cfg_attr(test, automock)]
@@ -264,11 +274,8 @@ impl Runner for SignerRunner {
             return Ok(false);
         }
 
-        // TODO XXX How handle errors ? It occurs in test_create_immutable_files_full_single_signature integration test (no signers in epoch 1)
-        // TODO XXX Do we warn and return "can not sign" or return an error ?
-        let current_signer_with_stake: Option<SignerWithStake> = {
-            let epoch_service = self.services.epoch_service.read().await;
-            let current_signers = epoch_service.current_signers_with_stake().await;
+        let current_signer: Option<Signer> = {
+            let current_signers = self.get_current_signers().await;
             if let Ok(signers) = current_signers {
                 signers
                     .iter()
@@ -280,7 +287,7 @@ impl Runner for SignerRunner {
             }
         };
 
-        if let Some(signer) = current_signer_with_stake {
+        if let Some(signer) = current_signer {
             debug!(" > got a Signer from pending certificate");
 
             if let Some(protocol_initializer) = self
diff --git a/mithril-signer/src/services/aggregator_client.rs b/mithril-signer/src/services/aggregator_client.rs
@@ -12,7 +12,7 @@ use mithril_common::{
     },
     messages::{
         AggregatorFeaturesMessage, CertificatePendingMessage, EpochSettingsMessage,
-        FromMessageAdapter, TryFromMessageAdapter, TryToMessageAdapter,
+        TryFromMessageAdapter, TryToMessageAdapter,
     },
     StdError, MITHRIL_API_VERSION_HEADER, MITHRIL_SIGNER_VERSION_HEADER,
 };
@@ -197,7 +197,11 @@ impl AggregatorClient for AggregatorHTTPClient {
         match response {
             Ok(response) => match response.status() {
                 StatusCode::OK => match response.json::<EpochSettingsMessage>().await {
-                    Ok(message) => Ok(Some(FromEpochSettingsAdapter::adapt(message))),
+                    Ok(message) => {
+                        let epoch_settings = FromEpochSettingsAdapter::try_adapt(message)
+                            .map_err(|e| AggregatorClientError::Adapter(anyhow!(e)))?;
+                        Ok(Some(epoch_settings))
+                    }
                     Err(err) => Err(AggregatorClientError::JsonParseFailed(anyhow!(err))),
                 },
                 StatusCode::PRECONDITION_FAILED => Err(self.handle_api_error(&response)),
@@ -587,7 +591,7 @@ mod tests {
         let epoch_settings = certificate_handler.retrieve_epoch_settings().await;
         epoch_settings.as_ref().expect("unexpected error");
         assert_eq!(
-            FromEpochSettingsAdapter::adapt(epoch_settings_expected),
+            FromEpochSettingsAdapter::try_adapt(epoch_settings_expected).unwrap(),
             epoch_settings.unwrap().unwrap()
         );
     }
diff --git a/mithril-signer/src/services/epoch_service.rs b/mithril-signer/src/services/epoch_service.rs
@@ -14,7 +14,7 @@ use mithril_common::StdResult;
 
 use crate::RunnerError;
 
-/// Errors dedicated to the CertifierService.
+/// Errors dedicated to the EpochService.
 #[derive(Debug, Error)]
 pub enum EpochServiceError {
     /// Raised when service has not collected data at least once.

Original file line number	Diff line number	Diff line change
`@@ -132,7 +132,7 @@ mod tests {`
`132`	`132`	`}`
`133`	`133`	`}`
`134`	`134`
`135`		`- // Test the retro compatibility with previous structure.`
	`135`	`+ // Test the backward compatibility with previous structure.`
`136`	`136`	`#[test]`
`137`	`137`	`fn test_actual_json_deserialized_into_previous_message() {`
`138`	`138`	`let json = ACTUAL_JSON;`