Skip to content

Conversation

@joaquim-verges
Copy link
Member

@joaquim-verges joaquim-verges commented Nov 21, 2025

Summary by CodeRabbit

  • Refactor
    • Optimized transaction execution by consolidating pending transaction retrieval with nonce allocation into a single unified operation, reducing processing overhead and improving efficiency in the transaction sending pipeline.

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

@coderabbitai
Copy link

coderabbitai bot commented Nov 21, 2025

Walkthrough

A new public async method peek_pending_transactions_with_optimistic_nonce is added to the store module to fetch pending transactions and optimistic nonce in a single operation using Redis pipelines. The worker send module is refactored to use this combined method, removing the separate retrieval of optimistic nonce and consolidating the logic.

Changes

Cohort / File(s) Change Summary
Store Module Enhancement
executors/src/eoa/store/mod.rs
Introduces new public async method peek_pending_transactions_with_optimistic_nonce that combines fetching pending transactions with queued_at data and retrieving optimistic nonce using two Redis pipelines. Handles limit=0 cases, validates optimistic nonce presence, fetches per-transaction user_request data, and cleans up missing entries from Redis.
Worker Send Refactoring
executors/src/eoa/worker/send.rs
Replaces separate calls to fetch pending transactions and retrieve optimistic nonce with single call to peek_pending_transactions_with_optimistic_nonce. Introduces optimistic_nonce variable, calculates batch_size as minimum of pending transactions and remaining budget, and uses optimistic_nonce as base for sequential nonce allocation (optimistic_nonce + index) when building new transactions.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

  • executors/src/eoa/store/mod.rs: Review the new method's Redis pipeline operations, error handling for missing optimistic nonce, and data cleanup logic for missing per-transaction entries.
  • executors/src/eoa/worker/send.rs: Verify that nonce assignment logic correctly uses optimistic_nonce as the sequential base and that batch_size calculation is sound.

Pre-merge checks and finishing touches

✅ Passed checks (3 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title 'Optimize EOA executor by combining pending transactions and nonce fetching' clearly and accurately summarizes the main change: consolidating separate fetch operations for pending transactions and nonce into a single combined operation.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch Optimize_EOA_executor_by_combining_pending_transactions_and_nonce_fetching

Tip

📝 Customizable high-level summaries are now available in beta!

You can now customize how CodeRabbit generates the high-level summary in your pull requests — including its content, structure, tone, and formatting.

  • Provide your own instructions using the high_level_summary_instructions setting.
  • Format the summary however you like (bullet lists, tables, multi-section layouts, contributor stats, etc.).
  • Use high_level_summary_in_walkthrough to move the summary from the description to the walkthrough section.

Example instruction:

"Divide the high-level summary into five sections:

  1. 📝 Description — Summarize the main change in 50–60 words, explaining what was done.
  2. 📓 References — List relevant issues, discussions, documentation, or related PRs.
  3. 📦 Dependencies & Requirements — Mention any new/updated dependencies, environment variable changes, or configuration updates.
  4. 📊 Contributor Summary — Include a Markdown table showing contributions:
    | Contributor | Lines Added | Lines Removed | Files Changed |
  5. ✔️ Additional Notes — Add any extra reviewer context.
    Keep each section concise (under 200 words) and use bullet or numbered lists for clarity."

Note: This feature is currently in beta for Pro-tier users, and pricing will be announced later.


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

Copy link
Member Author

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

@joaquim-verges joaquim-verges marked this pull request as ready for review November 21, 2025 09:37
Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

🧹 Nitpick comments (2)
executors/src/eoa/store/mod.rs (2)

637-640: Consider optimizing the limit==0 case to maintain consistency.

When limit == 0, the method calls get_optimistic_transaction_count(), which makes a separate Redis round-trip. This is inconsistent with the optimization goal of the method (combining fetches into a single operation). Consider using a pipeline even for this case to maintain consistency.

Apply this diff:

     pub async fn peek_pending_transactions_with_optimistic_nonce(
         &self,
         limit: u64,
     ) -> Result<(Vec<PendingTransaction>, u64), TransactionStoreError> {
-        if limit == 0 {
-            let optimistic = self.get_optimistic_transaction_count().await?;
-            return Ok((Vec::new(), optimistic));
-        }
-
         let pending_key = self.pending_transactions_zset_name();
         let optimistic_key = self.optimistic_transaction_count_key_name();
         let mut conn = self.redis.clone();
 
-        // First pipeline: Get transaction IDs and optimistic nonce together
-        let start = 0isize;
-        let stop = (limit - 1) as isize;
-
-        let (transaction_ids, optimistic_nonce): (
-            Vec<PendingTransactionStringWithQueuedAt>,
-            Option<u64>,
-        ) = twmq::redis::pipe()
-            .zrange_withscores(&pending_key, start, stop)
-            .get(&optimistic_key)
-            .query_async(&mut conn)
-            .await?;
+        // First pipeline: Get transaction IDs (if limit > 0) and optimistic nonce together
+        let (transaction_ids, optimistic_nonce): (
+            Vec<PendingTransactionStringWithQueuedAt>,
+            Option<u64>,
+        ) = if limit == 0 {
+            let optimistic: Option<u64> = twmq::redis::pipe()
+                .get(&optimistic_key)
+                .query_async(&mut conn)
+                .await?;
+            (Vec::new(), optimistic)
+        } else {
+            let start = 0isize;
+            let stop = (limit - 1) as isize;
+            twmq::redis::pipe()
+                .zrange_withscores(&pending_key, start, stop)
+                .get(&optimistic_key)
+                .query_async(&mut conn)
+                .await?
+        };
 
         let optimistic = optimistic_nonce.ok_or_else(|| self.nonce_sync_required_error())?;

666-701: Consider refactoring to reduce code duplication.

The logic in lines 666-701 (fetching user_request data, handling missing data, and scheduling deletions) is duplicated from peek_pending_transactions_paginated (lines 573-609). Consider extracting this common logic into a private helper method to improve maintainability.

For example, extract a helper method like:

async fn fetch_pending_transaction_data(
    &self,
    transaction_ids: Vec<PendingTransactionStringWithQueuedAt>,
) -> Result<Vec<PendingTransaction>, TransactionStoreError> {
    if transaction_ids.is_empty() {
        return Ok(Vec::new());
    }

    let mut conn = self.redis.clone();
    let mut pipe = twmq::redis::pipe();

    for (transaction_id, _) in &transaction_ids {
        let tx_data_key = self.transaction_data_key_name(transaction_id);
        pipe.hget(&tx_data_key, "user_request");
    }

    let user_requests: Vec<Option<String>> = pipe.query_async(&mut conn).await?;

    let mut pending_transactions: Vec<PendingTransaction> = Vec::new();
    let mut deletion_pipe = twmq::redis::pipe();

    for ((transaction_id, queued_at), user_request) in
        transaction_ids.into_iter().zip(user_requests)
    {
        match user_request {
            Some(user_request) => {
                let user_request_parsed = serde_json::from_str(&user_request)?;
                pending_transactions.push(PendingTransaction {
                    transaction_id,
                    queued_at,
                    user_request: user_request_parsed,
                });
            }
            None => {
                tracing::warn!(
                    "Transaction {} data was missing, deleting transaction from redis",
                    transaction_id
                );
                deletion_pipe.zrem(self.keys.pending_transactions_zset_name(), transaction_id);
            }
        }
    }

    if !deletion_pipe.is_empty() {
        deletion_pipe.query_async::<()>(&mut conn).await?;
    }

    Ok(pending_transactions)
}

Then use it in both methods to eliminate duplication.

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

Disabled knowledge base sources:

  • Linear integration is disabled by default for public repositories

You can enable these sources in your CodeRabbit configuration.

📥 Commits

Reviewing files that changed from the base of the PR and between a328989 and 30fd847.

📒 Files selected for processing (2)
  • executors/src/eoa/store/mod.rs (1 hunks)
  • executors/src/eoa/worker/send.rs (1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-09-20T06:58:40.230Z
Learnt from: d4mr
Repo: thirdweb-dev/engine-core PR: 48
File: executors/src/eoa/store/submitted.rs:229-230
Timestamp: 2025-09-20T06:58:40.230Z
Learning: The diff in executors/src/eoa/store/submitted.rs shows correct brace structure - the first closing brace closes the remove_transaction_from_redis_submitted_zset method and the second closing brace closes the impl CleanSubmittedTransactions block. The change only adds whitespace formatting.

Applied to files:

  • executors/src/eoa/store/mod.rs
  • executors/src/eoa/worker/send.rs
📚 Learning: 2025-07-06T15:44:13.701Z
Learnt from: d4mr
Repo: thirdweb-dev/engine-core PR: 5
File: executors/src/eoa/worker.rs:173-176
Timestamp: 2025-07-06T15:44:13.701Z
Learning: The EOA executor store uses comprehensive WATCH-based coordination where every Redis state mutation watches the lock key and validates ownership before proceeding. If the lock is lost during any operation, the transaction fails with LockLost error. This makes aggressive lock acquisition safe because only the actual lock holder can successfully perform state mutations, regardless of who claims the lock.

Applied to files:

  • executors/src/eoa/store/mod.rs
🧬 Code graph analysis (2)
executors/src/eoa/store/mod.rs (1)
executors/src/eoa/store/hydrate.rs (4)
  • deletion_pipe (113-114)
  • serde_json (92-92)
  • serde_json (132-132)
  • serde_json (159-159)
executors/src/eoa/worker/send.rs (2)
executors/src/metrics.rs (2)
  • current_timestamp_ms (287-289)
  • calculate_duration_seconds (273-275)
executors/src/eoa/store/atomic.rs (3)
  • eoa (84-86)
  • chain_id (89-91)
  • worker_id (94-96)
🔇 Additional comments (2)
executors/src/eoa/store/mod.rs (1)

631-704: LGTM! Effective optimization to reduce Redis round-trips.

The new method successfully combines fetching pending transactions and the optimistic nonce into a single operation, reducing Redis round-trips from 2 to 1. The implementation correctly:

  • Uses Redis pipelines for efficient batching
  • Validates optimistic nonce presence
  • Handles missing transaction data by scheduling deletions
  • Returns both results together

This optimization should improve performance in the send flow.

executors/src/eoa/worker/send.rs (1)

402-421: LGTM! Clean integration of the optimized method.

The changes properly integrate the new peek_pending_transactions_with_optimistic_nonce method to:

  • Eliminate a separate Redis call for fetching the optimistic nonce
  • Calculate batch_size explicitly for clearer logic
  • Update logging to reflect the combined operation

The nonce handling is correct, using optimistic_nonce + i for sequential nonce allocation (line 442).

@joaquim-verges joaquim-verges merged commit 5c473ef into main Nov 21, 2025
4 checks passed
@joaquim-verges joaquim-verges deleted the Optimize_EOA_executor_by_combining_pending_transactions_and_nonce_fetching branch November 21, 2025 09:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants