refactor: rollback use of CardanoTransactionRetriever in signable builder

Author: jpraynaud

mithril-aggregator/src/database/provider/cardano_transaction.rs

@@ -1,6 +1,6 @@
 use mithril_common::{
     entities::{Beacon, BlockNumber, CardanoTransaction, ImmutableFileNumber, TransactionHash},
-    signable_builder::{TransactionRetriever, TransactionStore},
+    signable_builder::TransactionStore,
     StdResult,
 };
 use mithril_persistence::sqlite::{
@@ -13,6 +13,8 @@ use async_trait::async_trait;
 use sqlite::{Row, Value};
 use std::{iter::repeat, sync::Arc};
 
+use crate::services::TransactionsRetriever;
+
 /// Cardano Transaction record is the representation of a cardano transaction.
 #[derive(Debug, PartialEq, Clone)]
 pub struct CardanoTransactionRecord {
@@ -280,7 +282,7 @@ impl TransactionStore for CardanoTransactionRepository {
 }
 
 #[async_trait]
-impl TransactionRetriever for CardanoTransactionRepository {
+impl TransactionsRetriever for CardanoTransactionRepository {
     async fn get_up_to(&self, beacon: &Beacon) -> StdResult<Vec<CardanoTransaction>> {
         self.get_transactions_up_to(beacon).await.map(|v| {
             v.into_iter()

mithril-aggregator/src/dependency_injection/builder.rs

@@ -33,7 +33,7 @@ use mithril_common::{
     signable_builder::{
         CardanoImmutableFilesFullSignableBuilder, CardanoTransactionsSignableBuilder,
         MithrilSignableBuilderService, MithrilStakeDistributionSignableBuilder,
-        SignableBuilderService, TransactionRetriever, TransactionStore,
+        SignableBuilderService, TransactionStore,
     },
     BeaconProvider, BeaconProviderImpl,
 };
@@ -136,9 +136,6 @@ pub struct DependenciesBuilder {
     /// Cardano transactions store.
     pub transaction_store: Option<Arc<dyn TransactionStore>>,
 
-    /// Cardano transactions retriever.
-    pub transaction_retriever: Option<Arc<dyn TransactionRetriever>>,
-
     /// Cardano transactions parser.
     pub transaction_parser: Option<Arc<dyn TransactionParser>>,
 
@@ -239,7 +236,6 @@ impl DependenciesBuilder {
             transaction_parser: None,
             transaction_repository: None,
             transaction_store: None,
-            transaction_retriever: None,
             immutable_digester: None,
             immutable_file_observer: None,
             immutable_cache_provider: None,
@@ -747,21 +743,6 @@ impl DependenciesBuilder {
         Ok(self.transaction_store.as_ref().cloned().unwrap())
     }
 
-    async fn build_transaction_retriever(&mut self) -> Result<Arc<dyn TransactionRetriever>> {
-        let transaction_retriever = self.get_transaction_repository().await?;
-
-        Ok(transaction_retriever as Arc<dyn TransactionRetriever>)
-    }
-
-    /// Transaction retriever.
-    pub async fn get_transaction_retriever(&mut self) -> Result<Arc<dyn TransactionRetriever>> {
-        if self.transaction_retriever.is_none() {
-            self.transaction_retriever = Some(self.build_transaction_retriever().await?);
-        }
-
-        Ok(self.transaction_retriever.as_ref().cloned().unwrap())
-    }
-
     async fn build_transaction_parser(&mut self) -> Result<Arc<dyn TransactionParser>> {
         // TODO: 'allow_unparsable_block' parameter should be configurable and its default value set to false
         let allow_unparsable_block = true;
@@ -1104,7 +1085,6 @@ impl DependenciesBuilder {
         let cardano_transactions_builder = Arc::new(CardanoTransactionsSignableBuilder::new(
             self.get_transaction_parser().await?,
             self.get_transaction_store().await?,
-            self.get_transaction_retriever().await?,
             &self.configuration.db_directory,
             self.get_logger().await?,
         ));

mithril-aggregator/src/services/prover.rs

@@ -8,7 +8,6 @@ use mithril_common::{
     entities::{
         Beacon, BlockRange, CardanoTransaction, CardanoTransactionsSetProof, TransactionHash,
     },
-    signable_builder::TransactionRetriever,
     StdResult,
 };
 
@@ -27,14 +26,22 @@ pub trait ProverService: Sync + Send {
     ) -> StdResult<Vec<CardanoTransactionsSetProof>>;
 }
 
+/// Transactions retriever
+#[cfg_attr(test, automock)]
+#[async_trait]
+pub trait TransactionsRetriever: Sync + Send {
+    /// Get transactions up to given beacon using chronological order
+    async fn get_up_to(&self, beacon: &Beacon) -> StdResult<Vec<CardanoTransaction>>;
+}
+
 /// Mithril prover
 pub struct MithrilProverService {
-    transaction_retriever: Arc<dyn TransactionRetriever>,
+    transaction_retriever: Arc<dyn TransactionsRetriever>,
 }
 
 impl MithrilProverService {
     /// Create a new Mithril prover
-    pub fn new(transaction_retriever: Arc<dyn TransactionRetriever>) -> Self {
+    pub fn new(transaction_retriever: Arc<dyn TransactionsRetriever>) -> Self {
         Self {
             transaction_retriever,
         }
@@ -121,20 +128,10 @@ mod tests {
     use anyhow::anyhow;
     use mithril_common::entities::CardanoTransaction;
     use mithril_common::test_utils::fake_data;
-    use mockall::{mock, predicate::eq};
+    use mockall::predicate::eq;
 
     use super::*;
 
-    mock! {
-        TransactionRetrieverImpl { }
-
-        #[async_trait]
-        impl TransactionRetriever for TransactionRetrieverImpl
-        {
-            async fn get_up_to(&self, beacon: &Beacon) -> StdResult<Vec<CardanoTransaction>>;
-        }
-    }
-
     fn generate_transactions(
         total_transactions: usize,
     ) -> (Vec<TransactionHash>, Vec<CardanoTransaction>) {
@@ -153,7 +150,7 @@ mod tests {
     #[tokio::test]
     async fn compute_proof_for_one_set_with_multiple_transactions() {
         let (transaction_hashes, transactions) = generate_transactions(3);
-        let mut transaction_retriever = MockTransactionRetrieverImpl::new();
+        let mut transaction_retriever = MockTransactionsRetriever::new();
         transaction_retriever
             .expect_get_up_to()
             .with(eq(fake_data::beacon()))
@@ -175,7 +172,7 @@ mod tests {
     #[tokio::test]
     async fn cant_compute_proof_for_unknown_transaction() {
         let (transaction_hashes, _transactions) = generate_transactions(3);
-        let mut transaction_retriever = MockTransactionRetrieverImpl::new();
+        let mut transaction_retriever = MockTransactionsRetriever::new();
         transaction_retriever
             .expect_get_up_to()
             .with(eq(fake_data::beacon()))
@@ -194,7 +191,7 @@ mod tests {
         let (transaction_hashes, transactions) = generate_transactions(5);
         // The last two are not in the "store"
         let transactions = transactions[0..=2].to_vec();
-        let mut transaction_retriever = MockTransactionRetrieverImpl::new();
+        let mut transaction_retriever = MockTransactionsRetriever::new();
         transaction_retriever
             .expect_get_up_to()
             .with(eq(fake_data::beacon()))
@@ -213,13 +210,10 @@ mod tests {
         transactions_set_proof[0].verify().unwrap();
     }
 
-    // this one can't be done right now because we don't have a merkle tree of merkle tree yet
-    // todo: compute_proof_for_multiple_set_with_multiple_transactions
-
     #[tokio::test]
     async fn cant_compute_proof_if_retriever_fail() {
         let (transaction_hashes, _transactions) = generate_transactions(3);
-        let mut transaction_retriever = MockTransactionRetrieverImpl::new();
+        let mut transaction_retriever = MockTransactionsRetriever::new();
         transaction_retriever
             .expect_get_up_to()
             .with(eq(fake_data::beacon()))

mithril-common/src/messages/cardano_transactions_proof.rs

@@ -190,8 +190,7 @@ mod tests {
         crypto_helper::MKProof,
         entities::{Beacon, CardanoTransaction},
         signable_builder::{
-            CardanoTransactionsSignableBuilder, MockTransactionRetriever, MockTransactionStore,
-            SignableBuilder,
+            CardanoTransactionsSignableBuilder, MockTransactionStore, SignableBuilder,
         },
     };
 
@@ -381,17 +380,10 @@ mod tests {
             .expect_store_transactions()
             .returning(|_| Ok(()))
             .times(1);
-        let transaction_retrieved = transactions.to_vec();
-        let mut mock_transaction_retriever = MockTransactionRetriever::new();
-        mock_transaction_retriever
-            .expect_get_up_to()
-            .return_once(|_| Ok(transaction_retrieved))
-            .times(1);
 
         let cardano_transaction_signable_builder = CardanoTransactionsSignableBuilder::new(
             Arc::new(DumbTransactionParser::new(transactions.to_vec())),
             Arc::new(mock_transaction_store),
-            Arc::new(mock_transaction_retriever),
             Path::new("/tmp"),
             Logger::root(slog::Discard, slog::o!()),
         );

mithril-common/src/signable_builder/cardano_transactions.rs

@@ -30,19 +30,10 @@ pub trait TransactionStore: Send + Sync {
     async fn store_transactions(&self, transactions: &[CardanoTransaction]) -> StdResult<()>;
 }
 
-/// Cardano transactions retriever
-#[cfg_attr(test, automock)]
-#[async_trait]
-pub trait TransactionRetriever: Sync + Send {
-    /// Get transactions up to given beacon using chronological order
-    async fn get_up_to(&self, beacon: &Beacon) -> StdResult<Vec<CardanoTransaction>>;
-}
-
 /// A [CardanoTransactionsSignableBuilder] builder
 pub struct CardanoTransactionsSignableBuilder {
     transaction_parser: Arc<dyn TransactionParser>,
     transaction_store: Arc<dyn TransactionStore>,
-    transaction_retriever: Arc<dyn TransactionRetriever>,
     logger: Logger,
     dirpath: PathBuf,
 }
@@ -52,14 +43,12 @@ impl CardanoTransactionsSignableBuilder {
     pub fn new(
         transaction_parser: Arc<dyn TransactionParser>,
         transaction_store: Arc<dyn TransactionStore>,
-        transaction_retriever: Arc<dyn TransactionRetriever>,
         dirpath: &Path,
         logger: Logger,
     ) -> Self {
         Self {
             transaction_parser,
             transaction_store,
-            transaction_retriever,
             logger,
             dirpath: dirpath.to_owned(),
         }
@@ -134,9 +123,6 @@ impl SignableBuilder<Beacon> for CardanoTransactionsSignableBuilder {
                 .await?;
         }
 
-        // We temporarily retrieve all the transactions from the database after we have recorded them.
-        // This avoids order side effects.
-        let transactions = self.transaction_retriever.get_up_to(&beacon).await?;
         let mk_root = self.compute_merkle_root(&transactions)?;
 
         let mut protocol_message = ProtocolMessage::new();
@@ -156,7 +142,7 @@ impl SignableBuilder<Beacon> for CardanoTransactionsSignableBuilder {
 #[cfg(test)]
 mod tests {
     use crate::cardano_transaction_parser::DumbTransactionParser;
-    use crate::signable_builder::{MockTransactionRetriever, MockTransactionStore};
+    use crate::signable_builder::MockTransactionStore;
 
     use super::*;
     use slog::Drain;
@@ -185,7 +171,6 @@ mod tests {
                 transactions_set_reference.clone(),
             )),
             Arc::new(MockTransactionStore::new()),
-            Arc::new(MockTransactionRetriever::new()),
             Path::new("/tmp"),
             create_logger(),
         );
@@ -246,23 +231,15 @@ mod tests {
             CardanoTransaction::new("tx-hash-456", 2, 12),
             CardanoTransaction::new("tx-hash-789", 3, 13),
         ];
-        let transactions_clone = transactions.clone();
         let transaction_parser = Arc::new(DumbTransactionParser::new(transactions.clone()));
         let mut mock_transaction_store = MockTransactionStore::new();
         mock_transaction_store
             .expect_store_transactions()
             .returning(|_| Ok(()));
         let transaction_store = Arc::new(mock_transaction_store);
-        let mut mock_transaction_retriever = MockTransactionRetriever::new();
-        mock_transaction_retriever
-            .expect_get_up_to()
-            .times(1)
-            .return_once(|_| Ok(transactions_clone));
-        let transaction_retriever = Arc::new(mock_transaction_retriever);
         let cardano_transactions_signable_builder = CardanoTransactionsSignableBuilder::new(
             transaction_parser,
             transaction_store,
-            transaction_retriever,
             Path::new("/tmp"),
             create_logger(),
         );
@@ -299,17 +276,9 @@ mod tests {
             .expect_store_transactions()
             .returning(|_| Ok(()));
         let transaction_store = Arc::new(mock_transaction_store);
-        let transactions_retrieved = transactions.clone();
-        let mut mock_transaction_retriever = MockTransactionRetriever::new();
-        mock_transaction_retriever
-            .expect_get_up_to()
-            .return_once(|_| Ok(transactions_retrieved))
-            .times(1);
-        let transaction_retriever = Arc::new(mock_transaction_retriever);
         let cardano_transactions_signable_builder = CardanoTransactionsSignableBuilder::new(
             transaction_parser,
             transaction_store,
-            transaction_retriever,
             Path::new("/tmp"),
             create_logger(),
         );