refactor(aggregator): implement TryFrom<CertificateRecord> for Certificate

Author: dlachaume

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

@@ -161,8 +161,10 @@ impl TryFrom<Certificate> for CertificateRecord {
     }
 }
 
-impl From<CertificateRecord> for Certificate {
-    fn from(other: CertificateRecord) -> Self {
+impl TryFrom<CertificateRecord> for Certificate {
+    type Error = StdError;
+
+    fn try_from(other: CertificateRecord) -> Result<Self, Self::Error> {
         let certificate_metadata = CertificateMetadata::new(
             other.network,
             other.protocol_version,
@@ -174,27 +176,29 @@ impl From<CertificateRecord> for Certificate {
         let (previous_hash, signature) = match other.parent_certificate_id {
             None => (
                 String::new(),
-                CertificateSignature::GenesisSignature(other.signature.try_into().unwrap()),
+                CertificateSignature::GenesisSignature(other.signature.try_into()?),
             ),
             Some(parent_certificate_id) => (
                 parent_certificate_id,
                 CertificateSignature::MultiSignature(
                     other.signed_entity_type,
-                    other.signature.try_into().unwrap(),
+                    other.signature.try_into()?,
                 ),
             ),
         };
 
-        Certificate {
+        let certificate = Certificate {
             hash: other.certificate_id,
             previous_hash,
             epoch: other.epoch,
             metadata: certificate_metadata,
             signed_message: other.protocol_message.compute_hash(),
             protocol_message: other.protocol_message,
-            aggregate_verification_key: other.aggregate_verification_key.try_into().unwrap(),
+            aggregate_verification_key: other.aggregate_verification_key.try_into()?,
             signature,
-        }
+        };
+
+        Ok(certificate)
     }
 }
 
@@ -400,7 +404,7 @@ mod tests {
         }
         let mut certificates_new: Vec<Certificate> = Vec::new();
         for certificate_record in certificate_records {
-            certificates_new.push(certificate_record.into());
+            certificates_new.push(certificate_record.try_into().unwrap());
         }
         assert_eq!(certificates.certificates_chained, certificates_new);
     }
@@ -409,7 +413,7 @@ mod tests {
     fn converting_certificate_record_to_certificate_should_not_recompute_hash() {
         let expected_hash = "my_hash";
         let record = CertificateRecord::dummy_genesis(expected_hash, Epoch(1));
-        let certificate: Certificate = record.into();
+        let certificate: Certificate = record.try_into().unwrap();
 
         assert_eq!(expected_hash, &certificate.hash);
     }

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

@@ -4,9 +4,9 @@ use anyhow::anyhow;
 use async_trait::async_trait;
 use sqlite::ConnectionThreadSafe;
 
-use mithril_common::StdResult;
 use mithril_common::certificate_chain::{CertificateRetriever, CertificateRetrieverError};
 use mithril_common::entities::{Certificate, Epoch};
+use mithril_common::{StdError, StdResult};
 use mithril_persistence::sqlite::ConnectionExtensions;
 
 use crate::database::query::{
@@ -30,49 +30,56 @@ impl CertificateRepository {
     /// Return the certificate corresponding to the given hash if any.
     pub async fn get_certificate<T>(&self, hash: &str) -> StdResult<Option<T>>
     where
-        T: From<CertificateRecord>,
+        T: TryFrom<CertificateRecord>,
+        T::Error: Into<StdError>,
     {
         let record = self
             .connection
             .fetch_first(GetCertificateRecordQuery::by_certificate_id(hash))?;
 
-        Ok(record.map(|c| c.into()))
+        record.map(|c| c.try_into().map_err(Into::into)).transpose()
     }
 
     /// Return the latest certificates.
     pub async fn get_latest_certificates<T>(&self, last_n: usize) -> StdResult<Vec<T>>
     where
-        T: From<CertificateRecord>,
+        T: TryFrom<CertificateRecord>,
+        T::Error: Into<StdError>,
     {
         let cursor = self.connection.fetch(GetCertificateRecordQuery::all())?;
 
-        Ok(cursor.take(last_n).map(|v| v.into()).collect())
+        cursor
+            .take(last_n)
+            .map(|c| c.try_into().map_err(Into::into))
+            .collect()
     }
 
     /// Return the latest genesis certificate.
     pub async fn get_latest_genesis_certificate<T>(&self) -> StdResult<Option<T>>
     where
-        T: From<CertificateRecord>,
+        T: TryFrom<CertificateRecord>,
+        T::Error: Into<StdError>,
     {
         let record = self
             .connection
             .fetch_first(GetCertificateRecordQuery::all_genesis())?;
 
-        Ok(record.map(|c| c.into()))
+        record.map(|c| c.try_into().map_err(Into::into)).transpose()
     }
 
     /// Return the first certificate signed per epoch as the reference
     /// certificate for this Epoch. This will be the parent certificate for all
     /// other certificates issued within this Epoch.
     pub async fn get_master_certificate_for_epoch<T>(&self, epoch: Epoch) -> StdResult<Option<T>>
     where
-        T: From<CertificateRecord>,
+        T: TryFrom<CertificateRecord>,
+        T::Error: Into<StdError>,
     {
         let record = self
             .connection
             .fetch_first(MasterCertificateQuery::for_epoch(epoch))?;
 
-        Ok(record.map(|c| c.into()))
+        record.map(|c| c.try_into().map_err(Into::into)).transpose()
     }
 
     /// Create a new certificate in the database.
@@ -86,7 +93,7 @@ impl CertificateRepository {
                 panic!("No entity returned by the persister, certificate = {certificate:#?}")
             });
 
-        Ok(record.into())
+        record.try_into()
     }
 
     /// Create many certificates at once in the database.
@@ -105,7 +112,7 @@ impl CertificateRepository {
         let new_certificates =
             self.connection.fetch(InsertCertificateRecordQuery::many(records))?;
 
-        Ok(new_certificates.map(|cert| cert.into()).collect())
+        new_certificates.map(|cert| cert.try_into()).collect::<StdResult<_>>()
     }
 
     /// Create, or replace if they already exist, many certificates at once in the database.
@@ -125,7 +132,7 @@ impl CertificateRepository {
             .connection
             .fetch(InsertOrReplaceCertificateRecordQuery::many(records))?;
 
-        Ok(new_certificates.map(|cert| cert.into()).collect())
+        new_certificates.map(|cert| cert.try_into()).collect::<StdResult<_>>()
     }
 
     /// Delete all the given certificates from the database
@@ -335,7 +342,7 @@ mod tests {
     async fn get_master_certificate_one_cert_in_current_epoch_recorded_returns_that_one() {
         let connection = Arc::new(main_db_connection().unwrap());
         let certificate = CertificateRecord::dummy_genesis("1", Epoch(1));
-        let expected_certificate: Certificate = certificate.clone().into();
+        let expected_certificate: Certificate = certificate.clone().try_into().unwrap();
         insert_certificate_records(&connection, vec![certificate]);
 
         let repository: CertificateRepository = CertificateRepository::new(connection);
@@ -357,7 +364,8 @@ mod tests {
             CertificateRecord::dummy_db_snapshot("2", "1", Epoch(1), 2),
             CertificateRecord::dummy_db_snapshot("3", "1", Epoch(1), 3),
         ];
-        let expected_certificate: Certificate = certificates.first().unwrap().clone().into();
+        let expected_certificate: Certificate =
+            certificates.first().unwrap().clone().try_into().unwrap();
         insert_certificate_records(&connection, certificates);
 
         let repository: CertificateRepository = CertificateRepository::new(connection);
@@ -379,7 +387,8 @@ mod tests {
             CertificateRecord::dummy_db_snapshot("2", "1", Epoch(1), 2),
             CertificateRecord::dummy_db_snapshot("3", "1", Epoch(1), 3),
         ];
-        let expected_certificate: Certificate = certificates.first().unwrap().clone().into();
+        let expected_certificate: Certificate =
+            certificates.first().unwrap().clone().try_into().unwrap();
         insert_certificate_records(&connection, certificates);
 
         let repository: CertificateRepository = CertificateRepository::new(connection);
@@ -402,7 +411,8 @@ mod tests {
             CertificateRecord::dummy_db_snapshot("3", "1", Epoch(1), 3),
             CertificateRecord::dummy_db_snapshot("4", "1", Epoch(2), 4),
         ];
-        let expected_certificate: Certificate = certificates.last().unwrap().clone().into();
+        let expected_certificate: Certificate =
+            certificates.last().unwrap().clone().try_into().unwrap();
         insert_certificate_records(&connection, certificates);
 
         let repository: CertificateRepository = CertificateRepository::new(connection);
@@ -427,7 +437,8 @@ mod tests {
             CertificateRecord::dummy_db_snapshot("5", "4", Epoch(2), 5),
             CertificateRecord::dummy_db_snapshot("6", "4", Epoch(2), 6),
         ];
-        let expected_certificate: Certificate = certificates.get(3).unwrap().clone().into();
+        let expected_certificate: Certificate =
+            certificates.get(3).unwrap().clone().try_into().unwrap();
         insert_certificate_records(&connection, certificates);
 
         let repository: CertificateRepository = CertificateRepository::new(connection);
@@ -469,7 +480,8 @@ mod tests {
             CertificateRecord::dummy_db_snapshot("3", "1", Epoch(1), 3),
             CertificateRecord::dummy_genesis("4", Epoch(1)),
         ];
-        let expected_certificate: Certificate = certificates.last().unwrap().clone().into();
+        let expected_certificate: Certificate =
+            certificates.last().unwrap().clone().try_into().unwrap();
         insert_certificate_records(&connection, certificates);
 
         let repository: CertificateRepository = CertificateRepository::new(connection);
@@ -494,7 +506,8 @@ mod tests {
             CertificateRecord::dummy_db_snapshot("5", "1", Epoch(2), 5),
             CertificateRecord::dummy_genesis("6", Epoch(2)),
         ];
-        let expected_certificate: Certificate = certificates.last().unwrap().clone().into();
+        let expected_certificate: Certificate =
+            certificates.last().unwrap().clone().try_into().unwrap();
         insert_certificate_records(&connection, certificates);
 
         let repository: CertificateRepository = CertificateRepository::new(connection);
@@ -517,7 +530,8 @@ mod tests {
             CertificateRecord::dummy_db_snapshot("3", "1", Epoch(1), 3),
             CertificateRecord::dummy_genesis("4", Epoch(2)),
         ];
-        let expected_certificate: Certificate = certificates.last().unwrap().clone().into();
+        let expected_certificate: Certificate =
+            certificates.last().unwrap().clone().try_into().unwrap();
         insert_certificate_records(&connection, certificates);
 
         let repository: CertificateRepository = CertificateRepository::new(connection);
@@ -578,7 +592,8 @@ mod tests {
             CertificateRecord::dummy_db_snapshot("3", "1", Epoch(1), 3),
         ];
         insert_certificate_records(&connection, records.clone());
-        let certificates: Vec<Certificate> = records.into_iter().map(|c| c.into()).collect();
+        let certificates: Vec<Certificate> =
+            records.into_iter().map(|c| c.try_into().unwrap()).collect();
 
         // Delete all records except the first
         repository

mithril-aggregator/src/tools/certificates_hash_migrator.rs

@@ -254,7 +254,7 @@ mod test {
     fn dummy_genesis(certificate_hash: &str, time_point: TimePoint) -> Certificate {
         let certificate = CertificateRecord::dummy_genesis(certificate_hash, time_point.epoch);
 
-        certificate.into()
+        certificate.try_into().unwrap()
     }
 
     fn dummy_certificate(
@@ -272,7 +272,7 @@ mod test {
                 .unwrap(),
         );
 
-        certificate.into()
+        certificate.try_into().unwrap()
     }
 
     fn signed_entity_for_certificate(certificate: &Certificate) -> Option<SignedEntityRecord> {