Address @taco-paco's feedback

Author: snawaz

Cargo.lock

@@ -29,7 +29,6 @@ magicblock-metrics = { workspace = true }
 magicblock-program = { workspace = true }
 magicblock-rpc-client = { workspace = true }
 magicblock-table-mania = { workspace = true }
-magicblock-config = { workspace = true }
 rusqlite = { workspace = true }
 solana-account = { workspace = true }
 solana-address-lookup-table-interface = { workspace = true }

magicblock-committor-service/Cargo.toml

@@ -371,6 +371,8 @@ mod tests {
 
     use async_trait::async_trait;
     use magicblock_program::magic_scheduled_base_intent::ScheduledBaseIntent;
+    use magicblock_rpc_client::MagicBlockRpcClientResult;
+    use solana_account::Account;
     use solana_pubkey::{pubkey, Pubkey};
     use solana_signature::Signature;
     use solana_signer::SignerError;
@@ -821,5 +823,25 @@ mod tests {
         async fn cleanup(self) -> Result<(), BufferExecutionError> {
             Ok(())
         }
+
+        async fn fetch_rent_reimbursements(
+            &self,
+            pubkeys: &[Pubkey],
+        ) -> TaskInfoFetcherResult<Vec<Pubkey>> {
+            Ok(pubkeys.iter().map(|_| Pubkey::new_unique()).collect())
+        }
+
+        fn peek_commit_id(&self, _pubkey: &Pubkey) -> Option<u64> {
+            None
+        }
+
+        fn reset(&self, _: ResetType) {}
+
+        async fn get_base_account(
+            &self,
+            _pubkey: &Pubkey,
+        ) -> MagicBlockRpcClientResult<Option<Account>> {
+            Ok(None) // AccountNotFound
+        }
     }
 }

magicblock-committor-service/src/intent_execution_manager/intent_execution_engine.rs

@@ -6,6 +6,9 @@ pub mod two_stage_executor;
 
 use std::{mem, ops::ControlFlow, sync::Arc, time::Duration};
 
+#[cfg(any(test, feature = "dev-context-only-utils"))]
+mod null_task_info_fetcher;
+
 use async_trait::async_trait;
 use futures_util::future::{join, try_join_all};
 use log::{trace, warn};
@@ -22,8 +25,13 @@ use magicblock_rpc_client::{
     MagicBlockRpcClientError, MagicBlockSendTransactionConfig,
     MagicBlockSendTransactionOutcome, MagicblockRpcClient,
 };
+
 use solana_keypair::Keypair;
 use solana_message::VersionedMessage;
+
+#[cfg(any(test, feature = "dev-context-only-utils"))]
+pub use null_task_info_fetcher::*;
+
 use solana_pubkey::Pubkey;
 use solana_rpc_client_api::config::RpcTransactionConfig;
 use solana_signature::Signature;
@@ -807,3 +815,88 @@ where
         try_join_all(cleanup_futs).await.map(|_| ())
     }
 }
+
+#[cfg(test)]
+mod tests {
+    use std::{collections::HashMap, sync::Arc};
+
+    use magicblock_rpc_client::MagicBlockRpcClientResult;
+    use solana_account::Account;
+    use solana_pubkey::Pubkey;
+
+    use crate::{
+        intent_execution_manager::intent_scheduler::create_test_intent,
+        intent_executor::{
+            task_info_fetcher::{
+                ResetType, TaskInfoFetcher, TaskInfoFetcherResult,
+            },
+            IntentExecutorImpl,
+        },
+        persist::IntentPersisterImpl,
+        tasks::task_builder::{TaskBuilderImpl, TasksBuilder},
+        transaction_preparator::TransactionPreparatorImpl,
+    };
+
+    struct MockInfoFetcher;
+    #[async_trait::async_trait]
+    impl TaskInfoFetcher for MockInfoFetcher {
+        async fn fetch_next_commit_ids(
+            &self,
+            pubkeys: &[Pubkey],
+        ) -> TaskInfoFetcherResult<HashMap<Pubkey, u64>> {
+            Ok(pubkeys.iter().map(|pubkey| (*pubkey, 0)).collect())
+        }
+
+        async fn fetch_rent_reimbursements(
+            &self,
+            pubkeys: &[Pubkey],
+        ) -> TaskInfoFetcherResult<Vec<Pubkey>> {
+            Ok(pubkeys.iter().map(|_| Pubkey::new_unique()).collect())
+        }
+
+        fn peek_commit_id(&self, _pubkey: &Pubkey) -> Option<u64> {
+            Some(0)
+        }
+
+        fn reset(&self, _: ResetType) {}
+
+        async fn get_base_account(
+            &self,
+            _pubkey: &Pubkey,
+        ) -> MagicBlockRpcClientResult<Option<Account>> {
+            Ok(None) // AccountNotFound
+        }
+    }
+
+    #[tokio::test]
+    async fn test_try_unite() {
+        let pubkey = [Pubkey::new_unique()];
+        let intent = create_test_intent(0, &pubkey);
+
+        let info_fetcher = Arc::new(MockInfoFetcher);
+        let commit_task = TaskBuilderImpl::commit_tasks(
+            &info_fetcher,
+            &intent,
+            &None::<IntentPersisterImpl>,
+        )
+        .await
+        .unwrap();
+        let finalize_task =
+            TaskBuilderImpl::finalize_tasks(&info_fetcher, &intent)
+                .await
+                .unwrap();
+
+        let result = IntentExecutorImpl::<
+            TransactionPreparatorImpl,
+            MockInfoFetcher,
+        >::try_unite_tasks(
+            &commit_task,
+            &finalize_task,
+            &Pubkey::new_unique(),
+            &None::<IntentPersisterImpl>,
+        );
+
+        let strategy = result.unwrap().unwrap();
+        assert!(strategy.lookup_tables_keys.is_empty());
+    }
+}

magicblock-committor-service/src/intent_executor/mod.rs

@@ -0,0 +1,42 @@
+use std::collections::HashMap;
+
+use async_trait::async_trait;
+use magicblock_rpc_client::MagicBlockRpcClientResult;
+use solana_account::Account;
+use solana_pubkey::Pubkey;
+
+use super::task_info_fetcher::{
+    ResetType, TaskInfoFetcher, TaskInfoFetcherResult,
+};
+
+pub struct NullTaskInfoFetcher;
+
+#[async_trait]
+impl TaskInfoFetcher for NullTaskInfoFetcher {
+    async fn fetch_next_commit_ids(
+        &self,
+        _pubkeys: &[Pubkey],
+    ) -> TaskInfoFetcherResult<HashMap<Pubkey, u64>> {
+        Ok(Default::default())
+    }
+
+    async fn fetch_rent_reimbursements(
+        &self,
+        _pubkeys: &[Pubkey],
+    ) -> TaskInfoFetcherResult<Vec<Pubkey>> {
+        Ok(Default::default())
+    }
+
+    fn peek_commit_id(&self, _pubkey: &Pubkey) -> Option<u64> {
+        None
+    }
+
+    fn reset(&self, _: ResetType) {}
+
+    async fn get_base_account(
+        &self,
+        _pubkey: &Pubkey,
+    ) -> MagicBlockRpcClientResult<Option<Account>> {
+        Ok(None) // AccountNotFound
+    }
+}

magicblock-committor-service/src/intent_executor/null_task_info_fetcher.rs

@@ -43,9 +43,7 @@ pub trait TaskInfoFetcher: Send + Sync + 'static {
     async fn get_base_account(
         &self,
         _pubkey: &Pubkey,
-    ) -> MagicBlockRpcClientResult<Option<Account>> {
-        Ok(None) // AccountNotFound
-    }
+    ) -> MagicBlockRpcClientResult<Option<Account>>;
 }
 
 pub enum ResetType<'a> {
@@ -309,37 +307,3 @@ impl TaskInfoFetcherError {
 }
 
 pub type TaskInfoFetcherResult<T, E = TaskInfoFetcherError> = Result<T, E>;
-
-#[cfg(any(test, feature = "dev-context-only-utils"))]
-pub struct NullTaskInfoFetcher;
-
-#[cfg(any(test, feature = "dev-context-only-utils"))]
-#[async_trait]
-impl TaskInfoFetcher for NullTaskInfoFetcher {
-    async fn fetch_next_commit_ids(
-        &self,
-        _pubkeys: &[Pubkey],
-    ) -> TaskInfoFetcherResult<HashMap<Pubkey, u64>> {
-        Ok(Default::default())
-    }
-
-    async fn fetch_rent_reimbursements(
-        &self,
-        _pubkeys: &[Pubkey],
-    ) -> TaskInfoFetcherResult<Vec<Pubkey>> {
-        Ok(Default::default())
-    }
-
-    fn peek_commit_id(&self, _pubkey: &Pubkey) -> Option<u64> {
-        None
-    }
-
-    fn reset(&self, _: ResetType) {}
-
-    async fn get_base_account(
-        &self,
-        _pubkey: &Pubkey,
-    ) -> MagicBlockRpcClientResult<Option<Account>> {
-        Ok(None) // AccountNotFound
-    }
-}

magicblock-committor-service/src/intent_executor/task_info_fetcher.rs

@@ -110,30 +110,23 @@ pub trait BaseTask: Send + Sync + DynClone + LabelValue {
 
 dyn_clone::clone_trait_object!(BaseTask);
 
-#[derive(Clone)]
-pub struct CommitTask {
-    pub commit_id: u64,
-    pub allow_undelegation: bool,
-    pub committed_account: CommittedAccount,
-    base_account: Option<Account>,
-    force_commit_state: bool,
-}
+pub struct CommitTaskBuilder;
 
-impl CommitTask {
+impl CommitTaskBuilder {
     // Accounts larger than COMMIT_STATE_SIZE_THRESHOLD, use CommitDiff to
     // reduce instruction size. Below this, commit is sent as CommitState.
     // Chose 256 as thresold seems good enough as it could hold 8 u32 fields
     // or 4 u64 fields.
     const COMMIT_STATE_SIZE_THRESHOLD: usize = 256;
 
-    pub async fn new<C: TaskInfoFetcher>(
+    pub async fn create_commit_task<C: TaskInfoFetcher>(
         commit_id: u64,
         allow_undelegation: bool,
         committed_account: CommittedAccount,
         task_info_fetcher: &Arc<C>,
-    ) -> Self {
+    ) -> CommitTask {
         let base_account = if committed_account.account.data.len()
-            > CommitTask::COMMIT_STATE_SIZE_THRESHOLD
+            > CommitTaskBuilder::COMMIT_STATE_SIZE_THRESHOLD
         {
             match task_info_fetcher
                 .get_base_account(&committed_account.pubkey)
@@ -155,19 +148,30 @@ impl CommitTask {
             None
         };
 
-        Self {
+        CommitTask {
             commit_id,
             allow_undelegation,
             committed_account,
             base_account,
             force_commit_state: false,
         }
     }
+}
 
+#[derive(Clone)]
+pub struct CommitTask {
+    pub commit_id: u64,
+    pub allow_undelegation: bool,
+    pub committed_account: CommittedAccount,
+    base_account: Option<Account>,
+    force_commit_state: bool,
+}
+
+impl CommitTask {
     pub fn is_commit_diff(&self) -> bool {
         !self.force_commit_state
             && self.committed_account.account.data.len()
-                > CommitTask::COMMIT_STATE_SIZE_THRESHOLD
+                > CommitTaskBuilder::COMMIT_STATE_SIZE_THRESHOLD
             && self.base_account.is_some()
     }
 
@@ -420,7 +424,7 @@ mod serialization_safety_test {
     use solana_account::Account;
 
     use crate::{
-        intent_executor::task_info_fetcher::NullTaskInfoFetcher,
+        intent_executor::NullTaskInfoFetcher,
         tasks::{
             args_task::{ArgsTask, ArgsTaskType},
             buffer_task::{BufferTask, BufferTaskType},
@@ -435,7 +439,7 @@ mod serialization_safety_test {
 
         // Test Commit variant
         let commit_task: ArgsTask = ArgsTaskType::Commit(
-            CommitTask::new(
+            CommitTaskBuilder::create_commit_task(
                 123,
                 true,
                 CommittedAccount {
@@ -499,7 +503,7 @@ mod serialization_safety_test {
 
         let buffer_task =
             BufferTask::new_preparation_required(BufferTaskType::Commit(
-                CommitTask::new(
+                CommitTaskBuilder::create_commit_task(
                     456,
                     false,
                     CommittedAccount {
@@ -527,7 +531,7 @@ mod serialization_safety_test {
         // Test BufferTask preparation
         let buffer_task =
             BufferTask::new_preparation_required(BufferTaskType::Commit(
-                CommitTask::new(
+                CommitTaskBuilder::create_commit_task(
                     789,
                     true,
                     CommittedAccount {

magicblock-committor-service/src/tasks/mod.rs

@@ -17,7 +17,8 @@ use crate::{
     persist::IntentPersister,
     tasks::{
         args_task::{ArgsTask, ArgsTaskType},
-        BaseActionTask, BaseTask, CommitTask, FinalizeTask, UndelegateTask,
+        BaseActionTask, BaseTask, CommitTaskBuilder, FinalizeTask,
+        UndelegateTask,
     },
 };
 
@@ -91,7 +92,7 @@ impl TasksBuilder for TaskBuilderImpl {
             .iter()
             .map(|account| async {
                 let commit_id = *commit_ids.get(&account.pubkey).expect("CommitIdFetcher provide commit ids for all listed pubkeys, or errors!");
-                let task = ArgsTaskType::Commit(CommitTask::new(
+                let task = ArgsTaskType::Commit(CommitTaskBuilder::create_commit_task(
                     commit_id,
                     allow_undelegation,
                     account.clone(),

magicblock-committor-service/src/tasks/task_builder.rs

@@ -387,6 +387,8 @@ mod tests {
         },
         persist::IntentPersisterImpl,
         tasks::{
+            intent_executor::NullTaskInfoFetcher,
+            persist::IntentPersisterImpl,
             task_builder::{TaskBuilderImpl, TasksBuilder},
             BaseActionTask, CommitTask, TaskStrategy, UndelegateTask,
         },
@@ -422,7 +424,7 @@ mod tests {
         data_size: usize,
     ) -> ArgsTask {
         ArgsTask::new(ArgsTaskType::Commit(
-            CommitTask::new(
+            CommitTaskBuilder::create_commit_task(
                 commit_id,
                 false,
                 CommittedAccount {