Skip to content

Commit effba68

Browse files
snawazsnawaz
committed
Address @taco-paco's feedback
1 parent a030b7e commit effba68

File tree

11 files changed

+108
-75
lines changed

11 files changed

+108
-75
lines changed

‎Cargo.lock‎

Lines changed: 6 additions & 6 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

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

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -344,6 +344,8 @@ mod tests {
344344

345345
use async_trait::async_trait;
346346
use magicblock_program::magic_scheduled_base_intent::ScheduledBaseIntent;
347+
use magicblock_rpc_client::MagicBlockRpcClientResult;
348+
use solana_account::Account;
347349
use solana_pubkey::{pubkey, Pubkey};
348350
use solana_sdk::{signature::Signature, signer::SignerError};
349351
use tokio::{sync::mpsc, time::sleep};
@@ -798,5 +800,12 @@ mod tests {
798800
}
799801

800802
fn reset(&self, _: ResetType) {}
803+
804+
async fn get_base_account(
805+
&self,
806+
_pubkey: &Pubkey,
807+
) -> MagicBlockRpcClientResult<Option<Account>> {
808+
Ok(None) // AccountNotFound
809+
}
801810
}
802811
}

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

Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,12 @@ pub mod single_stage_executor;
44
pub mod task_info_fetcher;
55
pub mod two_stage_executor;
66

7+
#[cfg(any(test, feature = "dev-context-only-utils"))]
8+
mod null_task_info_fetcher;
9+
10+
#[cfg(any(test, feature = "dev-context-only-utils"))]
11+
pub use null_task_info_fetcher::*;
12+
713
use std::{ops::ControlFlow, sync::Arc, time::Duration};
814

915
use async_trait::async_trait;
@@ -778,6 +784,8 @@ where
778784
mod tests {
779785
use std::{collections::HashMap, sync::Arc};
780786

787+
use magicblock_rpc_client::MagicBlockRpcClientResult;
788+
use solana_account::Account;
781789
use solana_pubkey::Pubkey;
782790

783791
use crate::{
@@ -815,6 +823,13 @@ mod tests {
815823
}
816824

817825
fn reset(&self, _: ResetType) {}
826+
827+
async fn get_base_account(
828+
&self,
829+
_pubkey: &Pubkey,
830+
) -> MagicBlockRpcClientResult<Option<Account>> {
831+
Ok(None) // AccountNotFound
832+
}
818833
}
819834

820835
#[tokio::test]

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

Lines changed: 42 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,42 @@
1+
use std::collections::HashMap;
2+
3+
use async_trait::async_trait;
4+
use magicblock_rpc_client::MagicBlockRpcClientResult;
5+
use solana_account::Account;
6+
use solana_pubkey::Pubkey;
7+
8+
use super::task_info_fetcher::{
9+
ResetType, TaskInfoFetcher, TaskInfoFetcherResult,
10+
};
11+
12+
pub struct NullTaskInfoFetcher;
13+
14+
#[async_trait]
15+
impl TaskInfoFetcher for NullTaskInfoFetcher {
16+
async fn fetch_next_commit_ids(
17+
&self,
18+
_pubkeys: &[Pubkey],
19+
) -> TaskInfoFetcherResult<HashMap<Pubkey, u64>> {
20+
Ok(Default::default())
21+
}
22+
23+
async fn fetch_rent_reimbursements(
24+
&self,
25+
_pubkeys: &[Pubkey],
26+
) -> TaskInfoFetcherResult<Vec<Pubkey>> {
27+
Ok(Default::default())
28+
}
29+
30+
fn peek_commit_id(&self, _pubkey: &Pubkey) -> Option<u64> {
31+
None
32+
}
33+
34+
fn reset(&self, _: ResetType) {}
35+
36+
async fn get_base_account(
37+
&self,
38+
_pubkey: &Pubkey,
39+
) -> MagicBlockRpcClientResult<Option<Account>> {
40+
Ok(None) // AccountNotFound
41+
}
42+
}

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

Lines changed: 1 addition & 37 deletions
Original file line numberDiff line numberDiff line change
@@ -43,9 +43,7 @@ pub trait TaskInfoFetcher: Send + Sync + 'static {
4343
async fn get_base_account(
4444
&self,
4545
_pubkey: &Pubkey,
46-
) -> MagicBlockRpcClientResult<Option<Account>> {
47-
Ok(None) // AccountNotFound
48-
}
46+
) -> MagicBlockRpcClientResult<Option<Account>>;
4947
}
5048

5149
pub enum ResetType<'a> {
@@ -294,37 +292,3 @@ pub enum TaskInfoFetcherError {
294292
}
295293

296294
pub type TaskInfoFetcherResult<T, E = TaskInfoFetcherError> = Result<T, E>;
297-
298-
#[cfg(any(test, feature = "dev-context-only-utils"))]
299-
pub struct NullTaskInfoFetcher;
300-
301-
#[cfg(any(test, feature = "dev-context-only-utils"))]
302-
#[async_trait]
303-
impl TaskInfoFetcher for NullTaskInfoFetcher {
304-
async fn fetch_next_commit_ids(
305-
&self,
306-
_pubkeys: &[Pubkey],
307-
) -> TaskInfoFetcherResult<HashMap<Pubkey, u64>> {
308-
Ok(Default::default())
309-
}
310-
311-
async fn fetch_rent_reimbursements(
312-
&self,
313-
_pubkeys: &[Pubkey],
314-
) -> TaskInfoFetcherResult<Vec<Pubkey>> {
315-
Ok(Default::default())
316-
}
317-
318-
fn peek_commit_id(&self, _pubkey: &Pubkey) -> Option<u64> {
319-
None
320-
}
321-
322-
fn reset(&self, _: ResetType) {}
323-
324-
async fn get_base_account(
325-
&self,
326-
_pubkey: &Pubkey,
327-
) -> MagicBlockRpcClientResult<Option<Account>> {
328-
Ok(None) // AccountNotFound
329-
}
330-
}

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

Lines changed: 22 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -109,30 +109,23 @@ pub trait BaseTask: Send + Sync + DynClone {
109109

110110
dyn_clone::clone_trait_object!(BaseTask);
111111

112-
#[derive(Clone)]
113-
pub struct CommitTask {
114-
pub commit_id: u64,
115-
pub allow_undelegation: bool,
116-
pub committed_account: CommittedAccount,
117-
base_account: Option<Account>,
118-
force_commit_state: bool,
119-
}
112+
pub struct CommitTaskBuilder;
120113

121-
impl CommitTask {
114+
impl CommitTaskBuilder {
122115
// Accounts larger than COMMIT_STATE_SIZE_THRESHOLD, use CommitDiff to
123116
// reduce instruction size. Below this, commit is sent as CommitState.
124117
// Chose 256 as thresold seems good enough as it could hold 8 u32 fields
125118
// or 4 u64 fields.
126119
const COMMIT_STATE_SIZE_THRESHOLD: usize = 256;
127120

128-
pub async fn new<C: TaskInfoFetcher>(
121+
pub async fn create_commit_task<C: TaskInfoFetcher>(
129122
commit_id: u64,
130123
allow_undelegation: bool,
131124
committed_account: CommittedAccount,
132125
task_info_fetcher: &Arc<C>,
133-
) -> Self {
126+
) -> CommitTask {
134127
let base_account = if committed_account.account.data.len()
135-
> CommitTask::COMMIT_STATE_SIZE_THRESHOLD
128+
> CommitTaskBuilder::COMMIT_STATE_SIZE_THRESHOLD
136129
{
137130
match task_info_fetcher
138131
.get_base_account(&committed_account.pubkey)
@@ -154,19 +147,30 @@ impl CommitTask {
154147
None
155148
};
156149

157-
Self {
150+
CommitTask {
158151
commit_id,
159152
allow_undelegation,
160153
committed_account,
161154
base_account,
162155
force_commit_state: false,
163156
}
164157
}
158+
}
165159

160+
#[derive(Clone)]
161+
pub struct CommitTask {
162+
pub commit_id: u64,
163+
pub allow_undelegation: bool,
164+
pub committed_account: CommittedAccount,
165+
base_account: Option<Account>,
166+
force_commit_state: bool,
167+
}
168+
169+
impl CommitTask {
166170
pub fn is_commit_diff(&self) -> bool {
167171
!self.force_commit_state
168172
&& self.committed_account.account.data.len()
169-
> CommitTask::COMMIT_STATE_SIZE_THRESHOLD
173+
> CommitTaskBuilder::COMMIT_STATE_SIZE_THRESHOLD
170174
&& self.base_account.is_some()
171175
}
172176

@@ -419,7 +423,7 @@ mod serialization_safety_test {
419423
use solana_account::Account;
420424

421425
use crate::{
422-
intent_executor::task_info_fetcher::NullTaskInfoFetcher,
426+
intent_executor::NullTaskInfoFetcher,
423427
tasks::{
424428
args_task::{ArgsTask, ArgsTaskType},
425429
buffer_task::{BufferTask, BufferTaskType},
@@ -434,7 +438,7 @@ mod serialization_safety_test {
434438

435439
// Test Commit variant
436440
let commit_task: ArgsTask = ArgsTaskType::Commit(
437-
CommitTask::new(
441+
CommitTaskBuilder::create_commit_task(
438442
123,
439443
true,
440444
CommittedAccount {
@@ -498,7 +502,7 @@ mod serialization_safety_test {
498502

499503
let buffer_task =
500504
BufferTask::new_preparation_required(BufferTaskType::Commit(
501-
CommitTask::new(
505+
CommitTaskBuilder::create_commit_task(
502506
456,
503507
false,
504508
CommittedAccount {
@@ -526,7 +530,7 @@ mod serialization_safety_test {
526530
// Test BufferTask preparation
527531
let buffer_task =
528532
BufferTask::new_preparation_required(BufferTaskType::Commit(
529-
CommitTask::new(
533+
CommitTaskBuilder::create_commit_task(
530534
789,
531535
true,
532536
CommittedAccount {

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

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -16,7 +16,8 @@ use crate::{
1616
persist::IntentPersister,
1717
tasks::{
1818
args_task::{ArgsTask, ArgsTaskType},
19-
BaseActionTask, BaseTask, CommitTask, FinalizeTask, UndelegateTask,
19+
BaseActionTask, BaseTask, CommitTaskBuilder, FinalizeTask,
20+
UndelegateTask,
2021
},
2122
};
2223

@@ -90,7 +91,7 @@ impl TasksBuilder for TaskBuilderImpl {
9091
.iter()
9192
.map(|account| async {
9293
let commit_id = *commit_ids.get(&account.pubkey).expect("CommitIdFetcher provide commit ids for all listed pubkeys, or errors!");
93-
let task = ArgsTaskType::Commit(CommitTask::new(
94+
let task = ArgsTaskType::Commit(CommitTaskBuilder::create_commit_task(
9495
commit_id,
9596
allow_undelegation,
9697
account.clone(),

‎magicblock-committor-service/src/tasks/task_strategist.rs‎

Lines changed: 5 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -261,9 +261,11 @@ mod tests {
261261

262262
use super::*;
263263
use crate::{
264-
intent_executor::task_info_fetcher::NullTaskInfoFetcher,
264+
intent_executor::NullTaskInfoFetcher,
265265
persist::IntentPersisterImpl,
266-
tasks::{BaseActionTask, CommitTask, TaskStrategy, UndelegateTask},
266+
tasks::{
267+
BaseActionTask, CommitTaskBuilder, TaskStrategy, UndelegateTask,
268+
},
267269
};
268270

269271
// Helper to create a simple commit task
@@ -272,7 +274,7 @@ mod tests {
272274
data_size: usize,
273275
) -> ArgsTask {
274276
ArgsTask::new(ArgsTaskType::Commit(
275-
CommitTask::new(
277+
CommitTaskBuilder::create_commit_task(
276278
commit_id,
277279
false,
278280
CommittedAccount {

‎test-integration/test-committor-service/tests/common.rs‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -162,7 +162,7 @@ pub fn generate_random_bytes(length: usize) -> Vec<u8> {
162162
#[allow(dead_code)]
163163
pub async fn create_commit_task(data: &[u8]) -> CommitTask {
164164
static COMMIT_ID: AtomicU64 = AtomicU64::new(0);
165-
CommitTask::new(
165+
CommitTaskBuilder::create_commit_task(
166166
COMMIT_ID.fetch_add(1, Ordering::Relaxed),
167167
false,
168168
CommittedAccount {

‎test-integration/test-committor-service/tests/test_transaction_preparator.rs‎

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -36,7 +36,7 @@ async fn test_prepare_commit_tx_with_single_account() {
3636

3737
let tasks = vec![
3838
Box::new(ArgsTask::new(ArgsTaskType::Commit(
39-
CommitTask::new(
39+
CommitTaskBuilder::create_commit_task(
4040
1,
4141
true,
4242
committed_account.clone(),
@@ -95,7 +95,7 @@ async fn test_prepare_commit_tx_with_multiple_accounts() {
9595

9696
let buffer_commit_task =
9797
BufferTask::new_preparation_required(BufferTaskType::Commit(
98-
CommitTask::new(
98+
CommitTaskBuilder::create_commit_task(
9999
1,
100100
true,
101101
committed_account2.clone(),
@@ -107,7 +107,7 @@ async fn test_prepare_commit_tx_with_multiple_accounts() {
107107
let tasks = vec![
108108
// account 1
109109
Box::new(ArgsTask::new(ArgsTaskType::Commit(
110-
CommitTask::new(
110+
CommitTaskBuilder::create_commit_task(
111111
1,
112112
true,
113113
committed_account1.clone(),
@@ -199,7 +199,7 @@ async fn test_prepare_commit_tx_with_base_actions() {
199199

200200
let buffer_commit_task =
201201
BufferTask::new_preparation_required(BufferTaskType::Commit(
202-
CommitTask::new(
202+
CommitTaskBuilder::create_commit_task(
203203
1,
204204
true,
205205
committed_account.clone(),

0 commit comments

Comments
 (0)