feat: Make CommitStrategy more granular

[snawaz]

Summary by CodeRabbit

• Refactor

  • Renamed commit strategy variant names for improved consistency and clarity across the codebase.

• Tests

  • Updated test cases to reflect updated commit strategy variant naming.

_{✏️ Tip: You can customize this high-level summary in your review settings.}

[coderabbitai]

📝 Walkthrough

Walkthrough

This pull request renames all variants of the CommitStrategy enum across the codebase. The variants are renamed as follows: Args to StateArgs, ArgsWithLookupTable to StateArgsWithLookupTable, FromBuffer to StateBuffer, and FromBufferWithLookupTable to StateBufferWithLookupTable. Corresponding string mappings in the as_str() method and TryFrom<&str> implementations are updated to reflect the new names. All references to these variants in tests, visitors, and database code are updated to use the new naming convention. No changes to method signatures, control flow, or error handling are introduced.

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[snawaz]

Warning

This pull request is not mergeable via GitHub because a downstack PR is open. Once all requirements are satisfied, merge this PR as a stack on Graphite.
Learn more

• feat: Make CommitStrategy more granular #618 👈 (View in Graphite)
• refactor: Rename optimize() -> try_optimize_tx_size() #617
• feat: Execute CommitDiff as BufferTask #616 : 1 other dependent PR (#665 )
• feat: Add support for CommitDiff to efficiently commit small changes in large delegated accounts #575
• master

This stack of pull requests is managed by Graphite. Learn more about stacking.

[coderabbitai]

Actionable comments posted: 1

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (2)

test-integration/test-committor-service/tests/test_ix_commit_local.rs (1)

108-121: Critical: Update missed references to old variant name.

Lines 114 and 119 still reference CommitStrategy::FromBuffer, which no longer exists after the rename to CommitStrategy::StateBuffer. This will cause compilation errors.

🔎 Proposed fix

 #[tokio::test]
 async fn test_ix_commit_order_book_change_680_bytes() {
     // We cannot use 680 as changed_len because that both 679 and 680 produce encoded tx
     // of size 1644 (which is the max limit), but while the size of raw bytes for 679 is within
     // 1232 limit, the size for 680 execeds by 1 (1233). That is why we used
-    // 681 as changed_len where CommitStrategy goes from Args to FromBuffer.
-    commit_book_order_account(681, CommitStrategy::FromBuffer, false).await;
+    // 681 as changed_len where CommitStrategy goes from Args to StateBuffer.
+    commit_book_order_account(681, CommitStrategy::StateBuffer, false).await;
 }
 
 #[tokio::test]
 async fn test_ix_commit_order_book_change_10k_bytes() {
-    commit_book_order_account(10 * 1024, CommitStrategy::FromBuffer, false)
+    commit_book_order_account(10 * 1024, CommitStrategy::StateBuffer, false)
         .await;
 }

magicblock-committor-service/src/persist/types/commit_strategy.rs (1)

44-59: Critical: Update TryFrom to match corrected string representations.

The TryFrom<&str> implementation must be updated to match the corrected string representations from as_str() (after fixing the "Stage"/"State" typos).

🔎 Proposed fix

 impl TryFrom<&str> for CommitStrategy {
     type Error = CommitPersistError;
     fn try_from(value: &str) -> Result<Self, CommitPersistError> {
         match value {
             "StateArgs" => Ok(Self::StateArgs),
-            "StateArgsWithLookupTable" => Ok(Self::StateArgsWithLookupTable),
-            "StageBuffer" => Ok(Self::StateBuffer),
-            "StageBufferWithLookupTable" => {
+            "StateArgsWithLookupTable" => Ok(Self::StateArgsWithLookupTable),
+            "StateBuffer" => Ok(Self::StateBuffer),
+            "StateBufferWithLookupTable" => {
                 Ok(Self::StateBufferWithLookupTable)
             }
             _ => Err(CommitPersistError::InvalidCommitStrategy(
                 value.to_string(),
             )),
         }
     }
 }

📜 Review details

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 99dc29b and 2ba11d8.

📒 Files selected for processing (5)

• magicblock-committor-service/src/persist/commit_persister.rs
• magicblock-committor-service/src/persist/db.rs
• magicblock-committor-service/src/persist/types/commit_strategy.rs
• magicblock-committor-service/src/tasks/task_visitors/persistor_visitor.rs
• test-integration/test-committor-service/tests/test_ix_commit_local.rs

🧰 Additional context used

🧠 Learnings (3)

📚 Learning: 2025-11-21T10:22:07.520Z

Learnt from: taco-paco
Repo: magicblock-labs/magicblock-validator PR: 661
File: magicblock-committor-service/src/intent_executor/single_stage_executor.rs:20-28
Timestamp: 2025-11-21T10:22:07.520Z
Learning: In magicblock-committor-service's SingleStageExecutor and TwoStageExecutor (single_stage_executor.rs and two_stage_executor.rs), the fields transaction_strategy, junk, and patched_errors are intentionally public because these executors are designed to be used independently outside of the IntentExecutor scope, and callers need access to these execution reports for cleanup and error handling.

Applied to files:

• magicblock-committor-service/src/persist/commit_persister.rs
• test-integration/test-committor-service/tests/test_ix_commit_local.rs
• magicblock-committor-service/src/persist/types/commit_strategy.rs

📚 Learning: 2025-12-03T09:33:48.707Z

Learnt from: Dodecahedr0x
Repo: magicblock-labs/magicblock-validator PR: 639
File: test-integration/test-committor-service/tests/test_ix_commit_local.rs:867-881
Timestamp: 2025-12-03T09:33:48.707Z
Learning: Repo: magicblock-labs/magicblock-validator PR: 639
Context: test-integration/test-committor-service/tests/test_ix_commit_local.rs (ix_commit_local)
Learning: The PhotonIndexer used for compressed account fetches (get_compressed_account) has built‑in retry logic (defaults to ~10 attempts), so tests should not add separate retry loops around compressed fetches unless there’s a specific need.

Applied to files:

• magicblock-committor-service/src/persist/commit_persister.rs
• test-integration/test-committor-service/tests/test_ix_commit_local.rs

📚 Learning: 2025-12-03T09:36:01.527Z

Learnt from: Dodecahedr0x
Repo: magicblock-labs/magicblock-validator PR: 639
File: magicblock-chainlink/src/remote_account_provider/mod.rs:1350-1353
Timestamp: 2025-12-03T09:36:01.527Z
Learning: Repo: magicblock-labs/magicblock-validator
File: magicblock-chainlink/src/remote_account_provider/mod.rs
Context: consolidate_fetched_remote_accounts
Learning: For unexpected result counts (>2), the project prefers logging an error and returning an empty Vec over panicking; acceptable during development per maintainer (Dodecahedr0x).

Applied to files:

• test-integration/test-committor-service/tests/test_ix_commit_local.rs

🔇 Additional comments (7)

magicblock-committor-service/src/persist/db.rs (1)

775-775: LGTM!

Test data correctly updated to use the renamed CommitStrategy variants (StateArgs and StateBuffer).

Also applies to: 910-910

magicblock-committor-service/src/persist/commit_persister.rs (1)

588-588: LGTM!

Test correctly updated to use the renamed CommitStrategy::StateArgs variant.

Also applies to: 595-595

test-integration/test-committor-service/tests/test_ix_commit_local.rs (2)

70-70: LGTM!

Single account tests correctly updated to use renamed variants (StateArgs and StateBuffer).

Also applies to: 75-75, 80-80, 85-85, 90-90, 95-95, 100-100, 105-105

---

267-268: LGTM!

Multiple account test expectations correctly updated to use the renamed CommitStrategy variants (StateArgs, StateBuffer, StateArgsWithLookupTable, StateBufferWithLookupTable).

Also applies to: 279-280, 291-292, 303-304, 315-316, 324-327, 333-334, 343-348, 358-360, 371-373, 383-384, 393-395, 402-403, 412-415, 424-425, 436-437, 449-450

magicblock-committor-service/src/tasks/task_visitors/persistor_visitor.rs (2)

26-66: LGTM!

Correctly updated to use renamed variants (StateArgsWithLookupTable and StateArgs) in the args task visitor.

---

68-107: LGTM!

Correctly updated to use renamed variants (StateBufferWithLookupTable and StateBuffer) in the buffer task visitor.

magicblock-committor-service/src/persist/types/commit_strategy.rs (1)

7-14: LGTM!

Enum variants correctly renamed to State-prefixed names.

[snawaz]

changes from this PR are folded into its ancestor.