Apply review comments

Author: jpraynaud

Cargo.lock

@@ -23,6 +23,7 @@ semver = "1.0.16"
 serde = { version = "1.0", features = ["derive"] }
 serde_json = "1.0"
 serde_yaml = "0.9.10"
+sha2 = "0.10.2"
 slog = { version = "2.7.0", features = ["max_level_trace", "release_max_level_debug"] }
 slog-async = "2.7.0"
 slog-bunyan = "2.4.0"

mithril-aggregator/Cargo.toml

@@ -23,19 +23,11 @@ use mockall::automock;
 #[cfg_attr(test, automock)]
 #[async_trait]
 pub trait ArtifactBuilderService: Send + Sync {
-    /// Compute artifact from signed entity type
-    async fn compute_artifact(
+    /// Create artifact for a signed entity type and a certificate
+    async fn create_artifact(
         &self,
         signed_entity_type: SignedEntityType,
         certificate: &Certificate,
-    ) -> StdResult<Arc<dyn Artifact>>;
-
-    /// Create an save signed entity
-    async fn create_and_save_signed_entity(
-        &self,
-        signed_entity_type: SignedEntityType,
-        certificate: &Certificate,
-        artifact: Arc<dyn Artifact>,
     ) -> StdResult<()>;
 }
 
@@ -62,10 +54,8 @@ impl MithrilArtifactBuilderService {
             cardano_immutable_files_full_artifact_builder,
         }
     }
-}
 
-#[async_trait]
-impl ArtifactBuilderService for MithrilArtifactBuilderService {
+    /// Compute artifact from signed entity type
     async fn compute_artifact(
         &self,
         signed_entity_type: SignedEntityType,
@@ -85,13 +75,18 @@ impl ArtifactBuilderService for MithrilArtifactBuilderService {
             SignedEntityType::CardanoStakeDistribution(_) => todo!(),
         }
     }
+}
 
-    async fn create_and_save_signed_entity(
+#[async_trait]
+impl ArtifactBuilderService for MithrilArtifactBuilderService {
+    async fn create_artifact(
         &self,
         signed_entity_type: SignedEntityType,
         certificate: &Certificate,
-        artifact: Arc<dyn Artifact>,
     ) -> StdResult<()> {
+        let artifact = self
+            .compute_artifact(signed_entity_type.clone(), certificate)
+            .await?;
         let signed_entity = SignedEntityRecord {
             signed_entity_id: artifact.get_id(),
             signed_entity_type,

mithril-aggregator/src/artifact_builder/artifact_builder_service.rs

@@ -1,5 +1,6 @@
 use async_trait::async_trait;
 use serde::{Deserialize, Serialize};
+use sha2::{Digest, Sha256};
 use std::sync::Arc;
 use tokio::sync::RwLock;
 
@@ -15,17 +16,37 @@ use mithril_common::{
 #[derive(Debug, PartialEq, Serialize, Deserialize, Clone)]
 pub struct MithrilStakeDistribution {
     signers_with_stake: Vec<SignerWithStake>,
+    hash: String,
 }
 
 impl MithrilStakeDistribution {
     /// MithrilStakeDistribution artifact factory
     pub fn new(signers_with_stake: Vec<SignerWithStake>) -> Self {
-        Self { signers_with_stake }
+        let mut signers_with_stake_sorted = signers_with_stake;
+        signers_with_stake_sorted.sort();
+        let mut mithril_stake_distribution = Self {
+            signers_with_stake: signers_with_stake_sorted,
+            hash: "".to_string(),
+        };
+        mithril_stake_distribution.hash = mithril_stake_distribution.compute_hash();
+        mithril_stake_distribution
+    }
+
+    fn compute_hash(&self) -> String {
+        let mut hasher = Sha256::new();
+        for signer_with_stake in &self.signers_with_stake {
+            hasher.update(signer_with_stake.compute_hash().as_bytes());
+        }
+        hex::encode(hasher.finalize())
     }
 }
 
 #[typetag::serde]
-impl Artifact for MithrilStakeDistribution {}
+impl Artifact for MithrilStakeDistribution {
+    fn get_id(&self) -> String {
+        self.hash.clone()
+    }
+}
 
 /// A [MithrilStakeDistributionArtifact] builder
 pub struct MithrilStakeDistributionArtifactBuilder {
@@ -79,4 +100,24 @@ mod tests {
         let artifact_expected = MithrilStakeDistribution::new(signers_with_stake);
         assert_eq!(artifact_expected, artifact);
     }
+
+    #[test]
+    fn sort_given_signers_when_created() {
+        let signers_with_stake = fake_data::signers_with_stakes(5);
+
+        assert_eq!(
+            MithrilStakeDistribution::new(signers_with_stake.clone()),
+            MithrilStakeDistribution::new(signers_with_stake.into_iter().rev().collect())
+        );
+    }
+
+    #[test]
+    fn hash_value_doesnt_change_if_signers_order_change() {
+        let signers_with_stake = fake_data::signers_with_stakes(5);
+
+        let sd = MithrilStakeDistribution::new(signers_with_stake.clone());
+        let sd2 = MithrilStakeDistribution::new(signers_with_stake.into_iter().rev().collect());
+
+        assert_eq!(sd.hash, sd2.hash);
+    }
 }

mithril-aggregator/src/artifact_builder/mithril_stake_distribution.rs

@@ -122,7 +122,7 @@ pub trait AggregatorRunnerTrait: Sync + Send {
     ) -> Result<Option<Certificate>, Box<dyn StdError + Sync + Send>>;
 
     /// Create an artifact and persist it.
-    async fn create_and_save_artifact(
+    async fn create_artifact(
         &self,
         signed_entity_type: &SignedEntityType,
         certificate: &Certificate,
@@ -441,20 +441,15 @@ impl AggregatorRunnerTrait for AggregatorRunner {
             .await
     }
 
-    async fn create_and_save_artifact(
+    async fn create_artifact(
         &self,
         signed_entity_type: &SignedEntityType,
         certificate: &Certificate,
     ) -> Result<(), Box<dyn StdError + Sync + Send>> {
-        debug!("RUNNER: create and save artifact");
-        let artifact = self
-            .dependencies
-            .artifact_builder_service
-            .compute_artifact(signed_entity_type.to_owned(), certificate)
-            .await?;
+        debug!("RUNNER: create artifact");
         self.dependencies
             .artifact_builder_service
-            .create_and_save_signed_entity(signed_entity_type.to_owned(), certificate, artifact)
+            .create_artifact(signed_entity_type.to_owned(), certificate)
             .await?;
 
         Ok(())

mithril-aggregator/src/runtime/runner.rs

@@ -283,7 +283,7 @@ impl AggregatorRuntime {
 
         self.runner.drop_pending_certificate().await?;
         self.runner
-            .create_and_save_artifact(&state.signed_entity_type, &certificate)
+            .create_artifact(&state.signed_entity_type, &certificate)
             .await?;
 
         Ok(IdleState {
@@ -671,7 +671,7 @@ mod tests {
             .once()
             .returning(|| Ok(Some(fake_data::certificate_pending())));
         runner
-            .expect_create_and_save_artifact()
+            .expect_create_artifact()
             .once()
             .returning(|_, _| Ok(()));
 

mithril-aggregator/src/runtime/state_machine.rs

@@ -50,7 +50,6 @@ strum_macros = "0.24.1"
 thiserror = "1.0.31"
 tokio = { version = "1.17.0", features = ["full"] }
 typetag = "0.2.8"
-uuid = { version = "1.3.0", features = ["v4", "fast-rng", "macro-diagnostics"] }
 walkdir = "2"
 warp = "0.3"
 

mithril-common/Cargo.toml

@@ -81,7 +81,7 @@ impl From<SignerWithStake> for Signer {
 }
 
 /// Signer represents a signing party in the network (including its stakes)
-#[derive(Clone, Debug, PartialEq, Eq, Default, Serialize, Deserialize)]
+#[derive(Clone, Debug, PartialEq, Eq, PartialOrd, Ord, Default, Serialize, Deserialize)]
 pub struct SignerWithStake {
     /// The unique identifier of the signer
     // TODO: Should be removed once the signer certification is fully deployed

mithril-common/src/entities/signer.rs

@@ -1,6 +1,5 @@
 use async_trait::async_trait;
 use std::fmt::Debug;
-use uuid::Uuid;
 
 use crate::{entities::ProtocolMessage, StdResult};
 
@@ -14,9 +13,7 @@ pub trait Beacon: Send + Sync {}
 #[typetag::serde(tag = "type")]
 pub trait Artifact: Debug + Send + Sync {
     /// Get artifact identifier
-    fn get_id(&self) -> String {
-        Uuid::new_v4().to_string()
-    }
+    fn get_id(&self) -> String;
 }
 
 /// SignableBuilder is trait for building a protocol message for a beacon